fix: Skip validations if user is superuser [TECH-1589][2.40] (#15649)

* fix: Skip validations if user is superuser [TECH-1589][2.40] * fix: Remove TODO comment [TECH-1589][2.40]
dhis2 · Nov 11, 2023 · cd330a5 · cd330a5
1 parent 3868201
commit cd330a5
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 7 deletions.
diff --git a/...-integration/src/test/java/org/hisp/dhis/dxf2/events/security/EnrollmentSecurityTest.java b/...-integration/src/test/java/org/hisp/dhis/dxf2/events/security/EnrollmentSecurityTest.java
@@ -27,6 +27,7 @@
  */
 package org.hisp.dhis.dxf2.events.security;
 
+import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ALL;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -103,10 +104,12 @@ class EnrollmentSecurityTest extends TransactionalIntegrationTest {
 
   private ProgramStage programStageB;
 
+  private User admin;
+
   @Override
   protected void setUpTest() {
     userService = _userService;
-    User admin = createAndInjectAdminUser();
+    admin = createAndInjectAdminUser();
     organisationUnitA = createOrganisationUnit('A');
     organisationUnitB = createOrganisationUnit('B');
     manager.save(organisationUnitA);
@@ -435,6 +438,23 @@ void testAddEnrollmentWithOrgUnitIdSchemeToOrgUnitWithoutProgramAccess() {
     assertEquals(ImportStatus.SUCCESS, importSummary.getStatus());
   }
 
+  @Test
+  void shouldReturnAllEnrollmentsWhenOrgUnitModeAllAndUserAuthorized() {
+    ImportSummary importSummary =
+        enrollmentService.addEnrollment(
+            createEnrollment(programA.getUid(), maleA.getUid()),
+            ImportOptions.getDefaultImportOptions());
+    assertEquals(ImportStatus.SUCCESS, importSummary.getStatus());
+
+    injectSecurityContext(admin);
+    ProgramInstanceQueryParams params = new ProgramInstanceQueryParams();
+    params.setOrganisationUnitMode(ALL);
+    params.setUser(admin);
+
+    Enrollments enrollments = enrollmentService.getEnrollments(params);
+    assertNotNull(enrollments);
+  }
+
   private Enrollment createEnrollment(String program, String person) {
     Enrollment enrollment = new Enrollment();
     enrollment.setEnrollment(CodeGenerator.generateUid());

diff --git a/...tegration/src/test/java/org/hisp/dhis/trackedentity/TrackedEntityInstanceServiceTest.java b/...tegration/src/test/java/org/hisp/dhis/trackedentity/TrackedEntityInstanceServiceTest.java
@@ -27,7 +27,11 @@
  */
 package org.hisp.dhis.trackedentity;
 
+import static java.util.Collections.emptySet;
+import static org.hisp.dhis.common.AccessLevel.CLOSED;
+import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ALL;
 import static org.hisp.dhis.trackedentity.TrackedEntityInstanceQueryParams.OrderColumn.ENROLLED_AT;
+import static org.hisp.dhis.utils.Assertions.assertContains;
 import static org.hisp.dhis.utils.Assertions.assertContainsOnly;
 import static org.hisp.dhis.utils.Assertions.assertIsEmpty;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -41,7 +45,9 @@
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
+import org.hisp.dhis.common.AssignedUserSelectionMode;
 import org.hisp.dhis.common.Grid;
+import org.hisp.dhis.common.IllegalQueryException;
 import org.hisp.dhis.common.QueryFilter;
 import org.hisp.dhis.common.QueryItem;
 import org.hisp.dhis.common.QueryOperator;
@@ -55,6 +61,7 @@
 import org.hisp.dhis.program.ProgramStageInstance;
 import org.hisp.dhis.program.ProgramStageInstanceService;
 import org.hisp.dhis.program.ProgramStageService;
+import org.hisp.dhis.program.ProgramType;
 import org.hisp.dhis.security.acl.AccessStringHelper;
 import org.hisp.dhis.test.integration.IntegrationTestBase;
 import org.hisp.dhis.trackedentityattributevalue.TrackedEntityAttributeValue;
@@ -117,6 +124,10 @@ class TrackedEntityInstanceServiceTest extends IntegrationTestBase {
 
   private User superUser;
 
+  private User userWithSearchInAllAuthority;
+
+  private Program disabledAccessProgram;
+
   @Override
   public void setUpTest() {
     super.userService = _userService;
@@ -180,6 +191,20 @@ public void setUpTest() {
     attributeService.addTrackedEntityAttribute(filtF);
     attributeService.addTrackedEntityAttribute(filtG);
 
+    userWithSearchInAllAuthority =
+        createAndAddUser(
+            false,
+            "userSearchInAll",
+            Set.of(organisationUnit),
+            Set.of(organisationUnit),
+            "F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS");
+
+    disabledAccessProgram = createProgram('C', new HashSet<>(), null);
+    disabledAccessProgram.setProgramType(ProgramType.WITH_REGISTRATION);
+    disabledAccessProgram.setAccessLevel(CLOSED);
+    disabledAccessProgram.getSharing().setPublicAccess(AccessStringHelper.disableDataSharing(null));
+    programService.addProgram(disabledAccessProgram);
+
     User user = createUserWithAuth("testUser");
     user.setTeiSearchOrganisationUnits(Set.of(organisationUnit));
     injectSecurityContext(user);
@@ -897,6 +922,48 @@ void shouldReturnEmptyIfTEWasUpdatedBeforePassedDateAndTime() {
     assertIsEmpty(trackedEntities);
   }
 
+  @Test
+  void shouldFailWhenModeAllUserCanSearchEverywhereButNotSuperuserAndNoAccessToProgram() {
+    injectSecurityContext(userWithSearchInAllAuthority);
+
+    TrackedEntityInstanceQueryParams params = new TrackedEntityInstanceQueryParams();
+    params.setOrganisationUnitMode(ALL);
+    params.setProgram(disabledAccessProgram);
+    params.setUserWithAssignedUsers(
+        AssignedUserSelectionMode.ALL, userWithSearchInAllAuthority, emptySet());
+
+    IllegalQueryException ex =
+        assertThrows(
+            IllegalQueryException.class,
+            () -> entityInstanceService.getTrackedEntityInstanceIds(params, false, false));
+
+    assertContains(
+        String.format(
+            "Current user is not authorized to read data from selected program:  %s",
+            disabledAccessProgram.getUid()),
+        ex.getMessage());
+  }
+
+  @Test
+  void shouldReturnAllEntitiesWhenSuperuserAndModeAll() {
+    injectSecurityContext(superUser);
+    addEntityInstances();
+
+    TrackedEntityInstanceQueryParams params = new TrackedEntityInstanceQueryParams();
+    params.setOrganisationUnitMode(ALL);
+    params.setOrganisationUnits(Set.of(organisationUnit));
+
+    List<Long> trackedEntities =
+        entityInstanceService.getTrackedEntityInstanceIds(params, true, true);
+    assertContainsOnly(
+        Set.of(
+            entityInstanceA1.getId(),
+            entityInstanceB1.getId(),
+            entityInstanceC1.getId(),
+            entityInstanceD1.getId()),
+        trackedEntities);
+  }
+
   private void initializeEntityInstance(TrackedEntityInstance entityInstance) {
     entityInstance.setTrackedEntityType(trackedEntityType);
     entityInstanceService.addTrackedEntityInstance(entityInstance);

diff --git a/...web-api/src/main/java/org/hisp/dhis/webapi/controller/event/EnrollmentCriteriaMapper.java b/...web-api/src/main/java/org/hisp/dhis/webapi/controller/event/EnrollmentCriteriaMapper.java
@@ -27,6 +27,7 @@
  */
 package org.hisp.dhis.webapi.controller.event;
 
+import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ALL;
 import static org.hisp.dhis.webapi.controller.event.mapper.OrderParamsHelper.toOrderParams;
 
 import java.util.Date;
@@ -43,6 +44,7 @@
 import org.hisp.dhis.program.ProgramInstanceQueryParams;
 import org.hisp.dhis.program.ProgramService;
 import org.hisp.dhis.program.ProgramStatus;
+import org.hisp.dhis.security.Authorities;
 import org.hisp.dhis.trackedentity.TrackedEntityInstance;
 import org.hisp.dhis.trackedentity.TrackedEntityInstanceService;
 import org.hisp.dhis.trackedentity.TrackedEntityType;
@@ -141,6 +143,13 @@ public ProgramInstanceQueryParams getFromUrl(
       throw new IllegalQueryException("Program does not exist: " + program);
     }
 
+    if (params.isOrganisationUnitMode(ALL)
+        && !currentUserService.currentUserIsAuthorized(
+            Authorities.F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS.name())) {
+      throw new IllegalQueryException(
+          "Current user is not authorized to query across all organisation units");
+    }
+
     TrackedEntityType te =
         trackedEntityType != null
             ? trackedEntityTypeService.getTrackedEntityType(trackedEntityType)

diff --git a/.../java/org/hisp/dhis/webapi/controller/tracker/export/TrackerEventCriteriaMapperUtils.java b/.../java/org/hisp/dhis/webapi/controller/tracker/export/TrackerEventCriteriaMapperUtils.java
@@ -80,17 +80,12 @@ public static void validateOrgUnitMode(
   }
 
   private static void validateUserCanSearchOrgUnitModeALL(User user) {
-    // TODO(tracker) This user check is unnecessary for events, but needs to be here for
-    // trackedEntities. In that case, it should be done in a separate validation, so when it gets
-    // here we already know it's not null
     if (user == null
         || !(user.isSuper()
             || user.isAuthorized(
                 Authorities.F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS.name()))) {
       throw new IllegalQueryException(
           "Current user is not authorized to query across all organisation units");
-
-      // TODO(tracker) Validate user scope if mode ALL needs to use user's search or capture scope
     }
   }
 

diff --git a/...va/org/hisp/dhis/webapi/controller/tracker/export/TrackerTrackedEntityCriteriaMapper.java b/...va/org/hisp/dhis/webapi/controller/tracker/export/TrackerTrackedEntityCriteriaMapper.java
@@ -29,6 +29,7 @@
 
 import static org.apache.commons.lang3.BooleanUtils.toBooleanDefaultIfNull;
 import static org.apache.commons.lang3.StringUtils.isNotEmpty;
+import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ALL;
 import static org.hisp.dhis.trackedentity.TrackedEntityInstanceQueryParams.OrderColumn.findColumn;
 import static org.hisp.dhis.webapi.controller.event.mapper.OrderParamsHelper.toOrderParams;
 import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamUtils.applyIfNonEmpty;
@@ -49,6 +50,7 @@
 import lombok.RequiredArgsConstructor;
 import org.apache.commons.lang3.StringUtils;
 import org.hisp.dhis.common.DimensionalItemObject;
+import org.hisp.dhis.common.IllegalQueryException;
 import org.hisp.dhis.common.OrganisationUnitSelectionMode;
 import org.hisp.dhis.common.QueryFilter;
 import org.hisp.dhis.common.QueryItem;
@@ -59,6 +61,7 @@
 import org.hisp.dhis.program.Program;
 import org.hisp.dhis.program.ProgramService;
 import org.hisp.dhis.program.ProgramStage;
+import org.hisp.dhis.security.Authorities;
 import org.hisp.dhis.trackedentity.TrackedEntityAttribute;
 import org.hisp.dhis.trackedentity.TrackedEntityAttributeService;
 import org.hisp.dhis.trackedentity.TrackedEntityInstanceQueryParams;
@@ -112,6 +115,13 @@ public TrackedEntityInstanceQueryParams map(TrackerTrackedEntityCriteria criteri
       orgUnits.addAll(user.getOrganisationUnits());
     }
 
+    if (criteria.getOuMode() == ALL
+        && !currentUserService.currentUserIsAuthorized(
+            Authorities.F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS.name())) {
+      throw new IllegalQueryException(
+          "Current user is not authorized to query across all organisation units");
+    }
+
     QueryFilter queryFilter = parseQueryFilter(criteria.getQuery());
 
     Map<String, TrackedEntityAttribute> attributes =

diff --git a/...rg/hisp/dhis/webapi/controller/tracker/export/TrackerTrackedEntityCriteriaMapperTest.java b/...rg/hisp/dhis/webapi/controller/tracker/export/TrackerTrackedEntityCriteriaMapperTest.java
@@ -33,6 +33,7 @@
 import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.containsString;
 import static org.hisp.dhis.DhisConvenienceTest.getDate;
+import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ALL;
 import static org.hisp.dhis.util.DateUtils.parseDate;
 import static org.hisp.dhis.utils.Assertions.assertContainsOnly;
 import static org.hisp.dhis.utils.Assertions.assertIsEmpty;
@@ -42,6 +43,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
@@ -52,6 +54,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 import org.hisp.dhis.common.AssignedUserSelectionMode;
+import org.hisp.dhis.common.IllegalQueryException;
 import org.hisp.dhis.common.OrganisationUnitSelectionMode;
 import org.hisp.dhis.common.QueryFilter;
 import org.hisp.dhis.common.QueryItem;
@@ -73,6 +76,7 @@
 import org.hisp.dhis.trackedentity.TrackerAccessManager;
 import org.hisp.dhis.user.CurrentUserService;
 import org.hisp.dhis.user.User;
+import org.hisp.dhis.user.UserRole;
 import org.hisp.dhis.webapi.controller.event.mapper.OrderParam;
 import org.hisp.dhis.webapi.controller.event.mapper.SortDirection;
 import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
@@ -653,10 +657,36 @@ void shouldCreateQueryFilterWhenCriteriaMultipleOperatorHasFinalColon()
         actualFilters);
   }
 
+  @Test
+  void shouldFailWhenModeAllUserCanSearchEverywhereButNotSuperuserAndNoAccessToProgram() {
+    when(currentUserService.getCurrentUser()).thenReturn(user);
+    criteria.setOuMode(ALL);
+
+    IllegalQueryException exception =
+        Assertions.assertThrows(IllegalQueryException.class, () -> mapper.map(criteria));
+    assertEquals(
+        "Current user is not authorized to query across all organisation units",
+        exception.getMessage());
+  }
+
+  @Test
+  void shouldMapWhenModeAllAndUserIsSuperuser() throws ForbiddenException, BadRequestException {
+    User admin = new User();
+    UserRole userRole = new UserRole();
+    userRole.setAuthorities(Set.of(ALL.name()));
+    admin.setUserRoles(Set.of(userRole));
+
+    when(currentUserService.currentUserIsAuthorized(anyString())).thenReturn(true);
+    criteria.setOuMode(ALL);
+
+    TrackedEntityInstanceQueryParams params = mapper.map(criteria);
+    assertEquals(ALL, params.getOrganisationUnitMode());
+  }
+
   @ParameterizedTest
   @EnumSource(
       value = OrganisationUnitSelectionMode.class,
-      names = {"CAPTURE", "ACCESSIBLE", "ALL"})
+      names = {"CAPTURE", "ACCESSIBLE"})
   void shouldPassWhenOuModeDoesNotNeedOrgUnitAndOrgUnitProvided(
       OrganisationUnitSelectionMode mode) {
     when(trackerAccessManager.canAccess(user, null, orgUnit1)).thenReturn(true);