From 230409aba7167a5bdcee9c528e6497e9e0aa2467 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 12 May 2021 16:52:49 -0400 Subject: [PATCH] Throw error in collectMultiNamespaceReferences for invalid inbound references --- ...ecure_saved_objects_client_wrapper.test.ts | 70 ++++++++----------- .../secure_saved_objects_client_wrapper.ts | 40 ++++++++--- 2 files changed, 60 insertions(+), 50 deletions(-) diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 2aab704b7718a..e5a2340aba3f0 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -1243,6 +1243,36 @@ describe('#collectMultiNamespaceReferences', () => { expectAuditEvent(AUDIT_ACTION, 'failure', reqObj3); }); }); + + test(`throws an error if the base client result includes a requested object without a valid inbound reference`, async () => { + // We *shouldn't* ever get an inbound reference that is not also present in the base client response objects array. + const spaces = [spaceX]; + + const obj1 = { ...reqObj1, spaces, inboundReferences: [] }; + const obj2 = { + type: 'a', + id: '2', + spaces, + ...getInboundRefsFrom({ type: 'some-type', id: 'some-id' }), + }; + clientOpts.baseClient.collectMultiNamespaceReferences.mockResolvedValueOnce({ + objects: [obj1, obj2], + }); + mockEnsureAuthorized.mockResolvedValue({ + status: 'partially_authorized', + typeActionMap: new Map().set('a', { + bulk_get: { isGloballyAuthorized: true, authorizedSpaces: [] }, + }), + }); + // When the loop gets to obj2, it will determine that the user is authorized for the object but *not* for the graph. However, it will + // also determine that there is *no* valid inbound reference tying this object back to what was requested. In this case, throw an + // error. + + const options = { namespace: spaceX }; // spaceX is the current space + await expect(() => + client.collectMultiNamespaceReferences([reqObj1], options) + ).rejects.toThrowError('Unexpected inbound reference to "some-type:some-id"'); + }); }); describe(`checks privileges`, () => { @@ -1433,46 +1463,6 @@ describe('#collectMultiNamespaceReferences', () => { // obj2, obj4, and obj7 are intentionally excluded from the audit record because we did not return any information about them to the user }); }); - - test(`circuit-breaker works as expected`, async () => { - // We *shouldn't* ever get an inbound reference that is not also present in the base client response objects array. - // The circuit-breaker is in place to prevent an infinite loop in the event this did happen. - const reqObj1 = { type: 'a', id: '1' }; - const spaces = [spaceX]; - - const obj1 = { ...reqObj1, spaces, inboundReferences: [] }; - const obj2 = { type: 'b', id: '2', spaces, ...getInboundRefsFrom(obj1) }; - const obj3 = { - type: 'a', - id: '3', - spaces, - ...getInboundRefsFrom(obj2, { type: 'some-type', id: 'some-id' }), - }; - clientOpts.baseClient.collectMultiNamespaceReferences.mockResolvedValueOnce({ - objects: [obj1, obj2, obj3], - }); - mockEnsureAuthorized.mockResolvedValue({ - status: 'partially_authorized', - typeActionMap: new Map().set('a', { - bulk_get: { isGloballyAuthorized: true, authorizedSpaces: [] }, - }), - // the user is not authorized to read type 'b', - }); - // When the loop gets to obj3, it will determine that the user is authorized for the object but *not* for the graph (because the user is - // not authorized to read obj3's inbound reference, obj2). However, it will also determine that there is another inbound reference that - // it hasn't traversed yet (some-type:some-id), so it will push obj3 to the end of the array and continue to loop. The circuit-breaker - // will end the loop in this scenario. - - const options = { namespace: spaceX }; // spaceX is the current space - const result = await client.collectMultiNamespaceReferences([reqObj1], options); - expect(result).toEqual({ - objects: [ - obj1, - { ...obj2, spaces: [], isMissing: true }, // obj2 is marked as Missing because the user was not authorized to access it - // obj3 is not included at all because the user was not authorized to access its inbound reference (obj4) - ], - }); - }); }); describe('#updateObjectsSpaces', () => { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index b096db77b59e5..ef3dcac4c064b 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -613,15 +613,18 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra (acc, { type, id }) => acc.add(`${type}:${id}`), new Set() ); + const retrievedObjectsSet = response.objects.reduce( + (acc, { type, id }) => acc.add(`${type}:${id}`), + new Set() + ); const traversedObjects = new Set(); const filteredObjectsMap = new Map(); const getIsAuthorizedForInboundReference = (inbound: { type: string; id: string }) => { const found = filteredObjectsMap.get(`${inbound.type}:${inbound.id}`); return found && !found.isMissing; // If true, this object can be linked back to one of the requested objects }; - let loopCount = 0; let objectsToProcess = [...response.objects]; - while (++loopCount && objectsToProcess.length > 0) { + while (objectsToProcess.length > 0) { const obj = objectsToProcess.shift()!; const { type, id, spaces, inboundReferences } = obj; const objKey = `${type}:${id}`; @@ -659,14 +662,31 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra filteredObjectsMap.set(objKey, obj); } else if (!isAuthorizedForObject && isAuthorizedForGraph) { filteredObjectsMap.set(objKey, { ...obj, spaces: [], isMissing: true }); - } else if ( - isAuthorizedForObject && - !isAuthorizedForGraph && - inboundReferences.some((x) => !traversedObjects.has(`${x.type}:${x.id}`)) && - loopCount <= response.objects.length // circuit-breaker to prevent infinite loops - ) { - // this object has inbound reference(s) that we haven't traversed yet; bump it to the back of the list - objectsToProcess = [...objectsToProcess, obj]; + } else if (isAuthorizedForObject && !isAuthorizedForGraph) { + const hasUntraversedInboundReferences = inboundReferences.some( + (ref) => + !traversedObjects.has(`${ref.type}:${ref.id}`) && + retrievedObjectsSet.has(`${ref.type}:${ref.id}`) + ); + + if (hasUntraversedInboundReferences) { + // this object has inbound reference(s) that we haven't traversed yet; bump it to the back of the list + objectsToProcess = [...objectsToProcess, obj]; + } else { + // There should never be a missing inbound reference. + // If there is, then something has gone terribly wrong. + const missingInboundReference = inboundReferences.find( + (ref) => + !traversedObjects.has(`${ref.type}:${ref.id}`) && + !retrievedObjectsSet.has(`${ref.type}:${ref.id}`) + ); + + if (missingInboundReference) { + throw new Error( + `Unexpected inbound reference to "${missingInboundReference.type}:${missingInboundReference.id}"` + ); + } + } } }