-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add tests on granular settings permissions #12403
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
Conversation
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.
PR Summary
Added comprehensive integration tests for granular settings permissions, focusing on role-based access control and permission management.
- Added tests in
/packages/twenty-server/test/integration/graphql/suites/settings-permissions/granular-settings-permissions.ts
to verify data model and workspace operations for custom roles - Implemented test cases for permission inheritance, ensuring
canUpdateAllSettings=false
is properly overridden by specific setting permissions - Added validation for dynamic permission updates, testing the ability to add/remove setting permissions from existing roles
- Included proper test setup/teardown with Permissions V2 feature flag management
- Added test coverage for denied operations, verifying proper error handling when users lack required permissions
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
originalMemberRoleId = rolesResponse.body.data.getRoles.find( | ||
(role: any) => role.label === 'Member', | ||
).id; |
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.
logic: Unsafe type assertion with 'any'. Consider adding type guard or error handling if role is not found.
originalMemberRoleId = rolesResponse.body.data.getRoles.find( | |
(role: any) => role.label === 'Member', | |
).id; | |
const memberRole = rolesResponse.body.data.getRoles.find( | |
(role: any) => role.label === 'Member', | |
); | |
if (!memberRole) throw new Error('Member role not found'); | |
originalMemberRoleId = memberRole.id; |
const customRole = response.body.data.getRoles.find( | ||
(role: any) => role.id === customRoleId, | ||
); |
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.
logic: Add error handling for case where customRole is not found in getRoles response
const customRole = response.body.data.getRoles.find( | |
(role: any) => role.id === customRoleId, | |
); | |
const customRole = response.body.data.getRoles.find( | |
(role: any) => role.id === customRoleId, | |
); | |
if (!customRole) throw new Error('Custom role not found'); |
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:17340 This environment will automatically shut down when the PR is closed or after 5 hours. |
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.
LGTM, thanks for improving the coverage!
Closes twentyhq/core-team-issues#605 Actually settingsPermissions checks were already implemented, but we had no tests on them. In the ticket we had mentioned _TO DO: in pemissions.service we should stop calling userRoleService.getRolesByUserWorkspaces and call getRoleIdForUserWorkspace instead which relies on the cache._ But actually roleId is not enough for settings permissions because we don't store them in the cache (unlien object records permissions - which I think we had forgotten about when adding that TODO.), so we will still need to make a db call to load the role's settingsPermissions. I think it's better to make just one db call to get the role and settingsPermissions from userWorkspaceId (as currently) than to make one redis call to get roleId for userWorksapce then one db call to get role and its settingsPermissions).
Closes twentyhq/core-team-issues#605 Actually settingsPermissions checks were already implemented, but we had no tests on them. In the ticket we had mentioned _TO DO: in pemissions.service we should stop calling userRoleService.getRolesByUserWorkspaces and call getRoleIdForUserWorkspace instead which relies on the cache._ But actually roleId is not enough for settings permissions because we don't store them in the cache (unlien object records permissions - which I think we had forgotten about when adding that TODO.), so we will still need to make a db call to load the role's settingsPermissions. I think it's better to make just one db call to get the role and settingsPermissions from userWorkspaceId (as currently) than to make one redis call to get roleId for userWorksapce then one db call to get role and its settingsPermissions).
Closes twentyhq/core-team-issues#605
Actually settingsPermissions checks were already implemented, but we had no tests on them.
In the ticket we had mentioned
TO DO: in pemissions.service we should stop calling userRoleService.getRolesByUserWorkspaces and call getRoleIdForUserWorkspace instead which relies on the cache.
But actually roleId is not enough for settings permissions because we don't store them in the cache (unlien object records permissions - which I think we had forgotten about when adding that TODO.), so we will still need to make a db call to load the role's settingsPermissions. I think it's better to make just one db call to get the role and settingsPermissions from userWorkspaceId (as currently) than to make one redis call to get roleId for userWorksapce then one db call to get role and its settingsPermissions).