From f2e05e9d98d3213eab46ca56e1d00bd918d8906a Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:57:42 +0100 Subject: [PATCH] chore: migrate oauth claim validation to zod (#32373) (cherry picked from commit 0808f24a37cf376a509f1b4046cd52fd2d72b20d) --- server/src/dtos/auth.dto.ts | 43 +++++ server/src/repositories/oauth.repository.ts | 54 +++--- server/src/services/auth.service.spec.ts | 139 ++++++++++------ server/src/services/auth.service.ts | 156 ++++++------------ .../test/factories/oauth-profile.factory.ts | 4 +- server/test/factories/types.ts | 4 +- 6 files changed, 215 insertions(+), 185 deletions(-) diff --git a/server/src/dtos/auth.dto.ts b/server/src/dtos/auth.dto.ts index 04c84d931..c50ac025d 100644 --- a/server/src/dtos/auth.dto.ts +++ b/server/src/dtos/auth.dto.ts @@ -1,8 +1,10 @@ import { createZodDto } from 'nestjs-zod'; +import sanitize from 'sanitize-filename'; import z from 'zod'; import type { UserMetadataItem } from 'src/types.js'; import { AuthApiKey, AuthSession, AuthSharedLink, AuthUser, UserAdmin } from 'src/database.js'; import { ImmichCookie, UserMetadataKey } from 'src/enum.js'; +import { HumanReadableSize } from 'src/utils/bytes.js'; import { toEmail } from 'src/validation.js'; export type CookieResponse = { @@ -23,6 +25,47 @@ export const isMetadataHidden = (auth?: AuthDto) => !!auth?.sharedLink && !auth. export const isEditedOnly = (auth: AuthDto, edited = false) => !!auth.sharedLink || edited; +const storageLabelClaimSchema = z.string().transform((label) => sanitize(label)); +const quotaSizeInBytesSchema = z + .int() + .min(-1) + .nullable() + .transform((quota) => (quota === null || quota === -1 ? null : quota * HumanReadableSize.GiB)); +const userRoleSchema = z.union([z.literal('user'), z.literal('admin')]); + +export const OAuthProfileSchema = z.object({ + defaults: z.object({ + immich_quota_bytes: quotaSizeInBytesSchema, + }), + sub: z.string(), + email: z.string().trim().toLowerCase().optional().catch(undefined), + email_verified: z.coerce.boolean().optional().catch(undefined), + name: z.string().trim().optional().catch(undefined), + given_name: z.string().trim().optional().catch(undefined), + family_name: z.string().trim().optional().catch(undefined), + preferred_username: z.string().trim().optional().catch(undefined), + picture: z.string().optional().catch(undefined), + immich_storage_label: storageLabelClaimSchema.optional().catch(undefined), + immich_quota_bytes: quotaSizeInBytesSchema.optional().catch(undefined), + immich_role: z + .union([userRoleSchema, z.array(userRoleSchema)]) + .transform((value) => { + if (!value) { + return; + } + + const roles = Array.isArray(value) ? value : [value]; + if (roles.includes('admin')) { + return 'admin'; + } + + return 'user'; + }) + .optional() + .catch(undefined), +}); +export type OAuthProfile = z.infer; + const LoginCredentialSchema = z .object({ email: toEmail.describe('User email').meta({ example: 'testuser@email.com' }), diff --git a/server/src/repositories/oauth.repository.ts b/server/src/repositories/oauth.repository.ts index d5bab1d0a..a80dd6b9f 100644 --- a/server/src/repositories/oauth.repository.ts +++ b/server/src/repositories/oauth.repository.ts @@ -15,26 +15,32 @@ import { randomState, skipSubjectCheck, } from 'openid-client'; +import { SystemConfig } from 'src/dtos/config.dto.js'; import { OAuthTokenEndpointAuthMethod } from 'src/enum.js'; import { LoggingRepository } from 'src/repositories/logging.repository.js'; -export type OAuthConfig = { - clientId: string; - clientSecret?: string; - issuerUrl: string; - accountManagementUrl: string; - endSessionEndpoint: string; - mobileOverrideEnabled: boolean; - mobileRedirectUri: string; - profileSigningAlgorithm: string; - prompt: string; - scope: string; - signingAlgorithm: string; - tokenEndpointAuthMethod: OAuthTokenEndpointAuthMethod; - timeout: number; - allowInsecureRequests: boolean; -}; -export type OAuthProfile = UserInfoResponse; +export type OAuthConfig = Pick< + SystemConfig['oauth'], + | 'defaultStorageQuota' + | 'storageLabelClaim' + | 'storageQuotaClaim' + | 'roleClaim' + | 'clientId' + | 'clientSecret' + | 'issuerUrl' + | 'accountManagementUrl' + | 'endSessionEndpoint' + | 'mobileOverrideEnabled' + | 'mobileRedirectUri' + | 'profileSigningAlgorithm' + | 'prompt' + | 'scope' + | 'signingAlgorithm' + | 'tokenEndpointAuthMethod' + | 'timeout' + | 'allowInsecureRequests' +>; +export type OAuthRawProfile = UserInfoResponse; @Injectable() export class OAuthRepository { @@ -84,23 +90,23 @@ export class OAuthRepository { url: string, expectedState: string, codeVerifier: string, - ): Promise<{ profile: OAuthProfile; sid?: string; idToken?: string }> { + ): Promise<{ rawProfile: OAuthRawProfile; sid?: string; idToken?: string }> { const client = await this.getClient(config); const pkceCodeVerifier = client.serverMetadata().supportsPKCE() ? codeVerifier : undefined; try { const tokens = await authorizationCodeGrant(client, new URL(url), { expectedState, pkceCodeVerifier }); - let profile: OAuthProfile; + let rawProfile: OAuthRawProfile; const tokenClaims = tokens.claims(); if (tokenClaims && 'email' in tokenClaims) { this.logger.debug('Using ID token claims instead of userinfo endpoint'); - profile = tokenClaims as OAuthProfile; + rawProfile = tokenClaims; } else { - profile = await fetchUserInfo(client, tokens.access_token, skipSubjectCheck); + rawProfile = await fetchUserInfo(client, tokens.access_token, skipSubjectCheck); } - if (!profile.sub) { + if (!rawProfile.sub) { throw new Error('Unexpected profile response, no `sub`'); } @@ -112,7 +118,7 @@ export class OAuthRepository { } } - return { profile, sid, idToken: tokens.id_token }; + return { rawProfile, sid, idToken: tokens.id_token }; } catch (error: Error | any) { if (error.message.includes('unexpected JWT alg received')) { this.logger.warn( @@ -138,7 +144,7 @@ export class OAuthRepository { return response.arrayBuffer(); } - private jwksClients: Map = new Map(); // useful for caching and performnce + private jwksClients: Map = new Map(); // useful for caching and performance async validateLogoutToken(config: OAuthConfig, logoutToken: string): Promise<{ sub?: string; sid?: string } | null> { const client = await this.getClient(config); const algorithm = client.clientMetadata().id_token_signed_response_alg ?? 'RS256'; diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 54428a5e8..fcfe01c26 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -719,7 +719,7 @@ describe(AuthService.name, () => { it('should not allow auto registering', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); mocks.user.getByEmail.mockResolvedValue(void 0); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: OAuthProfileFactory.create() }); await expect( sut.callback( @@ -734,10 +734,10 @@ describe(AuthService.name, () => { it('should link an existing user', async () => { const user = UserFactory.create(); - const profile = OAuthProfileFactory.create(); + const rawProfile = OAuthProfileFactory.create(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByEmail.mockResolvedValue(user); mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -749,15 +749,19 @@ describe(AuthService.name, () => { ); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); - expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub }); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: rawProfile.sub }); }); it('should store the OAuth bearer token on the new session', async () => { const user = UserFactory.create(); - const profile = OAuthProfileFactory.create(); + const rawProfile = OAuthProfileFactory.create(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile, sid: 'oauth-sid', idToken: 'oauth-bearer-token' }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + rawProfile, + sid: 'oauth-sid', + idToken: 'oauth-bearer-token', + }); mocks.user.getByEmail.mockResolvedValue(user); mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -775,10 +779,10 @@ describe(AuthService.name, () => { it('should normalize the email from the OAuth profile before linking', async () => { const user = UserFactory.create(); - const profile = OAuthProfileFactory.create({ email: ' TEST@IMMICH.CLOUD ' }); + const rawProfile = OAuthProfileFactory.create({ email: ' TEST@IMMICH.CLOUD ' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByEmail.mockResolvedValue(user); mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -790,14 +794,14 @@ describe(AuthService.name, () => { ); expect(mocks.user.getByEmail).toHaveBeenCalledWith('test@immich.cloud'); - expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub }); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: rawProfile.sub }); }); it('should not link to a user with a different oauth sub', async () => { const user = UserFactory.create({ oauthId: 'existing-sub' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: OAuthProfileFactory.create() }); mocks.user.getByEmail.mockResolvedValueOnce(user); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); @@ -818,7 +822,7 @@ describe(AuthService.name, () => { mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: OAuthProfileFactory.create() }); mocks.session.create.mockResolvedValue(SessionFactory.create()); await sut.callback( @@ -837,7 +841,7 @@ describe(AuthService.name, () => { mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.create.mockResolvedValue(UserFactory.create()); mocks.session.create.mockResolvedValue(SessionFactory.create()); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: { sub: 'sub' } }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: { sub: 'sub' } }); await expect( sut.callback( @@ -859,7 +863,7 @@ describe(AuthService.name, () => { it(`should use the mobile redirect override for a url of ${url}`, async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride); mocks.user.getByOAuthId.mockResolvedValue(UserFactory.create()); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: OAuthProfileFactory.create() }); mocks.session.create.mockResolvedValue(SessionFactory.create()); await sut.callback({ url, state: 'xyz789', codeVerifier: 'foo' }, {}, loginDetails); @@ -877,7 +881,9 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + rawProfile: OAuthProfileFactory.create(), + }); mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -893,7 +899,7 @@ describe(AuthService.name, () => { it('should infer name from given and family names', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ name: undefined, given_name: 'Given', family_name: 'Family' }), + rawProfile: OAuthProfileFactory.create({ name: undefined, given_name: 'Given', family_name: 'Family' }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); @@ -910,10 +916,10 @@ describe(AuthService.name, () => { }); it('should fallback to email when no username is provided', async () => { - const profile = OAuthProfileFactory.create({ name: undefined, given_name: undefined, family_name: undefined }); + const rawProfile = OAuthProfileFactory.create({ name: undefined, given_name: undefined, family_name: undefined }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.create.mockResolvedValue(UserFactory.create()); @@ -925,13 +931,13 @@ describe(AuthService.name, () => { loginDetails, ); - expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ name: profile.email })); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ name: rawProfile.email })); }); it('should ignore an invalid storage quota', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ immich_quota: 'abc' }), + rawProfile: OAuthProfileFactory.create({ immich_quota: 'abc' }), }); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); @@ -950,7 +956,7 @@ describe(AuthService.name, () => { it('should ignore an invalid negative quota', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ immich_quota: -5 }), + rawProfile: OAuthProfileFactory.create({ immich_quota: -5 }), }); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); @@ -969,7 +975,7 @@ describe(AuthService.name, () => { it('should set unlimited quota for -1', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ immich_quota: -1 }), + rawProfile: OAuthProfileFactory.create({ immich_quota: -1 }), }); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); @@ -987,7 +993,9 @@ describe(AuthService.name, () => { it('should set quota for 0 quota', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create({ immich_quota: 0 }) }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + rawProfile: OAuthProfileFactory.create({ immich_quota: 0 }), + }); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); @@ -1004,7 +1012,9 @@ describe(AuthService.name, () => { it('should use a valid storage quota', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create({ immich_quota: 5 }) }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + rawProfile: OAuthProfileFactory.create({ immich_quota: 5 }), + }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByOAuthId.mockResolvedValue(void 0); @@ -1023,11 +1033,11 @@ describe(AuthService.name, () => { it('should sync the profile picture', async () => { const fileId = newUuid(); const user = UserFactory.create({ oauthId: 'oauth-id' }); - const profile = OAuthProfileFactory.create({ picture: 'https://auth.immich.cloud/profiles/1.jpg' }); + const rawProfile = OAuthProfileFactory.create({ picture: 'https://auth.immich.cloud/profiles/1.jpg' }); const pictureBytes = new Uint8Array([1, 2, 3, 4, 5]); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.crypto.randomUUID.mockReturnValue(fileId); mocks.oauth.getProfilePicture.mockResolvedValue(pictureBytes.buffer); @@ -1044,7 +1054,7 @@ describe(AuthService.name, () => { profileImagePath: expect.stringContaining(`/data/profile/${user.id}/${fileId}.webp`), profileChangedAt: expect.any(Date), }); - expect(mocks.oauth.getProfilePicture).toHaveBeenCalledWith(profile.picture); + expect(mocks.oauth.getProfilePicture).toHaveBeenCalledWith(rawProfile.picture); expect(mocks.media.decodeImage).toHaveBeenCalledWith( Buffer.from(pictureBytes.buffer, pictureBytes.byteOffset, pictureBytes.byteLength), expect.objectContaining({ processInvalidImages: false }), @@ -1058,10 +1068,10 @@ describe(AuthService.name, () => { it('should not update the user when thumbnail processing fails on the OAuth picture', async () => { const user = UserFactory.create({ oauthId: 'oauth-id' }); - const profile = OAuthProfileFactory.create({ picture: 'https://auth.immich.cloud/profiles/1.jpg' }); + const rawProfile = OAuthProfileFactory.create({ picture: 'https://auth.immich.cloud/profiles/1.jpg' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.oauth.getProfilePicture.mockResolvedValue(new Uint8Array([1, 2, 3, 4, 5]).buffer); mocks.media.generateThumbnail.mockRejectedValue(new Error('not an image')); @@ -1085,7 +1095,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ + rawProfile: OAuthProfileFactory.create({ sub: oauthId, email: user.email, picture: 'https://auth.immich.cloud/profiles/1.jpg', @@ -1108,7 +1118,7 @@ describe(AuthService.name, () => { it('should only allow "admin" and "user" for the role claim', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ immich_role: 'foo' }), + rawProfile: OAuthProfileFactory.create({ immich_role: 'foo' }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); @@ -1128,7 +1138,7 @@ describe(AuthService.name, () => { it('should create an admin user if the role claim is set to admin', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ immich_role: 'admin' }), + rawProfile: OAuthProfileFactory.create({ immich_role: 'admin' }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); @@ -1149,7 +1159,7 @@ describe(AuthService.name, () => { oauth: { ...systemConfigStub.oauthWithAutoRegister.oauth, roleClaim: 'my_role' }, }); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ my_role: 'admin' }), + rawProfile: OAuthProfileFactory.create({ my_role: 'admin' }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); @@ -1168,7 +1178,7 @@ describe(AuthService.name, () => { 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'] }), + rawProfile: OAuthProfileFactory.create({ immich_role: ['user', 'admin'] }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); @@ -1187,7 +1197,7 @@ describe(AuthService.name, () => { 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'] }), + rawProfile: OAuthProfileFactory.create({ immich_role: ['user'] }), }); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); @@ -1209,7 +1219,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_quota: 5 }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_quota: 5 }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.update.mockResolvedValue({ ...user, quotaSizeInBytes: 5_368_709_120 }); @@ -1233,7 +1243,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_quota: -1 }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_quota: -1 }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.update.mockResolvedValue({ ...user, quotaSizeInBytes: null }); @@ -1257,7 +1267,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId!, preferred_username: 'idp-username' }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId!, preferred_username: 'idp-username' }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -1279,7 +1289,7 @@ describe(AuthService.name, () => { oauth: { ...systemConfigStub.oauthWithAutoRegister.oauth, storageLabelClaim: 'immich_label' }, }); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_label: 'synced-label' }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_label: 'synced-label' }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.getByStorageLabel.mockResolvedValue(void 0); @@ -1305,7 +1315,7 @@ describe(AuthService.name, () => { oauth: { ...systemConfigStub.oauthWithAutoRegister.oauth, storageLabelClaim: 'immich_label' }, }); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_label: 'synced.label' }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId!, immich_label: 'synced.label' }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.getByStorageLabel.mockResolvedValue(void 0); @@ -1330,7 +1340,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: oauthId, immich_role: 'admin' }), + rawProfile: OAuthProfileFactory.create({ sub: oauthId, immich_role: 'admin' }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.update.mockResolvedValue({ ...user, isAdmin: true }); @@ -1351,7 +1361,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: oauthId, immich_role: ['user'] }), + rawProfile: OAuthProfileFactory.create({ sub: oauthId, immich_role: ['user'] }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.update.mockResolvedValue({ ...user, isAdmin: false }); @@ -1372,7 +1382,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: oauthId }), + rawProfile: OAuthProfileFactory.create({ sub: oauthId }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -1391,7 +1401,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: OAuthProfileFactory.create({ sub: user.oauthId! }), + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId! }), }); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); @@ -1406,15 +1416,38 @@ describe(AuthService.name, () => { expect(mocks.user.syncUsage).not.toHaveBeenCalled(); }); + it('should update quota to default value on subsequent logins when it is absent from the profile', async () => { + const user = UserFactory.create({ oauthId: 'oauth-id' }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ + rawProfile: OAuthProfileFactory.create({ sub: user.oauthId! }), + }); + mocks.user.getByOAuthId.mockResolvedValue(user); + mocks.user.update.mockResolvedValueOnce({ ...user, quotaSizeInBytes: null }); + 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, + expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 }), + ); + }); + 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' }); + const rawProfile = OAuthProfileFactory.create({ immich_role: 'admin' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.getByEmail.mockResolvedValue(user); - mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: profile.sub }); - mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: profile.sub, isAdmin: true }); + mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: rawProfile.sub }); + mocks.user.update.mockResolvedValueOnce({ ...user, oauthId: rawProfile.sub, isAdmin: true }); mocks.session.create.mockResolvedValue(SessionFactory.create()); await sut.callback( @@ -1423,7 +1456,7 @@ describe(AuthService.name, () => { loginDetails, ); - expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub }); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: rawProfile.sub }); expect(mocks.user.update).toHaveBeenCalledWith(user.id, { isAdmin: true }); }); }); @@ -1432,10 +1465,10 @@ describe(AuthService.name, () => { it('should link an account', async () => { const user = UserFactory.create(); const auth = AuthFactory.from(user).apiKey({ permissions: [] }).build(); - const profile = OAuthProfileFactory.create(); + const rawProfile = OAuthProfileFactory.create(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile }); mocks.user.update.mockResolvedValue(user); await sut.link( @@ -1444,7 +1477,7 @@ describe(AuthService.name, () => { {}, ); - expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: profile.sub }); + expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: rawProfile.sub }); }); it('should link an account and update the session with the oauthSid', async () => { @@ -1454,7 +1487,7 @@ describe(AuthService.name, () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: { sub: 'sub' }, + rawProfile: { sub: 'sub' }, sid: session.oauthSid ?? undefined, idToken: session.oauthBearerToken ?? undefined, }); @@ -1480,7 +1513,7 @@ describe(AuthService.name, () => { const auth = { user: authUser, apiKey: authApiKey }; mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ rawProfile: OAuthProfileFactory.create() }); mocks.user.getByOAuthId.mockResolvedValue({ id: 'other-user' } as UserAdmin); await expect( diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index 1cb9e962c..cdad0a242 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -2,7 +2,6 @@ import { BadRequestException, ForbiddenException, Injectable, UnauthorizedExcept import { parseCookie } from 'cookie'; import { DateTime } from 'luxon'; import { IncomingHttpHeaders } from 'node:http'; -import sanitize from 'sanitize-filename'; import { LOGIN_DUMMY_HASH, LOGIN_URL, MOBILE_REDIRECT, SALT_ROUNDS } from 'src/constants.js'; import { AuthSharedLink, AuthUser, UserAdmin } from 'src/database.js'; import { @@ -14,6 +13,8 @@ import { OAuthBackchannelLogoutDto, OAuthCallbackDto, OAuthConfigDto, + OAuthProfile, + OAuthProfileSchema, PinCodeChangeDto, PinCodeResetDto, PinCodeSetupDto, @@ -23,10 +24,9 @@ import { import { SystemConfig, defaults } from 'src/dtos/config.dto.js'; import { UserAdminResponseDto, mapUserAdmin } from 'src/dtos/user.dto.js'; import { AuthType, ImmichCookie, ImmichHeader, ImmichQuery, JobName, Permission } from 'src/enum.js'; -import { OAuthProfile } from 'src/repositories/oauth.repository.js'; +import { OAuthConfig, OAuthRawProfile } from 'src/repositories/oauth.repository.js'; import { BaseService } from 'src/services/base.service.js'; import { isGranted } from 'src/utils/access.js'; -import { HumanReadableSize } from 'src/utils/bytes.js'; import { generateProfileImage } from 'src/utils/profile-image.js'; import { getUserAgentDetails } from 'src/utils/request.js'; @@ -38,18 +38,10 @@ export interface LoginDetails { appVersion: string | null; } -interface ClaimOptions { - key: string; - isValid: (value: unknown) => value is T; - parse?: (raw: unknown) => T; -} - -type OAuthClaimsConfig = Pick; - -interface ParsedOAuthClaims { - storageLabel?: string | null; - quotaSizeInBytes?: number | null; -} +type OAuthClaimsConfig = Pick< + SystemConfig['oauth'], + 'defaultStorageQuota' | 'storageLabelClaim' | 'storageQuotaClaim' | 'roleClaim' +>; export type ValidateRequest = { headers: IncomingHttpHeaders; @@ -294,6 +286,18 @@ export class AuthService extends BaseService { ); } + private parseOAuthClaims(profile: OAuthRawProfile, oauth: OAuthConfig) { + const { defaultStorageQuota, storageLabelClaim, storageQuotaClaim, roleClaim } = oauth; + + return OAuthProfileSchema.parse({ + ...profile, + defaults: { immich_quota_bytes: defaultStorageQuota }, + immich_storage_label: profile[storageLabelClaim], + immich_quota_bytes: profile[storageQuotaClaim], + immich_role: profile[roleClaim], + }); + } + async callback(dto: OAuthCallbackDto, headers: IncomingHttpHeaders, loginDetails: LoginDetails) { const { oauth } = await this.getConfig({ withCache: false }); if (!oauth.enabled) { @@ -312,18 +316,18 @@ export class AuthService extends BaseService { const url = this.resolveRedirectUri(oauth, dto.url); const { - profile, + rawProfile, sid: oauthSid, idToken: oauthBearerToken, } = await this.oauthRepository.getProfileAndOAuthSid(oauth, url, expectedState, codeVerifier); - const normalizedEmail = profile.email ? profile.email.trim().toLowerCase() : undefined; - const { autoRegister, roleClaim } = oauth; + const profile = this.parseOAuthClaims(rawProfile, oauth); + const { autoRegister } = oauth; this.logger.debug(`Logging in with OAuth: ${JSON.stringify(profile)}`); let user: UserAdmin | undefined = await this.userRepository.getByOAuthId(profile.sub); // link by email - if (!user && normalizedEmail) { - const emailUser = await this.userRepository.getByEmail(normalizedEmail); + if (!user && profile.email) { + const emailUser = await this.userRepository.getByEmail(profile.email); if (emailUser) { if (emailUser.oauthId) { this.logger.debug('OAuth login conflict: email already linked to different account'); @@ -333,10 +337,9 @@ export class AuthService extends BaseService { } } - const role = this.getRoleClaim(profile, roleClaim); - const isAdmin = role === 'admin'; + const isAdmin = profile.immich_role === 'admin'; - if (user && role && isAdmin !== user.isAdmin) { + if (user && profile.immich_role && isAdmin !== user.isAdmin) { user = await this.userRepository.update(user.id, { isAdmin }); } @@ -346,29 +349,28 @@ export class AuthService extends BaseService { } else { if (!autoRegister) { this.logger.warn( - `Unable to register ${profile.sub}/${normalizedEmail || '(no email)'}. User does not exist and auto registering is disabled. To enable set OAuth Auto Register to true in admin settings.`, + `Unable to register ${profile.sub}/${profile.email || '(no email)'}. User does not exist and auto registering is disabled. To enable set OAuth Auto Register to true in admin settings.`, ); throw new BadRequestException('OAuth authentication failed'); } - if (!normalizedEmail) { + if (!profile.email) { throw new BadRequestException('OAuth profile does not have an email address'); } - this.logger.log(`Registering new user: ${profile.sub}/${normalizedEmail}`); - - const claims = this.parseOAuthClaims(profile, oauth, true); + this.logger.log(`Registering new user: ${profile.sub}/${profile.email}`); user = await this.createUser({ name: profile.name || `${profile.given_name || ''} ${profile.family_name || ''}`.trim() || profile.preferred_username || - normalizedEmail, - email: normalizedEmail, + profile.email, + email: profile.email, oauthId: profile.sub, - quotaSizeInBytes: claims.quotaSizeInBytes ?? null, - storageLabel: claims.storageLabel ?? null, + quotaSizeInBytes: + profile.immich_quota_bytes === undefined ? profile.defaults.immich_quota_bytes : profile.immich_quota_bytes, + storageLabel: profile.immich_storage_label ?? null, isAdmin, }); } @@ -415,11 +417,13 @@ export class AuthService extends BaseService { } const { oauth } = await this.getConfig({ withCache: false }); - const { - profile: { sub: oauthId }, - sid, - idToken, - } = await this.oauthRepository.getProfileAndOAuthSid(oauth, dto.url, expectedState, codeVerifier); + const { rawProfile, sid, idToken } = await this.oauthRepository.getProfileAndOAuthSid( + oauth, + dto.url, + expectedState, + codeVerifier, + ); + const { sub: oauthId } = this.parseOAuthClaims(rawProfile, oauth); const duplicate = await this.userRepository.getByOAuthId(oauthId); if (duplicate && duplicate.id !== auth.user.id) { this.logger.warn(`OAuth link account failed: sub is already linked to another user (${duplicate.email}).`); @@ -611,73 +615,30 @@ export class AuthService extends BaseService { await this.sessionRepository.update(auth.session.id, { pinExpiresAt: null }); } - private getClaim(profile: OAuthProfile, options: ClaimOptions): T | undefined { - const raw = profile[options.key as keyof OAuthProfile]; - const value = options.parse ? options.parse(raw) : raw; - return options.isValid(value) ? value : undefined; - } - - private parseOAuthClaims(profile: OAuthProfile, oauth: OAuthClaimsConfig, useDefaults: boolean): ParsedOAuthClaims { - const { defaultStorageQuota, storageLabelClaim, storageQuotaClaim } = oauth; - const claims: ParsedOAuthClaims = {}; - - const storageLabel = this.getClaim(profile, { - key: storageLabelClaim, - isValid: (value): value is string => typeof value === 'string', - }); - - if (useDefaults) { - claims.storageLabel = storageLabel || null; - } - // Default claim (preferred_username) is registration-only; login sync is opt-in. - else if (storageLabelClaim !== defaults.oauth.storageLabelClaim && storageLabel !== undefined) { - claims.storageLabel = this.formatStorageLabel(storageLabel); - } - - const storageQuota = this.getClaim(profile, { - key: storageQuotaClaim, - parse: Number, - isValid: (value): value is number => - typeof value === 'number' && Number.isFinite(value) && (value === -1 || value >= 0), - }); - - if (storageQuota !== undefined) { - claims.quotaSizeInBytes = storageQuota === -1 ? null : storageQuota * HumanReadableSize.GiB; - } else if (useDefaults) { - claims.quotaSizeInBytes = - defaultStorageQuota === null || defaultStorageQuota === -1 ? null : defaultStorageQuota * HumanReadableSize.GiB; - } - - return claims; - } - - private formatStorageLabel(label: string): string | null { - if (!label) { - return null; - } - - return sanitize(label); - } - private async syncOAuthClaims(user: UserAdmin, profile: OAuthProfile, oauth: OAuthClaimsConfig): Promise { - const claims = this.parseOAuthClaims(profile, oauth, false); const updates: { storageLabel?: string | null; quotaSizeInBytes?: number | null; } = {}; - if (claims.storageLabel && claims.storageLabel !== user.storageLabel) { - const duplicate = await this.userRepository.getByStorageLabel(claims.storageLabel); + if ( + oauth.storageLabelClaim !== defaults.oauth.storageLabelClaim && + profile.immich_storage_label && + profile.immich_storage_label !== user.storageLabel + ) { + const duplicate = await this.userRepository.getByStorageLabel(profile.immich_storage_label); if (duplicate && duplicate.id !== user.id) { this.logger.warn(`Unable to sync OAuth storage label for user ${user.id}: label already in use`); } else { - updates.storageLabel = claims.storageLabel; + updates.storageLabel = profile.immich_storage_label; } } - if (claims.quotaSizeInBytes !== undefined && claims.quotaSizeInBytes !== user.quotaSizeInBytes) { - updates.quotaSizeInBytes = claims.quotaSizeInBytes; + if (profile.immich_quota_bytes === undefined && user.quotaSizeInBytes !== profile.defaults.immich_quota_bytes) { + updates.quotaSizeInBytes = profile.defaults.immich_quota_bytes; + } else if (profile.immich_quota_bytes !== undefined && profile.immich_quota_bytes !== user.quotaSizeInBytes) { + updates.quotaSizeInBytes = profile.immich_quota_bytes; } if (Object.keys(updates).length === 0) { @@ -691,19 +652,6 @@ export class AuthService extends BaseService { return this.userRepository.update(user.id, { ...updates, updatedAt: new Date() }); } - 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, diff --git a/server/test/factories/oauth-profile.factory.ts b/server/test/factories/oauth-profile.factory.ts index d7beb8a16..cb4391a3a 100644 --- a/server/test/factories/oauth-profile.factory.ts +++ b/server/test/factories/oauth-profile.factory.ts @@ -1,9 +1,9 @@ -import { OAuthProfile } from 'src/repositories/oauth.repository.js'; +import { OAuthRawProfile } from 'src/repositories/oauth.repository.js'; import { OAuthProfileLike } from 'test/factories/types.js'; import { newUuid } from 'test/small.factory.js'; export class OAuthProfileFactory { - private constructor(private value: OAuthProfile) {} + private constructor(private value: OAuthRawProfile) {} static create(dto: OAuthProfileLike = {}) { return OAuthProfileFactory.from(dto).build(); diff --git a/server/test/factories/types.ts b/server/test/factories/types.ts index c48a5e710..1bc75f7bf 100644 --- a/server/test/factories/types.ts +++ b/server/test/factories/types.ts @@ -1,7 +1,7 @@ import { Selectable } from 'kysely'; import { PersonUser } from 'src/database.js'; import { PersonUserRole } from 'src/dtos/person.dto.js'; -import { OAuthProfile } from 'src/repositories/oauth.repository.js'; +import { OAuthRawProfile } from 'src/repositories/oauth.repository.js'; import { ActivityTable } from 'src/schema/tables/activity.table.js'; import { AlbumUserTable } from 'src/schema/tables/album-user.table.js'; import { AlbumTable } from 'src/schema/tables/album.table.js'; @@ -48,4 +48,4 @@ export type ActivityLike = Partial>; export type ApiKeyLike = Partial>; export type PasskeyLike = Partial>; export type SessionLike = Partial>; -export type OAuthProfileLike = Partial; +export type OAuthProfileLike = Partial;