-
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
Conversation
Here's a bundle with sample AuditEvents created from this approach hapi-fhir-balp-poc-audit-event.bundle.json |
af6680b
to
b9e5453
Compare
Sample AuditEvent created after bug fix (JWT details missing due to typo in JWT keys) - hapi-fhir-balp-poc-put-patient-audit-event.json |
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.
Thanks @ndegwamartin for implementing this prototype; it is a great progress. Like we discussed in the dev call today, and with a deeper look at BalpAuditCaptureInterceptor, I think what we are going to save by reusing that class is not worth the complexity of the integration. Instead let's just use BALP related constants/enums like BalpProfileEnum and implement the rest using our infra/libs like PatientFinder
.
I have also shared some comments for future non-prototype implementation; no need to address them in this PR.
*/ | ||
String postProcess(RequestDetailsReader request, HttpResponse response) throws IOException; | ||
|
||
@Nullable |
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.
Is it possible to keep this interface change to the minimal proposed in the design doc, i.e., just agent
fields like who
which we cannot decide at core level?
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.
Agreed
return false; | ||
} | ||
|
||
private IBaseResource getResource(RequestDetails requestDetails) { |
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.
We might be able to avoid this functionality and avoid a separate fetch as the resource should be available either on the way the request is going to the FHIR server or coming back (or by stitching information together).
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.
Did you mean this method here?
fhir-gateway/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java
Line 256 in 1511a5d
private IBaseResource fetchStoredResource(RequestDetails requestDetails) { |
This one just loads it from requestDetails.getRequestContentsIfLoaded()
private IBaseResource getResource(RequestDetails requestDetails) {
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.
Yes, I meant that; made my comment on the wrong line.
String content = null; | ||
if (HttpUtil.isResponseValid(response)) { | ||
try { | ||
// Request was successful so record Audit Event |
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.
What do we need for other operations (other than access to non-public APIs)?
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.
The main difference with the other ops is that the method requires us to implement this interface to pass to the hook method - IPreResourceShowDetails.java
It is mainly just a wrapper around resources being processed so should be ok to use. They use it to manipulate the resources Read/Searched before returning to the user e.g. masking out private fields.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be at plugins
level or server
? This seems to be independent from the actual grant/deny decision.
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, this is independent and can reside on the server.
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 comment
The 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 comment
The 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
return NoOpAccessDecision.accessGranted(); | ||
return balpAccessDecision.withAccess(NoOpAccessDecision.accessGranted()); | ||
} | ||
return NoOpAccessDecision.accessDenied(); |
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.
IGenericClient genericClient = | ||
fhirContext.newRestfulGenericClient(httpFhirClient.getBaseUrl()); | ||
|
||
BalpAccessDecision balpAccessDecision = new BalpAccessDecision(genericClient); |
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.
It feels it is better to pass the genericClient
to ListAccessChecker and let it create instances of BalpAccessDecision
to return (and make BalpAccessDecision
immutable).
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.
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 comment
The 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.
@Override | ||
public String postProcess(RequestDetailsReader request, HttpResponse response) | ||
throws IOException { | ||
return ""; |
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 be overridden.
} | ||
|
||
@Override | ||
public @NotNull String massageResourceIdForStorage( |
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.
This also seems to be independent of plugins.
outcome.getBalpAuditEventSink(), outcome.getBalpAuditContextService()); | ||
|
||
// PoC - for now processing non-Bundle requests only | ||
switch (requestDetails.getRequestType()) { |
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.
I think this logic of going through different operation cases should eventually go into the plugins and the goal of that would be just to fill-out the agent
fields of the created AuditEvent(s). I understand you have probably done this just for the prototype; I am trying to understand the best pattern for core/server to implement this feature together.
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. Plugins approach would be more inline with the existing pattern
return false; | ||
} | ||
|
||
private IBaseResource getResource(RequestDetails requestDetails) { |
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.
Did you mean this method here?
fhir-gateway/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java
Line 256 in 1511a5d
private IBaseResource fetchStoredResource(RequestDetails requestDetails) { |
This one just loads it from requestDetails.getRequestContentsIfLoaded()
private IBaseResource getResource(RequestDetails requestDetails) {
*/ | ||
String postProcess(RequestDetailsReader request, HttpResponse response) throws IOException; | ||
|
||
@Nullable |
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.
Agreed
outcome.getBalpAuditEventSink(), outcome.getBalpAuditContextService()); | ||
|
||
// PoC - for now processing non-Bundle requests only | ||
switch (requestDetails.getRequestType()) { |
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. Plugins approach would be more inline with the existing pattern
<!-- 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 comment
The 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 /docs
folder due to re-formatting.
return NoOpAccessDecision.accessGranted(); | ||
return balpAccessDecision.withAccess(NoOpAccessDecision.accessGranted()); | ||
} | ||
return NoOpAccessDecision.accessDenied(); |
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.
IGenericClient genericClient = | ||
fhirContext.newRestfulGenericClient(httpFhirClient.getBaseUrl()); | ||
|
||
BalpAccessDecision balpAccessDecision = new BalpAccessDecision(genericClient); |
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.
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?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is independent and can reside on the server.
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 comment
The 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
This work is superseded by #400. |
resolves #395
[DNM] Do Not Merge
A PR testing out the Proof of Concept documented on the ticket here #395
Alternative
An alternative approach for the PoC implementation can be found here - opensrp#49