From b7bfdfa8b20fa53e80268fc4bd070348ab5febf7 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 21 Feb 2026 18:06:02 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Handle=20Delimited=20Stri?= =?UTF-8?q?ng=20Role=20Claims=20in=20OpenID=20Strategy=20(#11892)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- api/strategies/openidStrategy.js | 15 +++-- api/strategies/openidStrategy.spec.js | 96 +++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 198c8735ae..15e21f67ef 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -451,7 +451,7 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) { throw new Error(`You must have ${rolesList} role to log in.`); } - const roleValues = Array.isArray(roles) ? roles : [roles]; + const roleValues = Array.isArray(roles) ? roles : roles.split(/[\s,]+/).filter(Boolean); if (!requiredRoles.some((role) => roleValues.includes(role))) { const rolesList = @@ -524,13 +524,14 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) { } const adminRoles = get(adminRoleObject, adminRoleParameterPath); + let adminRoleValues = []; + if (Array.isArray(adminRoles)) { + adminRoleValues = adminRoles; + } else if (typeof adminRoles === 'string') { + adminRoleValues = adminRoles.split(/[\s,]+/).filter(Boolean); + } - if ( - adminRoles && - (adminRoles === true || - adminRoles === adminRole || - (Array.isArray(adminRoles) && adminRoles.includes(adminRole))) - ) { + if (adminRoles && (adminRoles === true || adminRoleValues.includes(adminRole))) { user.role = SystemRoles.ADMIN; logger.info(`[openidStrategy] User ${username} is an admin based on role: ${adminRole}`); } else if (user.role === SystemRoles.ADMIN) { diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index b1dc54d77b..00c65106ad 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -384,6 +384,62 @@ describe('setupOpenId', () => { expect(details.message).toBe('You must have "read" role to log in.'); }); + it('should allow login when roles claim is a space-separated string containing the required role', async () => { + // Arrange – IdP returns roles as a space-delimited string + jwtDecode.mockReturnValue({ + roles: 'role1 role2 requiredRole', + }); + + // Act + const { user } = await validate(tokenset); + + // Assert – login succeeds when required role is present after splitting + expect(user).toBeTruthy(); + expect(createUser).toHaveBeenCalled(); + }); + + it('should allow login when roles claim is a comma-separated string containing the required role', async () => { + // Arrange – IdP returns roles as a comma-delimited string + jwtDecode.mockReturnValue({ + roles: 'role1,role2,requiredRole', + }); + + // Act + const { user } = await validate(tokenset); + + // Assert – login succeeds when required role is present after splitting + expect(user).toBeTruthy(); + expect(createUser).toHaveBeenCalled(); + }); + + it('should allow login when roles claim is a mixed comma-and-space-separated string containing the required role', async () => { + // Arrange – IdP returns roles with comma-and-space delimiters + jwtDecode.mockReturnValue({ + roles: 'role1, role2, requiredRole', + }); + + // Act + const { user } = await validate(tokenset); + + // Assert – login succeeds when required role is present after splitting + expect(user).toBeTruthy(); + expect(createUser).toHaveBeenCalled(); + }); + + it('should reject login when roles claim is a space-separated string that does not contain the required role', async () => { + // Arrange – IdP returns a delimited string but required role is absent + jwtDecode.mockReturnValue({ + roles: 'role1 role2 otherRole', + }); + + // Act + const { user, details } = await validate(tokenset); + + // Assert – login is rejected with the correct error message + expect(user).toBe(false); + expect(details.message).toBe('You must have "requiredRole" role to log in.'); + }); + it('should allow login when single required role is present (backward compatibility)', async () => { // Arrange – ensure single role configuration (as set in beforeEach) // OPENID_REQUIRED_ROLE = 'requiredRole' @@ -1182,6 +1238,46 @@ describe('setupOpenId', () => { expect(user.role).toBeUndefined(); }); + it('should grant admin when admin role claim is a space-separated string containing the admin role', async () => { + // Arrange – IdP returns admin roles as a space-delimited string + process.env.OPENID_ADMIN_ROLE = 'site-admin'; + process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'app_roles'; + + jwtDecode.mockReturnValue({ + roles: ['requiredRole'], + app_roles: 'user site-admin moderator', + }); + + await setupOpenId(); + verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid'); + + // Act + const { user } = await validate(tokenset); + + // Assert – admin role is granted after splitting the delimited string + expect(user.role).toBe('ADMIN'); + }); + + it('should not grant admin when admin role claim is a space-separated string that does not contain the admin role', async () => { + // Arrange – delimited string present but admin role is absent + process.env.OPENID_ADMIN_ROLE = 'site-admin'; + process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'app_roles'; + + jwtDecode.mockReturnValue({ + roles: ['requiredRole'], + app_roles: 'user moderator', + }); + + await setupOpenId(); + verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid'); + + // Act + const { user } = await validate(tokenset); + + // Assert – admin role is not granted + expect(user.role).toBeUndefined(); + }); + it('should handle nested path with special characters in keys', async () => { process.env.OPENID_REQUIRED_ROLE = 'app-user'; process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'resource_access.my-app-123.roles';