-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[SOR] Intersect allowed and authorized types #244967
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
Changes from all commits
6f51d1e
3a28421
3ac4e78
d3c4c4a
fdb0612
b668fb5
95f5d0f
32fa80e
92b6022
0d61a99
6ffa7ed
3b11a37
c42d401
fcc7200
ece637f
3d77a91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -170,10 +170,15 @@ export const performFind = async <T = unknown, A = unknown>( | |||||||||||
| // If the user is unauthorized to find *anything* they requested, return an empty response | ||||||||||||
| return SavedObjectsUtils.createEmptyFindResponse<T, A>(options); | ||||||||||||
| } | ||||||||||||
| if (authorizationResult?.status === 'partially_authorized') { | ||||||||||||
| if ( | ||||||||||||
| authorizationResult?.status === 'fully_authorized' || | ||||||||||||
| authorizationResult?.status === 'partially_authorized' | ||||||||||||
| ) { | ||||||||||||
|
Comment on lines
+173
to
+176
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| typeToNamespacesMap = new Map<string, string[]>(); | ||||||||||||
| for (const [objType, entry] of authorizationResult.typeMap) { | ||||||||||||
| if (!entry.find) continue; | ||||||||||||
| // Discard the types that the SO repository doesn't know about (typically hidden objects). | ||||||||||||
| if (!allowedTypes.includes(objType)) continue; | ||||||||||||
|
Comment on lines
+180
to
+181
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixing the user's authorization and the client's scope here (which can be odd). However,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of the issue/complexity for me is that we have kibana/src/core/packages/saved-objects/api-server-internal/src/lib/search/search_dsl/query_params.ts Line 205 in d70608e
Shouldn't our
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, this would break the authorization part: if we only look at the requested The bug here is that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks for taking a closer look @afharo ! We also have a comment talking directly to the issue I expressed: typeToNamespacesMap, // If defined, this takes precedence over the `type` and `namespaces` fieldsStill a bit odd, but I think that's OK! |
||||||||||||
| // This ensures that the query DSL can filter only for object types that the user is authorized to access for a given space | ||||||||||||
| const { authorizedSpaces, isGloballyAuthorized } = entry.find; | ||||||||||||
| typeToNamespacesMap.set(objType, isGloballyAuthorized ? namespaces : authorizedSpaces); | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ import { | |
| generateIndexPatternSearchResults, | ||
| setupAuthorizeFunc, | ||
| setupAuthorizeFind, | ||
| HIDDEN_TYPE, | ||
| } from '../test_helpers/repository.test.common'; | ||
| import { savedObjectsExtensionsMock } from '../mocks/saved_objects_extensions.mock'; | ||
| import { arrayMapsAreEqual } from '@kbn/core-saved-objects-utils-server'; | ||
|
|
@@ -845,14 +846,108 @@ describe('SavedObjectsRepository Security Extension', () => { | |
| }); | ||
| }); | ||
|
|
||
| test(`passes an empty type map to the query when partially authorized but no types are authorized for find or are hidden`, async () => { | ||
| setupAuthorizeFind(mockSecurityExt, 'partially_authorized'); | ||
| setupRedactPassthrough(mockSecurityExt); | ||
|
|
||
| await findSuccess( | ||
| client, | ||
| repository, | ||
| { | ||
| type: [type, NAMESPACE_AGNOSTIC_TYPE, HIDDEN_TYPE], | ||
| namespaces: [namespace, 'ns-1'], | ||
| }, // include multiple types and spaces | ||
| namespace | ||
| ); | ||
|
|
||
| expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass | ||
| const { | ||
| typeToNamespacesMap: actualMap, | ||
| }: { typeToNamespacesMap: Map<string, string[] | undefined> } = | ||
| mockGetSearchDsl.mock.calls[0][2]; | ||
|
|
||
| expect(actualMap).not.toBeUndefined(); | ||
| const expectedMap = new Map<string, string[] | undefined>(); | ||
|
|
||
| expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy(); | ||
| }); | ||
|
|
||
| test(`passes an empty type map to the query when fully authorized but no types are authorized for find or are hidden`, async () => { | ||
| setupAuthorizeFind(mockSecurityExt, 'fully_authorized'); | ||
| setupRedactPassthrough(mockSecurityExt); | ||
|
|
||
| await findSuccess( | ||
| client, | ||
| repository, | ||
| { | ||
| type: [type, NAMESPACE_AGNOSTIC_TYPE, HIDDEN_TYPE], | ||
| namespaces: [namespace, 'ns-1'], | ||
| }, // include multiple types and spaces | ||
| namespace | ||
| ); | ||
|
|
||
| expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass | ||
| const { | ||
| typeToNamespacesMap: actualMap, | ||
| }: { typeToNamespacesMap: Map<string, string[] | undefined> } = | ||
| mockGetSearchDsl.mock.calls[0][2]; | ||
|
|
||
| expect(actualMap).not.toBeUndefined(); | ||
| const expectedMap = new Map<string, string[] | undefined>(); | ||
|
|
||
| expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy(); | ||
| }); | ||
|
|
||
| test(`uses the authorization map when partially authorized`, async () => { | ||
| setupAuthorizeFind(mockSecurityExt, 'partially_authorized'); | ||
| setupRedactPassthrough(mockSecurityExt); | ||
|
|
||
| await findSuccess( | ||
| client, | ||
| repository, | ||
| { type: [type, NAMESPACE_AGNOSTIC_TYPE], namespaces: [namespace, 'ns-1'] }, // include multiple types and spaces | ||
| { | ||
| type: [ | ||
| type, | ||
| 'foo', | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to add |
||
| // Explicitly request the hidden type despite the repository not having access to it to confirm that it's not authorized. | ||
| HIDDEN_TYPE, | ||
| NAMESPACE_AGNOSTIC_TYPE, | ||
| ], | ||
| namespaces: [namespace, 'ns-1'], | ||
| }, // include multiple types and spaces | ||
| namespace | ||
| ); | ||
|
|
||
| expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass | ||
| const { | ||
| typeToNamespacesMap: actualMap, | ||
| }: { typeToNamespacesMap: Map<string, string[] | undefined> } = | ||
| mockGetSearchDsl.mock.calls[0][2]; | ||
|
|
||
| expect(actualMap).not.toBeUndefined(); | ||
| const expectedMap = new Map<string, string[] | undefined>(); | ||
| expectedMap.set('foo', ['bar']); // this is what is hard-coded in authMap | ||
|
|
||
| expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy(); | ||
| }); | ||
|
|
||
| test(`uses the authorization map when fully authorized`, async () => { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just repeating the test above, but for fully authorized. |
||
| setupAuthorizeFind(mockSecurityExt, 'fully_authorized'); | ||
| setupRedactPassthrough(mockSecurityExt); | ||
|
|
||
| await findSuccess( | ||
| client, | ||
| repository, | ||
| { | ||
| type: [ | ||
| type, | ||
| 'foo', | ||
| // Explicitly request the hidden type despite the repository not having access to it to confirm that it's not authorized. | ||
| HIDDEN_TYPE, | ||
| NAMESPACE_AGNOSTIC_TYPE, | ||
| ], | ||
| namespaces: [namespace, 'ns-1'], | ||
| }, // include multiple types and spaces | ||
| namespace | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,6 +152,11 @@ export interface TypeIdTuple { | |
|
|
||
| export const mappings: SavedObjectsTypeMappingDefinition = { | ||
| properties: { | ||
| foo: { | ||
| properties: { | ||
| // No fields indexed | ||
| }, | ||
| }, | ||
| config: { | ||
| properties: { | ||
| otherField: { | ||
|
|
@@ -242,7 +247,13 @@ export const mappings: SavedObjectsTypeMappingDefinition = { | |
| export const authRecord: Record<string, AuthorizationTypeEntry> = { | ||
| find: { authorizedSpaces: ['bar'] }, | ||
| }; | ||
| export const authMap = Object.freeze(new Map([['foo', authRecord]])); | ||
| export const authMap = Object.freeze( | ||
| new Map([ | ||
| ['foo', authRecord], | ||
| // The user is authorized to read hidden types but tests will confirm that repositories without that hidden type listed, won't show this as authorized. | ||
| [HIDDEN_TYPE, authRecord], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| ]) | ||
| ); | ||
|
|
||
| export const checkAuthError = SavedObjectsErrorHelpers.createBadRequestError( | ||
| 'Failed to check authorization' | ||
|
|
@@ -353,6 +364,7 @@ export const createType = ( | |
|
|
||
| export const createRegistry = () => { | ||
| const registry = new SavedObjectTypeRegistry(); | ||
| registry.registerType(createType('foo')); | ||
| registry.registerType(createType('config')); | ||
| registry.registerType(createType('index-pattern')); | ||
| registry.registerType( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 only ensures the same path is followed for both authorization levels, although, functionally, it achieves the same result if we don't add this clause in the if.
Happy to remove this line if we think that performance might be a problem.
I'd appreciate @azasypkin's thoughts on this.
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.
Would it be more defensive to express this as
if (authorizationResult?.status) {?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 @elastic/kibana-security's input to help me answer that.
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 might be missing some other context here, but if you are concerned with having the overhead of additional conditional checks, then this can just check that
authorizationResultis not undefined or thatauthorizationResult.statusis notunauthorized. Though, just before this we're returning if unauthorized. By this point in the execution path, the status must be either partially or fully authorized.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.
Apologies for not being clear: what I meant is that, in the current code, the
fully_authorizeddoesn't go through this path, sotypeToNamespacesMapis not defined, and onlyallowedTypesis used.Maybe it's not worth spending CPU cycles to calculate the intersection of the authorized and the allowed types because the intersection will always equal
allowedTypes.I only added it because it made the code consistent before the fix. But happy to revert this change if we think that the extra CPU cycles are not necessary.
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.
Ah, I see. Thanks for clarifying. You are correct, and I think it is fine to skip this logic for
fully_authorizedif there is any concern about performance here.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 added a code comment for future us to know that's safe to be removed.
I left it for consistency, and because, while there are more CPU cycles used... I'd expect that, typically, users are
partially_authorized, so we're just removing that branch for a low percentage of the users.