feat: Introducing Organization Administrator with refactor for Instance Admin #39417
Conversation
trishaanand
requested review from
sharat87,
abhvsn and
AnaghHegde
as code owners
WalkthroughThis pull request refactors role and permission management across the application. The legacy Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Signup as UserSignupCEImpl
participant Utils as UserUtils
participant DB as Database
U->>Signup: Signup request
Signup->>Utils: makeInstanceAdministrator(user)
Utils->>DB: Assign permissions to instance/organization admin groups
DB-->>Utils: Confirmation
Utils-->>Signup: Success
Signup-->>U: Signup complete
sequenceDiagram
participant M as Migration066_RenameInstanceAdminRoleToOrgAdmin
participant CT as Config Collection
participant ORG as Organization Collection
participant PG as Permission Group Collection
M->>CT: Retrieve "instanceConfig"
CT-->>M: Config data
M->>ORG: Fetch default organization
ORG-->>M: Organization data
M->>PG: Rename permission group to Organization Admin
PG-->>M: Update result
M->>PG: Create new role entry
PG-->>M: Role created
Possibly related PRs
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
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (1)
83-119: 🛠️ Refactor suggestionPotential concurrency considerations
When updating permission groups, consider scenarios with concurrent calls. If two calls modify the same permission group simultaneously, the last update wins might overwrite changes from the other. A more atomic approach with add/remove style mutation or version checks could help mitigate race conditions.
🧹 Nitpick comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java (2)
66-66: Add a description for the ORGANIZATION_ADMIN role.While the role implementation is correct, consider adding a descriptive comment to document the role's purpose and responsibilities.
- ORGANIZATION_ADMIN(ORGANIZATION_ADMINISTRATOR_ROLE, "", Set.of(MANAGE_ORGANIZATION)), + ORGANIZATION_ADMIN( + ORGANIZATION_ADMINISTRATOR_ROLE, + "Role for managing organization-level settings and configurations", + Set.of(MANAGE_ORGANIZATION)),
66-66: Add a description for theORGANIZATION_ADMINrole.The empty description string reduces code maintainability. Consider adding a meaningful description that explains the role's purpose and responsibilities.
- ORGANIZATION_ADMIN(ORGANIZATION_ADMINISTRATOR_ROLE, "", Set.of(MANAGE_ORGANIZATION)), + ORGANIZATION_ADMIN( + ORGANIZATION_ADMINISTRATOR_ROLE, + "Role for managing organization-level settings and configurations", + Set.of(MANAGE_ORGANIZATION)),app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
62-85: Consider adding an index for more efficient lookups.In this query, fields like
assignedToUserIdsare used in a filter condition. To improve performance for large user bases, ensure thatPermissionGrouphas an index onassignedToUserIds(as well as any other relevant fields).app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration066_RenameInstanceAdminRoleToOrgAdmin.java (1)
1-121: Add tests or verification steps for the migration.The migration logic appears sound, but because it modifies critical permissions, consider automated tests or scripts to ensure data integrity. Additionally, the rollback section remains empty—implementing a partial or manual rollback strategy might be beneficial in case of environment-specific failures.
Would you like help creating a test or verification strategy for this migration?
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (2)
70-77: Conditional handling of organization admin
Skipping the organization admin assignment in cloud hosting scenarios may be correct for your design. Ensure this condition is documented for clarity and to avoid confusion among future maintainers.
174-185: Clear data retrieval methods
ThegetInstanceAdminPermissionGroupandgetOrganizationAdminPermissionGroupmethods centralize permission group logic. This modular approach enhances reusability and keeps your code DRY. Consider improving inline comments for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
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/repositories/ce/CacheableRepositoryHelperCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.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)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java
👮 Files not reviewed due to content moderation or server errors (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (21)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java (1)
99-99: LGTM! Role transition implemented correctly.The change from
makeSuperUsertomakeInstanceAdministratoraligns with the new role management system.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (1)
420-422: LGTM! Role assignment updated correctly.The change from
makeSuperUsertomakeInstanceAdministratormaintains consistent role management throughout the service layer.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java (1)
104-104: LGTM! Test setup updated appropriately.The change from
makeSuperUsertomakeInstanceAdministratorcorrectly sets up the test environment for organization admin functionality.app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/CacheableRepositoryTest.java (1)
43-43: LGTM! Test setup updated correctly.The change from
makeSuperUsertomakeInstanceAdministratormaintains consistency in role management across test cases.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (3)
63-63:❓ Verification inconclusive
LGTM! Verify the migration's impact on existing tenant admins.
The role change from
TENANT_ADMINtoORGANIZATION_ADMINlooks correct. The permissions are preserved during the migration.Run this script to verify that existing tenant admins are properly migrated:
Also applies to: 553-558
🏁 Script executed:
#!/bin/bash # Description: Check if any users still have the old TENANT_ADMIN role # and verify that ORGANIZATION_ADMIN permissions are correctly assigned # Search for any remaining TENANT_ADMIN references rg "TENANT_ADMIN" -A 5 # Check permission assignments in the database ast-grep --pattern $'class PermissionGroup { $$$ permissions { $$$ TENANT_ADMIN $$$ } $$$ }'Length of output: 136
Manual Verification Needed:
The change fromTENANT_ADMINtoORGANIZATION_ADMINappears correct, and the initial script did not reveal any lingering instances ofTENANT_ADMIN. However, since the script didn’t produce any output—which could be indicative of low inference quality—it’s important to manually verify that all legacy references have been removed and that the new role permissions are correctly applied.Action Items:
- Use a comprehensive search (e.g.,
rg -w "TENANT_ADMIN") across the codebase to ensure no references to the old role remain.- Confirm that instances expecting tenant admin permissions now properly reference
ORGANIZATION_ADMIN(for example, in permission assignment sections within relevant classes).
63-63: LGTM! Role transition implemented correctly.The changes correctly update the role reference from
TENANT_ADMINtoORGANIZATION_ADMINfor permission management.Also applies to: 553-553
63-63:❓ Verification inconclusive
LGTM! Verify the migration's impact on existing tenant admins.
The role change from
TENANT_ADMINtoORGANIZATION_ADMINlooks correct. The permissions are preserved during the migration.Run this script to verify that existing tenant admins are properly migrated:
Also applies to: 553-558
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to TENANT_ADMIN and verify permission groups # Search for any remaining TENANT_ADMIN references rg "TENANT_ADMIN" # Search for permission groups to verify the migration ast-grep --pattern $'class PermissionGroup { $$$ name = "Organization Administrator Role" $$$ }'Length of output: 121
Subject: Verify the Migration’s Impact on Tenant Admins
The update replacing
TENANT_ADMINwithORGANIZATION_ADMINappears correct (see the static import change). However, the automated searches (usingrgandast-grep) didn’t return any output, so it’s not clear whether these commands fully confirm the proper migration.
- Please manually verify that no references to
TENANT_ADMINremain in the codebase.- Confirm that the permission group now correctly includes the
"Organization Administrator Role"(as also noted for lines 553-558).Thanks for ensuring that tenant admin permissions remain preserved during migration.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (3)
293-293: LGTM! Method name better reflects its purpose.The renaming from
makeSuperUsertomakeInstanceAdministratorimproves code clarity by accurately describing the role being assigned.
293-293: LGTM! Method name improved for clarity.The renaming from
makeSuperUsertomakeInstanceAdministratorbetter reflects the method's purpose and aligns with the role transition.
293-293: LGTM! Method name better reflects its purpose.The change from
makeSuperUsertomakeInstanceAdministratorimproves code clarity by accurately describing the role being assigned.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (3)
614-614: LGTM! Method names better reflect their purposes.The renaming from
removeSuperUser/makeSuperUsertoremoveInstanceAdmin/makeInstanceAdministratorimproves code clarity by accurately describing the roles being managed.Also applies to: 647-647
614-614: LGTM! Method names updated consistently.The method renames maintain consistency with the role transition from super user to instance administrator.
Also applies to: 647-647
614-614: LGTM! Method names align with the new role system.The changes from
removeSuperUser/makeSuperUsertoremoveInstanceAdmin/makeInstanceAdministratorimprove code clarity by accurately describing the roles being managed.Also applies to: 647-647
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserUtils.java (2)
3-3: Import addition looks good.
20-27: Verify the usage of CommonConfig defaults.Ensure the constructor usage of
CommonConfigproperly handles default values within the class. Confirm that any optional or fallback behaviors inCommonConfigare covered, particularly if environment variables are missing or not set.app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/UserUtilsTest.java (4)
31-43: Good initialization of test data
The setup method creates distinct users with different IDs, ensuring no conflicts in subsequent tests. This approach is clear, concise, and sets the stage for more straightforward assertions.
45-69: Comprehensive validation of instance and organization admin assignments
The test effectively uses StepVerifier to verify that both users are assigned to the instance admin and organization admin permission groups. The additional check on assigned user IDs is thorough.
71-97: Robust removal test
The test adds followed by removing admin permissions in one flow, ensuring both steps are valid. Testing real-world usage scenarios like add-then-remove in one test prevents regressions.
99-126: Duplicate assignment check
Ensuring that a user is not re-added multiple times is critical. The test's final assertion of exactly one assigned user in each group removes ambiguity.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (1)
121-172: Symmetry in addition and removal methods
TheremoveInstanceAdminmethod closely mirrorsmakeInstanceAdministrator, maintaining consistent logic. This is good for readability and maintainability.app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AppsmithRole.java (1)
28-28: LGTM! Role definition is clear and maintains permissions.The
ORGANIZATION_ADMINrole correctly replacesTENANT_ADMINwhile preserving theMANAGE_ORGANIZATIONpermission.Also applies to: 66-66
…rmission groups for a user.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java(2 hunks)
🚧 Files skipped from review as they are similar to previous 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 (3)
- GitHub Check: build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
58-59: LGTM! Additional null check for organizationId.The added null check for organizationId aligns with the new organization-based permission model.
| Mono<Query> createQueryMono = getOrganizationAdminPermissionGroupId(user.getOrganizationId()) | ||
| .map(organizationAdminPermissionGroupId -> { | ||
| 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 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, organizationAdminPermissionGroupId)); | ||
|
|
||
| BridgeQuery<PermissionGroup> andCriteria = | ||
| Bridge.and(assignedToUserIdsCriteria, notDeletedCriteria, ceSupportedRolesCriteria); | ||
| BridgeQuery<PermissionGroup> andCriteria = | ||
| Bridge.and(assignedToUserIdsCriteria, notDeletedCriteria, ceSupportedRolesCriteria); | ||
|
|
||
| Query query = new Query(); | ||
| query.addCriteria(andCriteria); | ||
| Query query = new Query(); | ||
| query.addCriteria(andCriteria); | ||
|
|
||
| // Since we are only interested in the permission group ids, we can project only the id field. | ||
| query.fields().include(PermissionGroup.Fields.id); | ||
| // Since we are only interested in the permission group ids, we can project only the id field. | ||
| query.fields().include(PermissionGroup.Fields.id); | ||
|
|
||
| return query; | ||
| }); | ||
| return query; | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for missing organization admin permission group.
The query construction looks good, but we should handle the case where getOrganizationAdminPermissionGroupId returns empty.
🏁 Script executed:
#!/bin/bash
# Search for error handling of empty organization admin permission group
rg -A 5 "getOrganizationAdminPermissionGroupId.*empty|flatMap.*error"Length of output: 13241
Action: Implement Error Handling for Empty Admin Permission Group
The query construction in CacheableRepositoryHelperCEImpl.java (lines 63–86) currently maps the result of getOrganizationAdminPermissionGroupId(user.getOrganizationId()) directly without handling the scenario where the returned Mono might be empty. Similar patterns in the codebase (e.g., in DatasourceStorageServiceCEImpl.java and CommonGitServiceCEImpl.java) use error handling (via flatMap with Mono.error or similar operators) to deal with such cases. To ensure robust behavior and avoid potential downstream issues, please add error or empty value handling (for example, using switchIfEmpty or onErrorResume) for when the organization admin permission group is not found.
- File:
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java - Lines: 63–86
| public Mono<String> getOrganizationAdminPermissionGroupId(String organizationId) { | ||
|
|
||
| // Find the permission group id of the organization admin | ||
| BridgeQuery<PermissionGroup> andCriteria = Bridge.and( | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Organization.class.getSimpleName()), | ||
| Bridge.equal(PermissionGroup.Fields.defaultDomainId, organizationId)); | ||
|
|
||
| Query query = new Query(); | ||
| query.addCriteria(andCriteria); | ||
|
|
||
| // Since we are only interested in the permission group ids, we can project only the id field. | ||
| query.fields().include(PermissionGroup.Fields.id); | ||
|
|
||
| return mongoOperations | ||
| .find(query, PermissionGroup.class) | ||
| .map(permissionGroup -> permissionGroup.getId()) | ||
| .next(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing organization admin permission group.
The implementation looks good but should handle the case where no permission group is found.
return mongoOperations
.find(query, PermissionGroup.class)
.map(permissionGroup -> permissionGroup.getId())
- .next();
+ .next()
+ .switchIfEmpty(Mono.error(
+ new AppsmithException(
+ AppsmithError.NO_ORGANIZATION_ADMIN_PERMISSION_GROUP,
+ organizationId
+ )
+ ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Mono<String> getOrganizationAdminPermissionGroupId(String organizationId) { | |
| // Find the permission group id of the organization admin | |
| BridgeQuery<PermissionGroup> andCriteria = Bridge.and( | |
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Organization.class.getSimpleName()), | |
| Bridge.equal(PermissionGroup.Fields.defaultDomainId, organizationId)); | |
| Query query = new Query(); | |
| query.addCriteria(andCriteria); | |
| // Since we are only interested in the permission group ids, we can project only the id field. | |
| query.fields().include(PermissionGroup.Fields.id); | |
| return mongoOperations | |
| .find(query, PermissionGroup.class) | |
| .map(permissionGroup -> permissionGroup.getId()) | |
| .next(); | |
| } | |
| public Mono<String> getOrganizationAdminPermissionGroupId(String organizationId) { | |
| // Find the permission group id of the organization admin | |
| BridgeQuery<PermissionGroup> andCriteria = Bridge.and( | |
| Bridge.equal(PermissionGroup.Fields.defaultDomainType, Organization.class.getSimpleName()), | |
| Bridge.equal(PermissionGroup.Fields.defaultDomainId, organizationId)); | |
| Query query = new Query(); | |
| query.addCriteria(andCriteria); | |
| // Since we are only interested in the permission group ids, we can project only the id field. | |
| query.fields().include(PermissionGroup.Fields.id); | |
| return mongoOperations | |
| .find(query, PermissionGroup.class) | |
| .map(permissionGroup -> permissionGroup.getId()) | |
| .next() | |
| .switchIfEmpty(Mono.error( | |
| new AppsmithException( | |
| AppsmithError.NO_ORGANIZATION_ADMIN_PERMISSION_GROUP, | |
| organizationId | |
| ) | |
| )); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
97-123:⚠️ Potential issueAdd error handling for missing organization admin permission group.
The method should handle the case where no permission group is found.
return mongoOperations .find(query, PermissionGroup.class) .map(permissionGroup -> permissionGroup.getId()) .next() + .switchIfEmpty(Mono.error( + new AppsmithException( + AppsmithError.NO_ORGANIZATION_ADMIN_PERMISSION_GROUP, + organizationId + ) + )) .doOnSuccess( permissionGroupId -> inMemoryCacheableRepositoryHelper.setOrganizationAdminPermissionGroupId( organizationId, permissionGroupId));
📜 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/helpers/InMemoryCacheableRepositoryHelper.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java(3 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 (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/InMemoryCacheableRepositoryHelper.java (1)
43-49: LGTM! Clean implementation of getter and setter methods.The methods follow the established pattern in the class and provide clear functionality for managing organization admin permission group IDs.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (2)
56-62: LGTM! Comprehensive validation of user attributes.The validation now includes organization ID, which is essential for the new organization-based permission model.
64-87: LGTM! Well-structured query construction.The query construction is clean and modular, with clear separation of criteria.
| @@ -12,6 +14,8 @@ public class InMemoryCacheableRepositoryHelper { | |||
|
|
|||
| private String instanceAdminPermissionGroupId = null; | |||
|
|
|||
| private Map<String, String> inMemoryOrganizationIdOrganizationPermissionGroupIdMap = new HashMap<>(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using ConcurrentHashMap for thread safety.
The HashMap implementation might lead to race conditions in a multi-threaded environment.
- private Map<String, String> inMemoryOrganizationIdOrganizationPermissionGroupIdMap = new HashMap<>();
+ private Map<String, String> inMemoryOrganizationIdOrganizationPermissionGroupIdMap = new ConcurrentHashMap<>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Map<String, String> inMemoryOrganizationIdOrganizationPermissionGroupIdMap = new HashMap<>(); | |
| private Map<String, String> inMemoryOrganizationIdOrganizationPermissionGroupIdMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (1)
70-121: 🛠️ Refactor suggestionConsider adding validation for minimum number of instance administrators.
The method correctly handles permission group assignments and cache eviction. However, it's important to ensure that there's always at least one instance administrator to prevent lockout scenarios.
Consider adding a check to ensure at least one instance administrator remains:
public Mono<Boolean> makeInstanceAdministrator(List<User> users) { + if (users == null || users.isEmpty()) { + return Mono.error(new IllegalArgumentException("Users list cannot be empty")); + } // Rest of the method...
📜 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/helpers/ce/UserUtilsCE.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/UserUtilsTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/UserUtilsTest.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (2)
4-9: LGTM! Dependencies and constructor changes are well-structured.The new dependencies are properly injected and initialized in the constructor, following good dependency injection practices.
Also applies to: 37-39, 41-52
176-206: LGTM! Well-implemented caching for permission groups.The permission group retrieval methods effectively use caching to optimize performance while maintaining data consistency.
| return savedConfigAndPermissionGroupMono.map(Tuple2::getT2); | ||
| } | ||
| if (!cloudHosting) { | ||
| organizationAdminPgMono = getDefaultOrganizationAdminPermissionGroup(); | ||
| } | ||
|
|
||
| protected Mono<Tuple2<PermissionGroup, Config>> createConfigAndPermissionGroupForSuperAdmin() { | ||
| return Mono.zip(createInstanceAdminConfigObject(), createInstanceAdminPermissionGroupWithoutPermissions()) | ||
| return Mono.zip(getInstanceAdminPermissionGroup(), organizationAdminPgMono) | ||
| .flatMap(tuple -> { | ||
| Config savedInstanceConfig = tuple.getT1(); | ||
| PermissionGroup savedPermissionGroup = tuple.getT2(); | ||
|
|
||
| // Update the instance config with the permission group id | ||
| savedInstanceConfig.setConfig( | ||
| new JSONObject(Map.of(DEFAULT_PERMISSION_GROUP, savedPermissionGroup.getId()))); | ||
|
|
||
| Policy editConfigPolicy = Policy.builder() | ||
| .permission(MANAGE_INSTANCE_CONFIGURATION.getValue()) | ||
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | ||
| .build(); | ||
| Policy readConfigPolicy = Policy.builder() | ||
| .permission(READ_INSTANCE_CONFIGURATION.getValue()) | ||
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | ||
| .build(); | ||
|
|
||
| savedInstanceConfig.setPolicies(Set.of(editConfigPolicy, readConfigPolicy)); | ||
|
|
||
| // Add config permissions to permission group | ||
| Set<Permission> configPermissions = | ||
| Set.of(new Permission(savedInstanceConfig.getId(), MANAGE_INSTANCE_CONFIGURATION)); | ||
|
|
||
| return Mono.zip( | ||
| addPermissionsToPermissionGroup(savedPermissionGroup, configPermissions), | ||
| configRepository.save(savedInstanceConfig)); | ||
| }); | ||
| } | ||
| PermissionGroup instanceAdminPg = tuple.getT1(); | ||
| PermissionGroup organizationAdminPg = tuple.getT2(); | ||
|
|
||
| private Mono<Config> createInstanceAdminConfigObject() { | ||
| Config instanceAdminConfiguration = new Config(); | ||
| instanceAdminConfiguration.setName(FieldName.INSTANCE_CONFIG); | ||
|
|
||
| return configRepository.save(instanceAdminConfiguration); | ||
| } | ||
| if (instanceAdminPg.getAssignedToUserIds() == null) { | ||
| instanceAdminPg.setAssignedToUserIds(new HashSet<>()); | ||
| } | ||
| Set<String> assignedToUserIds = new HashSet<>(instanceAdminPg.getAssignedToUserIds()); | ||
| assignedToUserIds.removeAll(users.stream().map(User::getId).collect(Collectors.toList())); | ||
|
|
||
| private Mono<PermissionGroup> createInstanceAdminPermissionGroupWithoutPermissions() { | ||
| PermissionGroup instanceAdminPermissionGroup = new PermissionGroup(); | ||
| instanceAdminPermissionGroup.setName(FieldName.INSTANCE_ADMIN_ROLE); | ||
| BridgeUpdate updateObj = Bridge.update(); | ||
| String path = PermissionGroup.Fields.assignedToUserIds; | ||
|
|
||
| return permissionGroupRepository.save(instanceAdminPermissionGroup).flatMap(savedPermissionGroup -> { | ||
| Set<Permission> permissions = Set.of( | ||
| new Permission(savedPermissionGroup.getId(), READ_PERMISSION_GROUP_MEMBERS), | ||
| new Permission(savedPermissionGroup.getId(), ASSIGN_PERMISSION_GROUPS), | ||
| new Permission(savedPermissionGroup.getId(), UNASSIGN_PERMISSION_GROUPS)); | ||
| savedPermissionGroup.setPermissions(permissions); | ||
| updateObj.set(path, assignedToUserIds); | ||
| // Make Instance Admin is called before the first administrator is created. | ||
| Mono<Integer> updateInstanceAdminPgAssignmentMono = | ||
| permissionGroupRepository.updateById(instanceAdminPg.getId(), updateObj); | ||
|
|
||
| Policy readPermissionGroupPolicy = Policy.builder() | ||
| .permission(READ_PERMISSION_GROUP_MEMBERS.getValue()) | ||
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | ||
| .build(); | ||
| if (!hasLength(organizationAdminPg.getId())) { | ||
| return updateInstanceAdminPgAssignmentMono; | ||
| } | ||
|
|
||
| Policy assignPermissionGroupPolicy = Policy.builder() | ||
| .permission(ASSIGN_PERMISSION_GROUPS.getValue()) | ||
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | ||
| .build(); | ||
| // Also unassign the users from the organization admin permission group | ||
| Set<String> organizationAdminAssignedToUserIds = new HashSet<>(); | ||
| if (organizationAdminPg.getAssignedToUserIds() != null) { | ||
| organizationAdminAssignedToUserIds.addAll(organizationAdminPg.getAssignedToUserIds()); | ||
| } | ||
| organizationAdminAssignedToUserIds.removeAll( | ||
| users.stream().map(User::getId).collect(Collectors.toList())); | ||
| BridgeUpdate updateObj2 = Bridge.update(); | ||
| String path2 = PermissionGroup.Fields.assignedToUserIds; | ||
| updateObj2.set(path2, organizationAdminAssignedToUserIds); | ||
| return updateInstanceAdminPgAssignmentMono.then( | ||
| permissionGroupRepository.updateById(organizationAdminPg.getId(), updateObj2)); | ||
| }) | ||
| .then(Mono.just(users)) | ||
| .flatMapMany(Flux::fromIterable) | ||
| .flatMap(user -> permissionGroupRepository.evictAllPermissionGroupCachesForUser( | ||
| user.getEmail(), user.getOrganizationId())) | ||
| .then(Mono.just(Boolean.TRUE)); | ||
| } |
There was a problem hiding this comment.
Add safeguard against removing all instance administrators.
Similar to the makeInstanceAdministrator method, we should prevent removing all instance administrators.
Add validation to prevent removing all administrators:
public Mono<Boolean> removeInstanceAdmin(List<User> users) {
+ return getInstanceAdminPermissionGroup()
+ .filter(pg -> pg.getAssignedToUserIds() != null &&
+ pg.getAssignedToUserIds().size() > users.size())
+ .switchIfEmpty(Mono.error(
+ new IllegalStateException("Cannot remove all instance administrators")))
+ .then(Mono.defer(() -> {
// Existing method implementation
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Mono<Boolean> removeInstanceAdmin(List<User> users) { | |
| Mono<Tuple2<PermissionGroup, Config>> savedConfigAndPermissionGroupMono = | |
| createConfigAndPermissionGroupForSuperAdmin(); | |
| // TODO : Replace cloud hosting check with a feature flag check for multi tenancy | |
| boolean cloudHosting = commonConfig.isCloudHosting(); | |
| Mono<PermissionGroup> organizationAdminPgMono = Mono.just(new PermissionGroup()); | |
| // return the saved instance config | |
| return savedConfigAndPermissionGroupMono.map(Tuple2::getT2); | |
| } | |
| if (!cloudHosting) { | |
| organizationAdminPgMono = getDefaultOrganizationAdminPermissionGroup(); | |
| } | |
| protected Mono<Tuple2<PermissionGroup, Config>> createConfigAndPermissionGroupForSuperAdmin() { | |
| return Mono.zip(createInstanceAdminConfigObject(), createInstanceAdminPermissionGroupWithoutPermissions()) | |
| return Mono.zip(getInstanceAdminPermissionGroup(), organizationAdminPgMono) | |
| .flatMap(tuple -> { | |
| Config savedInstanceConfig = tuple.getT1(); | |
| PermissionGroup savedPermissionGroup = tuple.getT2(); | |
| // Update the instance config with the permission group id | |
| savedInstanceConfig.setConfig( | |
| new JSONObject(Map.of(DEFAULT_PERMISSION_GROUP, savedPermissionGroup.getId()))); | |
| Policy editConfigPolicy = Policy.builder() | |
| .permission(MANAGE_INSTANCE_CONFIGURATION.getValue()) | |
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | |
| .build(); | |
| Policy readConfigPolicy = Policy.builder() | |
| .permission(READ_INSTANCE_CONFIGURATION.getValue()) | |
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | |
| .build(); | |
| savedInstanceConfig.setPolicies(Set.of(editConfigPolicy, readConfigPolicy)); | |
| // Add config permissions to permission group | |
| Set<Permission> configPermissions = | |
| Set.of(new Permission(savedInstanceConfig.getId(), MANAGE_INSTANCE_CONFIGURATION)); | |
| return Mono.zip( | |
| addPermissionsToPermissionGroup(savedPermissionGroup, configPermissions), | |
| configRepository.save(savedInstanceConfig)); | |
| }); | |
| } | |
| PermissionGroup instanceAdminPg = tuple.getT1(); | |
| PermissionGroup organizationAdminPg = tuple.getT2(); | |
| private Mono<Config> createInstanceAdminConfigObject() { | |
| Config instanceAdminConfiguration = new Config(); | |
| instanceAdminConfiguration.setName(FieldName.INSTANCE_CONFIG); | |
| return configRepository.save(instanceAdminConfiguration); | |
| } | |
| if (instanceAdminPg.getAssignedToUserIds() == null) { | |
| instanceAdminPg.setAssignedToUserIds(new HashSet<>()); | |
| } | |
| Set<String> assignedToUserIds = new HashSet<>(instanceAdminPg.getAssignedToUserIds()); | |
| assignedToUserIds.removeAll(users.stream().map(User::getId).collect(Collectors.toList())); | |
| private Mono<PermissionGroup> createInstanceAdminPermissionGroupWithoutPermissions() { | |
| PermissionGroup instanceAdminPermissionGroup = new PermissionGroup(); | |
| instanceAdminPermissionGroup.setName(FieldName.INSTANCE_ADMIN_ROLE); | |
| BridgeUpdate updateObj = Bridge.update(); | |
| String path = PermissionGroup.Fields.assignedToUserIds; | |
| return permissionGroupRepository.save(instanceAdminPermissionGroup).flatMap(savedPermissionGroup -> { | |
| Set<Permission> permissions = Set.of( | |
| new Permission(savedPermissionGroup.getId(), READ_PERMISSION_GROUP_MEMBERS), | |
| new Permission(savedPermissionGroup.getId(), ASSIGN_PERMISSION_GROUPS), | |
| new Permission(savedPermissionGroup.getId(), UNASSIGN_PERMISSION_GROUPS)); | |
| savedPermissionGroup.setPermissions(permissions); | |
| updateObj.set(path, assignedToUserIds); | |
| // Make Instance Admin is called before the first administrator is created. | |
| Mono<Integer> updateInstanceAdminPgAssignmentMono = | |
| permissionGroupRepository.updateById(instanceAdminPg.getId(), updateObj); | |
| Policy readPermissionGroupPolicy = Policy.builder() | |
| .permission(READ_PERMISSION_GROUP_MEMBERS.getValue()) | |
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | |
| .build(); | |
| if (!hasLength(organizationAdminPg.getId())) { | |
| return updateInstanceAdminPgAssignmentMono; | |
| } | |
| Policy assignPermissionGroupPolicy = Policy.builder() | |
| .permission(ASSIGN_PERMISSION_GROUPS.getValue()) | |
| .permissionGroups(Set.of(savedPermissionGroup.getId())) | |
| .build(); | |
| // Also unassign the users from the organization admin permission group | |
| Set<String> organizationAdminAssignedToUserIds = new HashSet<>(); | |
| if (organizationAdminPg.getAssignedToUserIds() != null) { | |
| organizationAdminAssignedToUserIds.addAll(organizationAdminPg.getAssignedToUserIds()); | |
| } | |
| organizationAdminAssignedToUserIds.removeAll( | |
| users.stream().map(User::getId).collect(Collectors.toList())); | |
| BridgeUpdate updateObj2 = Bridge.update(); | |
| String path2 = PermissionGroup.Fields.assignedToUserIds; | |
| updateObj2.set(path2, organizationAdminAssignedToUserIds); | |
| return updateInstanceAdminPgAssignmentMono.then( | |
| permissionGroupRepository.updateById(organizationAdminPg.getId(), updateObj2)); | |
| }) | |
| .then(Mono.just(users)) | |
| .flatMapMany(Flux::fromIterable) | |
| .flatMap(user -> permissionGroupRepository.evictAllPermissionGroupCachesForUser( | |
| user.getEmail(), user.getOrganizationId())) | |
| .then(Mono.just(Boolean.TRUE)); | |
| } | |
| public Mono<Boolean> removeInstanceAdmin(List<User> users) { | |
| return getInstanceAdminPermissionGroup() | |
| .filter(pg -> pg.getAssignedToUserIds() != null && | |
| pg.getAssignedToUserIds().size() > users.size()) | |
| .switchIfEmpty(Mono.error( | |
| new IllegalStateException("Cannot remove all instance administrators"))) | |
| .then(Mono.defer(() -> { | |
| // TODO : Replace cloud hosting check with a feature flag check for multi tenancy | |
| boolean cloudHosting = commonConfig.isCloudHosting(); | |
| Mono<PermissionGroup> organizationAdminPgMono = Mono.just(new PermissionGroup()); | |
| if (!cloudHosting) { | |
| organizationAdminPgMono = getDefaultOrganizationAdminPermissionGroup(); | |
| } | |
| return Mono.zip(getInstanceAdminPermissionGroup(), organizationAdminPgMono) | |
| .flatMap(tuple -> { | |
| PermissionGroup instanceAdminPg = tuple.getT1(); | |
| PermissionGroup organizationAdminPg = tuple.getT2(); | |
| if (instanceAdminPg.getAssignedToUserIds() == null) { | |
| instanceAdminPg.setAssignedToUserIds(new HashSet<>()); | |
| } | |
| Set<String> assignedToUserIds = new HashSet<>(instanceAdminPg.getAssignedToUserIds()); | |
| assignedToUserIds.removeAll(users.stream().map(User::getId).collect(Collectors.toList())); | |
| BridgeUpdate updateObj = Bridge.update(); | |
| String path = PermissionGroup.Fields.assignedToUserIds; | |
| updateObj.set(path, assignedToUserIds); | |
| // Make Instance Admin is called before the first administrator is created. | |
| Mono<Integer> updateInstanceAdminPgAssignmentMono = | |
| permissionGroupRepository.updateById(instanceAdminPg.getId(), updateObj); | |
| if (!hasLength(organizationAdminPg.getId())) { | |
| return updateInstanceAdminPgAssignmentMono; | |
| } | |
| // Also unassign the users from the organization admin permission group | |
| Set<String> organizationAdminAssignedToUserIds = new HashSet<>(); | |
| if (organizationAdminPg.getAssignedToUserIds() != null) { | |
| organizationAdminAssignedToUserIds.addAll(organizationAdminPg.getAssignedToUserIds()); | |
| } | |
| organizationAdminAssignedToUserIds.removeAll( | |
| users.stream().map(User::getId).collect(Collectors.toList())); | |
| BridgeUpdate updateObj2 = Bridge.update(); | |
| String path2 = PermissionGroup.Fields.assignedToUserIds; | |
| updateObj2.set(path2, organizationAdminAssignedToUserIds); | |
| return updateInstanceAdminPgAssignmentMono.then( | |
| permissionGroupRepository.updateById(organizationAdminPg.getId(), updateObj2)); | |
| }) | |
| .then(Mono.just(users)) | |
| .flatMapMany(Flux::fromIterable) | |
| .flatMap(user -> permissionGroupRepository.evictAllPermissionGroupCachesForUser( | |
| user.getEmail(), user.getOrganizationId())) | |
| .then(Mono.just(Boolean.TRUE)); | |
| })); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (1)
113-119: Consider using bulk update for better performance.Since both updates are setting the same user IDs, consider combining them into a single bulk operation.
Example refactor:
-// Assign the users to the instance admin pg -Update update = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); -mongoTemplate.updateFirst(instanceAdminPgQuery, update, PermissionGroup.class); - -// Assign the users to the default organization admin pg -Update orgAdminUpdate = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); -mongoTemplate.updateFirst(orgAdminPermissionGroupQuery, orgAdminUpdate, PermissionGroup.class); +// Bulk update both permission groups +Query combinedQuery = new Query().addCriteria( + new Criteria().orOperator( + where(PermissionGroup.Fields.id).is(instanceAdminPermissionGroupId), + where(PermissionGroup.Fields.defaultDomainType).is(Organization.class.getSimpleName()) + .and(PermissionGroup.Fields.defaultDomainId).is(organization.getId()) + ) +); +Update bulkUpdate = new Update().set(PermissionGroup.Fields.assignedToUserIds, userIds); +mongoTemplate.updateMulti(combinedQuery, bulkUpdate, PermissionGroup.class);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration10000_UpdateSuperUser.java (2)
73-78: LGTM! Clear and descriptive query variable renaming.The renaming from
permissionGroupQuerytoinstanceAdminPgQueryimproves code clarity by explicitly indicating its purpose.
84-90: Consider adding null check for organization.The organization object is used to build the org admin query without verifying its existence.
Add a null check before proceeding:
Organization organization = mongoTemplate.findOne(organizationQuery, Organization.class); +if (organization == null) { + log.error("Default organization not found. Skipping org admin permission group update."); + return; +}
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (1)
125-176:⚠️ Potential issueAdd safeguard against removing all instance administrators.
Similar to the makeInstanceAdministrator method, we should prevent removing all instance administrators.
public Mono<Boolean> removeInstanceAdmin(List<User> users) { + return getInstanceAdminPermissionGroup() + .filter(pg -> pg.getAssignedToUserIds() != null && + pg.getAssignedToUserIds().size() > users.size()) + .switchIfEmpty(Mono.error( + new AppsmithException(AppsmithError.REMOVE_LAST_INSTANCE_ADMIN_ERROR))) + .flatMap(permissionGroup -> { // TODO : Replace cloud hosting check with a feature flag check for multi tenancy boolean cloudHosting = commonConfig.isCloudHosting(); Mono<PermissionGroup> organizationAdminPgMono = Mono.just(new PermissionGroup()); // Rest of the method... + }); }
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (1)
72-123: Implementation looks good but consider adding a cache invalidation strategy.The method effectively handles both instance and organization admin roles, but consider invalidating relevant caches to ensure consistency when permission assignments change.
📜 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/helpers/UserUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserUtils.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserUtilsCE.java (3)
4-8: Dependencies and injection look good.The added dependencies and constructor modifications properly support the refactoring for instance administration functionality.
Also applies to: 37-39, 45-53
178-193: Good use of caching for performance optimization.The caching implementation for instance admin permission group improves performance by avoiding repeated database queries.
195-208: Good implementation for retrieving the organization admin permission group.The method effectively uses caching and handles the organization context appropriately.
Description
To allow for partial organization related setting changes and not exposing instance level settings, introducing organization administrator role.
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
/test all
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13516210763
Commit: d4f1f97
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/Apps/PromisesApp_spec.js
- cypress/e2e/Regression/ClientSide/AdminSettings/Branding_settings_Spec.ts
- cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
- cypress/e2e/Regression/ClientSide/AdminSettings/General_settings_Spec.ts
- cypress/e2e/Regression/ClientSide/AuditLogs/Audit_logs_spec.js
- cypress/e2e/Regression/ClientSide/Branding/Branding_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Admin_settings_1_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Admin_settings_2_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Admin_settings_3_spec.ts
- cypress/e2e/Regression/ClientSide/Fork/ForkApplicationInDeployedMode_spec.ts
- cypress/e2e/Regression/ClientSide/Fork/ForkApplication_spec.ts
- cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js
- cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js
- cypress/e2e/Regression/ClientSide/Homepage/HomepageExperienceV2_spec.ts
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Analytics_spec.js
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/ExportApplication_spec.js
- cypress/e2e/Regression/ClientSide/ProductRamps/PrivateEmbedRamp_spec.ts
- cypress/e2e/Regression/ClientSide/SettingsPane/SettingsPane_EmbedSettings_spec.ts
- cypress/e2e/Regression/ClientSide/Workspace/DeleteWorkspace_spec.ts
- cypress/e2e/Regression/ClientSide/Workspace/MemberRoles_Spec.ts
- cypress/e2e/Regression/Enterprise/AdminSettings/Admin_Settings_NoAccess_spec.js
List of identified flaky tests.Tue, 25 Feb 2025 10:20:25 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit