From 8930d771577030abc3e951dbbb3b4bc44a15f8a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 15:24:48 +0200 Subject: [PATCH 1/9] fix(catalog-backend-module-msgraph): filter disabled users client-side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert the server-side accountEnabled base filter from #34165 which broke the group members endpoint. Filter disabled users client-side in both the /users and group members paths. Restore the mutual exclusivity check between userFilter and userGroupMemberFilter. Fixes #34422 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .changeset/fix-msgraph-group-member-filter.md | 5 ++ .patches/pr-34425.txt | 1 + .../config.d.ts | 3 +- .../src/microsoftGraph/config.test.ts | 29 ++------ .../src/microsoftGraph/config.ts | 41 +++++++---- .../src/microsoftGraph/read.test.ts | 2 +- .../src/microsoftGraph/read.ts | 72 +++++++++++++------ 7 files changed, 88 insertions(+), 65 deletions(-) create mode 100644 .changeset/fix-msgraph-group-member-filter.md create mode 100644 .patches/pr-34425.txt diff --git a/.changeset/fix-msgraph-group-member-filter.md b/.changeset/fix-msgraph-group-member-filter.md new file mode 100644 index 0000000000..73dc41b9b8 --- /dev/null +++ b/.changeset/fix-msgraph-group-member-filter.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-msgraph': patch +--- + +Reverted the server-side `accountEnabled eq true` base filter that was added in v1.51.0, which broke the `userGroupMember` path because the group members endpoint doesn't support `$filter` on that property. Disabled users are now filtered client-side in both the `/users` and group members paths. The mutual exclusivity check between `userFilter` and `userGroupMemberFilter` has been restored. diff --git a/.patches/pr-34425.txt b/.patches/pr-34425.txt new file mode 100644 index 0000000000..886a7394f2 --- /dev/null +++ b/.patches/pr-34425.txt @@ -0,0 +1 @@ +Fix msgraph userGroupMember filter error by filtering disabled users client-side diff --git a/plugins/catalog-backend-module-msgraph/config.d.ts b/plugins/catalog-backend-module-msgraph/config.d.ts index dec4532599..bddec4b658 100644 --- a/plugins/catalog-backend-module-msgraph/config.d.ts +++ b/plugins/catalog-backend-module-msgraph/config.d.ts @@ -60,9 +60,8 @@ export interface Config { /** * The filter to apply to extract users. - * Combined with the base `accountEnabled eq true` filter. * - * E.g. "userType eq 'member'" + * E.g. "accountEnabled eq true and userType eq 'member'" */ userFilter?: string; /** diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts index 6d9a6cc332..9fb1f44249 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts @@ -34,7 +34,7 @@ describe('readMicrosoftGraphConfig', () => { id: 'target', target: 'target', tenantId: 'tenantId', - userFilter: 'accountEnabled eq true', + userFilter: undefined, }, ]; expect(actual).toEqual(expected); @@ -69,7 +69,7 @@ describe('readMicrosoftGraphConfig', () => { clientSecret: 'clientSecret', authority: 'https://login.example.com/', userExpand: 'manager', - userFilter: "accountEnabled eq true and (userType eq 'member')", + userFilter: "userType eq 'member'", userSelect: ['id', 'displayName', 'department'], groupExpand: 'member', groupSelect: ['id', 'displayName', 'description'], @@ -123,7 +123,7 @@ describe('readProviderConfigs', () => { id: 'customProviderId', target: 'https://graph.microsoft.com/v1.0', tenantId: 'tenantId', - userFilter: 'accountEnabled eq true', + userFilter: undefined, userPath: 'users', groupPath: 'groups', }, @@ -178,7 +178,7 @@ describe('readProviderConfigs', () => { authority: 'https://login.example.com/', queryMode: 'advanced', userExpand: 'manager', - userFilter: "accountEnabled eq true and (userType eq 'member')", + userFilter: "userType eq 'member'", userSelect: ['id', 'displayName', 'department'], userPath: '/groups/{groupId}/members', groupExpand: 'member', @@ -197,27 +197,6 @@ describe('readProviderConfigs', () => { expect(actual).toEqual(expected); }); - it('should combine custom filter with accountEnabled filter by default', () => { - const config = { - catalog: { - providers: { - microsoftGraphOrg: { - customProviderId: { - tenantId: 'tenantId', - user: { - filter: "userType eq 'member'", - }, - }, - }, - }, - }, - }; - const [result] = readProviderConfigs(new ConfigReader(config)); - expect(result.userFilter).toBe( - "accountEnabled eq true and (userType eq 'member')", - ); - }); - it('should fail if clientId is set without clientSecret', () => { const config = { catalog: { diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts index 0eb939022e..5feabcb9b2 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts @@ -61,10 +61,9 @@ export type MicrosoftGraphProviderConfig = { */ clientSecret?: string; /** - * The filter to apply to extract users. This is combined with the base - * `accountEnabled eq true` filter that is always applied automatically. + * The filter to apply to extract users. * - * E.g. "userType eq 'member'" + * E.g. "accountEnabled eq true and userType eq 'member'" */ userFilter?: string; /** @@ -193,9 +192,7 @@ export function readMicrosoftGraphConfig( const clientSecret = providerConfig.getOptionalString('clientSecret'); const userExpand = providerConfig.getOptionalString('userExpand'); - const userFilter = buildUserFilter( - providerConfig.getOptionalString('userFilter'), - ); + const userFilter = providerConfig.getOptionalString('userFilter'); const userSelect = providerConfig.getOptionalStringArray('userSelect'); const userGroupMemberFilter = providerConfig.getOptionalString( 'userGroupMemberFilter', @@ -207,6 +204,17 @@ export function readMicrosoftGraphConfig( const groupFilter = providerConfig.getOptionalString('groupFilter'); const groupSearch = providerConfig.getOptionalString('groupSearch'); + if (userFilter && userGroupMemberFilter) { + throw new Error( + `userFilter and userGroupMemberFilter are mutually exclusive, only one can be specified.`, + ); + } + if (userFilter && userGroupMemberSearch) { + throw new Error( + `userGroupMemberSearch cannot be specified when userFilter is defined.`, + ); + } + const groupSelect = providerConfig.getOptionalStringArray('groupSelect'); const queryMode = providerConfig.getOptionalString('queryMode'); if ( @@ -304,7 +312,7 @@ export function readProviderConfig( const clientSecret = config.getOptionalString('clientSecret'); const userExpand = config.getOptionalString('user.expand'); - const userFilter = buildUserFilter(config.getOptionalString('user.filter')); + const userFilter = config.getOptionalString('user.filter'); const userSelect = config.getOptionalStringArray('user.select'); const userPath = config.getOptionalString('user.path') ?? 'users'; const loadUserPhotos = config.getOptionalBoolean('user.loadPhotos'); @@ -335,6 +343,17 @@ export function readProviderConfig( ); const userGroupMemberPath = config.getOptionalString('userGroupMember.path'); + if (userFilter && userGroupMemberFilter) { + throw new Error( + `userFilter and userGroupMemberFilter are mutually exclusive, only one can be specified.`, + ); + } + if (userFilter && userGroupMemberSearch) { + throw new Error( + `userGroupMemberSearch cannot be specified when userFilter is defined.`, + ); + } + if (clientId && !clientSecret) { throw new Error(`clientSecret must be provided when clientId is defined.`); } @@ -374,11 +393,3 @@ export function readProviderConfig( schedule, }; } - -function buildUserFilter(rawFilter: string | undefined): string { - const base = 'accountEnabled eq true'; - if (rawFilter) { - return `${base} and (${rawFilter})`; - } - return base; -} diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index d4fe3783db..74d31fd482 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -1390,7 +1390,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { - select: ['mail'], + select: ['mail', 'accountEnabled'], top: 999, }, undefined, diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index 4a6c4163d1..20e48d5388 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -42,6 +42,27 @@ import { LoggerService } from '@backstage/backend-plugin-api'; const PAGE_SIZE = 999; +function ensureSelectContains(select: string[], field: string): string[] { + if ( + select.some( + s => s.toLocaleLowerCase('en-US') === field.toLocaleLowerCase('en-US'), + ) + ) { + return select; + } + return [...select, field]; +} + +async function* filterDisabledUsers( + users: AsyncIterable, +): AsyncIterable { + for await (const user of users) { + if (user.accountEnabled !== false) { + yield user; + } + } +} + export async function readMicrosoftGraphUsers( client: MicrosoftGraphClient, options: { @@ -58,16 +79,21 @@ export async function readMicrosoftGraphUsers( ): Promise<{ users: UserEntity[]; // With all relations empty }> { - const users = client.getUsers( - { - filter: options.userFilter, - expand: options.userExpand, - select: options.userSelect, - top: PAGE_SIZE, - }, - options.queryMode, - options.userPath, - options.signal, + const select = options.userSelect + ? ensureSelectContains(options.userSelect, 'accountEnabled') + : undefined; + const users = filterDisabledUsers( + client.getUsers( + { + filter: options.userFilter, + expand: options.userExpand, + select, + top: PAGE_SIZE, + }, + options.queryMode, + options.userPath, + options.signal, + ), ); return { @@ -86,7 +112,6 @@ export async function readMicrosoftGraphUsersInGroups( options: { queryMode?: 'basic' | 'advanced'; userExpand?: string; - userFilter?: string; userSelect?: string[]; loadUserPhotos?: boolean; userGroupMemberSearch?: string; @@ -121,16 +146,20 @@ export async function readMicrosoftGraphUsersInGroups( userGroupMemberPromises.push( limiter(async () => { let groupMemberCount = 0; - for await (const user of client.getGroupUserMembers( - group.id!, - { - expand: options.userExpand, - filter: options.userFilter, - select: options.userSelect, - top: PAGE_SIZE, - }, - options.queryMode, - options.signal, + const memberSelect = options.userSelect + ? ensureSelectContains(options.userSelect, 'accountEnabled') + : undefined; + for await (const user of filterDisabledUsers( + client.getGroupUserMembers( + group.id!, + { + expand: options.userExpand, + select: memberSelect, + top: PAGE_SIZE, + }, + options.queryMode, + options.signal, + ), )) { userGroupMembers.set(user.id!, user); groupMemberCount++; @@ -455,7 +484,6 @@ export async function readMicrosoftGraphOrg( { queryMode: options.queryMode, userExpand: options.userExpand, - userFilter: options.userFilter, userSelect: options.userSelect, userGroupMemberFilter: options.userGroupMemberFilter, userGroupMemberSearch: options.userGroupMemberSearch, From ce2e763606b7b232ce74fc84db8c2aefb36be785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 15:33:08 +0200 Subject: [PATCH 2/9] Address review: use === true, update docs, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use accountEnabled === true instead of !== false (stricter, excludes users with unset accountEnabled) - Update doc comments to mention client-side filtering and remove accountEnabled from the filter example - Update changeset to mention userGroupMemberSearch exclusivity - Add accountEnabled: true to test mocks - Add regression tests for disabled user filtering Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .changeset/fix-msgraph-group-member-filter.md | 2 +- .../config.d.ts | 6 +- .../src/microsoftGraph/config.ts | 6 +- .../src/microsoftGraph/read.test.ts | 57 +++++++++++++++++++ .../src/microsoftGraph/read.ts | 2 +- 5 files changed, 67 insertions(+), 6 deletions(-) diff --git a/.changeset/fix-msgraph-group-member-filter.md b/.changeset/fix-msgraph-group-member-filter.md index 73dc41b9b8..a56b30a9af 100644 --- a/.changeset/fix-msgraph-group-member-filter.md +++ b/.changeset/fix-msgraph-group-member-filter.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend-module-msgraph': patch --- -Reverted the server-side `accountEnabled eq true` base filter that was added in v1.51.0, which broke the `userGroupMember` path because the group members endpoint doesn't support `$filter` on that property. Disabled users are now filtered client-side in both the `/users` and group members paths. The mutual exclusivity check between `userFilter` and `userGroupMemberFilter` has been restored. +Reverted the server-side `accountEnabled eq true` base filter that was added in v1.51.0, which broke the `userGroupMember` path because the group members endpoint doesn't support `$filter` on that property. Disabled users (`accountEnabled !== true`) are now filtered client-side in both the `/users` and group members paths. The mutual exclusivity checks between `userFilter` and `userGroupMemberFilter`/`userGroupMemberSearch` have been restored. diff --git a/plugins/catalog-backend-module-msgraph/config.d.ts b/plugins/catalog-backend-module-msgraph/config.d.ts index bddec4b658..6553484bda 100644 --- a/plugins/catalog-backend-module-msgraph/config.d.ts +++ b/plugins/catalog-backend-module-msgraph/config.d.ts @@ -59,9 +59,11 @@ export interface Config { // they could also be configured "in code". /** - * The filter to apply to extract users. + * The filter to apply to extract users. Disabled users + * (accountEnabled !== true) are always filtered out client-side + * regardless of this setting. * - * E.g. "accountEnabled eq true and userType eq 'member'" + * E.g. "userType eq 'member'" */ userFilter?: string; /** diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts index 5feabcb9b2..4f82bcd1f3 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts @@ -61,9 +61,11 @@ export type MicrosoftGraphProviderConfig = { */ clientSecret?: string; /** - * The filter to apply to extract users. + * The filter to apply to extract users. Disabled users (accountEnabled + * !== true) are always filtered out client-side regardless of this + * setting. * - * E.g. "accountEnabled eq true and userType eq 'member'" + * E.g. "userType eq 'member'" */ userFilter?: string; /** diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index 74d31fd482..af21e0aa20 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -76,6 +76,7 @@ describe('read microsoft graph', () => { id: 'userid', displayName: 'User Name', mail: 'user.name@example.com', + accountEnabled: true, }; } async function* getExampleGroups() { @@ -354,6 +355,60 @@ describe('read microsoft graph', () => { ); }); + it('should skip disabled users fetched via group membership', async () => { + client.getGroups.mockImplementation(getExampleGroups); + client.getGroupUserMembers.mockImplementation(async function* () { + yield { + id: 'enabled-user', + displayName: 'Enabled User', + mail: 'enabled@example.com', + accountEnabled: true, + }; + yield { + id: 'disabled-user', + displayName: 'Disabled User', + mail: 'disabled@example.com', + accountEnabled: false, + }; + yield { + id: 'unset-user', + displayName: 'Unset User', + mail: 'unset@example.com', + }; + }); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + const { users } = await readMicrosoftGraphUsersInGroups(client, { + userGroupMemberFilter: 'securityEnabled eq true', + logger: mockServices.logger.mock(), + }); + + const names = users.map(u => u.metadata.name); + expect(names).toEqual(['enabled_example.com']); + }); + + it('should include accountEnabled in select when userSelect is set', async () => { + client.getGroups.mockImplementation(getExampleGroups); + client.getGroupUserMembers.mockImplementation(getExampleUsers); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + await readMicrosoftGraphUsersInGroups(client, { + userGroupMemberFilter: 'securityEnabled eq true', + userSelect: ['id', 'displayName', 'mail'], + logger: mockServices.logger.mock(), + }); + + expect(client.getGroupUserMembers).toHaveBeenCalledWith( + 'groupid', + { + select: ['id', 'displayName', 'mail', 'accountEnabled'], + top: 999, + }, + undefined, + undefined, + ); + }); + it('should read users from Groups with advanced query mode', async () => { client.getGroups.mockImplementation(getExampleGroups); client.getGroupUserMembers.mockImplementation(getExampleUsers); @@ -1232,6 +1287,7 @@ describe('read microsoft graph', () => { async function* getExampleUsersEmail() { yield { mail: 'user.name@example.com', + accountEnabled: true, }; } @@ -1460,6 +1516,7 @@ describe('read microsoft graph', () => { id: `userid-${i}`, displayName: 'User Name', mail: 'user.name@example.com', + accountEnabled: true, }; } } diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index 20e48d5388..efb1f737ad 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -57,7 +57,7 @@ async function* filterDisabledUsers( users: AsyncIterable, ): AsyncIterable { for await (const user of users) { - if (user.accountEnabled !== false) { + if (user.accountEnabled === true) { yield user; } } From 5b07b4ef04ca688b840500df228c8496289bd4e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 15:37:20 +0200 Subject: [PATCH 3/9] Simplify ensureSelectContains to accept undefined, inline calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accept undefined input (returns undefined, preserving default projection). Inline the calls at both use sites per review feedback. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/microsoftGraph/read.ts | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index efb1f737ad..dba8cd6f64 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -42,7 +42,13 @@ import { LoggerService } from '@backstage/backend-plugin-api'; const PAGE_SIZE = 999; -function ensureSelectContains(select: string[], field: string): string[] { +function ensureSelectContains( + select: string[] | undefined, + field: string, +): string[] | undefined { + if (!select) { + return undefined; + } if ( select.some( s => s.toLocaleLowerCase('en-US') === field.toLocaleLowerCase('en-US'), @@ -79,15 +85,12 @@ export async function readMicrosoftGraphUsers( ): Promise<{ users: UserEntity[]; // With all relations empty }> { - const select = options.userSelect - ? ensureSelectContains(options.userSelect, 'accountEnabled') - : undefined; const users = filterDisabledUsers( client.getUsers( { filter: options.userFilter, expand: options.userExpand, - select, + select: ensureSelectContains(options.userSelect, 'accountEnabled'), top: PAGE_SIZE, }, options.queryMode, @@ -146,15 +149,15 @@ export async function readMicrosoftGraphUsersInGroups( userGroupMemberPromises.push( limiter(async () => { let groupMemberCount = 0; - const memberSelect = options.userSelect - ? ensureSelectContains(options.userSelect, 'accountEnabled') - : undefined; for await (const user of filterDisabledUsers( client.getGroupUserMembers( group.id!, { expand: options.userExpand, - select: memberSelect, + select: ensureSelectContains( + options.userSelect, + 'accountEnabled', + ), top: PAGE_SIZE, }, options.queryMode, From 72152935efef32e110c8a97044e28da8fb63f51d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 15:53:25 +0200 Subject: [PATCH 4/9] Use !== false for safer disabled user filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The === true check would silently drop all users if accountEnabled is not in the API response (e.g. if the default projection omits it). The !== false check is safer: it only filters users that are explicitly disabled, and passes through users with unset accountEnabled (same as pre-v1.51.0 behavior). Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/microsoftGraph/read.test.ts | 2 +- .../catalog-backend-module-msgraph/src/microsoftGraph/read.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index af21e0aa20..1465024560 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -384,7 +384,7 @@ describe('read microsoft graph', () => { }); const names = users.map(u => u.metadata.name); - expect(names).toEqual(['enabled_example.com']); + expect(names).toEqual(['enabled_example.com', 'unset_example.com']); }); it('should include accountEnabled in select when userSelect is set', async () => { diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index dba8cd6f64..fc6d78fea3 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -63,7 +63,7 @@ async function* filterDisabledUsers( users: AsyncIterable, ): AsyncIterable { for await (const user of users) { - if (user.accountEnabled === true) { + if (user.accountEnabled !== false) { yield user; } } From 42a907dfe03fe657bcd47fedb7e5874060551acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 15:55:33 +0200 Subject: [PATCH 5/9] Add test coverage for both user paths with and without userSelect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cover the disabled user filtering and select augmentation for both the /users and group members paths, with and without userSelect configured. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/microsoftGraph/read.test.ts | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index 1465024560..db10f65f9a 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -292,6 +292,74 @@ describe('read microsoft graph', () => { 120, ); }); + + it('should skip disabled users', async () => { + client.getUsers.mockImplementation(async function* () { + yield { + id: 'enabled', + displayName: 'Enabled', + mail: 'enabled@example.com', + accountEnabled: true, + }; + yield { + id: 'disabled', + displayName: 'Disabled', + mail: 'disabled@example.com', + accountEnabled: false, + }; + yield { + id: 'unset', + displayName: 'Unset', + mail: 'unset@example.com', + }; + }); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + const { users } = await readMicrosoftGraphUsers(client, { + logger: mockServices.logger.mock(), + }); + + expect(users.map(u => u.metadata.name)).toEqual([ + 'enabled_example.com', + 'unset_example.com', + ]); + }); + + it('should not pass select when userSelect is not configured', async () => { + client.getUsers.mockImplementation(getExampleUsers); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + await readMicrosoftGraphUsers(client, { + logger: mockServices.logger.mock(), + }); + + expect(client.getUsers).toHaveBeenCalledWith( + { top: 999 }, + undefined, + undefined, + undefined, + ); + }); + + it('should add accountEnabled to select when userSelect is configured', async () => { + client.getUsers.mockImplementation(getExampleUsers); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + await readMicrosoftGraphUsers(client, { + userSelect: ['id', 'displayName', 'mail'], + logger: mockServices.logger.mock(), + }); + + expect(client.getUsers).toHaveBeenCalledWith( + { + select: ['id', 'displayName', 'mail', 'accountEnabled'], + top: 999, + }, + undefined, + undefined, + undefined, + ); + }); }); describe('readMicrosoftGraphUsersInGroups', () => { @@ -409,6 +477,24 @@ describe('read microsoft graph', () => { ); }); + it('should not pass select when userSelect is not configured', async () => { + client.getGroups.mockImplementation(getExampleGroups); + client.getGroupUserMembers.mockImplementation(getExampleUsers); + client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); + + await readMicrosoftGraphUsersInGroups(client, { + userGroupMemberFilter: 'securityEnabled eq true', + logger: mockServices.logger.mock(), + }); + + expect(client.getGroupUserMembers).toHaveBeenCalledWith( + 'groupid', + { top: 999 }, + undefined, + undefined, + ); + }); + it('should read users from Groups with advanced query mode', async () => { client.getGroups.mockImplementation(getExampleGroups); client.getGroupUserMembers.mockImplementation(getExampleUsers); From 44f017668be72a31fae8f8c8402726948c92687c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 16:53:20 +0200 Subject: [PATCH 6/9] Fix remaining stale docs and add mutual exclusivity tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix two remaining config.d.ts entries that still referenced the automatic accountEnabled base filter - Add tests verifying that userFilter + userGroupMemberFilter and userFilter + userGroupMemberSearch are rejected Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../config.d.ts | 6 ++- .../src/microsoftGraph/config.test.ts | 38 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/config.d.ts b/plugins/catalog-backend-module-msgraph/config.d.ts index 6553484bda..b1252a3a90 100644 --- a/plugins/catalog-backend-module-msgraph/config.d.ts +++ b/plugins/catalog-backend-module-msgraph/config.d.ts @@ -164,7 +164,8 @@ export interface Config { expand?: string; /** * The filter to apply to extract users. - * Combined with the base `accountEnabled eq true` filter + * Disabled users (accountEnabled === false) are always + * filtered out client-side regardless of this setting. * * E.g. "userType eq 'member'" */ @@ -298,7 +299,8 @@ export interface Config { expand?: string; /** * The filter to apply to extract users. - * Combined with the base `accountEnabled eq true` filter + * Disabled users (accountEnabled === false) are always + * filtered out client-side regardless of this setting. * * E.g. "userType eq 'member'" */ diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts index 9fb1f44249..9a134cd1c3 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts @@ -197,6 +197,44 @@ describe('readProviderConfigs', () => { expect(actual).toEqual(expected); }); + it('should reject userFilter combined with userGroupMemberFilter', () => { + const config = { + catalog: { + providers: { + microsoftGraphOrg: { + customProviderId: { + tenantId: 'tenantId', + user: { filter: "userType eq 'member'" }, + userGroupMember: { filter: "displayName eq 'Team'" }, + }, + }, + }, + }, + }; + expect(() => readProviderConfigs(new ConfigReader(config))).toThrow( + 'mutually exclusive', + ); + }); + + it('should reject userFilter combined with userGroupMemberSearch', () => { + const config = { + catalog: { + providers: { + microsoftGraphOrg: { + customProviderId: { + tenantId: 'tenantId', + user: { filter: "userType eq 'member'" }, + userGroupMember: { search: '"displayName:team"' }, + }, + }, + }, + }, + }; + expect(() => readProviderConfigs(new ConfigReader(config))).toThrow( + 'userGroupMemberSearch cannot be specified', + ); + }); + it('should fail if clientId is set without clientSecret', () => { const config = { catalog: { From bbb9ca105fb1027b3988e77cc6404eadf44c6dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 17:02:02 +0200 Subject: [PATCH 7/9] Fix inconsistent accountEnabled wording across docs and changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consistently use 'accountEnabled === false' everywhere to describe which users are filtered out. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .changeset/fix-msgraph-group-member-filter.md | 2 +- plugins/catalog-backend-module-msgraph/config.d.ts | 8 ++++---- .../src/microsoftGraph/config.ts | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.changeset/fix-msgraph-group-member-filter.md b/.changeset/fix-msgraph-group-member-filter.md index a56b30a9af..81628dc187 100644 --- a/.changeset/fix-msgraph-group-member-filter.md +++ b/.changeset/fix-msgraph-group-member-filter.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend-module-msgraph': patch --- -Reverted the server-side `accountEnabled eq true` base filter that was added in v1.51.0, which broke the `userGroupMember` path because the group members endpoint doesn't support `$filter` on that property. Disabled users (`accountEnabled !== true`) are now filtered client-side in both the `/users` and group members paths. The mutual exclusivity checks between `userFilter` and `userGroupMemberFilter`/`userGroupMemberSearch` have been restored. +Reverted the server-side `accountEnabled eq true` base filter that was added in v1.51.0, which broke the `userGroupMember` path because the group members endpoint doesn't support `$filter` on that property. Disabled users (`accountEnabled === false`) are now filtered client-side in both the `/users` and group members paths. The mutual exclusivity checks between `userFilter` and `userGroupMemberFilter`/`userGroupMemberSearch` have been restored. diff --git a/plugins/catalog-backend-module-msgraph/config.d.ts b/plugins/catalog-backend-module-msgraph/config.d.ts index b1252a3a90..5ec4f0b175 100644 --- a/plugins/catalog-backend-module-msgraph/config.d.ts +++ b/plugins/catalog-backend-module-msgraph/config.d.ts @@ -60,8 +60,8 @@ export interface Config { /** * The filter to apply to extract users. Disabled users - * (accountEnabled !== true) are always filtered out client-side - * regardless of this setting. + * (`accountEnabled === false`) are always filtered out + * client-side regardless of this setting. * * E.g. "userType eq 'member'" */ @@ -164,7 +164,7 @@ export interface Config { expand?: string; /** * The filter to apply to extract users. - * Disabled users (accountEnabled === false) are always + * Disabled users (`accountEnabled === false`) are always * filtered out client-side regardless of this setting. * * E.g. "userType eq 'member'" @@ -299,7 +299,7 @@ export interface Config { expand?: string; /** * The filter to apply to extract users. - * Disabled users (accountEnabled === false) are always + * Disabled users (`accountEnabled === false`) are always * filtered out client-side regardless of this setting. * * E.g. "userType eq 'member'" diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts index 4f82bcd1f3..a3a0eecf58 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts @@ -61,9 +61,9 @@ export type MicrosoftGraphProviderConfig = { */ clientSecret?: string; /** - * The filter to apply to extract users. Disabled users (accountEnabled - * !== true) are always filtered out client-side regardless of this - * setting. + * The filter to apply to extract users. Disabled users + * (`accountEnabled === false`) are always filtered out client-side + * regardless of this setting. * * E.g. "userType eq 'member'" */ From 1fc84c8df87275942daf1eec07d460410fb3594b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 28 May 2026 17:24:37 +0200 Subject: [PATCH 8/9] Always request accountEnabled in $select MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Microsoft Graph API does NOT include accountEnabled in the default user projection — it requires an explicit $select. Without it, the client-side filterDisabledUsers check cannot work because the field is undefined in the response. Add a DEFAULT_USER_SELECT constant with all default Graph API user fields plus accountEnabled. ensureSelectContains now falls back to this list when no custom userSelect is configured. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/microsoftGraph/read.test.ts | 21 ++++++++---- .../src/microsoftGraph/read.ts | 32 +++++++++++++++---- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index db10f65f9a..7c697fd837 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -137,6 +137,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -186,6 +187,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, 'advanced', @@ -231,6 +233,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -280,6 +283,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -325,7 +329,7 @@ describe('read microsoft graph', () => { ]); }); - it('should not pass select when userSelect is not configured', async () => { + it('should request default fields including accountEnabled when userSelect is not configured', async () => { client.getUsers.mockImplementation(getExampleUsers); client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); @@ -334,7 +338,7 @@ describe('read microsoft graph', () => { }); expect(client.getUsers).toHaveBeenCalledWith( - { top: 999 }, + { select: expect.arrayContaining(['accountEnabled']), top: 999 }, undefined, undefined, undefined, @@ -411,7 +415,7 @@ describe('read microsoft graph', () => { expect(client.getGroupUserMembers).toHaveBeenCalledTimes(1); expect(client.getGroupUserMembers).toHaveBeenCalledWith( 'groupid', - { top: 999 }, + { select: expect.arrayContaining(['accountEnabled']), top: 999 }, undefined, undefined, ); @@ -477,7 +481,7 @@ describe('read microsoft graph', () => { ); }); - it('should not pass select when userSelect is not configured', async () => { + it('should request default fields including accountEnabled when userSelect is not configured', async () => { client.getGroups.mockImplementation(getExampleGroups); client.getGroupUserMembers.mockImplementation(getExampleUsers); client.getUserPhotoWithSizeLimit.mockResolvedValue(undefined); @@ -489,7 +493,7 @@ describe('read microsoft graph', () => { expect(client.getGroupUserMembers).toHaveBeenCalledWith( 'groupid', - { top: 999 }, + { select: expect.arrayContaining(['accountEnabled']), top: 999 }, undefined, undefined, ); @@ -544,7 +548,7 @@ describe('read microsoft graph', () => { expect(client.getGroupUserMembers).toHaveBeenCalledTimes(1); expect(client.getGroupUserMembers).toHaveBeenCalledWith( 'groupid', - { top: 999 }, + { select: expect.arrayContaining(['accountEnabled']), top: 999 }, 'advanced', undefined, ); @@ -604,6 +608,7 @@ describe('read microsoft graph', () => { 'groupid', { expand: 'manager', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1407,6 +1412,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: undefined, + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1451,6 +1457,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1493,6 +1500,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1625,6 +1633,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index fc6d78fea3..678c762c4a 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -42,21 +42,39 @@ import { LoggerService } from '@backstage/backend-plugin-api'; const PAGE_SIZE = 999; +// The default properties returned by the Microsoft Graph API for User +// objects when no $select is specified. accountEnabled is NOT included +// in this set — it requires an explicit $select. We always request it +// so that filterDisabledUsers can work. +// https://learn.microsoft.com/en-us/graph/api/user-list#optional-query-parameters +const DEFAULT_USER_SELECT = [ + 'accountEnabled', + 'businessPhones', + 'displayName', + 'givenName', + 'id', + 'jobTitle', + 'mail', + 'mobilePhone', + 'officeLocation', + 'preferredLanguage', + 'surname', + 'userPrincipalName', +]; + function ensureSelectContains( select: string[] | undefined, field: string, -): string[] | undefined { - if (!select) { - return undefined; - } +): string[] { + const base = select ?? DEFAULT_USER_SELECT; if ( - select.some( + base.some( s => s.toLocaleLowerCase('en-US') === field.toLocaleLowerCase('en-US'), ) ) { - return select; + return base; } - return [...select, field]; + return [...base, field]; } async function* filterDisabledUsers( From 39e0c4f155d893ad046fae3be7cb8831fa116f6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 29 May 2026 10:11:42 +0200 Subject: [PATCH 9/9] Introduce MINIMUM_USER_SELECT with id and accountEnabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace ensureSelectContains with ensureMinimumSelect that adds all fields our code requires (id for photo fetching and Map keys, accountEnabled for disabled user filtering). Separates the minimum viable set from the default projection list. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Fredrik Adelöw --- .../src/microsoftGraph/read.test.ts | 2 +- .../src/microsoftGraph/read.ts | 33 ++++++++----------- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index 7c697fd837..a9677ed6b1 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -1540,7 +1540,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { - select: ['mail', 'accountEnabled'], + select: ['mail', 'id', 'accountEnabled'], top: 999, }, undefined, diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index 678c762c4a..783bbcb881 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -44,11 +44,9 @@ const PAGE_SIZE = 999; // The default properties returned by the Microsoft Graph API for User // objects when no $select is specified. accountEnabled is NOT included -// in this set — it requires an explicit $select. We always request it -// so that filterDisabledUsers can work. +// in this set — it requires an explicit $select. // https://learn.microsoft.com/en-us/graph/api/user-list#optional-query-parameters const DEFAULT_USER_SELECT = [ - 'accountEnabled', 'businessPhones', 'displayName', 'givenName', @@ -62,19 +60,17 @@ const DEFAULT_USER_SELECT = [ 'userPrincipalName', ]; -function ensureSelectContains( - select: string[] | undefined, - field: string, -): string[] { +// Fields that our code requires regardless of what the user or default +// projection provides. These are always added to the $select list. +const MINIMUM_USER_SELECT = ['id', 'accountEnabled']; + +function ensureMinimumSelect(select: string[] | undefined): string[] { const base = select ?? DEFAULT_USER_SELECT; - if ( - base.some( - s => s.toLocaleLowerCase('en-US') === field.toLocaleLowerCase('en-US'), - ) - ) { - return base; - } - return [...base, field]; + const lower = new Set(base.map(s => s.toLocaleLowerCase('en-US'))); + const missing = MINIMUM_USER_SELECT.filter( + f => !lower.has(f.toLocaleLowerCase('en-US')), + ); + return missing.length > 0 ? [...base, ...missing] : base; } async function* filterDisabledUsers( @@ -108,7 +104,7 @@ export async function readMicrosoftGraphUsers( { filter: options.userFilter, expand: options.userExpand, - select: ensureSelectContains(options.userSelect, 'accountEnabled'), + select: ensureMinimumSelect(options.userSelect), top: PAGE_SIZE, }, options.queryMode, @@ -172,10 +168,7 @@ export async function readMicrosoftGraphUsersInGroups( group.id!, { expand: options.userExpand, - select: ensureSelectContains( - options.userSelect, - 'accountEnabled', - ), + select: ensureMinimumSelect(options.userSelect), top: PAGE_SIZE, }, options.queryMode,