-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add settingsPermission gate on the frontend #10179
Conversation
e66cc9a
to
e689387
Compare
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
This PR implements a comprehensive settings permission system for controlling access to settings features and routes. Here's a summary of the key changes:
- Added
SettingsProtectedRouteWrapper
component to gate access to settings routes based on user permissions and feature flags - Introduced
SettingsNavigationSectionWrapper
andSettingsNavigationItemWrapper
to manage visibility of navigation items based on permissions - Created new hooks
useHasSettingsPermission
anduseSettingsPermissionMap
to check and manage settings permissions - Added
currentUserWorkspaceState
to store user workspace permissions in Recoil state - Integrated permission checks in
UserProviderEffect
anduseApolloFactory
for proper state management
Key points to review:
SettingsNavigationSectionWrapper
recursively checks children visibilityuseHasSettingsPermission
follows a secure default-deny pattern- Permission gates are now centralized at route/navigation level instead of individual components
- Mock data structure updated to support testing the new permissions system
- Proper integration with existing feature flag system through
useFeatureFlagsMap
19 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/settings/components/SettingsNavigationDrawerItems.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationDrawerItems.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationSectionWrapper.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationSectionWrapper.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationSectionWrapper.tsx
Outdated
Show resolved
Hide resolved
const requiredFeatureFlagEnabled = useIsFeatureEnabled( | ||
requiresFeatureFlag || null, | ||
); |
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.
style: useIsFeatureEnabled(null) could have unexpected behavior. Consider adding a guard clause or early return when requiresFeatureFlag is undefined.
...wenty-front/src/pages/settings/roles/components/RoleWorkspaceMemberPickerDropdownContent.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/testing/decorators/ObjectMetadataItemsDecorator.tsx
Show resolved
Hide resolved
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.
Early reviewing to get your opinions on raised points as it seems to re-occurs later in the review ! From so far good job !
|
||
const currentWorkspaceFeatureFlags = currentWorkspace?.featureFlags; | ||
|
||
const initialFeatureFlags = Object.fromEntries( |
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.
Nitpick: We recently introduced buildRecordFromKeysWithSameValue
which could fit our need here and avoid force cast
const initialPermissions = buildRecordFromKeysWithSameValue(Object.values(SettingsFeatures), false); // strictly typed using predicates
if (!currentUserWorkspaceSettingsPermissions) {
return initialPermissions;
}
I don't know if it's something common in the twenty codebase, if yes then just omit this comment.
Concerns: I'm always fearful to manipulate enums like this. As its reliability only comes from the fact that all entries do have an initializer.
enum FooBar {
FOO,
BAR = 'BAR',
}
const entries = Object.values(FooBar);
console.log(entries) // [LOG]: ["FOO", 0, "BAR"]
packages/twenty-front/src/modules/workspace/hooks/useFeatureFlagsMap.ts
Outdated
Show resolved
Hide resolved
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.
could not reproduce on the main branch so may be related to your changes? :
https://github.com/user-attachments/assets/dd7ed9b9-b660-4348-8392-1c920ce500f7
otherwise works fine !
packages/twenty-front/src/modules/settings/roles/hooks/useHasSettingsPermission.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/roles/hooks/useHasSettingsPermission.ts
Show resolved
Hide resolved
...wenty-front/src/pages/settings/roles/components/RoleWorkspaceMemberPickerDropdownContent.tsx
Show resolved
Hide resolved
Was fixed on main, I rebased! |
packages/twenty-front/src/modules/auth/states/currentUserWorkspaceState.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationSectionWrapper.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsProtectedRouteWrapper.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationDrawerItem.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/components/SettingsNavigationDrawerItems.tsx
Outdated
Show resolved
Hide resolved
isAdvanced?: boolean; | ||
}; | ||
|
||
export const useSettingsNavigationItems = (): SettingsNavigationSection[] => { |
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.
Way better thanks ! Also easier for maintaining granular changes about permissions and feature flags
Context
With the new permissions system, we now need to hide some items from the settings navigation and gate some routes so they can't be accessed directly.
To avoid having to set permission gates in all the component pages, I'm introducing wrapper at the route level and in the Navigation. This is not required and is mostly for pages that are strictly mapped to a single permission, for the rest we still need to use the different hooks manually but it should avoid a bit of boilerplate for most of the cases.