From 603b056512dbd15190d8decea49ff060683966b9 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Mon, 30 Oct 2023 11:48:38 -0400 Subject: [PATCH] refactor(server): auth delete device (#4720) * refactor(server): auth delete device * fix: person e2e --- server/src/domain/access/access.core.ts | 5 ++++ server/src/domain/auth/auth.service.spec.ts | 17 +++++++++---- server/src/domain/auth/auth.service.ts | 18 +++++++++----- .../domain/repositories/access.repository.ts | 4 ++++ .../repositories/user-token.repository.ts | 2 +- .../infra/repositories/access.repository.ts | 24 +++++++++++++++++-- .../repositories/user-token.repository.ts | 4 ++-- server/test/e2e/auth.e2e-spec.ts | 8 +++++++ server/test/e2e/person.e2e-spec.ts | 6 ++--- server/test/fixtures/uuid.stub.ts | 3 ++- .../repositories/access.repository.mock.ts | 5 ++++ 11 files changed, 76 insertions(+), 20 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index 7c5a664308..1ad9712509 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -21,6 +21,8 @@ export enum Permission { ALBUM_SHARE = 'album.share', ALBUM_DOWNLOAD = 'album.download', + AUTH_DEVICE_DELETE = 'authDevice.delete', + ARCHIVE_READ = 'archive.read', TIMELINE_READ = 'timeline.read', @@ -196,6 +198,9 @@ export class AccessCore { case Permission.ARCHIVE_READ: return authUser.id === id; + case Permission.AUTH_DEVICE_DELETE: + return this.repository.authDevice.hasOwnerAccess(authUser.id, id); + case Permission.TIMELINE_READ: return authUser.id === id || (await this.repository.timeline.hasPartnerAccess(authUser.id, id)); diff --git a/server/src/domain/auth/auth.service.spec.ts b/server/src/domain/auth/auth.service.spec.ts index 6ff7d77037..2ef598b059 100644 --- a/server/src/domain/auth/auth.service.spec.ts +++ b/server/src/domain/auth/auth.service.spec.ts @@ -1,9 +1,11 @@ import { UserEntity } from '@app/infra/entities'; import { BadRequestException, UnauthorizedException } from '@nestjs/common'; import { + IAccessRepositoryMock, authStub, keyStub, loginResponseStub, + newAccessRepositoryMock, newCryptoRepositoryMock, newKeyRepositoryMock, newLibraryRepositoryMock, @@ -52,6 +54,7 @@ const fixtures = { describe('AuthService', () => { let sut: AuthService; + let accessMock: jest.Mocked; let cryptoMock: jest.Mocked; let userMock: jest.Mocked; let libraryMock: jest.Mocked; @@ -84,6 +87,7 @@ describe('AuthService', () => { }), } as any); + accessMock = newAccessRepositoryMock(); cryptoMock = newCryptoRepositoryMock(); userMock = newUserRepositoryMock(); libraryMock = newLibraryRepositoryMock(); @@ -92,7 +96,7 @@ describe('AuthService', () => { shareMock = newSharedLinkRepositoryMock(); keyMock = newKeyRepositoryMock(); - sut = new AuthService(cryptoMock, configMock, userMock, userTokenMock, libraryMock, shareMock, keyMock); + sut = new AuthService(accessMock, cryptoMock, configMock, libraryMock, userMock, userTokenMock, shareMock, keyMock); }); it('should be defined', () => { @@ -218,7 +222,7 @@ describe('AuthService', () => { redirectUri: '/auth/login?autoLaunch=0', }); - expect(userTokenMock.delete).toHaveBeenCalledWith('123', 'token123'); + expect(userTokenMock.delete).toHaveBeenCalledWith('token123'); }); it('should return the default redirect if auth type is OAUTH but oauth is not enabled', async () => { @@ -384,16 +388,19 @@ describe('AuthService', () => { await sut.logoutDevices(authStub.user1); expect(userTokenMock.getAll).toHaveBeenCalledWith(authStub.user1.id); - expect(userTokenMock.delete).toHaveBeenCalledWith(authStub.user1.id, 'not_active'); - expect(userTokenMock.delete).not.toHaveBeenCalledWith(authStub.user1.id, 'token-id'); + expect(userTokenMock.delete).toHaveBeenCalledWith('not_active'); + expect(userTokenMock.delete).not.toHaveBeenCalledWith('token-id'); }); }); describe('logoutDevice', () => { it('should logout the device', async () => { + accessMock.authDevice.hasOwnerAccess.mockResolvedValue(true); + await sut.logoutDevice(authStub.user1, 'token-1'); - expect(userTokenMock.delete).toHaveBeenCalledWith(authStub.user1.id, 'token-1'); + expect(accessMock.authDevice.hasOwnerAccess).toHaveBeenCalledWith(authStub.user1.id, 'token-1'); + expect(userTokenMock.delete).toHaveBeenCalledWith('token-1'); }); }); diff --git a/server/src/domain/auth/auth.service.ts b/server/src/domain/auth/auth.service.ts index 891a093490..13c69d6b7b 100644 --- a/server/src/domain/auth/auth.service.ts +++ b/server/src/domain/auth/auth.service.ts @@ -11,7 +11,9 @@ import cookieParser from 'cookie'; import { IncomingHttpHeaders } from 'http'; import { DateTime } from 'luxon'; import { ClientMetadata, Issuer, UserinfoResponse, custom, generators } from 'openid-client'; +import { AccessCore, Permission } from '../access'; import { + IAccessRepository, ICryptoRepository, IKeyRepository, ILibraryRepository, @@ -61,19 +63,22 @@ interface OAuthProfile extends UserinfoResponse { @Injectable() export class AuthService { - private userCore: UserCore; + private access: AccessCore; private configCore: SystemConfigCore; private logger = new Logger(AuthService.name); + private userCore: UserCore; constructor( + @Inject(IAccessRepository) accessRepository: IAccessRepository, @Inject(ICryptoRepository) private cryptoRepository: ICryptoRepository, @Inject(ISystemConfigRepository) configRepository: ISystemConfigRepository, + @Inject(ILibraryRepository) libraryRepository: ILibraryRepository, @Inject(IUserRepository) userRepository: IUserRepository, @Inject(IUserTokenRepository) private userTokenRepository: IUserTokenRepository, - @Inject(ILibraryRepository) libraryRepository: ILibraryRepository, @Inject(ISharedLinkRepository) private sharedLinkRepository: ISharedLinkRepository, @Inject(IKeyRepository) private keyRepository: IKeyRepository, ) { + this.access = AccessCore.create(accessRepository); this.configCore = SystemConfigCore.create(configRepository); this.userCore = UserCore.create(cryptoRepository, libraryRepository, userRepository); @@ -104,7 +109,7 @@ export class AuthService { async logout(authUser: AuthUserDto, authType: AuthType): Promise { if (authUser.accessTokenId) { - await this.userTokenRepository.delete(authUser.id, authUser.accessTokenId); + await this.userTokenRepository.delete(authUser.accessTokenId); } return { @@ -175,8 +180,9 @@ export class AuthService { return userTokens.map((userToken) => mapUserToken(userToken, authUser.accessTokenId)); } - async logoutDevice(authUser: AuthUserDto, deviceId: string): Promise { - await this.userTokenRepository.delete(authUser.id, deviceId); + async logoutDevice(authUser: AuthUserDto, id: string): Promise { + await this.access.requirePermission(authUser, Permission.AUTH_DEVICE_DELETE, id); + await this.userTokenRepository.delete(id); } async logoutDevices(authUser: AuthUserDto): Promise { @@ -185,7 +191,7 @@ export class AuthService { if (device.id === authUser.accessTokenId) { continue; } - await this.userTokenRepository.delete(authUser.id, device.id); + await this.userTokenRepository.delete(device.id); } } diff --git a/server/src/domain/repositories/access.repository.ts b/server/src/domain/repositories/access.repository.ts index 02cf94502d..7584762fc6 100644 --- a/server/src/domain/repositories/access.repository.ts +++ b/server/src/domain/repositories/access.repository.ts @@ -8,6 +8,10 @@ export interface IAccessRepository { hasSharedLinkAccess(sharedLinkId: string, assetId: string): Promise; }; + authDevice: { + hasOwnerAccess(userId: string, deviceId: string): Promise; + }; + album: { hasOwnerAccess(userId: string, albumId: string): Promise; hasSharedAlbumAccess(userId: string, albumId: string): Promise; diff --git a/server/src/domain/repositories/user-token.repository.ts b/server/src/domain/repositories/user-token.repository.ts index 51234eddcf..713b3f1ef8 100644 --- a/server/src/domain/repositories/user-token.repository.ts +++ b/server/src/domain/repositories/user-token.repository.ts @@ -5,7 +5,7 @@ export const IUserTokenRepository = 'IUserTokenRepository'; export interface IUserTokenRepository { create(dto: Partial): Promise; save(dto: Partial): Promise; - delete(userId: string, id: string): Promise; + delete(id: string): Promise; getByToken(token: string): Promise; getAll(userId: string): Promise; } diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index 24b8e8ef40..50085c4aad 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -1,16 +1,25 @@ import { IAccessRepository } from '@app/domain'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { AlbumEntity, AssetEntity, LibraryEntity, PartnerEntity, PersonEntity, SharedLinkEntity } from '../entities'; +import { + AlbumEntity, + AssetEntity, + LibraryEntity, + PartnerEntity, + PersonEntity, + SharedLinkEntity, + UserTokenEntity, +} from '../entities'; export class AccessRepository implements IAccessRepository { constructor( @InjectRepository(AssetEntity) private assetRepository: Repository, @InjectRepository(AlbumEntity) private albumRepository: Repository, + @InjectRepository(LibraryEntity) private libraryRepository: Repository, @InjectRepository(PartnerEntity) private partnerRepository: Repository, @InjectRepository(PersonEntity) private personRepository: Repository, @InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository, - @InjectRepository(LibraryEntity) private libraryRepository: Repository, + @InjectRepository(UserTokenEntity) private tokenRepository: Repository, ) {} library = { @@ -148,6 +157,17 @@ export class AccessRepository implements IAccessRepository { }, }; + authDevice = { + hasOwnerAccess: (userId: string, deviceId: string): Promise => { + return this.tokenRepository.exist({ + where: { + userId, + id: deviceId, + }, + }); + }, + }; + album = { hasOwnerAccess: (userId: string, albumId: string): Promise => { return this.albumRepository.exist({ diff --git a/server/src/infra/repositories/user-token.repository.ts b/server/src/infra/repositories/user-token.repository.ts index c67e659ac1..cb83134a3a 100644 --- a/server/src/infra/repositories/user-token.repository.ts +++ b/server/src/infra/repositories/user-token.repository.ts @@ -35,7 +35,7 @@ export class UserTokenRepository implements IUserTokenRepository { return this.repository.save(userToken); } - async delete(userId: string, id: string): Promise { - await this.repository.delete({ userId, id }); + async delete(id: string): Promise { + await this.repository.delete({ id }); } } diff --git a/server/test/e2e/auth.e2e-spec.ts b/server/test/e2e/auth.e2e-spec.ts index a42e1e1618..985645129f 100644 --- a/server/test/e2e/auth.e2e-spec.ts +++ b/server/test/e2e/auth.e2e-spec.ts @@ -189,6 +189,14 @@ describe(`${AuthController.name} (e2e)`, () => { expect(body).toEqual(errorStub.unauthorized); }); + it('should throw an error for a non-existent device id', async () => { + const { status, body } = await request(server) + .delete(`/auth/devices/${uuidStub.notFound}`) + .set('Authorization', `Bearer ${accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorStub.badRequest('Not found or no authDevice.delete access')); + }); + it('should logout a device', async () => { const [device] = await api.authApi.getAuthDevices(server, accessToken); const { status } = await request(server) diff --git a/server/test/e2e/person.e2e-spec.ts b/server/test/e2e/person.e2e-spec.ts index bb0af4c96c..298e4418ee 100644 --- a/server/test/e2e/person.e2e-spec.ts +++ b/server/test/e2e/person.e2e-spec.ts @@ -139,10 +139,10 @@ describe(`${PersonController.name}`, () => { it('should not accept invalid birth dates', async () => { for (const { birthDate, response } of [ - { birthDate: false, response: ['id must be a UUID'] }, + { birthDate: false, response: 'Not found or no person.write access' }, { birthDate: 'false', response: ['birthDate must be a Date instance'] }, - { birthDate: '123567', response: ['id must be a UUID'] }, - { birthDate: 123456, response: ['id must be a UUID'] }, + { birthDate: '123567', response: 'Not found or no person.write access' }, + { birthDate: 123567, response: 'Not found or no person.write access' }, ]) { const { status, body } = await request(server) .put(`/person/${uuidStub.notFound}`) diff --git a/server/test/fixtures/uuid.stub.ts b/server/test/fixtures/uuid.stub.ts index bce525444b..b10ef80da7 100644 --- a/server/test/fixtures/uuid.stub.ts +++ b/server/test/fixtures/uuid.stub.ts @@ -1,4 +1,5 @@ export const uuidStub = { invalid: 'invalid-uuid', - notFound: '00000000-0000-0000-0000-000000000000', + // valid uuid v4 + notFound: '00000000-0000-4000-a000-000000000000', }; diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index 416d5043a4..9fbf7922d2 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -3,6 +3,7 @@ import { AccessCore, IAccessRepository } from '@app/domain'; export interface IAccessRepositoryMock { asset: jest.Mocked; album: jest.Mocked; + authDevice: jest.Mocked; library: jest.Mocked; timeline: jest.Mocked; person: jest.Mocked; @@ -27,6 +28,10 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock => hasSharedLinkAccess: jest.fn(), }, + authDevice: { + hasOwnerAccess: jest.fn(), + }, + library: { hasOwnerAccess: jest.fn(), hasPartnerAccess: jest.fn(),