Skip to content

Conversation

@elibosley
Copy link
Member

@elibosley elibosley commented Apr 11, 2025

  • usePermissions applies both authz + graphQL directive logic to allow permissions and documentation in one place

Summary by CodeRabbit

  • New Features

    • Introduced a new GraphQL permission directive that documents required permissions for API fields.
    • Added enums for defining action verbs (create, update, delete, read) and possession types (any, own, own any) to enable granular access control.
    • Added a new health field to the Query type for improved API health monitoring.
  • Chores

    • Consolidated permission handling by updating import sources and retiring legacy authorization tests and code, enhancing overall maintainability.
    • Updated configuration version in the API settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 11, 2025

Walkthrough

This pull request implements enhanced authorization in the GraphQL API. A new directive, @usePermissions, is introduced along with enums AuthActionVerb and AuthPossession to capture field-level permission requirements. The previous authorization logic and tests (including files like auth.directive.ts, auth.directive.spec.ts, and auth-enum.utils.ts) have been removed. Additionally, the GraphQL module and numerous resolver files have been updated to import these entities from the new permissions module, ensuring the schema transformation supports detailed permission management.

Changes

File(s) Change Summary
api/generated-schema.graphql Added new directive @usePermissions with parameters and new enums; updated Subscription type to use the directive.
api/src/unraid-api/graph/directives/use-permissions.directive.ts Introduces the new GraphQL permission directive, related enums (AuthActionVerbEnum, AuthPossessionEnum), decorator function, and schema transformer; re-exports permission types.
api/src/unraid-api/graph/directives/auth.directive.ts,
api/src/unraid-api/graph/directives/auth.directive.spec.ts,
api/src/unraid-api/graph/utils/auth-enum.utils.ts
Removed old authorization logic, tests, and enum generation utility.
api/src/unraid-api/graph/graph.module.ts Updated to import the new permission directive and transformer, integrating them into buildSchemaOptions.
Multiple resolver files under
api/src/unraid-api/graph/resolvers/…
Updated import statements to source AuthActionVerb, AuthPossession, and UsePermissions from the new module (@app/unraid-api/graph/directives/use-permissions.directive.js) instead of nest-authz; minor formatting updates applied.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant GraphQLServer
    participant SchemaTransformer
    participant Resolver

    Client->>GraphQLServer: Send GraphQL request
    GraphQLServer->>SchemaTransformer: Apply `@usePermissions` transformation
    SchemaTransformer->>GraphQLServer: Return updated schema
    GraphQLServer->>Resolver: Execute resolver with permission checks
    Resolver->>GraphQLServer: Validate permissions
    GraphQLServer->>Client: Return response
Loading

Possibly related PRs

  • feat: code first graphql #1347: Related changes introduce a new directive @usePermissions and related enums for permission management in the GraphQL schema, enhancing the API's structure and functionality around permissions and access management.

Poem

In the code where rules take flight,
A new directive shines so bright.
Permissions guide each field and row,
Old logic fades as fresh winds blow.
Cheers to change in every byte—let’s code right!
🚀✨


🪧 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 resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @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.

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 comments (1)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (1)

1-110: 💡 Verification agent

🧩 Analysis chain

Verify test coverage for permission changes

While the implementation changes look good, it would be beneficial to ensure there's adequate test coverage for these permission changes, especially since the PR mentions removing previous authorization logic and tests.


🏁 Script executed:

#!/bin/bash
# Check for test files related to the new permission directive
echo "Checking for test files related to the new permission directive..."
fd "use-permissions.*\.spec" --type file

Length of output: 185


Attention: Insufficient Test Coverage for Permission Changes

It appears that no test files related to the new permission directive (matching the pattern use-permissions.*.spec) were found. Please ensure that tests are added to verify that the permission changes in the resolver functions (e.g., setState, addDiskToArray, removeDiskFromArray, etc.) are correctly enforced.

  • Verify that permission-related behavior is covered in your tests.
  • Add test cases to confirm that unauthorized requests are properly rejected and authorized requests pass as expected.
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (1)

39-48: Consider refactoring duplicate method implementation

The domain and domains methods have identical implementations. Consider refactoring to avoid code duplication.

@ResolveField(() => [VmDomain])
public async domains(): Promise<Array<VmDomain>> {
    try {
        return await this.vmsService.getDomains();
    } catch (error) {
        throw new Error(
            `Failed to retrieve VM domains: ${error instanceof Error ? error.message : 'Unknown error'}`
        );
    }
}

@ResolveField(() => [VmDomain])
-public async domain(): Promise<Array<VmDomain>> {
-    try {
-        return await this.vmsService.getDomains();
-    } catch (error) {
-        throw new Error(
-            `Failed to retrieve VM domains: ${error instanceof Error ? error.message : 'Unknown error'}`
-        );
-    }
+public async domain(): Promise<Array<VmDomain>> {
+    return this.domains();
}
api/src/unraid-api/graph/directives/use-permissions.directive.ts (2)

17-43: Consider DRY approach for repeated enum creation
The logic for creating AuthActionVerbEnum and AuthPossessionEnum is nearly identical, differing only in the source enum. You can refactor it into a reusable helper to avoid duplication and improve maintainability.

Example helper function:

function buildGraphQLEnum(enumObj: Record<string, string | number>, name: string, description: string) {
  const values = Object.entries(enumObj)
    .filter(([key]) => isNaN(Number(key)))
    .reduce((acc, [key]) => {
      acc[key] = { value: key };
      return acc;
    }, {} as Record<string, { value: string }>);

  return new GraphQLEnumType({ name, description, values });
}

85-122: Avoid using console.log for tracing
Line 90 logs a debug message with console.log, which is typically discouraged in production environments. Use a more robust logging strategy (e.g., Logger from NestJS) or remove it if it’s purely for troubleshooting.

- console.log('usePermissionsDirective', usePermissionsDirective);
+ // console.log removed or replaced with a proper logger
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5724ab and 81b240a.

📒 Files selected for processing (34)
  • api/generated-schema.graphql (2 hunks)
  • api/src/unraid-api/graph/directives/auth.directive.spec.ts (0 hunks)
  • api/src/unraid-api/graph/directives/auth.directive.ts (0 hunks)
  • api/src/unraid-api/graph/directives/use-permissions.directive.ts (1 hunks)
  • api/src/unraid-api/graph/graph.module.ts (2 hunks)
  • api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/parity.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/parity.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/config/config.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/connect/connect-settings.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/connect/connect.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/display/display.resolver.ts (2 hunks)
  • api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/network/network.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/services/services.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/shares/shares.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/user/user.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/utils/auth-enum.utils.ts (0 hunks)
💤 Files with no reviewable changes (3)
  • api/src/unraid-api/graph/directives/auth.directive.spec.ts
  • api/src/unraid-api/graph/utils/auth-enum.utils.ts
  • api/src/unraid-api/graph/directives/auth.directive.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
web/composables/gql/graphql.ts (1)
  • Display (501-524)
api/src/unraid-api/graph/graph.module.ts (1)
api/src/unraid-api/graph/directives/use-permissions.directive.ts (2)
  • UsePermissionsDirective (46-64)
  • usePermissionsSchemaTransformer (86-122)
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Build Web App
  • GitHub Check: Build API
  • GitHub Check: Test API
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (36)
api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts (1)

4-8: Import source updated to use local directive

The import statements have been modified to source AuthActionVerb, AuthPossession, and UsePermissions from the local directive module instead of the external nest-authz package. This change aligns with the PR objective of introducing a custom decorator for permission management.

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

4-8: Import source updated to use local directive

The import statements have been updated to source authorization entities from the local directive module. This maintains consistency with other resolver files while transitioning to the custom permission system.

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

3-7: Import source updated to use local directive

The import statements have been modified to use the local directive implementation instead of the external package. This change supports the new permission documentation feature while maintaining the same interface.

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

4-8: Import source updated to use local directive

The import statements have been modified to source permission-related entities from the local directive. This change is consistent with the updates in other resolver files and supports the custom permission documentation feature.

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

4-8: Import path updated for authorization components

The imports have been correctly updated to use the new directive from the local codebase instead of the external package. This aligns with the PR objective to use a custom decorator for unified permission management.

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

3-7: Import path updated for authorization components

The imports have been updated to use the local custom directive implementation instead of the external package, consistent with the PR objectives.

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

4-8: Import path updated for authorization components

The imports for AuthActionVerb, AuthPossession, and UsePermissions have been correctly updated to use the new custom directive implementation.

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

4-8: Import path updated for authorization components

The imports have been updated to use the custom directive implementation, maintaining consistent import patterns across all resolver files.

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

3-7: Import path updated for authorization components

The imports have been correctly updated to use the new custom directive, consistent with all other resolver files in this PR.

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

4-8: Updated Permissions Imports:
The import for AuthActionVerb, AuthPossession, and UsePermissions has been updated to source from @app/unraid-api/graph/directives/use-permissions.directive.js instead of nest-authz. This change aligns with the new permissions management approach introduced in this PR.

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

6-10: Consolidated Permissions Decorator Imports:
The permission decorators are now imported from the new module, ensuring consistency across the codebase. The usage with the @UsePermissions decorator on the services query remains unchanged and correct.

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

7-11: Consistent Import Update for Cloud Resolver:
The updated import of AuthActionVerb, AuthPossession, and UsePermissions from @app/unraid-api/graph/directives/use-permissions.directive.js is correctly implemented. This update supports the new GraphQL directive for permissions without affecting existing business logic.

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

3-7: Refined Permissions Imports in VM Mutations Resolver:
The import statement for the permission-related entities is correctly updated to the new source. The application of @UsePermissions across the mutation resolver methods is clear and aligns with the centralized permissions approach.

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

5-9: Updated API Key Resolver Imports:
The change to import AuthActionVerb, AuthPossession, and UsePermissions from @app/unraid-api/graph/directives/use-permissions.directive.js is properly executed. This update keeps the permission management consistent and maintains the integrity of the resolver logic.

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

10-14: Import source updated to use new permission directive

The import statement has been updated to use the local custom directive implementation instead of the external nest-authz package. This aligns with the PR objective of introducing a custom decorator that integrates both authorization and GraphQL directive logic.

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

7-11: Import source updated to use local permission directive

The imports have been correctly updated to use the new local custom directive implementation, consistent with the pattern across other resolver files.

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

4-8: Import source updated for permission-related entities

The import source for authorization-related entities has been updated to use the new custom directive implementation, maintaining consistency with other resolver files while preserving existing functionality.

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

16-20: Updated import source for permission decorator

The import statement has been updated to use the new custom directive implementation. This change is consistent with the modifications in other resolver files and supports the PR's goal of consolidating permission management.

api/src/unraid-api/graph/graph.module.ts (3)

12-15: New imports added for permission directive functionality

The imports for UsePermissionsDirective and usePermissionsSchemaTransformer from the custom directive file look good. This is consistent with the PR objective to introduce a custom decorator for permission management.


57-57: Added UsePermissionsDirective to schema build options

Good addition of the UsePermissionsDirective to the directives array within buildSchemaOptions. This properly registers the directive with the GraphQL schema.


59-59: Added schema transformer for permissions

The transformSchema property is correctly set to usePermissionsSchemaTransformer, which will enhance field descriptions with permission information. This is a clean way to implement documentation of permission requirements.

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

3-7: Updated imports to use custom permissions directive

The import has been correctly updated to use the custom permissions directive instead of the nest-authz package. This change maintains the same interface while transitioning to the new implementation.

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

3-7: Updated imports to use custom permissions directive

The imports have been properly updated to use the custom permissions implementation instead of nest-authz. This change is consistent with the other resolver files and supports the centralized permission management approach.

api/src/unraid-api/graph/resolvers/array/parity.mutations.resolver.ts (1)

5-9: Updated imports to use custom permissions directive

The import statements have been correctly updated to use the custom permissions directive. This change is in line with the rest of the codebase updates and maintains a consistent approach to permission management.

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

4-8: Import updated to use custom permissions directive

The updated import statement correctly points to the new local implementation of the permissions directive instead of the external nest-authz package. This change properly aligns with the PR objective of using a custom decorator for permissions management.

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

5-9: Import updated to use custom permissions directive

The updated import statement correctly points to the new local implementation of the permissions directive. This change is consistent with the project's move toward using a custom decorator for unified permission management.

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

5-9: Import updated to use custom permissions directive

The updated import statement correctly points to the new local implementation of the permissions directive. This change maintains the same permission structure while moving to the custom implementation.

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

5-9: Import updated to use custom permissions directive

The updated import statement correctly points to the new local implementation of the permissions directive. This change aligns with the PR objective of using a custom decorator for permissions management.

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

1-1: Removed unused imports

Unused imports Args and Mutation have been removed from @nestjs/graphql, improving code cleanliness.


6-10: Updated permission imports to use custom directive

The permission-related imports have been updated to use the new custom directive implementation from the project instead of the external nest-authz package. This aligns with the PR objective of introducing a custom decorator for permissions management.

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

5-9: Updated permission imports to use custom directive

The permission-related imports have been updated to use the new custom directive implementation instead of the external nest-authz package, maintaining consistency with other resolvers in the codebase.

api/generated-schema.graphql (2)

5-15: Added new usePermissions directive

A new GraphQL directive @usePermissions has been added to document and enforce required permissions for fields. This directive takes three parameters:

  • action: The required action verb (AuthActionVerb)
  • resource: The resource being accessed (String)
  • possession: The possession type required (AuthPossession)

This implementation allows for more explicit permission documentation directly in the GraphQL schema.


1607-1620: Added authorization enums

Two new enums have been added to support the permission system:

  1. AuthActionVerb: Defines possible actions (CREATE, UPDATE, DELETE, READ)
  2. AuthPossession: Defines possession types (ANY, OWN, OWN_ANY)

These enums provide type safety and structure for the permission system, enhancing maintainability and improving documentation of available permission options.

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

8-12: Updated permission imports to use custom directive

The permission-related imports have been updated to use the new custom directive implementation instead of the external nest-authz package, maintaining consistency with other resolvers in the codebase.


71-71: Improved decorator formatting

The @Query decorator has been moved to its own line, improving code readability without changing functionality.

api/src/unraid-api/graph/directives/use-permissions.directive.ts (1)

66-83:

❓ Verification inconclusive

Handle potential resource string injection and check enum casing

  1. If permissions.resource can come from external or untrusted inputs, injecting quotes or other characters could disrupt the generated directive. Consider sanitizing or escaping the string to avoid schema parsing errors.
  2. Verify that the uppercase usage of permissions.action.toUpperCase() and permissions.possession.toUpperCase() matches the enum keys declared in AuthActionVerbEnum and AuthPossessionEnum. If the original nest-authz enums use lowercase keys, you may need to adjust this approach.

Below is a possible fix for escaping quotes in permissions.resource:

 Directive(
-  `@usePermissions(action: ${permissions.action.toUpperCase()}, resource: "${permissions.resource}", possession: ${permissions.possession.toUpperCase()})`
+  (() => {
+    const sanitizedResource = permissions.resource.replace(/"/g, '\\"');
+    return `@usePermissions(action: ${permissions.action.toUpperCase()}, resource: "${sanitizedResource}", possession: ${permissions.possession.toUpperCase()})`;
+  })()
 )(target, propertyKey, descriptor);

Attention: Sanitize the resource string and verify enum casing
The code currently interpolates permissions.resource directly into the directive string. This could lead to injection issues if the value isn’t properly sanitized. Additionally, please confirm that applying .toUpperCase() to the enum properties (for both action and possession) is consistent with the case expected by AuthActionVerbEnum and AuthPossessionEnum.

  • Sanitize Input: If permissions.resource may contain user-controlled data, ensure that any quotes or special characters are escaped. For example:

    Directive(
    -  `@usePermissions(action: ${permissions.action.toUpperCase()}, resource: "${permissions.resource}", possession: ${permissions.possession.toUpperCase()})`
    +  (() => {
    +    const sanitizedResource = permissions.resource.replace(/"/g, '\\"');
    +    return `@usePermissions(action: ${permissions.action.toUpperCase()}, resource: "${sanitizedResource}", possession: ${permissions.possession.toUpperCase()})`;
    +  })()
    )(target, propertyKey, descriptor);
  • Verify Enum Casing: Double-check that the uppercase conversion of the enum values is appropriate for your project's definitions. If the consuming logic or external services expect lowercase keys, adjust accordingly.

Please review these points and verify their impact on the rest of the codebase.

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 (1)
api/src/unraid-api/graph/directives/use-permissions.directive.ts (1)

87-123: Consider throwing an error for missing directive arguments.
Currently, a warning is logged if action, resource, or possession is missing. You might want to throw an error instead so that invalid schemas fail early rather than silently passing through.

- console.warn(
-   `UsePermissions directive on ${typeName}.${fieldName} is missing required arguments.`
- );
+ throw new Error(
+   `UsePermissions directive on ${typeName}.${fieldName} is missing required arguments.`
+ );
📜 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 81b240a and 072c4b1.

📒 Files selected for processing (2)
  • api/src/unraid-api/graph/directives/use-permissions.directive.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Test API
  • GitHub Check: Build Web App
  • GitHub Check: Build API
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/unraid-api/graph/directives/use-permissions.directive.ts (4)

13-14: Good practice to re-export essential types.
Re-exporting AuthActionVerb and AuthPossession simplifies imports for downstream modules.


47-66: Directive arguments look consistent with best practices.
The directive properly validates required arguments (action, resource, possession) at the schema level.


68-85: Check for potential string injection in directive usage.
You’re embedding unescaped permissions.resource into a directive string. If these strings originate from user input, consider sanitizing or validating them to avoid GraphQL parse errors.


16-32:

✅ Verification successful

Validate handling of numeric enum members.
The logic omits numeric enum keys by filtering them out. This ensures a tidy GraphQL enum, but make sure no numeric-based keys need to be included.

Run the following script to check if AuthActionVerb or AuthPossession contain numeric keys that should not be ignored:


🏁 Script executed:

#!/bin/bash
fd -t f | xargs rg "enum Auth(A|P)(ctionVerb|ossession)"

Length of output: 160


Numeric Enum Filtering Verified

I confirmed that the generated schema (api/generated-schema.graphql) defines both AuthActionVerb and AuthPossession without any numeric keys. The filtering logic in use-permissions.directive.ts (lines 16-32) correctly omits numeric enum keys, and there’s no evidence that any necessary numeric-based keys are being erroneously removed.

@elibosley elibosley force-pushed the feat/permission-endpoints branch from 072c4b1 to 8590471 Compare April 14, 2025 15:46
@elibosley elibosley requested review from mdatelle and pujitm April 14, 2025 15:55
@github-actions
Copy link
Contributor

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

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

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

♻️ Duplicate comments (1)
api/src/unraid-api/graph/directives/use-permissions.directive.ts (1)

114-120: Slight simplification for string concatenation

You could simplify to something like:

fieldConfig.description = permissionDoc + (fieldConfig.description ?? '')

This was previously mentioned in a past review; marking it as duplicate.

🧹 Nitpick comments (3)
api/src/unraid-api/graph/directives/use-permissions.directive.ts (3)

16-32: buildGraphQLEnum utility function

Implementation cleanly builds a new GraphQLEnumType by filtering out numeric enum keys. This is a standard approach for string-based enums in TypeScript. Consider adding unit tests to validate that numeric keys are skipped and string keys are included.


68-85: UsePermissions decorator

Combining NestAuthz's UsePermissions with the GraphQL directive in one decorator is elegant. No immediate issues. Consider adding test coverage to ensure the decorator is applied and the directive is properly attached in the schema.


87-126: Schema transformer for permission documentation

Appending permission details to the field description is a nice touch. Consider throwing an error — rather than only logging a warning — if required arguments (action, resource, possession) are missing. This ensures that misconfigurations are caught early.

📜 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 072c4b1 and 691a8e4.

📒 Files selected for processing (35)
  • api/dev/Unraid.net/myservers.cfg (1 hunks)
  • api/generated-schema.graphql (4 hunks)
  • api/src/unraid-api/graph/directives/auth.directive.spec.ts (0 hunks)
  • api/src/unraid-api/graph/directives/auth.directive.ts (0 hunks)
  • api/src/unraid-api/graph/directives/use-permissions.directive.ts (1 hunks)
  • api/src/unraid-api/graph/graph.module.ts (2 hunks)
  • api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/parity.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/array/parity.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/config/config.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/connect/connect-settings.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/connect/connect.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/display/display.resolver.ts (2 hunks)
  • api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/network/network.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (3 hunks)
  • api/src/unraid-api/graph/services/services.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/shares/shares.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/user/user.resolver.ts (1 hunks)
  • api/src/unraid-api/graph/utils/auth-enum.utils.ts (0 hunks)
💤 Files with no reviewable changes (3)
  • api/src/unraid-api/graph/utils/auth-enum.utils.ts
  • api/src/unraid-api/graph/directives/auth.directive.spec.ts
  • api/src/unraid-api/graph/directives/auth.directive.ts
✅ Files skipped from review due to trivial changes (1)
  • api/dev/Unraid.net/myservers.cfg
🚧 Files skipped from review as they are similar to previous changes (29)
  • api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts
  • api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts
  • api/src/unraid-api/graph/resolvers/connect/connect-settings.resolver.ts
  • api/src/unraid-api/graph/user/user.resolver.ts
  • api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.ts
  • api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
  • api/src/unraid-api/graph/resolvers/online/online.resolver.ts
  • api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
  • api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts
  • api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts
  • api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
  • api/src/unraid-api/graph/graph.module.ts
  • api/src/unraid-api/graph/resolvers/array/parity.mutations.resolver.ts
  • api/src/unraid-api/graph/resolvers/config/config.resolver.ts
  • api/src/unraid-api/graph/services/services.resolver.ts
  • api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
  • api/src/unraid-api/graph/shares/shares.resolver.ts
  • api/src/unraid-api/graph/resolvers/connect/connect.resolver.ts
  • api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts
  • api/src/unraid-api/graph/resolvers/display/display.resolver.ts
  • api/src/unraid-api/graph/resolvers/network/network.resolver.ts
  • api/src/unraid-api/graph/resolvers/array/array.resolver.ts
  • api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts
  • api/src/unraid-api/graph/resolvers/array/parity.resolver.ts
  • api/src/unraid-api/graph/resolvers/servers/server.resolver.ts
  • api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts
  • api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts
  • api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
api/generated-schema.graphql (5)

1427-1428: Additional query field looks good

No issues spotted; the new query field vms: Vms! with the accompanying docstring is suitably clear.


1438-1438: New health field

The new health: String! field is straightforward. Ensure a corresponding resolver or data source is implemented, if not already done.


1608-1609: No functional changes here

Only a closing brace and blank line. No concerns to flag.


1610-1616: New AuthActionVerb enum definition

Enum members (CREATE, UPDATE, DELETE, READ) adequately cover the typical actions.


1618-1622: New AuthPossession enum definition

Members (ANY, OWN, OWN_ANY) look appropriate to define access possession.

api/src/unraid-api/graph/directives/use-permissions.directive.ts (3)

1-14: Imports and re-export statements look good

The chosen imports are appropriate. Re-exporting AuthActionVerb and AuthPossession from nest-authz for convenience is a tidy approach.


34-46: Creation of AuthActionVerbEnum and AuthPossessionEnum

Using the same utility function ensures consistency. Nicely documented.


48-66: Definition of UsePermissionsDirective

Directive arguments comprehensively capture needed properties for permission checks. The field descriptions are helpful for clarity in the resulting schema.

@elibosley elibosley merged commit 45ecab6 into main Apr 14, 2025
9 checks passed
@elibosley elibosley deleted the feat/permission-endpoints branch April 14, 2025 16:27
mdatelle pushed a commit that referenced this pull request Apr 14, 2025
* usePermissions applies both authz + graphQL directive logic to allow
permissions and documentation in one place

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a new GraphQL permission directive that documents required
permissions for API fields.
- Added enums for defining action verbs (create, update, delete, read)
and possession types (any, own, own any) to enable granular access
control.
- Added a new health field to the Query type for improved API health
monitoring.

- **Chores**
- Consolidated permission handling by updating import sources and
retiring legacy authorization tests and code, enhancing overall
maintainability.
  - Updated configuration version in the API settings.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
elibosley pushed a commit that referenced this pull request Apr 24, 2025
🤖 I have created a release *beep* *boop*
---


## [4.7.0](v4.6.6...v4.7.0)
(2025-04-24)


### Features

* add basic docker network listing
([#1317](#1317))
([c4fdff8](c4fdff8))
* add permission documentation by using a custom decorator
([#1355](#1355))
([45ecab6](45ecab6))
* basic vm controls ([#1293](#1293))
([bc3ca92](bc3ca92))
* code first graphql
([#1347](#1347))
([f5724ab](f5724ab))


### Bug Fixes

* container names always null
([#1335](#1335))
([8a5b238](8a5b238))
* **deps:** update all non-major dependencies
([#1337](#1337))
([2345732](2345732))
* hide reboot notice for patch releases
([#1341](#1341))
([4b57439](4b57439))
* move docker mutations to the mutations resolver
([#1333](#1333))
([1bbe7d2](1bbe7d2))
* PR build issue
([457d338](457d338))
* remove some unused fields from the report object
([#1342](#1342))
([cd323ac](cd323ac))
* sso unreliable if API outputs more than raw json
([#1353](#1353))
([e65775f](e65775f))
* vms now can detect starting of libvirt and start local hypervisor
([#1356](#1356))
([ad0f4c8](ad0f4c8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants