-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Search across spaces #67644
Search across spaces #67644
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
kibana/x-pack/plugins/encrypted_saved_objects/server/saved_objects/index.ts
Lines 74 to 92 in 3de9350
getDecryptedAsInternalUser: async <T = unknown>( | |
type: string, | |
id: string, | |
options?: SavedObjectsBaseOptions | |
): Promise<SavedObject<T>> => { | |
const [internalRepository, typeRegistry] = await internalRepositoryAndTypeRegistryPromise; | |
const savedObject = await internalRepository.get(type, id, options); | |
return { | |
...savedObject, | |
attributes: (await service.decryptAttributes( | |
{ | |
type, | |
id, | |
namespace: typeRegistry.isSingleNamespace(type) ? options?.namespace : undefined, | |
}, | |
savedObject.attributes as Record<string, unknown> | |
)) as T, | |
}; | |
}, |
- This PR as-is essentially adds the
namespaces
attribute for all single-namespace exported objects. However, that attribute doesn't mean anything in an export file. In Initial server-side support for sharing saved-objects phase 1.5 #66089 I actually explicitly omit that field from exported objects; I just wanted to point that out. I agree it makes sense to start returningnamespaces
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/default
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look and let me know what you think too:
Love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like Option 3, but it seems like you implemented Option 2 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
kibana/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
Line 159 in 36dd1b4
throw this.errors.decorateForbiddenError(new Error()); |
namespaces=[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took another pass on the changes since my last review. All looks excellent!
Co-authored-by: Joe Portner <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Joe Portner <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Joe Portner <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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/_find
API. All other endpoints will continue to enforce space restrictions as before.API Changes
Optional
namespaces
query parameterAn optional
namespaces
query parameter has been added to the/api/saved_objects/_find
endpoint, which accepts:A space id to search against. e.g.:
/api/saved_objects/_find?namespaces=marketing
Multiple spaces ids to search against. e.g.:
/api/saved_objects/_find?namespaces=marketing&namespaces=sales&namespaces=default
A wildcard to search against all available spaces e.g.:
/api/saved_objects/_find?namespaces=*
When
namespaces
is specified, the user's current space is ignored, unless also included in thenamespaces
query parameter, or if using the wildcard option. For example,/s/marketing/api/saved_objects/_find?namespaces=sales
would only search thesales
space, but both/s/marketing/api/saved_objects/_find?namespaces=sales&namespaces=marketing
and/s/marketing/api/saved_objects/_find?namespaces=*
would include themarketing
space.namespaces
added to saved object responsesSince we are returning saved objects across multiple spaces, there is a risk of ID collision. If a dashboard with id
foo
exists in both thesales
andmarketing
spaces, 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
namespaces
property at the top level, which will be an array of space ids.Example:
Resolves #27003