-
Couldn't load subscription status.
- Fork 11
refactor: permissions system rewrite #942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 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
WalkthroughThe 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
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…d apply usePermissions decorator
There was a problem hiding this 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 permissionsapi/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Line range hint
22-42: Consider organizing providers alphabeticallyThe providers array is mostly alphabetically ordered, but the newly added
AuthResolverbreaks 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, anyapi/src/unraid-api/users/users.service.ts (1)
Line range hint
66-80: Review and update getSessionUser implementationThe
getSessionUsermethod 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:
- For
createApiKey:
- Invalid role names
- Empty role array
- Duplicate roles
- For role management methods:
- Adding already existing roles
- Removing non-existent roles
- 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 nowcasbin) could increase the bundle size. Consider:
- Analyzing the impact on build size
- Planning for removal of unused auth libraries post-migration
- 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 theIDscalar type for theidfieldFor clarity and consistency, consider using the
IDscalar type for theidfield in theApiKeytype. In GraphQL, theIDtype is specifically designed for unique identifiers.Apply this diff to update the
idfield type:type ApiKey { - id: String! + id: ID! key: String! name: String! description: String roles: [String!]! createdAt: DateTime! lastUsed: DateTime }
24-24: Suggestion: Use theIDscalar type for theuserIdfieldIn the
AddRoleForUserInput, consider using theIDscalar type for theuserIdfield to clearly indicate that it represents a unique identifier.Apply this diff:
input AddRoleForUserInput { - userId: String! + userId: ID! role: String! }
29-29: Suggestion: Use theIDscalar type for theapiKeyIdfieldFor consistency, update the
apiKeyIdfield inAddRoleForApiKeyInputto use theIDscalar type.Apply this diff:
input AddRoleForApiKeyInput { - apiKeyId: String! + apiKeyId: ID! role: String! }
34-34: Suggestion: Use theIDscalar type for theapiKeyIdfieldSimilarly, in
RemoveRoleFromApiKeyInput, consider using theIDscalar type for theapiKeyIdfield.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 infindByKeyThe
findByKeymethod 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 keysWhen 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' roleIn
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
⛔ Files ignored due to path filters (3)
api/package-lock.jsonis excluded by!**/package-lock.jsonapi/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis 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:
- Extract shared functionality into a separate module
- Use an event-based communication pattern
- 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:
- The dependency is unavoidable due to the intrinsic relationship between authentication and user management
- The services have clear, distinct responsibilities
- 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:
- The actual role/permission implementation context
- Where policies are defined
- 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 fromguest - 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:
- The directory exists and has proper permissions
- Access to this directory is properly restricted
- 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:
- The directory path is configurable via environment variable
PATHS_AUTH_KEYwith a secure default at/boot/config/plugins/dynamix.my.servers/keys - 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
- 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:
- The directory is created with appropriate permissions during application startup
- 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 ts5-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 ts2-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:
- Explicitly listing admin permissions for audit purposes
- Implementing resource-specific restrictions even for admins
- Adding logging for sensitive operations
✅ Verification successful
Wildcard admin permissions are appropriate with existing role-based safeguards
Based on the analysis of the codebase:
- All operations are protected by
@UseRolesor@UsePermissionsdecorators that explicitly define required permissions - The system implements fine-grained RBAC with specific resource/action pairs for each operation
- Critical operations are properly segregated (e.g., API key management, config updates, notifications)
- 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:
Add input validation and consider role format
Two concerns with the implementation:
- The method lacks input validation for the
apiKeyparameter. - 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:
- These hardcoded keys should be migrated to the new system
- The method should be marked as deprecated
Consider this migration approach:
- Move service accounts to the new API key system
- 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:
- Add a new
ApiKeyStrategyextendingPassportStrategyto handle API key authentication - Update the
AuthGuardto include the new strategy in its array:AuthGuard([ServerHeaderStrategy.key, UserCookieStrategy.key, ApiKeyStrategy.key]) - 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:
- The
AuthGuardproperly handles both HTTP and GraphQL contexts, extracting API key headers from either standard requests or GraphQL connection parameters - The
HeaderStrategyvalidates API keys by checking the 'x-api-key' header and delegating toAuthService.validateApiKeyCasbin() - The
AuthServiceimplements 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_KEYis correctly defined in.env.developmentwith 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:
- Loading the actual RBAC model configuration used in production
- 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:
- Documenting the migration strategy
- Setting up clear boundaries between old and new auth systems
- 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:
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:
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this 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 (
validateApiKeyCasbinandvalidateCookiesCasbin), consider adding@deprecatedJSDoc 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
📒 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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this 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 synchronizationThe 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 operationsConsider 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
📒 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
validateApiKeyCasbinmethod - Cookie strategy still uses the old
validateCookiesmethod, 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>
There was a problem hiding this 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 ensureUserRolesConsider 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
📒 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:
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:
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:
There was a problem hiding this 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
anycould 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)
📒 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
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
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:
@UseRoledecorator will need to be refactored to use the new@UsePermissionsdecorator. 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
CasbinModulefor managing authorization enforcers.CasbinServicefor initializing and managing Casbin authorization logic.localApiKeyfor improved user authentication.reportfunctionality to improve API key validation handling.Bug Fixes
Documentation
Tests
AuthServiceandAuthResolverfunctionalities.localApiKeyfield in configuration state snapshots.