7324 unable to access collection bundles when performing search requests

.github/workflows/codeql-analysis.yml

@@ -39,19 +39,16 @@ jobs:
 
       # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
       # If this step fails, then you should remove it and run the build manually (see below)
-      - name: Autobuild
-        uses: github/codeql-action/autobuild@v2
+      # - name: Autobuild
+      #   uses: github/codeql-action/autobuild@v2
 
       # ℹ️ Command-line programs to run using the OS shell.
       # 📚 https://git.io/JvXDl
 
-      # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
-      #    and modify them (or add more) to build your code if your project
-      #    uses a compiled language
-
-      #- run: |
-      #   make bootstrap
-      #   make release
+      # ✏️ Manual build to avoid Maven wrapper issues with CodeQL autobuild
+      - name: Build with Maven
+        run: |
+          mvn clean install -DskipTests -Dmaven.javadoc.skip=true -Dcheckstyle.skip=true -Dpmd.skip=true -Dspotbugs.skip=true -Denforcer.skip=true -Dlicense.skip=true -Drat.skip=true -Dspotless.check.skip=true
 
       - name: Perform CodeQL Analysis
         uses: github/codeql-action/analyze@v2

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_6_0/7324-fix-collection-bundle-authorization.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 7324
+title: "Previously, the AuthorizationInterceptor did not properly authorize searches and reads
+   for COLLECTION type Bundles when users had appropriate permissions. This has been fixed by
+   adding BundleTypeEnum.COLLECTION to the set of standalone bundle resource types that are
+   treated similarly to DOCUMENT and MESSAGE bundles for authorization purposes."

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

@@ -1,11 +1,5 @@
 package ca.uhn.fhir.jpa.provider.r4;
 
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
-
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.delete.ThreadSafeResourceDeleterSvc;
 import ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor;
@@ -18,7 +12,6 @@
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.MethodOutcome;
-import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.client.interceptor.SimpleRequestHeaderInterceptor;
 import ca.uhn.fhir.rest.gclient.IOperation;
@@ -27,7 +20,6 @@
 import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
-import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor;
 import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRule;
 import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRuleBuilder;
@@ -88,14 +80,15 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
-import java.util.UUID;
 import java.util.stream.Stream;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.fail;
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.mockito.Mockito.when;
+import static org.junit.jupiter.api.Assertions.fail;
 
 
 public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Test {
@@ -397,6 +390,70 @@ public void testReadBundleById(AuthorizationInterceptor theAuthorizationIntercep
 		assertReadByIdAllowed(bundle.getIdElement(), theShouldAllow);
 	}
 
+	@ParameterizedTest
+	@MethodSource(value = "getReadStandaloneBundleArguments")
+	public void testReadCollectionBundleById(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) {
+		Bundle bundle = createCollectionBundle(createPatient("John", "Smith"));
+		myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor);
+
+		assertReadByIdAllowed(bundle.getIdElement(), theShouldAllow);
+	}
+
+	@ParameterizedTest
+	@MethodSource(value = "getReadStandaloneBundleInTransactionArguments")
+	public void testReadCollectionBundleInTransaction(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) {
+		Bundle collectionBundle = createCollectionBundle(createPatient("John", "Smith"));
+		createCollectionBundle(createPatient("Jane", "Doe"));
+		IIdType collectionBundleId = collectionBundle.getIdElement();
+
+		myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor);
+
+		SimpleRequestHeaderInterceptor interceptor = new SimpleRequestHeaderInterceptor("Authorization", "Bearer AAA");
+		try {
+			myClient.registerInterceptor(interceptor);
+
+			Bundle bundle;
+
+			// Read Bundle
+			bundle = new Bundle();
+			bundle.setType(Bundle.BundleType.TRANSACTION);
+			bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl(collectionBundleId.toUnqualifiedVersionless().getValue());
+			assertTransactionAllowed(bundle, theShouldAllow);
+
+			// Search
+			bundle = new Bundle();
+			bundle.setType(Bundle.BundleType.TRANSACTION);
+			bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("Bundle?type=collection");
+			assertTransactionAllowed(bundle, theShouldAllow);
+
+			// Simple Search
+			Bundle responseBundle = assertSearchAllowed("/Bundle?type=collection", theShouldAllow);
+
+			// Get next page
+			if (responseBundle != null) {
+				Bundle.BundleLinkComponent next = responseBundle.getLink("next");
+				assertNotNull(next);
+
+				bundle = new Bundle();
+				bundle.setType(Bundle.BundleType.TRANSACTION);
+				bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("/Bundle?" + Constants.PARAM_PAGINGACTION + next.getUrl());
+				assertTransactionAllowed(bundle, theShouldAllow);
+			}
+		} finally {
+			myClient.unregisterInterceptor(interceptor);
+		}
+	}
+
+	@ParameterizedTest
+	@MethodSource(value = "getReadStandaloneBundleArguments")
+	public void testGetNextPage_forCollectionBundles(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) {
+		myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor);
+		Bundle bundle = createCollectionBundle(createPatient("John", "Smith"));
+		Bundle firstBundle = new Bundle();
+		firstBundle.addLink().setRelation("next").setUrl(myClient.getServerBase() + "/"+ bundle.getIdElement().toUnqualifiedVersionless());
+		assertGetNextPageAllowed(firstBundle, theShouldAllow);
+	}
+
 	@ParameterizedTest
 	@MethodSource(value = "getReadStandaloneBundleInTransactionArguments")
 	public void testReadBundleInTransaction(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) {
@@ -520,7 +577,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
 
 	@Test
 	public void testReadCodeIn_AllowedInCompartment() throws IOException {
-		myValueSetDao.update(loadResourceFromClasspath(ValueSet.class, "r4/adi-vs2.json"));
+		myValueSetDao.update(loadResourceFromClasspath(ValueSet.class, "r4/adi-vs2.json"), mySrd);
 		myTermSvc.preExpandDeferredValueSetsToTerminologyTables();
 		logAllValueSetConcepts();
 
@@ -1490,11 +1547,11 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
 	public void testReadCompartmentTwoPatientIds() {
 		Patient patient1 = new Patient();
 		patient1.setActive(true);
-		IIdType p1id = myPatientDao.create(patient1).getId().toUnqualifiedVersionless();
+		IIdType p1id = myPatientDao.create(patient1, mySrd).getId().toUnqualifiedVersionless();
 
 		Patient patient2 = new Patient();
 		patient2.setActive(true);
-		IIdType p2id = myPatientDao.create(patient2).getId().toUnqualifiedVersionless();
+		IIdType p2id = myPatientDao.create(patient2, mySrd).getId().toUnqualifiedVersionless();
 
 		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
 			@Override
@@ -1537,27 +1594,27 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
 	public void testReadCompartmentTwoPatientIdsTwoEOBs() {
 		Patient patient1 = new Patient();
 		patient1.setActive(true);
-		IIdType p1id = myPatientDao.create(patient1).getId().toUnqualifiedVersionless();
+		IIdType p1id = myPatientDao.create(patient1, mySrd).getId().toUnqualifiedVersionless();
 
 		ExplanationOfBenefit eob1 = new ExplanationOfBenefit();
 		eob1.setPatient(new Reference(p1id));
-		myExplanationOfBenefitDao.create(eob1);
+		myExplanationOfBenefitDao.create(eob1, mySrd);
 
 		Patient patient2 = new Patient();
 		patient2.setActive(true);
-		IIdType p2id = myPatientDao.create(patient2).getId().toUnqualifiedVersionless();
+		IIdType p2id = myPatientDao.create(patient2, mySrd).getId().toUnqualifiedVersionless();
 
 		ExplanationOfBenefit eob2 = new ExplanationOfBenefit();
 		eob2.setPatient(new Reference(p2id));
-		myExplanationOfBenefitDao.create(eob2);
+		myExplanationOfBenefitDao.create(eob2, mySrd);
 
 		Patient patient3 = new Patient();
 		patient3.setActive(true);
-		IIdType p3id = myPatientDao.create(patient3).getId().toUnqualifiedVersionless();
+		IIdType p3id = myPatientDao.create(patient3, mySrd).getId().toUnqualifiedVersionless();
 
 		ExplanationOfBenefit eob3 = new ExplanationOfBenefit();
 		eob3.setPatient(new Reference(p3id));
-		myExplanationOfBenefitDao.create(eob3);
+		myExplanationOfBenefitDao.create(eob3, mySrd);
 
 		myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
 			@Override
@@ -1841,6 +1898,55 @@ public void testSearchBundles_forMessageBundles(AuthorizationInterceptor theAuth
 		assertSearchAllowed("/Bundle", theShouldAllow);
 	}
 
+	@ParameterizedTest
+	@MethodSource(value = "getReadStandaloneBundleArguments")
+	public void testSearchBundles_forCollectionBundles(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) {
+		myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor);
+		createCollectionBundle(createPatient("John", "Smith"));
+		assertSearchAllowed("/Bundle", theShouldAllow);
+	}
+
+	@Test
+	public void testSearchBundles_forCollectionBundles_withTagBasedAuthorization() {
+		// Create COLLECTION bundle with TEST-TAG-1
+		Patient patient1 = createPatient("John", "Smith");
+		Bundle collectionBundle1 = createCollectionBundle(patient1);
+		collectionBundle1.getMeta().addTag("http//myserver.ca/provider-1", "TEST-TAG-1", null);
+		myBundleDao.update(collectionBundle1, mySrd);
+
+		// Create COLLECTION bundle with TEST-TAG-2
+		Patient patient2 = createPatient("Jane", "Doe");
+		Bundle collectionBundle2 = createCollectionBundle(patient2);
+		collectionBundle2.getMeta().addTag("http//myserver.ca/provider-1", "TEST-TAG-2", null);
+		myBundleDao.update(collectionBundle2, mySrd);
+
+		// Set up authorization interceptor that only allows reading bundles with TEST-TAG-1
+		AuthorizationInterceptor interceptor = new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder()
+					.allow().read().resourcesOfType("Bundle").withAnyId()
+					.withFilterTester("_tag=http//myserver.ca/provider-1|TEST-TAG-1")
+					.andThen()
+					.build();
+			}
+		};
+		interceptor.setAuthorizationSearchParamMatcher(new AuthorizationSearchParamMatcher(mySearchParamMatcher));
+		myServer.getRestfulServer().registerInterceptor(interceptor);
+
+		// Search for bundles with TEST-TAG-1 should return TEST-TAG-1 COLLECTION
+		Bundle searchResults = assertSearchAllowed("/Bundle?_tag=http//myserver.ca/provider-1|TEST-TAG-1", true);
+
+		assertThat(searchResults).isNotNull();
+		assertThat(searchResults.getEntry()).as("Should find COLLECTION tagged TEST-TAG-1").hasSize(1);
+
+		// Search for bundles with TEST-TAG-2 should fail (no authorization)
+		assertSearchFailsWith403Forbidden("/Bundle?_tag=http//myserver.ca/provider-1|TEST-TAG-2");
+
+		// Search for bundles with no tag should fail (no authorization)
+		assertSearchFailsWith403Forbidden("/Bundle");
+	}
+
 	@Test
 	public void testSearchBundles_withPermissionToViewOneBundle_onlyAllowsViewingOneBundle() {
 		Bundle bundle1 = createMessageHeaderBundle(createPatient("John", "Smith"));
@@ -2028,6 +2134,14 @@ private Bundle createMessageHeaderBundle(Patient thePatient) {
 		return (Bundle) myBundleDao.create(bundle, mySrd).getResource();
 	}
 
+	private Bundle createCollectionBundle(Patient thePatient) {
+		Bundle bundle = new Bundle();
+		bundle.setType(Bundle.BundleType.COLLECTION);
+		bundle.addEntry().setResource(thePatient);
+
+		return (Bundle) myBundleDao.create(bundle, mySrd).getResource();
+	}
+
 	private void assertReadByIdAllowed(IIdType theId, boolean theShouldAllow) {
 		if (theShouldAllow) {
 			IBaseResource resource = myClient.read()

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

@@ -53,13 +53,13 @@
 import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static java.util.Objects.isNull;
 import static java.util.Objects.nonNull;
 import static org.apache.commons.lang3.StringUtils.EMPTY;
-import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 /**
@@ -74,6 +74,7 @@
  *
  * @see SearchNarrowingInterceptor
  */
+@SuppressWarnings("unused")
 @Interceptor(order = AuthorizationConstants.ORDER_AUTH_INTERCEPTOR)
 public class AuthorizationInterceptor implements IRuleApplier {
 
@@ -82,7 +83,7 @@ public class AuthorizationInterceptor implements IRuleApplier {
 	private static final AtomicInteger ourInstanceCount = new AtomicInteger(0);
 	private static final Logger ourLog = LoggerFactory.getLogger(AuthorizationInterceptor.class);
 	private static final Set<BundleTypeEnum> STANDALONE_BUNDLE_RESOURCE_TYPES =
-			Set.of(BundleTypeEnum.DOCUMENT, BundleTypeEnum.MESSAGE);
+			Set.of(BundleTypeEnum.DOCUMENT, BundleTypeEnum.MESSAGE, BundleTypeEnum.COLLECTION);
 
 	private final int myInstanceIndex = ourInstanceCount.incrementAndGet();
 	private final String myRequestSeenResourcesKey =
@@ -382,7 +383,7 @@ protected void handleDeny(RequestDetails theRequestDetails, Verdict decision) {
 	 */
 	protected void handleDeny(Verdict decision) {
 		if (decision.getDecidingRule() != null) {
-			String ruleName = defaultString(decision.getDecidingRule().getName(), "(unnamed rule)");
+			String ruleName = Objects.toString(decision.getDecidingRule().getName(), "(unnamed rule)");
 			throw new ForbiddenOperationException(Msg.code(333) + "Access denied by rule: " + ruleName);
 		}
 		throw new ForbiddenOperationException(Msg.code(334) + "Access denied by default policy (no applicable rules)");
@@ -441,7 +442,7 @@ public void hookOutgoingResponse(
 	@Hook(Pointcut.STORAGE_CASCADE_DELETE)
 	public void hookCascadeDeleteForConflict(
 			RequestDetails theRequestDetails, Pointcut thePointcut, IBaseResource theResourceToDelete) {
-		Validate.notNull(theResourceToDelete); // just in case
+		Objects.requireNonNull(theResourceToDelete); // just in case
 		checkPointcutAndFailIfDeny(theRequestDetails, thePointcut, theResourceToDelete);
 	}
 
@@ -472,9 +473,7 @@ private RestOperationTypeEnum determineRestOperationTypeFromBulkExportOptions(
 			BulkExportJobParameters theBulkExportOptions) {
 		RestOperationTypeEnum restOperationType = RestOperationTypeEnum.EXTENDED_OPERATION_SERVER;
 		BulkExportJobParameters.ExportStyle exportStyle = theBulkExportOptions.getExportStyle();
-		if (exportStyle.equals(BulkExportJobParameters.ExportStyle.SYSTEM)) {
-			restOperationType = RestOperationTypeEnum.EXTENDED_OPERATION_SERVER;
-		} else if (exportStyle.equals(BulkExportJobParameters.ExportStyle.PATIENT)) {
+		if (exportStyle.equals(BulkExportJobParameters.ExportStyle.PATIENT)) {
 			if (theBulkExportOptions.getPatientIds().size() == 1) {
 				restOperationType = RestOperationTypeEnum.EXTENDED_OPERATION_INSTANCE;
 			} else {
@@ -637,21 +636,24 @@ public static List<IBaseResource> toListOfResourcesAndExcludeContainer(
 	}
 
 	/**
-	 * This method determines if the given Resource should have permissions applied to the resources inside or
-	 * to the Resource itself.
+	 * This method determines if the given Resource should have permissions applied
+	 * to the resources inside or to the Resource itself.
 	 * For Parameters resources, we include child resources when checking the permissions.
-	 * For Bundle resources, we only look at resources inside if the Bundle is of type document, collection, or message.
+	 * For Bundle resources, we look at resources inside if the Bundle type is not in
+	 * STANDALONE_BUNDLE_RESOURCE_TYPES set.
 	 */
 	protected static boolean shouldExamineChildResources(IBaseResource theResource, FhirContext theFhirContext) {
 		if (theResource instanceof IBaseParameters) {
 			return true;
 		}
-		if (theResource instanceof IBaseBundle) {
-			BundleTypeEnum bundleType = BundleUtil.getBundleTypeEnum(theFhirContext, ((IBaseBundle) theResource));
+
+		if (theResource instanceof IBaseBundle baseBundle) {
+			BundleTypeEnum bundleType = BundleUtil.getBundleTypeEnum(theFhirContext, baseBundle);
 			boolean isStandaloneBundleResource =
 					bundleType != null && STANDALONE_BUNDLE_RESOURCE_TYPES.contains(bundleType);
 			return !isStandaloneBundleResource;
 		}
+
 		return false;
 	}
 
@@ -661,7 +663,7 @@ public static class Verdict {
 		private final PolicyEnum myDecision;
 
 		public Verdict(PolicyEnum theDecision, IAuthRule theDecidingRule) {
-			Validate.notNull(theDecision);
+			Objects.requireNonNull(theDecision);
 
 			myDecision = theDecision;
 			myDecidingRule = theDecidingRule;

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/SearchNarrowingWithConsentAndAuthInterceptorTest.java

@@ -3,6 +3,7 @@
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.support.IValidationSupport;
 import ca.uhn.fhir.model.api.Include;
+import ca.uhn.fhir.model.valueset.BundleTypeEnum;
 import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.annotation.IncludeParam;
 import ca.uhn.fhir.rest.annotation.Operation;
@@ -487,7 +488,7 @@ public Class<Patient> getResourceType() {
 		}
 
 
-		@Operation(name = "$everything")
+		@Operation(name = "$everything", bundleType = BundleTypeEnum.SEARCHSET)
 		public IBundleProvider everything(RequestDetails theRequestDetails, @IdParam IdType theId) {
 			assertNotNull(myNextPatientResponse);
 			myNextPatientResponse = ServerInterceptorUtil.fireStoragePreshowResource(myNextPatientResponse, theRequestDetails, myServer.getRestfulServer().getInterceptorService());