-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Enhance role-check system with stricter checks #15392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add new granular permissions: Applications, Layouts, AI, Upload File, Download File - Implement whitelist-based workspace field validation for improved security - Add SettingsPermissionsGuard to core entity resolvers (webhooks, files, domains, etc.) - Create CustomPermissionGuard and NoPermissionGuard for explicit permission documentation - Enhance ESLint rule to enforce permission guards on all mutations - Update frontend Settings UI with new permission controls - Add PermissionsModule to all affected resolver modules This improves the security posture by ensuring all mutations have explicit permission requirements and prevents unauthorized access to sensitive operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR introduces granular role-based access control by adding 5 new permissions (APPLICATIONS, LAYOUTS, AI, UPLOAD_FILE, DOWNLOAD_FILE) and implementing comprehensive security enhancements across the platform.
Key Changes:
- Added new guard types (
CustomPermissionGuard,NoPermissionGuard) to explicitly document permission handling patterns - Implemented whitelist-based field validation in
WorkspaceServiceusingWORKSPACE_FIELD_PERMISSIONSmap - prevents unauthorized field updates - Applied permission guards to 7+ core resolvers (applications, layouts, files, webhooks, AI agents, postgres credentials, domains)
- Enhanced ESLint rule to enforce permission guards on all mutations during development
- Updated frontend UI with new permission controls in settings
Security Improvements:
- Workspace field updates now require specific permissions per field (e.g.,
subdomainrequiresWORKSPACE,inviteHashrequiresWORKSPACE_MEMBERS) - Unknown fields are rejected with clear error messages
- Permission validation grouped by permission type for efficiency
- All mutations now have explicit permission guards (enforced by ESLint)
Confidence Score: 5/5
- This PR is safe to merge - it strengthens security through comprehensive permission validation
- The implementation is thorough and well-architected: whitelist-based validation prevents unauthorized updates, new guard types provide clear documentation, ESLint enforcement catches missing guards during development, and all changes follow established patterns
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/src/engine/metadata-modules/permissions/constants/permission-flag-type.constants.ts | 5/5 | Added new permission types: APPLICATIONS, LAYOUTS, AI, UPLOAD_FILE, DOWNLOAD_FILE |
| packages/twenty-server/src/engine/guards/custom-permission.guard.ts | 5/5 | New guard for endpoints with custom permission logic (always returns true, serves as documentation) |
| packages/twenty-server/src/engine/guards/no-permission.guard.ts | 5/5 | New guard for special cases that intentionally bypass permission validation (onboarding flows, etc.) |
| packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts | 5/5 | Implemented whitelist-based validation for workspace field updates using WORKSPACE_FIELD_PERMISSIONS map |
| tools/eslint-rules/rules/graphql-resolvers-should-be-guarded.ts | 5/5 | Enhanced ESLint rule to require permission guards on mutations (accepts SettingsPermissionsGuard, CustomPermissionGuard, or NoPermissionGuard) |
Sequence Diagram
sequenceDiagram
participant Client
participant Resolver
participant AuthGuard
participant PermissionGuard
participant PermissionsService
participant WorkspaceService
Client->>Resolver: GraphQL Mutation Request
Resolver->>AuthGuard: @UseGuards(WorkspaceAuthGuard)
AuthGuard->>AuthGuard: Verify authentication
AuthGuard-->>Resolver: User authenticated
alt Standard Permission Guard
Resolver->>PermissionGuard: @UseGuards(SettingsPermissionsGuard(PERMISSION_TYPE))
PermissionGuard->>PermissionsService: Check user permission
PermissionsService->>PermissionsService: Query role permissions
alt Has Permission
PermissionsService-->>PermissionGuard: Permission granted
PermissionGuard-->>Resolver: Access allowed
Resolver->>WorkspaceService: Execute business logic
WorkspaceService-->>Resolver: Return result
Resolver-->>Client: Success response
else No Permission
PermissionsService-->>PermissionGuard: Permission denied
PermissionGuard-->>Client: 403 Forbidden
end
else Custom Permission Guard
Resolver->>PermissionGuard: @UseGuards(CustomPermissionGuard)
PermissionGuard-->>Resolver: Always allow (marker)
Resolver->>WorkspaceService: Execute with custom validation
WorkspaceService->>WorkspaceService: validateWorkspaceUpdatePermissions()
WorkspaceService->>WorkspaceService: Check WORKSPACE_FIELD_PERMISSIONS whitelist
alt Field in whitelist
WorkspaceService->>PermissionsService: Check field-specific permission
alt Has Permission
PermissionsService-->>WorkspaceService: Permission granted
WorkspaceService-->>Resolver: Return result
Resolver-->>Client: Success response
else No Permission
PermissionsService-->>WorkspaceService: Permission denied
WorkspaceService-->>Client: 403 Forbidden with field list
end
else Field not in whitelist
WorkspaceService-->>Client: 403 Field not allowed
end
else No Permission Guard
Resolver->>PermissionGuard: @UseGuards(NoPermissionGuard)
PermissionGuard-->>Resolver: Always allow (special cases)
Resolver->>WorkspaceService: Execute without permission check
WorkspaceService-->>Resolver: Return result
Resolver-->>Client: Success response
end
35 files reviewed, no comments
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:47578 This environment will automatically shut down when the PR is closed or after 5 hours. |
- Introduced new permission flags: BILLING and MANAGE_VIEWS. - Updated GraphQL enums and metadata to include new permissions. - Enhanced SettingsRolePermissionsSettingsSection to manage new permissions. - Adjusted various resolvers to utilize the new permission flags for improved access control. This update strengthens the permission system by providing more granular control over billing and view management functionalities.
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Enum value ADMIN_PANEL was removed from enum PermissionFlagType GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Enum value ADMIN_PANEL was removed from enum PermissionFlagType
|
...oles/role-permissions/permission-flags/components/SettingsRolePermissionsSettingsSection.tsx
Show resolved
Hide resolved
| Icon: IconLayoutSidebarRightCollapse, | ||
| }, | ||
| { | ||
| key: PermissionFlagType.MANAGE_VIEWS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we want to do that one right we need a comprehensive work in the FE.
For instance at the moment when a user choose to hide or show a field on a view, it updates the view.
I think a user that does not have the MANAGE_VIEW permission should still be able to display a field that is being hidden, but that it should not update the view for everyone.
Overall I think the introduction of this permission would be less painful for users who won't have it if we introduce private / common views, otherwise users without the permissions will be restricted in their attempts to display data as they want to (if they need to hide / show a field each time they visit a view for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made two changes that should help:
- Moved Views from Settings to Tools which means it's now provisioned by default for Member roles (including existing)
- Made changes on the frontend so that changes are not persisted when the permission is missing, that way the user can still resize or move columns but it's only a local change
I agree private views would be nice as a followup.
packages/twenty-server/src/engine/core-modules/billing/billing.resolver.ts
Show resolved
Hide resolved
| @UseGuards(WorkspaceAuthGuard) | ||
| @UseGuards( | ||
| WorkspaceAuthGuard, | ||
| SettingsPermissionsGuard(PermissionFlagType.WORKFLOWS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this linked to workflows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomtrp could you please double check? I assumed it was but if you can double check I'm not missing anything, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are planning to migrate workflows on that entity but for now I think this is only used by applications. Could you confirm @martmull?
|
TODO: Don't load agent threads/ask ai if there's no AI permission |
Overview
This PR strengthens our permission system by introducing more granular role-based access control across the platform.
Changes
New Permissions Added
Security Enhancements
CustomPermissionGuardandNoPermissionGuardfor better code documentationAffected Components
Developer Experience
Testing
Migration Notes
Existing workspaces will need to assign the new permissions to roles as needed. By default, all new permissions are set to
falsefor non-admin roles.