paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Merge pull request #34425 from backstage/freben/fix-msgraph-group-member-filter

Browse Source 
fix(catalog-backend-module-msgraph): filter disabled group members client-side
This commit is contained in:
Fredrik Adelöw
2026-05-29 10:41:55 +02:00
committed by GitHub
parent 854c88cde4 39e0c4f155
commit 8f7c02a525
7 changed files with 287 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/fix-msgraph-group-member-filter.md
+5
View File
@@ -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.
.patches/pr-34425.txt
+1
View File
@@ -0,0 +1 @@
Fix msgraph userGroupMember filter error by filtering disabled users client-side
plugins/catalog-backend-module-msgraph/config.d.ts
Vendored
+7 -4
View File
@@ -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'"
*/
plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.test.ts
+28 -11
View File
@@ -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',
);
});
plugins/catalog-backend-module-msgraph/src/microsoftGraph/config.ts
+27 -14
View File
@@ -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;
}
plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts
+155 -3
View File
@@ -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,
plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts
+64 -22
View File
@@ -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<MicrosoftGraph.User>,
): AsyncIterable<MicrosoftGraph.User> {
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,