[Fix] Correct project manager permission validation logic #8794
Conversation
WalkthroughThis pull request introduces several enhancements across multiple Angular components and services. In the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (2)
apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts (1)
75-86: LGTM! Consider adding error handling.The implementation of
initializeManager()is clean and well-documented. It effectively sets the current user as a manager when they are an employee.Consider adding error handling for the form operations:
initializeManager() { const { employee } = this.store.user; if (employee) { + try { this.form.patchValue({ managerIds: [employee.id] }); this.form.get('managerIds').updateValueAndValidity(); + } catch (error) { + console.error('Error initializing manager:', error); + } } }apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (1)
547-564: Consider adding error handling for permission operations.The permission management in
selectProjectis well-structured but could benefit from error handling around permission operations.Consider wrapping the permission operations in a try-catch block:
if (isSelected && this.selectedProject) { const isManager = this.isManagerOfProject(this.selectedProject); const hasAllPermissions = this.hasAllPermissions([ PermissionsEnum.ALL_ORG_EDIT, PermissionsEnum.ORG_PROJECT_EDIT, PermissionsEnum.ORG_PROJECT_DELETE ]); + try { if (isManager || hasAllPermissions) { this._permissionsService.addPermission('CAN_MANAGE_PROJECT'); } else { this._permissionsService.removePermission('CAN_MANAGE_PROJECT'); } + } catch (error) { + console.error('Error managing project permissions:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.html(3 hunks)apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(5 hunks)apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (3)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (2)
284-286: LGTM! The manager check is accurate and efficient.The
isManagerOfProjectmethod correctly verifies if the current user is a manager of the project by checking their user ID against the project's manager list.
614-616: LGTM! The permission check is well-implemented.The
hasAllPermissionsmethod efficiently verifies multiple permissions usingeveryto ensure all required permissions are present.apps/gauzy/src/app/pages/projects/components/project-list/list.component.html (1)
67-108: LGTM! Permission checks are well-consolidated.The template changes effectively simplify permission management by:
- Reducing duplication in permission checks
- Using a single, consistent 'CAN_MANAGE_PROJECT' permission
- Improving template readability
|
View your CI Pipeline Execution ↗ for commit 725560f.
☁️ Nx Cloud last updated this comment at |
apps/gauzy/src/app/pages/projects/components/project-list/list.component.html
Outdated
Show resolved
Hide resolved
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts
Outdated
Show resolved
Hide resolved
apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (1)
253-253: Remove debug console.log statement.Remove the console.log statement as it's not needed in production code.
- console.log(project);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (3)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (3)
285-287: LGTM! Well-structured method for checking project manager status.The method efficiently checks if the current user is a manager of the project by comparing the user ID with the project member's employee user ID.
365-365: LGTM! Improved property access path.The change correctly accesses the employee ID through the user's employee property.
619-621: LGTM! Clean implementation of permission check.The method efficiently checks if the user has all the required permissions using Array.every().
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts (1)
36-46: Consider enhancing error handling in the permissions check flow.While the current implementation handles the basic flow well, consider adding error handling for the permissions guard check similar to what's done for the manager guard.
this._permissionsGuard.canActivate(route, state).subscribe( (hasPermissions) => { if (hasPermissions) { observer.next(true); } else { this._router.navigate([route.data['permissions'].redirectTo || '/pages/dashboard']); observer.next(false); } observer.complete(); + }, + (error) => { + console.error('Error checking permissions:', error); + this._router.navigate(['/pages/dashboard']); + observer.next(false); + observer.complete(); } );apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts (1)
29-35: Enhance type safety in permission check condition.The current implementation might be prone to type-related issues. Consider using explicit type checks.
-if ( - !this._store.user?.employee && - this._store.hasAllPermissions(PermissionsEnum.ORG_PROJECT_EDIT, PermissionsEnum.ORG_PROJECT_DELETE) -) { +if ( + this._store.user !== null && + !this._store.user.employee && + this._store.hasAllPermissions(PermissionsEnum.ORG_PROJECT_EDIT, PermissionsEnum.ORG_PROJECT_DELETE) +) {packages/ui-core/core/src/lib/services/organization/organization-projects.service.ts (1)
46-48: Consider adding error handling guidance for consumers.While the implementation is clean, consider adding error handling documentation for service consumers.
isManagerOfProject(projectId: ID, employeeId: ID): Observable<boolean> { - return this._http.get<boolean>(`${this.API_URL}/${projectId}/is-manager/${employeeId}`); + return this._http.get<boolean>(`${this.API_URL}/${projectId}/is-manager/${employeeId}`).pipe( + catchError((error) => { + console.error('Error checking project manager status:', error); + return of(false); + }) + ); }packages/core/src/lib/organization-project/organization-project.controller.ts (1)
279-297: Consider enhancing method name and error handling.The method implementation is good, but could be improved in a few areas:
- The method name
isManagercould be more descriptive, e.g.,isEmployeeProjectManager.- The API operation summary could include more details about the parameters.
- Consider adding error handling for cases where the project or employee doesn't exist.
- @ApiOperation({ - summary: 'Check if an employee is a manager of a specific project.' - }) + @ApiOperation({ + summary: 'Check if an employee is a manager of a specific project.', + description: 'Verifies whether the specified employee (by employeeId) has manager privileges for the given project (by projectId).' + }) - async isManager( + async isEmployeeProjectManager( @Param('projectId', UUIDValidationPipe) projectId: ID, @Param('employeeId', UUIDValidationPipe) employeeId: ID ): Promise<boolean> { + // Verify that both project and employee exist + const [project, employee] = await Promise.all([ + this.organizationProjectService.findOneByIdString(projectId), + this.employeeService.findOneByIdString(employeeId) + ]); + + if (!project || !employee) { + throw new NotFoundException('Project or employee not found'); + } + return await this.organizationProjectService.isManagerOfProject(projectId, employeeId); }apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (1)
548-569: Consider caching permission check results.The permission checks in the
selectProjectmethod are well-structured but could be optimized by caching the results to avoid repeated checks.+ private cachedPermissions = new Map<string, boolean>(); + + private getCachedPermissionResult(key: string, checkFn: () => boolean): boolean { + if (!this.cachedPermissions.has(key)) { + this.cachedPermissions.set(key, checkFn()); + } + return this.cachedPermissions.get(key); + } + async selectProject({ isSelected, data }): Promise<void> { try { this.disableButton = !isSelected; this.selectedProject = isSelected ? data : null; if (isSelected && this.selectedProject) { - const isManager = this.isManagerOfProject(this.selectedProject); + const isManager = this.getCachedPermissionResult( + `manager_${this.selectedProject.id}`, + () => this.isManagerOfProject(this.selectedProject) + ); - const hasAllPermissions = this.hasAllPermissions([ - PermissionsEnum.ALL_ORG_EDIT, - PermissionsEnum.ORG_PROJECT_EDIT, - PermissionsEnum.ORG_PROJECT_DELETE - ]); + const hasAllPermissions = this.getCachedPermissionResult( + 'all_permissions', + () => this.hasAllPermissions([ + PermissionsEnum.ALL_ORG_EDIT, + PermissionsEnum.ORG_PROJECT_EDIT, + PermissionsEnum.ORG_PROJECT_DELETE + ]) + );packages/core/src/lib/organization-project/organization-project.service.ts (1)
656-673: Consider enhancing error handling and query optimization.The
isManagerOfProjectmethod is well-implemented but could benefit from:
- Error handling for database query failures.
- Query optimization by selecting only necessary columns.
async isManagerOfProject(projectId: ID, employeeId: ID): Promise<boolean> { + try { const project = await this.typeOrmRepository .createQueryBuilder('project') + .select('project.id') .innerJoin('project.members', 'members') .where('project.id = :projectId', { projectId }) .andWhere('members.employeeId = :employeeId', { employeeId }) .andWhere('members.isManager = true') .getOne(); return !!project; + } catch (error) { + console.error('Error checking project manager status:', error); + throw new HttpException( + 'Failed to check project manager status', + HttpStatus.INTERNAL_SERVER_ERROR + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(7 hunks)apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts(1 hunks)apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts(1 hunks)apps/gauzy/src/app/pages/projects/projects-routing.module.ts(2 hunks)packages/core/src/lib/organization-project/organization-project.controller.ts(1 hunks)packages/core/src/lib/organization-project/organization-project.service.ts(1 hunks)packages/ui-core/core/src/lib/services/organization/organization-projects.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (5)
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts (1)
27-50: LGTM! Well-structured permission validation flow.The guard effectively implements a hierarchical permission check, first verifying project manager status before falling back to general permissions.
apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts (1)
46-61: LGTM! Well-implemented permission validation with proper error handling.The guard effectively uses RxJS operators for handling the async flow and error cases.
apps/gauzy/src/app/pages/projects/projects-routing.module.ts (1)
65-70: LGTM! Appropriate guard replacement with maintained permission requirements.The routing configuration correctly implements the new ProjectManagerPermissionGuard while maintaining the existing permission requirements.
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (2)
285-287: LGTM! Well-structured method for checking project manager status.The
isManagerOfProjectmethod is well-implemented, using a clean and efficient approach to check if the current user is a manager of the project.
619-621: LGTM! Efficient permission check implementation.The
hasAllPermissionsmethod is well-implemented, using theeveryarray method for efficient permission checking.
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/lib/organization-project/organization-project.module.ts (1)
15-31: Consider documenting the guard's purpose and usage.To improve maintainability, consider adding a comment block above the
@Moduledecorator explaining:
- The purpose of the
PermissionGuard- How it interacts with other guards (
ManagerOrPermissionGuard,ProjectManagerPermissionGuard)- The permission validation strategy
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/lib/organization-project/decorators/manager-or-permission.decorator.ts(1 hunks)packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts(1 hunks)packages/core/src/lib/organization-project/organization-project.controller.ts(2 hunks)packages/core/src/lib/organization-project/organization-project.module.ts(2 hunks)packages/core/src/lib/organization-project/organization-project.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/lib/organization-project/organization-project.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (5)
packages/core/src/lib/organization-project/organization-project.module.ts (1)
15-15: LGTM! Addition of PermissionGuard enhances access control.The integration of
PermissionGuardaligns well with the PR's objective of fixing project manager permission validation logic.Also applies to: 31-31
packages/core/src/lib/organization-project/decorators/manager-or-permission.decorator.ts (1)
1-7: LGTM! Well-structured decorator implementation.The decorator follows NestJS best practices by combining permission metadata and guard functionality using
applyDecorators. The implementation is concise and follows the single responsibility principle.packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts (1)
15-37: LGTM! Well-structured guard implementation.The guard follows NestJS best practices:
- Properly implements the
CanActivateinterface- Includes informative logging
- Has a clear fallback mechanism to the permission guard
- Handles edge cases when
employeeIdorprojectIdare missingpackages/core/src/lib/organization-project/organization-project.controller.ts (2)
45-49: LGTM! Good replacement of permission guard.The switch to
ManagerOrPermissionGuardwhile maintainingTenantPermissionGuardprovides better access control by checking both project manager status and permissions.
281-299: LGTM! Well-implemented endpoint for checking manager status.The new endpoint is:
- Well-documented with OpenAPI decorators
- Uses proper validation for UUID parameters
- Has clear and focused implementation
packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui-core/core/src/lib/resolvers/integration.resolver.ts (1)
44-51: Enhance error logging for better debugging.While the error handling is improved, consider adding more context to the error messages:
- console.warn('Access to integration is forbidden (403), continuing without integration data.'); + console.warn(`Access to integration "${name}" is forbidden (403). User: ${_store.user?.id}. Continuing without integration data.`); - console.error('Unexpected error in IntegrationResolver:', error); + console.error('Unexpected error in IntegrationResolver:', { + error, + integration: name, + organizationId, + tenantId + });apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (1)
547-568: Add error handling for permission operations.The dynamic permission management logic should include error handling for permission operations.
if (!hasAllPermissions) { const permissions = [PermissionsEnum.ORG_PROJECT_EDIT, PermissionsEnum.ORG_PROJECT_DELETE]; - permissions.forEach((permission) => - isManager - ? this._permissionsService.addPermission(permission) - : this._permissionsService.removePermission(permission) - ); + try { + await Promise.all(permissions.map(async (permission) => { + if (isManager) { + await this._permissionsService.addPermission(permission); + } else { + await this._permissionsService.removePermission(permission); + } + })); + } catch (error) { + console.error('Error updating permissions:', error); + this._errorHandlingService.handleError(error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/gauzy/src/app/pages/projects/components/project-edit/edit.component.ts(1 hunks)apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(6 hunks)apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts(1 hunks)packages/core/src/lib/organization-project/organization-project.controller.ts(3 hunks)packages/core/src/lib/organization-project/organization-project.service.ts(3 hunks)packages/ui-core/core/src/lib/resolvers/integration.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/lib/organization-project/organization-project.service.ts
- apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (4)
apps/gauzy/src/app/pages/projects/components/project-edit/edit.component.ts (1)
70-73: Remove debug console.log statement.Remove the debug console.log statement to avoid exposing debug information in production.
- console.log(integration);packages/core/src/lib/organization-project/organization-project.controller.ts (1)
281-299: Well-implemented manager check endpoint!The implementation is secure with proper parameter validation and well-documented with API metadata. The method aligns well with the permission management changes across the codebase.
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (2)
284-286: Well-implemented manager check!The implementation efficiently checks if the current user is a manager of the project.
618-620: Clean permission check implementation!The method efficiently checks for multiple permissions using Array.every().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts (1)
41-41: Add type safety for route data access.The direct access to route data properties could fail if the data structure is not as expected.
Consider adding an interface for the route data and using proper type guards:
interface ProjectRouteData { permissions?: { redirectTo?: string; }; } // Then in the guard: const routeData = route.data as ProjectRouteData; const redirectPath = routeData?.permissions?.redirectTo || '/pages/dashboard';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts(1 hunks)packages/ui-core/core/src/lib/resolvers/integration.resolver.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-core/core/src/lib/resolvers/integration.resolver.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
🔇 Additional comments (1)
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts (1)
1-15: Well-structured guard implementation!The class follows Angular best practices with proper dependency injection, typing, and scoping.
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts
Outdated
Show resolved
Hide resolved
|
@rahul This PR includes modifications to the project permission logic. The goal was to grant managers the necessary permissions to edit, delete, and manage a project. Since a manager, in the context of a project, team, or task, is not considered a standalone role but is instead identified by the Frontend:
Note: I had to modify the logic of the integration resolver because it blocked the redirection for editing. (Another way to do this?) Backend:
CC:@evereq |
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
New Features
Bug Fixes