feat: initial setup of permissions on keys

[elibosley]

Summary by CodeRabbit

Release Notes

• Permission Management

  • Removed legacy role-based permission system.
  • Introduced new granular permission model for API keys and users.
  • Added support for resource-specific permissions.

• API Key Management

  • Enhanced API key handling with more flexible permission configuration.
  • Improved synchronization of API key permissions.
  • Updated GraphQL schema to support new permission structure.

• Authentication

  • Deprecated previous role assignment methods.
  • Introduced new MeResolver for retrieving user information.
  • Simplified permission checking mechanisms.

• GraphQL Schema

  • Updated user and API key types to include permissions.
  • Added new Permission type for more precise access control.

• Testing

  • Updated test suites to reflect new permission and authentication models.
  • Improved test coverage for new functionality.
  • Added new tests for MeResolver.

• Chores

  • Removed unused files and methods related to legacy permission management.
  • Updated configuration files to streamline testing processes.
  • Removed deprecated API key files from the system.

[coderabbitai]

Walkthrough

The pull request introduces significant changes to the permissions and role management system in the Unraid API. This includes the removal of existing role-based access control (RBAC) files, such as permissions.json, and the deletion of related tests and functionalities. New structures for managing permissions in API keys are introduced, along with updates to GraphQL schemas to accommodate these changes. Overall, the modifications streamline and modernize how permissions are handled within the application.

Changes

File	Change Summary
api/dev/data/permissions.json	Deleted role definitions for admin, user, and guest
api/package.json	Added container:stop script
api/src/__test__/core/permissions.test.ts	Deleted test file for setupPermissions function
api/src/core/index.ts	Removed exports for permission-related modules
api/src/core/modules/index.ts	Removed exports for permission-related modules
api/src/core/modules/get-me.ts	Deleted getMe function for retrieving user info
api/src/core/modules/get-permissions.ts	Deleted getPermissions function for user permissions
api/src/core/permission-manager.ts	Removed PermissionManager class and related functionality
api/src/core/permissions.ts	Deleted role-based access control definitions and setup function
api/src/core/utils/permissions/check-permission.ts	Updated checkPermission function to disable permission checks
api/src/core/utils/permissions/ensure-permission.ts	Deprecated ensurePermission function with modified parameters
api/src/core/utils/permissions/get-permissions.ts	Deleted getPermissions function for role permissions
api/src/graphql/schema/types/auth/auth.graphql	Added new Permission type and updated ApiKey and ApiKeyWithSecret types
api/src/graphql/schema/types/users/me.graphql	Updated permissions field in Me type to use Permission objects
api/src/graphql/schema/types/users/user.graphql	Added permissions field to UserAccount and User types
api/src/unraid-api/auth/api-key.service.ts	Refactored API key management with permission synchronization
api/src/unraid-api/auth/auth.service.ts	Introduced permission management functionality for API keys
api/src/unraid-api/graph/resolvers/me/me.resolver.ts	Added new MeResolver with user information and permissions
api/src/unraid-api/graph/resolvers/resolvers.module.ts	Added MeResolver to the ResolversModule
api/src/unraid-api/graph/sandbox-plugin.ts	Updated renderSandboxPage function to return landing page
api/src/unraid-api/auth/api-key.service.spec.ts	Updated tests to reflect new API key permissions structure
api/src/unraid-api/auth/auth.service.spec.ts	Updated tests to include permissions in mockApiKeyWithSecret
api/src/index.ts	Removed imports and method call related to local API key creation
api/vite.config.ts	Updated test setup configuration to include reflect-metadata
api/src/mothership/utils/create-local-connect-api-key.ts	Deleted function for creating local API key

Possibly related PRs

• feat: generate key one time #992: The changes in this PR involve modifications to API key roles, which are relevant to the role definitions and permissions removed in the main PR, specifically regarding the "connect" role.
• feat(api): graphql sandbox on unraid servers #1047: This PR introduces a GraphQL sandbox that may utilize the permissions structure defined in the main PR, particularly in how roles and permissions are managed within the GraphQL context.

Suggested reviewers

• elibosley

Poem

🐰 Permissions dance, a rabbit's delight,
Old roles fade, new freedoms take flight
API keys sing their flexible song
GraphQL whispers where they belong
A code ballet of access reborn! 🔑✨

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

🪧 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.
  • Generate unit testing code for this file.
  • 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. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• 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 src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

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

Documentation and Community

• 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.

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (10)

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

20-20: Consider using a Map for efficient API key lookups.

Changing memoryApiKeys from a Map to an Array may decrease the performance of lookup operations, especially as the number of API keys grows. Using a Map allows for constant-time complexity when retrieving keys by their ID or other unique fields.

Apply this diff to revert back to a Map:

-protected memoryApiKeys: Array<ApiKeyWithSecret> = [];
+protected memoryApiKeys: Map<string, ApiKeyWithSecret> = new Map();

Update the methods that access memoryApiKeys accordingly to work with the Map.

---

98-107: Improve error handling in loadAllFromDisk.

Currently, if an API key file has invalid JSON or does not conform to the schema, an error is logged, and the process continues. Consider aggregating these errors and providing a summary, or alerting administrators about corrupted or invalid files for further investigation.

---

127-133: Simplify the return statement in findById.

The method can be simplified by returning the parsed key directly if it exists, which improves readability.

Apply this diff to streamline the method:

findById(id: string): ApiKey | null {
    const key = this.findByField('id', id);

-   if (key) {
-       return ApiKeySchema().parse(key);
-   }
-   return null;
+   return key ? ApiKeySchema().parse(key) : null;
}

api/src/core/utils/permissions/ensure-permission.ts (1)

6-6: Provide guidance on migrating to Casbin authorization.

The deprecation notice mentions "Use casbin auth in nest instead," but without additional context or references. Consider providing a link to documentation or guidelines on how to migrate to Casbin for authorization within NestJS.

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

1-1: Remove unused OnModuleInit import.

The OnModuleInit interface is imported but not implemented by the CasbinService class.

-import { Injectable, InternalServerErrorException, Logger, OnModuleInit } from '@nestjs/common';
+import { Injectable, InternalServerErrorException, Logger } from '@nestjs/common';

api/src/unraid-api/graph/sandbox-plugin.ts (1)

67-68: Remove unnecessary empty line.

There's an extra empty line that doesn't add to code readability.

-    
    return serverListener.renderLandingPage();

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

32-32: Remove debugging console.log statement.

Production code should not contain console.log statements.

-            console.log('Permissions', apiKeyEntity)

---

220-220: Consider documenting the empty permissions array.

Add a comment explaining why the session user starts with empty permissions despite having the ADMIN role.

             roles: [Role.ADMIN],
-            permissions: [],
+            // Session users inherit permissions from their roles
+            permissions: [],

api/src/graphql/schema/types/auth/auth.graphql (1)

1-4: Consider using an enum for actions instead of string array.

The actions field using [String!] allows any string value, which could lead to inconsistency and typos. Consider defining an enum type for allowed actions to ensure type safety.

enum PermissionAction {
  CREATE
  READ
  UPDATE
  DELETE
  # Add other actions as needed
}

type Permission {
  resource: Resource!
  actions: [PermissionAction!]
}

api/src/graphql/schema/types/users/user.graphql (1)

6-6: Add documentation for the permissions field.

Consider adding descriptive documentation for the permissions field in both the interface and type definitions to improve schema clarity.

interface UserAccount {
+  """
+  List of explicit permissions granted to the account
+  """
  permissions: [Permission!]
}

type User implements UserAccount {
+  """
+  List of explicit permissions granted to the user
+  """
  permissions: [Permission!]
}

Also applies to: 65-65

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee2acb and 8f22648.

⛔ Files ignored due to path filters (2)

• api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
• api/src/graphql/generated/api/types.ts is excluded by !**/generated/**

📒 Files selected for processing (26)

• api/dev/Unraid.net/myservers.cfg (1 hunks)
• api/dev/data/permissions.json (0 hunks)
• api/package.json (1 hunks)
• api/src/__test__/core/permissions.test.ts (0 hunks)
• api/src/core/index.ts (0 hunks)
• api/src/core/modules/get-me.ts (0 hunks)
• api/src/core/modules/get-permissions.ts (0 hunks)
• api/src/core/modules/index.ts (0 hunks)
• api/src/core/permission-manager.ts (0 hunks)
• api/src/core/permissions.ts (0 hunks)
• api/src/core/utils/permissions/check-permission.ts (1 hunks)
• api/src/core/utils/permissions/ensure-permission.ts (1 hunks)
• api/src/core/utils/permissions/get-permissions.ts (0 hunks)
• api/src/graphql/schema/types/auth/auth.graphql (2 hunks)
• api/src/graphql/schema/types/users/me.graphql (1 hunks)
• api/src/graphql/schema/types/users/user.graphql (2 hunks)
• api/src/unraid-api/auth/api-key.service.ts (6 hunks)
• api/src/unraid-api/auth/auth.service.ts (5 hunks)
• api/src/unraid-api/auth/casbin/casbin.service.ts (2 hunks)
• api/src/unraid-api/auth/user.decorator.ts (1 hunks)
• api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts (0 hunks)
• api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts (0 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1 hunks)
• api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
• api/src/unraid-api/graph/sandbox-plugin.ts (1 hunks)

💤 Files with no reviewable changes (11)

• api/src/core/index.ts
• api/src/test/core/permissions.test.ts
• api/src/core/modules/get-me.ts
• api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts
• api/src/core/utils/permissions/get-permissions.ts
• api/src/core/modules/get-permissions.ts
• api/src/core/permission-manager.ts
• api/src/core/modules/index.ts
• api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts
• api/src/core/permissions.ts
• api/dev/data/permissions.json

🧰 Additional context used

🪛 Gitleaks (8.21.2)

api/dev/Unraid.net/myservers.cfg

11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

---

12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build Web App
• GitHub Check: Build and Test API
• GitHub Check: Cloudflare Pages

🔇 Additional comments (7)

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

89-89: Initialize permissions appropriately when creating a new API key.

The permissions array is initialized to an empty array, but there might be default permissions that should be assigned based on the roles or other criteria. Ensure that the permissions are set correctly for the new API key.

Do you want me to assist in defining default permissions for new API keys based on roles?

api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)

13-18: Verify permission configuration aligns with business requirements.

The current permission setup allows ANY possession level for reading ME resource. Consider if this aligns with your security requirements.

✅ Verification successful

Permission configuration is appropriate for the ME resource

The ANY possession level is correct for this endpoint because:

• This is a "current user" endpoint that only returns the authenticated user's own data
• The implementation uses @graphqluser() decorator which ensures the data is automatically scoped to the requesting user
• The Casbin policy confirms this is an intentional design choice

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other ME resource permission configurations
rg -l "resource: Resource.ME" | xargs rg "possession:"

Length of output: 94

---

Script:

#!/bin/bash
# Search for ME resource usage and implementation
echo "=== ME Resource References ==="
rg "Resource.ME" -B 2 -A 2

echo -e "\n=== ME Resolver Implementation ==="
ast-grep --pattern 'class MeResolver {
  $$$
}'

Length of output: 2516

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

8-8: LGTM! Good refactor moving enforcer to class property.

The refactor to use a class property for the enforcer instance improves state management and reusability.

Also applies to: 20-23

api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)

22-22: LGTM! Clean integration of MeResolver.

The MeResolver is properly imported and added to the providers array, following the module's existing pattern.

Also applies to: 44-44

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

34-34: Verify permission synchronization order.

The code synchronizes roles before permissions. Ensure this order is intentional and doesn't create race conditions or permission gaps.

Also applies to: 46-46

✅ Verification successful

The permission synchronization order is correct and beneficial.

The current order (roles then permissions) is intentional and provides these advantages:

• Ensures role-granted permissions are established first
• Prevents race conditions through sequential await operations
• Maintains permission consistency as custom permissions are added last

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any role-dependent permission logic in the codebase
ast-grep --pattern 'if ($user.roles.$$$) {
  $$$
  addPermission($$$)
  $$$
}'

Length of output: 80

---

Script:

#!/bin/bash
# Find syncApiKeyPermissions implementation
ast-grep --pattern 'syncApiKeyPermissions($$$) {
  $$$
}'

# Look for role-related sync methods and their usage
rg "sync.*Role|role.*Sync" -A 5

# Get more context around the current usage
rg "syncApiKeyPermissions" -A 5 -B 5

Length of output: 10100

api/src/graphql/schema/types/users/me.graphql (1)

16-16: LGTM! Consistent with UserAccount interface.

The nullable permissions field aligns with the interface definition and maintains type consistency.

api/package.json (1)

28-28: LGTM! Consistent with existing container management scripts.

The new script follows the established pattern and provides a useful container management command.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)

35-58: 🛠️ Refactor suggestion

Expand test coverage with additional scenarios.

While the basic happy path is covered, several critical test scenarios are missing:

• Permission decorator validation
• Error handling scenarios
• Required field validation
• Edge cases with missing or invalid user data

This was previously mentioned in a past review. Please add the suggested test cases to ensure robust coverage.

🧹 Nitpick comments (5)

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

94-94: Consider initializing default permissions based on roles.

Currently, new API keys are created with an empty permissions array. Consider deriving default permissions from the assigned roles to ensure proper access control from creation.

---

103-134: Consider enhancing error logging for better debugging.

While the error handling is robust, consider adding more detailed logging:

• Log the total number of files processed
• Log successful API key loads
• Include file paths in error messages

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

35-40: Consider adding more test cases for permissions.

The mock data only includes a single permission. Consider adding test cases for:

• Multiple permissions
• Different resources and actions
• Edge cases like empty permissions array

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

56-61: Enhance test coverage for permissions-related functionality.

Consider adding test cases for:

• Validation of permissions structure
• Error handling for invalid permissions
• Persistence of permissions during API key updates

api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)

10-59: Improve test suite structure and organization.

Consider restructuring the test suite to group related test cases:

1. Add a describe block for successful scenarios
2. Add a describe block for error scenarios
3. Add a describe block for permission validation

Example structure:

describe('MeResolver', () => {
  // Setup code...

  describe('Successful scenarios', () => {
    it('should return user information');
    it('should handle minimal user data');
  });

  describe('Error scenarios', () => {
    it('should handle missing user data');
    it('should handle invalid permissions');
  });

  describe('Permission validation', () => {
    it('should deny access without proper permissions');
    it('should allow access with proper permissions');
  });
});

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 8f22648 and 5f7ae01.

📒 Files selected for processing (4)

• api/src/unraid-api/auth/api-key.service.spec.ts (8 hunks)
• api/src/unraid-api/auth/api-key.service.ts (5 hunks)
• api/src/unraid-api/auth/auth.service.spec.ts (2 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build Web App
• GitHub Check: Build and Test API
• GitHub Check: Cloudflare Pages

🔇 Additional comments (6)

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

17-27: Review initialization order to prevent race conditions.

The setupWatch is called in the constructor before memoryApiKeys is populated, which could lead to inconsistent state if file changes occur before onModuleInit completes.

---

29-36: LGTM! Well-structured initialization with proper error handling.

The implementation correctly loads API keys from disk and includes appropriate error handling and logging.

---

42-47: Review the file watcher implementation for potential issues.

The implementation has two potential issues:

1. The event handler parameters for the change event should be (path) => { ... } rather than (event, path) => { ... }.
2. Clearing memoryApiKeys before reloading may result in temporary empty state.

---

137-151: LGTM! Well-structured implementation with proper error handling.

The method effectively uses findByField for consistency and includes appropriate error handling and schema validation.

---

Line range hint 158-170: Validate the field parameter to prevent invalid lookups.

There's no validation for the field parameter, which could lead to undefined behavior if an incorrect field name is provided.

---

221-224: LGTM! Correct file path handling.

The implementation now correctly uses join for file path construction and includes proper error handling.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 8309253 and e48fe00.

⛔ Files ignored due to path filters (1)

• api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**

📒 Files selected for processing (2)

• api/src/graphql/schema/types/auth/auth.graphql (2 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• api/src/graphql/schema/types/auth/auth.graphql

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build Web App
• GitHub Check: Build and Test API
• GitHub Check: Cloudflare Pages

🔇 Additional comments (2)

api/src/unraid-api/graph/resolvers/me/me.resolver.ts (2)

1-7: LGTM! Well-organized imports.

The imports are clean, necessary, and follow NestJS best practices with proper use of path aliases.

---

12-26: Add input validation and error handling.

The implementation still lacks validation for user properties.

Consider implementing field-level permissions.

The current implementation uses a single permission check for all fields. Consider implementing field-level permissions for sensitive data like roles and permissions.

Example implementation:

@ResolveField()
@UsePermissions({
  action: AuthActionVerb.READ,
  resource: Resource.ROLES,
  possession: AuthPossession.ANY,
})
public async roles() {
  // Implementation
}

Simplify the return statement using object shorthand notation.

     public async me(@GraphqlUser() user: UserAccount): Promise<Me> {
+        const { description, permissions, id, name, roles } = user;
         return {
-            description: user.description,
-            permissions: user.permissions,
-            id: user.id,
-            name: user.name,
-            roles: user.roles,
+            description,
+            permissions,
+            id,
+            name,
+            roles,
         };
     }

[github-actions]

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/pr/1068/dynamix.unraid.net.staging.plg

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (2)

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

28-29: ⚠️ Potential issue

Move setupWatch initialization to onModuleInit.

The file watcher is being set up before memoryApiKeys is populated, which could lead to race conditions. The watcher might receive events before the initial data is loaded.

Apply this diff to fix the initialization order:

constructor() {
    this.basePath = getters.paths()['auth-keys'];
    ensureDirSync(this.basePath);
-   this.setupWatch();
}

async onModuleInit() {
    try {
        this.memoryApiKeys = await this.loadAllFromDisk();
        await this.createLocalApiKeyForConnectIfNecessary();
+       this.setupWatch();
    } catch (error) {
        this.logger.error('Failed to initialize API keys:', error);
        throw error;
    }
}

---

46-50: ⚠️ Potential issue

Fix file watcher implementation issues.

The current implementation has two issues:

1. Incorrect event handler parameters for chokidar's 'change' event
2. Race condition when updating memoryApiKeys

Apply this diff to fix both issues:

-watch(this.basePath, { ignoreInitial: false }).on('change', async (event, path) => {
-    this.logger.debug(`API key storage event: ${event} on ${path}`);
-    this.memoryApiKeys = [];
-    this.memoryApiKeys = await this.loadAllFromDisk();
+watch(this.basePath, { ignoreInitial: false }).on('change', async (path) => {
+    this.logger.debug(`API key changed: ${path}`);
+    const updatedApiKeys = await this.loadAllFromDisk();
+    this.memoryApiKeys = updatedApiKeys;
});

🧹 Nitpick comments (4)

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

97-97: Add type assertion for permissions array initialization.

Consider adding a type assertion to ensure type safety of the permissions array.

-apiKey.permissions = [];
+apiKey.permissions = [] as Array<{ resource: string; actions: string[] }>;

---

106-142: Enhance logging for Connect API key creation process.

While the implementation is solid, adding more detailed logging would help with debugging and monitoring.

 if (remote.apikey && (!remote.localApiKey || !(await this.findByKey(remote.localApiKey)))) {
+    this.logger.debug('Checking for existing Connect API key...');
     const hasExistingKey = this.findByField('name', 'Connect');

     if (hasExistingKey) {
+        this.logger.debug('Connect API key already exists');
         return;
     }
+    this.logger.debug('Creating new Connect API key...');

---

157-174: Enhance error messages with actionable information.

The error messages could be more specific about what action users or administrators should take.

-throw new Error('Authentication system error: Corrupted key file');
+throw new Error('Authentication system error: Corrupted key file. Please check the file format and restore from backup if necessary.');

-throw new Error('Invalid API key structure');
+throw new Error('Invalid API key structure. Please verify the key file matches the expected schema.');

-throw new Error('Failed to list API keys');
+throw new Error('Failed to list API keys. Please check directory permissions and ensure the auth-keys directory exists.');

---

178-191: Add input validation for ID parameter.

Consider adding validation for the ID parameter to ensure it's a valid UUID.

 findById(id: string): ApiKey | null {
+    if (!id || !/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id)) {
+        throw new GraphQLError('Invalid API key ID format');
+    }
     try {
         const key = this.findByField('id', id);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between df548f3 and 9c4b340.

📒 Files selected for processing (3)

• api/src/index.ts (1 hunks)
• api/src/mothership/utils/create-local-connect-api-key.ts (0 hunks)
• api/src/unraid-api/auth/api-key.service.ts (4 hunks)

💤 Files with no reviewable changes (1)

• api/src/mothership/utils/create-local-connect-api-key.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• api/src/index.ts

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build Web App
• GitHub Check: Build and Test API
• GitHub Check: Cloudflare Pages

🔇 Additional comments (2)

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

31-39: Well-structured initialization with proper error handling!

The implementation includes comprehensive error handling and logging, ensuring robust initialization of the API key service.

---

262-265: Correctly implemented file path handling!

The implementation now properly uses join for file path construction and includes proper formatting for the JSON output.

[github-actions]

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/pr/1068/dynamix.unraid.net.staging.plg

[github-actions]

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/pr/1068/dynamix.unraid.net.staging.plg

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)

10-12: 🛠️ Refactor suggestion

Remove unnecessary constructor and enhance Resolver type information.

The empty constructor serves no purpose and should be removed. Also, the @Resolver() decorator should specify the type it resolves.

-@Resolver()
+@Resolver('Me')
 export class MeResolver {
-    constructor() {}

🧰 Tools

🪛 Biome (1.9.4)

[error] 12-12: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)

🧹 Nitpick comments (1)

api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)

14-14: Add return type to Query decorator.

The @Query() decorator should specify its return type for better type safety and documentation.

-    @Query()
+    @Query(() => Me)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f1a012c and 5205515.

📒 Files selected for processing (2)

• api/src/index.ts (0 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1 hunks)

💤 Files with no reviewable changes (1)

• api/src/index.ts

🧰 Additional context used

🪛 Biome (1.9.4)

api/src/unraid-api/graph/resolvers/me/me.resolver.ts

[error] 12-12: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Cloudflare Pages

🔇 Additional comments (1)

api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)

20-34: LGTM! Input validation and error handling look good.

The implementation includes proper input validation, error handling, and default values for optional fields.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (6)

api/src/utils.ts (1)

247-261: LGTM! Consider enhancing type validation.

The type predicate implementation is correct and follows TypeScript best practices. However, consider strengthening the validation for more robust type safety:

 export function isUserAccount(user: any): user is UserAccount {
     return (
         user &&
-        typeof user.id === 'string' &&
-        typeof user.name === 'string' &&
-        Array.isArray(user.permissions) &&
-        Array.isArray(user.roles)
+        typeof user.id === 'string' && user.id.length > 0 &&
+        typeof user.name === 'string' && user.name.length > 0 &&
+        Array.isArray(user.permissions) && user.permissions.every(p => typeof p === 'string') &&
+        Array.isArray(user.roles) && user.roles.every(r => typeof r === 'string')
     );
 }

This enhancement:

• Ensures non-empty strings for id and name
• Validates that array elements are strings

api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)

37-39: Expand the basic test coverage.

While checking if the resolver is defined is a good start, consider adding more basic validation tests:

1. Verify resolver instance type
2. Verify AuthZService injection

Example tests to add:

it('should be instance of MeResolver', () => {
  expect(resolver).toBeInstanceOf(MeResolver);
});

it('should have AuthZService injected', () => {
  expect(authzService).toBeDefined();
  expect(authzService.checkPermission).toBeDefined();
});

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

32-32: Remove debug console.log statement.

Production code should use the logger service instead of console.log for consistency and proper log level management.

-            console.log('Permissions', apiKeyEntity);
+            this.logger.debug('API Key permissions:', apiKeyEntity);

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

22-22: Consider using Map for better lookup performance.

Changing memoryApiKeys from Map to Array might impact performance for key lookups. Using Array.find() has O(n) complexity, while Map.get() has O(1).

-    protected memoryApiKeys: Array<ApiKeyWithSecret> = [];
+    protected memoryApiKeys: Map<string, ApiKeyWithSecret> = new Map();

---

35-37: Enhance error logging for initialization failures.

The current error logging could be more detailed to help with troubleshooting initialization issues.

-            this.logger.error('Failed to initialize API keys:', error);
+            this.logger.error('Failed to initialize API keys:', {
+                error: error instanceof Error ? error.message : String(error),
+                stack: error instanceof Error ? error.stack : undefined,
+                phase: 'initialization'
+            });

---

148-166: Optimize file loading for large directories.

Consider using Promise.all for parallel file reading to improve performance with many API keys.

-            for (const file of files) {
-                if (file.endsWith('.json')) {
-                    try {
-                        const content = await readFile(join(this.basePath, file), 'utf8');
-                        const apiKey = ApiKeyWithSecretSchema().parse(JSON.parse(content));
-                        apiKeys.push(apiKey);
-                    } catch (error) {
+            const readPromises = files
+                .filter(file => file.endsWith('.json'))
+                .map(async file => {
+                    try {
+                        const content = await readFile(join(this.basePath, file), 'utf8');
+                        return ApiKeyWithSecretSchema().parse(JSON.parse(content));
+                    } catch (error) {

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 5205515 and 58ce565.

📒 Files selected for processing (5)

• api/src/unraid-api/auth/api-key.service.ts (4 hunks)
• api/src/unraid-api/auth/auth.service.ts (5 hunks)
• api/src/unraid-api/auth/user.decorator.ts (1 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1 hunks)
• api/src/utils.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• api/src/unraid-api/auth/user.decorator.ts

🧰 Additional context used

🪛 Biome (1.9.4)

api/src/unraid-api/auth/api-key.service.ts

[error] 200-200: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build Web App
• GitHub Check: Build and Test API
• GitHub Check: Cloudflare Pages

🔇 Additional comments (5)

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

100-114: Implementation looks good!

The syncApiKeyPermissions method follows best practices with proper permission cleanup, parallel execution, and error handling.

---

226-226: ⚠️ Potential issue

Verify session user permissions.

The session user has the ADMIN role but returns an empty permissions array. This seems inconsistent with the method's comment that states "Session receives administrator permissions". This could lead to unexpected authorization issues.

Consider either:

1. Syncing permissions based on the ADMIN role, or
2. Updating the method's documentation to clarify the permission model

-            permissions: [],
+            // TODO: Sync these permissions with ADMIN role capabilities
+            permissions: [
+                {
+                    resource: '*',
+                    actions: ['*']
+                }
+            ],

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

48-49: Avoid clearing memoryApiKeys before reloading to prevent race conditions.

Clearing memoryApiKeys before reloading may result in temporary empty state.

---

266-269: LGTM! Proper file path handling implemented.

The implementation correctly uses path.join and proper JSON formatting.

---

200-201: 🛠️ Refactor suggestion

Use Object.hasOwn instead of hasOwnProperty.

Replace Object.prototype.hasOwnProperty with Object.hasOwn for better safety and maintainability.

-        if (!ApiKeyWithSecretSchema().shape.hasOwnProperty(field)) {
+        if (!Object.hasOwn(ApiKeyWithSecretSchema().shape, field)) {

Likely invalid or redundant comment.

🧰 Tools

🪛 Biome (1.9.4)

[error] 200-200: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (3)

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

26-28: ⚠️ Potential issue

Set secure file permissions for API key storage.

The API key storage directory needs proper file permissions to prevent unauthorized access.

Apply this diff to set secure file permissions:

 constructor() {
     this.basePath = getters.paths()['auth-keys'];
     ensureDirSync(this.basePath);
+    chmodSync(this.basePath, 0o700);
 }

Ensure you've imported the necessary function:

import { chmodSync } from 'fs';

---

45-51: ⚠️ Potential issue

Fix race condition in setupWatch and add cleanup.

The current implementation has two issues:

1. Race condition when clearing and reloading memoryApiKeys
2. No cleanup of the watcher when the service is destroyed

Apply this diff to fix these issues:

+private watcher: ReturnType<typeof watch> | null = null;
+
 private setupWatch() {
-    watch(this.basePath, { ignoreInitial: false }).on('change', async (path) => {
+    this.watcher = watch(this.basePath, { ignoreInitial: false });
+    this.watcher.on('change', async (path) => {
         this.logger.debug(`API key changed: ${path}`);
-        this.memoryApiKeys = [];
-        this.memoryApiKeys = await this.loadAllFromDisk();
+        const updatedKeys = await this.loadAllFromDisk();
+        this.memoryApiKeys = updatedKeys;
     });
 }
+
+async onModuleDestroy() {
+    if (this.watcher) {
+        await this.watcher.close();
+        this.watcher = null;
+    }
+}

Don't forget to implement OnModuleDestroy:

-export class ApiKeyService implements OnModuleInit {
+export class ApiKeyService implements OnModuleInit, OnModuleDestroy {

---

106-142: ⚠️ Potential issue

Implement atomic operations for Connect API key creation.

The current implementation has potential race conditions and non-atomic operations when creating and updating the Connect API key.

Apply this diff to implement atomic operations:

 private async createLocalApiKeyForConnectIfNecessary() {
     if (getters.config().status !== FileLoadStatus.LOADED) {
         this.logger.error('Config file not loaded, cannot create local API key');
         return;
     }

     const { remote } = getters.config();
-    if (remote.apikey && (!remote.localApiKey || !(await this.findByKey(remote.localApiKey)))) {
-        const hasExistingKey = this.findByField('name', 'Connect');
-
-        if (hasExistingKey) {
-            return;
-        }
+    if (!remote.apikey || (remote.localApiKey && await this.findByKey(remote.localApiKey))) {
+        return;
+    }
+
+    const existingKey = this.findByField('name', 'Connect');
+    if (existingKey) {
+        return;
+    }

     try {
+        // Start atomic operation
         const localApiKey = await this.create(
             'Connect',
             'API key for Connect user',
             [Role.CONNECT],
             true
         );

         if (!localApiKey?.key) {
             throw new Error('Failed to create local API key - no key returned');
         }

-        if (localApiKey?.key) {
-            store.dispatch(
-                updateUserConfig({
-                    remote: {
-                        localApiKey: localApiKey.key,
-                    },
-                })
-            );
-        } else {
-            this.logger.error('Failed to create local API key - no key returned');
-            throw new Error('Failed to create local API key');
-        }
+        await store.dispatch(
+            updateUserConfig({
+                remote: {
+                    localApiKey: localApiKey.key,
+                },
+            })
+        );
+    } catch (error) {
+        // Rollback on failure
+        if (localApiKey?.id) {
+            await this.deleteApiKey(localApiKey.id);
+        }
+        throw error;
     }
 }

🧹 Nitpick comments (5)

api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (4)

21-28: Enhance type safety and verification in AuthZService mock.

The current mock implementation could be improved for better type safety and verification:

 checkPermission: vi.fn().mockImplementation((action, resource, possession) => {
-    // Return false for specific test scenarios
-    if (action === 'write' && resource === Resource.ME) {
-        return Promise.resolve(false);
-    }
-    return Promise.resolve(true);
+    // Verify possession parameter
+    if (!['own', 'any'].includes(possession)) {
+        throw new Error(`Invalid possession: ${possession}`);
+    }
+    
+    // Type-safe resource check
+    if (!Object.values(Resource).includes(resource)) {
+        throw new Error(`Invalid resource: ${resource}`);
+    }
+    
+    // Type-safe action check
+    const validActions = ['read', 'write', 'delete'];
+    if (!validActions.includes(action)) {
+        throw new Error(`Invalid action: ${action}`);
+    }
+    
+    return Promise.resolve(action !== 'write' || resource !== Resource.ME);
 }),

---

37-39: Enhance basic resolver verification.

While checking if the resolver is defined is good, consider adding more basic verification:

 it('should be defined', () => {
     expect(resolver).toBeDefined();
+    expect(resolver).toBeInstanceOf(MeResolver);
+    expect(resolver.me).toBeInstanceOf(Function);
+    expect(authzService).toBeDefined();
+    expect(authzService.checkPermission).toBeInstanceOf(Function);
 });

---

57-64: Remove redundant assertions.

The block-level equality check is redundant with the individual property checks that follow:

-        expect(result).toEqual({
-            id: mockUser.id,
-            name: mockUser.name,
-            description: mockUser.description,
-            roles: mockUser.roles,
-            permissions: mockUser.permissions,
-        } as Me);
-        expect(result).toBeDefined();
+        expect(result).toBeDefined();

---

69-73: Avoid non-null assertion operator and improve type safety.

Replace non-null assertions with proper type checking:

-        expect(result.permissions!).toHaveLength(1);
-        expect(result.permissions![0]).toEqual({
+        expect(result.permissions).toBeDefined();
+        expect(result.permissions).toHaveLength(1);
+        
+        const [permission] = result.permissions ?? [];
+        expect(permission).toBeDefined();
+        expect(permission).toEqual({
             resource: Resource.ME,
             actions: expect.arrayContaining(['read']),
         });

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

144-176: Improve file handling in loadAllFromDisk.

The current implementation could be optimized to handle file resources better and provide more detailed error messages.

Apply this diff to improve file handling:

 async loadAllFromDisk(): Promise<ApiKeyWithSecret[]> {
+    const apiKeys: ApiKeyWithSecret[] = [];
     try {
         const files = await readdir(this.basePath);
-        const apiKeys: ApiKeyWithSecret[] = [];
 
         for (const file of files) {
             if (file.endsWith('.json')) {
+                const filePath = join(this.basePath, file);
                 try {
-                    const content = await readFile(join(this.basePath, file), 'utf8');
+                    const content = await readFile(filePath, 'utf8');
                     const apiKey = ApiKeyWithSecretSchema().parse(JSON.parse(content));
 
                     apiKeys.push(apiKey);
                 } catch (error) {
                     if (error instanceof SyntaxError) {
-                        throw new Error('Authentication system error: Corrupted key file');
+                        throw new Error(`Authentication system error: Corrupted key file ${file}`);
                     }
                     if (error instanceof ZodError) {
                         this.logger.error(`Invalid API key structure in file ${file}`, error.errors);
-                        throw new Error('Invalid API key structure');
+                        throw new Error(`Invalid API key structure in file ${file}`);
                     }
-                    this.logger.warn(`Error reading API key file ${file}: ${error}`);
+                    const errorMessage = error instanceof Error ? error.message : String(error);
+                    this.logger.warn(`Error reading API key file ${file}: ${errorMessage}`);
                 }
             }
         }
-        return apiKeys;
     } catch (error) {
         if (error instanceof Error) {
             throw error;
         }
         this.logger.error(`Failed to read API key directory: ${error}`);
         throw new Error('Failed to list API keys');
     }
+    return apiKeys;
 }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 58ce565 and 9311000.

📒 Files selected for processing (2)

• api/src/unraid-api/auth/api-key.service.ts (4 hunks)
• api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (3)

• GitHub Check: Build and Test API
• GitHub Check: Build Web App
• GitHub Check: Cloudflare Pages

🔇 Additional comments (1)

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

276-279: ⚠️ Potential issue

Set secure file permissions when saving API keys.

API key files should have restricted permissions to prevent unauthorized access.

Apply this diff to set secure file permissions:

 await writeFile(
     join(this.basePath, `${validatedApiKey.id}.json`),
     JSON.stringify(sortedApiKey, null, 2)
 );
+await chmodSync(join(this.basePath, `${validatedApiKey.id}.json`), 0o600);

Likely invalid or redundant comment.

[github-actions]

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/pr/1068/dynamix.unraid.net.staging.plg