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

Copy Saved Objects to Spaces API #38014

Merged
merged 84 commits into from
Aug 21, 2019
Merged

Conversation

legrego
Copy link
Member

@legrego legrego commented Jun 4, 2019

Summary

Introduces an API to allow saved objects to be copied from one space to other spaces.

The API design closely follows the Import & Resolve Import Errors Saved Objects APIs, with the following notable exception:

The "resolve import errors" api allows callers to replace references to index patterns and saved searches. The corresponding Copy saved objects "conflict resolution" endpoint does not allow for such behavior, as the primary consumer (the UI) would need a way to do a cross-space search of these objects. While we could technically put something in place, a more holistic solution would be preferred, such as #27003

Copy saved objects endpoints:

  • POST /api/spaces/_copy_saved_objects - copy objects from the default space.
  • POST /s/my-space/api/spaces/_copy_saved_objects - copy objects from the my-space space.

Sample request payload

{
	"spaces": ["space-1", "space-3"],
	"objects": [{
		"type": "dashboard",
		"id": "7adfa750-4c81-11e8-b3d7-01146121b73d"
	}],
	"overwrite": true,
	"includeReferences": true
}

Copy saved objects "conflict resolution" endpoints:

  • POST /api/spaces/_resolve_copy_saved_objects_errors - resolve copy conflicts from the default space.
  • POST /s/my-space/api/spaces/_resolve_copy_saved_objects_errors - resolve copy conflicts from the my-space space.

Sample request payload

{
	"objects": [{
		"type": "dashboard",
		"id": "7adfa750-4c81-11e8-b3d7-01146121b73d"
	}],
	"includeReferences": true,
    "retries": {
		"space-1": [{
          "type": "index-pattern",
          "id": "foo",
          "overwrite": true
       }]
	}
}

@legrego
Copy link
Member Author

legrego commented Jun 4, 2019

retest

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@legrego
Copy link
Member Author

legrego commented Jun 5, 2019

@kobelb here's one possible implementation, using the approach you offered here where we swap the security and spaces SOC wrapper orders. It's by no means review-ready, it's just meant to convey the general approach. Thoughts on what we have here before I flesh this out further?

@kobelb kobelb self-requested a review June 5, 2019 14:40
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member Author

legrego commented Aug 20, 2019

Retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member Author

legrego commented Aug 20, 2019

@kobelb thanks for the test review - much appreciated! I think I've addressed all of your feedback if you're able to take another look.

@legrego
Copy link
Member Author

legrego commented Aug 21, 2019

If there are additional non-critical findings with the tests, I would prefer to merge this sooner rather than later in order to get #39002 in a reviewable state ASAP. We can address findings in a followup post-FF

attributes: {},
},
],
importSavedObjectsImpl: opts => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwriting this removes this check expect(objectsToImport).toEqual(setupOpts.objects);.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does... my thinking here was that it wasn't creating any meaningful gaps, since the override is only used to test specific failure conditions. I'll update regardless.

`);

expect((savedObjectsService.importExport.importSavedObjects as jest.Mock).mock.calls)
.toMatchInlineSnapshot(`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm not overly fond of using jest's snapshots for stuff like this... It requires that we supplement it with another expectation and can lead to hard to understand failures. I don't necessarily think we need to change this now, but I'd prefer we have an offline discussion with @azasypkin to figure out when/where we want to use the snapshotting functionality in the future.

attributes: {},
},
],
resolveImportErrorsImpl: opts => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying this removes this check: expect(objectsToResolve).toEqual(setupOpts.objects); from above.

describe('useRbacForRequest is true', () => {
test(`throws Boom.forbidden when user isn't authorized for any spaces`, async () => {
const username = Symbol();
test(`ignores the provided purpose`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be throwing an error when there's an invalid purpose when useRbac is false? It feels like this would be more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm fine either way

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor

kobelb commented Aug 21, 2019

retest

1 similar comment
@legrego
Copy link
Member Author

legrego commented Aug 21, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member Author

legrego commented Aug 21, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego merged commit 608e239 into elastic:master Aug 21, 2019
@legrego legrego deleted the spaces/copy-to-space-api branch August 21, 2019 21:27
legrego added a commit to legrego/kibana that referenced this pull request Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Spaces Platform Security - Spaces feature release_note:enhancement review Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants