fix: guard user.id before photo fetch, warn on groupIncludeSubGroups, skip dangling child refs

- Guard getUserPhotoGated with user.id presence to prevent requesting users/undefined/photo when Graph omits the id field - Log a warning when groupIncludeSubGroups is configured, matching the existing warning for unsupported userGroupMember* options - Skip spec.children population when groupFilter/groupSearch is active, since child groups may not pass the filter and would create dangling references in the catalog Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: pillaris <pillaris@adobe.com>
2026-04-27 15:10:25 +05:30
parent 9e9eca49c7
commit e1714a861c
2 changed files with 108 additions and 9 deletions
@@ -205,6 +205,25 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
        ),
      );
    });
+
+    it('warns when groupIncludeSubGroups is set', async () => {
+      const provider = new MicrosoftGraphIncrementalEntityProvider({
+        id: 'default',
+        provider: {
+          ...baseProviderConfig,
+          groupIncludeSubGroups: true,
+        } as any,
+        logger,
+      });
+
+      await provider.around(async () => {});
+
+      expect(logger.warn).toHaveBeenCalledWith(
+        expect.stringContaining(
+          'groupIncludeSubGroups is not supported',
+        ),
+      );
+    });
  });

  describe('next — users phase', () => {
@@ -416,6 +435,29 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
      expect(mockGetUserPhotoGated).toHaveBeenCalledWith(mockClient, 'u1', 120);
    });

+    it('skips photo fetch when user has no id to avoid requesting users/undefined/photo', async () => {
+      mockRequestOnePage.mockResolvedValueOnce({
+        items: [
+          {
+            // No id field — Graph can theoretically omit it
+            displayName: 'No-ID User',
+            userPrincipalName: 'noid@example.com',
+          },
+        ],
+        nextLink: undefined,
+      });
+
+      const provider = new MicrosoftGraphIncrementalEntityProvider({
+        id: 'default',
+        provider: baseProviderConfig as any,
+        logger,
+      });
+
+      await provider.next(makeContext());
+
+      expect(mockGetUserPhotoGated).not.toHaveBeenCalled();
+    });
+
    it('continues processing remaining users when a photo load fails and logs the error', async () => {
      mockRequestOnePage.mockResolvedValueOnce({
        items: [
@@ -640,6 +682,50 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
      expect(parentEntity!.entity.spec?.children).toHaveLength(1);
    });

+    it('omits child group refs when groupFilter is active to avoid dangling references', async () => {
+      const providerWithFilter = {
+        ...baseProviderConfig,
+        groupFilter: 'securityEnabled eq true',
+      };
+      (mockClient.getOrganization as jest.Mock).mockResolvedValue({
+        id: 'org-id',
+        displayName: 'My Org',
+      });
+      mockRequestOnePage.mockResolvedValueOnce({
+        items: [
+          { id: 'grp-parent', displayName: 'Parent', mail: 'parent@example.com' },
+        ],
+        nextLink: undefined,
+      });
+      (mockClient.getGroupMembers as jest.Mock).mockReturnValue(
+        asyncYield({
+          '@odata.type': '#microsoft.graph.group',
+          id: 'grp-child',
+          displayName: 'Child Group',
+          mail: 'child@example.com',
+        }),
+      );
+
+      const provider = new MicrosoftGraphIncrementalEntityProvider({
+        id: 'default',
+        provider: providerWithFilter as any,
+        logger,
+      });
+
+      const result = await provider.next(
+        { client: mockClient, provider: providerWithFilter as any },
+        groupsCursor,
+      );
+
+      const parentEntity = result.entities!.find(
+        e =>
+          e.entity.metadata.annotations?.[
+            MICROSOFT_GRAPH_GROUP_ID_ANNOTATION
+          ] === 'grp-parent',
+      );
+      expect(parentEntity!.entity.spec?.children).toHaveLength(0);
+    });
+
    it('returns done:false with groups cursor when nextLink is present', async () => {
      const nextLink =
        'https://graph.microsoft.com/v1.0/groups?$skiptoken=page2';
@@ -274,6 +274,14 @@ export class MicrosoftGraphIncrementalEntityProvider
      );
    }

+    if (provider.groupIncludeSubGroups) {
+      this.options.logger.warn(
+        `${this.getProviderName()}: groupIncludeSubGroups is not supported by ` +
+          `MicrosoftGraphIncrementalEntityProvider and will be ignored. ` +
+          `Switch to MicrosoftGraphOrgEntityProvider if you require this option.`,
+      );
+    }
+
    const client = MicrosoftGraphClient.create(provider);
    await burst({ client, provider });
  }
@@ -321,9 +329,9 @@ export class MicrosoftGraphIncrementalEntityProvider
      rawUsers.map(user =>
        limiter(async () => {
          let userPhoto: string | undefined;
-          if (provider.loadUserPhotos !== false) {
+          if (user.id && provider.loadUserPhotos !== false) {
            try {
-              userPhoto = await getUserPhotoGated(client, user.id!, 120);
+              userPhoto = await getUserPhotoGated(client, user.id, 120);
            } catch (e) {
              this.options.logger.debug(
                `${this.getProviderName()}: failed to load photo for user ${
@@ -469,14 +477,19 @@ export class MicrosoftGraphIncrementalEntityProvider
                );
              }
            } else if (member['@odata.type'] === '#microsoft.graph.group') {
-              const childEntity = await groupTransformer(
-                member as MicrosoftGraph.Group,
-              );
-              if (childEntity) {
-                childEntity.metadata.name = capEntityName(
-                  childEntity.metadata.name,
+              // Only emit child refs when no group filter/search is active.
+              // With a filter, child groups may not be ingested themselves,
+              // which would produce dangling spec.children references.
+              if (!provider.groupFilter && !provider.groupSearch) {
+                const childEntity = await groupTransformer(
+                  member as MicrosoftGraph.Group,
                );
-                childRefs.push(stringifyEntityRef(childEntity));
+                if (childEntity) {
+                  childEntity.metadata.name = capEntityName(
+                    childEntity.metadata.name,
+                  );
+                  childRefs.push(stringifyEntityRef(childEntity));
+                }
              }
            }
          }