-
Notifications
You must be signed in to change notification settings - Fork 34
[DNM] HAPI FHIR BALP Interceptor PoC for AuditEvents #398
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.
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
Changes from all commits
b3be2d5
1511a5d
b9e5453
8cbcbc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| import ca.uhn.fhir.context.FhirContext; | ||
| import ca.uhn.fhir.rest.api.RequestTypeEnum; | ||
| import ca.uhn.fhir.rest.client.api.IGenericClient; | ||
| import com.auth0.jwt.interfaces.DecodedJWT; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.base.Preconditions; | ||
|
|
@@ -35,6 +36,7 @@ | |
| import com.google.fhir.gateway.interfaces.NoOpAccessDecision; | ||
| import com.google.fhir.gateway.interfaces.PatientFinder; | ||
| import com.google.fhir.gateway.interfaces.RequestDetailsReader; | ||
| import com.google.fhir.gateway.plugin.audit.BalpAccessDecision; | ||
| import java.io.IOException; | ||
| import java.util.Collections; | ||
| import java.util.Objects; | ||
|
|
@@ -61,16 +63,19 @@ public class ListAccessChecker implements AccessChecker { | |
| private final String patientListId; | ||
| private final PatientFinder patientFinder; | ||
| private final Escaper PARAM_ESCAPER = UrlEscapers.urlFormParameterEscaper(); | ||
| private final BalpAccessDecision balpAccessDecision; | ||
|
|
||
| private ListAccessChecker( | ||
| HttpFhirClient httpFhirClient, | ||
| String patientListId, | ||
| FhirContext fhirContext, | ||
| PatientFinder patientFinder) { | ||
| PatientFinder patientFinder, | ||
| BalpAccessDecision balpAccessDecision) { | ||
| this.fhirContext = fhirContext; | ||
| this.httpFhirClient = httpFhirClient; | ||
| this.patientListId = patientListId; | ||
| this.patientFinder = patientFinder; | ||
| this.balpAccessDecision = balpAccessDecision; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -103,6 +108,7 @@ private boolean listIncludesItems(String itemsParam) { | |
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Note this returns true iff at least one of the patient IDs is found in the associated list. | ||
| // The rationale is that a user should have access to a resource iff they are authorized to access | ||
| // at least one of the patients referenced in that resource. This is a subjective decision, so we | ||
|
|
@@ -186,36 +192,41 @@ private AccessDecision processGet(RequestDetailsReader requestDetails) { | |
| // There should be a patient id in search params; the param name is based on the resource. | ||
| if (FhirUtil.isSameResourceType(requestDetails.getResourceName(), ResourceType.List)) { | ||
| if (patientListId.equals(FhirUtil.getIdOrNull(requestDetails))) { | ||
| return NoOpAccessDecision.accessGranted(); | ||
| return balpAccessDecision.withAccess(NoOpAccessDecision.accessGranted()); | ||
| } | ||
| return NoOpAccessDecision.accessDenied(); | ||
| } | ||
| Set<String> patientIds = patientFinder.findPatientsFromParams(requestDetails); | ||
| Set<String> patientQueries = Sets.newHashSet(); | ||
| patientIds.forEach(patientId -> patientQueries.add(String.format("Patient/%s", patientId))); | ||
| return new NoOpAccessDecision(serverListIncludesAllPatients(patientQueries)); | ||
| return balpAccessDecision.withAccess( | ||
| new NoOpAccessDecision(serverListIncludesAllPatients(patientQueries))); | ||
| } | ||
|
|
||
| private AccessDecision processPost(RequestDetailsReader requestDetails) { | ||
| // We have decided to let clients add new patients while understanding its security risks. | ||
| if (FhirUtil.isSameResourceType(requestDetails.getResourceName(), ResourceType.Patient)) { | ||
| return AccessGrantedAndUpdateList.forPatientResource( | ||
| patientListId, httpFhirClient, fhirContext); | ||
| return balpAccessDecision.withAccess( | ||
| AccessGrantedAndUpdateList.forPatientResource( | ||
| patientListId, httpFhirClient, fhirContext)); | ||
| } | ||
| Set<String> patientIds = patientFinder.findPatientsInResource(requestDetails); | ||
| return new NoOpAccessDecision(serverListIncludesAnyPatient(patientIds)); | ||
| return balpAccessDecision.withAccess( | ||
| new NoOpAccessDecision(serverListIncludesAnyPatient(patientIds))); | ||
| } | ||
|
|
||
| private AccessDecision processPut(RequestDetailsReader requestDetails) throws IOException { | ||
| if (FhirUtil.isSameResourceType(requestDetails.getResourceName(), ResourceType.Patient)) { | ||
| AccessDecision accessDecision = checkPatientAccessInUpdate(requestDetails); | ||
| if (accessDecision == null) { | ||
| return AccessGrantedAndUpdateList.forPatientResource( | ||
| patientListId, httpFhirClient, fhirContext); | ||
| return balpAccessDecision.withAccess( | ||
| AccessGrantedAndUpdateList.forPatientResource( | ||
| patientListId, httpFhirClient, fhirContext)); | ||
| } | ||
| return accessDecision; | ||
| return balpAccessDecision.withAccess(accessDecision); | ||
| } | ||
| return checkNonPatientAccessInUpdate(requestDetails, RequestTypeEnum.PUT); | ||
| return balpAccessDecision.withAccess( | ||
| checkNonPatientAccessInUpdate(requestDetails, RequestTypeEnum.PUT)); | ||
| } | ||
|
|
||
| private AccessDecision processPatch(RequestDetailsReader requestDetails) throws IOException { | ||
|
|
@@ -225,9 +236,10 @@ private AccessDecision processPatch(RequestDetailsReader requestDetails) throws | |
| logger.error("Creating a new Patient via PATCH is not allowed"); | ||
| return NoOpAccessDecision.accessDenied(); | ||
| } | ||
| return accessDecision; | ||
| return balpAccessDecision.withAccess(accessDecision); | ||
| } | ||
| return checkNonPatientAccessInUpdate(requestDetails, RequestTypeEnum.PATCH); | ||
| return balpAccessDecision.withAccess( | ||
| checkNonPatientAccessInUpdate(requestDetails, RequestTypeEnum.PATCH)); | ||
| } | ||
|
|
||
| private AccessDecision processDelete(RequestDetailsReader requestDetails) { | ||
|
|
@@ -243,7 +255,8 @@ private AccessDecision processDelete(RequestDetailsReader requestDetails) { | |
| Set<String> patientIds = patientFinder.findPatientsFromParams(requestDetails); | ||
| Set<String> patientQueries = Sets.newHashSet(); | ||
| patientIds.forEach(patientId -> patientQueries.add(String.format("Patient/%s", patientId))); | ||
| return new NoOpAccessDecision(serverListIncludesAllPatients(patientQueries)); | ||
| return balpAccessDecision.withAccess( | ||
| new NoOpAccessDecision(serverListIncludesAllPatients(patientQueries))); | ||
| } | ||
|
|
||
| private AccessDecision checkNonPatientAccessInUpdate( | ||
|
|
@@ -304,7 +317,7 @@ private AccessDecision processBundle(RequestDetailsReader requestDetails) throws | |
| Set<String> putPatientIds = patientRequestsInBundle.getUpdatedPatients(); | ||
|
|
||
| if (!createPatients && putPatientIds.isEmpty()) { | ||
| return NoOpAccessDecision.accessGranted(); | ||
| return balpAccessDecision.withAccess(NoOpAccessDecision.accessGranted()); | ||
| } | ||
|
|
||
| if (putPatientIds.isEmpty()) { | ||
|
|
@@ -400,7 +413,15 @@ public AccessChecker create( | |
| FhirContext fhirContext, | ||
| PatientFinder patientFinder) { | ||
| String patientListId = getListId(jwt); | ||
| return new ListAccessChecker(httpFhirClient, patientListId, fhirContext, patientFinder); | ||
|
|
||
| // Assumes Audit repository FHIR Server is using same HAPI FHIR version (otherwise we need to | ||
| // canonicalize) | ||
| IGenericClient genericClient = | ||
| fhirContext.newRestfulGenericClient(httpFhirClient.getBaseUrl()); | ||
|
|
||
| BalpAccessDecision balpAccessDecision = new BalpAccessDecision(genericClient); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels it is better to pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. On Immutability do you mean that we should return only new instances/deep copies of the AccessDecision after processing by the access checkers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, something like that; basically have a "write owner" of each object that creates that and then can pass it around but it is not mutated afterwards. |
||
| return new ListAccessChecker( | ||
| httpFhirClient, patientListId, fhirContext, patientFinder, balpAccessDecision); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package com.google.fhir.gateway.plugin.audit; | ||
|
|
||
| import ca.uhn.fhir.rest.client.api.IGenericClient; | ||
| import ca.uhn.fhir.storage.interceptor.balp.IBalpAuditContextServices; | ||
| import ca.uhn.fhir.storage.interceptor.balp.IBalpAuditEventSink; | ||
| import com.google.fhir.gateway.interfaces.AccessDecision; | ||
| import com.google.fhir.gateway.interfaces.RequestDetailsReader; | ||
| import com.google.fhir.gateway.interfaces.RequestMutation; | ||
| import java.io.IOException; | ||
| import org.apache.http.HttpResponse; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public class BalpAccessDecision implements AccessDecision { | ||
|
|
||
| private final BalpAuditEventSink eventSink; | ||
| private final IBalpAuditContextServices contextServices; | ||
| private AccessDecision accessDecision; | ||
|
|
||
| public BalpAccessDecision(IGenericClient genericClient) { | ||
| this.eventSink = new BalpAuditEventSink(genericClient); | ||
| this.contextServices = new BalpAuditContextService(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean canAccess() { | ||
| return accessDecision.canAccess(); | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader) { | ||
| return accessDecision.getRequestMutation(requestDetailsReader); | ||
| } | ||
|
|
||
| @Override | ||
| public String postProcess(RequestDetailsReader request, HttpResponse response) | ||
| throws IOException { | ||
| return accessDecision.postProcess(request, response); | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable IBalpAuditEventSink getBalpAuditEventSink() { | ||
| return this.eventSink; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable IBalpAuditContextServices getBalpAuditContextService() { | ||
| return this.contextServices; | ||
| } | ||
|
|
||
| public BalpAccessDecision withAccess(AccessDecision accessDecision) { | ||
| this.accessDecision = accessDecision; | ||
| return this; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package com.google.fhir.gateway.plugin.audit; | ||
|
|
||
| import ca.uhn.fhir.rest.api.server.RequestDetails; | ||
| import ca.uhn.fhir.rest.server.exceptions.AuthenticationException; | ||
| import ca.uhn.fhir.storage.interceptor.balp.IBalpAuditContextServices; | ||
| import com.auth0.jwt.JWT; | ||
| import com.auth0.jwt.exceptions.JWTDecodeException; | ||
| import com.auth0.jwt.interfaces.DecodedJWT; | ||
| import com.google.fhir.gateway.JwtUtil; | ||
| import jakarta.annotation.Nonnull; | ||
| import org.apache.http.HttpHeaders; | ||
| import org.hl7.fhir.instance.model.api.IBaseResource; | ||
| import org.hl7.fhir.instance.model.api.IIdType; | ||
| import org.hl7.fhir.r4.model.Identifier; | ||
| import org.hl7.fhir.r4.model.Reference; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| public class BalpAuditContextService implements IBalpAuditContextServices { | ||
|
|
||
| private static final String BEARER_PREFIX = "Bearer "; | ||
| private static final String CLAIM_NAME = "name"; | ||
| private static final String CLAIM_PREFERRED_NAME = "preferred_username"; | ||
| private static final String CLAIM_SUBJECT = "sub"; | ||
|
|
||
| @Override | ||
| public @NotNull Reference getAgentClientWho(RequestDetails requestDetails) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is independent and can reside on the server. |
||
|
|
||
| return new Reference() | ||
| // .setReference("Device/fhir-info-gateway") | ||
| .setType("Device") | ||
| .setDisplay("FHIR Info Gateway") | ||
| .setIdentifier( | ||
| new Identifier() | ||
| .setSystem("http://fhir-info-gateway/devices") | ||
| .setValue("fhir-info-gateway-001")); | ||
| } | ||
|
|
||
| @Override | ||
| public @NotNull Reference getAgentUserWho(RequestDetails requestDetails) { | ||
|
|
||
| String username = getClaimIfExists(requestDetails, CLAIM_PREFERRED_NAME); | ||
| String name = getClaimIfExists(requestDetails, CLAIM_NAME); | ||
| String subject = getClaimIfExists(requestDetails, CLAIM_SUBJECT); | ||
|
|
||
| return new Reference() | ||
| // .setReference("Practitioner/" + subject) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be a valid reference to a resource in the FHIR server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FHIR allows creating a reference with an Identifier only if a Reference would break validation - https://hl7.org/fhir/references.html#Reference |
||
| .setType("Practitioner") | ||
| .setDisplay(name) | ||
| .setIdentifier( | ||
| new Identifier() | ||
| .setSystem("http://fhir-info-gateway/practitioners") | ||
| .setValue(username)); | ||
| } | ||
|
|
||
| @Override | ||
| public @NotNull String massageResourceIdForStorage( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also seems to be independent of plugins. |
||
| @Nonnull RequestDetails theRequestDetails, | ||
| @Nonnull IBaseResource theResource, | ||
| @Nonnull IIdType theResourceId) { | ||
|
|
||
| /** | ||
| * Server not configured to allow external references resulting to InvalidRequestException: HTTP | ||
| * 400 : HAPI-0507: Resource contains external reference to URL. Here we should use relative | ||
| * references instead e.g. Patient/123; | ||
| */ | ||
| // String serverBaseUrl = theRequestDetails.getFhirServerBase(); | ||
| return theRequestDetails.getId() != null | ||
| ? theRequestDetails.getId().getValue() | ||
| : ""; // For entity POST there will be no agent.who entry reference since not generated yet | ||
| } | ||
|
|
||
| public String getClaimIfExists(RequestDetails requestDetails, String claimName) { | ||
| String claim; | ||
| try { | ||
| String authHeader = requestDetails.getHeader(HttpHeaders.AUTHORIZATION); | ||
| String bearerToken = authHeader.substring(BEARER_PREFIX.length()); | ||
| DecodedJWT jwt; | ||
|
|
||
| jwt = JWT.decode(bearerToken); | ||
| claim = JwtUtil.getClaimOrDie(jwt, claimName); | ||
| } catch (JWTDecodeException | AuthenticationException e) { | ||
| claim = ""; | ||
| } | ||
| return claim; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package com.google.fhir.gateway.plugin.audit; | ||
|
|
||
| import ca.uhn.fhir.rest.client.api.IGenericClient; | ||
| import ca.uhn.fhir.storage.interceptor.balp.IBalpAuditEventSink; | ||
| import org.hl7.fhir.r4.model.AuditEvent; | ||
|
|
||
| public class BalpAuditEventSink implements IBalpAuditEventSink { | ||
|
|
||
| private final IGenericClient genericClient; | ||
|
|
||
| public BalpAuditEventSink(IGenericClient genericClient) { | ||
| this.genericClient = genericClient; | ||
| } | ||
|
|
||
| @Override | ||
| public void recordAuditEvent(AuditEvent auditEvent) { | ||
| genericClient.create().resource(auditEvent).prettyPrint().encodedJson().execute(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -266,7 +266,7 @@ | |
|
|
||
| <!-- apply a specific flavor of google-java-format and reflow long strings --> | ||
| <googleJavaFormat> | ||
| <version>1.15.0</version> | ||
| <version>1.17.0</version> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Upgraded to JDK 21 hence this bump. Requires us to run spotless apply and push all content in |
||
| <style>GOOGLE</style> | ||
| <reflowLongStrings>true</reflowLongStrings> | ||
| </googleJavaFormat> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we file AuditEvents for access-denied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can add the support.
We might want to make it configurable especially if we are using the same FHIR Data Store to persist both the health data and the forbidden access Audit Events because I assume the latter to be profuse e.g. any valid request with an expired token would be logged.
Thus the caveat would be the growing database size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that we need some configuration parameters to control what type of AuditEvents are created (and in future where they are stored).
But back to the actual AuditEvent for access-denied question, it seems to me that the BALP IG is fairly silent about this and just refers to AccessDenied in core FHIR. But it also has a profile for consent authorization decisions. Hence I am not sure creation of AuditEvent in the access-denied scenarios is necessary or not.