From 1b16be896d10a44803b2d8279794e0eb11c02b60 Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Mon, 9 Sep 2024 13:06:43 +0530 Subject: [PATCH] fix: NPE while creating a policies copy (#36172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description PR to handle the empty collection and null values for policies. We expect web should never end up in this state but we have received enough evidences that because of broken functionality in GAC we may end up in this scenario. One such example is https://github.com/appsmithorg/appsmith/issues/36063 /test all ### :mag: Cypress test results > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: > Commit: e225d54a2ca108792a53fe54a1db96e4b1c3b1cc > Cypress dashboard. > Tags: @tag.All > Spec: > The following are new failures, please fix them before merging the PR:
    >
  1. cypress/e2e/Regression/ClientSide/Debugger/Widget_property_navigation_spec.ts >
  2. cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts
> List of identified flaky tests. >
Mon, 09 Sep 2024 07:01:41 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of user permissions to prevent potential errors related to null values in policies. - Enhanced robustness in policy management methods to avoid null-related errors when retrieving existing policies. - **Tests** - Added unit tests for user permissions handling and policy management to ensure robust behavior under various conditions. --- .../ce/BaseAppsmithRepositoryCEImpl.java | 3 +- .../solutions/ce/PolicySolutionCEImpl.java | 6 ++- .../ce/BaseAppsmithRepositoryImplTest.java | 52 +++++++++++++++++++ .../ce/PolicySolutionCEImplTest.java | 12 +++++ 4 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryImplTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java index 7d131e95b48..8a297df4bf0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java @@ -416,7 +416,8 @@ public Mono setUserPermissionsInObject(T obj, Set permissionGroups) { Set permissions = new HashSet<>(); obj.setUserPermissions(permissions); - Set policies = new HashSet<>(obj.getPolicies()); + Set existingPolicies = obj.getPolicies(); + final Set policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies); if (CollectionUtils.isEmpty(policies) || permissionGroups.isEmpty()) { return Mono.just(obj); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java index 031b938313e..8617fbf739d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java @@ -69,7 +69,8 @@ public T addPoliciesToExistingObject(@NonNull Map policies = new HashSet<>(obj.getPolicies()); + Set existingPolicies = obj.getPolicies(); + final Set policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies); // Append the user to the existing permission policy if it already exists. for (Policy policy : policies) { @@ -102,7 +103,8 @@ public T removePoliciesFromExistingObject(Map policies = new HashSet<>(obj.getPolicies()); + Set existingPolicies = obj.getPolicies(); + final Set policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies); // Remove the user from the existing permission policy if it exists. for (Policy policy : policies) { String permission = policy.getPermission(); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryImplTest.java new file mode 100644 index 00000000000..4678384ff29 --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryImplTest.java @@ -0,0 +1,52 @@ +package com.appsmith.server.repositories.ce; + +import com.appsmith.external.models.BaseDomain; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.HashSet; +import java.util.Set; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +class BaseAppsmithRepositoryImplTest { + + BaseAppsmithRepositoryCEImpl baseAppsmithRepositoryImpl; + + @BeforeEach + public void setup() { + baseAppsmithRepositoryImpl = new BaseAppsmithRepositoryCEImpl<>() {}; + } + + class TestClass extends BaseDomain {} + + @Test + void testSetUserPermissionsInObject_whenPoliciesIsEmptySet_emptyCollectionValueIsSet() { + // Test the method setPoliciesInObject when the policies are null + // The method should set an empty collection value in the object + // The method should return the object + TestClass obj = baseAppsmithRepositoryImpl + .setUserPermissionsInObject(new TestClass(), null) + .block(); + assertNotNull(obj); + Assertions.assertEquals(0, obj.getPolicies().size()); + } + + @Test + void testSetUserPermissionsInObject_whenPoliciesIsNull_nullPoliciesAreSet() { + // Test the method setPoliciesInObject when the policies are empty + // The method should set an empty collection value in the object + // The method should return the object + TestClass obj = new TestClass(); + obj.setPolicies(null); + Set permissionGroups = new HashSet<>(); + permissionGroups.add(UUID.randomUUID().toString()); + obj = baseAppsmithRepositoryImpl + .setUserPermissionsInObject(obj, permissionGroups) + .block(); + assertNotNull(obj); + Assertions.assertNull(obj.getPolicies()); + } +} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java index 8adbefd23b6..fde23ba9fad 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/PolicySolutionCEImplTest.java @@ -32,6 +32,18 @@ private static class TestClass extends BaseDomain { TestClass() {} } + @Test + void testAddNewPoliciesToNullPoliciesObject() { + TestClass obj = new TestClass(); + obj.setPolicies(null); + Map policyMap = new HashMap<>(); + policyMap.put("read", new Policy("read", new HashSet<>(Set.of("group1")))); + + BaseDomain result = policySolution.addPoliciesToExistingObject(policyMap, obj); + + assertTrue(result.getPolicies().containsAll(policyMap.values())); + } + @Test void testAddNewPoliciesToEmptyObject() { BaseDomain obj = new TestClass(); // Assuming BaseDomain has a default empty set of policies.