feat(web,server): link/unlink oauth account (#1154)

* feat(web,server): link/unlink oauth account * chore: linting * fix: broken oauth callback * fix: user core bugs * fix: tests * fix: use user response * chore: update docs * feat: prevent the same oauth account from being linked twice * chore: mock logger
2025-12-08 20:29:05 +00:00 · 2022-12-26 10:35:52 -05:00
parent ab0a3690f3
commit 7dc12dea1e
27 changed files with 877 additions and 205 deletions
--- a/server/apps/immich/src/api-v1/auth/auth.service.ts
+++ b/server/apps/immich/src/api-v1/auth/auth.service.ts
@@ -74,9 +74,7 @@ export class AuthService {
      throw new BadRequestException('Wrong password');
    }

-    user.password = newPassword;
-
-    return this.userCore.updateUser(authUser, user, dto);
+    return this.userCore.updateUser(authUser, authUser.id, { password: newPassword });
  }

  public async adminSignUp(dto: SignUpDto): Promise<AdminSignupResponseDto> {
--- a/server/apps/immich/src/api-v1/oauth/oauth.controller.ts
+++ b/server/apps/immich/src/api-v1/oauth/oauth.controller.ts
@@ -3,8 +3,10 @@ import { ApiTags } from '@nestjs/swagger';
 import { Response } from 'express';
 import { AuthType } from '../../constants/jwt.constant';
 import { AuthUserDto, GetAuthUser } from '../../decorators/auth-user.decorator';
+import { Authenticated } from '../../decorators/authenticated.decorator';
 import { ImmichJwtService } from '../../modules/immich-jwt/immich-jwt.service';
 import { LoginResponseDto } from '../auth/response-dto/login-response.dto';
+import { UserResponseDto } from '../user/response-dto/user-response.dto';
 import { OAuthCallbackDto } from './dto/oauth-auth-code.dto';
 import { OAuthConfigDto } from './dto/oauth-config.dto';
 import { OAuthService } from './oauth.service';
@@ -22,12 +24,26 @@ export class OAuthController {

  @Post('/callback')
  public async callback(
-    @GetAuthUser() authUser: AuthUserDto,
    @Res({ passthrough: true }) response: Response,
    @Body(ValidationPipe) dto: OAuthCallbackDto,
  ): Promise<LoginResponseDto> {
-    const loginResponse = await this.oauthService.callback(authUser, dto);
+    const loginResponse = await this.oauthService.login(dto);
    response.setHeader('Set-Cookie', this.immichJwtService.getCookies(loginResponse, AuthType.OAUTH));
    return loginResponse;
  }
+
+  @Authenticated()
+  @Post('link')
+  public async link(
+    @GetAuthUser() authUser: AuthUserDto,
+    @Body(ValidationPipe) dto: OAuthCallbackDto,
+  ): Promise<UserResponseDto> {
+    return this.oauthService.link(authUser, dto);
+  }
+
+  @Authenticated()
+  @Post('unlink')
+  public async unlink(@GetAuthUser() authUser: AuthUserDto): Promise<UserResponseDto> {
+    return this.oauthService.unlink(authUser);
+  }
 }
--- a/server/apps/immich/src/api-v1/oauth/oauth.service.spec.ts
+++ b/server/apps/immich/src/api-v1/oauth/oauth.service.spec.ts
@@ -1,7 +1,7 @@
 import { SystemConfig } from '@app/database/entities/system-config.entity';
 import { UserEntity } from '@app/database/entities/user.entity';
 import { ImmichConfigService } from '@app/immich-config';
-import { BadRequestException } from '@nestjs/common';
+import { BadRequestException, Logger } from '@nestjs/common';
 import { generators, Issuer } from 'openid-client';
 import { AuthUserDto } from '../../decorators/auth-user.decorator';
 import { ImmichJwtService } from '../../modules/immich-jwt/immich-jwt.service';
@@ -32,6 +32,21 @@ const loginResponse = {
  userEmail: 'user@immich.com,',
 } as LoginResponseDto;

+jest.mock('@nestjs/common', () => {
+  return {
+    ...jest.requireActual('@nestjs/common'),
+    Logger: function MockLogger() {
+      Object.assign(this as Logger, {
+        verbose: jest.fn(),
+        debug: jest.fn(),
+        log: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+      });
+    },
+  };
+});
+
 describe('OAuthService', () => {
  let sut: OAuthService;
  let userRepositoryMock: jest.Mocked<IUserRepository>;
@@ -109,9 +124,9 @@ describe('OAuthService', () => {
    });
  });

-  describe('callback', () => {
+  describe('login', () => {
    it('should throw an error if OAuth is not enabled', async () => {
-      await expect(sut.callback(authUser, { url: '' })).rejects.toBeInstanceOf(BadRequestException);
+      await expect(sut.login({ url: '' })).rejects.toBeInstanceOf(BadRequestException);
    });

    it('should not allow auto registering', async () => {
@@ -122,10 +137,8 @@ describe('OAuthService', () => {
        },
      } as SystemConfig);
      sut = new OAuthService(immichJwtServiceMock, immichConfigServiceMock, userRepositoryMock);
-      jest.spyOn(sut['logger'], 'debug').mockImplementation(() => null);
-      jest.spyOn(sut['logger'], 'warn').mockImplementation(() => null);
      userRepositoryMock.getByEmail.mockResolvedValue(null);
-      await expect(sut.callback(authUser, { url: 'http://immich/auth/login?code=abc123' })).rejects.toBeInstanceOf(
+      await expect(sut.login({ url: 'http://immich/auth/login?code=abc123' })).rejects.toBeInstanceOf(
        BadRequestException,
      );
      expect(userRepositoryMock.getByEmail).toHaveBeenCalledTimes(1);
@@ -139,15 +152,11 @@ describe('OAuthService', () => {
        },
      } as SystemConfig);
      sut = new OAuthService(immichJwtServiceMock, immichConfigServiceMock, userRepositoryMock);
-      jest.spyOn(sut['logger'], 'debug').mockImplementation(() => null);
-      jest.spyOn(sut['logger'], 'warn').mockImplementation(() => null);
      userRepositoryMock.getByEmail.mockResolvedValue(user);
      userRepositoryMock.update.mockResolvedValue(user);
      immichJwtServiceMock.createLoginResponse.mockResolvedValue(loginResponse);

-      await expect(sut.callback(authUser, { url: 'http://immich/auth/login?code=abc123' })).resolves.toEqual(
-        loginResponse,
-      );
+      await expect(sut.login({ url: 'http://immich/auth/login?code=abc123' })).resolves.toEqual(loginResponse);

      expect(userRepositoryMock.getByEmail).toHaveBeenCalledTimes(1);
      expect(userRepositoryMock.update).toHaveBeenCalledWith(user.id, { oauthId: sub });
@@ -161,16 +170,12 @@ describe('OAuthService', () => {
        },
      } as SystemConfig);
      sut = new OAuthService(immichJwtServiceMock, immichConfigServiceMock, userRepositoryMock);
-      jest.spyOn(sut['logger'], 'debug').mockImplementation(() => null);
-      jest.spyOn(sut['logger'], 'log').mockImplementation(() => null);
      userRepositoryMock.getByEmail.mockResolvedValue(null);
      userRepositoryMock.getAdmin.mockResolvedValue(user);
      userRepositoryMock.create.mockResolvedValue(user);
      immichJwtServiceMock.createLoginResponse.mockResolvedValue(loginResponse);

-      await expect(sut.callback(authUser, { url: 'http://immich/auth/login?code=abc123' })).resolves.toEqual(
-        loginResponse,
-      );
+      await expect(sut.login({ url: 'http://immich/auth/login?code=abc123' })).resolves.toEqual(loginResponse);

      expect(userRepositoryMock.getByEmail).toHaveBeenCalledTimes(2); // second call is for domain check before create
      expect(userRepositoryMock.create).toHaveBeenCalledTimes(1);
@@ -178,6 +183,57 @@ describe('OAuthService', () => {
    });
  });

+  describe('link', () => {
+    it('should link an account', async () => {
+      immichConfigServiceMock.getConfig.mockResolvedValue({
+        oauth: {
+          enabled: true,
+          autoRegister: true,
+        },
+      } as SystemConfig);
+
+      userRepositoryMock.update.mockResolvedValue(user);
+
+      await sut.link(authUser, { url: 'http://immich/user-settings?code=abc123' });
+
+      expect(userRepositoryMock.update).toHaveBeenCalledWith(authUser.id, { oauthId: sub });
+    });
+
+    it('should not link an already linked oauth.sub', async () => {
+      immichConfigServiceMock.getConfig.mockResolvedValue({
+        oauth: {
+          enabled: true,
+          autoRegister: true,
+        },
+      } as SystemConfig);
+
+      userRepositoryMock.getByOAuthId.mockResolvedValue({ id: 'other-user' } as UserEntity);
+
+      await expect(sut.link(authUser, { url: 'http://immich/user-settings?code=abc123' })).rejects.toBeInstanceOf(
+        BadRequestException,
+      );
+
+      expect(userRepositoryMock.update).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('unlink', () => {
+    it('should unlink an account', async () => {
+      immichConfigServiceMock.getConfig.mockResolvedValue({
+        oauth: {
+          enabled: true,
+          autoRegister: true,
+        },
+      } as SystemConfig);
+
+      userRepositoryMock.update.mockResolvedValue(user);
+
+      await sut.unlink(authUser);
+
+      expect(userRepositoryMock.update).toHaveBeenCalledWith(authUser.id, { oauthId: '' });
+    });
+  });
+
  describe('getLogoutEndpoint', () => {
    it('should return null if OAuth is not configured', async () => {
      await expect(sut.getLogoutEndpoint()).resolves.toBeNull();
--- a/server/apps/immich/src/api-v1/oauth/oauth.service.ts
+++ b/server/apps/immich/src/api-v1/oauth/oauth.service.ts
@@ -4,6 +4,7 @@ import { ClientMetadata, custom, generators, Issuer, UserinfoResponse } from 'op
 import { AuthUserDto } from '../../decorators/auth-user.decorator';
 import { ImmichJwtService } from '../../modules/immich-jwt/immich-jwt.service';
 import { LoginResponseDto } from '../auth/response-dto/login-response.dto';
+import { UserResponseDto } from '../user/response-dto/user-response.dto';
 import { IUserRepository, USER_REPOSITORY } from '../user/user-repository';
 import { UserCore } from '../user/user.core';
 import { OAuthCallbackDto } from './dto/oauth-auth-code.dto';
@@ -47,12 +48,8 @@ export class OAuthService {
    return { enabled: true, buttonText, url };
  }

-  public async callback(authUser: AuthUserDto, dto: OAuthCallbackDto): Promise<LoginResponseDto> {
-    const redirectUri = dto.url.split('?')[0];
-    const client = await this.getClient();
-    const params = client.callbackParams(dto.url);
-    const tokens = await client.callback(redirectUri, params, { state: params.state });
-    const profile = await client.userinfo<OAuthProfile>(tokens.access_token || '');
+  public async login(dto: OAuthCallbackDto): Promise<LoginResponseDto> {
+    const profile = await this.callback(dto.url);

    this.logger.debug(`Logging in with OAuth: ${JSON.stringify(profile)}`);
    let user = await this.userCore.getByOAuthId(profile.sub);
@@ -61,7 +58,7 @@ export class OAuthService {
    if (!user) {
      const emailUser = await this.userCore.getByEmail(profile.email);
      if (emailUser) {
-        user = await this.userCore.updateUser(authUser, emailUser, { oauthId: profile.sub });
+        user = await this.userCore.updateUser(emailUser, emailUser.id, { oauthId: profile.sub });
      }
    }

@@ -88,6 +85,20 @@ export class OAuthService {
    return this.immichJwtService.createLoginResponse(user);
  }

+  public async link(user: AuthUserDto, dto: OAuthCallbackDto): Promise<UserResponseDto> {
+    const { sub: oauthId } = await this.callback(dto.url);
+    const duplicate = await this.userCore.getByOAuthId(oauthId);
+    if (duplicate && duplicate.id !== 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.');
+    }
+    return this.userCore.updateUser(user, user.id, { oauthId });
+  }
+
+  public async unlink(user: AuthUserDto): Promise<UserResponseDto> {
+    return this.userCore.updateUser(user, user.id, { oauthId: '' });
+  }
+
  public async getLogoutEndpoint(): Promise<string | null> {
    const config = await this.immichConfigService.getConfig();
    const { enabled } = config.oauth;
@@ -98,6 +109,14 @@ export class OAuthService {
    return (await this.getClient()).issuer.metadata.end_session_endpoint || null;
  }

+  private async callback(url: string): Promise<any> {
+    const redirectUri = url.split('?')[0];
+    const client = await this.getClient();
+    const params = client.callbackParams(url);
+    const tokens = await client.callback(redirectUri, params, { state: params.state });
+    return await client.userinfo<OAuthProfile>(tokens.access_token || '');
+  }
+
  private async getClient() {
    const config = await this.immichConfigService.getConfig();
    const { enabled, clientId, clientSecret, issuerUrl } = config.oauth;
--- a/server/apps/immich/src/api-v1/user/response-dto/user-response.dto.ts
+++ b/server/apps/immich/src/api-v1/user/response-dto/user-response.dto.ts
@@ -10,6 +10,7 @@ export class UserResponseDto {
  shouldChangePassword!: boolean;
  isAdmin!: boolean;
  deletedAt?: Date;
+  oauthId!: string;
 }

 export function mapUser(entity: UserEntity): UserResponseDto {
@@ -23,5 +24,6 @@ export function mapUser(entity: UserEntity): UserResponseDto {
    shouldChangePassword: entity.shouldChangePassword,
    isAdmin: entity.isAdmin,
    deletedAt: entity.deletedAt,
+    oauthId: entity.oauthId,
  };
 }
--- a/server/apps/immich/src/api-v1/user/user.core.ts
+++ b/server/apps/immich/src/api-v1/user/user.core.ts
@@ -25,27 +25,22 @@ export class UserCore {
    return hash(password, salt);
  }

-  async updateUser(authUser: AuthUserDto, userToUpdate: UserEntity, data: Partial<UserEntity>): Promise<UserEntity> {
-    if (!authUser.isAdmin && (authUser.id !== userToUpdate.id || userToUpdate.id != data.id)) {
+  async updateUser(authUser: AuthUserDto, id: string, dto: Partial<UserEntity>): Promise<UserEntity> {
+    if (!(authUser.isAdmin || authUser.id === id)) {
      throw new ForbiddenException('You are not allowed to update this user');
    }

-    // TODO: can this happen? If so we should implement a test case, otherwise remove it (also from DTO)
-    if (userToUpdate.isAdmin) {
-      const adminUser = await this.userRepository.getAdmin();
-      if (adminUser && adminUser.id !== userToUpdate.id) {
-        throw new BadRequestException('Admin user exists');
-      }
+    if (dto.isAdmin && authUser.isAdmin && authUser.id !== id) {
+      throw new BadRequestException('Admin user exists');
    }

    try {
-      const payload: Partial<UserEntity> = { ...data };
-      if (payload.password) {
+      if (dto.password) {
        const salt = await this.generateSalt();
-        payload.salt = salt;
-        payload.password = await this.hashPassword(payload.password, salt);
+        dto.salt = salt;
+        dto.password = await this.hashPassword(dto.password, salt);
      }
-      return this.userRepository.update(userToUpdate.id, payload);
+      return this.userRepository.update(id, dto);
    } catch (e) {
      Logger.error(e, 'Failed to update user info');
      throw new InternalServerErrorException('Failed to update user info');
--- a/server/apps/immich/src/api-v1/user/user.service.spec.ts
+++ b/server/apps/immich/src/api-v1/user/user.service.spec.ts
@@ -141,6 +141,25 @@ describe('UserService', () => {
  });

  describe('Create user', () => {
+    it('should let the admin update himself', async () => {
+      const dto = { id: adminUser.id, shouldChangePassword: true, isAdmin: true };
+
+      when(userRepositoryMock.get).calledWith(adminUser.id).mockResolvedValueOnce(null);
+      when(userRepositoryMock.update).calledWith(adminUser.id, dto).mockResolvedValueOnce(adminUser);
+
+      await sut.updateUser(adminUser, dto);
+
+      expect(userRepositoryMock.update).toHaveBeenCalledWith(adminUser.id, dto);
+    });
+
+    it('should not let the another user become an admin', async () => {
+      const dto = { id: immichUser.id, shouldChangePassword: true, isAdmin: true };
+
+      when(userRepositoryMock.get).calledWith(immichUser.id).mockResolvedValueOnce(immichUser);
+
+      await expect(sut.updateUser(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException);
+    });
+
    it('should not create a user if there is no local admin account', async () => {
      when(userRepositoryMock.getAdmin).calledWith().mockResolvedValueOnce(null);

--- a/server/apps/immich/src/api-v1/user/user.service.ts
+++ b/server/apps/immich/src/api-v1/user/user.service.ts
@@ -65,12 +65,12 @@ export class UserService {
    return mapUser(createdUser);
  }

-  async updateUser(authUser: AuthUserDto, updateUserDto: UpdateUserDto): Promise<UserResponseDto> {
-    const user = await this.userCore.get(updateUserDto.id);
+  async updateUser(authUser: AuthUserDto, dto: UpdateUserDto): Promise<UserResponseDto> {
+    const user = await this.userCore.get(dto.id);
    if (!user) {
      throw new NotFoundException('User not found');
    }
-    const updatedUser = await this.userCore.updateUser(authUser, user, updateUserDto);
+    const updatedUser = await this.userCore.updateUser(authUser, dto.id, dto);
    return mapUser(updatedUser);
  }

--- a/server/apps/immich/test/user.e2e-spec.ts
+++ b/server/apps/immich/test/user.e2e-spec.ts
@@ -107,6 +107,7 @@ describe('User', () => {
              shouldChangePassword: true,
              profileImagePath: '',
              deletedAt: null,
+              oauthId: '',
            },
            {
              email: userTwoEmail,
@@ -118,6 +119,7 @@ describe('User', () => {
              shouldChangePassword: true,
              profileImagePath: '',
              deletedAt: null,
+              oauthId: '',
            },
          ]),
        );
--- a/server/immich-openapi-specs.json
+++ b/server/immich-openapi-specs.json
@@ -1826,6 +1826,58 @@
        ]
      }
    },
+    "/oauth/link": {
+      "post": {
+        "operationId": "link",
+        "parameters": [],
+        "requestBody": {
+          "required": true,
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/OAuthCallbackDto"
+              }
+            }
+          }
+        },
+        "responses": {
+          "201": {
+            "description": "",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/UserResponseDto"
+                }
+              }
+            }
+          }
+        },
+        "tags": [
+          "OAuth"
+        ]
+      }
+    },
+    "/oauth/unlink": {
+      "post": {
+        "operationId": "unlink",
+        "parameters": [],
+        "responses": {
+          "201": {
+            "description": "",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/UserResponseDto"
+                }
+              }
+            }
+          }
+        },
+        "tags": [
+          "OAuth"
+        ]
+      }
+    },
    "/device-info": {
      "post": {
        "operationId": "createDeviceInfo",
@@ -2287,6 +2339,9 @@
          "deletedAt": {
            "format": "date-time",
            "type": "string"
+          },
+          "oauthId": {
+            "type": "string"
          }
        },
        "required": [
@@ -2297,7 +2352,8 @@
          "createdAt",
          "profileImagePath",
          "shouldChangePassword",
-          "isAdmin"
+          "isAdmin",
+          "oauthId"
        ]
      },
      "CreateUserDto": {
--- a/server/libs/database/src/entities/user.entity.ts
+++ b/server/libs/database/src/entities/user.entity.ts
@@ -24,7 +24,7 @@ export class UserEntity {
  @Column({ default: '', select: false })
  salt?: string;

-  @Column({ default: '', select: false })
+  @Column({ default: '' })
  oauthId!: string;

  @Column({ default: '' })