From d50ea005a1c5d9f99aa7c5240ca077686df3c1ff Mon Sep 17 00:00:00 2001 From: bo0tzz Date: Sat, 18 Apr 2026 12:27:02 +0200 Subject: [PATCH] feat: manage link token via cookie instead --- e2e/src/specs/server/api/oauth.e2e-spec.ts | 12 +- mobile/openapi/README.md | 1 - .../openapi/lib/api/authentication_api.dart | 56 --------- .../lib/model/login_credential_dto.dart | 20 +--- mobile/openapi/lib/model/sign_up_dto.dart | 20 +--- open-api/immich-openapi-specs.json | 8 -- open-api/typescript-sdk/src/fetch-client.ts | 4 - .../src/controllers/auth.controller.spec.ts | 7 +- server/src/controllers/auth.controller.ts | 6 +- server/src/controllers/oauth.controller.ts | 36 ++++-- server/src/dtos/auth.dto.ts | 1 - server/src/enum.ts | 1 + ...1776362646907-CreateOAuthLinkTokenTable.ts | 1 + .../schema/tables/oauth-link-token.table.ts | 3 + server/src/services/auth.service.spec.ts | 111 ++++++------------ server/src/services/auth.service.ts | 62 ++++------ server/src/utils/response.ts | 1 + .../specs/services/auth.service.spec.ts | 6 +- web/src/lib/route.ts | 2 +- web/src/routes/auth/link/+page.svelte | 8 +- web/src/routes/auth/link/+page.ts | 2 - web/src/routes/auth/login/+page.svelte | 2 +- 22 files changed, 117 insertions(+), 253 deletions(-) diff --git a/e2e/src/specs/server/api/oauth.e2e-spec.ts b/e2e/src/specs/server/api/oauth.e2e-spec.ts index d7edbb5a6e..e323952ea5 100644 --- a/e2e/src/specs/server/api/oauth.e2e-spec.ts +++ b/e2e/src/specs/server/api/oauth.e2e-spec.ts @@ -363,11 +363,13 @@ describe(`/oauth`, () => { password: 'password', }); const callbackParams = await loginWithOAuth('oauth-user3'); - const { status, body } = await request(app).post('/oauth/callback').send(callbackParams); - expect(status).toBe(403); - expect(body.message).toBe('oauth_account_link_required'); - expect(body.userEmail).toBe('oauth-user3@immich.app'); - expect(body.oauthLinkToken).toBeDefined(); + const response = await request(app).post('/oauth/callback').send(callbackParams); + expect(response.status).toBe(403); + expect(response.body.message).toBe('oauth_account_link_required'); + expect(response.body.userEmail).toBe('oauth-user3@immich.app'); + expect(response.body.oauthLinkToken).toBeUndefined(); + const setCookie = response.headers['set-cookie'] as unknown as string[]; + expect(setCookie.some((cookie) => cookie.startsWith('immich_oauth_link_token='))).toBe(true); }); }); }); diff --git a/mobile/openapi/README.md b/mobile/openapi/README.md index 50bbff2bae..0a55f2d325 100644 --- a/mobile/openapi/README.md +++ b/mobile/openapi/README.md @@ -122,7 +122,6 @@ Class | Method | HTTP request | Description *AuthenticationApi* | [**changePinCode**](doc//AuthenticationApi.md#changepincode) | **PUT** /auth/pin-code | Change pin code *AuthenticationApi* | [**finishOAuth**](doc//AuthenticationApi.md#finishoauth) | **POST** /oauth/callback | Finish OAuth *AuthenticationApi* | [**getAuthStatus**](doc//AuthenticationApi.md#getauthstatus) | **GET** /auth/status | Retrieve auth status -*AuthenticationApi* | [**linkOAuthAccount**](doc//AuthenticationApi.md#linkoauthaccount) | **POST** /oauth/link | Link OAuth account *AuthenticationApi* | [**lockAuthSession**](doc//AuthenticationApi.md#lockauthsession) | **POST** /auth/session/lock | Lock auth session *AuthenticationApi* | [**login**](doc//AuthenticationApi.md#login) | **POST** /auth/login | Login *AuthenticationApi* | [**logout**](doc//AuthenticationApi.md#logout) | **POST** /auth/logout | Logout diff --git a/mobile/openapi/lib/api/authentication_api.dart b/mobile/openapi/lib/api/authentication_api.dart index e1219f2c03..bd4fdbd0b9 100644 --- a/mobile/openapi/lib/api/authentication_api.dart +++ b/mobile/openapi/lib/api/authentication_api.dart @@ -224,62 +224,6 @@ class AuthenticationApi { return null; } - /// Link OAuth account - /// - /// Link an OAuth account to the authenticated user. - /// - /// Note: This method returns the HTTP [Response]. - /// - /// Parameters: - /// - /// * [OAuthCallbackDto] oAuthCallbackDto (required): - Future linkOAuthAccountWithHttpInfo(OAuthCallbackDto oAuthCallbackDto,) async { - // ignore: prefer_const_declarations - final apiPath = r'/oauth/link'; - - // ignore: prefer_final_locals - Object? postBody = oAuthCallbackDto; - - final queryParams = []; - final headerParams = {}; - final formParams = {}; - - const contentTypes = ['application/json']; - - - return apiClient.invokeAPI( - apiPath, - 'POST', - queryParams, - postBody, - headerParams, - formParams, - contentTypes.isEmpty ? null : contentTypes.first, - ); - } - - /// Link OAuth account - /// - /// Link an OAuth account to the authenticated user. - /// - /// Parameters: - /// - /// * [OAuthCallbackDto] oAuthCallbackDto (required): - Future linkOAuthAccount(OAuthCallbackDto oAuthCallbackDto,) async { - final response = await linkOAuthAccountWithHttpInfo(oAuthCallbackDto,); - if (response.statusCode >= HttpStatus.badRequest) { - throw ApiException(response.statusCode, await _decodeBodyBytes(response)); - } - // When a remote server returns no body with a status of 204, we shall not decode it. - // At the time of writing this, `dart:convert` will throw an "Unexpected end of input" - // FormatException when trying to decode an empty string. - if (response.body.isNotEmpty && response.statusCode != HttpStatus.noContent) { - return await apiClient.deserializeAsync(await _decodeBodyBytes(response), 'UserAdminResponseDto',) as UserAdminResponseDto; - - } - return null; - } - /// Lock auth session /// /// Remove elevated access to locked assets from the current session. diff --git a/mobile/openapi/lib/model/login_credential_dto.dart b/mobile/openapi/lib/model/login_credential_dto.dart index 693bd9b5ed..1fdfdc3d40 100644 --- a/mobile/openapi/lib/model/login_credential_dto.dart +++ b/mobile/openapi/lib/model/login_credential_dto.dart @@ -14,49 +14,32 @@ class LoginCredentialDto { /// Returns a new [LoginCredentialDto] instance. LoginCredentialDto({ required this.email, - this.oauthLinkToken, required this.password, }); /// User email String email; - /// OAuth link token to consume on successful login - /// - /// Please note: This property should have been non-nullable! Since the specification file - /// does not include a default value (using the "default:" property), however, the generated - /// source code must fall back to having a nullable type. - /// Consider adding a "default:" property in the specification file to hide this note. - /// - String? oauthLinkToken; - /// User password String password; @override bool operator ==(Object other) => identical(this, other) || other is LoginCredentialDto && other.email == email && - other.oauthLinkToken == oauthLinkToken && other.password == password; @override int get hashCode => // ignore: unnecessary_parenthesis (email.hashCode) + - (oauthLinkToken == null ? 0 : oauthLinkToken!.hashCode) + (password.hashCode); @override - String toString() => 'LoginCredentialDto[email=$email, oauthLinkToken=$oauthLinkToken, password=$password]'; + String toString() => 'LoginCredentialDto[email=$email, password=$password]'; Map toJson() { final json = {}; json[r'email'] = this.email; - if (this.oauthLinkToken != null) { - json[r'oauthLinkToken'] = this.oauthLinkToken; - } else { - // json[r'oauthLinkToken'] = null; - } json[r'password'] = this.password; return json; } @@ -71,7 +54,6 @@ class LoginCredentialDto { return LoginCredentialDto( email: mapValueOfType(json, r'email')!, - oauthLinkToken: mapValueOfType(json, r'oauthLinkToken'), password: mapValueOfType(json, r'password')!, ); } diff --git a/mobile/openapi/lib/model/sign_up_dto.dart b/mobile/openapi/lib/model/sign_up_dto.dart index a6c60c60a8..54c8fa07d2 100644 --- a/mobile/openapi/lib/model/sign_up_dto.dart +++ b/mobile/openapi/lib/model/sign_up_dto.dart @@ -15,7 +15,6 @@ class SignUpDto { SignUpDto({ required this.email, required this.name, - this.oauthLinkToken, required this.password, }); @@ -25,15 +24,6 @@ class SignUpDto { /// User name String name; - /// OAuth link token to consume on successful login - /// - /// Please note: This property should have been non-nullable! Since the specification file - /// does not include a default value (using the "default:" property), however, the generated - /// source code must fall back to having a nullable type. - /// Consider adding a "default:" property in the specification file to hide this note. - /// - String? oauthLinkToken; - /// User password String password; @@ -41,7 +31,6 @@ class SignUpDto { bool operator ==(Object other) => identical(this, other) || other is SignUpDto && other.email == email && other.name == name && - other.oauthLinkToken == oauthLinkToken && other.password == password; @override @@ -49,21 +38,15 @@ class SignUpDto { // ignore: unnecessary_parenthesis (email.hashCode) + (name.hashCode) + - (oauthLinkToken == null ? 0 : oauthLinkToken!.hashCode) + (password.hashCode); @override - String toString() => 'SignUpDto[email=$email, name=$name, oauthLinkToken=$oauthLinkToken, password=$password]'; + String toString() => 'SignUpDto[email=$email, name=$name, password=$password]'; Map toJson() { final json = {}; json[r'email'] = this.email; json[r'name'] = this.name; - if (this.oauthLinkToken != null) { - json[r'oauthLinkToken'] = this.oauthLinkToken; - } else { - // json[r'oauthLinkToken'] = null; - } json[r'password'] = this.password; return json; } @@ -79,7 +62,6 @@ class SignUpDto { return SignUpDto( email: mapValueOfType(json, r'email')!, name: mapValueOfType(json, r'name')!, - oauthLinkToken: mapValueOfType(json, r'oauthLinkToken'), password: mapValueOfType(json, r'password')!, ); } diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 89f9d0d932..282ae03b8c 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -18013,10 +18013,6 @@ "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" }, - "oauthLinkToken": { - "description": "OAuth link token to consume on successful login", - "type": "string" - }, "password": { "description": "User password", "example": "password", @@ -21804,10 +21800,6 @@ "example": "Admin", "type": "string" }, - "oauthLinkToken": { - "description": "OAuth link token to consume on successful login", - "type": "string" - }, "password": { "description": "User password", "example": "password", diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 219e947845..8b6ac68976 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -1010,8 +1010,6 @@ export type SignUpDto = { email: string; /** User name */ name: string; - /** OAuth link token to consume on successful login */ - oauthLinkToken?: string; /** User password */ password: string; }; @@ -1026,8 +1024,6 @@ export type ChangePasswordDto = { export type LoginCredentialDto = { /** User email */ email: string; - /** OAuth link token to consume on successful login */ - oauthLinkToken?: string; /** User password */ password: string; }; diff --git a/server/src/controllers/auth.controller.spec.ts b/server/src/controllers/auth.controller.spec.ts index a61397e75c..79f239983d 100644 --- a/server/src/controllers/auth.controller.spec.ts +++ b/server/src/controllers/auth.controller.spec.ts @@ -118,6 +118,7 @@ describe(AuthController.name, () => { expect(service.login).toHaveBeenCalledWith( expect.objectContaining({ email: 'admin@immich.app' }), expect.anything(), + expect.anything(), ); }); @@ -129,7 +130,11 @@ describe(AuthController.name, () => { .send({ name: 'admin', email: 'admin@local', password: 'password' }); expect(status).toEqual(201); - expect(service.login).toHaveBeenCalledWith(expect.objectContaining({ email: 'admin@local' }), expect.anything()); + expect(service.login).toHaveBeenCalledWith( + expect.objectContaining({ email: 'admin@local' }), + expect.anything(), + expect.anything(), + ); }); it('should auth cookies on a secure connection', async () => { diff --git a/server/src/controllers/auth.controller.ts b/server/src/controllers/auth.controller.ts index 63cdce4f32..37de5b5e75 100644 --- a/server/src/controllers/auth.controller.ts +++ b/server/src/controllers/auth.controller.ts @@ -34,11 +34,15 @@ export class AuthController { history: new HistoryBuilder().added('v1').beta('v1').stable('v2'), }) async login( + @Req() request: Request, @Res({ passthrough: true }) res: Response, @Body() loginCredential: LoginCredentialDto, @GetLoginDetails() loginDetails: LoginDetails, ): Promise { - const body = await this.service.login(loginCredential, loginDetails); + const body = await this.service.login(loginCredential, loginDetails, request.headers); + if (request.cookies?.[ImmichCookie.OAuthLinkToken]) { + res.clearCookie(ImmichCookie.OAuthLinkToken); + } return respondWithCookie(res, body, { isSecure: loginDetails.isSecure, values: [ diff --git a/server/src/controllers/oauth.controller.ts b/server/src/controllers/oauth.controller.ts index de5e182489..0d9e737ff0 100644 --- a/server/src/controllers/oauth.controller.ts +++ b/server/src/controllers/oauth.controller.ts @@ -13,7 +13,7 @@ import { import { UserAdminResponseDto } from 'src/dtos/user.dto'; import { ApiTag, AuthType, ImmichCookie } from 'src/enum'; import { Auth, Authenticated, GetLoginDetails } from 'src/middleware/auth.guard'; -import { AuthService, LoginDetails } from 'src/services/auth.service'; +import { AuthService, LoginDetails, OAuthLinkRequiredException } from 'src/services/auth.service'; import { respondWithCookie } from 'src/utils/response'; @ApiTags(ApiTag.Authentication) @@ -73,17 +73,29 @@ export class OAuthController { @Body() dto: OAuthCallbackDto, @GetLoginDetails() loginDetails: LoginDetails, ): Promise { - const body = await this.service.callback(dto, request.headers, loginDetails); - res.clearCookie(ImmichCookie.OAuthState); - res.clearCookie(ImmichCookie.OAuthCodeVerifier); - return respondWithCookie(res, body, { - isSecure: loginDetails.isSecure, - values: [ - { key: ImmichCookie.AccessToken, value: body.accessToken }, - { key: ImmichCookie.AuthType, value: AuthType.OAuth }, - { key: ImmichCookie.IsAuthenticated, value: 'true' }, - ], - }); + try { + const body = await this.service.callback(dto, request.headers, loginDetails); + res.clearCookie(ImmichCookie.OAuthState); + res.clearCookie(ImmichCookie.OAuthCodeVerifier); + return respondWithCookie(res, body, { + isSecure: loginDetails.isSecure, + values: [ + { key: ImmichCookie.AccessToken, value: body.accessToken }, + { key: ImmichCookie.AuthType, value: AuthType.OAuth }, + { key: ImmichCookie.IsAuthenticated, value: 'true' }, + ], + }); + } catch (error) { + if (error instanceof OAuthLinkRequiredException) { + res.clearCookie(ImmichCookie.OAuthState); + res.clearCookie(ImmichCookie.OAuthCodeVerifier); + respondWithCookie(res, null, { + isSecure: loginDetails.isSecure, + values: [{ key: ImmichCookie.OAuthLinkToken, value: error.oauthLinkToken }], + }); + } + throw error; + } } @Post('unlink') diff --git a/server/src/dtos/auth.dto.ts b/server/src/dtos/auth.dto.ts index 54e7fae0bc..1f75401e33 100644 --- a/server/src/dtos/auth.dto.ts +++ b/server/src/dtos/auth.dto.ts @@ -23,7 +23,6 @@ const LoginCredentialSchema = z .object({ email: toEmail.describe('User email').meta({ example: 'testuser@email.com' }), password: z.string().describe('User password').meta({ example: 'password' }), - oauthLinkToken: z.string().optional().describe('OAuth link token to consume on successful login'), }) .meta({ id: 'LoginCredentialDto' }); diff --git a/server/src/enum.ts b/server/src/enum.ts index 08d710e08c..83ac8fab9e 100644 --- a/server/src/enum.ts +++ b/server/src/enum.ts @@ -15,6 +15,7 @@ export enum ImmichCookie { SharedLinkToken = 'immich_shared_link_token', OAuthState = 'immich_oauth_state', OAuthCodeVerifier = 'immich_oauth_code_verifier', + OAuthLinkToken = 'immich_oauth_link_token', } export const ImmichCookieSchema = z.enum(ImmichCookie).describe('Immich cookie').meta({ id: 'ImmichCookie' }); diff --git a/server/src/schema/migrations/1776362646907-CreateOAuthLinkTokenTable.ts b/server/src/schema/migrations/1776362646907-CreateOAuthLinkTokenTable.ts index 8e06b4ccd0..e732cda4b0 100644 --- a/server/src/schema/migrations/1776362646907-CreateOAuthLinkTokenTable.ts +++ b/server/src/schema/migrations/1776362646907-CreateOAuthLinkTokenTable.ts @@ -6,6 +6,7 @@ export async function up(db: Kysely): Promise { "id" uuid NOT NULL DEFAULT uuid_generate_v4(), "token" bytea NOT NULL, "oauthSub" varchar NOT NULL, + "oauthSid" varchar, "userEmail" varchar NOT NULL, "expiresAt" timestamp with time zone NOT NULL, "createdAt" timestamp with time zone NOT NULL DEFAULT now() diff --git a/server/src/schema/tables/oauth-link-token.table.ts b/server/src/schema/tables/oauth-link-token.table.ts index beadc1bc9d..60e9c0d414 100644 --- a/server/src/schema/tables/oauth-link-token.table.ts +++ b/server/src/schema/tables/oauth-link-token.table.ts @@ -11,6 +11,9 @@ export class OAuthLinkTokenTable { @Column() oauthSub!: string; + @Column({ nullable: true }) + oauthSid!: string | null; + @Column() userEmail!: string; diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 64e99294b1..d9e7abc763 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -4,7 +4,7 @@ import { SALT_ROUNDS } from 'src/constants'; import { UserAdmin } from 'src/database'; import { AuthDto, SignUpDto } from 'src/dtos/auth.dto'; import { AuthType, Permission } from 'src/enum'; -import { AuthService } from 'src/services/auth.service'; +import { AuthService, OAuthLinkRequiredException } from 'src/services/auth.service'; import { UserMetadataItem } from 'src/types'; import { ApiKeyFactory } from 'test/factories/api-key.factory'; import { AuthFactory } from 'test/factories/auth.factory'; @@ -50,13 +50,13 @@ describe(AuthService.name, () => { it('should throw an error if password login is disabled', async () => { mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.disabled); - await expect(sut.login(dto, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.login(dto, loginDetails, {})).rejects.toBeInstanceOf(UnauthorizedException); }); it('should check the user exists', async () => { mocks.user.getByEmail.mockResolvedValue(void 0); - await expect(sut.login(dto, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.login(dto, loginDetails, {})).rejects.toBeInstanceOf(UnauthorizedException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); @@ -64,7 +64,7 @@ describe(AuthService.name, () => { it('should check the user has a password', async () => { mocks.user.getByEmail.mockResolvedValue({} as UserAdmin); - await expect(sut.login(dto, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.login(dto, loginDetails, {})).rejects.toBeInstanceOf(UnauthorizedException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); @@ -75,7 +75,7 @@ describe(AuthService.name, () => { mocks.user.getByEmail.mockResolvedValue(user); mocks.session.create.mockResolvedValue(session); - await expect(sut.login(dto, loginDetails)).resolves.toEqual({ + await expect(sut.login(dto, loginDetails, {})).resolves.toEqual({ accessToken: 'cmFuZG9tLWJ5dGVz', userId: user.id, userEmail: user.email, @@ -89,7 +89,7 @@ describe(AuthService.name, () => { expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); - it('should link an OAuth account when oauthLinkToken is provided', async () => { + it('should link an OAuth account when link token cookie is present', async () => { const user = UserFactory.create({ password: 'immich_password' }); const session = SessionFactory.create(); mocks.user.getByEmail.mockResolvedValue(user); @@ -97,6 +97,7 @@ describe(AuthService.name, () => { mocks.oauthLinkToken.consumeToken.mockResolvedValue({ id: 'token-id', oauthSub: 'oauth-sub-123', + oauthSid: null, userEmail: user.email, token: Buffer.from('hashed'), expiresAt: new Date(Date.now() + 600_000), @@ -104,20 +105,41 @@ describe(AuthService.name, () => { }); mocks.user.update.mockResolvedValue(user); - await sut.login({ email, password: 'password', oauthLinkToken: 'plain-token' }, loginDetails); + await sut.login(dto, loginDetails, { cookie: 'immich_oauth_link_token=plain-token' }); expect(mocks.oauthLinkToken.consumeToken).toHaveBeenCalledTimes(1); expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: 'oauth-sub-123' }); }); - it('should reject login with invalid oauthLinkToken', async () => { + it('should propagate oauthSid from link token to the session', 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', + oauthSid: 'idp-sid-456', + 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(dto, loginDetails, { cookie: 'immich_oauth_link_token=plain-token' }); + + expect(mocks.session.create).toHaveBeenCalledWith(expect.objectContaining({ oauthSid: 'idp-sid-456' })); + }); + + it('should reject login with invalid link token cookie', 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', oauthLinkToken: 'bad-token' }, loginDetails), - ).rejects.toThrow('Invalid or expired link token'); + await expect(sut.login(dto, loginDetails, { cookie: 'immich_oauth_link_token=bad-token' })).rejects.toThrow( + 'Invalid or expired link token', + ); }); }); @@ -738,7 +760,7 @@ describe(AuthService.name, () => { const profile = OAuthProfileFactory.create(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); + mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile, sid: 'idp-sid-789' }); mocks.user.getByEmail.mockResolvedValue(user); mocks.oauthLinkToken.deleteByEmail.mockResolvedValue(); mocks.oauthLinkToken.create.mockResolvedValue({} as any); @@ -749,12 +771,14 @@ describe(AuthService.name, () => { {}, loginDetails, ), - ).rejects.toThrow(ForbiddenException); + ).rejects.toThrow(OAuthLinkRequiredException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); expect(mocks.user.update).not.toHaveBeenCalled(); expect(mocks.oauthLinkToken.deleteByEmail).toHaveBeenCalledTimes(1); - expect(mocks.oauthLinkToken.create).toHaveBeenCalledTimes(1); + expect(mocks.oauthLinkToken.create).toHaveBeenCalledWith( + expect.objectContaining({ oauthSub: profile.sub, oauthSid: 'idp-sid-789' }), + ); }); it('should normalize the email from the OAuth profile before looking up user', async () => { @@ -1136,65 +1160,6 @@ describe(AuthService.name, () => { }); }); - describe('link', () => { - it('should link an account', async () => { - const user = UserFactory.create(); - const auth = AuthFactory.from(user).apiKey({ permissions: [] }).build(); - const profile = OAuthProfileFactory.create(); - - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile }); - mocks.user.update.mockResolvedValue(user); - - await sut.link( - auth, - { url: 'http://immich/user-settings?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - ); - - expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: profile.sub }); - }); - - it('should link an account and update the session with the oauthSid', async () => { - const user = UserFactory.create(); - const session = SessionFactory.create(); - const auth = AuthFactory.from(user).session(session).build(); - - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ - profile: { sub: 'sub' }, - sid: session.oauthSid ?? undefined, - }); - mocks.user.update.mockResolvedValue(user); - mocks.session.update.mockResolvedValue(session); - - await sut.link( - auth, - { url: 'http://immich/user-settings?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - ); - - expect(mocks.session.update).toHaveBeenCalledWith(session.id, { oauthSid: session.oauthSid }); - expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: 'sub' }); - }); - - it('should not link an already linked oauth.sub', async () => { - const authUser = UserFactory.create(); - const authApiKey = ApiKeyFactory.create({ permissions: [] }); - const auth = { user: authUser, apiKey: authApiKey }; - - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); - mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() }); - mocks.user.getByOAuthId.mockResolvedValue({ id: 'other-user' } as UserAdmin); - - await expect( - sut.link(auth, { url: 'http://immich/user-settings?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, {}), - ).rejects.toBeInstanceOf(BadRequestException); - - expect(mocks.user.update).not.toHaveBeenCalled(); - }); - }); - describe('unlink', () => { it('should unlink an account', async () => { const user = UserFactory.create(); diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index 9d21e2c225..d5048f7065 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -42,6 +42,15 @@ interface ClaimOptions { isValid: (value: unknown) => boolean; } +export class OAuthLinkRequiredException extends ForbiddenException { + constructor( + public readonly userEmail: string, + public readonly oauthLinkToken: string, + ) { + super({ message: 'oauth_account_link_required', userEmail }); + } +} + export type ValidateRequest = { headers: IncomingHttpHeaders; queryParams: Record; @@ -56,7 +65,7 @@ export type ValidateRequest = { @Injectable() export class AuthService extends BaseService { - async login(dto: LoginCredentialDto, details: LoginDetails) { + async login(dto: LoginCredentialDto, details: LoginDetails, headers: IncomingHttpHeaders) { const config = await this.getConfig({ withCache: false }); if (!config.passwordLogin.enabled) { throw new UnauthorizedException('Password login has been disabled'); @@ -75,8 +84,10 @@ export class AuthService extends BaseService { throw new UnauthorizedException('Incorrect email or password'); } - if (dto.oauthLinkToken) { - const hashedToken = this.cryptoRepository.hashSha256(dto.oauthLinkToken); + let linkedOAuthSid: string | undefined; + const linkTokenCookie = this.getCookieOAuthLinkToken(headers); + if (linkTokenCookie) { + const hashedToken = this.cryptoRepository.hashSha256(linkTokenCookie); const record = await this.oauthLinkTokenRepository.consumeToken(hashedToken); if (!record) { throw new BadRequestException('Invalid or expired link token'); @@ -88,9 +99,10 @@ export class AuthService extends BaseService { } await this.userRepository.update(user.id, { oauthId: record.oauthSub }); + linkedOAuthSid = record.oauthSid ?? undefined; } - return this.createLoginResponse(user, details); + return this.createLoginResponse(user, details, linkedOAuthSid); } async logout(auth: AuthDto, authType: AuthType): Promise { @@ -344,14 +356,11 @@ export class AuthService extends BaseService { await this.oauthLinkTokenRepository.create({ token: hashedToken, oauthSub: profile.sub, + oauthSid: oauthSid ?? null, userEmail: emailUser.email, expiresAt: DateTime.now().plus({ minutes: 10 }).toJSDate(), }); - throw new ForbiddenException({ - message: 'oauth_account_link_required', - userEmail: emailUser.email, - oauthLinkToken: plainToken, - }); + throw new OAuthLinkRequiredException(emailUser.email, plainToken); } } @@ -430,36 +439,6 @@ export class AuthService extends BaseService { } } - async link(auth: AuthDto, dto: OAuthCallbackDto, headers: IncomingHttpHeaders): Promise { - const expectedState = dto.state ?? this.getCookieOauthState(headers); - if (!expectedState?.length) { - throw new BadRequestException('OAuth state is missing'); - } - - const codeVerifier = dto.codeVerifier ?? this.getCookieCodeVerifier(headers); - if (!codeVerifier?.length) { - throw new BadRequestException('OAuth code verifier is missing'); - } - - const { oauth } = await this.getConfig({ withCache: false }); - const { - profile: { sub: oauthId }, - sid, - } = await this.oauthRepository.getProfileAndOAuthSid(oauth, dto.url, expectedState, codeVerifier); - 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}).`); - throw new BadRequestException('This OAuth account has already been linked to another user.'); - } - - if (auth.session) { - await this.sessionRepository.update(auth.session.id, { oauthSid: sid }); - } - - const user = await this.userRepository.update(auth.user.id, { oauthId }); - return mapUserAdmin(user); - } - async unlink(auth: AuthDto): Promise { if (auth.session) { await this.sessionRepository.update(auth.session.id, { oauthSid: null }); @@ -510,6 +489,11 @@ export class AuthService extends BaseService { return cookies[ImmichCookie.OAuthCodeVerifier] || null; } + private getCookieOAuthLinkToken(headers: IncomingHttpHeaders): string | null { + const cookies = parse(headers.cookie || ''); + return cookies[ImmichCookie.OAuthLinkToken] || null; + } + async validateSharedLinkKey(key: string | string[]): Promise { key = Array.isArray(key) ? key[0] : key; diff --git a/server/src/utils/response.ts b/server/src/utils/response.ts index d5356285f0..272a64ecc5 100644 --- a/server/src/utils/response.ts +++ b/server/src/utils/response.ts @@ -18,6 +18,7 @@ export const respondWithCookie = (res: Response, body: T, { isSecure, values [ImmichCookie.MaintenanceToken]: { ...defaults, maxAge: Duration.fromObject({ days: 1 }).toMillis() }, [ImmichCookie.OAuthState]: defaults, [ImmichCookie.OAuthCodeVerifier]: defaults, + [ImmichCookie.OAuthLinkToken]: { ...defaults, maxAge: Duration.fromObject({ minutes: 10 }).toMillis() }, // no httpOnly so that the client can know the auth state [ImmichCookie.IsAuthenticated]: { ...defaults, httpOnly: false }, [ImmichCookie.SharedLinkToken]: { ...defaults, maxAge: Duration.fromObject({ days: 1 }).toMillis() }, diff --git a/server/test/medium/specs/services/auth.service.spec.ts b/server/test/medium/specs/services/auth.service.spec.ts index 1fc306f790..28e668f135 100644 --- a/server/test/medium/specs/services/auth.service.spec.ts +++ b/server/test/medium/specs/services/auth.service.spec.ts @@ -77,7 +77,7 @@ describe(AuthService.name, () => { const { user } = await ctx.newUser({ password: passwordHashed }); const dto = { email: user.email, password: 'wrong-password' }; - await expect(sut.login(dto, mediumFactory.loginDetails())).rejects.toThrow('Incorrect email or password'); + await expect(sut.login(dto, mediumFactory.loginDetails(), {})).rejects.toThrow('Incorrect email or password'); }); it('should accept a correct password and return a login response', async () => { @@ -87,7 +87,7 @@ describe(AuthService.name, () => { const { user } = await ctx.newUser({ password: passwordHashed }); const dto = { email: user.email, password }; - await expect(sut.login(dto, mediumFactory.loginDetails())).resolves.toEqual({ + await expect(sut.login(dto, mediumFactory.loginDetails(), {})).resolves.toEqual({ accessToken: expect.any(String), isAdmin: user.isAdmin, isOnboarded: false, @@ -147,7 +147,7 @@ describe(AuthService.name, () => { expect((response as any).password).not.toBeDefined(); await expect( - sut.login({ email: user.email, password: dto.newPassword }, mediumFactory.loginDetails()), + sut.login({ email: user.email, password: dto.newPassword }, mediumFactory.loginDetails(), {}), ).resolves.toBeDefined(); }); diff --git a/web/src/lib/route.ts b/web/src/lib/route.ts index f3ba7aa67c..0ef3a08e99 100644 --- a/web/src/lib/route.ts +++ b/web/src/lib/route.ts @@ -51,7 +51,7 @@ export const Docs = { export const Route = { // auth login: (params?: { continue?: string; autoLaunch?: 0 | 1 }) => '/auth/login' + asQueryString(params), - authLink: (params?: { oauthLinkToken?: string; email?: string }) => '/auth/link' + asQueryString(params), + authLink: (params?: { email?: string }) => '/auth/link' + asQueryString(params), logout: (params?: { continue?: string }) => '/auth/logout' + asQueryString(params), register: () => '/auth/register', changePassword: () => '/auth/change-password', diff --git a/web/src/routes/auth/link/+page.svelte b/web/src/routes/auth/link/+page.svelte index 65922e631d..48fabaee88 100644 --- a/web/src/routes/auth/link/+page.svelte +++ b/web/src/routes/auth/link/+page.svelte @@ -8,7 +8,6 @@ import { getServerErrorMessage } from '$lib/utils/handle-error'; import { login } from '@immich/sdk'; import { Alert, Button, Field, Input, PasswordInput, Stack, toastManager } from '@immich/ui'; - import { onMount } from 'svelte'; import { t } from 'svelte-i18n'; import type { PageData } from './$types'; @@ -18,22 +17,17 @@ let { data }: Props = $props(); - let oauthLinkToken = $state(data.oauthLinkToken); let email = $state(data.email || authManager.user?.email || ''); let password = $state(''); let errorMessage = $state(''); let loading = $state(false); - onMount(async () => { - await goto(Route.authLink(), { replaceState: true }); - }); - const handleSubmit = async (event: Event) => { event.preventDefault(); try { errorMessage = ''; loading = true; - const user = await login({ loginCredentialDto: { email, password, oauthLinkToken } }); + const user = await login({ loginCredentialDto: { email, password } }); eventManager.emit('AuthLogin', user); await authManager.refresh(); toastManager.primary($t('linked_oauth_account')); diff --git a/web/src/routes/auth/link/+page.ts b/web/src/routes/auth/link/+page.ts index 11d37855b4..425a93f69d 100644 --- a/web/src/routes/auth/link/+page.ts +++ b/web/src/routes/auth/link/+page.ts @@ -2,7 +2,6 @@ import { getFormatter } from '$lib/utils/i18n'; import type { PageLoad } from './$types'; export const load = (async ({ url }) => { - const oauthLinkToken = url.searchParams.get('oauthLinkToken') || ''; const email = url.searchParams.get('email') || ''; const $t = await getFormatter(); @@ -10,7 +9,6 @@ export const load = (async ({ url }) => { meta: { title: $t('link_to_oauth'), }, - oauthLinkToken, email, }; }) satisfies PageLoad; diff --git a/web/src/routes/auth/login/+page.svelte b/web/src/routes/auth/login/+page.svelte index 5353af1683..19f4e00a60 100644 --- a/web/src/routes/auth/login/+page.svelte +++ b/web/src/routes/auth/login/+page.svelte @@ -72,7 +72,7 @@ } catch (error) { if (isHttpError(error) && error.data?.message === 'oauth_account_link_required') { const errorData = error.data as unknown as Record; - await goto(Route.authLink({ oauthLinkToken: errorData.oauthLinkToken, email: errorData.userEmail })); + await goto(Route.authLink({ email: errorData.userEmail })); return; } console.error('Error [login-form] [oauth.callback]', error);