Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saved Object Management - Copy to Space #37286

Closed
kobelb opened this issue May 28, 2019 · 11 comments · Fixed by #39002
Closed

Saved Object Management - Copy to Space #37286

kobelb opened this issue May 28, 2019 · 11 comments · Fixed by #39002
Labels
enhancement New value added to drive a business result Feature:Security/Spaces Platform Security - Spaces feature Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@kobelb
Copy link
Contributor

kobelb commented May 28, 2019

Currently, to "copy" objects from one space to another we instruct users to export the saved objects from the source space, change their space to their destination space, then import the saved objects. Additionally, the user must repeat the import for all destination spaces, which can be time consuming and painful.

Now that #34896 has merged and we can more easily export a saved object and all of it's references, we should add a first-class "copy to space" option to the saved object management screen.

@kobelb kobelb added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label May 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@kobelb kobelb added Feature:Security/Spaces Platform Security - Spaces feature enhancement New value added to drive a business result labels May 28, 2019
@legrego
Copy link
Member

legrego commented May 29, 2019

High-level tasks

🔌 Design and implement extension point into Saved Objects UI

Considerations

Is this a candidate for the Embeddable/Actions API?

Should we allow for bulk operations, like Export and Delete behave today, or should this be an action specified at the grid row level? Either way, we can support copying related saved objects.

Bulk example
image

Row level example
image

💥 Design and implement "Copy to Space" action

Once the extension point is in place, then the Spaces plugin can embed its own "Copy to Space" action in the Saved Objects Management UI.

Considerations

We will need to be somewhat intelligent about how we show this to the end-user. At an absolute minimum, we should only show a list of spaces the user has access to.
We should further filter this list by hiding any spaces where the user won't be authorized to perform the copy action.

question: When determining which spaces the user is able to copy to, should we be checking for the "Saved Objects Management" feature privilege, or should we be checking that the user has privileges for the underlying saved object types? For example, if a user wants to copy a dashboard (and related SOs) from Space A to Space B, is it sufficient to have the dashboard:all privilege in Space B, or should they be required to have the "Saved Objects Management" feature privilege? I'm leaning toward the management feature privilege, but I'm open to counter arguments.

✏️ Implement Spaces API endpoint to allow objects to be copied

Considerations

This endpoint will likely be constrained to the existing SO import/export API limit of 10,000 saved objects. It's unlikely to be a problem, but the endpoint needs to account for the fact that it could happen.

This endpoint (and corresponding UI) will need to account for conflicts when copying into the target space(s).

Currently, the import/export functionality requires an instance of the Saved Objects Client to function. This client is scoped to the current request, which binds it to the current user and space. While this is fine for the export operation, it won't work for the subsequent import operation. Here are a few options I've come up with so far:

  1. Export in-memory on the client, and then fire off individual import requests into the target spaces from the client. I don't love this option as it'll potentially be memory intensive on the client, and may not scale for larger datasets. This potentially requires the least amount of effort, however.
  2. Rewrite the import/export clients to optionally accept a SavedObjectsRepository instead of a SavedObjectsClient. This will allow us to circumvent the scoped client, but it requires us to implement our own authz, and this endpoint won't be able to take advantage of any other SOC wrappers that may exist.
  3. Expose a way to create SavedObjectsClient instances that can be bound to an arbitrary namespace. This potentially involves changing the way that the secure SOC wrapper authorizes requests, which could prove time consuming.
  4. Use server.inject to mimic what's happening in option 1 above. I consider this a non-starter, as this isn't a pattern we likely want to persist in the NP.

@kobelb
Copy link
Contributor Author

kobelb commented May 29, 2019

Is this a candidate for the Embeddable/Actions API?

I'm not too familiar with the Embeddable/Actions API to comment on this.

Should we allow for bulk operations, like Export and Delete behave today, or should this be an action specified at the grid row level? Either way, we can support copying related saved objects.

Ideally, both would be possible. I'm not sure if there's a reason why we don't allow "Export" to occur on a per-row basis though, there might be a reason why not to do so. @mikecote I know you've worked on this screen somewhat recently, any insights?

I'm leaning toward the management feature privilege, but I'm open to counter arguments.

I'm rather torn on this, but I'm also leaning toward the management feature privilege as well. Especially because we're going to be using the management specific APIs to make this possible, and we could potentially restrict access to these APIs to only users with that feature. Let's start with this and relax the constraint if it is problematic/limiting.

@legrego
Copy link
Member

legrego commented May 29, 2019

@stacey-gammon would this be a candidate for the Embeddable/Actions API? We're looking for Spaces to augment the UI of the Saved Objects Management screen, in one or both of the ways outlined in the screenshots here

@stacey-gammon
Copy link
Contributor

stacey-gammon commented May 29, 2019

This is actually pretty interesting.

I was thinking the other day that we actually don't need to couple the actions API and the embeddables API. So you could do this via embeddables and actions... but do you really need the component to be an "embeddable"? Not really...

so we could actually separate the actions API from embeddables and have actions just take in any trigger context, so instead of:

class MyAction extends Action {
  execute({ embeddable, triggerContext } {...}
} 

you could do

class MyAction extends Action {
  execute(triggerContext: { } {...}
} 

and for most embeddable use cases, they would pass the embeddable as part of triggerContext

class MyAction extends Action {
  execute(triggerContext: { embeddable, anyExtraContext })  {...}
} 

Then, we just have a really generic way of having developers register triggers and actions and we don't need to add a new registry every time someone wants to expose part of their UI to be extendable via a context menu.

I wasn't really planning on thinking this through till at least post PR 1 when an actual use case for it came up... but this actually feels like a good candidate.

@mikecote
Copy link
Contributor

For the per-row export option, there isn't a reason we don't have it. The export APIs would support a single item if ever this was to be implemented.

@legrego
Copy link
Member

legrego commented May 30, 2019

@stacey-gammon thanks for your input! I won't hold you to a timeline on landing these APIs, but I'll investigate using them if they're available when we work on this. If not, we can always go back in a subsequent PR and clean it up.

@mikecote good to know, thanks!

@legrego
Copy link
Member

legrego commented Jun 3, 2019

@kobelb I updated my comment above with additional thoughts around the API implementation.

@kobelb
Copy link
Contributor Author

kobelb commented Jun 3, 2019

Export in-memory on the client, and then fire off individual import requests into the target spaces from the client. I don't love this option as it'll potentially be memory intensive on the client, and may not scale for larger datasets. This potentially requires the least amount of effort, however.

I agree that this is a less than ideal solution. However, it definitely seems the easiest. We currently limit the import payload size to 10mB, so theoretically we'd only need to consume 10mB of browser memory to facilitate this process. However, without any changes to the export/import APIs, we'd use the export size times the number of spaces of bandwidth to perform this operation. It depends on the network topology and the size of the saved objects for how large of an impact this has.

Rewrite the import/export clients to optionally accept a SavedObjectsRepository instead of a SavedObjectsClient. This will allow us to circumvent the scoped client, but it requires us to implement our own authz, and this endpoint won't be able to take advantage of any other SOC wrappers that may exist.

If I'm thinking about this properly, we should only need to do this for the import API itself. As the export API will only be dealing with a single space. The risk in not using the SOC wrappers at all as part of the import is that it'd be skipping over the encrypted saved objects wrapper. Even though on the export we're stripping the encrypted attributes, we don't want to be inserting unencrypted attributes if they do so happen to be in the import payload.

Expose a way to create SavedObjectsClient instances that can be bound to an arbitrary namespace. This potentially involves changing the way that the secure SOC wrapper authorizes requests, which could prove time consuming.

I do think we're going to need some facility in the future to let the routes themselves specify the namespace(s) directly instead of only inferring it from the route URL. If we wanted to add a "copy to namespace(s)" route, we couldn't infer both the destination and source namespaces from the basepath of the URL, as the namespaces need to be different.

Currently, the SOC wrappers ordering is Security (which uses the spaces service directly to determine the current namespace and perform the authorization) and then the Spaces wrapper (which augments the options to include the namespace). This is largely because I kept insisting that we have to perform authorization first before doing anything else, which isn't a good reason in retrospect.

If we were to change the ordering of the wrappers to be Spaces (which augments the options to include the namespace) and then Security (which can use the actual namespace argument to perform the authorization), we can then augment the spaces wrapper to allow the consumer to specify a namespace otherwise we infer one from the basepath. We'd potentially have to figure out how to make it explicitly obvious that we're specifying the default namespace, besides by omitting the namespace argument entirely, but this seems do-able.

@legrego
Copy link
Member

legrego commented Jun 4, 2019

Thanks for your detailed followup!

If we were to change the ordering of the wrappers to be Spaces (which augments the options to include the namespace) and then Security (which can use the actual namespace argument to perform the authorization), we can then augment the spaces wrapper to allow the consumer to specify a namespace otherwise we infer one from the basepath. We'd potentially have to figure out how to make it explicitly obvious that we're specifying the default namespace, besides by omitting the namespace argument entirely, but this seems do-able.

I like this approach, and it doesn't feel like it'd be all that bad to implement. I'll see what this looks like, and we can decide if we like it or not

@kobelb
Copy link
Contributor Author

kobelb commented Jun 4, 2019

If this starts to get more complicated than initially anticipated, we can potentially introduce the ability to copy an individual saved object and all of its dependencies to only one target space at a time. This would still be a lot better than what we have right now, without exacerbating the limitations of the current implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Security/Spaces Platform Security - Spaces feature Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants