From 91b835cfeb177e9cbfc22bc096b9fa285430c49b Mon Sep 17 00:00:00 2001
From: Jason Rasmussen <jrasm91@gmail.com>
Date: Tue, 21 May 2024 09:07:34 -0400
Subject: [PATCH] fix: auth sub override (#9635)

---
 server/src/services/auth.service.spec.ts   | 32 ++++++++++++++++------
 server/src/services/auth.service.ts        |  2 +-
 server/test/fixtures/system-config.stub.ts | 14 ++++++++--
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts
index 11339315db..b93599b38a 100644
--- a/server/src/services/auth.service.spec.ts
+++ b/server/src/services/auth.service.spec.ts
@@ -377,7 +377,7 @@ describe('AuthService', () => {
     });
 
     it('should not allow auto registering', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.noAutoRegister);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthEnabled);
       userMock.getByEmail.mockResolvedValue(null);
       await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).rejects.toBeInstanceOf(
         BadRequestException,
@@ -386,7 +386,7 @@ describe('AuthService', () => {
     });
 
     it('should link an existing user', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.noAutoRegister);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthEnabled);
       userMock.getByEmail.mockResolvedValue(userStub.user1);
       userMock.update.mockResolvedValue(userStub.user1);
       sessionMock.create.mockResolvedValue(sessionStub.valid);
@@ -399,6 +399,20 @@ describe('AuthService', () => {
       expect(userMock.update).toHaveBeenCalledWith(userStub.user1.id, { oauthId: sub });
     });
 
+    it('should not link to a user with a different oauth sub', async () => {
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister);
+      userMock.getByEmail.mockResolvedValueOnce({ ...userStub.user1, oauthId: 'existing-sub' });
+      userMock.getAdmin.mockResolvedValue(userStub.user1);
+      userMock.create.mockResolvedValue(userStub.user1);
+
+      await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
+        loginResponseStub.user1oauth,
+      );
+
+      expect(userMock.update).not.toHaveBeenCalled();
+      expect(userMock.create).toHaveBeenCalled();
+    });
+
     it('should allow auto registering by default', async () => {
       systemMock.get.mockResolvedValue(systemConfigStub.enabled);
       userMock.getByEmail.mockResolvedValue(null);
@@ -415,7 +429,7 @@ describe('AuthService', () => {
     });
 
     it('should use the mobile redirect override', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.override);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride);
       userMock.getByOAuthId.mockResolvedValue(userStub.user1);
       sessionMock.create.mockResolvedValue(sessionStub.valid);
 
@@ -425,7 +439,7 @@ describe('AuthService', () => {
     });
 
     it('should use the mobile redirect override for ios urls with multiple slashes', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.override);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride);
       userMock.getByOAuthId.mockResolvedValue(userStub.user1);
       sessionMock.create.mockResolvedValue(sessionStub.valid);
 
@@ -435,7 +449,7 @@ describe('AuthService', () => {
     });
 
     it('should use the default quota', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.withDefaultStorageQuota);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
       userMock.getByEmail.mockResolvedValue(null);
       userMock.getAdmin.mockResolvedValue(userStub.user1);
       userMock.create.mockResolvedValue(userStub.user1);
@@ -448,7 +462,7 @@ describe('AuthService', () => {
     });
 
     it('should ignore an invalid storage quota', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.withDefaultStorageQuota);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
       userMock.getByEmail.mockResolvedValue(null);
       userMock.getAdmin.mockResolvedValue(userStub.user1);
       userMock.create.mockResolvedValue(userStub.user1);
@@ -462,7 +476,7 @@ describe('AuthService', () => {
     });
 
     it('should ignore a negative quota', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.withDefaultStorageQuota);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
       userMock.getByEmail.mockResolvedValue(null);
       userMock.getAdmin.mockResolvedValue(userStub.user1);
       userMock.create.mockResolvedValue(userStub.user1);
@@ -476,7 +490,7 @@ describe('AuthService', () => {
     });
 
     it('should not set quota for 0 quota', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.withDefaultStorageQuota);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
       userMock.getByEmail.mockResolvedValue(null);
       userMock.getAdmin.mockResolvedValue(userStub.user1);
       userMock.create.mockResolvedValue(userStub.user1);
@@ -496,7 +510,7 @@ describe('AuthService', () => {
     });
 
     it('should use a valid storage quota', async () => {
-      systemMock.get.mockResolvedValue(systemConfigStub.withDefaultStorageQuota);
+      systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
       userMock.getByEmail.mockResolvedValue(null);
       userMock.getAdmin.mockResolvedValue(userStub.user1);
       userMock.create.mockResolvedValue(userStub.user1);
diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts
index 4c0efc4e6b..115a630b78 100644
--- a/server/src/services/auth.service.ts
+++ b/server/src/services/auth.service.ts
@@ -201,7 +201,7 @@ export class AuthService {
     // link existing user
     if (!user) {
       const emailUser = await this.userRepository.getByEmail(profile.email);
-      if (emailUser) {
+      if (emailUser && !emailUser.oauthId) {
         user = await this.userRepository.update(emailUser.id, { oauthId: profile.sub });
       }
     }
diff --git a/server/test/fixtures/system-config.stub.ts b/server/test/fixtures/system-config.stub.ts
index c01fd212ec..be21fc4060 100644
--- a/server/test/fixtures/system-config.stub.ts
+++ b/server/test/fixtures/system-config.stub.ts
@@ -15,7 +15,7 @@ export const systemConfigStub = {
       enabled: false,
     },
   },
-  noAutoRegister: {
+  oauthEnabled: {
     oauth: {
       enabled: true,
       autoRegister: false,
@@ -23,7 +23,15 @@ export const systemConfigStub = {
       buttonText: 'OAuth',
     },
   },
-  override: {
+  oauthWithAutoRegister: {
+    oauth: {
+      enabled: true,
+      autoRegister: true,
+      autoLaunch: false,
+      buttonText: 'OAuth',
+    },
+  },
+  oauthWithMobileOverride: {
     oauth: {
       enabled: true,
       autoRegister: true,
@@ -32,7 +40,7 @@ export const systemConfigStub = {
       buttonText: 'OAuth',
     },
   },
-  withDefaultStorageQuota: {
+  oauthWithStorageQuota: {
     oauth: {
       enabled: true,
       autoRegister: true,