From a7821e20806da8539f24840fa1c5bb2eed9750aa Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 30 May 2026 11:23:56 -0400 Subject: [PATCH] fix: Apply OpenID role-sync fallback for present-but-empty claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both role-sync call sites skipped on a falsy `openIdRoleValues`, treating an empty claim string ('') the same as a missing claim and returning before `selectOpenIdRole` could apply the configured fallback role. An IdP emitting an empty roles claim for a user with no mapped groups left the stale local role in place instead of the authoritative fallback. Skip only when the helper returns `undefined` (missing/invalid), letting an empty string flow through to fallback selection — consistent with how an empty array is already handled. Adds regression coverage on both the OpenID strategy and the remote-agent API auth paths. --- api/strategies/openidStrategy.js | 2 +- api/strategies/openidStrategy.spec.js | 18 ++++++++++++++++++ .../api/src/middleware/remoteAgentAuth.spec.ts | 16 ++++++++++++++++ packages/api/src/middleware/remoteAgentAuth.ts | 2 +- 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 1b0728dac0..f33fdb82ad 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -510,7 +510,7 @@ async function applyOpenIdRoleSync({ decodeToken: jwtDecode, resolveGroupOverage, }); - if (!openIdRoleValues) { + if (openIdRoleValues === undefined) { logger.warn( `[openidStrategy] OpenID role sync skipped; claim '${options.claim}' was not found, invalid, or unresolved`, ); diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 4c09b527ca..916fac5d07 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -1652,6 +1652,24 @@ describe('setupOpenId', () => { expect(user.role).toBe('USER'); }); + it('uses fallback when the role claim is present but empty', async () => { + // The required-role gate reads the same `roles` claim this test empties, so + // disable it to model an IdP that authenticates the user yet emits no roles. + delete process.env.OPENID_REQUIRED_ROLE; + jwtDecode.mockReturnValue({ + roles: '', + permissions: ['not-admin'], + }); + + const { user } = await validate(tokenset); + + expect(user.role).toBe('USER'); + expect(updateUser).toHaveBeenCalledWith( + 'newUserId', + expect.objectContaining({ role: 'USER' }), + ); + }); + it('rejects login when configured sync roles do not exist', async () => { findRolesByNames.mockImplementation(async (roleNames) => roleNames diff --git a/packages/api/src/middleware/remoteAgentAuth.spec.ts b/packages/api/src/middleware/remoteAgentAuth.spec.ts index 29e111234b..f7c7c96db0 100644 --- a/packages/api/src/middleware/remoteAgentAuth.spec.ts +++ b/packages/api/src/middleware/remoteAgentAuth.spec.ts @@ -1500,6 +1500,22 @@ describe('createRemoteAgentAuth', () => { expect(req.user).toMatchObject({ role: 'USER' }); }); + it('applies fallback when the role claim is present but empty', async () => { + enableApiRoleSync(); + setupOidcMocks({ + sub: 'sub123', + email: 'agent@test.com', + roles: '', + }); + + const deps = makeDeps(); + const req = makeReq({ authorization: `Bearer ${FAKE_TOKEN}` }); + await createRemoteAgentAuth(deps)(req as Request, makeRes().res, mockNext); + + expect(deps.updateUser).toHaveBeenCalledWith('uid123', { role: 'USER' }); + expect(req.user).toMatchObject({ role: 'USER' }); + }); + it('preserves an existing ADMIN role because generic role sync cannot manage admin', async () => { enableApiRoleSync(); setupOidcMocks({ diff --git a/packages/api/src/middleware/remoteAgentAuth.ts b/packages/api/src/middleware/remoteAgentAuth.ts index dc50e5428c..7e7dc483c4 100644 --- a/packages/api/src/middleware/remoteAgentAuth.ts +++ b/packages/api/src/middleware/remoteAgentAuth.ts @@ -480,7 +480,7 @@ async function selectOpenIdRoleForOpenIdSync( accessClaims: payload, decodeToken: () => payload, }); - if (!openIdRoleValues) { + if (openIdRoleValues === undefined) { logger.warn( `[remoteAgentAuth] OpenID role sync skipped; claim '${options.claim}' was not found or invalid`, );