Skip to content

Commit

Permalink
fix: NPE while creating a policies copy (#36172)
Browse files Browse the repository at this point in the history
## 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
#36063

/test all

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!CAUTION]
> 🔴 🔴 🔴 Some tests have failed.
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/10767073729>
> Commit: e225d54
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10767073729&attempt=2&selectiontype=test&testsstatus=failed&specsstatus=fail"
target="_blank">Cypress dashboard</a>.
> Tags: @tag.All
> Spec: 
> The following are new failures, please fix them before merging the PR:
<ol>
>
<li>cypress/e2e/Regression/ClientSide/Debugger/Widget_property_navigation_spec.ts
>
<li>cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts</ol>
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master"
target="_blank">List of identified flaky tests</a>.
> <hr>Mon, 09 Sep 2024 07:01:41 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
abhvsn committed Sep 9, 2024
1 parent 188e9f0 commit 1b16be8
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,8 @@ public Mono<T> setUserPermissionsInObject(T obj, Set<String> permissionGroups) {
Set<String> permissions = new HashSet<>();
obj.setUserPermissions(permissions);

Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies);
if (CollectionUtils.isEmpty(policies) || permissionGroups.isEmpty()) {
return Mono.just(obj);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ public <T extends BaseDomain> T addPoliciesToExistingObject(@NonNull Map<String,
policyMap1.put(entry.getKey(), policy);
}

final Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies);

// Append the user to the existing permission policy if it already exists.
for (Policy policy : policies) {
Expand Down Expand Up @@ -102,7 +103,8 @@ public <T extends BaseDomain> T removePoliciesFromExistingObject(Map<String, Pol
policyMap1.put(entry.getKey(), entry.getValue());
}

Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> 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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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<TestClass> 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<String> permissionGroups = new HashSet<>();
permissionGroups.add(UUID.randomUUID().toString());
obj = baseAppsmithRepositoryImpl
.setUserPermissionsInObject(obj, permissionGroups)
.block();
assertNotNull(obj);
Assertions.assertNull(obj.getPolicies());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ private static class TestClass extends BaseDomain {
TestClass() {}
}

@Test
void testAddNewPoliciesToNullPoliciesObject() {
TestClass obj = new TestClass();
obj.setPolicies(null);
Map<String, Policy> 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.
Expand Down

0 comments on commit 1b16be8

Please sign in to comment.