fix: remove unused api key calls #1628

elibosley · 2025-08-27T16:39:44Z

Summary by CodeRabbit

New Features
- None.
Refactor
- Streamlined API key retrieval to a single, consistent path, simplifying role updates and improving null-safety.
Bug Fixes
- Improved error handling during API key operations, providing more robust behavior when data is missing or unreadable.
Tests
- Expanded coverage for error scenarios and initialization flows to ensure reliable API key loading and updates.
Chores
- Minor formatting cleanup to maintain code quality.

coderabbitai · 2025-08-27T16:39:52Z

Caution

Review failed

The pull request is closed.

Walkthrough

Renamed and removed the findByIdWithSecret API across services and tests, consolidating lookups to the async findById. Updated auth role add/remove flows to operate directly on the returned ApiKey, initializing roles when absent, saving via saveApiKey, and enhancing error handling. Adjusted tests to initialize module state and cover error paths.

Changes

Cohort / File(s)	Summary
Auth API Key Service (impl) `api/src/unraid-api/auth/api-key.service.ts`	Removed public findByIdWithSecret; update path now awaits async findById. Lookup and error behavior unchanged aside from async callsite.
Auth Service (impl) `api/src/unraid-api/auth/auth.service.ts`	Replaced usages of findByIdWithSecret with findById; add/remove role now initializes missing roles array, mutates/saves ApiKey directly, and adds context to error handling.
Auth Tests `api/src/unraid-api/auth/auth.service.spec.ts`	Updated tests to use findById only; expectations adjusted to operate on returned ApiKey; removed secret-based variants.
API Key Service Tests `api/src/unraid-api/auth/api-key.service.spec.ts`	Tests updated to call findById; added error-handling tests for invalid data and file-read failures; async beforeEach with onModuleInit and mocked loadAllFromDisk.
Shared Interface `packages/unraid-shared/src/services/api-key.ts`	Removed ApiKeyService.findByIdWithSecret signature; interface now exposes async findById only.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant Auth as AuthService
  participant KeySvc as ApiKeyService
  participant AuthZ as AuthzService
  participant Store as Storage

  rect rgb(240,248,255)
  note over C,Auth: Add role to API key (updated async flow)
  C->>Auth: addRoleToApiKey(apiKeyId, role)
  Auth->>KeySvc: findById(apiKeyId)
  KeySvc->>Store: read api-keys
  Store-->>KeySvc: ApiKey | null
  KeySvc-->>Auth: ApiKey | null
  alt ApiKey found
    Auth->>Auth: ensure roles[], mutate roles
    Auth->>KeySvc: saveApiKey(ApiKey)
    KeySvc->>Store: write api-key
    Store-->>KeySvc: ok
    KeySvc-->>Auth: ok
    Auth->>AuthZ: addRoleForUser(apiKeyId, role)
    AuthZ-->>Auth: ok
    Auth-->>C: result
  else Not found
    Auth-->>C: error "API key not found"
  end
  end

sequenceDiagram
  autonumber
  participant C as Client
  participant Auth as AuthService
  participant KeySvc as ApiKeyService
  participant AuthZ as AuthzService
  participant Store as Storage

  rect rgb(245,240,255)
  note over C,Auth: Remove role from API key (updated async flow)
  C->>Auth: removeRoleFromApiKey(apiKeyId, role)
  Auth->>KeySvc: findById(apiKeyId)
  KeySvc->>Store: read api-keys
  Store-->>KeySvc: ApiKey | null
  KeySvc-->>Auth: ApiKey | null
  alt ApiKey found
    Auth->>Auth: ensure roles[], filter role
    Auth->>KeySvc: saveApiKey(ApiKey)
    KeySvc->>Store: write api-key
    Store-->>KeySvc: ok
    KeySvc-->>Auth: ok
    Auth->>AuthZ: deleteRoleForUser(apiKeyId, role)
    AuthZ-->>Auth: ok
    Auth-->>C: result
  else Not found
    Auth-->>C: error "API key not found"
  end
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

fix: remove unused api key calls #1628 — Removes findByIdWithSecret and updates all call sites (api-key service, auth service, shared interface, plugin wrapper, tests), overlapping the same functions and refactors.

Poem

I hop through keys, no secrets to find,
Async breezes whisper, tidy and kind.
Roles line up neatly, saved with care,
Errors well-labeled float in the air.
Thump-thump! I review, then cheerfully say:
One clean hop to a simpler day. 🐇✨

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled by default for public repositories
Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 41ea19f and 0fd11d0.

📒 Files selected for processing (5)

api/src/unraid-api/auth/api-key.service.spec.ts (4 hunks)
api/src/unraid-api/auth/api-key.service.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (2 hunks)
api/src/unraid-api/auth/auth.service.ts (2 hunks)
packages/unraid-shared/src/services/api-key.ts (0 hunks)

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/remove-auth-service

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

claude · 2025-08-27T16:39:58Z

Claude finished @elibosley's task —— View job

PR Review - Critical Issues Found

Task: Review ONLY the diff/changeset for HIGH-PRIORITY issues

Todo List:

Read and analyze the changed files
Check for critical bugs in the diff
Look for security issues in changes
Identify any breaking changes
Check for data loss risks
Provide final assessment

🚨 Critical Issue Found in auth.service.ts

CRITICAL BUG - Unnecessary I/O Operations in removeRoleFromApiKey

In removeRoleFromApiKey (lines 228-237), there's a logic flaw:

if (!apiKey.roles) {
    apiKey.roles = [];  // Initialize empty array
}
apiKey.roles = apiKey.roles.filter((r) => r !== role);  // Filter (no-op if role not present)
await this.apiKeyService.saveApiKey(apiKey);            // ⚠️ Always saves, even when no change
await this.authzService.deleteRoleForUser(apiKeyId, role); // ⚠️ Always deletes, even if role wasn't there

Problems:

Unnecessary I/O: If the role doesn't exist, still performs expensive file saves and authz operations
AuthZ Inconsistency: Calling deleteRoleForUser when role was never assigned could cause state issues

Recommended Fix: Check if role exists before filtering, only save/delete if changes were made.

🚨 Breaking Change Identified

PUBLIC API METHOD REMOVAL

Removed: findByIdWithSecret(id: string): ApiKey | null from shared interface and implementation
Replaced with: await findById(id) (async method)
Impact: Any external consumers using the synchronous method will break

✅ Positive Changes Noted

Null Safety: Added apiKey.roles null checks in both addRoleToApiKey and removeRoleFromApiKey - prevents runtime crashes
Async Consistency: Using async findById in update() method creates consistent API usage

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (3)

api/src/unraid-api/auth/auth.service.spec.ts (1)

233-237: Make saveApiKey expectations less brittle

Use partial/arrayContaining to reduce coupling to object shape and order.

Apply:

-            expect(apiKeyService.saveApiKey).toHaveBeenCalledWith({
-                ...mockApiKeyWithoutRole,
-                roles: [Role.ADMIN, role],
-            });
+            expect(apiKeyService.saveApiKey).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    id: apiKeyId,
+                    roles: expect.arrayContaining([Role.ADMIN, role]),
+                })
+            );

-            expect(apiKeyService.saveApiKey).toHaveBeenCalledWith({
-                ...apiKey,
-                roles: [Role.GUEST],
-            });
+            expect(apiKeyService.saveApiKey).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    id: apiKey.id,
+                    roles: [Role.GUEST],
+                })
+            );

Also applies to: 261-265

api/src/unraid-api/auth/api-key.service.spec.ts (2)

229-260: Duplicate “describe('findById')” block

There are two describe('findById') sections; consolidate to avoid redundancy and future drift.

I can propose a follow-up patch to merge these tests into the first block and keep only unique scenarios (invalid structure/read error).

253-254: Follow testing guideline: avoid asserting exact error messages

Prefer generic .rejects.toThrow() to reduce brittleness.

Apply:
-            await expect(apiKeyService.onModuleInit()).rejects.toThrow('Invalid API key structure');
+            await expect(apiKeyService.onModuleInit()).rejects.toThrow();
-            await expect(apiKeyService.onModuleInit()).rejects.toThrow('Read failed');
+            await expect(apiKeyService.onModuleInit()).rejects.toThrow();
Also applies to: 258-259

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled by default for public repositories
Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 674323f and 41ea19f.

📒 Files selected for processing (6)

api/src/unraid-api/auth/api-key.service.spec.ts (4 hunks)
api/src/unraid-api/auth/api-key.service.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (2 hunks)
api/src/unraid-api/auth/auth.service.ts (2 hunks)
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts (0 hunks)
packages/unraid-shared/src/services/api-key.ts (0 hunks)

💤 Files with no reviewable changes (2)

packages/unraid-shared/src/services/api-key.ts
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts

🧰 Additional context used

📓 Path-based instructions (6)

**/*.{ts,tsx}