mirror of
https://github.com/immich-app/immich.git
synced 2026-07-28 14:47:30 -07:00
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 <jrasm91@gmail.com>
This commit is contained in:
co-authored by
Jason Rasmussen
parent
49c0baf076
commit
a560e08693
@@ -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', () => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user