Throw error in collectMultiNamespaceReferences for invalid inbound re…

…ferences

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', () => {

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<string>()
     );
+    const retrievedObjectsSet = response.objects.reduce(
+      (acc, { type, id }) => acc.add(`${type}:${id}`),
+      new Set<string>()
+    );
     const traversedObjects = new Set<string>();
     const filteredObjectsMap = new Map<string, SavedObjectReferenceWithContext>();
     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}"`
+            );
+          }
+        }
       }
     }