feat: Introduce Organization Administrator concept along with Instance Administrator #39381
Conversation
…ng new instance admin role
…ow also updates organization admin for non cloud hosted environments
trishaanand
requested review from
sharat87,
abhvsn and
AnaghHegde
as code owners
WalkthroughThis pull request refactors the role management structure across the application. The tenant-based "TENANT_ADMIN" role is removed in favor of an organization-based "ORGANIZATION_ADMIN" role. In addition, a new constant for organization roles is added, and multiple method names and parameters have been updated to reflect this change. The updates span from core authorization classes and migrations to tests, ensuring that user assignment and permission modifications consistently use the new organization-centric approach. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User Client
participant Signup as UserSignupCEImpl
participant Utils as UserUtilsCE
participant DB as Database/MongoTemplate
Client->>Signup: signupAndLoginSuper(user details)
Signup->>Utils: makeInstanceAdministrator(users)
Utils->>DB: Assign Instance Admin & Organization Admin Groups
DB-->>Utils: Confirmation
Utils-->>Signup: Assignment result
Signup-->>Client: User signed up & role assigned
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration066_RenameInstanceAdminRoleToOrgAdmin.java (3)
23-24: Consider a more detailed migration annotation comment.While the current
@ChangeUnitannotation succinctly describes what the migration does, adding a brief summary inside a JavaDoc or annotation comment (indicating steps the migration performs) can help maintainers quickly identify its purpose.
35-55: Log a success message after completion.Currently, there’s debug logging for failure scenarios (like when instance config or organization isn’t found). Consider logging a short success message at the end of
execute()to confirm that the rename and creation steps completed successfully.
91-120: Validate the renamed permission group presence.You’re logging warnings if no permission group was updated. That’s good. As a future enhancement, consider verifying if the domain ID matches the one you intended to rename (e.g. multiple groups with the same name). This avoids confusion in multi-domain contexts.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (3)
68-119: Consider refactoring large sections into smaller methods.
makeInstanceAdministratorcontains several logical steps (cloud hosting checks, retrieving PGs, updating assigned users). Splitting it into smaller helper methods can improve readability and reusability.
121-172: DRY principle for removal logic.
removeInstanceAdminmostly mirrorsmakeInstanceAdministrator, but in reverse. Abstracting out the shared logic for retrieving permission groups or user sets can reduce duplication and future maintenance overhead.
182-185: Log when multiple organization admin permission groups exist.
getOrganizationAdminPermissionGroupcalls.next(), implying more than one matching group could exist. Logging a warning or an informational log if multiple roles are found can prevent confusion.app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java (1)
66-66: Consider adding a description for ORGANIZATION_ADMIN role.While the role definition is correct, consider adding a meaningful description instead of an empty string to maintain consistency with other roles and improve clarity.
- ORGANIZATION_ADMIN(ORGANIZATION_ADMINISTRATOR_ROLE, "", Set.of(MANAGE_ORGANIZATION)), + ORGANIZATION_ADMIN(ORGANIZATION_ADMINISTRATOR_ROLE, "Administrator role for managing organization-wide settings and permissions", Set.of(MANAGE_ORGANIZATION)),app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/UserUtilsTest.java (2)
45-69: Add test cases for error scenarios.The test suite would benefit from additional test cases covering error scenarios such as:
- Attempting to make a non-existent user an administrator
- Handling null or empty user lists
- Verifying behavior when permission groups don't exist
71-97: Consider testing concurrent operations.Since this involves permission management, it would be valuable to add test cases verifying thread-safety when multiple operations are performed concurrently.
Example test case structure:
@Test void makeInstanceAdministrator_WhenConcurrentOperations_HandlesRaceConditions() { List<User> users = Arrays.asList(user1, user2); Flux.merge( userUtils.makeInstanceAdministrator(users), userUtils.removeInstanceAdmin(users), userUtils.makeInstanceAdministrator(users) ) .then(/* verify final state */) .as(StepVerifier::create) .verifyComplete(); }app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java (1)
100-102: Update TODO comment to reflect new role structure.The TODO comment still refers to "super-user" and mentions multitenancy, but the codebase has moved to an organization-based model.
- // Make api_user super-user to test organization admin functionality - // Todo change this to organization admin once we introduce multitenancy + // Make api_user an instance administrator to test organization admin functionalityapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (2)
296-298: Update log message to reflect new role terminology.The log message still refers to "makeSuperUser" but the code now uses "makeInstanceAdministrator".
- "UserSignupCEImpl::Time taken to complete makeSuperUser: {} ms", pair.getT1()); + "UserSignupCEImpl::Time taken to complete makeInstanceAdministrator: {} ms", pair.getT1());
268-269: Consider renaming method to align with new role structure.The method name "signupAndLoginSuper" could be updated to better reflect the current role management approach.
- public Mono<User> signupAndLoginSuper( + public Mono<User> signupAndLoginInstanceAdmin(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserUtils.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration066_RenameInstanceAdminRoleToOrgAdmin.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/UserUtilsTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/CacheableRepositoryTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (12)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration066_RenameInstanceAdminRoleToOrgAdmin.java (1)
57-89: Handle potential null checks fororganizationAdminRole.The code retrieves an organization admin permission group, then directly uses its fields. In rare cases, the group may be absent. Add checks or an early return to safely handle null or missing roles, logging an error message if so.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (3)
4-5: Ensure CommonConfig is properly configured.You’ve imported and injected
CommonConfigfor cloud hosting checks. Validate thatcommonConfig.isCloudHosting()is defined properly in all relevant environments to avoid unexpected behavior.
43-49: Constructor injection looks consistent.This constructor properly injects the new dependencies. Good clarity and alignment with best practices for dependency injection.
174-180: Handle missing defaultPermissionGroup gracefully.
getInstanceAdminPermissionGrouprelies onDEFAULT_PERMISSION_GROUP. IfinstanceAdminConfigurationprovides a null or invalid ID, you might receive an emptyMono. Consider logging or gracefully handling such scenarios to avoid silent failures.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (1)
63-64: Confirm consistency of ORGANIZATION_ADMIN usage.Replacing
TENANT_ADMINreferences withORGANIZATION_ADMINcan introduce inconsistencies if old references remain. Double-check other references within this file and across the codebase for alignment.Also applies to: 552-553
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserUtils.java (1)
15-21: LGTM! Constructor changes align with organization-based role management.The constructor changes correctly reflect the shift from tenant-based to organization-based role management by replacing
PermissionGroupPermissionwithCommonConfig.Also applies to: 22-27
app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/CacheableRepositoryTest.java (1)
43-43: LGTM! Test setup updated to use new role management.The change from
makeSuperUsertomakeInstanceAdministratorcorrectly aligns with the new organization-based role management approach.app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java (1)
99-99: LGTM! Test updated to use new role management.The change from
makeSuperUsertomakeInstanceAdministratorcorrectly aligns with the new organization-based role management approach.app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
210-210: LGTM!The new constant follows the established naming convention and is appropriately placed in the file.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (2)
614-614: LGTM! Consistent with the new Instance Administrator concept.The change from
removeSuperUsertoremoveInstanceAdminaligns with the new role structure.
647-647: LGTM! Consistent with the new Instance Administrator concept.The change from
makeSuperUsertomakeInstanceAdministratoraligns with the new role structure.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (1)
420-422: LGTM! Consistent with the new Instance Administrator concept.The change from
makeSuperUsertomakeInstanceAdministratoraligns with the new role structure.
Failed server tests
|
…n type while calculating permission groups for user
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java(1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
82-83: Neat projection for ID-only retrieval
Using field projection to retrieve only the permission group IDs helps reduce query overhead and maintain clarity in the returned data.
| BridgeQuery<PermissionGroup> assignedToUserIdsCriteria = | ||
| Bridge.equal(PermissionGroup.Fields.assignedToUserIds, user.getId()); | ||
|
|
||
| BridgeQuery<PermissionGroup> notDeletedCriteria = notDeleted(); | ||
| BridgeQuery<PermissionGroup> notDeletedCriteria = notDeleted(); | ||
|
|
||
| // The roles should be either workspace default roles, user management role, or instance admin role | ||
| BridgeQuery<PermissionGroup> ceSupportedRolesCriteria = Bridge.or( | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Workspace.class.getSimpleName()), | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, User.class.getSimpleName()), | ||
| Bridge.equal(PermissionGroup.Fields.id, instanceAdminPermissionGroupId)); | ||
| // The roles should be either workspace default roles, user management role, or organization admin role | ||
| BridgeQuery<PermissionGroup> ceSupportedRolesCriteria = Bridge.or( | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Workspace.class.getSimpleName()), | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, User.class.getSimpleName()), | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Organization.class.getSimpleName())); | ||
|
|
||
| BridgeQuery<PermissionGroup> andCriteria = | ||
| Bridge.and(assignedToUserIdsCriteria, notDeletedCriteria, ceSupportedRolesCriteria); | ||
| BridgeQuery<PermissionGroup> andCriteria = | ||
| Bridge.and(assignedToUserIdsCriteria, notDeletedCriteria, ceSupportedRolesCriteria); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure domain types align with the new admin concept
The updated criteria logic looks streamlined and removes references to instance-based roles, focusing on organization-based roles. It's essential to verify that no leftover references to the old "instance admin" or "tenant admin" roles remain within the codebase or mismatch the newly introduced organization-centric approach.
Run the following script to confirm if any vestiges of "instanceAdminPermissionGroupId", "TENANT_ADMIN", or "instance admin" remain:
🏁 Script executed:
#!/bin/bash
# Description: Locate references to old instance/tenant admin roles in the codebase
rg -A 3 -i "instanceAdminPermissionGroupId|TENANT_ADMIN|instanceadmin" Length of output: 50719
Action Required: Align All Legacy Admin References with the New Organization-Centric Approach
The updated criteria in CacheableRepositoryHelperCEImpl.java is correctly streamlined for organization-based roles. However, the search results show that legacy instance/tenant admin references (e.g., "instanceAdminPermissionGroupId", TENANT_ADMIN, and "instance admin") still appear in several parts of the codebase. Please review and determine if these references:
- Are intentionally retained (for migration, backward compatibility, or transitional logic), or
- Require further refactoring (in controllers, migration scripts, helper classes, and tests) to fully conform to the new admin concept.
Files to check:
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/InMemoryCacheableRepositoryHelper.java- Migration files (e.g.,
Migration066_RenameInstanceAdminRoleToOrgAdmin.java,Migration10000_UpdateSuperUser.java) - Relevant controllers and service files (e.g., InstanceAdminController, OrganizationController)
- Test files under the
src/test/javapaths related to user and permission group management
Please update or remove outdated references where appropriate to avoid potential inconsistencies with the organization-centric role logic.
|
Closing this for another PR because the branch reached a bad state I wasnt able to recover from. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD c0b2b0b yet
Thu, 20 Feb 2025 16:28:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit