chore: Add user context to AclPermission argument for repository methods
#34263
Conversation
WalkthroughThe recent changes across various files aim to streamline handling user permissions and context within the Appsmith server backend. By removing unnecessary usage of Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
abhvsn
changed the title
AclPermission argument for repository methods
…add-user-context-to-acl-permission
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
abhvsn
added
ok-to-test
abhvsn
requested review from
sharat87,
nidhi-nair,
sondermanish and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (2)
Line range hint
248-248: The usage of a password in the test casecheckDecryptionOfAuthenticationDTOTestmay be flagged by security tools as sensitive information. However, since this is a test environment and likely uses dummy data, this is generally acceptable. Ensure that real credentials are never hardcoded, even in test environments.- String password = "password"; + String password = "dummyPassword"; // Use more clearly dummy data to avoid confusion
Line range hint
431-431: Similar to the previous comment, ensure that the password used intestDatasourceCreate_withUpdatableConnection_recreatesConnectionAlwaysis dummy data. Adjusting the naming or adding a comment could help clarify that these credentials are not real.- String password = "password"; + String password = "testPassword"; // Ensure this is dummy data
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (32)
- app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/PermissionAspect.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ReactiveContextUtils.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserPermissionUtils.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/applications/NewActionApplicationExportableServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomThemeRepositoryCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/BaseService.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (6 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (5 hunks)
Files skipped from review due to trivial changes (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java
Additional context used
Learnings (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (2)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29 Timestamp: 2024-06-03T11:52:27.242Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/applications/NewActionApplicationExportableServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29 Timestamp: 2024-06-03T11:52:27.242Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.
Gitleaks
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java
248-248: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches. (hashicorp-tf-password)
431-431: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches. (hashicorp-tf-password)
Additional comments not posted (38)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCE.java (1)
33-33: Ensure that the implementation ofgetPermissionGroupsForUserin the implementing class handles user contexts appropriately.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (1)
25-25: Ensure that the implementation ofsetUserPermissionsInObjectin the implementing class handles user contexts appropriately.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserPermissionUtils.java (1)
46-58: Good implementation ofupdateAclWithUserContext. The proactive clearing of user context to avoid data leaks is commendable.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/applications/NewActionApplicationExportableServiceCEImpl.java (1)
28-28: The method now correctly simplifies the permission parameter by passingnull. Ensure thatfindByPageIdsForExporthandlesnullpermissions appropriately.Verification successful
The method
findByPageIdsForExportinNewActionServiceCEImplcallsrepository.findByPageIds(unpublishedPages, permission)and processes the results. However, the handling ofnullfor thepermissionparameter is not explicitly shown in this snippet.To ensure that
nullpermissions are handled correctly, we need to verify howrepository.findByPageIdsmanages anullvalue forpermission.
The method
findByPageIdsinCustomNewActionRepositoryCEImplcallsqueryBuilder().criteria(...).permission(permission).all(). This indicates that thepermissionparameter is directly passed to the query builder. To ensure thatnullpermissions are handled correctly, we need to verify how thepermissionmethod in the query builder handles anullvalue.Let's extract the implementation of the
permissionmethod in the query builder to confirm its handling ofnull.
The
permissionmethod in the query builder checks if thepermissionparameter isnulland handles it accordingly. Specifically, ifpermissionisnull, it does not apply any permission-based criteria, allowing the query to proceed without restrictions.This confirms that the method
findByPageIdsForExportinNewActionServiceCEImplcan handlenullpermissions appropriately.
CustomNewActionRepositoryCEImpl.java: public Flux<NewAction> findByPageIds(List<String> pageIds, AclPermission permission) { ... .permission(permission) ... }BaseAppsmithRepositoryCEImpl.java: public static Criteria userAcl(Set<String> permissionGroups, AclPermission permission) { if (permission == null) { return null; } ... }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the method `findByPageIdsForExport` in `NewActionService` properly handles `null` permissions. # Test: Search for the method implementation and check for null handling. rg --type java $'findByPageIdsForExport' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionService.javaLength of output: 138
Script:
#!/bin/bash # Description: Search for the method `findByPageIdsForExport` in the entire repository to locate its implementation. # Test: Search for the method implementation. rg --type java 'findByPageIdsForExport'Length of output: 1559
Script:
#!/bin/bash # Description: Extract the implementation of the method `findByPageIdsForExport` in `NewActionServiceCEImpl` to verify null handling. # Test: Extract the method implementation. rg --type java -A 20 'public Flux<NewAction> findByPageIdsForExport' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaLength of output: 1197
Script:
#!/bin/bash # Description: Extract the implementation of the method `findByPageIds` in the repository to verify null handling. # Test: Extract the method implementation. rg --type java -A 20 'findByPageIds' app/server/appsmith-server/src/main/java/com/appsmith/server/repositoriesLength of output: 12675
Script:
#!/bin/bash # Description: Extract the implementation of the `permission` method in the query builder to verify null handling. # Test: Extract the method implementation. rg --type java -A 20 'permission' app/server/appsmith-server/src/main/java/com/appsmith/server/repositoriesLength of output: 301930
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomThemeRepositoryCEImpl.java (3)
32-36: Correctly updates theREAD_THEMESpermission with user context before querying. This ensures that permissions are accurately managed with respect to the user.
41-45: This method also correctly updates theREAD_THEMESpermission with user context, maintaining consistency in permission handling across methods.
49-54: Properly updates theMANAGE_THEMESpermission with user context, which is essential for operations that modify theme data.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomPermissionGroupRepositoryCEImpl.java (1)
75-76: Correctly delegates the functionality to the superclass, simplifying the implementation and potentially reducing errors by reusing tested code.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (1)
66-66: > [!TIP]Codebase Verification
The implementations of
findByDefaultApplicationIdin bothCustomActionCollectionRepositoryCEImpl.javaandCustomNewActionRepositoryCEImpl.javado not include explicit null checks for thepermissionparameter. This could potentially lead to null pointer exceptions ifpermissionis null.
CustomActionCollectionRepositoryCEImpl.java:public Flux<ActionCollection> findByDefaultApplicationId(String defaultApplicationId, AclPermission permission) { BridgeQuery<ActionCollection> query = Bridge.<ActionCollection>equal( BranchAwareDomain.Fields.defaultResources_applicationId, defaultApplicationId) .isNull(ActionCollection.Fields.unpublishedCollection_deletedAt); return queryBuilder().criteria(query).permission(permission).all(); }
CustomNewActionRepositoryCEImpl.java:public Flux<NewAction> findByDefaultApplicationId(String defaultApplicationId, AclPermission permission) { final String defaultResources = BranchAwareDomain.Fields.defaultResources; return queryBuilder() .criteria(Bridge.equal(NewAction.Fields.defaultResources_applicationId, defaultApplicationId) .isNull(NewAction.Fields.unpublishedAction_deletedAt)) .permission(permission) .all(); }To avoid potential null pointer exceptions, the method should include a null check for the
permissionparameter.Recommended Action:
Add null checks for thepermissionparameter in both method implementations.
CustomActionCollectionRepositoryCEImpl.java:public Flux<ActionCollection> findByDefaultApplicationId(String defaultApplicationId, AclPermission permission) { if (permission == null) { // Handle null permission appropriately, e.g., log a warning or throw an exception } BridgeQuery<ActionCollection> query = Bridge.<ActionCollection>equal( BranchAwareDomain.Fields.defaultResources_applicationId, defaultApplicationId) .isNull(ActionCollection.Fields.unpublishedCollection_deletedAt); return queryBuilder().criteria(query).permission(permission).all(); }
CustomNewActionRepositoryCEImpl.java:public Flux<NewAction> findByDefaultApplicationId(String defaultApplicationId, AclPermission permission) { if (permission == null) { // Handle null permission appropriately, e.g., log a warning or throw an exception } final String defaultResources = BranchAwareDomain.Fields.defaultResources; return queryBuilder() .criteria(Bridge.equal(NewAction.Fields.defaultResources_applicationId, defaultApplicationId) .isNull(NewAction.Fields.unpublishedAction_deletedAt)) .permission(permission) .all(); }CustomActionCollectionRepositoryCEImpl.java: Line 3
CustomNewActionRepositoryCEImpl.java: Line 3
Analysis chain
The method now accepts a direct
nullfor the second parameter instead ofOptional.empty(). This change simplifies the method signature. Validate that the method correctly handlesnullvalues to avoid potential null pointer exceptions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify null handling in `findByDefaultApplicationId`. # Test: Check method implementation for null checks. Expect: Proper null handling. rg --type java 'findByDefaultApplicationId' -A 10 -B 10Length of output: 22102
Script:
#!/bin/bash # Description: Verify null handling in `findByDefaultApplicationId` method implementations. # Check the method implementation for null checks to avoid potential null pointer exceptions. # Search for the method implementation in CustomActionCollectionRepositoryCEImpl.java rg 'public Flux<ActionCollection> findByDefaultApplicationId' -A 20 app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java # Search for the method implementation in CustomNewActionRepositoryCEImpl.java rg 'public Flux<NewAction> findByDefaultApplicationId' -A 20 app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.javaLength of output: 2653
Skipped due to learnings
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29 Timestamp: 2024-06-03T11:52:27.242Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
150-150: The methodsfindByPageIdsandfindByPageIdsForExportnow directly acceptAclPermissionwithout usingOptional. This is consistent with the simplification across the service layer. Confirm that all usages of these methods handle the direct type correctly.Also applies to: 152-152
Verification successful
The methods
findByPageIdsandfindByPageIdsForExportare correctly defined to acceptAclPermissiondirectly. Now, let's verify the usages of these methods to ensure they handle the directAclPermissiontype correctly.Usages of
findByPageIds:
NewActionServiceCEImpl.java(lines 150-152)CustomNewActionRepositoryCEImpl.java(lines 150-152)Usages of
findByPageIdsForExport:
NewActionServiceCEImpl.java(lines 152-154)NewActionApplicationExportableServiceCEImpl.java(lines 150-152)ApplicationForkingServiceCEImpl.java(lines 150-152)ActionCollectionApplicationExportableServiceCEImpl.java(lines 150-152)Let's inspect these files to confirm that each usage correctly handles the direct
AclPermissiontype.
The verification process confirms that the methods
findByPageIdsandfindByPageIdsForExportare correctly defined to acceptAclPermissiondirectly and that all usages of these methods handle the directAclPermissiontype correctly.
NewActionServiceCEImpl.java: Methods are used with directAclPermissiontype.CustomNewActionRepositoryCEImpl.java: Method is used with directAclPermissiontype.NewActionApplicationExportableServiceCEImpl.java: Method is used with directAclPermissiontype.ApplicationForkingServiceCEImpl.java: Method is used with directAclPermissiontype.ActionCollectionApplicationExportableServiceCEImpl.java: Method is used with directAclPermissiontype.Thus, the review comment is accurate and the changes have been correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify direct type handling in `findByPageIds` and `findByPageIdsForExport`. # Test: Search for method usages. Expect: Correct handling of direct `AclPermission` type. rg --type java 'findByPageIds' -A 5 -B 5 rg --type java 'findByPageIdsForExport' -A 5 -B 5Length of output: 34738
Script:
#!/bin/bash # Verify the handling of direct AclPermission type in the usages of findByPageIds and findByPageIdsForExport # Check the usage in NewActionServiceCEImpl.java rg --type java 'findByPageIds' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java rg --type java 'findByPageIdsForExport' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java # Check the usage in CustomNewActionRepositoryCEImpl.java rg --type java 'findByPageIds' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java # Check the usage in NewActionApplicationExportableServiceCEImpl.java rg --type java 'findByPageIdsForExport' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/applications/NewActionApplicationExportableServiceCEImpl.java # Check the usage in ApplicationForkingServiceCEImpl.java rg --type java 'findByPageIdsForExport' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java # Check the usage in ActionCollectionApplicationExportableServiceCEImpl.java rg --type java 'findByPageIdsForExport' -A 5 -B 5 app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/exportable/applications/ActionCollectionApplicationExportableServiceCEImpl.javaLength of output: 4215
app/server/appsmith-server/src/main/java/com/appsmith/server/acl/AclPermission.java (1)
135-137: The addition of auserfield inAclPermissionwith a setter method enhances the flexibility to handle user context in permission checks. Ensure that this new field is properly utilized in contexts where user-specific permissions are critical.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (2)
185-190: The implementation of user context in permission setting is correctly applied usingReactiveContextUtils.getCurrentUser().
208-209: Ensure that user context is consistently applied in error handling paths for permission checks.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (3)
293-294: Ensure that the methodfindByPageIdis updated to reflect the removal ofOptionalin its signature.
351-351: Ensure that the methodfindByPageIdis updated to reflect the removal ofOptionalin its signature.
412-413: Ensure that the methodfindByPageIdis updated to reflect the removal ofOptionalin its signature.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2)
160-173: The refactoring to directly useUserinstead ofOptionalfor permission groups retrieval is a good improvement in terms of code clarity and performance.
400-401: Correct implementation of setting user permissions in the object. This method now properly utilizes the user context for permission checks.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (1)
434-435: Ensure proper user context handling in permission updates.This change integrates the user context into permission setting by fetching the current user and updating permissions accordingly. This aligns with the PR's objective to enhance user context handling across repository methods.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (1)
Line range hint
182-183: This implementation usesReactiveContextUtils.getCurrentUser()to fetch the current user and then sets user permissions on the workspace object. This aligns with the PR's objective to enhance user context handling in permission checks.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (2)
182-183: The implementation correctly fetches the current user usingReactiveContextUtils.getCurrentUser()and sets user permissions on the page object. This is part of the broader initiative to enhance user context in permission checks, ensuring that permissions are accurately set based on the current user context.
[APROVED]
597-604: This change introduces enhanced permission handling by incorporating user context into the ACL permission checks for pages. It's a good practice as it aligns with the overall objectives of the PR to enhance security and context-aware permission management.app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (1)
195-198: The methodfindByIdis used here with the specific permissiondatasourcePermission.getDeletePermission()anddatasourcePermission.getExecutePermission(). This change aligns with the PR's goal to enhance permission handling by integrating user context more deeply into permission checks. Ensure that the permission levels are correctly set for each operation to prevent unauthorized access.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1)
180-180: Add user context to permission checks for enhanced security.The use of
ReactiveContextUtils.getCurrentUser()across various methods ensures that user context is appropriately captured and utilized for permission validation, aligning with the PR's objectives.Also applies to: 230-230, 278-278, 308-308, 353-353
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (4)
17-17: Added import statement forUserclass.This is necessary for the new user context handling in permissions.
21-21: Added import statement forReactiveContextUtils.This utility will be crucial for handling reactive user contexts, aligning with the PR's objective of enhancing user context handling.
300-300: Updated method calls tosetUserPermissionsInObjectto includeUserparameter.This change aligns with the PR's goal of adding user context to permission checks. It's also a good practice to explicitly handle user permissions rather than inferring them implicitly.
Also applies to: 405-405
317-329: Mocking static methodgetCurrentUserfromReactiveContextUtilsand verifying the user permissions in the assertions.This is a necessary change to ensure that the user context is correctly integrated and tested. The use of
MockedStaticis appropriate here to isolate the testing environment from external static method calls, which is a common practice in unit testing for static methods.Also applies to: 422-443
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (2)
338-339: Integrate user context into repository method calls effectively.The integration of
ReactiveContextUtils.getCurrentUser()to fetch the current user and pass it tosetUserPermissionsInObjectaligns with the PR's objective of adding user context to permissions. This ensures that the user's permissions are checked correctly based on the current user context, enhancing security and functionality.
691-692: Ensure proper user context handling during the save operation.The addition of user context to the
saveoperation by fetching the current user withReactiveContextUtils.getCurrentUser()and passing it tosetUserPermissionsInObjectis a good practice. This ensures that user-specific permissions are correctly applied when action collections are saved, which is crucial for maintaining proper access control and data integrity.app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (2)
32-32: Added imports forReactiveContextUtilsandUserPermissionUtilsindicate a shift towards utilizing these utilities for reactive user context handling and permission updates within the class. Ensure these utilities are used consistently throughout the class.
Line range hint
602-636: Significant changes were made to useReactiveContextUtils.getCurrentUser()for fetching the current user and setting permissions on various domain objects like pages, actions, and workspaces. This is a good use of reactive programming to ensure permissions are dynamically adjusted based on the user context.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
267-267: Ensure proper handling ofMono<User>andWorkspaceinstances for null values.This script checks if null values are being handled properly in the method
generateAndSetDatasourcePolicies, especially sinceMono<User>andWorkspacecould potentially be empty.Verification successful
The
generateAndSetDatasourcePoliciesmethod properly handles potential null values forMono<User>andWorkspaceinstances:
- The
Mono<User>is handled usingflatMap, which ensures the method proceeds only ifuserMonoemits aUser.- The
Workspaceis retrieved with aswitchIfEmptyclause that throws an exception if noWorkspaceis found, effectively handling the null case.No further action is required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper null checks for `user` and `workspace` in `generateAndSetDatasourcePolicies`. # Test: Search for null checks in the method implementation. rg --type java 'generateAndSetDatasourcePolicies' -C 20Length of output: 13362
390-391: Ensure user context is properly propagated and handled.This script will help confirm that the user context obtained from
ReactiveContextUtils.getCurrentUser()is being correctly used and propagated through the system, particularly in the methodvalidateAndSaveDatasourceToRepository.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (1)
Line range hint
1427-1457: Ensure user context is properly added to permissions handling in page, action, and action collection objects.This change integrates the user context into the ACL checks for pages, actions, and action collections. It uses the
ReactiveContextUtils.getCurrentUser()method to fetch the current user and passes this user to thesetUserPermissionsInObjectmethod. This is a crucial enhancement for security and context-aware permissions handling.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3)
285-286: The addition of user context to thesetUserPermissionsInObjectmethod helps in enhancing security and personalization by ensuring that permissions are set based on the current user context. This aligns with the PR's objective to improve permission management with user context.
771-771: The removal ofOptionalfrom the method signature and directly passingnullfor permissions and sort parameters is a good simplification. It aligns with the Java best practices of avoiding unnecessary use ofOptionalfor method parameters, which can lead to cleaner and more readable code.
1708-1714: The refactoring to removeOptionalfrom the method signatures and directly handle the parameters simplifies the code and improves readability. By directly passing thepermission, the code becomes less complex and easier to understand.
| .zipWith(ReactiveContextUtils.getCurrentUser()) | ||
| .flatMap(tuple2 -> { | ||
| IdPoliciesOnly idPolicy = tuple2.getT1(); | ||
| User user = tuple2.getT2(); | ||
| ; | ||
| Datasource datasource = new Datasource(); | ||
| datasource.setId(idPolicy.getId()); | ||
| datasource.setPolicies(idPolicy.getPolicies()); | ||
| return datasourceRepository.setUserPermissionsInObject(datasource); | ||
| return datasourceRepository.setUserPermissionsInObject(datasource, user); |
There was a problem hiding this comment.
Refactor the handling of user permissions in datasources to ensure proper context.
The current implementation fetches the current user and sets permissions in the datasource object. However, this block of code can be refactored for better readability and efficiency. Consider encapsulating the logic into a separate method to handle user permissions for datasources, which can be reused elsewhere if needed.
private Mono<Datasource> setUserPermissionsForDatasource(Datasource datasource) {
return ReactiveContextUtils.getCurrentUser()
.flatMap(user -> datasourceRepository.setUserPermissionsInObject(datasource, user));
}Then replace the current implementation with a call to this new method.
| @@ -192,7 +193,7 @@ private Mono<Datasource> createEx(@NotNull Datasource datasource, Optional<AclPe | |||
| }) | |||
| .flatMap(datasource1 -> { | |||
| Mono<User> userMono = sessionUserService.getCurrentUser(); | |||
| return generateAndSetDatasourcePolicies(userMono, datasource1, permission); | |||
| return generateAndSetDatasourcePolicies(userMono, datasource1, permission.orElse(null)); | |||
There was a problem hiding this comment.
Consider using Optional's map method to simplify the code.
- return generateAndSetDatasourcePolicies(userMono, datasource1, permission.orElse(null));
+ return permission.map(perm -> generateAndSetDatasourcePolicies(userMono, datasource1, perm))
+ .orElseGet(() -> generateAndSetDatasourcePolicies(userMono, datasource1, null));This change makes it clearer that permission is being used to conditionally alter the behavior of generateAndSetDatasourcePolicies.
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.
| return generateAndSetDatasourcePolicies(userMono, datasource1, permission.orElse(null)); | |
| return permission.map(perm -> generateAndSetDatasourcePolicies(userMono, datasource1, perm)) | |
| .orElseGet(() -> generateAndSetDatasourcePolicies(userMono, datasource1, null)); |
| @@ -192,7 +193,7 @@ public Mono<List<String>> forkApplications( | |||
| return createForkedPageMono.flatMap(savedPage -> { | |||
| clonedPages.add(savedPage); | |||
| Flux<NewAction> sourceActionFlux = newActionService | |||
| .findByPageIdsForExport(List.of(templatePageId), Optional.empty()) | |||
| .findByPageIdsForExport(List.of(templatePageId), null) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The method findByPageIdsForExport in NewActionServiceCEImpl.java directly passes the permission parameter to the repository.findByPageIds method without any null checks or special handling for null values. This could potentially lead to security issues if the repository method does not handle null permissions correctly.
NewActionServiceCEImpl.java:public Flux<NewAction> findByPageIdsForExport(List<String> unpublishedPages, AclPermission permission) { return repository.findByPageIds(unpublishedPages, permission).doOnNext(newAction -> { this.setCommonFieldsFromNewActionIntoAction(newAction, newAction.getUnpublishedAction()); this.setCommonFieldsFromNewActionIntoAction(newAction, newAction.getPublishedAction()); }); }
Analysis chain
The call to findByPageIdsForExport has been modified to pass null for the ACL permission parameter, which might be intended to fetch all actions irrespective of permissions. However, this could lead to security issues if not handled correctly inside the method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the method `findByPageIdsForExport` correctly handles null ACL permissions.
ast-grep --lang java --pattern $'class NewActionService {
$$$
findByPageIdsForExport($_, $_) {
$$$
}
$$$
}'
Length of output: 122
Script:
#!/bin/bash
# Description: Find the exact location of the `findByPageIdsForExport` method definition.
rg 'findByPageIdsForExport' --glob '*.java'
Length of output: 1563
Script:
#!/bin/bash
# Description: Extract the implementation of the `findByPageIdsForExport` method in `NewActionServiceCEImpl.java`.
ast-grep --lang java --pattern $'class NewActionServiceCEImpl {
$$$
findByPageIdsForExport($_, $_) {
$$$
}
$$$
}'
Length of output: 128
Script:
#!/bin/bash
# Description: Extract the implementation of the `findByPageIdsForExport` method in `NewActionServiceCEImpl.java`.
rg 'findByPageIdsForExport' -A 20 app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Length of output: 1160
| if (!StringUtils.hasLength(branchName)) { | ||
| return UserPermissionUtils.updateAclWithUserContext(aclPermission).then(Mono.defer(() -> repository | ||
| .queryBuilder() | ||
| .byId(defaultApplicationId) | ||
| .fields(projectionFieldNames) | ||
| .permission(aclPermission) | ||
| .one() | ||
| .switchIfEmpty(Mono.error(new AppsmithException( | ||
| AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION, defaultApplicationId))); | ||
| AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION, defaultApplicationId))))); |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate multiple instances where defaultApplicationId is used, but there are no null checks found for this variable. Given this, the concern about a potential NullPointerException is valid.
defaultApplicationIdis used in various methods and queries.- There are no explicit null checks for
defaultApplicationIdin the file.
To prevent potential NullPointerException, consider adding a null check for defaultApplicationId before using it in a query.
- Lines 817-825 in
ApplicationServiceCEImpl.java
Category:
Analysis chain
Consider adding a null check for defaultApplicationId before using it in a query to prevent potential NullPointerException.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `defaultApplicationId` across the codebase to ensure it's always non-null when passed to `findByBranchNameAndDefaultApplicationId`.
# Test: Search for the function usage. Expect: No occurrences of passing a null `defaultApplicationId`.
rg --type java $'findByBranchNameAndDefaultApplicationId\\([^,]+, null'
Length of output: 71
Script:
#!/bin/bash
# Description: Verify if `defaultApplicationId` is ever null before being used in the query in the file `ApplicationServiceCEImpl.java`.
# Search for the declaration and assignments of `defaultApplicationId` in the file.
rg --type java --word-regex 'defaultApplicationId' app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
# Search for null checks on `defaultApplicationId` in the file.
rg --type java --word-regex 'defaultApplicationId == null' app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
Length of output: 1217
Script:
#!/bin/bash
# Description: Verify if `defaultApplicationId` is ever null before being used in the query in the file `ApplicationServiceCEImpl.java`.
# Search for the declaration and assignments of `defaultApplicationId` in the file.
rg --type java --word-regexp 'defaultApplicationId' app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
# Search for null checks on `defaultApplicationId` in the file.
rg --type java --word-regexp 'defaultApplicationId == null' app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
Length of output: 3797
| Mono<User> currentUserMono = ReactiveContextUtils.getCurrentUser(); | ||
|
|
||
| Flux<T> result = currentUserMono.flatMapMany(user -> { | ||
| permission.setUser(user); | ||
| return repository | ||
| .queryBuilder() | ||
| .criteria(Bridge.or(criteria)) | ||
| .permission(permission) | ||
| .sort(sort) | ||
| .includeAnonymousUserPermissions(false) | ||
| .all(); | ||
| }); |
There was a problem hiding this comment.
Tip
Codebase Verification
The implementation of ReactiveContextUtils.getCurrentUser() does not include any explicit error handling. It simply retrieves the security context and maps the authentication principal to a User object. Given this, the original review comment suggesting the addition of error handling is valid.
- Consider adding error handling for the user fetch operation to ensure that the service remains robust even if user details are not available or fail to fetch.
Category:
Analysis chain
This implementation of fetching user context and setting permissions is well-integrated within the repository query. However, consider adding error handling for the user fetch operation to ensure that the service remains robust even if user details are not available or fail to fetch.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling when user details fail to fetch.
# Test: Simulate a failure in fetching user details. Expect: Graceful error handling without service disruption.
rg --type java 'ReactiveContextUtils.getCurrentUser()' -A 10 -B 10
Length of output: 64932
Script:
#!/bin/bash
# Search for the implementation of ReactiveContextUtils.getCurrentUser()
ast-grep --lang java --pattern 'class ReactiveContextUtils { $$$ getCurrentUser() { $$$ } $$$ }'
Length of output: 98
Script:
#!/bin/bash
# Search for the definition of the getCurrentUser method in ReactiveContextUtils
rg 'Mono<User> getCurrentUser' -A 10 -B 10
Length of output: 7645
| @@ -58,13 +57,13 @@ public void renameContextInImportableResources( | |||
|
|
|||
| @Override | |||
| public Flux<NewAction> getExistingResourcesInCurrentArtifactFlux(Artifact artifact) { | |||
| return repository.findByApplicationId(artifact.getId(), Optional.empty(), Optional.empty()); | |||
| return repository.findByApplicationId(artifact.getId()); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The review comment correctly identifies the need to update all invocations of findByApplicationId to the simplified method signature. However, the following invocations still use the old method signature with Optional.empty():
app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java
Please update these invocations to align with the new method signature.
Analysis chain
The method findByApplicationId now directly uses the artifact ID without the previously used Optional parameter, aligning with the simplification strategy. Ensure that all invocations of this method across the codebase have been updated accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all invocations of `findByApplicationId` have been updated.
# Test: Search for old method signature. Expect: No occurrences of the old method signature.
rg --type java 'findByApplicationId(.*, Optional.empty())'
Length of output: 538
| public class ReactiveContextUtils { | ||
| public static Mono<User> getCurrentUser() { | ||
| return ReactiveSecurityContextHolder.getContext() | ||
| .map(SecurityContext::getAuthentication) | ||
| .map(auth -> (User) auth.getPrincipal()); | ||
| } |
There was a problem hiding this comment.
Consider adding type checking before casting the authentication principal to User to prevent potential ClassCastException.
- .map(auth -> (User) auth.getPrincipal());
+ .map(auth -> {
+ if (auth.getPrincipal() instanceof User) {
+ return (User) auth.getPrincipal();
+ } else {
+ return Mono.error(new ClassCastException("Principal is not an instance of User"));
+ }
+ });Committable suggestion was skipped due to low confidence.
| @Around("execution(* com.appsmith.server.repositories..*(..))") | ||
| public Object handlePermission(ProceedingJoinPoint joinPoint) throws Throwable { | ||
|
|
||
| Class<?> returnType = | ||
| ((MethodSignature) joinPoint.getSignature()).getMethod().getReturnType(); | ||
| if (!Mono.class.isAssignableFrom(returnType) && !Flux.class.isAssignableFrom(returnType)) { | ||
| return joinPoint.proceed(joinPoint.getArgs()); | ||
| } | ||
|
|
||
| AclPermission permissionWithoutUserContext = Arrays.stream(joinPoint.getArgs()) | ||
| .filter(arg -> arg instanceof AclPermission | ||
| // TODO (Abhijeet): This is a temporary fix to avoid Optional<AclPermission> in the repository | ||
| // methods. | ||
| || (arg instanceof Optional && ((Optional<?>) arg).orElse(null) instanceof AclPermission)) | ||
| .map(arg -> { | ||
| if (arg instanceof AclPermission) { | ||
| return (AclPermission) arg; | ||
| } | ||
| return (AclPermission) ((Optional<?>) arg).orElse(null); | ||
| }) | ||
| // We expect only one permission object to be passed to the repository methods. | ||
| .findFirst() | ||
| .orElse(null); | ||
| if (permissionWithoutUserContext == null) { | ||
| return joinPoint.proceed(joinPoint.getArgs()); | ||
| } | ||
|
|
||
| Mono<AclPermission> permissionMono = updateAclWithUserContext(permissionWithoutUserContext); | ||
| if (Mono.class.isAssignableFrom(returnType)) { | ||
| return permissionMono.then(Mono.defer(() -> { | ||
| try { | ||
| return (Mono<?>) joinPoint.proceed(joinPoint.getArgs()); | ||
| } catch (Throwable e) { | ||
| log.error( | ||
| "Error occurred while adding the user context to the permission object when invoking function {}", | ||
| joinPoint.getSignature().getName(), | ||
| e); | ||
| return Mono.error(e); | ||
| } | ||
| })); | ||
| } else if (Flux.class.isAssignableFrom(returnType)) { | ||
| return permissionMono.thenMany(Flux.defer(() -> { | ||
| try { | ||
| return (Flux<?>) joinPoint.proceed(joinPoint.getArgs()); | ||
| } catch (Throwable e) { | ||
| log.error( | ||
| "Error occurred while adding the user context to the permission object when invoking function {}", | ||
| joinPoint.getSignature().getName(), | ||
| e); | ||
| return Flux.error(e); | ||
| } | ||
| })); | ||
| } | ||
| return joinPoint.proceed(joinPoint.getArgs()); | ||
| } |
There was a problem hiding this comment.
The implementation of permission handling using aspect-oriented programming is correct. It effectively updates permissions with user context and handles both Mono and Flux types. Consider adding more detailed logging for better traceability of permission updates.
+ log.debug("Updating permissions for method: {}", joinPoint.getSignature().getName());Committable suggestion was skipped due to low confidence.
Description
PR to add the user context to the permissions object passed in as argument for the repository methods.
Why this is needed
We are using non-reactive driver (JDBC) for Postgres which makes the DB calls synchronous in nature. This is forcing us to add blocking calls to fetch the permission groups for the user. As a part of this call we are fetching the current user from the
ReactiveSecurityContext, but with blocking call this results in null value. To fix this we are going ahead with adding the user object in theAclPermissionin the reactive chain itself to avoid the dependency ofReactiveSecurityContextin the synchronous call./test all
Communication
Should the DevRel and Marketing teams inform users about this change?
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9583347634
Commit: a2e0212
Cypress dashboard.
Tags: ``