feat: initial setup of permissions on keys#1068
Conversation
WalkthroughThe pull request introduces significant changes to the permissions and role management system in the Unraid API. This includes the removal of existing role-based access control (RBAC) files, such as Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (10)
api/src/unraid-api/auth/api-key.service.ts (3)
20-20: Consider using aMapfor efficient API key lookups.Changing
memoryApiKeysfrom aMapto anArraymay decrease the performance of lookup operations, especially as the number of API keys grows. Using aMapallows for constant-time complexity when retrieving keys by their ID or other unique fields.Apply this diff to revert back to a
Map:-protected memoryApiKeys: Array<ApiKeyWithSecret> = []; +protected memoryApiKeys: Map<string, ApiKeyWithSecret> = new Map();Update the methods that access
memoryApiKeysaccordingly to work with theMap.
98-107: Improve error handling inloadAllFromDisk.Currently, if an API key file has invalid JSON or does not conform to the schema, an error is logged, and the process continues. Consider aggregating these errors and providing a summary, or alerting administrators about corrupted or invalid files for further investigation.
127-133: Simplify the return statement infindById.The method can be simplified by returning the parsed key directly if it exists, which improves readability.
Apply this diff to streamline the method:
findById(id: string): ApiKey | null { const key = this.findByField('id', id); - if (key) { - return ApiKeySchema().parse(key); - } - return null; + return key ? ApiKeySchema().parse(key) : null; }api/src/core/utils/permissions/ensure-permission.ts (1)
6-6: Provide guidance on migrating to Casbin authorization.The deprecation notice mentions "Use casbin auth in nest instead," but without additional context or references. Consider providing a link to documentation or guidelines on how to migrate to Casbin for authorization within NestJS.
api/src/unraid-api/auth/casbin/casbin.service.ts (1)
1-1: Remove unused OnModuleInit import.The
OnModuleInitinterface is imported but not implemented by theCasbinServiceclass.-import { Injectable, InternalServerErrorException, Logger, OnModuleInit } from '@nestjs/common'; +import { Injectable, InternalServerErrorException, Logger } from '@nestjs/common';api/src/unraid-api/graph/sandbox-plugin.ts (1)
67-68: Remove unnecessary empty line.There's an extra empty line that doesn't add to code readability.
- return serverListener.renderLandingPage();api/src/unraid-api/auth/auth.service.ts (2)
32-32: Remove debugging console.log statement.Production code should not contain console.log statements.
- console.log('Permissions', apiKeyEntity)
220-220: Consider documenting the empty permissions array.Add a comment explaining why the session user starts with empty permissions despite having the ADMIN role.
roles: [Role.ADMIN], - permissions: [], + // Session users inherit permissions from their roles + permissions: [],api/src/graphql/schema/types/auth/auth.graphql (1)
1-4: Consider using an enum for actions instead of string array.The
actionsfield using[String!]allows any string value, which could lead to inconsistency and typos. Consider defining an enum type for allowed actions to ensure type safety.enum PermissionAction { CREATE READ UPDATE DELETE # Add other actions as needed } type Permission { resource: Resource! actions: [PermissionAction!] }api/src/graphql/schema/types/users/user.graphql (1)
6-6: Add documentation for the permissions field.Consider adding descriptive documentation for the permissions field in both the interface and type definitions to improve schema clarity.
interface UserAccount { + """ + List of explicit permissions granted to the account + """ permissions: [Permission!] } type User implements UserAccount { + """ + List of explicit permissions granted to the user + """ permissions: [Permission!] }Also applies to: 65-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (26)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/data/permissions.json(0 hunks)api/package.json(1 hunks)api/src/__test__/core/permissions.test.ts(0 hunks)api/src/core/index.ts(0 hunks)api/src/core/modules/get-me.ts(0 hunks)api/src/core/modules/get-permissions.ts(0 hunks)api/src/core/modules/index.ts(0 hunks)api/src/core/permission-manager.ts(0 hunks)api/src/core/permissions.ts(0 hunks)api/src/core/utils/permissions/check-permission.ts(1 hunks)api/src/core/utils/permissions/ensure-permission.ts(1 hunks)api/src/core/utils/permissions/get-permissions.ts(0 hunks)api/src/graphql/schema/types/auth/auth.graphql(2 hunks)api/src/graphql/schema/types/users/me.graphql(1 hunks)api/src/graphql/schema/types/users/user.graphql(2 hunks)api/src/unraid-api/auth/api-key.service.ts(6 hunks)api/src/unraid-api/auth/auth.service.ts(5 hunks)api/src/unraid-api/auth/casbin/casbin.service.ts(2 hunks)api/src/unraid-api/auth/user.decorator.ts(1 hunks)api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts(0 hunks)api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts(0 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)api/src/unraid-api/graph/sandbox-plugin.ts(1 hunks)
💤 Files with no reviewable changes (11)
- api/src/core/index.ts
- api/src/test/core/permissions.test.ts
- api/src/core/modules/get-me.ts
- api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts
- api/src/core/utils/permissions/get-permissions.ts
- api/src/core/modules/get-permissions.ts
- api/src/core/permission-manager.ts
- api/src/core/modules/index.ts
- api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts
- api/src/core/permissions.ts
- api/dev/data/permissions.json
🧰 Additional context used
🪛 Gitleaks (8.21.2)
api/dev/Unraid.net/myservers.cfg
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/unraid-api/auth/api-key.service.ts (1)
89-89: Initializepermissionsappropriately when creating a new API key.The
permissionsarray is initialized to an empty array, but there might be default permissions that should be assigned based on the roles or other criteria. Ensure that the permissions are set correctly for the new API key.Do you want me to assist in defining default permissions for new API keys based on roles?
api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)
13-18: Verify permission configuration aligns with business requirements.The current permission setup allows ANY possession level for reading ME resource. Consider if this aligns with your security requirements.
✅ Verification successful
Permission configuration is appropriate for the ME resource
The ANY possession level is correct for this endpoint because:
- This is a "current user" endpoint that only returns the authenticated user's own data
- The implementation uses @graphqluser() decorator which ensures the data is automatically scoped to the requesting user
- The Casbin policy confirms this is an intentional design choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ME resource permission configurations rg -l "resource: Resource.ME" | xargs rg "possession:"Length of output: 94
Script:
#!/bin/bash # Search for ME resource usage and implementation echo "=== ME Resource References ===" rg "Resource.ME" -B 2 -A 2 echo -e "\n=== ME Resolver Implementation ===" ast-grep --pattern 'class MeResolver { $$$ }'Length of output: 2516
api/src/unraid-api/auth/casbin/casbin.service.ts (1)
8-8: LGTM! Good refactor moving enforcer to class property.The refactor to use a class property for the enforcer instance improves state management and reusability.
Also applies to: 20-23
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
22-22: LGTM! Clean integration of MeResolver.The MeResolver is properly imported and added to the providers array, following the module's existing pattern.
Also applies to: 44-44
api/src/unraid-api/auth/auth.service.ts (1)
34-34: Verify permission synchronization order.The code synchronizes roles before permissions. Ensure this order is intentional and doesn't create race conditions or permission gaps.
Also applies to: 46-46
✅ Verification successful
The permission synchronization order is correct and beneficial.
The current order (roles then permissions) is intentional and provides these advantages:
- Ensures role-granted permissions are established first
- Prevents race conditions through sequential await operations
- Maintains permission consistency as custom permissions are added last
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any role-dependent permission logic in the codebase ast-grep --pattern 'if ($user.roles.$$$) { $$$ addPermission($$$) $$$ }'Length of output: 80
Script:
#!/bin/bash # Find syncApiKeyPermissions implementation ast-grep --pattern 'syncApiKeyPermissions($$$) { $$$ }' # Look for role-related sync methods and their usage rg "sync.*Role|role.*Sync" -A 5 # Get more context around the current usage rg "syncApiKeyPermissions" -A 5 -B 5Length of output: 10100
api/src/graphql/schema/types/users/me.graphql (1)
16-16: LGTM! Consistent with UserAccount interface.The nullable permissions field aligns with the interface definition and maintains type consistency.
api/package.json (1)
28-28: LGTM! Consistent with existing container management scripts.The new script follows the established pattern and provides a useful container management command.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)
35-58: 🛠️ Refactor suggestionExpand test coverage with additional scenarios.
While the basic happy path is covered, several critical test scenarios are missing:
- Permission decorator validation
- Error handling scenarios
- Required field validation
- Edge cases with missing or invalid user data
This was previously mentioned in a past review. Please add the suggested test cases to ensure robust coverage.
🧹 Nitpick comments (5)
api/src/unraid-api/auth/api-key.service.ts (2)
94-94: Consider initializing default permissions based on roles.Currently, new API keys are created with an empty permissions array. Consider deriving default permissions from the assigned roles to ensure proper access control from creation.
103-134: Consider enhancing error logging for better debugging.While the error handling is robust, consider adding more detailed logging:
- Log the total number of files processed
- Log successful API key loads
- Include file paths in error messages
api/src/unraid-api/auth/auth.service.spec.ts (1)
35-40: Consider adding more test cases for permissions.The mock data only includes a single permission. Consider adding test cases for:
- Multiple permissions
- Different resources and actions
- Edge cases like empty permissions array
api/src/unraid-api/auth/api-key.service.spec.ts (1)
56-61: Enhance test coverage for permissions-related functionality.Consider adding test cases for:
- Validation of permissions structure
- Error handling for invalid permissions
- Persistence of permissions during API key updates
api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)
10-59: Improve test suite structure and organization.Consider restructuring the test suite to group related test cases:
- Add a describe block for successful scenarios
- Add a describe block for error scenarios
- Add a describe block for permission validation
Example structure:
describe('MeResolver', () => { // Setup code... describe('Successful scenarios', () => { it('should return user information'); it('should handle minimal user data'); }); describe('Error scenarios', () => { it('should handle missing user data'); it('should handle invalid permissions'); }); describe('Permission validation', () => { it('should deny access without proper permissions'); it('should allow access with proper permissions'); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/src/unraid-api/auth/api-key.service.spec.ts(8 hunks)api/src/unraid-api/auth/api-key.service.ts(5 hunks)api/src/unraid-api/auth/auth.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/auth/api-key.service.ts (6)
17-27: Review initialization order to prevent race conditions.The
setupWatchis called in the constructor beforememoryApiKeysis populated, which could lead to inconsistent state if file changes occur beforeonModuleInitcompletes.
29-36: LGTM! Well-structured initialization with proper error handling.The implementation correctly loads API keys from disk and includes appropriate error handling and logging.
42-47: Review the file watcher implementation for potential issues.The implementation has two potential issues:
- The event handler parameters for the
changeevent should be(path) => { ... }rather than(event, path) => { ... }.- Clearing
memoryApiKeysbefore reloading may result in temporary empty state.
137-151: LGTM! Well-structured implementation with proper error handling.The method effectively uses
findByFieldfor consistency and includes appropriate error handling and schema validation.
Line range hint
158-170: Validate the field parameter to prevent invalid lookups.There's no validation for the
fieldparameter, which could lead to undefined behavior if an incorrect field name is provided.
221-224: LGTM! Correct file path handling.The implementation now correctly uses
joinfor file path construction and includes proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
api/src/graphql/schema/types/auth/auth.graphql(2 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/graphql/schema/types/auth/auth.graphql
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/graph/resolvers/me/me.resolver.ts (2)
1-7: LGTM! Well-organized imports.The imports are clean, necessary, and follow NestJS best practices with proper use of path aliases.
12-26: Add input validation and error handling.The implementation still lacks validation for user properties.
Consider implementing field-level permissions.
The current implementation uses a single permission check for all fields. Consider implementing field-level permissions for sensitive data like roles and permissions.
Example implementation:
@ResolveField() @UsePermissions({ action: AuthActionVerb.READ, resource: Resource.ROLES, possession: AuthPossession.ANY, }) public async roles() { // Implementation }Simplify the return statement using object shorthand notation.
public async me(@GraphqlUser() user: UserAccount): Promise<Me> { + const { description, permissions, id, name, roles } = user; return { - description: user.description, - permissions: user.permissions, - id: user.id, - name: user.name, - roles: user.roles, + description, + permissions, + id, + name, + roles, }; }
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
api/src/unraid-api/auth/api-key.service.ts (2)
28-29:⚠️ Potential issueMove
setupWatchinitialization toonModuleInit.The file watcher is being set up before
memoryApiKeysis populated, which could lead to race conditions. The watcher might receive events before the initial data is loaded.Apply this diff to fix the initialization order:
constructor() { this.basePath = getters.paths()['auth-keys']; ensureDirSync(this.basePath); - this.setupWatch(); } async onModuleInit() { try { this.memoryApiKeys = await this.loadAllFromDisk(); await this.createLocalApiKeyForConnectIfNecessary(); + this.setupWatch(); } catch (error) { this.logger.error('Failed to initialize API keys:', error); throw error; } }
46-50:⚠️ Potential issueFix file watcher implementation issues.
The current implementation has two issues:
- Incorrect event handler parameters for chokidar's 'change' event
- Race condition when updating
memoryApiKeysApply this diff to fix both issues:
-watch(this.basePath, { ignoreInitial: false }).on('change', async (event, path) => { - this.logger.debug(`API key storage event: ${event} on ${path}`); - this.memoryApiKeys = []; - this.memoryApiKeys = await this.loadAllFromDisk(); +watch(this.basePath, { ignoreInitial: false }).on('change', async (path) => { + this.logger.debug(`API key changed: ${path}`); + const updatedApiKeys = await this.loadAllFromDisk(); + this.memoryApiKeys = updatedApiKeys; });
🧹 Nitpick comments (4)
api/src/unraid-api/auth/api-key.service.ts (4)
97-97: Add type assertion for permissions array initialization.Consider adding a type assertion to ensure type safety of the permissions array.
-apiKey.permissions = []; +apiKey.permissions = [] as Array<{ resource: string; actions: string[] }>;
106-142: Enhance logging for Connect API key creation process.While the implementation is solid, adding more detailed logging would help with debugging and monitoring.
if (remote.apikey && (!remote.localApiKey || !(await this.findByKey(remote.localApiKey)))) { + this.logger.debug('Checking for existing Connect API key...'); const hasExistingKey = this.findByField('name', 'Connect'); if (hasExistingKey) { + this.logger.debug('Connect API key already exists'); return; } + this.logger.debug('Creating new Connect API key...');
157-174: Enhance error messages with actionable information.The error messages could be more specific about what action users or administrators should take.
-throw new Error('Authentication system error: Corrupted key file'); +throw new Error('Authentication system error: Corrupted key file. Please check the file format and restore from backup if necessary.'); -throw new Error('Invalid API key structure'); +throw new Error('Invalid API key structure. Please verify the key file matches the expected schema.'); -throw new Error('Failed to list API keys'); +throw new Error('Failed to list API keys. Please check directory permissions and ensure the auth-keys directory exists.');
178-191: Add input validation for ID parameter.Consider adding validation for the ID parameter to ensure it's a valid UUID.
findById(id: string): ApiKey | null { + if (!id || !/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id)) { + throw new GraphQLError('Invalid API key ID format'); + } try { const key = this.findByField('id', id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/index.ts(1 hunks)api/src/mothership/utils/create-local-connect-api-key.ts(0 hunks)api/src/unraid-api/auth/api-key.service.ts(4 hunks)
💤 Files with no reviewable changes (1)
- api/src/mothership/utils/create-local-connect-api-key.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/auth/api-key.service.ts (2)
31-39: Well-structured initialization with proper error handling!The implementation includes comprehensive error handling and logging, ensuring robust initialization of the API key service.
262-265: Correctly implemented file path handling!The implementation now properly uses
joinfor file path construction and includes proper formatting for the JSON output.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)
10-12: 🛠️ Refactor suggestionRemove unnecessary constructor and enhance Resolver type information.
The empty constructor serves no purpose and should be removed. Also, the
@Resolver()decorator should specify the type it resolves.-@Resolver() +@Resolver('Me') export class MeResolver { - constructor() {}🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)
14-14: Add return type to Query decorator.The
@Query()decorator should specify its return type for better type safety and documentation.- @Query() + @Query(() => Me)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/index.ts(0 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/graph/resolvers/me/me.resolver.ts
[error] 12-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)
20-34: LGTM! Input validation and error handling look good.The implementation includes proper input validation, error handling, and default values for optional fields.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
api/src/utils.ts (1)
247-261: LGTM! Consider enhancing type validation.The type predicate implementation is correct and follows TypeScript best practices. However, consider strengthening the validation for more robust type safety:
export function isUserAccount(user: any): user is UserAccount { return ( user && - typeof user.id === 'string' && - typeof user.name === 'string' && - Array.isArray(user.permissions) && - Array.isArray(user.roles) + typeof user.id === 'string' && user.id.length > 0 && + typeof user.name === 'string' && user.name.length > 0 && + Array.isArray(user.permissions) && user.permissions.every(p => typeof p === 'string') && + Array.isArray(user.roles) && user.roles.every(r => typeof r === 'string') ); }This enhancement:
- Ensures non-empty strings for
idandname- Validates that array elements are strings
api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (1)
37-39: Expand the basic test coverage.While checking if the resolver is defined is a good start, consider adding more basic validation tests:
- Verify resolver instance type
- Verify AuthZService injection
Example tests to add:
it('should be instance of MeResolver', () => { expect(resolver).toBeInstanceOf(MeResolver); }); it('should have AuthZService injected', () => { expect(authzService).toBeDefined(); expect(authzService.checkPermission).toBeDefined(); });api/src/unraid-api/auth/auth.service.ts (1)
32-32: Remove debug console.log statement.Production code should use the logger service instead of console.log for consistency and proper log level management.
- console.log('Permissions', apiKeyEntity); + this.logger.debug('API Key permissions:', apiKeyEntity);api/src/unraid-api/auth/api-key.service.ts (3)
22-22: Consider using Map for better lookup performance.Changing
memoryApiKeysfromMaptoArraymight impact performance for key lookups. UsingArray.find()has O(n) complexity, whileMap.get()has O(1).- protected memoryApiKeys: Array<ApiKeyWithSecret> = []; + protected memoryApiKeys: Map<string, ApiKeyWithSecret> = new Map();
35-37: Enhance error logging for initialization failures.The current error logging could be more detailed to help with troubleshooting initialization issues.
- this.logger.error('Failed to initialize API keys:', error); + this.logger.error('Failed to initialize API keys:', { + error: error instanceof Error ? error.message : String(error), + stack: error instanceof Error ? error.stack : undefined, + phase: 'initialization' + });
148-166: Optimize file loading for large directories.Consider using Promise.all for parallel file reading to improve performance with many API keys.
- for (const file of files) { - if (file.endsWith('.json')) { - try { - const content = await readFile(join(this.basePath, file), 'utf8'); - const apiKey = ApiKeyWithSecretSchema().parse(JSON.parse(content)); - apiKeys.push(apiKey); - } catch (error) { + const readPromises = files + .filter(file => file.endsWith('.json')) + .map(async file => { + try { + const content = await readFile(join(this.basePath, file), 'utf8'); + return ApiKeyWithSecretSchema().parse(JSON.parse(content)); + } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
api/src/unraid-api/auth/api-key.service.ts(4 hunks)api/src/unraid-api/auth/auth.service.ts(5 hunks)api/src/unraid-api/auth/user.decorator.ts(1 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts(1 hunks)api/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/auth/user.decorator.ts
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/auth/api-key.service.ts
[error] 200-200: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/src/unraid-api/auth/auth.service.ts (2)
100-114: Implementation looks good!The syncApiKeyPermissions method follows best practices with proper permission cleanup, parallel execution, and error handling.
226-226:⚠️ Potential issueVerify session user permissions.
The session user has the ADMIN role but returns an empty permissions array. This seems inconsistent with the method's comment that states "Session receives administrator permissions". This could lead to unexpected authorization issues.
Consider either:
- Syncing permissions based on the ADMIN role, or
- Updating the method's documentation to clarify the permission model
- permissions: [], + // TODO: Sync these permissions with ADMIN role capabilities + permissions: [ + { + resource: '*', + actions: ['*'] + } + ],api/src/unraid-api/auth/api-key.service.ts (3)
48-49: Avoid clearingmemoryApiKeysbefore reloading to prevent race conditions.Clearing
memoryApiKeysbefore reloading may result in temporary empty state.
266-269: LGTM! Proper file path handling implemented.The implementation correctly uses path.join and proper JSON formatting.
200-201: 🛠️ Refactor suggestionUse Object.hasOwn instead of hasOwnProperty.
Replace Object.prototype.hasOwnProperty with Object.hasOwn for better safety and maintainability.
- if (!ApiKeyWithSecretSchema().shape.hasOwnProperty(field)) { + if (!Object.hasOwn(ApiKeyWithSecretSchema().shape, field)) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 200-200: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/src/unraid-api/auth/api-key.service.ts (3)
26-28:⚠️ Potential issueSet secure file permissions for API key storage.
The API key storage directory needs proper file permissions to prevent unauthorized access.
Apply this diff to set secure file permissions:
constructor() { this.basePath = getters.paths()['auth-keys']; ensureDirSync(this.basePath); + chmodSync(this.basePath, 0o700); }Ensure you've imported the necessary function:
import { chmodSync } from 'fs';
45-51:⚠️ Potential issueFix race condition in setupWatch and add cleanup.
The current implementation has two issues:
- Race condition when clearing and reloading memoryApiKeys
- No cleanup of the watcher when the service is destroyed
Apply this diff to fix these issues:
+private watcher: ReturnType<typeof watch> | null = null; + private setupWatch() { - watch(this.basePath, { ignoreInitial: false }).on('change', async (path) => { + this.watcher = watch(this.basePath, { ignoreInitial: false }); + this.watcher.on('change', async (path) => { this.logger.debug(`API key changed: ${path}`); - this.memoryApiKeys = []; - this.memoryApiKeys = await this.loadAllFromDisk(); + const updatedKeys = await this.loadAllFromDisk(); + this.memoryApiKeys = updatedKeys; }); } + +async onModuleDestroy() { + if (this.watcher) { + await this.watcher.close(); + this.watcher = null; + } +}Don't forget to implement OnModuleDestroy:
-export class ApiKeyService implements OnModuleInit { +export class ApiKeyService implements OnModuleInit, OnModuleDestroy {
106-142:⚠️ Potential issueImplement atomic operations for Connect API key creation.
The current implementation has potential race conditions and non-atomic operations when creating and updating the Connect API key.
Apply this diff to implement atomic operations:
private async createLocalApiKeyForConnectIfNecessary() { if (getters.config().status !== FileLoadStatus.LOADED) { this.logger.error('Config file not loaded, cannot create local API key'); return; } const { remote } = getters.config(); - if (remote.apikey && (!remote.localApiKey || !(await this.findByKey(remote.localApiKey)))) { - const hasExistingKey = this.findByField('name', 'Connect'); - - if (hasExistingKey) { - return; - } + if (!remote.apikey || (remote.localApiKey && await this.findByKey(remote.localApiKey))) { + return; + } + + const existingKey = this.findByField('name', 'Connect'); + if (existingKey) { + return; + } try { + // Start atomic operation const localApiKey = await this.create( 'Connect', 'API key for Connect user', [Role.CONNECT], true ); if (!localApiKey?.key) { throw new Error('Failed to create local API key - no key returned'); } - if (localApiKey?.key) { - store.dispatch( - updateUserConfig({ - remote: { - localApiKey: localApiKey.key, - }, - }) - ); - } else { - this.logger.error('Failed to create local API key - no key returned'); - throw new Error('Failed to create local API key'); - } + await store.dispatch( + updateUserConfig({ + remote: { + localApiKey: localApiKey.key, + }, + }) + ); + } catch (error) { + // Rollback on failure + if (localApiKey?.id) { + await this.deleteApiKey(localApiKey.id); + } + throw error; } }
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts (4)
21-28: Enhance type safety and verification in AuthZService mock.The current mock implementation could be improved for better type safety and verification:
checkPermission: vi.fn().mockImplementation((action, resource, possession) => { - // Return false for specific test scenarios - if (action === 'write' && resource === Resource.ME) { - return Promise.resolve(false); - } - return Promise.resolve(true); + // Verify possession parameter + if (!['own', 'any'].includes(possession)) { + throw new Error(`Invalid possession: ${possession}`); + } + + // Type-safe resource check + if (!Object.values(Resource).includes(resource)) { + throw new Error(`Invalid resource: ${resource}`); + } + + // Type-safe action check + const validActions = ['read', 'write', 'delete']; + if (!validActions.includes(action)) { + throw new Error(`Invalid action: ${action}`); + } + + return Promise.resolve(action !== 'write' || resource !== Resource.ME); }),
37-39: Enhance basic resolver verification.While checking if the resolver is defined is good, consider adding more basic verification:
it('should be defined', () => { expect(resolver).toBeDefined(); + expect(resolver).toBeInstanceOf(MeResolver); + expect(resolver.me).toBeInstanceOf(Function); + expect(authzService).toBeDefined(); + expect(authzService.checkPermission).toBeInstanceOf(Function); });
57-64: Remove redundant assertions.The block-level equality check is redundant with the individual property checks that follow:
- expect(result).toEqual({ - id: mockUser.id, - name: mockUser.name, - description: mockUser.description, - roles: mockUser.roles, - permissions: mockUser.permissions, - } as Me); - expect(result).toBeDefined(); + expect(result).toBeDefined();
69-73: Avoid non-null assertion operator and improve type safety.Replace non-null assertions with proper type checking:
- expect(result.permissions!).toHaveLength(1); - expect(result.permissions![0]).toEqual({ + expect(result.permissions).toBeDefined(); + expect(result.permissions).toHaveLength(1); + + const [permission] = result.permissions ?? []; + expect(permission).toBeDefined(); + expect(permission).toEqual({ resource: Resource.ME, actions: expect.arrayContaining(['read']), });api/src/unraid-api/auth/api-key.service.ts (1)
144-176: Improve file handling in loadAllFromDisk.The current implementation could be optimized to handle file resources better and provide more detailed error messages.
Apply this diff to improve file handling:
async loadAllFromDisk(): Promise<ApiKeyWithSecret[]> { + const apiKeys: ApiKeyWithSecret[] = []; try { const files = await readdir(this.basePath); - const apiKeys: ApiKeyWithSecret[] = []; for (const file of files) { if (file.endsWith('.json')) { + const filePath = join(this.basePath, file); try { - const content = await readFile(join(this.basePath, file), 'utf8'); + const content = await readFile(filePath, 'utf8'); const apiKey = ApiKeyWithSecretSchema().parse(JSON.parse(content)); apiKeys.push(apiKey); } catch (error) { if (error instanceof SyntaxError) { - throw new Error('Authentication system error: Corrupted key file'); + throw new Error(`Authentication system error: Corrupted key file ${file}`); } if (error instanceof ZodError) { this.logger.error(`Invalid API key structure in file ${file}`, error.errors); - throw new Error('Invalid API key structure'); + throw new Error(`Invalid API key structure in file ${file}`); } - this.logger.warn(`Error reading API key file ${file}: ${error}`); + const errorMessage = error instanceof Error ? error.message : String(error); + this.logger.warn(`Error reading API key file ${file}: ${errorMessage}`); } } } - return apiKeys; } catch (error) { if (error instanceof Error) { throw error; } this.logger.error(`Failed to read API key directory: ${error}`); throw new Error('Failed to list API keys'); } + return apiKeys; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/auth/api-key.service.ts(4 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
276-279:⚠️ Potential issueSet secure file permissions when saving API keys.
API key files should have restricted permissions to prevent unauthorized access.
Apply this diff to set secure file permissions:
await writeFile( join(this.basePath, `${validatedApiKey.id}.json`), JSON.stringify(sortedApiKey, null, 2) ); +await chmodSync(join(this.basePath, `${validatedApiKey.id}.json`), 0o600);Likely invalid or redundant comment.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
3 participants
Summary by CodeRabbit
Release Notes
Permission Management
API Key Management
Authentication
MeResolverfor retrieving user information.GraphQL Schema
Permissiontype for more precise access control.Testing
MeResolver.Chores