From a4506758aa6bfa1e9f80e500e4a77516584e45ae Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Thu, 15 Aug 2024 09:14:23 -0400 Subject: [PATCH] refactor: auth service (#11811) --- e2e/src/api/specs/api-key.e2e-spec.ts | 206 +++++++++++++++++++ open-api/immich-openapi-specs.json | 2 +- server/src/controllers/api-key.controller.ts | 3 +- server/src/dtos/auth.dto.ts | 2 + server/src/middleware/auth.guard.ts | 18 +- server/src/repositories/event.repository.ts | 6 +- server/src/services/auth.service.spec.ts | 124 ++++++++--- server/src/services/auth.service.ts | 37 +++- 8 files changed, 352 insertions(+), 46 deletions(-) create mode 100644 e2e/src/api/specs/api-key.e2e-spec.ts diff --git a/e2e/src/api/specs/api-key.e2e-spec.ts b/e2e/src/api/specs/api-key.e2e-spec.ts new file mode 100644 index 0000000000..32d18f612d --- /dev/null +++ b/e2e/src/api/specs/api-key.e2e-spec.ts @@ -0,0 +1,206 @@ +import { LoginResponseDto, createApiKey } from '@immich/sdk'; +import { createUserDto, uuidDto } from 'src/fixtures'; +import { errorDto } from 'src/responses'; +import { app, asBearerAuth, utils } from 'src/utils'; +import request from 'supertest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; + +const create = (accessToken: string) => + createApiKey({ apiKeyCreateDto: { name: 'api key' } }, { headers: asBearerAuth(accessToken) }); + +describe('/api-keys', () => { + let admin: LoginResponseDto; + let user: LoginResponseDto; + + beforeAll(async () => { + await utils.resetDatabase(); + + admin = await utils.adminSetup(); + user = await utils.userSetup(admin.accessToken, createUserDto.user1); + }); + + beforeEach(async () => { + await utils.resetDatabase(['api_keys']); + }); + + describe('POST /api-keys', () => { + it('should require authentication', async () => { + const { status, body } = await request(app).post('/api-keys').send({ name: 'API Key' }); + expect(status).toBe(401); + expect(body).toEqual(errorDto.unauthorized); + }); + + it('should create an api key', async () => { + const { status, body } = await request(app) + .post('/api-keys') + .send({ name: 'API Key' }) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(body).toEqual({ + apiKey: { + id: expect.any(String), + name: 'API Key', + createdAt: expect.any(String), + updatedAt: expect.any(String), + }, + secret: expect.any(String), + }); + expect(status).toEqual(201); + }); + }); + + describe('GET /api-keys', () => { + it('should require authentication', async () => { + const { status, body } = await request(app).get('/api-keys'); + expect(status).toBe(401); + expect(body).toEqual(errorDto.unauthorized); + }); + + it('should start off empty', async () => { + const { status, body } = await request(app).get('/api-keys').set('Authorization', `Bearer ${admin.accessToken}`); + expect(body).toEqual([]); + expect(status).toEqual(200); + }); + + it('should return a list of api keys', async () => { + const [{ apiKey: apiKey1 }, { apiKey: apiKey2 }, { apiKey: apiKey3 }] = await Promise.all([ + create(admin.accessToken), + create(admin.accessToken), + create(admin.accessToken), + ]); + const { status, body } = await request(app).get('/api-keys').set('Authorization', `Bearer ${admin.accessToken}`); + expect(body).toHaveLength(3); + expect(body).toEqual(expect.arrayContaining([apiKey1, apiKey2, apiKey3])); + expect(status).toEqual(200); + }); + }); + + describe('GET /api-keys/:id', () => { + it('should require authentication', async () => { + const { status, body } = await request(app).get(`/api-keys/${uuidDto.notFound}`); + expect(status).toBe(401); + expect(body).toEqual(errorDto.unauthorized); + }); + + it('should require authorization', async () => { + const { apiKey } = await create(user.accessToken); + const { status, body } = await request(app) + .get(`/api-keys/${apiKey.id}`) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest('API Key not found')); + }); + + it('should require a valid uuid', async () => { + const { status, body } = await request(app) + .get(`/api-keys/${uuidDto.invalid}`) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(['id must be a UUID'])); + }); + + it('should get api key details', async () => { + const { apiKey } = await create(user.accessToken); + const { status, body } = await request(app) + .get(`/api-keys/${apiKey.id}`) + .set('Authorization', `Bearer ${user.accessToken}`); + expect(status).toBe(200); + expect(body).toEqual({ + id: expect.any(String), + name: 'api key', + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + }); + }); + + describe('PUT /api-keys/:id', () => { + it('should require authentication', async () => { + const { status, body } = await request(app).put(`/api-keys/${uuidDto.notFound}`).send({ name: 'new name' }); + expect(status).toBe(401); + expect(body).toEqual(errorDto.unauthorized); + }); + + it('should require authorization', async () => { + const { apiKey } = await create(user.accessToken); + const { status, body } = await request(app) + .put(`/api-keys/${apiKey.id}`) + .send({ name: 'new name' }) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest('API Key not found')); + }); + + it('should require a valid uuid', async () => { + const { status, body } = await request(app) + .put(`/api-keys/${uuidDto.invalid}`) + .send({ name: 'new name' }) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(['id must be a UUID'])); + }); + + it('should update api key details', async () => { + const { apiKey } = await create(user.accessToken); + const { status, body } = await request(app) + .put(`/api-keys/${apiKey.id}`) + .send({ name: 'new name' }) + .set('Authorization', `Bearer ${user.accessToken}`); + expect(status).toBe(200); + expect(body).toEqual({ + id: expect.any(String), + name: 'new name', + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + }); + }); + + describe('DELETE /api-keys/:id', () => { + it('should require authentication', async () => { + const { status, body } = await request(app).delete(`/api-keys/${uuidDto.notFound}`); + expect(status).toBe(401); + expect(body).toEqual(errorDto.unauthorized); + }); + + it('should require authorization', async () => { + const { apiKey } = await create(user.accessToken); + const { status, body } = await request(app) + .delete(`/api-keys/${apiKey.id}`) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest('API Key not found')); + }); + + it('should require a valid uuid', async () => { + const { status, body } = await request(app) + .delete(`/api-keys/${uuidDto.invalid}`) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(['id must be a UUID'])); + }); + + it('should delete an api key', async () => { + const { apiKey } = await create(user.accessToken); + const { status } = await request(app) + .delete(`/api-keys/${apiKey.id}`) + .set('Authorization', `Bearer ${user.accessToken}`); + expect(status).toBe(204); + }); + }); + + describe('authentication', () => { + it('should work as a header', async () => { + const { secret } = await create(admin.accessToken); + const { status, body } = await request(app).get('/api-keys').set('x-api-key', secret); + expect(body).toHaveLength(1); + expect(status).toBe(200); + }); + + it('should work as a query param', async () => { + const { secret } = await create(admin.accessToken); + const { status, body } = await request(app).get(`/api-keys?apiKey=${secret}`); + expect(body).toHaveLength(1); + expect(status).toBe(200); + }); + }); +}); diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index f2693f1913..aa0d9fa2bb 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -1185,7 +1185,7 @@ } ], "responses": { - "200": { + "204": { "description": "" } }, diff --git a/server/src/controllers/api-key.controller.ts b/server/src/controllers/api-key.controller.ts index 54144e78d5..feba7cccbb 100644 --- a/server/src/controllers/api-key.controller.ts +++ b/server/src/controllers/api-key.controller.ts @@ -1,4 +1,4 @@ -import { Body, Controller, Delete, Get, Param, Post, Put } from '@nestjs/common'; +import { Body, Controller, Delete, Get, HttpCode, HttpStatus, Param, Post, Put } from '@nestjs/common'; import { ApiTags } from '@nestjs/swagger'; import { APIKeyCreateDto, APIKeyCreateResponseDto, APIKeyResponseDto, APIKeyUpdateDto } from 'src/dtos/api-key.dto'; import { AuthDto } from 'src/dtos/auth.dto'; @@ -40,6 +40,7 @@ export class APIKeyController { } @Delete(':id') + @HttpCode(HttpStatus.NO_CONTENT) @Authenticated() deleteApiKey(@Auth() auth: AuthDto, @Param() { id }: UUIDParamDto): Promise { return this.service.delete(auth, id); diff --git a/server/src/dtos/auth.dto.ts b/server/src/dtos/auth.dto.ts index 6488901fb6..f2d5bd2324 100644 --- a/server/src/dtos/auth.dto.ts +++ b/server/src/dtos/auth.dto.ts @@ -25,6 +25,8 @@ export enum ImmichHeader { export enum ImmichQuery { SHARED_LINK_KEY = 'key', + API_KEY = 'apiKey', + SESSION_KEY = 'sessionKey', } export type CookieResponse = { diff --git a/server/src/middleware/auth.guard.ts b/server/src/middleware/auth.guard.ts index bac25d80ed..c4aa928dbd 100644 --- a/server/src/middleware/auth.guard.ts +++ b/server/src/middleware/auth.guard.ts @@ -89,20 +89,14 @@ export class AuthGuard implements CanActivate { return true; } + const { admin: adminRoute, sharedLink: sharedLinkRoute } = { sharedLink: false, admin: false, ...options }; const request = context.switchToHttp().getRequest(); - const authDto = await this.authService.validate(request.headers, request.query as Record); - if (authDto.sharedLink && !(options as SharedLinkRoute).sharedLink) { - this.logger.warn(`Denied access to non-shared route: ${request.path}`); - return false; - } - - if (!authDto.user.isAdmin && (options as AdminRoute).admin) { - this.logger.warn(`Denied access to admin only route: ${request.path}`); - return false; - } - - request.user = authDto; + request.user = await this.authService.authenticate({ + headers: request.headers, + queryParams: request.query as Record, + metadata: { adminRoute, sharedLinkRoute, uri: request.path }, + }); return true; } diff --git a/server/src/repositories/event.repository.ts b/server/src/repositories/event.repository.ts index aecc9d7239..0bb973b293 100644 --- a/server/src/repositories/event.repository.ts +++ b/server/src/repositories/event.repository.ts @@ -59,7 +59,11 @@ export class EventRepository implements OnGatewayConnection, OnGatewayDisconnect async handleConnection(client: Socket) { try { this.logger.log(`Websocket Connect: ${client.id}`); - const auth = await this.authService.validate(client.request.headers, {}); + const auth = await this.authService.authenticate({ + headers: client.request.headers, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: '/api/socket.io' }, + }); await client.join(auth.user.id); this.serverSend(ServerEvent.WEBSOCKET_CONNECT, { userId: auth.user.id }); } catch (error: Error | any) { diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 7aa03e6bdd..ed73c5aa00 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -1,7 +1,5 @@ -import { BadRequestException, UnauthorizedException } from '@nestjs/common'; -import { IncomingHttpHeaders } from 'node:http'; +import { BadRequestException, ForbiddenException, UnauthorizedException } from '@nestjs/common'; import { Issuer, generators } from 'openid-client'; -import { Socket } from 'socket.io'; import { AuthType } from 'src/constants'; import { AuthDto, SignUpDto } from 'src/dtos/auth.dto'; import { UserMetadataEntity } from 'src/entities/user-metadata.entity'; @@ -252,15 +250,26 @@ describe('AuthService', () => { }); describe('validate - socket connections', () => { - it('should throw token is not provided', async () => { - await expect(sut.validate({}, {})).rejects.toBeInstanceOf(UnauthorizedException); + it('should throw when token is not provided', async () => { + await expect( + sut.authenticate({ + headers: {}, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); }); it('should validate using authorization header', async () => { userMock.get.mockResolvedValue(userStub.user1); sessionMock.getByToken.mockResolvedValue(sessionStub.valid); - const client = { request: { headers: { authorization: 'Bearer auth_token' } } }; - await expect(sut.validate((client as Socket).request.headers, {})).resolves.toEqual({ + await expect( + sut.authenticate({ + headers: { authorization: 'Bearer auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).resolves.toEqual({ user: userStub.user1, session: sessionStub.valid, }); @@ -270,28 +279,48 @@ describe('AuthService', () => { describe('validate - shared key', () => { it('should not accept a non-existent key', async () => { shareMock.getByKey.mockResolvedValue(null); - const headers: IncomingHttpHeaders = { 'x-immich-share-key': 'key' }; - await expect(sut.validate(headers, {})).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + sut.authenticate({ + headers: { 'x-immich-share-key': 'key' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: true, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); }); it('should not accept an expired key', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.expired); - const headers: IncomingHttpHeaders = { 'x-immich-share-key': 'key' }; - await expect(sut.validate(headers, {})).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + sut.authenticate({ + headers: { 'x-immich-share-key': 'key' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: true, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); }); it('should not accept a key without a user', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.expired); userMock.get.mockResolvedValue(null); - const headers: IncomingHttpHeaders = { 'x-immich-share-key': 'key' }; - await expect(sut.validate(headers, {})).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + sut.authenticate({ + headers: { 'x-immich-share-key': 'key' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: true, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); }); it('should accept a base64url key', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.valid); userMock.get.mockResolvedValue(userStub.admin); - const headers: IncomingHttpHeaders = { 'x-immich-share-key': sharedLinkStub.valid.key.toString('base64url') }; - await expect(sut.validate(headers, {})).resolves.toEqual({ + await expect( + sut.authenticate({ + headers: { 'x-immich-share-key': sharedLinkStub.valid.key.toString('base64url') }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: true, uri: 'test' }, + }), + ).resolves.toEqual({ user: userStub.admin, sharedLink: sharedLinkStub.valid, }); @@ -301,8 +330,13 @@ describe('AuthService', () => { it('should accept a hex key', async () => { shareMock.getByKey.mockResolvedValue(sharedLinkStub.valid); userMock.get.mockResolvedValue(userStub.admin); - const headers: IncomingHttpHeaders = { 'x-immich-share-key': sharedLinkStub.valid.key.toString('hex') }; - await expect(sut.validate(headers, {})).resolves.toEqual({ + await expect( + sut.authenticate({ + headers: { 'x-immich-share-key': sharedLinkStub.valid.key.toString('hex') }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: true, uri: 'test' }, + }), + ).resolves.toEqual({ user: userStub.admin, sharedLink: sharedLinkStub.valid, }); @@ -313,24 +347,50 @@ describe('AuthService', () => { describe('validate - user token', () => { it('should throw if no token is found', async () => { sessionMock.getByToken.mockResolvedValue(null); - const headers: IncomingHttpHeaders = { 'x-immich-user-token': 'auth_token' }; - await expect(sut.validate(headers, {})).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + sut.authenticate({ + headers: { 'x-immich-user-token': 'auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); }); it('should return an auth dto', async () => { sessionMock.getByToken.mockResolvedValue(sessionStub.valid); - const headers: IncomingHttpHeaders = { cookie: 'immich_access_token=auth_token' }; - await expect(sut.validate(headers, {})).resolves.toEqual({ + await expect( + sut.authenticate({ + headers: { cookie: 'immich_access_token=auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).resolves.toEqual({ user: userStub.user1, session: sessionStub.valid, }); }); + it('should throw if admin route and not an admin', async () => { + sessionMock.getByToken.mockResolvedValue(sessionStub.valid); + await expect( + sut.authenticate({ + headers: { cookie: 'immich_access_token=auth_token' }, + queryParams: {}, + metadata: { adminRoute: true, sharedLinkRoute: false, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + it('should update when access time exceeds an hour', async () => { sessionMock.getByToken.mockResolvedValue(sessionStub.inactive); sessionMock.update.mockResolvedValue(sessionStub.valid); - const headers: IncomingHttpHeaders = { cookie: 'immich_access_token=auth_token' }; - await expect(sut.validate(headers, {})).resolves.toBeDefined(); + await expect( + sut.authenticate({ + headers: { cookie: 'immich_access_token=auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).resolves.toBeDefined(); expect(sessionMock.update.mock.calls[0][0]).toMatchObject({ id: 'not_active', updatedAt: expect.any(Date) }); }); }); @@ -338,15 +398,25 @@ describe('AuthService', () => { describe('validate - api key', () => { it('should throw an error if no api key is found', async () => { keyMock.getKey.mockResolvedValue(null); - const headers: IncomingHttpHeaders = { 'x-api-key': 'auth_token' }; - await expect(sut.validate(headers, {})).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + sut.authenticate({ + headers: { 'x-api-key': 'auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).rejects.toBeInstanceOf(UnauthorizedException); expect(keyMock.getKey).toHaveBeenCalledWith('auth_token (hashed)'); }); it('should return an auth dto', async () => { keyMock.getKey.mockResolvedValue(keyStub.admin); - const headers: IncomingHttpHeaders = { 'x-api-key': 'auth_token' }; - await expect(sut.validate(headers, {})).resolves.toEqual({ user: userStub.admin, apiKey: keyStub.admin }); + await expect( + sut.authenticate({ + headers: { 'x-api-key': 'auth_token' }, + queryParams: {}, + metadata: { adminRoute: false, sharedLinkRoute: false, uri: 'test' }, + }), + ).resolves.toEqual({ user: userStub.admin, apiKey: keyStub.admin }); expect(keyMock.getKey).toHaveBeenCalledWith('auth_token (hashed)'); }); }); diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index c151c10a66..0ba44601b9 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -1,5 +1,6 @@ import { BadRequestException, + ForbiddenException, Inject, Injectable, InternalServerErrorException, @@ -19,6 +20,7 @@ import { ChangePasswordDto, ImmichCookie, ImmichHeader, + ImmichQuery, LoginCredentialDto, LogoutResponseDto, OAuthAuthorizeResponseDto, @@ -53,6 +55,16 @@ interface ClaimOptions { isValid: (value: unknown) => boolean; } +export type ValidateRequest = { + headers: IncomingHttpHeaders; + queryParams: Record; + metadata: { + sharedLinkRoute: boolean; + adminRoute: boolean; + uri: string; + }; +}; + @Injectable() export class AuthService { private configCore: SystemConfigCore; @@ -143,14 +155,31 @@ export class AuthService { return mapUserAdmin(admin); } - async validate(headers: IncomingHttpHeaders, params: Record): Promise { - const shareKey = (headers[ImmichHeader.SHARED_LINK_KEY] || params.key) as string; + async authenticate({ headers, queryParams, metadata }: ValidateRequest): Promise { + const authDto = await this.validate({ headers, queryParams }); + const { adminRoute, sharedLinkRoute, uri } = metadata; + + if (!authDto.user.isAdmin && adminRoute) { + this.logger.warn(`Denied access to admin only route: ${uri}`); + throw new ForbiddenException('Forbidden'); + } + + if (authDto.sharedLink && !sharedLinkRoute) { + this.logger.warn(`Denied access to non-shared route: ${uri}`); + throw new ForbiddenException('Forbidden'); + } + + return authDto; + } + + private async validate({ headers, queryParams }: Omit): Promise { + const shareKey = (headers[ImmichHeader.SHARED_LINK_KEY] || queryParams[ImmichQuery.SHARED_LINK_KEY]) as string; const session = (headers[ImmichHeader.USER_TOKEN] || headers[ImmichHeader.SESSION_TOKEN] || - params.sessionKey || + queryParams[ImmichQuery.SESSION_KEY] || this.getBearerToken(headers) || this.getCookieToken(headers)) as string; - const apiKey = (headers[ImmichHeader.API_KEY] || params.apiKey) as string; + const apiKey = (headers[ImmichHeader.API_KEY] || queryParams[ImmichQuery.API_KEY]) as string; if (shareKey) { return this.validateSharedLink(shareKey);