-
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
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' | ||
| ) { | ||
| 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, |
||
| // 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(`returns empty authorization map for partially authorized if the authorized types are not part of the query`, 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 | ||
| 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(`returns empty authorization map for fully authorized if the authorized types are not part of the query`, async () => { | ||
| setupAuthorizeFind(mockSecurityExt, 'fully_authorized'); | ||
| setupRedactPassthrough(mockSecurityExt); | ||
|
|
||
| await findSuccess( | ||
| client, | ||
| repository, | ||
| { | ||
| type: [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>(); | ||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -113,9 +113,12 @@ export const swapReferencesRoute = | |
| }, | ||
| router.handleLegacyErrors( | ||
| handleErrors(async (ctx, req, res) => { | ||
| const savedObjectsClient = (await ctx.core).savedObjects.client; | ||
| const [core] = await getStartServices(); | ||
| const types = core.savedObjects.getTypeRegistry().getAllTypes(); | ||
| const savedObjectsContract = (await ctx.core).savedObjects; | ||
| const types = savedObjectsContract.typeRegistry.getAllTypes(); | ||
| const savedObjectsClient = savedObjectsContract.getClient({ | ||
| // Make sure to search through all the hidden types as well. | ||
| includedHiddenTypes: types.filter((t) => t.hidden).map((t) => t.name), | ||
|
Comment on lines
+119
to
+120
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 assume that we want to search references to the Data View across all types. Is my assumption correct? |
||
| }); | ||
| const type = req.body.fromType || DATA_VIEW_SAVED_OBJECT_TYPE; | ||
| const searchId = | ||
| !Array.isArray(req.body.forId) && req.body.forId !== undefined | ||
|
|
||
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.