From d83826b604baeebd5250d7f42ece0b7e3b3dbd36 Mon Sep 17 00:00:00 2001 From: Ihsan Soydemir Date: Tue, 23 Sep 2025 16:39:34 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=90=20feat:=20Support=20Multiple=20Rol?= =?UTF-8?q?es=20in=20`OPENID=5FREQUIRED=5FROLE`=20(#9171)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: support multiple roles in OPENID_REQUIRED_ROLE - Allow comma-separated roles in OPENID_REQUIRED_ROLE environment variable - User needs ANY of the specified roles to login (OR logic) - Maintain backward compatibility with single role configuration - Add comprehensive test coverage for multiple role scenarios * Add tests * Fix linter * Add missing closing brace * Add new line * Simplify tests * Refresh OpenID verify callback in tests * Fix OpenID spec and resolve linting errors * test: Add backward compatibility test for single required role in OpenID strategy --------- Co-authored-by: Danny Avila --- api/strategies/openidStrategy.js | 12 ++++- api/strategies/openidStrategy.spec.js | 72 ++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 011676ecad..6a4e783d4a 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -371,6 +371,10 @@ async function setupOpenId() { const fullName = getFullName(userinfo); if (requiredRole) { + const requiredRoles = requiredRole + .split(',') + .map((role) => role.trim()) + .filter(Boolean); let decodedToken = ''; if (requiredRoleTokenKind === 'access') { decodedToken = jwtDecode(tokenset.access_token); @@ -393,9 +397,13 @@ async function setupOpenId() { ); } - if (!roles.includes(requiredRole)) { + if (!requiredRoles.some((role) => roles.includes(role))) { + const rolesList = + requiredRoles.length === 1 + ? `"${requiredRoles[0]}"` + : `one of: ${requiredRoles.map((r) => `"${r}"`).join(', ')}`; return done(null, false, { - message: `You must have the "${requiredRole}" role to log in.`, + message: `You must have ${rolesList} role to log in.`, }); } } diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index de033b01f8..35841e3783 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -338,7 +338,25 @@ describe('setupOpenId', () => { // Assert – verify that the strategy rejects login expect(user).toBe(false); - expect(details.message).toBe('You must have the "requiredRole" role to log in.'); + 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' + // Default jwtDecode mock in beforeEach already returns this role + jwtDecode.mockReturnValue({ + roles: ['requiredRole', 'anotherRole'], + }); + + // Act + const { user } = await validate(tokenset); + + // Assert – verify that login succeeds with single role configuration + expect(user).toBeTruthy(); + expect(user.email).toBe(tokenset.claims().email); + expect(user.username).toBe(tokenset.claims().preferred_username); + expect(createUser).toHaveBeenCalled(); }); it('should attempt to download and save the avatar if picture is provided', async () => { @@ -364,6 +382,58 @@ describe('setupOpenId', () => { // Depending on your implementation, user.avatar may be undefined or an empty string. }); + it('should support comma-separated multiple roles', async () => { + // Arrange + process.env.OPENID_REQUIRED_ROLE = 'someRole,anotherRole,admin'; + await setupOpenId(); // Re-initialize the strategy + verifyCallback = require('openid-client/passport').__getVerifyCallback(); + jwtDecode.mockReturnValue({ + roles: ['anotherRole', 'aThirdRole'], + }); + + // Act + const { user } = await validate(tokenset); + + // Assert + expect(user).toBeTruthy(); + expect(user.email).toBe(tokenset.claims().email); + }); + + it('should reject login when user has none of the required multiple roles', async () => { + // Arrange + process.env.OPENID_REQUIRED_ROLE = 'someRole,anotherRole,admin'; + await setupOpenId(); // Re-initialize the strategy + verifyCallback = require('openid-client/passport').__getVerifyCallback(); + jwtDecode.mockReturnValue({ + roles: ['aThirdRole', 'aFourthRole'], + }); + + // Act + const { user, details } = await validate(tokenset); + + // Assert + expect(user).toBe(false); + expect(details.message).toBe( + 'You must have one of: "someRole", "anotherRole", "admin" role to log in.', + ); + }); + + it('should handle spaces in comma-separated roles', async () => { + // Arrange + process.env.OPENID_REQUIRED_ROLE = ' someRole , anotherRole , admin '; + await setupOpenId(); // Re-initialize the strategy + verifyCallback = require('openid-client/passport').__getVerifyCallback(); + jwtDecode.mockReturnValue({ + roles: ['someRole'], + }); + + // Act + const { user } = await validate(tokenset); + + // Assert + expect(user).toBeTruthy(); + }); + it('should default to usePKCE false when OPENID_USE_PKCE is not defined', async () => { const OpenIDStrategy = require('openid-client/passport').Strategy;