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,