Improve auth interceptor operation handling (#6278)

* Fix #6258 - Improve auth interceptor operation handling

* Cleanup
This commit is contained in:
James Agnew 2024-10-18 07:21:24 -04:00 committed by GitHub
parent 3e715665a1
commit 2b3858572d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 256 additions and 140 deletions

View File

@ -0,0 +1,10 @@
---
type: fix
issue: 6258
title: "The AuthorizationInterceptor handling for operations has been improved
so that operation rules now directly test the contents of response Bundle
or Parameters objects returned by the operation when configure to require
explicit response authorization. This fixes a regression in 7.4.0 where
operation responses could sometimes be denied even if appropriate
permissions were granted to view resources in a response bundle. Thanks to
Gijsbert van den Brink for reporting the issue with a sample test!"

View File

@ -20,11 +20,15 @@ import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.MethodOutcome;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.client.interceptor.SimpleRequestHeaderInterceptor;
import ca.uhn.fhir.rest.gclient.IOperation;
import ca.uhn.fhir.rest.gclient.IOperationUnnamed;
import ca.uhn.fhir.rest.gclient.IOperationUntypedWithInput;
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.interceptor.auth.AuthorizationInterceptor;
import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRule;
import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRuleBuilder;
import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRuleTester;
import ca.uhn.fhir.rest.server.interceptor.auth.PolicyEnum;
import ca.uhn.fhir.rest.server.interceptor.auth.RuleBuilder;
@ -68,6 +72,7 @@ import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;
@ -897,8 +902,17 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
}
@Test
public void testDiffOperation_AllowedByType_Instance() {
@ParameterizedTest
@CsvSource({
// ResourceId , RequireExplicitResponseAuthorization , ShouldSucceed
"Patient/A , true , true",
"Patient/A/_history/2 , true , true",
"Observation/B , true , false",
"Patient/A , false , true",
"Patient/A/_history/2 , false , true",
"Observation/B , false , true"
})
public void testDiffOperation_AllowedByType_Instance(String theResourceId, boolean theRequireExplicitResponseAuthorization, boolean theShouldSucceed) {
createPatient(withId("A"), withActiveTrue());
createPatient(withId("A"), withActiveFalse());
createObservation(withId("B"), withStatus("final"));
@ -906,30 +920,42 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andAllowAllResponses().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andAllowAllResponses().andThen()
.allow().read().resourcesOfType(Patient.class).withAnyId().andThen()
.denyAll()
.build();
RuleBuilder ruleBuilder = new RuleBuilder();
if (theRequireExplicitResponseAuthorization) {
ruleBuilder.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andRequireExplicitResponseAuthorization().andThen();
ruleBuilder.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andRequireExplicitResponseAuthorization().andThen();
} else {
ruleBuilder.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andAllowAllResponses().andThen();
ruleBuilder.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andAllowAllResponses().andThen();
}
ruleBuilder.allow().read().resourcesOfType(Patient.class).withAnyId().andThen();
ruleBuilder.denyAll();
return ruleBuilder.build();
}
});
Parameters diff;
diff = myClient.operation().onInstance("Patient/A").named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
assertThat(diff.getParameter()).hasSize(1);
diff = myClient.operation().onInstanceVersion(new IdType("Patient/A/_history/2")).named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
assertThat(diff.getParameter()).hasSize(1);
try {
myClient.operation().onInstance("Observation/B").named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
fail();
} catch (ForbiddenOperationException e) {
// good
IOperation operation = myClient.operation();
IOperationUnnamed target;
if (theResourceId.contains("_history")) {
target = operation.onInstanceVersion(new IdType(theResourceId));
} else {
target = operation.onInstance(theResourceId);
}
IOperationUntypedWithInput<Parameters> executable = target.named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class);
if (theShouldSucceed) {
diff = executable.execute();
assertThat(diff.getParameter()).hasSize(1);
} else {
try {
executable.execute();
fail();
} catch (ForbiddenOperationException e) {
// good
}
}
}
@Test
@ -943,8 +969,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andAllowAllResponses().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andAllowAllResponses().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andRequireExplicitResponseAuthorization().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andRequireExplicitResponseAuthorization().andThen()
.allow().read().resourcesOfType(Patient.class).withAnyId().andThen()
.denyAll()
.build();
@ -977,6 +1003,72 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
}
@Test
public void testDocumentOperation_withExplicitAuthorization() {
IIdType patientId = createPatient();
IIdType compositionId = createResource("Composition", withSubject(patientId));
AuthorizationInterceptor interceptor = new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().read().instance(patientId).andThen()
.allow().operation().named("$document").onInstance(compositionId).andRequireExplicitResponseAuthorization().andThen()
.allow().read().instance(compositionId).andThen()
.denyAll()
.build();
}
};
myServer.getRestfulServer().registerInterceptor(interceptor);
Bundle bundle = myClient.operation().onInstanceVersion(compositionId).named("$document").withNoParameters(Parameters.class).returnResourceType(Bundle.class).execute();
assertEquals(2, bundle.getEntry().size());
}
@Test
public void testDocumentOperation_explicitAuthorizationNotNeeded() {
IIdType patientId = createPatient();
IIdType compositionId = createResource("Composition", withSubject(patientId));
AuthorizationInterceptor interceptor = new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named("$document").onInstance(compositionId).andAllowAllResponses().andThen()
.denyAll()
.build();
}
};
myServer.getRestfulServer().registerInterceptor(interceptor);
Bundle bundle = myClient.operation().onInstanceVersion(compositionId).named("$document").withNoParameters(Parameters.class).returnResourceType(Bundle.class).execute();
assertEquals(2, bundle.getEntry().size());
}
@Test
public void testDocumentOperation_withoutExplicitAuthorization() {
IIdType patientId = createPatient();
IIdType compositionId = createResource("Composition", withSubject(patientId));
AuthorizationInterceptor interceptor = new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named("$document").onInstance(compositionId).andRequireExplicitResponseAuthorization().andThen()
.allow().read().instance(compositionId).andThen()
.denyAll()
.build();
}
};
myServer.getRestfulServer().registerInterceptor(interceptor);
try {
myClient.operation().onInstanceVersion(compositionId).named("$document").withNoParameters(Parameters.class).returnResourceType(Bundle.class).execute();
fail();
} catch (ForbiddenOperationException e) {
// good
}
}
@Test
public void testGraphQL_AllowedByType_Instance() throws IOException {
@ -1205,8 +1297,17 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(resp));
}
@Test
public void testOperationEverything_SomeIncludedResourcesNotAuthorized() {
@ParameterizedTest
@CsvSource({
// RequireExplicitResponseAuthorization , AddNotExplicitlyAuthorizedResources , ShouldSucceed
"true , false , true",
"true , true , false",
"false , false , true",
"false , true , true",
})
public void testOperationEverything_SomeIncludedResourcesNotAuthorized(boolean theRequireExplicitResponseAuthorization, boolean theAddNotExplicitlyAuthorizedResources, boolean theShouldSucceed) {
int expectedCount = 2;
Patient pt1 = new Patient();
pt1.setActive(true);
final IIdType pid1 = myClient.create().resource(pt1).execute().getId().toUnqualifiedVersionless();
@ -1216,45 +1317,57 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
obs1.setSubject(new Reference(pid1));
myClient.create().resource(obs1).execute();
if (theAddNotExplicitlyAuthorizedResources) {
// Add an Encounter, which will be returned by $everything but that hasn't been
// explicitly authorized
Encounter enc = new Encounter();
enc.setSubject(new Reference(pid1));
myClient.create().resource(enc).execute();
Organization org = new Organization();
org.setName("Hello");
IIdType orgId = myClient.create().resource(org).execute().getId().toUnqualifiedVersionless();
expectedCount++;
pt1.setId(pid1);
pt1.setManagingOrganization(new Reference(orgId));
myClient.update().resource(pt1).execute();
expectedCount++;
}
myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@SuppressWarnings("deprecation")
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named(JpaConstants.OPERATION_EVERYTHING).onInstance(pid1).andRequireExplicitResponseAuthorization().andThen()
.allow().read().resourcesOfType(Patient.class).inCompartment("Patient", pid1).andThen()
.allow().read().resourcesOfType(Observation.class).inCompartment("Patient", pid1).andThen()
.allow().create().resourcesOfType(Encounter.class).withAnyId().andThen()
.build();
IAuthRuleBuilder ruleBuilder = new RuleBuilder();
if (theRequireExplicitResponseAuthorization) {
ruleBuilder.allow().operation().named(JpaConstants.OPERATION_EVERYTHING).onInstance(pid1).andRequireExplicitResponseAuthorization().andThen();
} else {
ruleBuilder.allow().operation().named(JpaConstants.OPERATION_EVERYTHING).onInstance(pid1).andAllowAllResponsesWithAllResourcesAccess().andThen();
}
ruleBuilder.allow().read().resourcesOfType(Patient.class).inCompartment("Patient", pid1).andThen();
ruleBuilder.allow().read().resourcesOfType(Observation.class).inCompartment("Patient", pid1).andThen();
return ruleBuilder.build();
}
});
Bundle outcome = myClient
IOperationUntypedWithInput<Bundle> executable = myClient
.operation()
.onInstance(pid1)
.named(JpaConstants.OPERATION_EVERYTHING)
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
assertThat(outcome.getEntry()).hasSize(2);
.returnResourceType(Bundle.class);
// Add an Encounter, which will be returned by $everything but that hasn't been
// explicitly authorized
Encounter enc = new Encounter();
enc.setSubject(new Reference(pid1));
myClient.create().resource(enc).execute();
try {
myClient
.operation()
.onInstance(pid1)
.named(JpaConstants.OPERATION_EVERYTHING)
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
fail();
} catch (ForbiddenOperationException e) {
assertThat(e.getMessage()).contains("Access denied by default policy");
if (theShouldSucceed) {
Bundle outcome = executable.execute();
assertThat(outcome.getEntry()).hasSize(expectedCount);
} else {
try {
executable.execute();
fail();
} catch (ForbiddenOperationException e) {
assertThat(e.getMessage()).contains("Access denied by default policy");
}
}
}
@ -2029,6 +2142,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
super(theResourceTypes);
}
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
List<IAuthRule> rules = new ArrayList<>(super.buildRuleList(theRequestDetails));
List<IAuthRule> rulesToAdd = new RuleBuilder().allow().transaction().withAnyOperation().andApplyNormalRules().build();

View File

@ -164,7 +164,8 @@ public class AuthorizationInterceptor implements IRuleApplier {
rules.size(),
getPointcutNameOrEmpty(thePointcut),
getResourceTypeOrEmpty(theInputResource),
getResourceTypeOrEmpty(theOutputResource));
getResourceTypeOrEmpty(theOutputResource),
thePointcut);
Verdict verdict = null;
for (IAuthRule nextRule : rules) {
@ -528,7 +529,7 @@ public class AuthorizationInterceptor implements IRuleApplier {
case EXTENDED_OPERATION_TYPE:
case EXTENDED_OPERATION_INSTANCE: {
if (theResponseObject != null) {
resources = toListOfResourcesAndExcludeContainer(theResponseObject, fhirContext);
resources = toListOfResourcesAndExcludeContainerUnlessStandalone(theResponseObject, fhirContext);
}
break;
}
@ -575,7 +576,7 @@ public class AuthorizationInterceptor implements IRuleApplier {
OUT,
}
protected static List<IBaseResource> toListOfResourcesAndExcludeContainer(
protected static List<IBaseResource> toListOfResourcesAndExcludeContainerUnlessStandalone(
IBaseResource theResponseObject, FhirContext fhirContext) {
if (theResponseObject == null) {
return Collections.emptyList();
@ -588,6 +589,13 @@ public class AuthorizationInterceptor implements IRuleApplier {
return Collections.singletonList(theResponseObject);
}
return toListOfResourcesAndExcludeContainer(theResponseObject, fhirContext);
}
@Nonnull
public static List<IBaseResource> toListOfResourcesAndExcludeContainer(
IBaseResource theResponseObject, FhirContext fhirContext) {
List<IBaseResource> retVal;
retVal = fhirContext.newTerser().getAllPopulatedChildElementsOfType(theResponseObject, IBaseResource.class);
// Exclude the container

View File

@ -27,11 +27,9 @@ public interface IAuthRuleBuilderOperationNamedAndScoped {
IAuthRuleBuilderRuleOpClassifierFinished andAllowAllResponses();
/**
* Responses for this operation will not be checked and access to all resources is allowed. This
* is intended for operations which are known to fetch a graph of resources that is known to be
* safe, such as `$everything` which may access and fetch resources outside the patient's compartment
* but enforces safety in what it fetches via strict SQL queries.
* @deprecated This is a synonym for {@link #andAllowAllResponses()}, use that method instead
*/
@Deprecated(since = "7.6.0")
IAuthRuleBuilderRuleOpClassifierFinished andAllowAllResponsesWithAllResourcesAccess();
/**

View File

@ -85,6 +85,7 @@ class OperationRule extends BaseRule implements IAuthRule {
myAppliesToTypes = theAppliesToTypes;
}
@SuppressWarnings("EnumSwitchStatementWhichMissesCases")
@Override
public Verdict applyRule(
RestOperationTypeEnum theOperation,
@ -97,23 +98,8 @@ class OperationRule extends BaseRule implements IAuthRule {
Pointcut thePointcut) {
FhirContext ctx = theRequestDetails.getServer().getFhirContext();
// Operation rules apply to the execution of the operation itself, not to side effects like
// loading resources (that will presumably be reflected in the response). Those loads need
// to be explicitly authorized
if (!myAllowAllResourcesAccess && isResourceAccess(thePointcut)) {
return null;
}
boolean applies = false;
switch (theOperation) {
case ADD_TAGS:
case DELETE_TAGS:
case GET_TAGS:
case GET_PAGE:
case GRAPHQL_REQUEST:
// These things can't be tracked by the AuthorizationInterceptor
// at this time
return null;
case EXTENDED_OPERATION_SERVER:
if (myAppliesToServer || myAppliesAtAnyLevel) {
applies = true;
@ -138,10 +124,7 @@ class OperationRule extends BaseRule implements IAuthRule {
applies = true;
} else {
IIdType requestResourceId = null;
if (theInputResourceId != null) {
requestResourceId = theInputResourceId;
}
if (requestResourceId == null && myAllowAllResponses) {
if (theRequestDetails.getId() != null) {
requestResourceId = theRequestDetails.getId();
}
if (requestResourceId != null) {
@ -168,40 +151,6 @@ class OperationRule extends BaseRule implements IAuthRule {
}
}
break;
case CREATE:
break;
case DELETE:
break;
case HISTORY_INSTANCE:
break;
case HISTORY_SYSTEM:
break;
case HISTORY_TYPE:
break;
case READ:
break;
case SEARCH_SYSTEM:
break;
case SEARCH_TYPE:
break;
case TRANSACTION:
break;
case UPDATE:
break;
case VALIDATE:
break;
case VREAD:
break;
case METADATA:
break;
case META_ADD:
break;
case META:
break;
case META_DELETE:
break;
case PATCH:
break;
default:
return null;
}
@ -214,13 +163,33 @@ class OperationRule extends BaseRule implements IAuthRule {
return null;
}
return newVerdict(
theOperation,
theRequestDetails,
theInputResource,
theInputResourceId,
theOutputResource,
theRuleApplier);
if (theOutputResource == null) {
// This is the request part
return newVerdict(
theOperation,
theRequestDetails,
theInputResource,
theInputResourceId,
theOutputResource,
theRuleApplier);
} else {
// This is the response part, so we might want to check all of the
// resources in the response
if (myAllowAllResponses) {
return newVerdict(
theOperation,
theRequestDetails,
theInputResource,
theInputResourceId,
theOutputResource,
theRuleApplier);
} else {
List<IBaseResource> outputResources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(
theOutputResource, theRequestDetails.getFhirContext());
return RuleImplOp.applyRulesToResponseResources(
theRequestDetails, theRuleApplier, thePointcut, outputResources);
}
}
}
/**

View File

@ -828,31 +828,48 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
return verdict;
} else if (theOutputResource != null) {
List<IBaseResource> outputResources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(
theOutputResource, theRequestDetails.getFhirContext());
Verdict verdict = null;
for (IBaseResource nextResource : outputResources) {
if (nextResource == null) {
continue;
}
Verdict newVerdict = theRuleApplier.applyRulesAndReturnDecision(
RestOperationTypeEnum.READ, theRequestDetails, null, null, nextResource, thePointcut);
if (newVerdict == null) {
continue;
} else if (verdict == null) {
verdict = newVerdict;
} else if (verdict.getDecision() == PolicyEnum.ALLOW && newVerdict.getDecision() == PolicyEnum.DENY) {
verdict = newVerdict;
}
}
return verdict;
return applyRulesToResponseBundle(theRequestDetails, theOutputResource, theRuleApplier, thePointcut);
} else {
return null;
}
}
@Nullable
private static Verdict applyRulesToResponseBundle(
RequestDetails theRequestDetails,
IBaseResource theOutputResource,
IRuleApplier theRuleApplier,
Pointcut thePointcut) {
List<IBaseResource> outputResources =
AuthorizationInterceptor.toListOfResourcesAndExcludeContainerUnlessStandalone(
theOutputResource, theRequestDetails.getFhirContext());
return applyRulesToResponseResources(theRequestDetails, theRuleApplier, thePointcut, outputResources);
}
@Nullable
public static Verdict applyRulesToResponseResources(
RequestDetails theRequestDetails,
IRuleApplier theRuleApplier,
Pointcut thePointcut,
List<IBaseResource> outputResources) {
Verdict verdict = null;
for (IBaseResource nextResource : outputResources) {
if (nextResource == null) {
continue;
}
Verdict newVerdict = theRuleApplier.applyRulesAndReturnDecision(
RestOperationTypeEnum.READ, theRequestDetails, null, null, nextResource, thePointcut);
if (newVerdict == null) {
continue;
} else if (verdict == null) {
verdict = newVerdict;
} else if (verdict.getDecision() == PolicyEnum.ALLOW && newVerdict.getDecision() == PolicyEnum.DENY) {
verdict = newVerdict;
}
}
return verdict;
}
private boolean isInvalidNestedBundleRequest(BundleEntryParts theEntry) {
IBaseResource resource = theEntry.getResource();
if (!(resource instanceof IBaseBundle)) {

View File

@ -4230,7 +4230,7 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Bundle");
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(searchSet, ourCtx);
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainerUnlessStandalone(searchSet, ourCtx);
assertEquals(1, resources.size());
assertTrue(resources.contains(bundle));
}
@ -4247,7 +4247,7 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Patient");
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(searchSet, ourCtx);
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainerUnlessStandalone(searchSet, ourCtx);
assertEquals(2, resources.size());
assertTrue(resources.contains(patient1));
assertTrue(resources.contains(patient2));