Skip to content

Conversation

@mdatelle
Copy link
Contributor

@mdatelle mdatelle commented Oct 29, 2024

This PR implements a new API key management system with role-based access control (RBAC) using Casbin and nest-authz. The system allows creating, querying, and managing API keys with associated roles and permissions.

I'll keep this in draft mode for now until we discus how we want to handle key generation and migration to the new system. This is built around the existing authorization system so it continues to work until we sunset it.

Notes:

  • The resolvers using the @UseRole decorator will need to be refactored to use the new @UsePermissions decorator. I can create a separate branch for that effort since there's a bunch of files that need the update.

Links:
https://casbin.org/docs/overview
https://github.com/node-casbin/nest-authz

Summary by CodeRabbit

  • New Features

    • Introduced a new API key management system with enhanced role and permission management.
    • Added a GraphQL resolver for authentication operations, including API key creation and role assignments.
    • Implemented rate limiting for API requests via the Throttler module.
    • Enhanced API key validation and role management functionalities.
    • Added a new environment variable for API key storage path.
    • Integrated Casbin for improved authorization capabilities.
    • Introduced a custom throttler guard for Fastify.
    • Updated GraphQL schema to support multiple roles for users.
    • Added a new API key configuration file for structured management.
    • Enhanced CORS handling with improved type safety.
    • Reorganized and improved the Vite configuration for better module resolution and server behavior.
    • Expanded session cookie configuration for enhanced security.
    • Added a new JSON file for API key configuration with essential attributes.
    • Added a new CasbinModule for managing authorization enforcers.
    • Introduced CasbinService for initializing and managing Casbin authorization logic.
    • Added a listener for managing local API keys, enhancing user authentication processes.
    • Enhanced configuration structure to include localApiKey for improved user authentication.
    • Updated the API to handle local API key creation and management.
    • Improved error handling for API key validation and role management.
    • Enhanced the report functionality to improve API key validation handling.
  • Bug Fixes

    • Improved error handling for API key validation and role management.
  • Documentation

    • Updated documentation comments for clarity in GraphQL schema types.
  • Tests

    • Added comprehensive test suites for the new API key service and authentication resolver functionalities.
    • Expanded test coverage for the AuthService and AuthResolver functionalities.
    • Updated tests to reflect changes in roles and permissions management.
    • Added tests for new API key creation and permission assignments.
    • Updated tests to include the new localApiKey field in configuration state snapshots.

mdatelle added 3 commits October 28, 2024 17:36
- add nest-authz package
- create new resolver with mutations and queries for the new auth service
- create a casbin policy and model based on exsisting permissions
- create api key service methods
- add findOneByKey method to the user service
- add authz service methods to the auth services
- add AuthZModule config to the auth module
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2024

Walkthrough

The changes in this pull request introduce a variety of enhancements and modifications across multiple files within the project. Key updates include the addition of a new environment variable for authentication key paths, the introduction of a new API key configuration JSON file, and the integration of new dependencies for rate limiting and access control. Significant updates to the GraphQL schema and resolvers enhance permission management and role assignments. Additionally, a new service for managing API keys is implemented, alongside comprehensive test suites for various components.

Changes

File Change Summary
api/.env.development Added new variable: PATHS_AUTH_KEY=./dev/keys
api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json Created new JSON file for API key configuration with properties: id, key, name, description, roles, createdAt.
api/package.json Added dependencies: "@nestjs/throttler": "^6.2.1" and "casbin": "^5.32.0".
api/src/__test__/store/modules/paths.test.ts Updated test to include "auth-keys" in the expected output of Object.keys(paths).
api/src/graphql/schema/types/auth/auth.graphql Added enums: Resource, Role; types: ApiKey, ApiKeyWithSecret; input types for API key and permission management; mutations and queries for API key operations.
api/src/store/modules/paths.ts Added new property 'auth-keys' in initialState, sourced from PATHS_AUTH_KEY.
api/src/unraid-api/auth/api-key.service.spec.ts Introduced test suite for ApiKeyService with multiple tests for various functionalities.
api/src/unraid-api/auth/api-key.service.ts Added ApiKeyService class with methods for managing API keys, including creation, retrieval, and storage.
api/src/unraid-api/auth/auth.module.ts Updated AuthModule to include ApiKeyService and new imports for authorization management.
api/src/unraid-api/auth/auth.service.spec.ts Updated test suite for AuthService, transitioning to vitest, and expanded test coverage.
api/src/unraid-api/auth/auth.service.ts Enhanced AuthService with new methods for API key validation and role management.
api/src/unraid-api/auth/casbin/index.ts Exported all entities from model and policy modules.
api/src/unraid-api/auth/casbin/model.ts Added constant CASBIN_MODEL defining access control model structure.
api/src/unraid-api/auth/casbin/policy.ts Introduced BASE_POLICY constant defining access control policies.
api/src/unraid-api/auth/header.strategy.ts Updated ServerHeaderStrategy class for API key validation logic.
api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts Added unit tests for AuthResolver covering API key management and permission handling.
api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts Implemented AuthResolver for authentication operations with GraphQL.
api/src/unraid-api/graph/resolvers/resolvers.module.ts Updated ResolversModule to include AuthResolver and import AuthModule.
api/src/unraid-api/app/app.module.ts Added ThrottlerModule and ThrottlerGuard for rate limiting.
api/src/graphql/express/get-images.ts Removed deprecated image serving function.
api/src/graphql/schema/types/apikeys/apikey.graphql Deleted GraphQL schema definitions related to API keys.
api/src/graphql/schema/types/users/me.graphql Updated roles field in Me type to an array of strings.
api/src/graphql/schema/types/users/user.graphql Updated roles field in UserAccount interface and User type to an array of strings.
api/src/unraid-api/auth/cookie.strategy.ts Updated UserCookieStrategy class to use new cookie validation method.
api/src/unraid-api/auth/cookie.service.ts Expanded SessionCookieConfig to include secure, httpOnly, and sameSite properties.
api/src/unraid-api/auth/users.service.ts Removed UsersService class and its associated methods.
api/src/unraid-api/graph/connect/connect.resolver.ts Updated authorization from @UseRoles to @UsePermissions for connect query and enableDynamicRemoteAccess mutation.
api/src/unraid-api/graph/network/network.resolver.ts Updated authorization from @UseRoles to @UsePermissions for network query.
api/src/unraid-api/graph/resolvers/array/array.resolver.ts Updated authorization from @UseRoles to @UsePermissions for array query and subscription.
api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.ts Updated authorization from @UseRoles to @UsePermissions for cloud-related methods.
api/src/unraid-api/graph/resolvers/config/config.resolver.ts Updated authorization from @UseRoles to @UsePermissions for config query and setAdditionalAllowedOrigins mutation.
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts Updated authorization from @UseRoles to @UsePermissions for disks query.
api/src/unraid-api/graph/resolvers/display/display.resolver.ts Updated authorization from @UseRoles to @UsePermissions for display query and subscription.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts Updated authorization from @UseRoles to @UsePermissions for Docker-related methods.
api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts Updated authorization from @UseRoles to @UsePermissions for flash query.
api/src/unraid-api/graph/resolvers/info/info.resolver.ts Updated authorization from @UseRoles to @UsePermissions for info query.
api/src/unraid-api/graph/resolvers/online/online.resolver.ts Updated authorization from @UseRoles to @UsePermissions for online query.
api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts Updated authorization from @UseRoles to @UsePermissions for owner query and subscription.
api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts Updated authorization from @UseRoles to @UsePermissions for registration-related methods.
api/src/unraid-api/graph/resolvers/servers/server.resolver.ts Updated authorization from @UseRoles to @UsePermissions for server-related methods.
api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts Updated authorization from @UseRoles to @UsePermissions for vars query.
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts Updated authorization from @UseRoles to @UsePermissions for VMs-related methods.
api/src/unraid-api/graph/services/services.resolver.ts Updated authorization from @UseRoles to @UsePermissions for services query.
api/src/unraid-api/graph/shares/shares.resolver.ts Updated authorization from @UseRoles to @UsePermissions for shares query.
api/src/utils.ts Added utility functions: getRequest and handleAuthError for context-aware request handling and standardized error handling.
api/src/unraid-api/auth/casbin/casbin.module.ts Introduced new CasbinModule for managing Casbin authorization.
api/src/unraid-api/auth/casbin/casbin.service.ts Added CasbinService for managing Casbin enforcers and initializing authorization.
api/src/cli/commands/report.ts Updated report generation logic and error handling for API keys.
api/src/core/utils/files/config-file-normalizer.ts Added localApiKey property to remote configuration in getWriteableConfig function.
api/src/graphql/client/api/get-api-client.ts Renamed parameter from upcApiKey to localApiKey in getApiApolloClient function.
api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts Updated logic for connecting sign-in to include local API key creation.
api/src/graphql/resolvers/subscription/remote-graphql/remote-query.ts Updated API key handling to use localApiKey.
api/src/store/actions/add-remote-subscription.ts Adjusted API key retrieval logic to use localApiKey.
api/src/store/listeners/local-api-key-listener.ts Introduced listener for managing local API keys.
api/src/store/modules/config.ts Enhanced remote configuration structure to include localApiKey.
api/src/types/my-servers-config.d.ts Updated MyServersConfig interface to include localApiKey in remote property.
api/src/__test__/core/utils/files/config-file-normalizer.test.ts Updated test cases to include localApiKey in expected configuration objects.
api/src/__test__/store/modules/config.test.ts Updated expected state snapshot to include localApiKey.

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. This feature will be included in our new Pro Plan when released, so please consider upgrading your current plan.


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

@mdatelle mdatelle requested review from elibosley and pujitm October 29, 2024 21:17
@mdatelle mdatelle marked this pull request as ready for review November 1, 2024 18:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 24

🧹 Outside diff range and nitpick comments (21)
api/src/unraid-api/auth/casbin/model.ts (2)

1-16: Document the Casbin model structure and its components.

The model implements RBAC with hierarchical resource matching, but lacks documentation explaining the purpose and usage of each component.

Add JSDoc comments explaining:

+/**
+ * Casbin authorization model for API key management.
+ * 
+ * Structure:
+ * - Request/Policy: (subject, object, action, possession)
+ * - Roles: Supports role inheritance
+ * - Effect: Allows if any matching rule allows
+ * - Matcher: Supports hierarchical resources and wildcards
+ */
 export const CASBIN_MODEL = `

11-12: Consider adding explicit deny rules.

The current policy effect only considers allow rules (p.eft == allow). This might be too permissive as it lacks explicit deny capabilities.

Consider using:

-e = some(where (p.eft == allow))
+e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
api/src/unraid-api/auth/header.strategy.ts (1)

12-14: Consider adding type safety for AuthService.

While the constructor simplification is good, consider adding explicit type annotation for better type safety.

-    constructor(private authService: AuthService) {
+    constructor(private readonly authService: AuthService) {
api/.env.development (1)

3-3: Add documentation about the keys directory structure.

Consider adding a comment or documentation that explains:

  • The expected directory structure
  • The format of key files
  • Required permissions
  • Backup considerations

Example:

-PATHS_AUTH_KEY=./dev/keys # Auth key directory
+PATHS_AUTH_KEY=./dev/keys # Auth key directory - Stores API key JSON files with 600 permissions
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)

Line range hint 22-42: Consider organizing providers alphabetically

The providers array is mostly alphabetically ordered, but the newly added AuthResolver breaks this pattern. While this is a minor issue, maintaining consistent ordering can improve code maintainability.

    providers: [
        ArrayResolver,
-       AuthResolver,
        CloudResolver,
        ConfigResolver,
        DisksResolver,
        DisplayResolver,
        DockerResolver,
        FlashResolver,
        InfoResolver,
        NotificationsResolver,
        OnlineResolver,
        OwnerResolver,
        RegistrationResolver,
        ServerResolver,
        VarsResolver,
        VmsResolver,
        NotificationsService,
    ],
api/src/unraid-api/auth/casbin/policy.ts (2)

61-65: Consider reversing the role inheritance hierarchy.

The current inheritance model has higher-privileged roles inheriting from guest. This is typically reversed in RBAC systems where lower-privileged roles inherit from higher ones. This ensures proper permission escalation and makes it easier to manage role capabilities.

Consider restructuring the inheritance as:

-g, admin, guest
-g, upc, guest
-g, my_servers, guest
-g, notifier, guest
+g, guest, notifier
+g, notifier, my_servers
+g, my_servers, upc
+g, upc, admin

6-31: Standardize permission naming conventions.

Some resource names use hyphens (crash-reporting-enabled) while others use slashes (connect/dynamic-remote-access). This inconsistency could lead to maintenance issues.

Consider adopting a consistent naming convention, preferably using slashes for hierarchy and camelCase or kebab-case consistently:

-p, upc, crash-reporting-enabled, read, any
+p, upc, system/crash-reporting, read, any
api/src/unraid-api/users/users.service.ts (1)

Line range hint 66-80: Review and update getSessionUser implementation

The getSessionUser method returns a hardcoded UPC service account. This seems inconsistent with its documented purpose of representing a webgui user session.

Consider implementing proper session user representation instead of reusing the UPC service account.

api/src/store/modules/paths.ts (1)

65-67: Consider adding directory permissions check.

Since this directory will store sensitive API keys, ensure that appropriate directory permissions are set during creation.

Consider implementing a startup check to verify and set correct permissions (e.g., 600) on the keys directory. This is especially important since the path is configurable via environment variables.

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

20-31: Use obvious test values for mock data.

While the current UUIDs appear to be test data, it's better to use clearly fake values (e.g., 'test-id-1', 'test-key-1') to:

  • Make it immediately obvious these are test values
  • Prevent accidental copy-paste of UUID-like values into production
  • Avoid triggering security scanners
 const mockApiKey: ApiKey = {
     __typename: 'ApiKey',
-    id: '10f356da-1e9e-43b8-9028-a26a645539a6',
-    key: '73717ca0-8c15-40b9-bcca-8d85656d1438',
+    id: 'test-id-1',
+    key: 'test-key-1',
     name: 'Test API Key',
     description: 'Test API Key Description',
🧰 Tools
🪛 Gitleaks

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

(generic-api-key)


45-168: Consider adding more error and edge cases.

While the current test coverage is good, consider adding these scenarios:

  1. For createApiKey:
    • Invalid role names
    • Empty role array
    • Duplicate roles
  2. For role management methods:
    • Adding already existing roles
    • Removing non-existent roles
  3. For addPermission:
    • Invalid resource/action combinations
    • Permission conflicts

Would you like me to provide example test cases for these scenarios?

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

16-27: Consider using obvious mock values for sensitive fields.

While using UUIDs for mock data is realistic, it could be mistaken for real API keys in logs or documentation. Consider using clearly fake values like 'test-id-123' and 'test-key-456' to prevent any confusion.

 const mockApiKey: ApiKey = {
     __typename: 'ApiKey',
-    id: '10f356da-1e9e-43b8-9028-a26a645539a6',
-    key: '73717ca0-8c15-40b9-bcca-8d85656d1438',
+    id: 'test-id-123',
+    key: 'test-key-456',
     name: 'Test API Key',
🧰 Tools
🪛 Gitleaks

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

(generic-api-key)

api/package.json (2)

88-88: Consider using the latest version of Casbin.

The current version specification "^5.32.0" is several minor versions behind the latest stable release (v5.44.1). While the caret allows for minor version updates, explicitly using the latest version ensures you have all security patches and performance improvements.

-    "casbin": "^5.32.0",
+    "casbin": "^5.44.1",

88-88: Monitor bundle size impact of multiple auth libraries.

Having multiple authorization libraries (nest-access-control, nest-authz, and now casbin) could increase the bundle size. Consider:

  1. Analyzing the impact on build size
  2. Planning for removal of unused auth libraries post-migration
  3. Tree-shaking opportunities to minimize impact

Consider implementing a facade pattern to abstract the authorization interface, making it easier to switch between implementations during the migration phase.

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

2-2: Suggestion: Use the ID scalar type for the id field

For clarity and consistency, consider using the ID scalar type for the id field in the ApiKey type. In GraphQL, the ID type is specifically designed for unique identifiers.

Apply this diff to update the id field type:

 type ApiKey {
-    id: String!
+    id: ID!
     key: String!
     name: String!
     description: String
     roles: [String!]!
     createdAt: DateTime!
     lastUsed: DateTime
 }

24-24: Suggestion: Use the ID scalar type for the userId field

In the AddRoleForUserInput, consider using the ID scalar type for the userId field to clearly indicate that it represents a unique identifier.

Apply this diff:

 input AddRoleForUserInput {
-    userId: String!
+    userId: ID!
     role: String!
 }

29-29: Suggestion: Use the ID scalar type for the apiKeyId field

For consistency, update the apiKeyId field in AddRoleForApiKeyInput to use the ID scalar type.

Apply this diff:

 input AddRoleForApiKeyInput {
-    apiKeyId: String!
+    apiKeyId: ID!
     role: String!
 }

34-34: Suggestion: Use the ID scalar type for the apiKeyId field

Similarly, in RemoveRoleFromApiKeyInput, consider using the ID scalar type for the apiKeyId field.

Apply this diff:

 input RemoveRoleFromApiKeyInput {
-    apiKeyId: String!
+    apiKeyId: ID!
     role: String!
 }
api/src/unraid-api/auth/api-key.service.ts (2)

72-101: Optimize API key lookup in findByKey

The findByKey method reads and parses every API key file to find a matching key. This approach may become a performance bottleneck as the number of API keys grows. Consider implementing an indexing mechanism or storing API keys in a database for more efficient lookups.

For example, you could:

  • Use a key-value store like Redis to store and retrieve API keys quickly.
  • Maintain an in-memory index or cache that maps keys to file paths.

Would you like assistance in designing an optimized solution for API key storage and retrieval?

🧰 Tools
🪛 Biome

[error] 90-90: Unnecessary continue statement

Unsafe fix: Delete the unnecessary continue statement

(lint/correctness/noUnnecessaryContinue)


103-116: Add file system permissions check when saving API keys

When writing the API key file in saveApiKey, consider verifying that the application has the necessary write permissions to the directory. If not, provide a clear error message to aid in troubleshooting.

Apply this diff to add a permissions check:

 public async saveApiKey(apiKey: ApiKey): Promise<void> {
+    try {
+        await fs.promises.access(this.paths().basePath, fs.constants.W_OK);
+    } catch (error) {
+        this.logger.error(`No write permission to API key directory: ${error}`);
+        throw new InternalServerErrorException('No write permission to API key directory.');
+    }
     try {
         await fs.promises.writeFile(
             this.paths().keyFile(apiKey.id),
             JSON.stringify(apiKey, null, 2)
         );
     } catch (error: unknown) {
         if (error instanceof Error) {
             throw new InternalServerErrorException(`Failed to save API key: ${error.message}`);
         } else {
             throw new InternalServerErrorException('Failed to save API key: Unknown error');
         }
     }
 }
api/src/unraid-api/auth/auth.service.ts (1)

132-138: Review the automatic assignment of 'guest' role

In ensureUserRoles, automatically adding a 'guest' role to users without roles may have security implications. It's important to explicitly manage user roles to prevent unauthorized access.

Consider requiring explicit role assignments during user creation or authentication processes instead of defaulting to 'guest'.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fc9bd8f and 1f81a0d.

⛔ Files ignored due to path filters (3)
  • api/package-lock.json is excluded by !**/package-lock.json
  • 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 (21)
  • api/.env.development (1 hunks)
  • api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json (1 hunks)
  • api/package.json (1 hunks)
  • api/src/__test__/store/modules/paths.test.ts (2 hunks)
  • api/src/graphql/schema/types/auth/auth.graphql (1 hunks)
  • api/src/store/modules/paths.ts (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/auth/auth.guard.ts (2 hunks)
  • api/src/unraid-api/auth/auth.module.ts (1 hunks)
  • api/src/unraid-api/auth/auth.service.spec.ts (1 hunks)
  • api/src/unraid-api/auth/auth.service.ts (1 hunks)
  • api/src/unraid-api/auth/casbin/index.ts (1 hunks)
  • api/src/unraid-api/auth/casbin/model.ts (1 hunks)
  • api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
  • api/src/unraid-api/auth/header.strategy.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts (1 hunks)
  • api/src/unraid-api/users/users.module.ts (1 hunks)
  • api/src/unraid-api/users/users.service.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • api/src/unraid-api/auth/casbin/index.ts
🧰 Additional context used
🪛 Gitleaks
api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json

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

(generic-api-key)

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

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

(generic-api-key)

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

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

(generic-api-key)

api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts

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

(generic-api-key)

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

[error] 90-90: Unnecessary continue statement

Unsafe fix: Delete the unnecessary continue statement

(lint/correctness/noUnnecessaryContinue)

🔇 Additional comments (31)
api/src/unraid-api/users/users.module.ts (2)

1-1: LGTM: Proper import of forwardRef.

The addition of forwardRef from @nestjs/common is correct for handling circular module dependencies.


6-6: Consider evaluating the necessity of circular dependency.

While the circular dependency resolution is implemented correctly, consider if this architectural pattern is optimal for your use case. Circular dependencies can make the codebase harder to maintain and test. Some alternatives to consider:

  1. Extract shared functionality into a separate module
  2. Use an event-based communication pattern
  3. Implement a facade service that coordinates between Auth and Users

This would improve maintainability and reduce coupling between modules.

Let's analyze the dependency structure:

✅ Verification successful

Let me analyze the service implementations to better understand the dependency relationship.


Let me try a different approach to examine the service dependencies.


The circular dependency is justified and properly implemented

The circular dependency between AuthModule and UsersModule is necessary due to the core authentication and user management functionality:

  • AuthService requires UsersService for user-related operations during authentication
  • UsersService requires ApiKeyService (from AuthModule) for API key to user conversion

The current implementation using forwardRef() is the correct approach as:

  1. The dependency is unavoidable due to the intrinsic relationship between authentication and user management
  2. The services have clear, distinct responsibilities
  3. The circular reference is properly managed at the module level
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Analyze module dependencies to understand the circular dependency

# Check imports in AuthModule
echo "Checking AuthModule dependencies:"
rg -A 5 "import.*from.*users" "api/src/unraid-api/auth/"

# Check imports in UsersModule
echo "Checking UsersModule dependencies:"
rg -A 5 "import.*from.*auth" "api/src/unraid-api/users/"

Length of output: 2520


Script:

#!/bin/bash
# Check AuthService implementation
echo "AuthService implementation:"
ast-grep --pattern 'export class AuthService {
  $$$
}'

# Check UsersService implementation
echo "UsersService implementation:"
ast-grep --pattern 'export class UsersService {
  $$$
}'

Length of output: 248


Script:

#!/bin/bash
# Check how AuthService uses UsersService
echo "AuthService using UsersService:"
rg -B 2 -A 5 "UsersService" "api/src/unraid-api/auth/auth.service.ts"

# Check how UsersService uses ApiKeyService
echo "UsersService using ApiKeyService:"
rg -B 2 -A 5 "ApiKeyService" "api/src/unraid-api/users/users.service.ts"

Length of output: 1307

api/src/unraid-api/auth/casbin/model.ts (2)

9-9: Verify role inheritance depth limits.

The role definition g = _, _ allows unlimited role inheritance depth, which could impact performance with deep role hierarchies.

Let's check for potential deep role hierarchies:

✅ Verification successful

These scripts will help us understand:

  1. The actual role/permission implementation context
  2. Where policies are defined
  3. The actual role hierarchies in policy files

Role inheritance implementation is safe and controlled

Based on the policy definitions found in the codebase, the role inheritance is well-structured and shallow:

  • Only one level of inheritance where all roles (admin, upc, my_servers, notifier) inherit from guest
  • No deep hierarchies or circular dependencies
  • Clear and explicit permission assignments for each role

The g = _, _ definition in the model is standard Casbin syntax and the actual implementation shows no performance concerns regarding role hierarchy depth.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for role definitions to analyze hierarchy depth
ast-grep --pattern 'g($role1, $role2)' 

Length of output: 40


Script:

#!/bin/bash
# Let's search for role-related patterns in the codebase
rg -i "role|permission" api/src/unraid-api/auth/casbin/ -A 5 -B 5

Length of output: 3705


Script:

#!/bin/bash
# Look for policy definitions or role assignments
rg "\.csv|policy" api/src/unraid-api/auth/casbin/

Length of output: 233


Script:

#!/bin/bash
# Check the content of any policy files
fd -e csv . api/src/unraid-api/auth/casbin/ --exec cat {}

Length of output: 61


3-3: Clarify the purpose of the possession parameter.

The non-standard possession parameter in the request definition needs explanation. This appears to be a custom extension to the typical (sub, obj, act) model.

Let's check if this parameter is documented elsewhere:

✅ Verification successful

Let me gather more context about how this parameter is defined and used in the authentication system.


Let me check how this parameter is used in the permission checking logic.


Based on the gathered information, I can now provide a clear explanation of the possession parameter's purpose.

The possession parameter is a well-defined access control attribute

The possession parameter is part of the Casbin access control model implementation and serves two specific purposes:

  • Defines the scope of access as either 'own' (user's own resources) or 'any' (any resource)
  • Used in permission matching where possession == p.possession || p.possession == '*'

Examples from the codebase:

  • Personal resources like user's cloud data use possession: 'own'
  • System-wide resources like docker networks use possession: 'any'
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for documentation or usage of the possession parameter
rg -i "possession" --type ts

Length of output: 5506


Script:

#!/bin/bash
# Look for type definitions and interfaces related to permissions
rg -A 5 -B 5 "type.*Permission|interface.*Permission" --type ts

# Check the Casbin model and policy definitions
cat api/src/unraid-api/auth/casbin/model.ts

Length of output: 2282


Script:

#!/bin/bash
# Look for the implementation of permission checking
rg -A 10 "checkPermission|ensurePermission" --type ts

# Look for any documentation about permissions in markdown files
rg -i "permission" -g "*.md"

Length of output: 38676

api/src/__test__/store/modules/paths.test.ts (2)

Line range hint 5-31: LGTM! Test updated correctly for new auth-keys path.

The snapshot update properly reflects the addition of the auth-keys path required for the new API key management system.


29-29: Verify auth-keys directory setup and security.

Since this path will store sensitive API key data, please ensure:

  1. The directory exists and has proper permissions
  2. Access to this directory is properly restricted
  3. The path is correctly configured in the environment variables

Let's verify the directory configuration:

✅ Verification successful

Directory security measures are properly implemented

The auth-keys directory setup has been verified and shows appropriate security measures:

  1. The directory path is configurable via environment variable PATHS_AUTH_KEY with a secure default at /boot/config/plugins/dynamix.my.servers/keys
  2. The ApiKeyService implements proper directory creation with:
    • Automatic directory creation if it doesn't exist
    • Proper error handling and logging
    • Secure file operations using promises for async I/O
  3. API keys are stored as individual JSON files with:
    • UUID-based filenames
    • Structured JSON content
    • Proper error handling during read/write operations

The implementation follows security best practices for storing sensitive API key data.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for auth-keys related configuration and setup

# Test 1: Check if the path is defined in environment files
echo "Checking environment configuration:"
rg -l "PATHS_AUTH_KEY" .env*

# Test 2: Check for any security-related configuration for this path
echo -e "\nChecking for security configurations:"
rg -A 3 "auth-keys" src/config

Length of output: 330


Script:

#!/bin/bash
# Let's try a broader search to understand the context and usage of auth-keys

# Check for auth-keys references in the codebase
echo "Checking auth-keys references:"
rg -l "auth-keys"

# Look for environment or configuration files
echo -e "\nLooking for config files:"
fd -e yaml -e yml -e json -e env

# Check for security-related code around auth-keys
echo -e "\nChecking security implementations:"
rg -B 2 -A 2 "auth-keys"

Length of output: 2293


Script:

#!/bin/bash
# Let's examine the security implementation in the API key service

# Check the API key service implementation
echo "Checking API key service implementation:"
cat api/src/unraid-api/auth/api-key.service.ts

# Check the path resolution implementation
echo -e "\nChecking path resolution:"
cat api/src/store/modules/paths.ts

# Check for file permission handling
echo -e "\nChecking file permission handling:"
rg -B 2 -A 2 "chmod|chown|writeFile|mkdir"

Length of output: 47457

api/src/unraid-api/auth/header.strategy.ts (3)

1-8: LGTM! Good choice using passport-custom.

The switch to passport-custom provides better flexibility for implementing custom authentication logic while maintaining backward compatibility with the strategy name.


10-10: LGTM! Good security practice.

Making the strategy key readonly prevents accidental modifications and improves security by ensuring the authentication strategy identifier remains immutable.


25-25: Verify the impact of the new validation method.

The switch to validateApiKeyCasbin changes the validation logic. Ensure all existing API key formats and permissions are compatible with the new system.

✅ Verification successful

Let me gather more information about the validation methods to understand the changes better.


Let me try a different approach to examine the validation methods.


Based on the code examination, I can now provide a final response:

The new validateApiKeyCasbin method is a safe enhancement to the validation system

The new validation method maintains backward compatibility while adding role-based access control:

  • It performs the same basic API key validation
  • Adds role synchronization with Casbin for fine-grained permissions
  • Maintains the same UserAccount return type and error handling
  • Has comprehensive test coverage validating the behavior

The implementation is well-tested and includes proper role management, making it a safe upgrade from the previous validation method.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all API key validation calls in the codebase
echo "Searching for API key validation calls..."
rg -l "validateUser|validateApiKey" --type ts

# Search for API key format patterns
echo "Searching for API key patterns..."
rg -l "x-api-key" --type ts

Length of output: 1107


Script:

#!/bin/bash
# Check the implementation of both validation methods
echo "Checking auth service implementation..."
ast-grep --pattern 'validateApiKey($_) {
  $$$
}'

echo -e "\nChecking new Casbin validation..."
ast-grep --pattern 'validateApiKeyCasbin($_) {
  $$$
}'

echo -e "\nChecking test cases..."
rg -A 10 "validateApiKeyCasbin" api/src/unraid-api/auth/auth.service.spec.ts

Length of output: 419


Script:

#!/bin/bash
# Check the auth service file content
echo "Checking auth service implementation..."
cat api/src/unraid-api/auth/auth.service.ts

echo -e "\nChecking related test cases..."
cat api/src/unraid-api/auth/auth.service.spec.ts

# Check for any migration or change related files
echo -e "\nChecking for migration files..."
fd -e ts -e js migration

Length of output: 11402

api/.env.development (1)

3-3: Verify directory creation and permissions handling.

Since this path will store sensitive API key data, ensure that:

  1. The directory is created with appropriate permissions during application startup
  2. Access to the keys directory is properly restricted
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)

18-19: LGTM: Import statements follow NestJS conventions

The imports for AuthResolver and AuthModule are correctly structured and follow NestJS module organization patterns.

api/src/unraid-api/auth/casbin/policy.ts (3)

33-52: Verify My Servers read-only access implications.

The my_servers role is limited to read operations except for connect/dynamic-remote-access. Verify if this aligns with the intended use cases.

#!/bin/bash
# Search for server management operations that might need write access

# Look for server management endpoints
ast-grep --pattern '@$_("/servers")
$$$
class $_ {
  $$$
}'

# Look for server update operations
rg "updateServer|modifyServer|configureServer" --type ts

5-32: Verify completeness of UPC permissions.

The UPC role has extensive read permissions but limited update capabilities. Some critical operations might be missing.

#!/bin/bash
# Search for UPC-related operations in the codebase that might need permissions

# Look for UPC-related endpoints
ast-grep --pattern 'class $_ {
  $$$
  @$_("upc")
  $$$
}'

# Look for UPC-related service methods
rg -A 5 "UpcService|UPC" --type ts

2-3: Review security implications of admin wildcard permissions.

The admin role has unrestricted access using wildcards (*). While this provides flexibility, it could pose security risks if an admin token is compromised.

Consider:

  1. Explicitly listing admin permissions for audit purposes
  2. Implementing resource-specific restrictions even for admins
  3. Adding logging for sensitive operations
✅ Verification successful

Wildcard admin permissions are appropriate with existing role-based safeguards

Based on the analysis of the codebase:

  1. All operations are protected by @UseRoles or @UsePermissions decorators that explicitly define required permissions
  2. The system implements fine-grained RBAC with specific resource/action pairs for each operation
  3. Critical operations are properly segregated (e.g., API key management, config updates, notifications)
  4. The wildcard admin permissions (p, admin, *, *, *) serve as a practical super-user access pattern, similar to root/sudo in Unix systems

The current implementation is secure because:

  • Authentication is enforced via GraphqlAuthGuard
  • Every operation requires explicit permission declarations
  • Role-based access control provides an audit trail of permitted operations
  • The system follows the principle of least privilege for all other roles
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if there are any sensitive operations that should be restricted
# Look for endpoints or operations marked as sensitive in the codebase

rg -i "sensitive|restricted|critical" --type ts

Length of output: 120


Script:

#!/bin/bash
# Let's analyze the API endpoints and their handlers to understand the operations
# that admin role would have access to

# Find all route definitions and controller methods
rg -A 5 "@(Get|Post|Put|Delete|Patch)(\(|Route)" --type ts

# Also check for any authentication/authorization middleware
rg -A 3 "(@UseGuards|@Authorized)" --type ts

Length of output: 1592


Script:

#!/bin/bash
# Let's check for GraphQL resolvers and their operations since we see GraphQL endpoints
# This will help understand the full scope of operations admin would have access to

# Find all GraphQL resolvers
ast-grep --pattern 'class $_ {
  $$$
  @$_
  async $_($$$) {
    $$$
  }
  $$$
}'

# Also check for any mutation definitions to understand write operations
rg -A 3 "@Mutation|@Query" --type ts

Length of output: 20787

api/src/unraid-api/users/users.service.ts (4)

12-12: LGTM!

The ApiKeyService dependency is properly injected following Angular's dependency injection pattern.


54-64: ⚠️ Potential issue

Add input validation and consider role format

Two concerns with the implementation:

  1. The method lacks input validation for the apiKey parameter.
  2. Joining roles with commas (key.roles.join(',')) might not be the best approach for RBAC integration with Casbin.

Consider adding input validation:

 async findOneByKey(apiKey: string): Promise<UserAccount | null> {
+    if (!apiKey?.trim()) {
+        return null;
+    }
     const key = await this.apiKeyService.findByKey(apiKey);
     if (!key) return null;

Let's verify how roles are used in the Casbin configuration:

#!/bin/bash
# Check Casbin policy files and configuration
fd -g "*casbin*" -t f

# Look for role usage patterns
rg -A 3 "roles.*=.*join|roles.*=.*split"

Line range hint 14-53: Consider deprecating hardcoded API keys

The apiKeyToUser method contains hardcoded service accounts and API key checks. With the new API key management system:

  1. These hardcoded keys should be migrated to the new system
  2. The method should be marked as deprecated

Consider this migration approach:

  1. Move service accounts to the new API key system
  2. Add a deprecation notice:
+    /**
+     * @deprecated Use findOneByKey instead. Will be removed once all service accounts
+     * are migrated to the new API key management system.
+     */
     apiKeyToUser(apiKey: string): UserAccount | null {

Let's check for other hardcoded API key usage:

#!/bin/bash
# Look for other potential hardcoded API keys
rg -A 3 "apikey.*=|API_KEY.*="

# Check for other references to these service accounts
rg -l "my_servers|upc|notifier.*service account"

5-8: Review security implications of BYPASS_PERMISSION_CHECKS

The import of BYPASS_PERMISSION_CHECKS suggests the existence of a security bypass mechanism. While useful for development/testing, ensure this flag is properly secured in production environments.

Let's verify the usage of this flag:

✅ Verification successful

Security bypass flag is properly controlled and logged

The BYPASS_PERMISSION_CHECKS flag is well-implemented with appropriate safeguards:

  • Only enabled through environment variable configuration (process.env.BYPASS_PERMISSION_CHECKS === 'true')
  • Usage is logged with warning messages when active
  • Limited to specific security-critical paths with proper checks
  • Used in development/testing contexts with clear documentation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any instances where BYPASS_PERMISSION_CHECKS might be set to true
rg -l "BYPASS_PERMISSION_CHECKS.*=.*true" 

# Check environment files for this variable
fd -g "*.env*" --exec grep -l "BYPASS_PERMISSION_CHECKS"

Length of output: 5750


Script:

#!/bin/bash
# Let's search for BYPASS_PERMISSION_CHECKS usage more effectively
rg -A 3 "BYPASS_PERMISSION_CHECKS"

# Also check environment files specifically
fd -g ".env*" -x cat {} \;

Length of output: 2194

api/src/unraid-api/auth/auth.guard.ts (3)

47-51: LGTM! Improved type safety with explicit return type.

The explicit return type declaration boolean | Promise<boolean> | Observable<boolean> enhances type safety and code clarity.


66-66: LGTM! Improved variable naming.

The variable name additionalConnectionParamHeaders is more descriptive and better reflects its purpose in handling WebSocket connection parameters.


Line range hint 47-74: Verify integration with new API key system.

Since this PR implements a new API key management system with RBAC, we should ensure this guard properly integrates with it.

Let's verify the authentication strategies and potential API key handling:

Consider the following architectural recommendations:

  1. Add a new ApiKeyStrategy extending PassportStrategy to handle API key authentication
  2. Update the AuthGuard to include the new strategy in its array: AuthGuard([ServerHeaderStrategy.key, UserCookieStrategy.key, ApiKeyStrategy.key])
  3. Ensure the guard properly integrates with Casbin's enforcer for RBAC checks

Would you like me to provide implementation examples for these recommendations?

✅ Verification successful

The auth guard properly integrates with the new API key system

The code review shows that the auth guard implementation correctly integrates with the new API key RBAC system through:

  1. The AuthGuard properly handles both HTTP and GraphQL contexts, extracting API key headers from either standard requests or GraphQL connection parameters
  2. The HeaderStrategy validates API keys by checking the 'x-api-key' header and delegating to AuthService.validateApiKeyCasbin()
  3. The AuthService implements proper API key validation with Casbin RBAC by:
    • Validating the API key existence
    • Syncing the API key roles with Casbin enforcer
    • Throwing UnauthorizedException for invalid keys

The implementation follows security best practices and properly integrates authentication with authorization through Casbin's RBAC system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for API key related authentication strategies and their integration

# Check for any new API key related strategy files
echo "Checking for API key strategy files..."
fd -e ts "key.*strategy" src/

# Look for Casbin enforcer usage in auth guards
echo "Checking for Casbin integration in guards..."
rg -l "Enforcer|enforce" "src/**/guard.ts"

# Check for other guards that might need similar updates
echo "Checking for other auth guards that might need updates..."
fd -e ts ".*guard.ts" src/

Length of output: 656


Script:

#!/bin/bash
# Let's try searching from the repository root instead of src/
echo "Checking for API key related files and strategies..."
fd -e ts "key|auth.*strategy"

echo "Looking for auth guards..."
fd -e ts ".*guard.ts"

echo "Checking for Casbin enforcer usage..."
rg "Enforcer|enforce"

echo "Checking for API key related imports and usage..."
rg -A 3 "ApiKey|API_KEY"

echo "Looking at the auth module structure..."
fd -e ts "auth"

Length of output: 110083


Script:

#!/bin/bash
# Let's check the auth guard implementation and its integration with API key validation

# Check the auth guard implementation
echo "Checking auth guard implementation..."
rg -A 10 "canActivate" api/src/unraid-api/auth/auth.guard.ts

# Check for API key validation in auth service
echo "Checking API key validation in auth service..."
rg -A 10 "validateApiKeyCasbin" api/src/unraid-api/auth/auth.service.ts

# Check for header strategy implementation
echo "Checking header strategy implementation..."
rg -A 10 "validate" api/src/unraid-api/auth/header.strategy.ts

# Look for Casbin model and policy configuration
echo "Checking Casbin configuration..."
rg -A 10 "CASBIN_MODEL|BASE_POLICY" api/src/unraid-api/auth/casbin.ts

Length of output: 2386

api/src/store/modules/paths.ts (1)

65-67: LGTM! Implementation follows established patterns.

The new path entry for API keys is well-implemented, following the existing patterns for path resolution and environment variable configuration.

Let's verify the environment variable configuration:

✅ Verification successful

Environment variable configuration is properly implemented

The verification confirms:

  • PATHS_AUTH_KEY is correctly defined in .env.development with a development-specific path
  • The fallback path is only referenced in the paths configuration, not hardcoded elsewhere
  • The implementation follows the same pattern as other environment-controlled paths in the system
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify PATHS_AUTH_KEY environment variable usage across the codebase

# Test 1: Check if PATHS_AUTH_KEY is defined in environment files
echo "Checking environment files for PATHS_AUTH_KEY..."
rg "PATHS_AUTH_KEY=" -g "*.env*"

# Test 2: Check for any hardcoded references to the default path
echo "Checking for hardcoded paths..."
rg "/boot/config/plugins/dynamix.my.servers/keys"

Length of output: 457

api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts (3)

1-11: LGTM! Well-organized imports.

The imports are logically structured, separating external dependencies from internal modules.


33-43: LGTM! Well-structured test setup.

The beforeAll block properly initializes all required services in a logical order, with clear dependency injection.


34-34: Verify Casbin enforcer configuration.

The current test initializes a blank enforcer without any RBAC model or policy configuration. Consider:

  1. Loading the actual RBAC model configuration used in production
  2. Adding test policies to verify permission checks
api/src/unraid-api/auth/api-key.service.spec.ts (2)

1-11: LGTM: Imports and mocking setup are well-structured.

The necessary testing utilities are imported and the file system and store modules are properly mocked for test isolation.


1-177: Overall: Well-structured and comprehensive test suite.

The test suite demonstrates good practices:

  • Proper test isolation using beforeEach/afterEach
  • Comprehensive coverage of happy paths and error cases
  • Clear test descriptions
  • Effective mocking of external dependencies

While some additional test cases have been suggested, the current implementation provides a solid foundation for testing the API key service.

🧰 Tools
🪛 Gitleaks

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

(generic-api-key)

api/package.json (1)

88-88: Verify the migration strategy for authorization systems.

The addition of Casbin alongside existing authorization libraries (nest-access-control, nest-authz) suggests a transitional approach. While this is good for gradual migration, consider:

  1. Documenting the migration strategy
  2. Setting up clear boundaries between old and new auth systems
  3. Planning for eventual cleanup of deprecated auth libraries

Run this script to analyze the usage of different auth systems:

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

45-45: Ensure ApiKeyService is adequately tested

With the addition of ApiKeyService, please ensure that comprehensive unit tests are provided to verify its functionality and to prevent potential regressions.

Run the following script to check for existing unit tests for ApiKeyService:

✅ Verification successful

ApiKeyService is well-tested with comprehensive coverage

The service has extensive unit tests covering all major functionality including:

  • Path handling and directory creation
  • API key creation
  • Finding keys (all, by ID, and by key value)
  • Key persistence operations
  • Error handling scenarios

The tests include proper mocking, edge cases, and error conditions, providing good confidence in the service's reliability.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of unit tests for ApiKeyService

# Test: Search for test files related to ApiKeyService
# Expected: Should find at least one test file

fd 'api-key.service.spec.ts' ./src

Length of output: 127


Script:

#!/bin/bash
# Let's first find the correct path for api-key service and its tests
fd "api-key.service" .

# Also search for any test files containing ApiKeyService
rg "ApiKeyService.*spec" -l

Length of output: 147


Script:

#!/bin/bash
# Let's examine the content of the test file to verify test coverage
cat ./api/src/unraid-api/auth/api-key.service.spec.ts

Length of output: 6462

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

71-74: ⚠️ Potential issue

Verify the action and resource in @UsePermissions for addRoleForUser

The @UsePermissions decorator for addRoleForUser specifies action: 'create' and resource: 'permission'. Since this mutation adds a role to a user, it might be more appropriate to set the resource to 'user' and the action to 'update'.

Please verify that the specified action and resource align with the intended permissions model.


112-112: ⚠️ Potential issue

Correct variable usage in removeRoleFromApiKey method

The variables apiKeyId and role are not defined in this scope. You should use input.apiKeyId and input.role instead.

Apply this diff to fix the issue:

-return this.authService.removeRoleFromApiKey(apiKeyId, role);
+return this.authService.removeRoleFromApiKey(input.apiKeyId, input.role);

Likely invalid or redundant comment.

mdatelle and others added 2 commits November 1, 2024 15:23
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (3)
api/src/unraid-api/auth/auth.service.ts (3)

19-36: Consider marking legacy methods as deprecated.

Since these methods will be replaced by their Casbin counterparts (validateApiKeyCasbin and validateCookiesCasbin), consider adding @deprecated JSDoc tags with migration instructions.

+    /** @deprecated Use validateApiKeyCasbin instead. */
     async validateUser(apiKey: string): Promise<UserAccount> {

+    /** @deprecated Use validateCookiesCasbin instead. */
     async validateCookies(cookies: object): Promise<UserAccount> {

50-54: Consider structuring debug logs for better parsing.

While the debug logging is helpful, consider using a structured format for better log parsing and monitoring.

-        this.logger.debug(
-            `Validating API key with roles: ${JSON.stringify(
-                await this.authzService.getRolesForUser(apiKeyEntity.id)
-            )}`
-        );
+        this.logger.debug('Validating API key roles', {
+            apiKeyId: apiKeyEntity.id,
+            roles: await this.authzService.getRolesForUser(apiKeyEntity.id)
+        });

136-142: Add logging for user role initialization.

Consider adding debug logs to track when guest roles are assigned to users.

 private async ensureUserRoles(userId: string): Promise<void> {
     const existingRoles = await this.authzService.getRolesForUser(userId);

     if (existingRoles.length === 0) {
+        this.logger.debug(`Initializing user ${userId} with guest role`);
         await this.authzService.addRoleForUser(userId, 'guest');
     }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e6f4194 and 62a3e9e.

📒 Files selected for processing (1)
  • api/src/unraid-api/auth/auth.service.ts (1 hunks)
🔇 Additional comments (2)
api/src/unraid-api/auth/auth.service.ts (2)

1-18: LGTM! Dependencies and setup are well-structured.

The imports and constructor dependencies are properly organized, with good use of dependency injection principles and logging setup.


64-78: LGTM! Good implementation of cookie validation with role verification.

The method properly validates the session and ensures user roles are set up.

Michael Datelle and others added 2 commits November 1, 2024 15:40
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
api/src/unraid-api/auth/auth.service.ts (2)

84-103: Consider adding transaction support for role synchronization

The role synchronization process involves multiple operations that should ideally be atomic to maintain data consistency.

Consider wrapping the role synchronization in a transaction if your database supports it:

public async syncApiKeyRoles(apiKeyId: string, roles: string[]): Promise<void> {
    try {
        await this.dataSource.transaction(async (transactionalEntityManager) => {
            const existingRoles = await this.authzService.getRolesForUser(apiKeyId);
            
            for (const role of existingRoles) {
                await this.authzService.deleteRoleForUser(apiKeyId, role);
            }
            
            for (const role of roles) {
                await this.authzService.addRoleForUser(apiKeyId, role);
            }
        });
    } catch (error: unknown) {
        this.logger.error(`Failed to sync roles for API key ${apiKeyId}`, error);
        throw new Error(
            `Failed to sync roles: ${error instanceof Error ? error.message : String(error)}`
        );
    }
}

147-153: Add logging for role assignment operations

Consider adding debug logging to track role assignments:

 private async ensureUserRoles(userId: string): Promise<void> {
     const existingRoles = await this.authzService.getRolesForUser(userId);

     if (existingRoles.length === 0) {
+        this.logger.debug(`Assigning default 'guest' role to user: ${userId}`);
         await this.authzService.addRoleForUser(userId, 'guest');
     }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 62a3e9e and 42bd3df.

📒 Files selected for processing (1)
  • api/src/unraid-api/auth/auth.service.ts (1 hunks)
🔇 Additional comments (5)
api/src/unraid-api/auth/auth.service.ts (5)

2-18: LGTM: Clean dependency setup and proper DI pattern

The new dependencies are properly imported and injected following the dependency injection pattern. The Logger setup is appropriate for the service scope.


68-82: LGTM: Robust session validation implementation

The method implements proper session validation with appropriate error handling and role verification.


105-109: Refer to previous review comment about input validation


117-145: Refer to previous review comments about concurrency and input validation


1-153: Verify integration with existing authorization system

Since this is part of a permissions system rewrite, we should verify the coexistence with the existing system.

✅ Verification successful

Let me gather more context about the authentication strategies to understand the transition between systems.


Based on the implementation details from the strategies and tests, I can now provide a final response:

Coexistence of auth systems is properly managed

The implementation shows a clean transition to the new RBAC system:

  • Header strategy exclusively uses the new validateApiKeyCasbin method
  • Cookie strategy still uses the old validateCookies method, but the auth service maintains backward compatibility
  • Comprehensive test coverage confirms both systems work as expected
  • No circular dependencies or conflicting implementations found
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential conflicts between old and new auth systems
echo "Checking for potential auth system conflicts..."

# Look for usage of old auth methods
rg -l "validateUser|validateCookies" --type ts

# Check for places where both systems might be used
rg -l "validateApiKeyCasbin|validateCookiesCasbin" --type ts

# Look for potential circular dependencies
ast-grep --pattern 'import { AuthService } from "./auth.service"'

Length of output: 540


Script:

#!/bin/bash
# Check the implementation of both old and new strategies
echo "=== Header Strategy Implementation ==="
cat api/src/unraid-api/auth/header.strategy.ts

echo -e "\n=== Cookie Strategy Implementation ==="
cat api/src/unraid-api/auth/cookie.strategy.ts

echo -e "\n=== Auth Service Tests ==="
cat api/src/unraid-api/auth/auth.service.spec.ts

Length of output: 8892

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
api/src/unraid-api/auth/auth.service.ts (1)

155-161: Enhance error handling in ensureUserRoles

Consider adding error handling and logging for better debugging and reliability.

Apply this diff:

 private async ensureUserRoles(userId: string): Promise<void> {
+    try {
         const existingRoles = await this.authzService.getRolesForUser(userId);

         if (existingRoles.length === 0) {
+            this.logger.debug(`Assigning default 'guest' role to user ${userId}`);
             await this.authzService.addRoleForUser(userId, 'guest');
         }
+    } catch (error) {
+        this.logger.error(`Failed to ensure roles for user ${userId}`, error);
+        throw new Error(`Failed to ensure user roles: ${error instanceof Error ? error.message : String(error)}`);
+    }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 42bd3df and ddd4ffd.

📒 Files selected for processing (1)
  • api/src/unraid-api/auth/auth.service.ts (1 hunks)
🔇 Additional comments (4)
api/src/unraid-api/auth/auth.service.ts (4)

1-18: LGTM: Dependencies and constructor setup look good!

The new dependencies are properly imported and injected, with good separation of concerns between different services. The Logger initialization follows best practices.


42-66: ⚠️ Potential issue

Fix potential undefined roles issue and enhance error logging

The past review comment about handling undefined roles is still valid. Additionally, consider enhancing error logging for better debugging.

Apply this diff:

 async validateApiKeyCasbin(apiKey: string): Promise<UserAccount> {
+    this.logger.debug(`Validating API key: ${apiKey.substring(0, 8)}...`);
     const apiKeyEntity = await this.apiKeyService.findByKey(apiKey);

     if (!apiKeyEntity) {
+        this.logger.warn(`Invalid API key attempt: ${apiKey.substring(0, 8)}...`);
         throw new UnauthorizedException('Invalid API key');
     }

     if (!apiKeyEntity.roles) {
+        this.logger.debug(`Initializing empty roles for API key: ${apiKeyEntity.id}`);
         apiKeyEntity.roles = [];
     }

     await this.syncApiKeyRoles(apiKeyEntity.id, apiKeyEntity.roles);
     this.logger.debug(
         `Validating API key with roles: ${JSON.stringify(
             await this.authzService.getRolesForUser(apiKeyEntity.id)
         )}`
     );

     return {
         id: apiKeyEntity.id,
         name: apiKeyEntity.name,
         description: apiKeyEntity.description ?? `API Key ${apiKeyEntity.name}`,
-        roles: apiKeyEntity.roles.join(','),
+        roles: apiKeyEntity.roles?.join(',') ?? '',
     };
 }

Likely invalid or redundant comment.


105-109: ⚠️ Potential issue

Add input validation and duplicate checking for permissions

The method should validate inputs and check for existing policies before adding them.

Apply this diff:

 public async addPermission(role: string, resource: string, action: string): Promise<boolean> {
+    if (!role || !resource || !action) {
+        throw new Error('Role, resource, and action are required');
+    }
+
+    const exists = await this.authzService.hasPolicy(role, resource, action);
+    if (exists) {
+        return true;
+    }
+
     await this.authzService.addPolicy(role, resource, action);
     return true;
 }

Likely invalid or redundant comment.


117-135: Consider implementing transactional operations for role modifications

The method could face race conditions when modifying roles. Consider implementing transactional operations if your database supports it.

Let's verify if the database supports transactions:

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
api/src/utils.ts (2)

178-201: Add request validation for enhanced security.

The implementation looks good with the BadRequestException and proper context type handling. However, consider adding validation for essential request properties:

 if (!request) {
     throw new BadRequestException(
         `Unsupported execution context type: ${contextType}. Only HTTP and GraphQL contexts are supported.`
     );
 }

+    // Validate request integrity
+    if (!request.headers || !request.method) {
+        throw new BadRequestException('Malformed request: missing required properties');
+    }
+
 return request;

223-229: Improve type safety in context sanitization.

The type casting to any could be replaced with a more type-safe approach.

-            for (const [key, value] of Object.entries(obj)) {
-                if (typeof value === 'string' && key.toLowerCase().includes('key')) {
-                    (obj as any)[key] = '[REDACTED]';
-                }
-            }
+            const entries = Object.entries(obj) as [string, unknown][];
+            for (const [key, value] of entries) {
+                if (typeof value === 'string' && key.toLowerCase().includes('key')) {
+                    (obj as Record<string, unknown>)[key] = '[REDACTED]';
+                }
+            }
📜 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 7d525bf and 86f0b32.

📒 Files selected for processing (1)
  • api/src/utils.ts (2 hunks)
🔇 Additional comments (2)
api/src/utils.ts (2)

1-7: LGTM: Imports are well-organized and necessary.

All imports are properly organized and utilized in the new functions.


213-245: LGTM: Robust error handling with security considerations.

The implementation successfully addresses previous feedback:

  • Uses structuredClone for deep cloning
  • Implements updateObject for context sanitization
  • Removes redundant "Failed to" prefix
  • Prevents information leakage

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

@mdatelle mdatelle merged commit 2b7413e into main Dec 16, 2024
9 checks passed
@mdatelle mdatelle deleted the refactor/permissions-system-rewrite branch December 16, 2024 20:25
@coderabbitai coderabbitai bot mentioned this pull request Apr 9, 2025
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.

3 participants