[Fix] Project Manager Or Permission Validation #8794
Conversation
WalkthroughThis pull request introduces several permission and role management enhancements in both frontend and backend modules. On the frontend, new methods and guards enable granular user permission checks during project selection and team management. Backend API endpoints and services are updated to support managerial status evaluations and include refined error handling. Core guard classes have been refactored to centralize access logic. Routing has also been modified to incorporate the new guard for project editing. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PLC as ProjectListComponent
participant PS as NgxPermissionsService
participant OPS as OrganizationProjectsService
U->>PLC: selectProject(project)
PLC->>PLC: isManagerOfProject(project)
alt User is Manager
PLC->>PS: addPermission(CAN_MANAGE_PROJECT)
else Not Manager
PLC->>PLC: _hasAllPermissions(permissions)
end
PLC->>PLC: Update component state
sequenceDiagram
participant R as Router
participant PMGuard as ProjectManagerOrPermissionGuard
participant OPS as OrganizationProjectService
participant PGuard as PermissionGuard
R->>PMGuard: canActivate(route, state)
PMGuard->>OPS: isManagerOfProject(projectId, employeeId)
alt User is Manager
PMGuard-->>R: Allow access
else Not Manager
PMGuard->>PGuard: Check additional permissions
PGuard-->>PMGuard: Return permission result
PMGuard-->>R: Allow or deny access based on result
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
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 2cece68.
☁️ 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
Outdated
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 |
apps/gauzy/src/app/pages/projects/components/project-edit/edit.component.ts
Outdated
Show resolved
Hide resolved
apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts
Outdated
Show resolved
Hide resolved
apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/organization-project/organization-project.module.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/organization-project/organization-project.service.ts
Show resolved
Hide resolved
| @@ -41,17 +41,17 @@ export const IntegrationResolver: ResolveFn<Observable<IIntegrationTenant | bool | |||
|
|
|||
| // Handle errors and redirect if an error occurs | |||
| return integration$.pipe( | |||
| catchError(() => { | |||
There was a problem hiding this comment.
@samuelmbabhazi Why you changed integration.resolver logic here?
There was a problem hiding this comment.
@samuelmbabhazi Why you changed integration.resolver logic here?
I change this logic since the employee does not have the necessary permissions to access it and this blocks the redirection for editing,
Here I allow him to access the project without the data coming from the integrations,
Here the question is whether the permissions for the integration are necessary and for our case this data is it really associated with a specific project
There was a problem hiding this comment.
@samuelmbabhazi That integration.resolver we used multiple places you completly changed the logic and make it work here, but broken everywhere we used it. So please revert it.
There was a problem hiding this comment.
After your chages, still having same issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts (3)
11-17: Replace console.log with a proper logging service.The constructor properly extends PermissionGuard and passes the required dependencies to the parent class. However, consider replacing all console.log statements with a proper logging service such as NestJS's built-in Logger for better observability and control in production environments.
28-37: Add more robust validation for employeeId and projectId.The current implementation checks if both
employeeIdandprojectIdexist, but doesn't validate their formats. Consider adding more robust validation to prevent potential issues with invalid IDs.// Get user details from the request context const { employeeId } = request.user; // Check if the user is a project manager -if (employeeId && projectId) { +if (employeeId && projectId && isUUID(projectId)) { const isManager = await this._projectService.isManagerOfProject(projectId, employeeId); if (isManager) { console.log(`✅ User (employeeId: ${employeeId}) is manager of project ${projectId}, access granted.`); return true; } }You'll need to import isUUID from 'class-validator' or implement your own validation approach.
21-41: Replace console.log statements with proper logging.Replace all console.log statements with a proper logging service, such as NestJS's built-in Logger. This will provide better control over log levels and formatting in different environments.
+import { Logger } from '@nestjs/common'; @Injectable() export class ManagerOrPermissionGuard extends PermissionGuard implements CanActivate { + private readonly logger = new Logger(ManagerOrPermissionGuard.name); constructor( // ...existing constructor ) { super(cacheManager, reflector, rolePermissionService); } async canActivate(context: ExecutionContext): Promise<boolean> { - console.log('✅ ManagerOrPermissionGuard canActivate called'); + this.logger.debug('ManagerOrPermissionGuard canActivate called'); // ...existing code if (isManager) { - console.log(`✅ User (employeeId: ${employeeId}) is manager of project ${projectId}, access granted.`); + this.logger.debug(`User (employeeId: ${employeeId}) is manager of project ${projectId}, access granted.`); return true; } // If the user is not a manager, delegate the check to PermissionGuard - console.log(`🔄 User is not a manager, deferring to PermissionGuard.`); + this.logger.debug(`User is not a manager, deferring to PermissionGuard.`); return super.canActivate(context); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/gauzy/src/app/pages/projects/components/project-edit/edit.component.ts(1 hunks)apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts(1 hunks)apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts(1 hunks)packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/gauzy/src/app/pages/projects/components/project-edit/edit.component.ts
- apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts
- apps/gauzy/src/app/pages/projects/guards/project-manager-permission.guard.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (2)
packages/core/src/lib/organization-project/guards/manager-or-permission.guard.ts (2)
12-12: Remove unused CACHE_MANAGER parameter name.The cache manager is injected but never directly used in this class's implementation. While it's passed to the parent class, consider removing the parameter name to avoid confusion.
- @Inject(CACHE_MANAGER) cacheManager: Cache, + @Inject(CACHE_MANAGER) _: Cache,
20-42: Good implementation of the manager-first permission check approach.The
canActivatemethod correctly implements the PR objectives by first checking if a user is a project manager and granting access immediately if true, before falling back to standard permission checks. This is a clean implementation of the requirement.
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)
87-105: Consider preserving existing managers.
The current implementation overwrites any existing manager IDs with just the current employee's ID. If multiple managers can exist, switch to an additive approach to avoid unintentionally discarding other managers.Here is an example diff to preserve existing managers while adding the current employee:
- managerControl.patchValue([employee.id]); + const currentManagers = new Set(managerControl.value || []); + currentManagers.add(employee.id); + managerControl.patchValue([...currentManagers]);packages/core/src/lib/organization-project/guards/project-manager-or-permission.guard.ts (1)
41-45: Consider upgrading console logs to a logger.
Use a standardized logger instead ofconsole.logto ensure consistent formatting and log levels (info, warn, error) in production code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(6 hunks)apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts(2 hunks)packages/core/src/lib/organization-project/decorators/manager-or-permission.decorator.ts(1 hunks)packages/core/src/lib/organization-project/guards/project-manager-or-permission.guard.ts(1 hunks)packages/core/src/lib/organization-project/organization-project.controller.ts(9 hunks)packages/core/src/lib/organization-project/organization-project.service.ts(6 hunks)packages/core/src/lib/shared/guards/base.guard.ts(1 hunks)packages/core/src/lib/shared/guards/permission.guard.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/lib/organization-project/decorators/manager-or-permission.decorator.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 (16)
apps/gauzy/src/app/pages/teams/teams-mutation/teams-mutation.component.ts (2)
5-14: Imports look good.
No issues found with newly added imports forIEmployee,IOrganization,IImageAsset,IOrganizationProject,IOrganizationTeam,ITag,IUser, andPermissionsEnum.
82-85: Double-check permission-driven logic.
Currently, the default manager is set if the user lacksCHANGE_SELECTED_EMPLOYEEpermission. Please confirm that this aligns with the intended business logic (i.e., if they cannot change the employee, they automatically become a manager).packages/core/src/lib/shared/guards/base.guard.ts (1)
12-15: Placeholder authorization logic.
The guard is allowing all requests by default. Ensure that you override or implement actual checks in extending classes, or replace this placeholder to enforce real authorization rules.packages/core/src/lib/organization-project/guards/project-manager-or-permission.guard.ts (1)
36-38: Fallback strategy is valid.
Gracefully falling back toPermissionGuardwhenemployeeIdorprojectIdis missing is a solid approach to avoid unexpected breakage.apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (3)
283-294: Well-implemented manager check functionThis function correctly checks if the current user is a manager of a given project by examining project members and comparing with the current user ID. The boolean return type and clear naming make the function purpose explicit.
555-577: Good implementation of dynamic permission handlingThe permission management logic is well-structured. The code dynamically assigns or removes permissions based on whether the user is a project manager or has the necessary global permissions. This approach ensures managers can edit/delete projects even without explicit permissions.
However, consider adding a cleanup mechanism to remove temporary permissions when navigating away from the project context:
+// Add a method to clean up temporary permissions +cleanupTemporaryPermissions(): void { + const permissions = [PermissionsEnum.ORG_PROJECT_EDIT, PermissionsEnum.ORG_PROJECT_DELETE]; + permissions.forEach((permission: PermissionsEnum) => + this._permissionsService.removePermission(permission) + ); +} +// Call this method in ngOnDestroy or when navigating away
628-630: Efficient permission check utility methodThis utility function elegantly checks if all specified permissions are present using Array.every() for concise evaluation.
packages/core/src/lib/shared/guards/permission.guard.ts (2)
15-22: Good inheritance structure with proper access modifiersExtending BaseGuard and changing access modifiers from private to protected enables inheritance functionality while maintaining encapsulation. This change allows subclasses like ProjectManagerOrPermissionGuard to access these properties while preventing external access.
57-57: Updated variable name for consistencyThe renamed
_cacheManagervariable is now used correctly, maintaining the caching functionality while aligning with naming conventions.packages/core/src/lib/organization-project/organization-project.service.ts (3)
179-179: Improved entity destructuringThe destructuring now includes
...entityto collect all remaining properties, which is a more flexible and maintainable approach.
189-207: Enhanced member update logic with conditional processingThe conditional check for array types before processing members prevents unnecessary updates when only partial data is provided (e.g., when just updating project visibility). This addresses the issue noted in the past review comment.
662-681: Well-implemented manager check methodThis method efficiently queries the database to determine if an employee is a manager of a specific project. The query correctly filters by project ID, employee ID, manager status, and active/archived flags, ensuring accurate results.
packages/core/src/lib/organization-project/organization-project.controller.ts (4)
48-49: Improved guard implementation for project permissionsReplacing the PermissionGuard with ProjectManagerOrPermissionGuard enhances the security model by first checking if a user is a project manager before falling back to permission checks. This aligns with the PR objective of correcting project manager permission validation logic.
92-97: Method renamed for better clarityRenaming from
findByEmployeetofindProjectsByEmployeeimproves API readability by being more explicit about what the endpoint returns.
124-126: Method renamed for better clarityRenaming from
updateByEmployeetoupdateProjectByEmployeeprovides better clarity about the endpoint's purpose.
359-392: Well-implemented project manager check endpointThis new endpoint provides a clean way to verify if an employee is a project manager, which is essential for the frontend permission checks. The endpoint is well-documented with appropriate parameters and return type.
rahul-rocket
changed the title
| import { PermissionsEnum } from '@gauzy/contracts'; | ||
| import { ProjectManagerOrPermissionGuard } from '../guards/project-manager-or-permission.guard'; | ||
|
|
||
| export const ManagerOrPermissions = (...permissions: PermissionsEnum[]) => |
There was a problem hiding this comment.
@samuelmbabhazi Please remove this decorators if not needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/gauzy/src/app/pages/projects/guards/project-manager.guard.ts (1)
1-68: Thorough implementation of project manager verification guardThis guard has a well-designed implementation that follows Angular best practices for route protection. It properly handles multiple conditions to determine if a user should be granted access:
- First checks for existence of a project ID
- Checks for admin permissions (edit rights and employee selection rights)
- If not an admin, verifies if the user is a manager of the specific project
The console log on line 51 should be removed as it exposes debug information that shouldn't be present in production code:
- console.log(`✅ Access granted: User (employeeId: ${employee?.id}) is manager of project ${projectId}.`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts(6 hunks)apps/gauzy/src/app/pages/projects/guards/project-manager-or-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)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gauzy/src/app/pages/projects/projects-routing.module.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (6)
apps/gauzy/src/app/pages/projects/guards/project-manager-or-permission.guard.ts (1)
1-43: Well-structured guard implementation combining project manager status and permissionsThe implementation of this guard provides an elegant solution to the use case mentioned in the PR objectives. It properly checks if a user is a project manager first, and only if that fails, it falls back to checking regular permissions.
Some noteworthy aspects:
- Good use of RxJS operators (
switchMap,tap,catchError)- Comprehensive error handling
- Clear documentation explaining the logic flow
- Proper redirection handling based on route data
The guard effectively combines the functionality of two separate guards, making the authorization flow cleaner and more maintainable.
apps/gauzy/src/app/pages/projects/components/project-list/list.component.ts (5)
289-294: Effective implementation of manager detectionThis method efficiently checks if the current user is a manager of a project by filtering the project members. The implementation is concise and focuses on the specific condition that needs to be verified.
555-577: Robust temporary permissions handling based on project roleThe implementation matches the PR objective description well by dynamically granting or removing permissions based on the user's role in the selected project.
When a user selects a project:
- It checks if they're a manager of that specific project
- It verifies if they already have all required permissions
- If they don't have all permissions but are a manager, it temporarily grants the necessary edit and delete permissions
- If they're not a manager, it ensures these permissions are removed
This approach correctly implements the requirement of granting temporary permissions only when a project is selected and only if the user is a manager of that specific project.
634-637: Efficient permission checking utility methodThe
_hasAllPermissionsmethod is well implemented usingArray.prototype.every()which provides early termination on the first failure. This is more efficient than checking each permission individually and combining the results.
6-6: Dependencies updated properly for permission managementThe component has been correctly updated to include the necessary imports and dependencies:
- RxJS operators for handling reactive streams
- NgxPermissionsService for dynamic permission management
- OrganizationProjectStore for project state management
These additions align well with the new functionality being added to the component.
Also applies to: 9-9, 89-90
372-372: Fixed property access path for employee IDThis change correctly updates the property access path to handle the nested structure of the user object.
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
Refactor