Skip to content

Commit b7bfdfa

Browse files
authored
🪪 fix: Handle Delimited String Role Claims in OpenID Strategy (danny-avila#11892)
* fix: handle space/comma-separated string roles claim in OpenID strategy When an OpenID provider returns the roles claim as a delimited string (e.g. "role1 role2 admin"), the previous code wrapped the entire string as a single array element, causing role checks to always fail even for users with the required role. Split string claims on whitespace and commas before comparison so that both array and delimited-string formats are handled correctly. Adds regression tests for space-separated, comma-separated, mixed, and non-matching delimited string cases. * fix: enhance admin role handling in OpenID strategy Updated the OpenID strategy to correctly handle admin roles specified as space-separated or comma-separated strings. The logic now splits these strings into an array for accurate role checks. Added tests to verify that admin roles are granted or denied based on the presence of the specified admin role in the delimited string format.
1 parent cca9d63 commit b7bfdfa

2 files changed

Lines changed: 104 additions & 7 deletions

File tree

api/strategies/openidStrategy.js

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
451451
throw new Error(`You must have ${rolesList} role to log in.`);
452452
}
453453

454-
const roleValues = Array.isArray(roles) ? roles : [roles];
454+
const roleValues = Array.isArray(roles) ? roles : roles.split(/[\s,]+/).filter(Boolean);
455455

456456
if (!requiredRoles.some((role) => roleValues.includes(role))) {
457457
const rolesList =
@@ -524,13 +524,14 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
524524
}
525525

526526
const adminRoles = get(adminRoleObject, adminRoleParameterPath);
527+
let adminRoleValues = [];
528+
if (Array.isArray(adminRoles)) {
529+
adminRoleValues = adminRoles;
530+
} else if (typeof adminRoles === 'string') {
531+
adminRoleValues = adminRoles.split(/[\s,]+/).filter(Boolean);
532+
}
527533

528-
if (
529-
adminRoles &&
530-
(adminRoles === true ||
531-
adminRoles === adminRole ||
532-
(Array.isArray(adminRoles) && adminRoles.includes(adminRole)))
533-
) {
534+
if (adminRoles && (adminRoles === true || adminRoleValues.includes(adminRole))) {
534535
user.role = SystemRoles.ADMIN;
535536
logger.info(`[openidStrategy] User ${username} is an admin based on role: ${adminRole}`);
536537
} else if (user.role === SystemRoles.ADMIN) {

api/strategies/openidStrategy.spec.js

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,62 @@ describe('setupOpenId', () => {
384384
expect(details.message).toBe('You must have "read" role to log in.');
385385
});
386386

387+
it('should allow login when roles claim is a space-separated string containing the required role', async () => {
388+
// Arrange – IdP returns roles as a space-delimited string
389+
jwtDecode.mockReturnValue({
390+
roles: 'role1 role2 requiredRole',
391+
});
392+
393+
// Act
394+
const { user } = await validate(tokenset);
395+
396+
// Assert – login succeeds when required role is present after splitting
397+
expect(user).toBeTruthy();
398+
expect(createUser).toHaveBeenCalled();
399+
});
400+
401+
it('should allow login when roles claim is a comma-separated string containing the required role', async () => {
402+
// Arrange – IdP returns roles as a comma-delimited string
403+
jwtDecode.mockReturnValue({
404+
roles: 'role1,role2,requiredRole',
405+
});
406+
407+
// Act
408+
const { user } = await validate(tokenset);
409+
410+
// Assert – login succeeds when required role is present after splitting
411+
expect(user).toBeTruthy();
412+
expect(createUser).toHaveBeenCalled();
413+
});
414+
415+
it('should allow login when roles claim is a mixed comma-and-space-separated string containing the required role', async () => {
416+
// Arrange – IdP returns roles with comma-and-space delimiters
417+
jwtDecode.mockReturnValue({
418+
roles: 'role1, role2, requiredRole',
419+
});
420+
421+
// Act
422+
const { user } = await validate(tokenset);
423+
424+
// Assert – login succeeds when required role is present after splitting
425+
expect(user).toBeTruthy();
426+
expect(createUser).toHaveBeenCalled();
427+
});
428+
429+
it('should reject login when roles claim is a space-separated string that does not contain the required role', async () => {
430+
// Arrange – IdP returns a delimited string but required role is absent
431+
jwtDecode.mockReturnValue({
432+
roles: 'role1 role2 otherRole',
433+
});
434+
435+
// Act
436+
const { user, details } = await validate(tokenset);
437+
438+
// Assert – login is rejected with the correct error message
439+
expect(user).toBe(false);
440+
expect(details.message).toBe('You must have "requiredRole" role to log in.');
441+
});
442+
387443
it('should allow login when single required role is present (backward compatibility)', async () => {
388444
// Arrange – ensure single role configuration (as set in beforeEach)
389445
// OPENID_REQUIRED_ROLE = 'requiredRole'
@@ -1182,6 +1238,46 @@ describe('setupOpenId', () => {
11821238
expect(user.role).toBeUndefined();
11831239
});
11841240

1241+
it('should grant admin when admin role claim is a space-separated string containing the admin role', async () => {
1242+
// Arrange – IdP returns admin roles as a space-delimited string
1243+
process.env.OPENID_ADMIN_ROLE = 'site-admin';
1244+
process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'app_roles';
1245+
1246+
jwtDecode.mockReturnValue({
1247+
roles: ['requiredRole'],
1248+
app_roles: 'user site-admin moderator',
1249+
});
1250+
1251+
await setupOpenId();
1252+
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
1253+
1254+
// Act
1255+
const { user } = await validate(tokenset);
1256+
1257+
// Assert – admin role is granted after splitting the delimited string
1258+
expect(user.role).toBe('ADMIN');
1259+
});
1260+
1261+
it('should not grant admin when admin role claim is a space-separated string that does not contain the admin role', async () => {
1262+
// Arrange – delimited string present but admin role is absent
1263+
process.env.OPENID_ADMIN_ROLE = 'site-admin';
1264+
process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'app_roles';
1265+
1266+
jwtDecode.mockReturnValue({
1267+
roles: ['requiredRole'],
1268+
app_roles: 'user moderator',
1269+
});
1270+
1271+
await setupOpenId();
1272+
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
1273+
1274+
// Act
1275+
const { user } = await validate(tokenset);
1276+
1277+
// Assert – admin role is not granted
1278+
expect(user.role).toBeUndefined();
1279+
});
1280+
11851281
it('should handle nested path with special characters in keys', async () => {
11861282
process.env.OPENID_REQUIRED_ROLE = 'app-user';
11871283
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'resource_access.my-app-123.roles';

0 commit comments

Comments
 (0)