Skip to content

Conversation

@elibosley
Copy link
Member

@elibosley elibosley commented Aug 18, 2025

Summary by CodeRabbit

  • New Features
    • Added a CLI option to list API keys, showing key details (name, description, roles, permissions, and value).
  • Tests
    • Introduced comprehensive test coverage for the API key CLI, including listing, creation, deletion, lookup by name, and option parsing, with both success and error scenarios.
  • Chores
    • Updated API configuration version to 4.13.1.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 18, 2025

Walkthrough

Adds a new --list capability to the API key CLI, updates option parsing and command flow to handle listing keys, and introduces a comprehensive unit test suite for the command. Also increments the API config version from 4.12.0 to 4.13.1.

Changes

Cohort / File(s) Summary
API key CLI: list feature & tests
api/src/unraid-api/cli/apikey/api-key.command.ts, api/src/unraid-api/cli/apikey/api-key.command.spec.ts
Introduces --list flag, adds parseList(), updates KeyOptions with list?: boolean, implements listKeys() to enumerate and print key details, adjusts roles parsing; adds extensive unit tests covering list, delete, create, fetch-by-name, option parsing, and error paths.
Config version bump
api/dev/configs/api.json
Increments version from "4.12.0" to "4.13.1"; no other changes.

Sequence Diagram(s)

sequenceDiagram
  participant U as User
  participant C as ApiKeyCommand
  participant S as ApiKeyService
  participant L as LogService

  U->>C: run(--list)
  C->>S: getAllApiKeys()
  S-->>C: [ApiKeys]
  loop for each key
    C->>S: getApiKeySecret(key)
    S-->>C: secret
    C->>L: log key details + secret
  end
  C-->>U: printed list
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • mdatelle
  • pujitm
  • zackspear

Poem

A hop, a flag, a list I see,
Keys parade in terminal glee.
Roles and perms in tidy rows,
Secrets whispered, only it knows.
Version ticks to one-three-one,
Bunny stamps “done”—another run. 🐇✨

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 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 541b0ed and 618871f.

📒 Files selected for processing (3)
  • api/dev/configs/api.json (1 hunks)
  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts (1 hunks)
  • api/src/unraid-api/cli/apikey/api-key.command.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

TypeScript source files must use import specifiers with .js extensions for ESM compatibility

Files:

  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts
  • api/src/unraid-api/cli/apikey/api-key.command.ts
api/src/unraid-api/**

📄 CodeRabbit Inference Engine (CLAUDE.md)

Place new API source files in api/src/unraid-api/ rather than legacy locations

Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code

Files:

  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts
  • api/src/unraid-api/cli/apikey/api-key.command.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Prefer not to mock simple dependencies in API tests

Files:

  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)

Files:

  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Avoid unnecessary or obvious comments; only add comments when needed for clarity

Files:

  • api/src/unraid-api/cli/apikey/api-key.command.spec.ts
  • api/src/unraid-api/cli/apikey/api-key.command.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/apikey/api-key.command.spec.ts
🔇 Additional comments (13)
api/dev/configs/api.json (1)

2-2: Version bump aligns with the new API key listing feature.

The version update from "4.12.0" to "4.13.1" is appropriate for the addition of the new --list functionality to the API key CLI command.

api/src/unraid-api/cli/apikey/api-key.command.ts (7)

16-16: LGTM! New list option properly defined.

The list?: boolean property is correctly added to the KeyOptions interface to support the new listing functionality.


24-24: Command description accurately reflects all capabilities.

The updated description properly includes the new --list option alongside the existing functionality.


59-59: Good improvement: Role parsing now uses uppercase for consistency.

Converting roles to uppercase ensures consistency with the Role enum values, which improves the user experience by making the CLI case-insensitive.


103-109: New parseList option follows established patterns.

The implementation correctly follows the same pattern as other boolean options in the command, consistent with the existing parseCreate() and parseDelete() methods.


172-172: Default options properly include the new list property.

The default options object correctly includes list: false to maintain backward compatibility.


175-178: List option handling is well-positioned and implemented.

The early return pattern for the list option is consistent with the existing delete option handling and prevents conflicts between different command modes.


137-168: Ignore async await for findByField – it’s synchronous

The findByField method on ApiKeyService is declared as

public findByField(field: keyof ApiKeyWithSecret, value: string): ApiKeyWithSecret | null {}

and returns its result directly from in-memory data. No Promise is involved, so adding await would be both unnecessary and incorrect.

• Location: api/src/unraid-api/auth/api-key.service.ts, lines 238–242

Likely an incorrect or invalid review comment.

api/src/unraid-api/cli/apikey/api-key.command.spec.ts (5)

21-40: Comprehensive mock data setup covers various scenarios.

The mock API keys include different roles, permissions, and properties that enable thorough testing of the listing functionality.


92-135: Thorough testing of the new list functionality.

The test cases comprehensively cover the list option behavior including:

  • Multiple keys with detailed output verification
  • Empty state handling
  • Singular vs plural messaging

137-192: Comprehensive delete option testing with edge cases.

The delete tests cover all important scenarios including successful deletion, no selection, empty state, and error handling with proper process exit verification.


194-254: Complete create option testing covers validation requirements.

The create tests properly verify key creation with roles, existing key handling, and validation of required roles/permissions.


282-323: Excellent coverage of option parsing methods.

The option parsing tests verify all new and existing parsing methods including the new parseList() method, with appropriate edge case testing for role validation.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/api-key-listing

🪧 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
Copy link

claude bot commented Aug 18, 2025

Claude finished @elibosley's task —— View job


PR Review: API Key Listing Feature

Reviewing the changes for critical issues in the diff only:

  • Read and analyze changed files
  • Check for critical bugs, security issues, breaking changes, and data loss risks
  • Provide review feedback

Analysis of Changes

🔍 Security Issue Found:

In api-key.command.ts line 162-165, the listKeys() method exposes the actual API key secrets in plaintext:

// Get the actual key value using findByField
const keyWithSecret = this.apiKeyService.findByField('id', key.id);
if (keyWithSecret) {
    this.logger.log(`  Key: ${keyWithSecret.key}`);  // ⚠️ Exposes secrets
}

Risk: This logs sensitive API key secrets to console output, which could be:

  • Captured in logs/terminal history
  • Visible to unauthorized users running the command
  • Accidentally shared in screenshots/recordings

Recommendation: Remove secret exposure or add explicit warning/confirmation before displaying secrets.

Other changes (version bump, case fix, new option) appear safe.

@github-actions
Copy link
Contributor

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1593/dynamix.unraid.net.plg

@elibosley elibosley closed this Sep 10, 2025
@elibosley elibosley deleted the feat/api-key-listing branch September 10, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants