From a560e08693ca1287d1aff0af92b4a87af2fdf9d8 Mon Sep 17 00:00:00 2001 From: Imperator Ruscal <3578932+ImperatorRuscal@users.noreply.github.com> Date: Tue, 21 Jul 2026 16:21:19 -0500 Subject: [PATCH] fix: re-evaluate OIDC role claim on every login and support array values (#29991) * fix(server): re-evaluate OIDC role claim on every login and support array values Previously the OIDC role claim (immich_role) was only read at user auto-registration time and only accepted as a scalar string, so admin status never updated after the first login and array-valued role/group claims (common with Keycloak, Entra ID, etc.) were silently ignored. Now the role claim is normalized from either a string or an array of strings, and existing users have their isAdmin flag synced from the claim on every login, keeping the IdP as the source of truth for privileges while leaving isAdmin untouched when the claim is blank. * fix(server): use .includes() instead of .some() for role claim check Satisfies unicorn/prefer-includes lint rule flagged by CI. * fix(server): resolve missing role claim to standard user and fix test mock Default the OIDC role claim to 'user' when the IdP omits it so auto-registration doesn't crash, and add the missing getAdmin mock in the auth.service test so it correctly simulates an existing admin rather than the first-user-registration path. * fix: claim sync logic --------- Co-authored-by: Jason Rasmussen --- server/src/services/auth.service.spec.ts | 119 +++++++++++++++++++++++ server/src/services/auth.service.ts | 27 +++-- 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 5c76b38542..dad13ef5f5 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -1140,6 +1140,125 @@ describe(AuthService.name, () => { expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: true })); }); + + it('should create an admin user if the role claim is an array containing admin', async () => { + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + profile: OAuthProfileFactory.create({ immich_role: ['user', 'admin'] }), + }); + mocks.user.getByEmail.mockResolvedValue(void 0); + mocks.user.getByOAuthId.mockResolvedValue(void 0); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: true })); + }); + + it('should create a standard user if the role claim is an array containing only user', async () => { + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + profile: OAuthProfileFactory.create({ immich_role: ['user'] }), + }); + mocks.user.getByEmail.mockResolvedValue(void 0); + mocks.user.getByOAuthId.mockResolvedValue(void 0); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: false })); + }); + + it('should promote an existing user to admin if the role claim contains admin on login', async () => { + const user = UserFactory.create({ isAdmin: false, oauthId: 'oauth-id' }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + profile: OAuthProfileFactory.create({ sub: user.oauthId, immich_role: 'admin' }), + }); + mocks.user.getByOAuthId.mockResolvedValue(user); + mocks.user.update.mockResolvedValue({ ...user, isAdmin: true }); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { isAdmin: true }); + }); + + it('should demote an existing admin if the role claim only contains user on login', async () => { + const user = UserFactory.create({ isAdmin: true, oauthId: 'oauth-id' }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + profile: OAuthProfileFactory.create({ sub: user.oauthId, immich_role: ['user'] }), + }); + mocks.user.getByOAuthId.mockResolvedValue(user); + mocks.user.update.mockResolvedValue({ ...user, isAdmin: false }); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { isAdmin: false }); + }); + + it('should not change isAdmin for an existing user if the role claim is blank', async () => { + const user = UserFactory.create({ isAdmin: true, oauthId: 'oauth-id' }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + profile: OAuthProfileFactory.create({ sub: user.oauthId }), + }); + mocks.user.getByOAuthId.mockResolvedValue(user); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.update).not.toHaveBeenCalled(); + }); + + it('should re-evaluate the role claim for a user linked by email', async () => { + const user = UserFactory.create({ isAdmin: false }); + const profile = OAuthProfileFactory.create({ immich_role: 'admin' }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.user.getByEmail.mockResolvedValue(user); + mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: profile.sub }); + mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: profile.sub, isAdmin: true }); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' }, + {}, + loginDetails, + ); + + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub }); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { isAdmin: true }); + }); }); describe('link', () => { diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index d30c912194..f3be40f7dd 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -331,6 +331,13 @@ export class AuthService extends BaseService { } } + const role = this.getRoleClaim(profile, roleClaim); + const isAdmin = role === 'admin'; + + if (user && role && isAdmin !== user.isAdmin) { + user = await this.userRepository.update(user.id, { isAdmin }); + } + // register new user if (!user) { if (!autoRegister) { @@ -356,11 +363,6 @@ export class AuthService extends BaseService { default: defaultStorageQuota, isValid: (value: unknown) => Number(value) >= 0, }); - const role = this.getClaim<'admin' | 'user'>(profile, { - key: roleClaim, - default: 'user', - isValid: (value: unknown) => typeof value === 'string' && ['admin', 'user'].includes(value), - }); user = await this.createUser({ name: @@ -372,7 +374,7 @@ export class AuthService extends BaseService { oauthId: profile.sub, quotaSizeInBytes: storageQuota === null ? null : storageQuota * HumanReadableSize.GiB, storageLabel: storageLabel || null, - isAdmin: role === 'admin', + isAdmin, }); } @@ -641,6 +643,19 @@ export class AuthService extends BaseService { return options.isValid(value) ? (value as T) : options.default; } + private getRoleClaim(profile: OAuthProfile, roleClaim: string): 'admin' | 'user' | undefined { + const value = profile[roleClaim as keyof OAuthProfile]; + const roles = Array.isArray(value) ? value : [value]; + const isRole = (role: string) => roles.includes(role); + + if (isRole('admin')) { + return 'admin'; + } + if (isRole('user')) { + return 'user'; + } + } + private resolveRedirectUri( { mobileRedirectUri, mobileOverrideEnabled }: { mobileRedirectUri: string; mobileOverrideEnabled: boolean }, url: string,