diff --git a/server/src/schema/tables/user.table.ts b/server/src/schema/tables/user.table.ts index 3a340d976b..0839924d2a 100644 --- a/server/src/schema/tables/user.table.ts +++ b/server/src/schema/tables/user.table.ts @@ -64,8 +64,9 @@ export class UserTable { @Column({ unique: true, nullable: true, default: null }) storageLabel!: string | null; + // TODO remove default, make nullable, and convert empty spaces to null @Column({ default: '' }) - name!: Generated; + name!: string; @Column({ type: 'bigint', nullable: true }) quotaSizeInBytes!: ColumnType | null; diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index f2cc3ada95..2c4b31c83a 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -8,6 +8,7 @@ import { AuthService } 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'; +import { OAuthProfileFactory } from 'test/factories/oauth-profile.factory'; import { SessionFactory } from 'test/factories/session.factory'; import { UserFactory } from 'test/factories/user.factory'; import { sharedLinkStub } from 'test/fixtures/shared-link.stub'; @@ -15,31 +16,7 @@ import { systemConfigStub } from 'test/fixtures/system-config.stub'; import { newUuid } from 'test/small.factory'; import { newTestService, ServiceMocks } from 'test/utils'; -const oauthResponse = ({ - id, - email, - name, - profileImagePath, -}: { - id: string; - email: string; - name: string; - profileImagePath?: string; -}) => ({ - accessToken: 'cmFuZG9tLWJ5dGVz', - userId: id, - userEmail: email, - name, - profileImagePath, - isAdmin: false, - isOnboarded: false, - shouldChangePassword: false, -}); - -// const token = Buffer.from('my-api-key', 'utf8').toString('base64'); - const email = 'test@immich.com'; -const sub = 'my-auth-user-sub'; const loginDetails = { isSecure: true, clientIp: '127.0.0.1', @@ -48,11 +25,9 @@ const loginDetails = { appVersion: null, }; -const fixtures = { - login: { - email, - password: 'password', - }, +const dto = { + email, + password: 'password', }; describe(AuthService.name, () => { @@ -63,7 +38,6 @@ describe(AuthService.name, () => { ({ sut, mocks } = newTestService(AuthService)); mocks.oauth.authorize.mockResolvedValue({ url: 'http://test', state: 'state', codeVerifier: 'codeVerifier' }); - mocks.oauth.getProfile.mockResolvedValue({ sub, email }); mocks.oauth.getLogoutEndpoint.mockResolvedValue('http://end-session-endpoint'); }); @@ -75,13 +49,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(fixtures.login, 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(fixtures.login, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.login(dto, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); @@ -89,7 +63,7 @@ describe(AuthService.name, () => { it('should check the user has a password', async () => { mocks.user.getByEmail.mockResolvedValue({} as UserAdmin); - await expect(sut.login(fixtures.login, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.login(dto, loginDetails)).rejects.toBeInstanceOf(UnauthorizedException); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); }); @@ -100,7 +74,7 @@ describe(AuthService.name, () => { mocks.user.getByEmail.mockResolvedValue(user); mocks.session.create.mockResolvedValue(session); - await expect(sut.login(fixtures.login, loginDetails)).resolves.toEqual({ + await expect(sut.login(dto, loginDetails)).resolves.toEqual({ accessToken: 'cmFuZG9tLWJ5dGVz', userId: user.id, userEmail: user.email, @@ -624,6 +598,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.getProfile.mockResolvedValue(OAuthProfileFactory.create()); await expect( sut.callback( @@ -638,31 +613,31 @@ describe(AuthService.name, () => { it('should link an existing user', async () => { const user = UserFactory.create(); + const profile = OAuthProfileFactory.create(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); + mocks.oauth.getProfile.mockResolvedValue(profile); mocks.user.getByEmail.mockResolvedValue(user); mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' }, + {}, + loginDetails, + ); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1); - expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: sub }); + expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub }); }); it('should not link to a user with a different oauth sub', async () => { - const user = UserFactory.create({ isAdmin: true, oauthId: 'existing-sub' }); + const user = UserFactory.create({ oauthId: 'existing-sub' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create()); mocks.user.getByEmail.mockResolvedValueOnce(user); - mocks.user.getAdmin.mockResolvedValue(user); - mocks.user.create.mockResolvedValue(user); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); await expect( sut.callback( @@ -677,35 +652,30 @@ describe(AuthService.name, () => { }); it('should allow auto registering by default', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create()); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' }, + {}, + loginDetails, + ); expect(mocks.user.getByEmail).toHaveBeenCalledTimes(2); // second call is for domain check before create expect(mocks.user.create).toHaveBeenCalledTimes(1); }); it('should throw an error if user should be auto registered but the email claim does not exist', async () => { - const user = UserFactory.create({ isAdmin: true }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); mocks.user.getByEmail.mockResolvedValue(void 0); - mocks.user.getAdmin.mockResolvedValue(user); - mocks.user.create.mockResolvedValue(user); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + mocks.user.create.mockResolvedValue(UserFactory.create()); mocks.session.create.mockResolvedValue(SessionFactory.create()); - mocks.oauth.getProfile.mockResolvedValue({ sub, email: undefined }); + mocks.oauth.getProfile.mockResolvedValue({ sub: 'sub' }); await expect( sut.callback( @@ -725,10 +695,9 @@ describe(AuthService.name, () => { 'app.immich:///oauth-callback?code=abc123', ]) { it(`should use the mobile redirect override for a url of ${url}`, async () => { - const user = UserFactory.create(); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride); - mocks.user.getByOAuthId.mockResolvedValue(user); + mocks.user.getByOAuthId.mockResolvedValue(UserFactory.create()); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create()); mocks.session.create.mockResolvedValue(SessionFactory.create()); await sut.callback({ url, state: 'xyz789', codeVerifier: 'foo' }, {}, loginDetails); @@ -743,135 +712,136 @@ describe(AuthService.name, () => { } it('should use the default quota', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); - mocks.user.create.mockResolvedValue(user); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create()); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 })); }); - it('should ignore an invalid storage quota', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_quota: 'abc' }); - mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + it('should infer name from given and family names', async () => { + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); + mocks.oauth.getProfile.mockResolvedValue( + OAuthProfileFactory.create({ name: undefined, given_name: 'Given', family_name: 'Family' }), + ); mocks.user.getByEmail.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + mocks.user.create.mockResolvedValue(UserFactory.create()); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ name: 'Given Family' })); + }); + + it('should fallback to email when no username is provided', async () => { + const profile = OAuthProfileFactory.create({ name: undefined, given_name: undefined, family_name: undefined }); + + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); + mocks.oauth.getProfile.mockResolvedValue(profile); + mocks.user.getByEmail.mockResolvedValue(void 0); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + mocks.user.create.mockResolvedValue(UserFactory.create()); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); + + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ name: profile.email })); + }); + + it('should ignore an invalid storage quota', async () => { + mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ immich_quota: 'abc' })); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); + mocks.user.getByEmail.mockResolvedValue(void 0); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); + mocks.session.create.mockResolvedValue(SessionFactory.create()); + + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 })); }); it('should ignore a negative quota', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_quota: -5 }); - mocks.user.getAdmin.mockResolvedValue(user); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ immich_quota: -5 })); + mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 })); }); it('should set quota for 0 quota', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_quota: 0 }); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ immich_quota: 0 })); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByEmail.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); - expect(mocks.user.create).toHaveBeenCalledWith({ - email: user.email, - isAdmin: false, - name: ' ', - oauthId: user.oauthId, - quotaSizeInBytes: 0, - storageLabel: null, - }); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 0 })); }); it('should use a valid storage quota', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_quota: 5 }); + mocks.oauth.getProfile.mockResolvedValue(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); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); - expect(mocks.user.create).toHaveBeenCalledWith({ - email: user.email, - isAdmin: false, - name: ' ', - oauthId: user.oauthId, - quotaSizeInBytes: 5_368_709_120, - storageLabel: null, - }); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 5_368_709_120 })); }); it('should sync the profile picture', async () => { const fileId = newUuid(); const user = UserFactory.create({ oauthId: 'oauth-id' }); - const pictureUrl = 'https://auth.immich.cloud/profiles/1.jpg'; + const profile = OAuthProfileFactory.create({ picture: 'https://auth.immich.cloud/profiles/1.jpg' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfile.mockResolvedValue({ - sub: user.oauthId, - email: user.email, - picture: pictureUrl, - }); + mocks.oauth.getProfile.mockResolvedValue(profile); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.crypto.randomUUID.mockReturnValue(fileId); mocks.oauth.getProfilePicture.mockResolvedValue({ @@ -881,131 +851,96 @@ describe(AuthService.name, () => { mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); expect(mocks.user.update).toHaveBeenCalledWith(user.id, { profileImagePath: expect.stringContaining(`/data/profile/${user.id}/${fileId}.jpg`), profileChangedAt: expect.any(Date), }); - expect(mocks.oauth.getProfilePicture).toHaveBeenCalledWith(pictureUrl); + expect(mocks.oauth.getProfilePicture).toHaveBeenCalledWith(profile.picture); }); it('should not sync the profile picture if the user already has one', async () => { const user = UserFactory.create({ oauthId: 'oauth-id', profileImagePath: 'not-empty' }); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled); - mocks.oauth.getProfile.mockResolvedValue({ - sub: user.oauthId, - email: user.email, - picture: 'https://auth.immich.cloud/profiles/1.jpg', - }); + mocks.oauth.getProfile.mockResolvedValue( + OAuthProfileFactory.create({ + sub: user.oauthId, + email: user.email, + picture: 'https://auth.immich.cloud/profiles/1.jpg', + }), + ); mocks.user.getByOAuthId.mockResolvedValue(user); mocks.user.update.mockResolvedValue(user); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); expect(mocks.user.update).not.toHaveBeenCalled(); expect(mocks.oauth.getProfilePicture).not.toHaveBeenCalled(); }); it('should only allow "admin" and "user" for the role claim', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_role: 'foo' }); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ immich_role: 'foo' })); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true })); mocks.user.getByOAuthId.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); - expect(mocks.user.create).toHaveBeenCalledWith({ - email: user.email, - name: ' ', - oauthId: user.oauthId, - quotaSizeInBytes: null, - storageLabel: null, - isAdmin: false, - }); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: false })); }); it('should create an admin user if the role claim is set to admin', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthWithAutoRegister); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, immich_role: 'admin' }); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ immich_role: 'admin' })); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); - expect(mocks.user.create).toHaveBeenCalledWith({ - email: user.email, - name: ' ', - oauthId: user.oauthId, - quotaSizeInBytes: null, - storageLabel: null, - isAdmin: true, - }); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: true })); }); it('should accept a custom role claim', async () => { - const user = UserFactory.create({ oauthId: 'oauth-id' }); - mocks.systemMetadata.get.mockResolvedValue({ - oauth: { ...systemConfigStub.oauthWithAutoRegister, roleClaim: 'my_role' }, + oauth: { ...systemConfigStub.oauthWithAutoRegister.oauth, roleClaim: 'my_role' }, }); - mocks.oauth.getProfile.mockResolvedValue({ sub: user.oauthId, email: user.email, my_role: 'admin' }); + mocks.oauth.getProfile.mockResolvedValue(OAuthProfileFactory.create({ my_role: 'admin' })); mocks.user.getByEmail.mockResolvedValue(void 0); mocks.user.getByOAuthId.mockResolvedValue(void 0); - mocks.user.create.mockResolvedValue(user); + mocks.user.create.mockResolvedValue(UserFactory.create({ oauthId: 'oauth-id' })); mocks.session.create.mockResolvedValue(SessionFactory.create()); - await expect( - sut.callback( - { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, - {}, - loginDetails, - ), - ).resolves.toEqual(oauthResponse(user)); + await sut.callback( + { url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, + {}, + loginDetails, + ); - expect(mocks.user.create).toHaveBeenCalledWith({ - email: user.email, - name: ' ', - oauthId: user.oauthId, - quotaSizeInBytes: null, - storageLabel: null, - isAdmin: true, - }); + expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ isAdmin: true })); }); }); @@ -1013,8 +948,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(); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); + mocks.oauth.getProfile.mockResolvedValue(profile); mocks.user.update.mockResolvedValue(user); await sut.link( @@ -1023,7 +960,7 @@ describe(AuthService.name, () => { {}, ); - expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: sub }); + expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: profile.sub }); }); it('should not link an already linked oauth.sub', async () => { @@ -1032,6 +969,7 @@ describe(AuthService.name, () => { const auth = { user: authUser, apiKey: authApiKey }; mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled); + mocks.oauth.getProfile.mockResolvedValue(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 b8e1b78107..5932855a21 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -261,6 +261,11 @@ export class AuthService extends BaseService { } async callback(dto: OAuthCallbackDto, headers: IncomingHttpHeaders, loginDetails: LoginDetails) { + const { oauth } = await this.getConfig({ withCache: false }); + if (!oauth.enabled) { + throw new BadRequestException('OAuth is not enabled'); + } + const expectedState = dto.state ?? this.getCookieOauthState(headers); if (!expectedState?.length) { throw new BadRequestException('OAuth state is missing'); @@ -271,7 +276,6 @@ export class AuthService extends BaseService { throw new BadRequestException('OAuth code verifier is missing'); } - const { oauth } = await this.getConfig({ withCache: false }); const url = this.resolveRedirectUri(oauth, dto.url); const profile = await this.oauthRepository.getProfile(oauth, url, expectedState, codeVerifier); const { autoRegister, defaultStorageQuota, storageLabelClaim, storageQuotaClaim, roleClaim } = oauth; @@ -298,7 +302,8 @@ export class AuthService extends BaseService { throw new BadRequestException(`User does not exist and auto registering is disabled.`); } - if (!profile.email) { + const email = profile.email; + if (!email) { throw new BadRequestException('OAuth profile does not have an email address'); } @@ -320,10 +325,13 @@ export class AuthService extends BaseService { isValid: (value: unknown) => isString(value) && ['admin', 'user'].includes(value), }); - const userName = profile.name ?? `${profile.given_name || ''} ${profile.family_name || ''}`; user = await this.createUser({ - name: userName, - email: profile.email, + name: + profile.name || + `${profile.given_name || ''} ${profile.family_name || ''}`.trim() || + profile.preferred_username || + email, + email, oauthId: profile.sub, quotaSizeInBytes: storageQuota === null ? null : storageQuota * HumanReadableSize.GiB, storageLabel: storageLabel || null, diff --git a/server/test/factories/oauth-profile.factory.ts b/server/test/factories/oauth-profile.factory.ts new file mode 100644 index 0000000000..31a8e4ea77 --- /dev/null +++ b/server/test/factories/oauth-profile.factory.ts @@ -0,0 +1,28 @@ +import { OAuthProfile } from 'src/repositories/oauth.repository'; +import { OAuthProfileLike } from 'test/factories/types'; +import { newUuid } from 'test/small.factory'; + +export class OAuthProfileFactory { + private constructor(private value: OAuthProfile) {} + + static create(dto: OAuthProfileLike = {}) { + return OAuthProfileFactory.from(dto).build(); + } + + static from(dto: OAuthProfileLike = {}) { + const sub = newUuid(); + return new OAuthProfileFactory({ + sub, + name: 'Name', + given_name: 'Given', + family_name: 'Family', + email: `oauth-${sub}@immich.cloud`, + email_verified: true, + ...dto, + }); + } + + build() { + return { ...this.value }; + } +} diff --git a/server/test/factories/types.ts b/server/test/factories/types.ts index e2d9e4e1c3..5c8c8ee2c2 100644 --- a/server/test/factories/types.ts +++ b/server/test/factories/types.ts @@ -1,4 +1,5 @@ import { Selectable } from 'kysely'; +import { OAuthProfile } from 'src/repositories/oauth.repository'; import { ActivityTable } from 'src/schema/tables/activity.table'; import { AlbumUserTable } from 'src/schema/tables/album-user.table'; import { AlbumTable } from 'src/schema/tables/album.table'; @@ -34,3 +35,4 @@ export type PartnerLike = Partial>; export type ActivityLike = Partial>; export type ApiKeyLike = Partial>; export type SessionLike = Partial>; +export type OAuthProfileLike = Partial; diff --git a/server/test/medium/specs/services/user.service.spec.ts b/server/test/medium/specs/services/user.service.spec.ts index 24a06404b1..2250034eea 100644 --- a/server/test/medium/specs/services/user.service.spec.ts +++ b/server/test/medium/specs/services/user.service.spec.ts @@ -47,15 +47,15 @@ describe(UserService.name, () => { const { sut, ctx } = setup(); ctx.getMock(EventRepository).emit.mockResolvedValue(); const user = mediumFactory.userInsert(); - await expect(sut.createUser({ email: user.email })).resolves.toMatchObject({ email: user.email }); - await expect(sut.createUser({ email: user.email })).rejects.toThrow('User exists'); + await expect(sut.createUser({ name: 'Test', email: user.email })).resolves.toMatchObject({ email: user.email }); + await expect(sut.createUser({ name: 'Test', email: user.email })).rejects.toThrow('User exists'); }); it('should not return password', async () => { const { sut, ctx } = setup(); ctx.getMock(EventRepository).emit.mockResolvedValue(); const dto = mediumFactory.userInsert({ password: 'password' }); - const user = await sut.createUser({ email: dto.email, password: 'password' }); + const user = await sut.createUser({ name: 'Test', email: dto.email, password: 'password' }); expect((user as any).password).toBeUndefined(); }); });