From 9cd0d6ac658475efa25683ef6e3f2e1d68f7e903 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 27 Aug 2025 15:50:15 -0400 Subject: [PATCH] fix: remove unused api key calls (#1628) ## Summary by CodeRabbit - New Features - None. - Bug Fixes - Improved reliability of API key updates and clearer errors when keys are missing or data is invalid. - More robust initialization to prevent intermittent failures during API key operations. - Refactor - Simplified API key management by unifying lookup flows and adopting consistent async handling. - Tests - Expanded coverage for error scenarios and strengthened test setup for initialization and file-read issues. - Chores - Minor formatting cleanups. --- .../unraid-api/auth/api-key.service.spec.ts | 17 ++++++------- api/src/unraid-api/auth/api-key.service.ts | 6 +---- api/src/unraid-api/auth/auth.service.spec.ts | 15 ++---------- api/src/unraid-api/auth/auth.service.ts | 24 +++++++------------ .../unraid-shared/src/services/api-key.ts | 5 ---- 5 files changed, 21 insertions(+), 46 deletions(-) diff --git a/api/src/unraid-api/auth/api-key.service.spec.ts b/api/src/unraid-api/auth/api-key.service.spec.ts index 109635c9f..aa5b5b738 100644 --- a/api/src/unraid-api/auth/api-key.service.spec.ts +++ b/api/src/unraid-api/auth/api-key.service.spec.ts @@ -226,12 +226,12 @@ describe('ApiKeyService', () => { }); }); - describe('findByIdWithSecret', () => { - it('should return API key with secret when found', async () => { + describe('findById', () => { + it('should return API key when found', async () => { vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([mockApiKey]); await apiKeyService.onModuleInit(); - const result = await apiKeyService.findByIdWithSecret(mockApiKey.id); + const result = await apiKeyService.findById(mockApiKey.id); expect(result).toEqual(mockApiKey); }); @@ -240,7 +240,7 @@ describe('ApiKeyService', () => { vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([]); await apiKeyService.onModuleInit(); - const result = await apiKeyService.findByIdWithSecret('non-existent-id'); + const result = await apiKeyService.findById('non-existent-id'); expect(result).toBeNull(); }); @@ -353,7 +353,7 @@ describe('ApiKeyService', () => { describe('update', () => { let updateMockApiKey: ApiKey; - beforeEach(() => { + beforeEach(async () => { // Create a fresh copy of the mock data for update tests updateMockApiKey = { id: 'test-api-id', @@ -370,9 +370,11 @@ describe('ApiKeyService', () => { createdAt: new Date().toISOString(), }; - vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([updateMockApiKey]); + // Initialize the memoryApiKeys with the test data + // The loadAllFromDisk mock will be called by onModuleInit + vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([{ ...updateMockApiKey }]); vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); - apiKeyService.onModuleInit(); + await apiKeyService.onModuleInit(); }); it('should update name and description', async () => { @@ -384,7 +386,6 @@ describe('ApiKeyService', () => { name: updatedName, description: updatedDescription, }); - expect(result.name).toBe(updatedName); expect(result.description).toBe(updatedDescription); expect(result.roles).toEqual(updateMockApiKey.roles); diff --git a/api/src/unraid-api/auth/api-key.service.ts b/api/src/unraid-api/auth/api-key.service.ts index 1108485b2..4a7ad6edb 100644 --- a/api/src/unraid-api/auth/api-key.service.ts +++ b/api/src/unraid-api/auth/api-key.service.ts @@ -226,10 +226,6 @@ export class ApiKeyService implements OnModuleInit { } } - public findByIdWithSecret(id: string): ApiKey | null { - return this.findByField('id', id); - } - public findByField(field: keyof ApiKey, value: string): ApiKey | null { if (!value) return null; @@ -330,7 +326,7 @@ export class ApiKeyService implements OnModuleInit { roles?: Role[]; permissions?: Permission[] | AddPermissionInput[]; }): Promise { - const apiKey = this.findByIdWithSecret(id); + const apiKey = await this.findById(id); if (!apiKey) { throw new GraphQLError('API key not found'); } diff --git a/api/src/unraid-api/auth/auth.service.spec.ts b/api/src/unraid-api/auth/auth.service.spec.ts index 961368b89..d3318e28d 100644 --- a/api/src/unraid-api/auth/auth.service.spec.ts +++ b/api/src/unraid-api/auth/auth.service.spec.ts @@ -228,10 +228,6 @@ describe('AuthService', () => { }; vi.spyOn(apiKeyService, 'findById').mockResolvedValue(mockApiKeyWithoutRole); - vi.spyOn(apiKeyService, 'findByIdWithSecret').mockResolvedValue({ - ...mockApiKey, - roles: [Role.ADMIN], - }); vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); vi.spyOn(authzService, 'addRoleForUser').mockResolvedValue(true); @@ -239,9 +235,8 @@ describe('AuthService', () => { expect(result).toBe(true); expect(apiKeyService.findById).toHaveBeenCalledWith(apiKeyId); - expect(apiKeyService.findByIdWithSecret).toHaveBeenCalledWith(apiKeyId); expect(apiKeyService.saveApiKey).toHaveBeenCalledWith({ - ...mockApiKey, + ...mockApiKeyWithoutRole, roles: [Role.ADMIN, role], }); expect(authzService.addRoleForUser).toHaveBeenCalledWith(apiKeyId, role); @@ -259,13 +254,8 @@ describe('AuthService', () => { describe('removeRoleFromApiKey', () => { it('should remove role from API key', async () => { const apiKey = { ...mockApiKey, roles: [Role.ADMIN, Role.GUEST] }; - const apiKeyWithSecret = { - ...mockApiKey, - roles: [Role.ADMIN, Role.GUEST], - }; vi.spyOn(apiKeyService, 'findById').mockResolvedValue(apiKey); - vi.spyOn(apiKeyService, 'findByIdWithSecret').mockResolvedValue(apiKeyWithSecret); vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); vi.spyOn(authzService, 'deleteRoleForUser').mockResolvedValue(true); @@ -273,9 +263,8 @@ describe('AuthService', () => { expect(result).toBe(true); expect(apiKeyService.findById).toHaveBeenCalledWith(apiKey.id); - expect(apiKeyService.findByIdWithSecret).toHaveBeenCalledWith(apiKey.id); expect(apiKeyService.saveApiKey).toHaveBeenCalledWith({ - ...apiKeyWithSecret, + ...apiKey, roles: [Role.GUEST], }); expect(authzService.deleteRoleForUser).toHaveBeenCalledWith(apiKey.id, Role.ADMIN); diff --git a/api/src/unraid-api/auth/auth.service.ts b/api/src/unraid-api/auth/auth.service.ts index 5457a3a04..fd7620227 100644 --- a/api/src/unraid-api/auth/auth.service.ts +++ b/api/src/unraid-api/auth/auth.service.ts @@ -201,15 +201,12 @@ export class AuthService { } try { + if (!apiKey.roles) { + apiKey.roles = []; + } if (!apiKey.roles.includes(role)) { - const apiKeyWithSecret = await this.apiKeyService.findByIdWithSecret(apiKeyId); - - if (!apiKeyWithSecret) { - throw new UnauthorizedException('API key not found with secret'); - } - - apiKeyWithSecret.roles.push(role); - await this.apiKeyService.saveApiKey(apiKeyWithSecret); + apiKey.roles.push(role); + await this.apiKeyService.saveApiKey(apiKey); await this.authzService.addRoleForUser(apiKeyId, role); } @@ -231,14 +228,11 @@ export class AuthService { } try { - const apiKeyWithSecret = await this.apiKeyService.findByIdWithSecret(apiKeyId); - - if (!apiKeyWithSecret) { - throw new UnauthorizedException('API key not found with secret'); + if (!apiKey.roles) { + apiKey.roles = []; } - - apiKeyWithSecret.roles = apiKeyWithSecret.roles.filter((r) => r !== role); - await this.apiKeyService.saveApiKey(apiKeyWithSecret); + apiKey.roles = apiKey.roles.filter((r) => r !== role); + await this.apiKeyService.saveApiKey(apiKey); await this.authzService.deleteRoleForUser(apiKeyId, role); return true; diff --git a/packages/unraid-shared/src/services/api-key.ts b/packages/unraid-shared/src/services/api-key.ts index 2069eb882..13af48867 100644 --- a/packages/unraid-shared/src/services/api-key.ts +++ b/packages/unraid-shared/src/services/api-key.ts @@ -12,11 +12,6 @@ export interface ApiKeyService { */ findById(id: string): Promise; - /** - * Find an API key by its ID - * Note: This returns ApiKey without the secret for security - */ - findByIdWithSecret(id: string): ApiKey | null; /** * Find an API key by a specific field