From b42fdcfca957e90b18c653106af15dfcacb51015 Mon Sep 17 00:00:00 2001 From: bo0tzz Date: Thu, 16 Apr 2026 21:39:48 +0200 Subject: [PATCH] fix: review notes --- e2e/src/specs/server/api/oauth.e2e-spec.ts | 3 +- i18n/en.json | 4 +- open-api/immich-openapi-specs.json | 88 ++----------------- open-api/typescript-sdk/src/fetch-client.ts | 29 +----- server/src/controllers/oauth.controller.ts | 17 ---- server/src/dtos/auth.dto.ts | 14 +-- .../oauth-link-token.repository.ts | 1 - server/src/services/auth.service.spec.ts | 35 +++++++- server/src/services/auth.service.ts | 19 +++- web/src/lib/utils.ts | 4 - web/src/routes/auth/link/+page.svelte | 12 ++- 11 files changed, 72 insertions(+), 154 deletions(-) diff --git a/e2e/src/specs/server/api/oauth.e2e-spec.ts b/e2e/src/specs/server/api/oauth.e2e-spec.ts index 8e095c7ee3..6ff2446fe8 100644 --- a/e2e/src/specs/server/api/oauth.e2e-spec.ts +++ b/e2e/src/specs/server/api/oauth.e2e-spec.ts @@ -364,9 +364,10 @@ describe(`/oauth`, () => { }); const callbackParams = await loginWithOAuth('oauth-user3'); const { status, body } = await request(app).post('/oauth/callback').send(callbackParams); - expect(status).toBe(400); + expect(status).toBe(403); expect(body.message).toBe('oauth_account_link_required'); expect(body.userEmail).toBe('oauth-user3@immich.app'); + expect(body.linkToken).toBeDefined(); }); }); }); diff --git a/i18n/en.json b/i18n/en.json index 8e02d4ee41..2ff1123d42 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1642,8 +1642,8 @@ "notifications": "Notifications", "notifications_setting_description": "Manage notifications", "oauth": "OAuth", - "oauth_link_existing_account": "Log in with your Immich password to link your OAuth account.", - "oauth_link_password_login_required": "An account with this email already exists. Password login is required to link your OAuth account. Please contact your administrator.", + "oauth_link_existing_account": "Log in with your Immich password to link your OAuth account", + "oauth_link_password_login_required": "An account with this email already exists but password login is required to link your OAuth account. Please contact your administrator", "obtainium_configurator": "Obtainium Configurator", "obtainium_configurator_instructions": "Use Obtainium to install and update the Android app directly from Immich GitHub's release. Create an API key and select a variant to create your Obtainium configuration link", "ocr": "OCR", diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 720c67fe98..b743441bfd 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -7439,65 +7439,6 @@ "x-immich-state": "Stable" } }, - "/oauth/link": { - "post": { - "description": "Link an OAuth account to the authenticated user.", - "operationId": "linkOAuthAccount", - "parameters": [], - "requestBody": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/OAuthLinkDto" - } - } - }, - "required": true - }, - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/UserAdminResponseDto" - } - } - }, - "description": "" - } - }, - "security": [ - { - "bearer": [] - }, - { - "cookie": [] - }, - { - "api_key": [] - } - ], - "summary": "Link OAuth account", - "tags": [ - "Authentication" - ], - "x-immich-history": [ - { - "version": "v1", - "state": "Added" - }, - { - "version": "v1", - "state": "Beta" - }, - { - "version": "v2", - "state": "Stable" - } - ], - "x-immich-state": "Stable" - } - }, "/oauth/mobile-redirect": { "get": { "description": "Requests to this URL are automatically forwarded to the mobile app, and is used in some cases for OAuth redirecting.", @@ -18072,6 +18013,10 @@ "pattern": "^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$", "type": "string" }, + "linkToken": { + "description": "OAuth link token to consume on successful login", + "type": "string" + }, "password": { "description": "User password", "example": "password", @@ -19116,27 +19061,6 @@ ], "type": "object" }, - "OAuthLinkDto": { - "properties": { - "codeVerifier": { - "description": "OAuth code verifier (PKCE)", - "type": "string" - }, - "linkToken": { - "description": "OAuth link token from prior callback", - "type": "string" - }, - "state": { - "description": "OAuth state parameter", - "type": "string" - }, - "url": { - "description": "OAuth callback URL", - "type": "string" - } - }, - "type": "object" - }, "OAuthTokenEndpointAuthMethod": { "description": "OAuth token endpoint auth method", "enum": [ @@ -21875,6 +21799,10 @@ "pattern": "^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$", "type": "string" }, + "linkToken": { + "description": "OAuth link token to consume on successful login", + "type": "string" + }, "name": { "description": "User name", "example": "Admin", diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 78eb900512..29829ca408 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -1008,6 +1008,8 @@ export type AssetOcrResponseDto = { export type SignUpDto = { /** User email */ email: string; + /** OAuth link token to consume on successful login */ + linkToken?: string; /** User name */ name: string; /** User password */ @@ -1024,6 +1026,8 @@ export type ChangePasswordDto = { export type LoginCredentialDto = { /** User email */ email: string; + /** OAuth link token to consume on successful login */ + linkToken?: string; /** User password */ password: string; }; @@ -1421,16 +1425,6 @@ export type OAuthCallbackDto = { /** OAuth callback URL */ url: string; }; -export type OAuthLinkDto = { - /** OAuth code verifier (PKCE) */ - codeVerifier?: string; - /** OAuth link token from prior callback */ - linkToken?: string; - /** OAuth state parameter */ - state?: string; - /** OAuth callback URL */ - url?: string; -}; export type PartnerResponseDto = { avatarColor: UserAvatarColor; /** User email */ @@ -4954,21 +4948,6 @@ export function finishOAuth({ oAuthCallbackDto }: { body: oAuthCallbackDto }))); } -/** - * Link OAuth account - */ -export function linkOAuthAccount({ oAuthLinkDto }: { - oAuthLinkDto: OAuthLinkDto; -}, opts?: Oazapfts.RequestOpts) { - return oazapfts.ok(oazapfts.fetchJson<{ - status: 200; - data: UserAdminResponseDto; - }>("/oauth/link", oazapfts.json({ - ...opts, - method: "POST", - body: oAuthLinkDto - }))); -} /** * Redirect OAuth to mobile */ diff --git a/server/src/controllers/oauth.controller.ts b/server/src/controllers/oauth.controller.ts index 4cbb636e2a..de5e182489 100644 --- a/server/src/controllers/oauth.controller.ts +++ b/server/src/controllers/oauth.controller.ts @@ -9,7 +9,6 @@ import { OAuthBackchannelLogoutDto, OAuthCallbackDto, OAuthConfigDto, - OAuthLinkDto, } from 'src/dtos/auth.dto'; import { UserAdminResponseDto } from 'src/dtos/user.dto'; import { ApiTag, AuthType, ImmichCookie } from 'src/enum'; @@ -87,22 +86,6 @@ export class OAuthController { }); } - @Post('link') - @Authenticated() - @HttpCode(HttpStatus.OK) - @Endpoint({ - summary: 'Link OAuth account', - description: 'Link an OAuth account to the authenticated user.', - history: new HistoryBuilder().added('v1').beta('v1').stable('v2'), - }) - linkOAuthAccount( - @Req() request: Request, - @Auth() auth: AuthDto, - @Body() dto: OAuthLinkDto, - ): Promise { - return this.service.link(auth, dto, request.headers); - } - @Post('unlink') @Authenticated() @HttpCode(HttpStatus.OK) diff --git a/server/src/dtos/auth.dto.ts b/server/src/dtos/auth.dto.ts index 297ae80f7c..b111810381 100644 --- a/server/src/dtos/auth.dto.ts +++ b/server/src/dtos/auth.dto.ts @@ -23,6 +23,7 @@ const LoginCredentialSchema = z .object({ email: toEmail.describe('User email').meta({ example: 'testuser@email.com' }), password: z.string().describe('User password').meta({ example: 'password' }), + linkToken: z.string().optional().describe('OAuth link token to consume on successful login'), }) .meta({ id: 'LoginCredentialDto' }); @@ -110,18 +111,6 @@ const OAuthCallbackSchema = z }) .meta({ id: 'OAuthCallbackDto' }); -const OAuthLinkSchema = z - .object({ - url: z.string().optional().describe('OAuth callback URL'), - state: z.string().optional().describe('OAuth state parameter'), - codeVerifier: z.string().optional().describe('OAuth code verifier (PKCE)'), - linkToken: z.string().optional().describe('OAuth link token from prior callback'), - }) - .refine((data) => data.url || data.linkToken, { - message: 'Either url or linkToken is required', - }) - .meta({ id: 'OAuthLinkDto' }); - const OAuthConfigSchema = z .object({ redirectUri: z.string().describe('OAuth redirect URI'), @@ -161,7 +150,6 @@ export class SessionUnlockDto extends createZodDto(SessionUnlockSchema) {} export class PinCodeChangeDto extends createZodDto(PinCodeChangeSchema) {} export class ValidateAccessTokenResponseDto extends createZodDto(ValidateAccessTokenResponseSchema) {} export class OAuthCallbackDto extends createZodDto(OAuthCallbackSchema) {} -export class OAuthLinkDto extends createZodDto(OAuthLinkSchema) {} export class OAuthConfigDto extends createZodDto(OAuthConfigSchema) {} export class OAuthAuthorizeResponseDto extends createZodDto(OAuthAuthorizeResponseSchema) {} export class OAuthBackchannelLogoutDto extends createZodDto(OAuthBackchannelLogoutSchema) {} diff --git a/server/src/repositories/oauth-link-token.repository.ts b/server/src/repositories/oauth-link-token.repository.ts index 4e03d52fd5..737ca8c025 100644 --- a/server/src/repositories/oauth-link-token.repository.ts +++ b/server/src/repositories/oauth-link-token.repository.ts @@ -13,7 +13,6 @@ export class OAuthLinkTokenRepository { return this.db.insertInto('oauth_link_token').values(dto).returningAll().executeTakeFirstOrThrow(); } - // Atomic consume: delete and return in one query (single-use guarantee) consumeToken(token: Buffer) { return this.db .deleteFrom('oauth_link_token') diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 3046592838..772f057fba 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -88,6 +88,37 @@ describe(AuthService.name, () => { expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); + + it('should link an OAuth account when linkToken is provided', async () => { + const user = UserFactory.create({ password: 'immich_password' }); + const session = SessionFactory.create(); + mocks.user.getByEmail.mockResolvedValue(user); + mocks.session.create.mockResolvedValue(session); + mocks.oauthLinkToken.consumeToken.mockResolvedValue({ + id: 'token-id', + oauthSub: 'oauth-sub-123', + userEmail: user.email, + token: Buffer.from('hashed'), + expiresAt: new Date(Date.now() + 600_000), + createdAt: new Date(), + }); + mocks.user.update.mockResolvedValue(user); + + await sut.login({ email, password: 'password', linkToken: 'plain-token' }, loginDetails); + + expect(mocks.oauthLinkToken.consumeToken).toHaveBeenCalledTimes(1); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: 'oauth-sub-123' }); + }); + + it('should reject login with invalid linkToken', async () => { + const user = UserFactory.create({ password: 'immich_password' }); + mocks.user.getByEmail.mockResolvedValue(user); + mocks.oauthLinkToken.consumeToken.mockResolvedValue(null as any); + + await expect(sut.login({ email, password: 'password', linkToken: 'bad-token' }, loginDetails)).rejects.toThrow( + 'Invalid or expired link token', + ); + }); }); describe('changePassword', () => { @@ -718,7 +749,7 @@ describe(AuthService.name, () => { {}, loginDetails, ), - ).rejects.toThrow('oauth_account_link_required'); + ).rejects.toThrow(ForbiddenException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); expect(mocks.user.update).not.toHaveBeenCalled(); @@ -762,7 +793,7 @@ describe(AuthService.name, () => { {}, loginDetails, ), - ).rejects.toThrow('oauth_account_link_required'); + ).rejects.toThrow(ForbiddenException); expect(mocks.user.update).not.toHaveBeenCalled(); expect(mocks.user.create).not.toHaveBeenCalled(); diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index db58c925e7..03f6e3ead8 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -75,6 +75,21 @@ export class AuthService extends BaseService { throw new UnauthorizedException('Incorrect email or password'); } + if (dto.linkToken) { + const hashedToken = this.cryptoRepository.hashSha256(dto.linkToken); + const record = await this.oauthLinkTokenRepository.consumeToken(hashedToken); + if (!record) { + throw new BadRequestException('Invalid or expired link token'); + } + + const duplicate = await this.userRepository.getByOAuthId(record.oauthSub); + if (duplicate && duplicate.id !== user.id) { + throw new BadRequestException('This OAuth account has already been linked to another user.'); + } + + await this.userRepository.update(user.id, { oauthId: record.oauthSub }); + } + return this.createLoginResponse(user, details); } @@ -330,9 +345,9 @@ export class AuthService extends BaseService { token: hashedToken, oauthSub: profile.sub, userEmail: emailUser.email, - expiresAt: new Date(Date.now() + 10 * 60 * 1000), + expiresAt: DateTime.now().plus({ minutes: 10 }).toJSDate(), }); - throw new BadRequestException({ + throw new ForbiddenException({ message: 'oauth_account_link_required', userEmail: emailUser.email, linkToken: plainToken, diff --git a/web/src/lib/utils.ts b/web/src/lib/utils.ts index 8012f9cb1e..6f7e3449d4 100644 --- a/web/src/lib/utils.ts +++ b/web/src/lib/utils.ts @@ -15,7 +15,6 @@ import { getBaseUrl, getPeopleThumbnailPath, getUserProfileImagePath, - linkOAuthAccount, startOAuth, unlinkOAuthAccount, type AssetResponseDto, @@ -293,9 +292,6 @@ export const oauth = { login: (location: Location) => { return finishOAuth({ oAuthCallbackDto: { url: location.href } }); }, - link: (location: Location) => { - return linkOAuthAccount({ oAuthLinkDto: { url: location.href } }); - }, unlink: () => { return unlinkOAuthAccount(); }, diff --git a/web/src/routes/auth/link/+page.svelte b/web/src/routes/auth/link/+page.svelte index bf3d6e3a88..e20d6590a2 100644 --- a/web/src/routes/auth/link/+page.svelte +++ b/web/src/routes/auth/link/+page.svelte @@ -6,7 +6,7 @@ import { featureFlagsManager } from '$lib/managers/feature-flags-manager.svelte'; import { Route } from '$lib/route'; import { getServerErrorMessage } from '$lib/utils/handle-error'; - import { linkOAuthAccount, login } from '@immich/sdk'; + import { login } from '@immich/sdk'; import { Alert, Button, Field, Input, PasswordInput, Stack, toastManager } from '@immich/ui'; import { t } from 'svelte-i18n'; import type { PageData } from './$types'; @@ -17,24 +17,22 @@ let { data }: Props = $props(); + let linkToken = $state(data.linkToken); let email = $state(data.email || authManager.user?.email || ''); let password = $state(''); let errorMessage = $state(''); let loading = $state(false); - // Strip sensitive params from URL after reading — keeps linkToken out of browser history - history.replaceState(null, '', Route.authLink()); + goto(Route.authLink(), { replaceState: true }); const handleSubmit = async (event: Event) => { event.preventDefault(); try { errorMessage = ''; loading = true; - const user = await login({ loginCredentialDto: { email, password } }); + const user = await login({ loginCredentialDto: { email, password, linkToken } }); eventManager.emit('AuthLogin', user); - - const response = await linkOAuthAccount({ oAuthLinkDto: { linkToken: data.linkToken } }); - authManager.setUser(response); + await authManager.refresh(); toastManager.primary($t('linked_oauth_account')); await goto(Route.photos(), { invalidateAll: true }); } catch (error) {