Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #4101 - handle patch in fhir-smart interceptor #4103

Merged
merged 3 commits into from
Dec 9, 2022
Merged

issue #4101 - handle patch in fhir-smart interceptor #4103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3a237e
issue #4101 - handle patch in fhir-smart interceptor
lmsurpre Nov 30, 2022
8cd8136
updated comment
lmsurpre Dec 6, 2022
baa9e74
add clarifying comments
lmsurpre Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...main/java/org/linuxforhealth/fhir/smart/AuthzPolicyEnforcementPersistenceInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,15 @@ public void beforeDelete(FHIRPersistenceEvent event) throws FHIRPersistenceInter

@Override
public void beforeUpdate(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
beforeUpdateOrPatch(event);
}

@Override
public void beforePatch(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
beforeUpdateOrPatch(event);
}

private void beforeUpdateOrPatch(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
DecodedJWT jwt = JWT.decode(getAccessToken());
Set<String> patientIdFromToken = getPatientIdFromToken(jwt);
Map<ContextType, List<Scope>> scopesFromToken = getScopesFromToken(jwt);
Expand Down
81 changes: 80 additions & 1 deletion fhir-smart/src/test/java/org/linuxforhealth/fhir/smart/test/AuthzPolicyEnforcementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ public void testCreate(String scopeString, List<String> contextIds, Set<Resource
}
}

// For the update interaction, both read and write permissions are needed.
// * In the "allowed" case both READ and WRITE "shouldSucceed" (or else it should not have been allowed)
// * In the "disallowed" (Exception) case, then either the READ or WRITE interaction was expected
// to not succeed (or else it should have been allowed)
@Test(dataProvider = "scopeStringProvider")
public void testUpdate(String scopeString, List<String> contextIds, Set<ResourceType> resourceTypesPermittedByScope, Permission permission) {
FHIRRequestContext.get().setHttpHeaders(buildRequestHeaders(scopeString, contextIds));
Expand Down Expand Up @@ -255,7 +259,8 @@ public void testUpdate(String scopeString, List<String> contextIds, Set<Resource
shouldSucceed(resourceTypesPermittedByScope, BINARY, WRITE_APPROVED, permission));
}

// Test update Binary Resource which has a securityContext. Should Fail since securityContext is not supported.
// Test beforePatch Binary Resource which has a securityContext.
// It should be an exception since securityContext is not supported.
try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Binary");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(binaryWithSecurityContext, properties);
Expand All @@ -267,9 +272,82 @@ public void testUpdate(String scopeString, List<String> contextIds, Set<Resource
shouldSucceed(resourceTypesPermittedByScope, BINARY, WRITE_APPROVED, permission)) {
assertTrue(e.getMessage().equals("securityContext is not supported for resource type Binary"));
}
// else beforeUpdate was rejected due to normal fhir-smart behavior (non-securityContext-related)
}
}

// The patch interaction is just like the update interaction; both read and write permissions are needed.
// * In the "allowed" case both READ and WRITE "shouldSucceed" (or else it should not have been allowed)
// * In the "disallowed" (Exception) case, then either the READ or WRITE interaction was expected
// to not succeed (or else it should have been allowed)
@Test(dataProvider = "scopeStringProvider")
public void testPatch(String scopeString, List<String> contextIds, Set<ResourceType> resourceTypesPermittedByScope, Permission permission) {
FHIRRequestContext.get().setHttpHeaders(buildRequestHeaders(scopeString, contextIds));

try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Patient");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(patient, properties);
event.setPrevFhirResource(patient);
interceptor.beforePatch(event);
assertTrue(shouldSucceed(resourceTypesPermittedByScope, PATIENT, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, PATIENT, WRITE_APPROVED, permission));
} catch (FHIRPersistenceInterceptorException e) {
assertFalse(shouldSucceed(resourceTypesPermittedByScope, PATIENT, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, PATIENT, WRITE_APPROVED, permission));
}

try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Observation");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(observation, properties);
event.setPrevFhirResource(observation);
interceptor.beforePatch(event);
assertTrue(shouldSucceed(resourceTypesPermittedByScope, OBSERVATION, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, OBSERVATION, WRITE_APPROVED, permission));
} catch (FHIRPersistenceInterceptorException e) {
assertFalse(shouldSucceed(resourceTypesPermittedByScope, OBSERVATION, READ_APPROVED, permission) &&
Copy link
Collaborator

@punktilious punktilious Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an || instead of && ?

I'm assuming you want both of these to be false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the && is right. If either one of these is false, then we expect the method under test to throw a FHIRPersistenceInterceptorException.

shouldSucceed(resourceTypesPermittedByScope, OBSERVATION, WRITE_APPROVED, permission));
}

try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Condition");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(condition, properties);
event.setPrevFhirResource(condition);
interceptor.beforePatch(event);
assertTrue(shouldSucceed(resourceTypesPermittedByScope, CONDITION, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, CONDITION, WRITE_APPROVED, permission));
} catch (FHIRPersistenceInterceptorException e) {
assertFalse(shouldSucceed(resourceTypesPermittedByScope, CONDITION, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, CONDITION, WRITE_APPROVED, permission));
}

// Test beforePatch Binary Resource which does not have a securityContext. Should Succeed
try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Binary");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(binary, properties);
event.setPrevFhirResource(binary);
interceptor.beforePatch(event);
assertTrue(shouldSucceed(resourceTypesPermittedByScope, BINARY, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, BINARY, WRITE_APPROVED, permission));
} catch (FHIRPersistenceInterceptorException e) {
assertFalse(shouldSucceed(resourceTypesPermittedByScope, BINARY, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, BINARY, WRITE_APPROVED, permission));
}

// Test beforePatch Binary Resource which has a securityContext.
// It should be an exception since securityContext is not supported.
try {
properties.put(FHIRPersistenceEvent.PROPNAME_RESOURCE_TYPE, "Binary");
FHIRPersistenceEvent event = new FHIRPersistenceEvent(binaryWithSecurityContext, properties);
event.setPrevFhirResource(binaryWithSecurityContext);
interceptor.beforePatch(event);
fail("Did not receive the expected FHIRPersistenceInterceptorException");
} catch (FHIRPersistenceInterceptorException e) {
if (shouldSucceed(resourceTypesPermittedByScope, BINARY, READ_APPROVED, permission) &&
shouldSucceed(resourceTypesPermittedByScope, BINARY, WRITE_APPROVED, permission)) {
assertTrue(e.getMessage().equals("securityContext is not supported for resource type Binary"));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else?

Copy link
Member Author

@lmsurpre lmsurpre Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}
// else beforePatch was rejected due to normal fhir-smart behavior (non-securityContext-related)

// else beforePatch was rejected due to normal fhir-smart behavior (non-securityContext-related)
}
}

@Test(dataProvider = "scopeStringProvider")
Expand Down Expand Up @@ -1559,6 +1637,7 @@ private Map<String, List<String>> buildRequestHeaders(String scopeString, List<S

/**
* @return true if the interaction should succeed, otherwise false
* @implNote this method is used to help establish the "expected outcome" of a given interaction
*/
private boolean shouldSucceed(Set<ResourceType> resourceTypesPermittedByScope, ResourceType requiredResourceType,
List<Permission> permissionsPermittedByScope, Permission requiredPermission) {
Expand Down