Conversation
aedd880 to
a33c3df
Compare
af184f1 to
b00786a
Compare
b00786a to
36dd1b4
Compare
|
@jportner this is ready for a first-pass review, whenever you get a chance |
jportner
left a comment
There was a problem hiding this comment.
Looks pretty good so far! I have a few suggestions in the comments below. In addition:
- Encrypted saved objects -- From the looks of it you may want to make some corresponding changes in this method:
- This PR as-is essentially adds the
namespacesattribute for all single-namespace exported objects. However, that attribute doesn't mean anything in an export file. In #66089 I actually explicitly omit that field from exported objects; I just wanted to point that out. I agree it makes sense to start returningnamespacesfor other Saved Object operations (find, get, update, etc.) but I still think we should ultimately omit it from exported objects. @kobelb and I had a conversation about this a while back, interested to see if he has any opinions to share.
| if (namespaces?.some((namespace) => namespace.indexOf('*') >= 0)) { | ||
| throw Boom.notAcceptable(`namespaces cannot contain wildcards ("*")`); | ||
| } |
There was a problem hiding this comment.
One thing that sticks out to me here is that we're effectively changing the valid API parameters based on whether or not the Spaces plugin is enabled. We rely on the SpacesSavedObjectsClient wrapper to transform a "*" into an array of space IDs.
Perhaps instead we should treat "*" as the Default space here? Then we would have the following cases when searching with "*":
- Spaces and Security enabled: Search in all authorized spaces
- Spaces enabled, Security disabled: Search in all spaces
- Spaces disabled, Security enabled: Search in current/default space
- Spaces and Security disabled: Search in current/default space
(Note: just double checked, the Spaces wrapper has a higher priority than the Security wrapper. Maybe would need to change the Security wrapper too to also treat "*" as the Default space, haven't fully thought through all of the ramifications.)
There was a problem hiding this comment.
One thing that sticks out to me here is that we're effectively changing the valid API parameters based on whether or not the Spaces plugin is enabled. We rely on the SpacesSavedObjectsClient wrapper to transform a "*" into an array of space IDs
Yeah, that's a good point.
Perhaps instead we should treat "" as the Default space here? Then we would have the following cases when searching with ""
I'm torn on this. Conceptually it makes sense, as the Default space is the only real namespace that exists when the Spaces plugin is disabled. We could alternatively pass this through unmodified, which would just return 0 objects, since there won't ever be a namespace which contains a wildcard. I double checked the terms query, and it does not appear to interpret wildcards, so passing this through shouldn't return unexpected objects. My intent for putting this in was prevent the wildcard from being interpreted, but I think that was premature.
So, I think we have three options:
- Leave this alone, and continue to disallow
* - Change
*toundefined/defaultto represent the Default space - Allow the namespaces through as-is. Don't block or rename the namespaces.
I think I'm favoring option 3, but I'm interested in hearing your thoughts too.
I like option 2 because the APIs will be compatible between the OSS and Default distributions, but there's a part of me that worries about silently interpreting * as the default space here. A bug in the already complex saved objects service could inadvertently cause us to return objects from the Default Space when the user isn't actually authorized to access them.
There was a problem hiding this comment.
We could alternatively pass this through unmodified, which would just return 0 objects, since there won't ever be a namespace which contains a wildcard.
This is true currently, but we might eventually support a "*" in a multi-namespace object's namespaces array with #58756.
there's a part of me that worries about silently interpreting * as the default space here. A bug in the already complex saved objects service could inadvertently cause us to return objects from the Default Space when the user isn't actually authorized to access them.
Yeah, that's certainly a valid concern. It would only be a problem if that bug managed to happen while Spaces and Security are enabled, right?
All of the logic combined is pretty complex, but the Spaces SO client code for checking for a wildcard is pretty simple.
Ultimately I'd still lean towards option 3, but leaving option 1 in place is probably fine as this is a corner case.
There was a problem hiding this comment.
This is true currently, but we might eventually support a "*" in a multi-namespace object's namespaces array with #58756.
Great point! Thanks for the reminder.
Yeah, that's certainly a valid concern. It would only be a problem if that bug managed to happen while Spaces and Security are enabled, right?
Yes, that's right. For what it's worth, I expect that this scenario (spaces + security enabled) is one of the more common setups these days, especially since both are free.
Ultimately I'd still lean towards option 3, but leaving option 1 in place is probably fine as this is a corner case.
Let me see what option 3 looks like and we can decide
There was a problem hiding this comment.
@jportner I think I like Option 3 as well. Take a look and let me know what you think too: 602d8ff (#67644)
There was a problem hiding this comment.
Take a look and let me know what you think too:
Love it!
There was a problem hiding this comment.
I think I like Option 3, but it seems like you implemented Option 2 😉
There was a problem hiding this comment.
I think I like Option 3, but it seems like you implemented Option 2 😉
Ha, you're right, sorry about that. If we go with option 3, then searching using the * will return 0 results for the OSS distribution, and the Default distribution when Spaces is disabled. Are you comfortable with that inconsistency?
In reality, the namespaces option shouldn't be used in OSS, so I'm not terribly concerned about that scenario. The default distribution with Spaces disabled has the potential to be problematic though. With Option 3, consumers will have to check to see if Spaces is enabled first before using the ?namespaces=* parameter. We have a hard enough time getting teams to think about Spaces at all, let alone the scenario where Spaces may be disabled.
There was a problem hiding this comment.
I'd like for OSS Kibana Platform to be a neutral foundation for third-party plugins to build on top off, but not breaking plugins when Spaces is disabled sounds like a good idea. So happy to go with Option 2. If a third-party developer ever opens an issue we can change this behaviour ;-)
x-pack/test/saved_object_api_integration/common/lib/saved_object_test_cases.ts
Outdated
Show resolved
Hide resolved
x-pack/test/saved_object_api_integration/common/lib/saved_object_test_cases.ts
Outdated
Show resolved
Hide resolved
I agree, the fact that it's showing up in the export is an oversight on my part. Thanks! |
src/core/server/saved_objects/service/lib/search_dsl/search_dsl.ts
Outdated
Show resolved
Hide resolved
x-pack/test/saved_object_api_integration/common/lib/saved_object_test_cases.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
…na into spaces/search-across-spaces
Sorry, was kinda busy with the ES client this week, did not find time to review this. Will try to do it before EOW |
src/core/server/saved_objects/service/lib/search_dsl/query_params.ts
Outdated
Show resolved
Hide resolved
| references: [], | ||
| namespaces: ['default'], |
There was a problem hiding this comment.
So we are adding the namespaces property to all SO APIs even if it's only relevant for find atm?
There was a problem hiding this comment.
Yes, that's my intent, so that the APIs are at least consistent with each other
| describe('wildcard namespace', () => { | ||
| it('should return 200 with individual responses from the default namespace', async () => |
There was a problem hiding this comment.
A little misleading, as it doesn't seem to match your description:
A wildcard to search against all available spaces e.g.: /api/saved_objects/_find?namespaces=*
But I guess as we are in OSS test suite, we can't really do better.
| public async find<T = unknown>(options: SavedObjectsFindOptions) { | ||
| await this.ensureAuthorized(options.type, 'find', options.namespace, { options }); | ||
| if ( | ||
| this.getSpacesService() == null && | ||
| Array.isArray(options.namespaces) && | ||
| options.namespaces.length > 0 | ||
| ) { | ||
| throw this.errors.createBadRequestError( | ||
| `_find across namespaces is not permitted when the Spaces plugin is disabled.` | ||
| ); | ||
| } | ||
| await this.ensureAuthorized(options.type, 'find', options.namespaces, { options }); |
There was a problem hiding this comment.
Not my scope, but +1 with joe here.
| this.debugLogger( | ||
| `SpacesClient.getAll(), using RBAC. returning 403/Forbidden. Not authorized for any spaces.` | ||
| `SpacesClient.getAll(), using RBAC. returning 403/Forbidden. Not authorized for any spaces for ${purpose} purpose.` |
There was a problem hiding this comment.
Unrelated to the review, but asking for GS: what is the best way to retrieve the list of spaces a user is allowed to access from a Request?
There was a problem hiding this comment.
From the GS perspective, you shouldn't have to figure this out yourself. The Spaces plugin takes care of this automatically when you ask to search using the /api/saved_objects/_find?namespaces=* syntax
To answer your question though, you can get the list of available spaces by doing something like this:
public setup(core: CoreSetup, plugins: PluginsSetup) {
const {spacesService} = plugins.spaces;
registerRoutes({ router, spacesService })
}
function registerRoutes({ spacesService, router }) {
router.get({
path: '/api/plugin/foo',
validate: false
}, (context, req, res) => {
const availableSpaces = await spacesService.scopedClient(req).getAll();
});
}There was a problem hiding this comment.
From the GS perspective, you shouldn't have to figure this out yourself.
Not yet 100% sure on that one. We need to distinguish the results from the current namespace vs the others. We may only need to perform a single query and apply logic depending on the active namespace for that, but we might also want to have some specific search criteria for both searches (current vs other), therefor having a search call for the current NS and another the the others.
Thanks for the snippet btw.
There was a problem hiding this comment.
but we might also want to have some specific search criteria for both searches (current vs other), therefor having a search call for the current NS and another the the others.
Ah, that's interesting (and good to know!). I think we'll want to change the API if we end up needing specific criteria for both searches. We can't have this endpoint take an array of namespaces when there could be thousands of spaces to search across
There was a problem hiding this comment.
@pgayvallet do you have a sense of which design is more likely? Should we account for searching across all spaces except the current space as part of this PR, or should we address that later on if needed?
x-pack/test/saved_object_api_integration/security_and_spaces/apis/find.ts
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/search_dsl/query_params.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const normalizedNamespaces = namespaces | ||
| ? Array.from( | ||
| new Set(namespaces.map((namespace) => (namespace === '*' ? 'default' : namespace))) |
There was a problem hiding this comment.
Why does OSS interpret * as 'default'? Usually OSS objects will all be in the default space, but a third-party spaces plugin could leverages the namespaces array and create objects in other spaces. I would then expect '*' to remove all namespaces filtering.
There was a problem hiding this comment.
We went back and forth on this: #67644 (comment)
My original implementation threw an error if a wildcard made its way here, as I had an invalid fear of that being treated as an actual wildcard during search. We aren't using the DSL in a way that allows for the wildcard, so it would instead be treated as a literal * if we passed it through unmodified.
@jportner and I felt that interpreting * as default would be the most consistent with the way OSS behaves today, or the way the default distribution behaves when spaces is disabled. I could see an argument for supporting a third-party spaces plugin, but it didn't seem worthwhile to me. I worry about stripping out the namespaces filtering altogether, given the complexity of the saved objects service (I would hate to accidentally expose objects the user isn't authorized to see). This is probably an irrational fear though, so we can explore alternatives if you'd prefer.
There was a problem hiding this comment.
Can we add a comment explaining the reasoning from #67644 (comment) so we remember why there's this magic 😂
|
@elasticmachine merge upstream |
…na into spaces/search-across-spaces
|
@jportner @rudolf @pgayvallet I believe I've addressed all the concrete feedback here. The main open question we have left is how we should interpret |
| if (should.length === 0) { | ||
| throw new Error('cannot specify empty namespaces array'); | ||
| } |
There was a problem hiding this comment.
Can we safely exclude this from happening for multi namespace types? I thought we should fail for namespaces.length === 0 for both cases at the top of the function, maybe even if SavedObjectsRepository#find? The HTTP API and
namespaces=[]
There was a problem hiding this comment.
I'll move this check to the top of the function, good call. I could duplicate this check within the repository, but it seems unnecessary at this point (IMO). Happy to revisit though. We have an explicit check within the spaces_saved_objects_client because we want to ensure the behavior is consistent from an authorization perspective, so as to not leak more information than necessary
|
|
||
| const normalizedNamespaces = namespaces | ||
| ? Array.from( | ||
| new Set(namespaces.map((namespace) => (namespace === '*' ? 'default' : namespace))) |
There was a problem hiding this comment.
Can we add a comment explaining the reasoning from #67644 (comment) so we remember why there's this magic 😂
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
jportner
left a comment
There was a problem hiding this comment.
Took another pass on the changes since my last review. All looks excellent!
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (314 commits) [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) Search across spaces (elastic#67644) ...
Summary
This is the first round of support for accessing saved objects in spaces other than your current space. The scope of this is limited to the
/api/saved_objects/_findAPI. All other endpoints will continue to enforce space restrictions as before.API Changes
Optional
namespacesquery parameterAn optional
namespacesquery parameter has been added to the/api/saved_objects/_findendpoint, which accepts:A space id to search against. e.g.:
/api/saved_objects/_find?namespaces=marketingMultiple spaces ids to search against. e.g.:
/api/saved_objects/_find?namespaces=marketing&namespaces=sales&namespaces=defaultA wildcard to search against all available spaces e.g.:
/api/saved_objects/_find?namespaces=*When
namespacesis specified, the user's current space is ignored, unless also included in thenamespacesquery parameter, or if using the wildcard option. For example,/s/marketing/api/saved_objects/_find?namespaces=saleswould only search thesalesspace, but both/s/marketing/api/saved_objects/_find?namespaces=sales&namespaces=marketingand/s/marketing/api/saved_objects/_find?namespaces=*would include themarketingspace.namespacesadded to saved object responsesSince we are returning saved objects across multiple spaces, there is a risk of ID collision. If a dashboard with id
fooexists in both thesalesandmarketingspaces, then a query across these spaces would return these two dashboards, without a way to distinguish them from each other.To solve this, we are augmenting the APIs which retrieve saved objects to include a
namespacesproperty at the top level, which will be an array of space ids.Example:
{ "type": "dashboard", "id": "foo", "attributes": { ... }, "references": [], "namespaces": ["default"] }Resolves #27003