fix: api key creation cli #1637
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughBumps api/dev config version. Refactors ApiKeyService role parsing to validate and warn on invalid roles. Adds gating methods and adjusts overwrite lookup in AddApiKeyQuestionSet. Adds --overwrite flag and non-interactive overwrite handling in ApiKeyCommand. Adds comprehensive unit tests for service, CLI command, and question set. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as ApiKeyCommand
participant Inq as InquirerService
participant Svc as ApiKeyService
participant Log as LogService
User->>CLI: run(options)
alt create === false
CLI->>Svc: findByField('name', options.name)
alt Found
Svc-->>CLI: existingKey
CLI->>Log: info(existingKey.value)
CLI-->>User: existingKey
else Not Found
CLI-->>User: null
end
else create === true
alt has name AND (roles OR permissions)
note over CLI: Skip prompts
else missing minimum info
CLI->>Inq: prompt('add-api-key', options)
Inq-->>CLI: answers
end
alt existingKey found AND not overwrite
CLI->>Log: error("API key with name 'X' already exists. Use --overwrite to replace it.")
CLI-->>User: exit
else
CLI->>Svc: create({ name, description, roles, permissions, overwrite })
Svc-->>CLI: newKey
CLI->>Log: info(newKey.value)
CLI-->>User: newKey
end
end
sequenceDiagram
autonumber
participant CMD as ApiKeyCommand.parseRoles
participant Svc as ApiKeyService.convertRolesStringArrayToRoles
participant Log as ApiKeyService.logger
CMD->>Svc: convert([" admin ", "Foo", "USER"])
Svc->>Svc: normalize each (trim + uppercase)
Svc->>Svc: validate against validRoles
alt any invalid
Svc->>Log: warn("Invalid roles: Foo")
end
Svc-->>CMD: [Role.ADMIN, Role.USER]
alt result empty
CMD-->>CMD: throw Error("Invalid roles. Valid options are: ...")
else
CMD-->>CMD: return roles
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @elibosley's task —— View job Code Review Analysis - DIFF Changes OnlyTodo List:
✅ No critical issues found in changes Analysis Summary:
Key improvements in changes:
No security vulnerabilities, breaking changes, data loss risks, or logic errors detected in the modified lines. |
| if (invalidRoles.length > 0) { | ||
| this.logger.warn(`Ignoring invalid roles: ${invalidRoles.join(', ')}`); | ||
| } | ||
|
|
||
| return validRoles; |
There was a problem hiding this comment.
would invalidRoles be useful to return from here as well?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
api/src/unraid-api/auth/api-key.service.ts (1)
112-132: Consider deduping roles to avoid redundant entries.
Repeated roles offer no additional privilege and just bloat payloads.Apply minimal dedupe while preserving order:
public convertRolesStringArrayToRoles(roles: string[]): Role[] { const validRoles: Role[] = []; const invalidRoles: string[] = []; for (const roleStr of roles) { const upperRole = roleStr.trim().toUpperCase(); const role = Role[upperRole as keyof typeof Role]; if (role && ApiKeyService.validRoles.has(role)) { validRoles.push(role); } else { invalidRoles.push(roleStr); } } if (invalidRoles.length > 0) { this.logger.warn(`Ignoring invalid roles: ${invalidRoles.join(', ')}`); } - return validRoles; + return Array.from(new Set(validRoles)); }Note: Update tests if you adopt this.
api/src/unraid-api/auth/api-key.service.spec.ts (1)
768-774: Test name contradicts behavior.
The implementation preserves duplicates; rename to reflect that, or dedupe in code.- it('should deduplicate roles', () => { + it('should preserve duplicate roles', () => {api/src/unraid-api/cli/apikey/add-api-key.questions.ts (1)
76-83: Type nit: confirm returns boolean, not string.
Prevents type drift in options.- parseOverwrite(val: string) { + parseOverwrite(val: boolean) { return val; }api/src/unraid-api/cli/apikey/api-key.command.spec.ts (7)
5-5: Reset mocks between testsAdd afterEach to clear mocks to avoid inter-test leakage.
Apply this diff:
-import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';And insert after the providers are retrieved:
@@ - logService = module.get<LogService>(LogService); + logService = module.get<LogService>(LogService); + inquirerService = module.get<InquirerService>(InquirerService); }); + afterEach(() => { + vi.clearAllMocks(); + });
11-15: Expose InquirerService for assertionsCapture InquirerService so tests can assert no prompting when not needed.
Apply this diff:
describe('ApiKeyCommand', () => { let command: ApiKeyCommand; let apiKeyService: ApiKeyService; let logService: LogService; + let inquirerService: InquirerService;
71-80: Avoid coupling to whitespace in parseRoles -> service handoffAsserting exact args with a leading space is brittle. Just assert delegation occurred and the result.
Apply this diff:
- expect(mockConvert).toHaveBeenCalledWith(['ADMIN', ' VIEWER']); + expect(mockConvert).toHaveBeenCalledTimes(1); expect(result).toEqual([Role.ADMIN, Role.VIEWER]);
144-152: Assert no prompt when creating with sufficient argsEnsure CLI skips interactive prompt on fully-specified create.
Apply this diff:
expect(apiKeyService.create).toHaveBeenCalledWith({ name: 'TEST', description: 'Test description', roles: [Role.ADMIN], permissions: undefined, overwrite: true, }); expect(logService.log).toHaveBeenCalledWith('test-key-123'); + expect(inquirerService.prompt).not.toHaveBeenCalled();
181-189: Also assert no prompt when creating with only permissionsKeeps behavior-focused and prevents regressions.
Apply this diff:
expect(apiKeyService.create).toHaveBeenCalledWith({ name: 'TEST_PERMS', description: 'Test with permissions', roles: undefined, permissions: mockPermissions, overwrite: true, }); expect(logService.log).toHaveBeenCalledWith('test-key-456'); + expect(inquirerService.prompt).not.toHaveBeenCalled();
210-217: Assert no prompt on default-description pathSame rationale as above.
Apply this diff:
expect(apiKeyService.create).toHaveBeenCalledWith({ name: 'NO_DESC', description: 'CLI generated key: NO_DESC', roles: [Role.VIEWER], permissions: undefined, overwrite: true, }); + expect(inquirerService.prompt).not.toHaveBeenCalled();
240-284: Remove duplicate parseRoles tests misplaced under “run”These three tests re-cover parseRoles and belong above (already covered). Keeping them here increases noise and maintenance.
Apply this diff to delete the duplicates:
- it('should handle uppercase role conversion', () => { - const mockConvert = vi - .spyOn(apiKeyService, 'convertRolesStringArrayToRoles') - .mockImplementation((roles) => { - return roles - .map((roleStr) => Role[roleStr.trim().toUpperCase() as keyof typeof Role]) - .filter(Boolean); - }); - - const result = command.parseRoles('admin,connect'); - - expect(mockConvert).toHaveBeenCalledWith(['admin', 'connect']); - expect(result).toEqual([Role.ADMIN, Role.CONNECT]); - }); - - it('should handle lowercase role conversion', () => { - const mockConvert = vi - .spyOn(apiKeyService, 'convertRolesStringArrayToRoles') - .mockImplementation((roles) => { - return roles - .map((roleStr) => Role[roleStr.trim().toUpperCase() as keyof typeof Role]) - .filter(Boolean); - }); - - const result = command.parseRoles('viewer'); - - expect(mockConvert).toHaveBeenCalledWith(['viewer']); - expect(result).toEqual([Role.VIEWER]); - }); - - it('should handle mixed case role conversion', () => { - const mockConvert = vi - .spyOn(apiKeyService, 'convertRolesStringArrayToRoles') - .mockImplementation((roles) => { - return roles - .map((roleStr) => Role[roleStr.trim().toUpperCase() as keyof typeof Role]) - .filter(Boolean); - }); - - const result = command.parseRoles('Admin,CoNnEcT'); - - expect(mockConvert).toHaveBeenCalledWith(['Admin', 'CoNnEcT']); - expect(result).toEqual([Role.ADMIN, Role.CONNECT]); - });api/src/unraid-api/cli/__test__/api-key.command.test.ts (4)
4-4: Reset mocks between testsAdd afterEach to clear mocks across the suite.
Apply this diff:
-import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';And insert after providers are retrieved:
@@ - questionSet = module.get<AddApiKeyQuestionSet>(AddApiKeyQuestionSet); + questionSet = module.get<AddApiKeyQuestionSet>(AddApiKeyQuestionSet); }); + afterEach(() => { + vi.clearAllMocks(); + });
86-95: Also assert no creation/prompt when fetching existing keyStrengthens behavior guarantees.
Apply this diff:
expect(apiKeyService.findByField).toHaveBeenCalledWith('name', 'test-key'); expect(logService.log).toHaveBeenCalledWith('test-api-key-123'); + expect(apiKeyService.create).not.toHaveBeenCalled(); + expect(inquirerService.prompt).not.toHaveBeenCalled();
101-116: Assert no prompt when creating with sufficient argsAvoids regressions into unnecessary interactivity.
Apply this diff:
expect(apiKeyService.create).toHaveBeenCalledWith({ name: 'new-key', description: 'Test description', roles: ['ADMIN'], permissions: undefined, overwrite: true, }); expect(logService.log).toHaveBeenCalledWith('new-api-key-456'); + expect(inquirerService.prompt).not.toHaveBeenCalled();
118-136: Strengthen assertion on created payload after promptVerify overwrite flag and key fields without being brittle.
Apply this diff:
expect(inquirerService.prompt).toHaveBeenCalledWith('add-api-key', { name: '', create: true, }); - expect(apiKeyService.create).toHaveBeenCalled(); + expect(apiKeyService.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'prompted-key', + roles: ['USER'], + permissions: [], + description: 'Prompted description', + overwrite: true, + }), + );
📜 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.
📒 Files selected for processing (7)
api/dev/configs/api.json(1 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(1 hunks)api/src/unraid-api/auth/api-key.service.ts(1 hunks)api/src/unraid-api/cli/__test__/api-key.command.test.ts(1 hunks)api/src/unraid-api/cli/apikey/add-api-key.questions.ts(3 hunks)api/src/unraid-api/cli/apikey/api-key.command.spec.ts(1 hunks)api/src/unraid-api/cli/apikey/api-key.command.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/apikey/add-api-key.questions.tsapi/src/unraid-api/cli/apikey/api-key.command.tsapi/src/unraid-api/auth/api-key.service.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.spec.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.ts
api/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS code at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/apikey/add-api-key.questions.tsapi/src/unraid-api/cli/apikey/api-key.command.tsapi/src/unraid-api/auth/api-key.service.spec.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx,js,jsx}: API test suite is Vitest; do not use Jest
In API tests, prefer to not mock simple dependencies
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.spec.ts
{api/**/*.{test,spec}.{ts,tsx,js,jsx},web/__test__/**/*.{test,spec}.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{api/**/*.{test,spec}.{ts,tsx,js,jsx},web/__test__/**/*.{test,spec}.{ts,tsx,js,jsx}}: Use .rejects.toThrow() without arguments when asserting thrown errors in tests; don’t assert exact error strings unless that is what’s being tested
Focus on behavior in tests rather than implementation details (e.g., exact error message wording)
Avoid brittle tests that break on minor changes to messages, logs, or non-essential details
Use mocks as nouns, not verbs
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In TypeScript files, use .js extensions in import specifiers for ESM compatibility
Files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/apikey/add-api-key.questions.tsapi/src/unraid-api/cli/apikey/api-key.command.tsapi/src/unraid-api/auth/api-key.service.spec.ts
🧠 Learnings (6)
📚 Learning: 2025-08-28T20:27:35.954Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:27:35.954Z
Learning: Applies to {api/**/*.{test,spec}.{ts,tsx,js,jsx},web/__test__/**/*.{test,spec}.{ts,tsx,js,jsx}} : Avoid brittle tests that break on minor changes to messages, logs, or non-essential details
Applied to files:
api/src/unraid-api/cli/__test__/api-key.command.test.tsapi/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.spec.ts
📚 Learning: 2025-08-28T20:27:35.954Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:27:35.954Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx,js,jsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/cli/__test__/api-key.command.test.ts
📚 Learning: 2025-08-11T15:07:39.222Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Applies to api/**/*.{test,spec}.{js,jsx,ts,tsx} : Use Vitest for tests in the api; do not use Jest
Applied to files:
api/src/unraid-api/cli/__test__/api-key.command.test.ts
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/apikey/add-api-key.questions.tsapi/src/unraid-api/cli/apikey/api-key.command.tsapi/src/unraid-api/auth/api-key.service.spec.ts
📚 Learning: 2024-11-04T20:41:22.303Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:41:22.303Z
Learning: In `api/src/unraid-api/auth/auth.service.ts`, the `addRoleToApiKey` function operates on API keys stored as JSON files in a directory, not a database, so concurrency is not a concern for modifying `apiKey.roles`.
Applied to files:
api/src/unraid-api/cli/apikey/api-key.command.spec.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/apikey/api-key.command.tsapi/src/unraid-api/auth/api-key.service.spec.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/auth/api-key.service.ts
🧬 Code graph analysis (2)
api/src/unraid-api/auth/api-key.service.ts (1)
packages/unraid-shared/src/services/api-key.ts (1)
ApiKeyService(9-77)
api/src/unraid-api/auth/api-key.service.spec.ts (2)
api/src/store/index.ts (1)
getters(18-23)packages/unraid-shared/src/services/api-key.ts (1)
ApiKeyService(9-77)
🔇 Additional comments (10)
api/dev/configs/api.json (1)
2-2: LGTM: version bump only.api/src/unraid-api/auth/api-key.service.ts (1)
112-132: Clearer, safer role parsing with validation and warnings.
Good move away from map/filter; trims, normalizes, and warns as expected.api/src/unraid-api/cli/apikey/api-key.command.ts (2)
56-64: Good: delegate role parsing to service.
Centralizes validation and keeps CLI lean.
145-146: Nice: avoid logging sensitive payloads.
Printing a neutral progress message reduces accidental secret leakage.api/src/unraid-api/cli/apikey/add-api-key.questions.ts (4)
42-47: Good gating: roles asked only when missing.
Aligns with the create CLI’s minimum-info logic.
62-67: Good gating: permissions asked only when missing.
Keeps wizard concise.
87-87: Overwrite check now correctly uses name lookup.
Matches how the CLI searches keys.
42-47: Ensure CLI honors overwrite collected here.
This question is useful only if ApiKeyCommand passes options.overwrite to create(); see suggested fix there.api/src/unraid-api/cli/apikey/api-key.command.spec.ts (1)
3-10: Good: ESM-friendly import specifiers and Vitest usageImports use .js extensions and the suite uses Vitest as required.
api/src/unraid-api/cli/__test__/api-key.command.test.ts (1)
1-9: Good: Vitest and ESM-compatible importsConforms to our testing and ESM import guidelines.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.18.0](v4.17.0...v4.18.0) (2025-09-02) ### Features * **api:** enhance OIDC redirect URI handling in service and tests ([#1618](#1618)) ([4e945f5](4e945f5)) ### Bug Fixes * api key creation cli ([#1637](#1637)) ([c147a6b](c147a6b)) * **cli:** support `--log-level` for `start` and `restart` cmds ([#1623](#1623)) ([a1ee915](a1ee915)) * confusing server -> status query ([#1635](#1635)) ([9d42b36](9d42b36)) * use unraid css variables in sonner ([#1634](#1634)) ([26a95af](26a95af)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit