Feature flags env variable gating #9481
Conversation
Added support for multi-workspace checks during impersonation and streamlined token verification flows. Adjusted session clearing and navigation logic to improve user transition between workspaces.
…nation flow Simplified test assertions in `useAuth.test.tsx` by removing redundant property checks from the signup method. Enhanced `useImpersonate` hook by refactoring workspace condition logic and introducing fresh metadata states to improve session handling.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements feature flag access control in the admin panel based on environment variables. Here's a focused review of the key changes:
- Added
useFeatureFlagManagementCapabilityhook to gate feature flag management based onIS_BILLING_ENABLEDorDEBUG_MODEenvironment variables - Modified
SettingsAdminContentto conditionally render feature flag controls based on user capabilities - Added server-side capability check in
AdminPanelResolverbut missing validation inupdateWorkspaceFeatureFlagsservice method - Improved state management in
useAuthandVerifyEffectfor better token verification flow - Removed unnecessary assertions in
useAuth.test.tsxthat could miss response structure issues
The critical security issue is that the updateWorkspaceFeatureFlags service method doesn't validate the user's capability before allowing updates, which could bypass the intended access control. This should be addressed before merging.
11 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -268,6 +268,8 @@ export const useAuth = () => { | |||
|
|
|||
| const handleVerify = useCallback( | |||
| async (loginToken: string) => { | |||
| setIsVerifyPendingState(true); | |||
There was a problem hiding this comment.
logic: setIsVerifyPendingState(true) should be wrapped in try/catch to ensure it gets set back to false if verification fails
| if (isDefined(loginToken)) { | ||
| setIsAppWaitingForFreshObjectMetadata(true); | ||
| verify(loginToken); | ||
| } else if (!isLogged) { |
There was a problem hiding this comment.
logic: verify() returns a Promise but it's not being awaited. This could lead to race conditions if the verification fails and the component unmounts before completion.
...es/twenty-front/src/modules/settings/admin-panel/hooks/useFeatureFlagManagementCapability.ts
Outdated
Show resolved
Hide resolved
...front/src/modules/settings/admin-panel/graphql/queries/getFeatureFlagManagementCapability.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminContent.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/dtos/user-lookup.entity.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if (isDefined(loginToken)) { | ||
| setIsAppWaitingForFreshObjectMetadata(true); |
There was a problem hiding this comment.
It would be good to give a bit more context in your PR description when you introduce something like that as it doesn't directly relate to the original issue / I'm not sure exactly what problem we're solving here (race condition / dual execution?). Seems similar to topics discussed here: #9288
There was a problem hiding this comment.
Two commits come from this PR #9451
It allows impersonation on the same workspace.
closes #9032