diff --git a/.changeset/fix-msgraph-group-member-filter.md b/.changeset/fix-msgraph-group-member-filter.md new file mode 100644 index 0000000000..81628dc187 --- /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 (`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/.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..5ec4f0b175 100644 --- a/plugins/catalog-backend-module-msgraph/config.d.ts +++ b/plugins/catalog-backend-module-msgraph/config.d.ts @@ -59,8 +59,9 @@ export interface Config { // they could also be configured "in code". /** - * The filter to apply to extract users. - * Combined with the base `accountEnabled eq true` filter. + * 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'" */ @@ -163,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'" */ @@ -297,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 6d9a6cc332..9a134cd1c3 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,24 +197,41 @@ describe('readProviderConfigs', () => { expect(actual).toEqual(expected); }); - it('should combine custom filter with accountEnabled filter by default', () => { + it('should reject userFilter combined with userGroupMemberFilter', () => { const config = { catalog: { providers: { microsoftGraphOrg: { customProviderId: { tenantId: 'tenantId', - user: { - filter: "userType eq 'member'", - }, + user: { filter: "userType eq 'member'" }, + userGroupMember: { filter: "displayName eq 'Team'" }, }, }, }, }, }; - const [result] = readProviderConfigs(new ConfigReader(config)); - expect(result.userFilter).toBe( - "accountEnabled eq true and (userType eq 'member')", + 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', ); }); diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts index 0eb939022e..a3a0eecf58 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts @@ -61,8 +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. Disabled users + * (`accountEnabled === false`) are always filtered out client-side + * regardless of this setting. * * E.g. "userType eq 'member'" */ @@ -193,9 +194,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 +206,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 +314,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 +345,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 +395,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..a9677ed6b1 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() { @@ -136,6 +137,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -185,6 +187,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, 'advanced', @@ -230,6 +233,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -279,6 +283,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -291,6 +296,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 request default fields including accountEnabled 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( + { select: expect.arrayContaining(['accountEnabled']), 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', () => { @@ -342,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, ); @@ -354,6 +427,78 @@ 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', 'unset_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 request default fields including accountEnabled 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', + { select: expect.arrayContaining(['accountEnabled']), top: 999 }, + undefined, + undefined, + ); + }); + it('should read users from Groups with advanced query mode', async () => { client.getGroups.mockImplementation(getExampleGroups); client.getGroupUserMembers.mockImplementation(getExampleUsers); @@ -403,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, ); @@ -463,6 +608,7 @@ describe('read microsoft graph', () => { 'groupid', { expand: 'manager', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1232,6 +1378,7 @@ describe('read microsoft graph', () => { async function* getExampleUsersEmail() { yield { mail: 'user.name@example.com', + accountEnabled: true, }; } @@ -1265,6 +1412,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: undefined, + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1309,6 +1457,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1351,6 +1500,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { + select: expect.arrayContaining(['accountEnabled']), top: 999, }, undefined, @@ -1390,7 +1540,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledTimes(1); expect(client.getUsers).toHaveBeenCalledWith( { - select: ['mail'], + select: ['mail', 'id', 'accountEnabled'], top: 999, }, undefined, @@ -1460,6 +1610,7 @@ describe('read microsoft graph', () => { id: `userid-${i}`, displayName: 'User Name', mail: 'user.name@example.com', + accountEnabled: true, }; } } @@ -1482,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 4a6c4163d1..783bbcb881 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -42,6 +42,47 @@ 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. +// https://learn.microsoft.com/en-us/graph/api/user-list#optional-query-parameters +const DEFAULT_USER_SELECT = [ + 'businessPhones', + 'displayName', + 'givenName', + 'id', + 'jobTitle', + 'mail', + 'mobilePhone', + 'officeLocation', + 'preferredLanguage', + 'surname', + 'userPrincipalName', +]; + +// 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; + 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( + users: AsyncIterable, +): AsyncIterable { + for await (const user of users) { + if (user.accountEnabled !== false) { + yield user; + } + } +} + export async function readMicrosoftGraphUsers( client: MicrosoftGraphClient, options: { @@ -58,16 +99,18 @@ 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 users = filterDisabledUsers( + client.getUsers( + { + filter: options.userFilter, + expand: options.userExpand, + select: ensureMinimumSelect(options.userSelect), + top: PAGE_SIZE, + }, + options.queryMode, + options.userPath, + options.signal, + ), ); return { @@ -86,7 +129,6 @@ export async function readMicrosoftGraphUsersInGroups( options: { queryMode?: 'basic' | 'advanced'; userExpand?: string; - userFilter?: string; userSelect?: string[]; loadUserPhotos?: boolean; userGroupMemberSearch?: string; @@ -121,16 +163,17 @@ 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, + for await (const user of filterDisabledUsers( + client.getGroupUserMembers( + group.id!, + { + expand: options.userExpand, + select: ensureMinimumSelect(options.userSelect), + top: PAGE_SIZE, + }, + options.queryMode, + options.signal, + ), )) { userGroupMembers.set(user.id!, user); groupMemberCount++; @@ -455,7 +498,6 @@ export async function readMicrosoftGraphOrg( { queryMode: options.queryMode, userExpand: options.userExpand, - userFilter: options.userFilter, userSelect: options.userSelect, userGroupMemberFilter: options.userGroupMemberFilter, userGroupMemberSearch: options.userGroupMemberSearch,