Delete-expunge does not honor delete resourcesOfType permissions (#5857)

* Delete-expunge does not honor delete resourcesOfType permissions - fix

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5856-delete-expunge-does-not-honor-delete-resources-of-type-permission.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 5856
+title: "Previously, it was possible to execute the `$delete-expunge` operation on a resource even if the user did not 
+have delete permissions for the given resource type. This has been fixed."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java

@@ -1425,12 +1425,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 
 
 	@Test
-	public void testDeleteExpungeBlocked() {
+	public void testDeleteExpunge_allResourcesPermission_forbidden() {
-		// Create Patient, and Observation that refers to it
+		createPatient();
-		Patient patient = new Patient();
-		patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100");
-		patient.addName().setFamily("Tester").addGiven("Siobhan");
-		myClient.create().resource(patient).execute().getId().toUnqualifiedVersionless();
 
 		// Allow any deletes, but don't allow expunge
 		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@@ -1442,25 +1438,12 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 			}
 		});
 
-		try {
+		validateDeleteConditionalByUrlIsForbidden("Patient?name=Siobhan&_expunge=true");
-			myClient
-				.delete()
-				.resourceConditionalByUrl("Patient?name=Siobhan&_expunge=true")
-				.execute();
-			fail();
-		} catch (ForbiddenOperationException e) {
-			// good
-		}
 	}
 
 	@Test
-	public void testDeleteExpungeAllowed() {
+	public void testDeleteExpunge_allResourcesPermission_allowed() {
-
+		createPatient();
-		// Create Patient, and Observation that refers to it
-		Patient patient = new Patient();
-		patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100");
-		patient.addName().setFamily("Tester").addGiven("Raghad");
-		myClient.create().resource(patient).execute().getId().toUnqualifiedVersionless();
 
 		// Allow deletes and allow expunge
 		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@@ -1473,12 +1456,139 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 			}
 		});
 
-		myClient
+		executeDeleteConditionalByUrl("Patient?name=Siobhan&_expunge=true");
-			.delete()
+	}
-			.resourceConditionalByUrl("Patient?name=Siobhan&_expunge=true")
+
+	private void createDeleteByTypeRule(String theType) {
+		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder().allow()
+					.delete()
+					.onExpunge()
+					.resourcesOfType(theType)
+					.withAnyId()
+					.andThen().build();
+			}
+		});
+	}
+
+	@Test
+	public void testDeleteExpunge_typePermission_allowed() {
+		IIdType id = createPatient();
+		createDeleteByTypeRule("Patient");
+		executeDeleteConditionalByUrl("Patient?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_typePermission_forbidden() {
+		IIdType id = createPatient();
+		createDeleteByTypeRule("Observation");
+		validateDeleteConditionalByUrlIsForbidden("Patient?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_noIdTypePermission_forbidden() {
+		createDeleteByTypeRule("Observation");
+		validateDeleteConditionalByUrlIsForbidden("Patient?_expunge=true");
+	}
+
+	private void createPatientCompartmentRule(IIdType theId) {
+		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder().allow()
+					.delete()
+					.onExpunge()
+					.allResources()
+					.inCompartment("Patient", theId)
+					.andThen().build();
+			}
+		});
+	}
+
+	@Test
+	public void testDeleteExpunge_compartmentPermission_allowed() {
+		IIdType id = createPatient();
+		createPatientCompartmentRule(id);
+		executeDeleteConditionalByUrl("Patient?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_compartmentPermission_forbidden() {
+		IIdType id = createPatient();
+		IdDt compartmentId = new IdDt();
+		compartmentId.setParts(null, "Patient", "123", null);
+		createPatientCompartmentRule(compartmentId);
+		validateDeleteConditionalByUrlIsForbidden("Patient?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_urlWithSearchParameterCompartmentPermission_forbidden() {
+		IIdType id = createPatient();
+		IdDt compartmentId = new IdDt();
+		compartmentId.setParts(null, "Patient", "123", null);
+		createPatientCompartmentRule(compartmentId);
+		validateDeleteConditionalByUrlIsForbidden("Observation?_expunge=true&patient=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_multipleIdsCompartmentPermission_forbidden() {
+		IIdType id = createPatient();
+		createPatientCompartmentRule(id);
+		validateDeleteConditionalByUrlIsForbidden("Patient?_expunge=true&_id=" + id.getIdPart() + "_id=123");
+	}
+
+	private void createTypeInPatientCompartmentRule(IIdType theId) {
+		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder().allow()
+					.delete()
+					.onExpunge()
+					.resourcesOfType("Patient")
+					.inCompartment("Patient", theId)
+					.andThen().build();
+			}
+		});
+	}
+
+	@Test
+	public void testDeleteExpunge_typeInCompartmentPermission_allowed() {
+		IIdType id = createPatient();
+		createTypeInPatientCompartmentRule(id);
+		executeDeleteConditionalByUrl("Patient?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	@Test
+	public void testDeleteExpunge_typeInCompartmentPermission_forbidden() {
+		IIdType id = createPatient();
+		createTypeInPatientCompartmentRule(id);
+		validateDeleteConditionalByUrlIsForbidden("Observation?_expunge=true&_id=" + id.getIdPart());
+	}
+
+	private void validateDeleteConditionalByUrlIsForbidden(String theUrl) {
+		try {
+			executeDeleteConditionalByUrl(theUrl);
+			fail();
+		} catch (ForbiddenOperationException e) {
+			// good
+		}
+	}
+
+	private void executeDeleteConditionalByUrl(String theUrl) {
+		myClient.delete()
+			.resourceConditionalByUrl(theUrl)
 			.execute();
 	}
 
+	private IIdType createPatient() {
+		Patient patient = new Patient();
+		patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100");
+		patient.addName().setFamily("Tester").addGiven("Raghad");
+		return myClient.create().resource(patient).execute().getId().toUnqualifiedVersionless();
+	}
+
 	@Test
 	public void testSmartFilterSearchAllowed() {
 		createObservation(withId("allowed"), withObservationCode(TermTestUtil.URL_MY_CODE_SYSTEM, "A"));

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java

@@ -48,6 +48,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -56,6 +57,8 @@ import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static ca.uhn.fhir.rest.server.interceptor.auth.AppliesTypeEnum.ALL_RESOURCES;
+import static ca.uhn.fhir.rest.server.interceptor.auth.AppliesTypeEnum.TYPES;
 import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
@@ -267,13 +270,16 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 			case DELETE:
 				if (theOperation == RestOperationTypeEnum.DELETE) {
 					if (thePointcut == Pointcut.STORAGE_PRE_DELETE_EXPUNGE && myAppliesToDeleteExpunge) {
-						return newVerdict(
+						target.resourceType = theRequestDetails.getResourceName();
-								theOperation,
+						String[] resourceIds = theRequestDetails.getParameters().get("_id");
-								theRequestDetails,
+						if (resourceIds != null) {
-								theInputResource,
+							target.resourceIds =
-								theInputResourceId,
+									extractResourceIdsFromRequestParameters(theRequestDetails, resourceIds);
-								theOutputResource,
+						} else {
-								theRuleApplier);
+							target.setSearchParams(theRequestDetails);
+						}
+
+						break;
 					}
 					if (myAppliesToDeleteCascade != (thePointcut == Pointcut.STORAGE_CASCADE_DELETE)) {
 						return null;
@@ -431,6 +437,18 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 				theRuleApplier);
 	}
 
+	private List<IIdType> extractResourceIdsFromRequestParameters(
+			RequestDetails theRequestDetails, String[] theResourceIds) {
+		return Arrays.stream(theResourceIds)
+				.map(id -> {
+					IIdType inputResourceId =
+							theRequestDetails.getFhirContext().getVersion().newIdType();
+					inputResourceId.setParts(null, theRequestDetails.getResourceName(), id, null);
+					return inputResourceId;
+				})
+				.collect(Collectors.toList());
+	}
+
 	/**
 	 * Apply any special processing logic specific to this rule.
 	 * This is intended to be overridden.