-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Introduce SSO bypass permission. #15417
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
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:38151 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.
Greptile Overview
Greptile Summary
This PR introduces an SSO bypass permission system that allows authorized users to use alternative authentication methods (Google, Microsoft, or Password) when SSO is enforced for a workspace.
Key Changes
- Database Schema: Adds three boolean columns (
isGoogleAuthBypassEnabled,isMicrosoftAuthBypassEnabled,isPasswordAuthBypassEnabled) to the workspace table - Permission System: Introduces a new
SSO_BYPASSpermission flag that gates access to bypass authentication - Backend Logic: Implements permission checks in
auth.service.tsthat validate both workspace-level bypass flags and user-level permissions before allowing alternative auth methods - Frontend UI: Adds settings toggles for workspace admins to enable bypass per provider, and a "Bypass SSO" link on the sign-in page for eligible users
- Security: Properly validates that users must be workspace members with explicit
SSO_BYPASSpermission to use bypass methods - System Validation: Ensures bypass flags can only be enabled if the corresponding auth provider is enabled at the system level
Implementation Quality
The implementation follows a defense-in-depth approach with multiple validation layers. Permission checks occur both at the workspace configuration level (whether bypass is enabled) and at the user level (whether the specific user has the SSO_BYPASS permission). The frontend properly reflects available bypass options and manages bypass mode state.
Confidence Score: 4/5
- This PR is safe to merge with minor considerations around testing on production-like data
- The implementation is well-structured with proper security controls, comprehensive test coverage, and follows established patterns. The permission checking logic is sound and validates both workspace membership and explicit permissions. The frontend implementation correctly manages state and UI visibility. One point deducted because database migrations adding new columns should ideally be tested on production-like datasets per custom instruction afbb9dab-0419-4839-9ab7-73551d42b0ae, though this specific migration is low-risk as it only adds new nullable boolean columns with defaults.
- No files require special attention - all implementations follow best practices and security guidelines
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/src/database/typeorm/core/migrations/common/1761651107128-addSsoBypassFlag.ts | 5/5 | Adds three new boolean columns to workspace table for SSO bypass flags with proper up/down migrations |
| packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts | 4/5 | Implements bypass logic with permission checks for all auth providers, properly gates bypass behind user workspace membership and permission flags |
| packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts | 5/5 | Adds validation to ensure bypass flags can only be enabled if system-level auth provider is enabled |
| packages/twenty-front/src/modules/auth/sign-in-up/components/SignInUpWorkspaceScopeForm.tsx | 5/5 | Shows bypass providers when bypass mode is enabled, properly merges with regular providers |
| packages/twenty-front/src/modules/settings/security/components/SettingsSecurityAuthBypassOptionsList.tsx | 5/5 | Settings UI component for toggling bypass flags, only shown when SSO providers exist |
Sequence Diagram
sequenceDiagram
participant User
participant Frontend
participant AuthService
participant PermissionsService
participant WorkspaceService
participant Database
User->>Frontend: Navigate to sign-in page
Frontend->>WorkspaceService: Query public workspace data
WorkspaceService->>Database: Get workspace config
Database-->>WorkspaceService: Return workspace + bypass flags
WorkspaceService-->>Frontend: Return authProviders + authBypassProviders
alt Only SSO enabled and bypass available
Frontend->>Frontend: Show "Bypass SSO" link
User->>Frontend: Click "Bypass SSO"
Frontend->>Frontend: Enable bypass mode
Frontend->>Frontend: Show bypass providers (Google/Microsoft/Password)
end
User->>Frontend: Submit credentials with bypass provider
Frontend->>AuthService: validateLoginWithPassword() or signInUpWithSocialSSO()
AuthService->>AuthService: Check if provider enabled
alt Provider disabled
AuthService->>AuthService: canUserBypassAuthProvider()
AuthService->>Database: Check user workspace exists
Database-->>AuthService: Return userWorkspace or null
alt User not in workspace
AuthService-->>Frontend: Return false (no bypass)
else User in workspace
AuthService->>PermissionsService: userHasWorkspaceSettingPermission(SSO_BYPASS)
PermissionsService->>Database: Get user role and permissions
Database-->>PermissionsService: Return role with permission flags
PermissionsService-->>AuthService: Return permission check result
alt Has SSO_BYPASS permission
AuthService->>AuthService: Allow login
AuthService-->>Frontend: Return auth tokens
else No permission
AuthService-->>Frontend: Throw FORBIDDEN_EXCEPTION
end
end
else Provider enabled
AuthService->>AuthService: Allow login normally
AuthService-->>Frontend: Return auth tokens
end
31 files reviewed, no comments
...nty-front/src/modules/settings/security/components/SettingsSecurityAuthBypassOptionsList.tsx
Outdated
Show resolved
Hide resolved
...nty-front/src/modules/settings/security/components/SettingsSecurityAuthBypassOptionsList.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/sign-in-up/hooks/useWorkspaceBypass.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/sign-in-up/hooks/useWorkspaceBypass.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/sign-in-up/hooks/useWorkspaceBypass.ts
Outdated
Show resolved
Hide resolved
| throw new Error(t`User is not logged in`); | ||
| } | ||
|
|
||
| const key = `is${capitalize(authProvider)}AuthBypassEnabled`; |
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 would try to avoid this kind of optimization, you prefer to keep something typed and explicit. It might look "smart" and repeating a few things might look dumb but here you're making it much more likely to create bugs if we refactor this in the future (capitalize make it very unstable)
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 did this because SettingsSecurityAuthProvidersOptionsList does the same thing - I tried to match SettingsSecurityAuthBypassOptionsList to SettingsSecurityAuthProvidersOptionsList as much as possible so refactor and reading code is consistent later.
Will change if you'd prefer to change.
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.
Ah that's a good reason to do it indeed!
Yes if you can update both file to do something less engineered that'd be great
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.
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.
Sure, thank you!
packages/twenty-front/src/modules/users/hooks/useLoadCurrentUser.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/users/hooks/useLoadCurrentUser.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/pages/settings/security/SettingsSecurity.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| const user = await this.userRepository.findOne({ |
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.
By moving the condition check below you're probably allowing an attacker to know if a user exists or not on an SSO-only workspace which can be considered a security issue
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.
As discussed on Discord, the attacker can also find this information using checkUserExists query.
Secondly, allowing bypass without making the code more messy is kind of hard - discussed on Discord on how we need a flag because we have no centralized context and everything is tightly coupled.
packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| private async canUserBypassAuthProvider({ |
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 don't understand, is this used for Google/Microsoft? I only see it in Password
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.
signInUpWithSocialSSO method calls signInUp which in turn calls isAuthProviderEnabledOrThrow which calls canUserBypassAuthProvider.
Password flow was not dependent on isAuthProviderEnabledOrThrow, so I had to add canUserBypassAuthProvider separately in validateLoginWithPassword.
It's a tightly-coupled code problem, auth needs extremely careful rework to fix and not introduce regressions.
| return false; | ||
| } | ||
|
|
||
| const userWorkspace = |
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.
Can't we access it from user directly instead of doing one more db request?
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.
Also this is poorly named (not you) if this is called checkXXXExists but is what should be use for the find method!
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.
Done. Had a separate fetch since Google and Microsoft did not fetch userWorkspace relation.
However, updated those to fetch workspace when fetching user by email and then replaced the DB query with find.
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.
In terms of naming, since we are not checking in the DB, I maintained the name of userWorkspace.
packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/sign-in-up/components/SignInUpWorkspaceScopeForm.tsx
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| workspaceValidator.isAuthEnabledOrThrow(authParams.provider, workspace); |
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.
This is redundant to avoid introducing multiple sources of error for the same thing (isAuthEnabled used earlier in the function).
I tried to avoid, but then an explicit throw had to be used and the error messages would need to be maintained in multiple places.
| import { workspaceAuthProvidersState } from '@/workspace/states/workspaceAuthProvidersState'; | ||
| import { workspaceBypassModeState } from '@/workspace/states/workspaceBypassModeState'; | ||
|
|
||
| export const useWorkspaceBypass = () => { |
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.
See this comment from Charles: #15035 (comment) -> avoid umbrella hooks.
It's acceptable in that case in my opinion because it's very tightly coupled, but be careful with this pattern and think about re-renders :)
FelixMalfait
left a comment
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.
great work!
|
Thanks @mabdullahabaid for your contribution! |


Closes Core Issue #1772.
Note
Introduces SSO bypass with a new permission flag and workspace-level provider toggles, enabling permitted users to log in via Google/Microsoft/Password when SSO-only, with backend enforcement and frontend UI/hooks/queries.
PermissionFlagType.SSO_BYPASS; updateAuthServiceto allow login via non-SSO providers when workspace bypass is enabled and user hasSSO_BYPASS.isGoogleAuthBypassEnabled,isMicrosoftAuthBypassEnabled,isPasswordAuthBypassEnabled(migration, entity, update input, service validation).PublicWorkspaceDataOutputwithauthBypassProviders; resolver computes it; permissions defaults includeSSO_BYPASS.authBypassProviderstoGetPublicWorkspaceDataByDomain; new statesworkspaceAuthBypassProvidersState,workspaceBypassModeState.useWorkspaceBypass; update sign-in form and footer to offer "Bypass SSO" and use merged providers when enabled; remove auto-redirect when single SSO.useCanChangePassword.Written by Cursor Bugbot for commit 8c393b2. This will update automatically on new commits. Configure here.