AB#1233 Prevent loops when merging organizations/locations

NCI-Agency · Jan 23, 2025 · 8555839 · 8555839
1 parent 8029a64
commit 8555839
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 21 deletions.
diff --git a/src/main/java/mil/dds/anet/resources/LocationResource.java b/src/main/java/mil/dds/anet/resources/LocationResource.java
@@ -128,16 +128,7 @@ public Integer updateLocation(@GraphQLRootContext GraphQLContext context,
     assertPermission(user, DaoUtils.getUuid(l));
 
     // Check for loops in the hierarchy
-    if (!Utils.isEmptyOrNull(l.getParentLocations())) {
-      final Set<String> parentLocationUuids =
-          l.getParentLocations().stream().map(Location::getUuid).collect(Collectors.toSet());
-      final Map<String, Set<String>> children = engine.buildLocationHash(DaoUtils.getUuid(l), true);
-      children.keySet().retainAll(parentLocationUuids);
-      if (!children.isEmpty()) {
-        throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR,
-            "Location can not be its own (grand…)parent");
-      }
-    }
+    checkForLoops(l.getUuid(), l.getParentLocations());
 
     final int numRows = dao.update(l);
     if (numRows == 0) {
@@ -172,6 +163,19 @@ public Integer updateLocation(@GraphQLRootContext GraphQLContext context,
     return numRows;
   }
 
+  private void checkForLoops(String locationUuid, List<Location> parentLocations) {
+    if (!Utils.isEmptyOrNull(parentLocations)) {
+      final Set<String> parentLocationUuids =
+          parentLocations.stream().map(Location::getUuid).collect(Collectors.toSet());
+      final Map<String, Set<String>> children = engine.buildLocationHash(locationUuid, true);
+      children.keySet().retainAll(parentLocationUuids);
+      if (!children.isEmpty()) {
+        throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR,
+            "Location can not be its own (grand…)parent");
+      }
+    }
+  }
+
   @GraphQLMutation(name = "mergeLocations")
   public Integer mergeLocations(@GraphQLRootContext GraphQLContext context,
       @GraphQLArgument(name = "loserUuid") String loserUuid,
@@ -183,6 +187,9 @@ public Integer mergeLocations(@GraphQLRootContext GraphQLContext context,
     // Check that given two locations can be merged
     areLocationsMergeable(winnerLocation, loserLocation);
     validateLocation(winnerLocation);
+    // Check for loops in the hierarchy
+    checkForLoops(winnerLocation.getUuid(), winnerLocation.getParentLocations());
+    checkForLoops(loserUuid, winnerLocation.getParentLocations());
 
     int numRows = dao.mergeLocations(loserLocation, winnerLocation);
     if (numRows == 0) {
@@ -195,7 +202,7 @@ public Integer mergeLocations(@GraphQLRootContext GraphQLContext context,
 
   private void validateLocation(Location winnerLocation) {
     if (winnerLocation.getName() == null || winnerLocation.getName().trim().isEmpty()) {
-      throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Location Name must not be null");
+      throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Location Name must not be empty");
     }
   }
 

diff --git a/src/main/java/mil/dds/anet/resources/OrganizationResource.java b/src/main/java/mil/dds/anet/resources/OrganizationResource.java
@@ -132,20 +132,13 @@ public Integer updateOrganization(@GraphQLRootContext GraphQLContext context,
 
     final Person user = DaoUtils.getUserFromContext(context);
     // Verify correct Organization
-    assertPermission(user, DaoUtils.getUuid(org));
+    assertPermission(user, org.getUuid());
+    // Check for loops in the hierarchy
+    checkForLoops(org.getUuid(), org.getParentOrgUuid());
 
     // Load the existing organization, so we can check for differences.
     final Organization existing = dao.getByUuid(org.getUuid());
 
-    // Check for loops in the hierarchy
-    if (org.getParentOrgUuid() != null) {
-      final Map<String, String> children = engine.buildTopLevelOrgHash(DaoUtils.getUuid(org));
-      if (children.containsKey(org.getParentOrgUuid())) {
-        throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR,
-            "Organization can not be its own (grand…)parent");
-      }
-    }
-
     if (!AuthUtils.isAdmin(user)) {
       // Check if user has administrative permission for the organizations that will be
       // modified with the parent organization update
@@ -170,6 +163,16 @@ public Integer updateOrganization(@GraphQLRootContext GraphQLContext context,
     return numRows;
   }
 
+  private void checkForLoops(String orgUuid, String parentOrgUuid) {
+    if (parentOrgUuid != null) {
+      final Map<String, String> children = engine.buildTopLevelOrgHash(orgUuid);
+      if (children.containsKey(parentOrgUuid)) {
+        throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR,
+            "Organization can not be its own (grand…)parent");
+      }
+    }
+  }
+
   private int update(Person user, Organization org, Organization existing) {
     final int numRows;
     try {
@@ -230,6 +233,9 @@ public Integer mergeOrganizations(@GraphQLRootContext GraphQLContext context,
 
     final var loserOrganization = dao.getByUuid(loserUuid);
     checkWhetherOrganizationsAreMergeable(winnerOrganization, loserOrganization);
+    // Check for loops in the hierarchy
+    checkForLoops(winnerOrganization.getUuid(), winnerOrganization.getParentOrgUuid());
+    checkForLoops(loserUuid, winnerOrganization.getParentOrgUuid());
     final var numberOfAffectedRows = dao.mergeOrganizations(loserOrganization, winnerOrganization);
     if (numberOfAffectedRows == 0) {
       throw new ResponseStatusException(HttpStatus.NOT_FOUND,