Conversation
…s and authorization checks
…ed access - Updated AnnotationsController to accept projectId in relevant endpoints for task and asset annotations. - Modified AssetController to include projectId in asset retrieval, update, and deletion methods. - Enhanced AnnotationService and AssetService to validate project ownership when fetching, creating, updating, or deleting annotations and assets. - Introduced SecuritySettings for configurable presigned URL expiry in AssetService. - Removed ConfigurationController as it was deemed unnecessary. - Added AuthorizationDtos for handling authorization checks and contexts. - Updated interfaces for IAnnotationService and IAssetService to reflect changes in method signatures. - Improved logging for better traceability of project-related operations.
…t ID in method calls
… backend-driven authorization
…n checks and remove PermissionService
…n handling in API client
… for route access control
…n vPermission directive
…nd streamline permission handling
…ss various components
…nd streamline permission checks
…k-related methods and routes
There was a problem hiding this comment.
Pull Request Overview
This PR implements permission hardening by enhancing authorization security at multiple levels across the application. The changes improve project-scoped security by requiring project context for asset and annotation operations, reduce presigned URL exposure time, and migrate from client-side permission caching to server-side authorization checks.
- Server-side methods now enforce project-scoped security checks for assets and annotations
- Frontend migrates from cached permissions to real-time server authorization checks
- Presigned URL default expiry reduced from 1 hour to 5 minutes for improved security
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| IPermissionConfigurationService.cs | Adds new method for user-specific global permissions |
| IAssetService.cs | Updates interface to require project ID for all asset operations |
| IAnnotationService.cs | Updates interface to require project ID for all annotation operations |
| AssetService.cs | Implements project-scoped validation and configurable URL expiry |
| AnnotationService.cs | Implements project-scoped validation with task/asset verification |
| SecuritySettings.cs | Adds configuration for presigned URL expiry settings |
| PermissionsController.cs | New controller for backend authorization checks |
| Frontend services/stores | Removes client-side permission caching in favor of server authorization |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ILogger<AssetService> logger, | ||
| IOptions<SecuritySettings>? securityOptions = null) |
There was a problem hiding this comment.
The securityOptions parameter should not be nullable since SecuritySettings is a required configuration. Remove the ? and null default to ensure proper dependency injection validation.
| /// <summary> | ||
| /// Transfers an asset to a target data source within the same project, validating project scope. | ||
| /// </summary> |
There was a problem hiding this comment.
Duplicate summary documentation. Remove the redundant summary tags on lines 543-545 as the method already has proper documentation above.
| /// <summary> | |
| /// Transfers an asset to a target data source within the same project, validating project scope. | |
| /// </summary> |
| // Optimistically hide while checking | ||
| hideElement(el); |
There was a problem hiding this comment.
[nitpick] Optimistically hiding elements during permission checks can cause flickering and poor user experience. Consider showing a loading state or skeleton instead of completely hiding elements during authorization checks.
| * Checks if user has a global permission (available regardless of project) | ||
| */ | ||
| const hasGlobalPermission = (permission: string): boolean => { | ||
| return permissionStore.hasGlobalPermission(permission); | ||
| // Trigger async fetch; return cached value if present | ||
| void ensure(permission, { global: true }); | ||
| const key = makeKey(permission, { global: true }); | ||
| return !!resultsCache[key]; | ||
| }; | ||
|
|
||
| /** | ||
| * Checks if user has any of the specified global permissions | ||
| */ | ||
| const hasAnyGlobalPermission = (permissions: string[]): boolean => { | ||
| return permissionStore.hasAnyGlobalPermission(permissions); | ||
| permissions.forEach(p => void ensure(p, { global: true })); | ||
| return permissions.some(p => !!resultsCache[makeKey(p, { global: true })]); | ||
| }; | ||
|
|
||
| /** | ||
| * Checks if user has all of the specified global permissions | ||
| */ | ||
| const hasAllGlobalPermissions = (permissions: string[]): boolean => { | ||
| return permissionStore.hasAllGlobalPermissions(permissions); | ||
| permissions.forEach(p => void ensure(p, { global: true })); | ||
| return permissions.every(p => !!resultsCache[makeKey(p, { global: true })]); |
There was a problem hiding this comment.
The synchronous permission check methods trigger async operations but return immediately with potentially stale cache data. This creates a race condition where the first call always returns false even if the user has permission. Consider returning a default value or implementing a different pattern for initial permission checks.
No description provided.