From 414893a6872dfae95d9be60541b4c362e93e3275 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Sat, 28 Jan 2023 00:12:11 -0500 Subject: [PATCH] fix(server): auth strategies (#1459) * fix(server): auth strategies * chore: tests --- .../immich-auth/strategies/api-key.strategy.ts | 2 +- .../strategies/public-share.strategy.ts | 2 +- .../strategies/user-auth.strategy.ts | 18 ++++++------------ .../domain/src/api-key/api-key.service.spec.ts | 4 ++-- .../libs/domain/src/api-key/api-key.service.ts | 6 +++--- .../libs/domain/src/auth/auth.service.spec.ts | 2 +- server/libs/domain/src/auth/auth.service.ts | 6 +++--- .../domain/src/share/share.service.spec.ts | 8 ++++---- server/libs/domain/src/share/share.service.ts | 13 +++---------- 9 files changed, 24 insertions(+), 37 deletions(-) diff --git a/server/apps/immich/src/modules/immich-auth/strategies/api-key.strategy.ts b/server/apps/immich/src/modules/immich-auth/strategies/api-key.strategy.ts index ad4d15901f..a6e306cd73 100644 --- a/server/apps/immich/src/modules/immich-auth/strategies/api-key.strategy.ts +++ b/server/apps/immich/src/modules/immich-auth/strategies/api-key.strategy.ts @@ -15,7 +15,7 @@ export class APIKeyStrategy extends PassportStrategy(Strategy, API_KEY_STRATEGY) super(options); } - validate(token: string): Promise { + validate(token: string): Promise { return this.apiKeyService.validate(token); } } diff --git a/server/apps/immich/src/modules/immich-auth/strategies/public-share.strategy.ts b/server/apps/immich/src/modules/immich-auth/strategies/public-share.strategy.ts index ff9ec02901..e42575cda5 100644 --- a/server/apps/immich/src/modules/immich-auth/strategies/public-share.strategy.ts +++ b/server/apps/immich/src/modules/immich-auth/strategies/public-share.strategy.ts @@ -16,7 +16,7 @@ export class PublicShareStrategy extends PassportStrategy(Strategy, PUBLIC_SHARE super(options); } - async validate(key: string): Promise { + validate(key: string): Promise { return this.shareService.validate(key); } } diff --git a/server/apps/immich/src/modules/immich-auth/strategies/user-auth.strategy.ts b/server/apps/immich/src/modules/immich-auth/strategies/user-auth.strategy.ts index 3ce1d9c670..717482e3dd 100644 --- a/server/apps/immich/src/modules/immich-auth/strategies/user-auth.strategy.ts +++ b/server/apps/immich/src/modules/immich-auth/strategies/user-auth.strategy.ts @@ -1,24 +1,18 @@ -import { Injectable, UnauthorizedException } from '@nestjs/common'; +import { AuthService, AuthUserDto } from '@app/domain'; +import { Injectable } from '@nestjs/common'; import { PassportStrategy } from '@nestjs/passport'; -import { AuthService, AuthUserDto, UserService } from '@app/domain'; -import { Strategy } from 'passport-custom'; import { Request } from 'express'; +import { Strategy } from 'passport-custom'; export const AUTH_COOKIE_STRATEGY = 'auth-cookie'; @Injectable() export class UserAuthStrategy extends PassportStrategy(Strategy, AUTH_COOKIE_STRATEGY) { - constructor(private userService: UserService, private authService: AuthService) { + constructor(private authService: AuthService) { super(); } - async validate(request: Request): Promise { - const authUser = await this.authService.validate(request.headers); - - if (!authUser) { - throw new UnauthorizedException('Incorrect token provided'); - } - - return authUser; + validate(request: Request): Promise { + return this.authService.validate(request.headers); } } diff --git a/server/libs/domain/src/api-key/api-key.service.spec.ts b/server/libs/domain/src/api-key/api-key.service.spec.ts index 4761734c3a..2788ba7098 100644 --- a/server/libs/domain/src/api-key/api-key.service.spec.ts +++ b/server/libs/domain/src/api-key/api-key.service.spec.ts @@ -1,5 +1,5 @@ import { APIKeyEntity } from '@app/infra/db/entities'; -import { BadRequestException, UnauthorizedException } from '@nestjs/common'; +import { BadRequestException } from '@nestjs/common'; import { authStub, userEntityStub, newCryptoRepositoryMock, newKeyRepositoryMock } from '../../test'; import { ICryptoRepository } from '../auth'; import { IKeyRepository } from './api-key.repository'; @@ -124,7 +124,7 @@ describe(APIKeyService.name, () => { it('should throw an error for an invalid id', async () => { keyMock.getKey.mockResolvedValue(null); - await expect(sut.validate(token)).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.validate(token)).resolves.toBeNull(); expect(keyMock.getKey).toHaveBeenCalledWith('bXktYXBpLWtleQ== (hashed)'); }); diff --git a/server/libs/domain/src/api-key/api-key.service.ts b/server/libs/domain/src/api-key/api-key.service.ts index c5bf096933..218f57559d 100644 --- a/server/libs/domain/src/api-key/api-key.service.ts +++ b/server/libs/domain/src/api-key/api-key.service.ts @@ -1,4 +1,4 @@ -import { BadRequestException, Inject, Injectable, UnauthorizedException } from '@nestjs/common'; +import { BadRequestException, Inject, Injectable } from '@nestjs/common'; import { AuthUserDto, ICryptoRepository } from '../auth'; import { IKeyRepository } from './api-key.repository'; import { APIKeyCreateDto } from './dto/api-key-create.dto'; @@ -56,7 +56,7 @@ export class APIKeyService { return keys.map(mapKey); } - async validate(token: string): Promise { + async validate(token: string): Promise { const hashedToken = this.crypto.hashSha256(token); const keyEntity = await this.repository.getKey(hashedToken); if (keyEntity?.user) { @@ -71,6 +71,6 @@ export class APIKeyService { }; } - throw new UnauthorizedException('Invalid API Key'); + return null; } } diff --git a/server/libs/domain/src/auth/auth.service.spec.ts b/server/libs/domain/src/auth/auth.service.spec.ts index 486d26d82a..6cb59a7f24 100644 --- a/server/libs/domain/src/auth/auth.service.spec.ts +++ b/server/libs/domain/src/auth/auth.service.spec.ts @@ -229,7 +229,7 @@ describe('AuthService', () => { describe('validate - api request', () => { it('should throw if no user is found', async () => { userMock.get.mockResolvedValue(null); - await expect(sut.validate({ email: 'a', userId: 'test' })).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.validate({ email: 'a', userId: 'test' })).resolves.toBeNull(); }); it('should return an auth dto', async () => { diff --git a/server/libs/domain/src/auth/auth.service.ts b/server/libs/domain/src/auth/auth.service.ts index 6872cbcecd..7b15522dc6 100644 --- a/server/libs/domain/src/auth/auth.service.ts +++ b/server/libs/domain/src/auth/auth.service.ts @@ -115,10 +115,10 @@ export class AuthService { } } - public async validate(headers: IncomingHttpHeaders): Promise { + public async validate(headers: IncomingHttpHeaders): Promise { const tokenValue = this.extractTokenFromHeader(headers); if (!tokenValue) { - throw new UnauthorizedException('No access token provided in request'); + return null; } const hashedToken = this.cryptoRepository.hashSha256(tokenValue); @@ -133,7 +133,7 @@ export class AuthService { }; } - throw new UnauthorizedException('Invalid access token provided'); + return null; } extractTokenFromHeader(headers: IncomingHttpHeaders) { diff --git a/server/libs/domain/src/share/share.service.spec.ts b/server/libs/domain/src/share/share.service.spec.ts index c8dd994fc3..bba5b6fe6b 100644 --- a/server/libs/domain/src/share/share.service.spec.ts +++ b/server/libs/domain/src/share/share.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException, ForbiddenException, UnauthorizedException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { authStub, userEntityStub, @@ -34,18 +34,18 @@ describe(ShareService.name, () => { describe('validate', () => { it('should not accept a non-existant key', async () => { shareMock.getByKey.mockResolvedValue(null); - await expect(sut.validate('key')).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.validate('key')).resolves.toBeNull(); }); it('should not accept an expired key', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.expired); - await expect(sut.validate('key')).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.validate('key')).resolves.toBeNull(); }); it('should not accept a key without a user', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.expired); userMock.get.mockResolvedValue(null); - await expect(sut.validate('key')).rejects.toBeInstanceOf(UnauthorizedException); + await expect(sut.validate('key')).resolves.toBeNull(); }); it('should accept a valid key', async () => { diff --git a/server/libs/domain/src/share/share.service.ts b/server/libs/domain/src/share/share.service.ts index e175b6e943..6b1f3c5ed3 100644 --- a/server/libs/domain/src/share/share.service.ts +++ b/server/libs/domain/src/share/share.service.ts @@ -1,11 +1,4 @@ -import { - BadRequestException, - ForbiddenException, - Inject, - Injectable, - Logger, - UnauthorizedException, -} from '@nestjs/common'; +import { BadRequestException, ForbiddenException, Inject, Injectable, Logger } from '@nestjs/common'; import { AuthUserDto, ICryptoRepository } from '../auth'; import { IUserRepository, UserCore } from '../user'; import { EditSharedLinkDto } from './dto'; @@ -28,7 +21,7 @@ export class ShareService { this.userCore = new UserCore(userRepository, cryptoRepository); } - async validate(key: string): Promise { + async validate(key: string): Promise { const link = await this.shareCore.getByKey(key); if (link) { if (!link.expiresAt || new Date(link.expiresAt) > new Date()) { @@ -47,7 +40,7 @@ export class ShareService { } } } - throw new UnauthorizedException(); + return null; } async getAll(authUser: AuthUserDto): Promise {