-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Fix Admin control to disable workspace creation for non-admin users #14895
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.
Greptile Overview
Summary
This PR implements admin control over workspace creation by adding a new configuration variable `IS_WORKSPACE_CREATION_LIMITED_TO_WORKSPACE_ADMINS` that restricts workspace creation to admin users only. The implementation adds a new method `isWorkspaceCreationAllowed` in the `SignInUpService` that checks if workspace creation is limited and validates user permissions before allowing workspace creation.The solution works by:
- Adding a new boolean config variable that defaults to
falsefor backward compatibility - Creating a
countUserWorkspacesmethod inUserWorkspaceServiceto count how many workspaces a user belongs to - Implementing permission logic that allows users with no existing workspaces to create their first workspace (for initial setup), but restricts additional workspace creation to users with
canAccessFullAdminPanelpermission when the restriction is enabled - Integrating this check into the
signUpInNewWorkspaceresolver to enforce the restriction at the API level
The implementation follows existing patterns in the codebase, using the established AuthException mechanism for error handling and leveraging the existing user permission system (canAccessFullAdminPanel). This addresses the need for centralized instance management in enterprise deployments where administrators need to control workspace proliferation.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts | 5/5 | Added countUserWorkspaces method to count workspaces for a given user |
| packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts | 4/5 | Integrated workspace creation permission check into signup resolver |
| packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts | 4/5 | Added new boolean config variable to control workspace creation restrictions |
| packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts | 4/5 | Implemented core logic for workspace creation permission validation |
Confidence score: 4/5
- This PR implements a well-structured security feature with minimal risk of breaking existing functionality
- Score reflects solid implementation following existing patterns with appropriate error handling and backward compatibility
- Pay close attention to the permission logic in
sign-in-up.service.tsto ensure the admin validation works correctly
Sequence Diagram
sequenceDiagram
participant User
participant AuthResolver
participant SignInUpService
participant TwentyConfigService
participant UserWorkspaceService
participant UserRepository
participant WorkspaceRepository
participant DomainManagerService
participant UserWorkspaceService2 as UserWorkspaceService
participant OnboardingService
User->>AuthResolver: "signUpInNewWorkspace()"
AuthResolver->>SignInUpService: "isWorkspaceCreationAllowed(currentUser)"
SignInUpService->>TwentyConfigService: "get('IS_WORKSPACE_CREATION_LIMITED_TO_WORKSPACE_ADMINS')"
TwentyConfigService-->>SignInUpService: "config value"
alt workspace creation is limited
SignInUpService->>UserWorkspaceService: "countUserWorkspaces(currentUser.id)"
UserWorkspaceService-->>SignInUpService: "workspace count"
alt user has workspaces AND is not admin
SignInUpService-->>AuthResolver: "throw AuthException('Workspace creation is restricted to admins')"
AuthResolver-->>User: "AuthException"
end
end
SignInUpService-->>AuthResolver: "validation passed"
AuthResolver->>SignInUpService: "signUpOnNewWorkspace(userData)"
SignInUpService->>DomainManagerService: "generateSubdomain()"
DomainManagerService-->>SignInUpService: "subdomain"
SignInUpService->>WorkspaceRepository: "create(workspaceData)"
SignInUpService->>WorkspaceRepository: "save(workspace)"
WorkspaceRepository-->>SignInUpService: "saved workspace"
SignInUpService->>UserWorkspaceService2: "create(userWorkspaceData)"
UserWorkspaceService2-->>SignInUpService: "userWorkspace"
SignInUpService->>OnboardingService: "setOnboardingInviteTeamPending()"
SignInUpService-->>AuthResolver: "{user, workspace}"
AuthResolver-->>User: "SignUpOutput with loginToken"
4 files reviewed, 2 comments
packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
Outdated
Show resolved
Hide resolved
| await this.signInUpService.isWorkspaceCreationAllowed(currentUser); | ||
|
|
||
| const { user, workspace } = await this.signInUpService.signUpOnNewWorkspace( | ||
| { type: 'existingUser', existingUser: currentUser }, |
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.
Potential bug: A race condition in signUpInNewWorkspace allows concurrent requests to bypass the single workspace creation limit for non-admin users because the check and creation are not atomic.
-
Description: The
signUpInNewWorkspaceresolver first checks if a user can create a workspace viaisWorkspaceCreationAllowedand then performs the creation in a separate step. These two operations are not atomic. A race condition exists where a non-admin user with zero workspaces can send concurrent requests. Both requests can pass the permission check before either has created a user-workspace association. This allows both requests to proceed, resulting in the user creating multiple workspaces and bypassing the business logic that should restrict them to only one. -
Suggested fix: Wrap the permission check in
isWorkspaceCreationAllowedand the subsequent workspace creation logic within a single database transaction. Using aSERIALIZABLEisolation level will ensure the check-then-act sequence is atomic, preventing the race condition from allowing multiple workspace creations.
severity: 0.55, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:3775 This environment will automatically shut down when the PR is closed or after 5 hours. |
packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.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.
Thank you!
|
Thanks @ketanMehtaa for your contribution! |
- Introduced new field types: PDF and IMAGE, including their metadata structures. - Updated GraphQL and REST API contracts to accommodate new field types. - Implemented validation and utility functions for handling PDF and Image fields. - Added integration tests to ensure proper functionality of new field types. - Updated documentation and specifications related to the new features. This commit enhances the data model by allowing users to upload and manage PDF and Image files, improving the overall functionality of the application.

fix #13460