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.

Sign up for GitHub

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

Jump to bottom

Spaces - make space a hidden saved object type #41688

Merged
merged 19 commits into from
Jul 30, 2019
Merged

Spaces - make space a hidden saved object type #41688

merged 19 commits into from
Jul 30, 2019

Conversation

legrego
Copy link
Member

@legrego legrego commented Jul 22, 2019

Summary

This PR marks the space saved object type as "hidden". Spaces should not be interacted with directly via the Saved Objects APIs, but rather through the dedicated spaces APIs.

This was previously enforced by the Spaces Saved Objects Client Wrapper, but with the addition of #40275, it is now possible to retrieve an instance of the Saved Objects Client that does not include this enforcement.

Marking the space type as hidden allows us to remove this restriction from the wrapper altogether, and instead rely on the built-in capabilities of the Saved Objects Service.

Reviewer notes
The code change itself is very small. The bulk of the LoC delta is testing. We used to get away with testing the wrapper itself, but now that the wrapper no longer has this check, we have to rely on functional API tests to ensure this is working as expected.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

Pinging @elastic/kibana-security

@legrego legrego added release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0 labels Jul 22, 2019
@legrego legrego marked this pull request as ready for review July 25, 2019 13:23
@legrego legrego requested a review from a team as a code owner July 25, 2019 13:23
@legrego legrego requested a review from kobelb July 25, 2019 13:24
@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member Author

legrego commented Jul 25, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member Author

legrego commented Jul 25, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor

kobelb commented Jul 26, 2019

The test coverage you've added with this is great! I was surprised to see that the saved object api integration tests didn't cover hidden types already. Being super pedantic, I created a PR to change the api integration tests for testing with the space type directly to use a generic "hidden type" legrego#21 because it felt like we'd want these tests to not be scoped to the explicit spaces saved object type but all hidden types.

@legrego
Copy link
Member Author

legrego commented Jul 30, 2019

Thanks for the PR @kobelb!

I created a PR to change the api integration tests for testing with the space type directly to use a generic "hidden type" legrego#21 because it felt like we'd want these tests to not be scoped to the explicit spaces saved object type but all hidden types.

I'm torn on this. On one hand, I completely agree that we should have test coverage for hidden types, and your PR certainly addresses that. What I worry about is that we don't have an explicit test for the space type anymore...I think of these API tests as being more black-box than white-box, and the change to test hidden types feels like we're testing more of an implementation detail of the Spaces plugin. We could change the way that the spaces saved object type behaves (either on purpose or by mistake), and the hidden type tests wouldn't catch that.

I think there is benefit to having both tests, just like we did/do for the "rbac user" and "dual auth user" scenarios. The code tells is that there should be no functional difference between the two, but we still have the different scenarios to verify this.

How would you feel about leaving this PR as-is, but opening another to address the testing gap for hidden saved object types?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb
Copy link
Contributor

kobelb commented Jul 30, 2019 •
edited
Loading

Good point, these changes do remove the coverage from we have from the api integration tests to ensure that the spaces saved object type can't be interacted with using the saved object APIs.

We could potentially remedy this by adding some high-level tests here to ensure we can't do this with a superuser just at the default space.

I see the "saved object api integration tests" as providing us coverage to the core concepts of the "saved object system". We're already using a test fixture to mock-out the globaltype so we can test how the system behaves with a global type. It feels like we should do the same with the hidden types. The fact that these test utilize the other dashboard/visualization/etc. types that are provided by OSS bothers me, because we're then prone to needing to change these tests when application authors make changes. We saw this maintenance burden when working on the "ui capability api integration tests" and switched to using the test fixtures, which alleviated this burden.

I think there is benefit to having both tests, just like we did/do for the "rbac user" and "dual auth user" scenarios. The code tells is that there should be no functional difference between the two, but we still have the different scenarios to verify this.

I primarily see the benefit in testing all the permutations/combinations for the "hidden type" not specifically for the "spaces type" based on the aforementioned reasoning.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

kobelb
kobelb approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for tolerating by pedantry.

@legrego legrego merged commit 4486ff4 into elastic:master Jul 30, 2019
@legrego legrego deleted the spaces/hidden-so-type branch July 30, 2019 20:44
@legrego legrego mentioned this pull request Jul 30, 2019
Merged
legrego added a commit to legrego/kibana that referenced this pull request Jul 30, 2019
@legrego @kobelb
Spaces - make space a hidden saved object type (elastic#41688)
647388d 
* make space a hidden saved object type

* bulk_create api tests

* bulk_get tests

* create functional tests

* delete space tests

* export space tests

* find space tests

* get space tests

* import space tests

* resolve_import_errirs space tests

* update space tests

* standardize test names where appropriate

* remove unused import

* Switching tests from using the space type directly to a "hidden t… (#21)

* add space saved object api tests


Co-authored-by: Brandon Kobel <brandon.kobel@gmail.com>
legrego added a commit that referenced this pull request Jul 30, 2019
@legrego @kobelb
Spaces - make space a hidden saved object type (#41688) (#42301)
a12d378 
* make space a hidden saved object type

* bulk_create api tests

* bulk_get tests

* create functional tests

* delete space tests

* export space tests

* find space tests

* get space tests

* import space tests

* resolve_import_errirs space tests

* update space tests

* standardize test names where appropriate

* remove unused import

* Switching tests from using the space type directly to a "hidden t… (#21)

* add space saved object api tests


Co-authored-by: Brandon Kobel <brandon.kobel@gmail.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 31, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/clone…
646afd9 
…-or-edit-existing-rollup-job

* 'master' of github.com:elastic/kibana: (114 commits)
  [ML] Fixing empty index pattern list (elastic#42299)
  [Markdown] Shim new platform - cleanup plugin (elastic#41760)
  [Code] Enable hierarchicalDocumentSymbolSupport for java language server (elastic#42233)
  Add New Platform mocks for data plugin (elastic#42261)
  Http server route handler implementation (elastic#41894)
  [SR] Allow custom index pattern to be used instead of selectable list when choosing indices to restore (elastic#41534)
  [Code] distributed Code abstraction (elastic#41374)
  [SIEM] Sets page titles to the current page you are on  (elastic#42157)
  Saved Objects export API stable type order (elastic#42310)
  cancellation of interpreter execution (elastic#40238)
  [SIEM] Fixes a crash when Machine Learning influencers is an undefined value (elastic#42198)
  Changed the job to work with a dedicated index (elastic#42297)
  FTR: fix testSubjects.missingOrFail (elastic#42290)
  Increase retry timeout to prevent flaky tests (elastic#42291)
  Spaces - make space a hidden saved object type (elastic#41688)
  Allow applications to register feature privileges which are excluded from the base privileges (elastic#41300)
  Disable flaky log column reorder test (elastic#42285)
  Fixing add element in element reducer (elastic#42276)
  Fix infinite loop (elastic#42228)
  [Maps][File upload] Remove geojson deep clone logic, handle on maps side (elastic#41835)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobelb kobelb kobelb approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@legrego @elasticmachine @kobelb