FHIR icon indicating copy to clipboard operation
FHIR copied to clipboard

Support Binary.securityContext for Binary resources if the securityContext target is Patient

Open PrasannaHegde1 opened this issue 1 year ago • 2 comments

Describe the bug Currently for binary resources there is no special support for the securityContext element. With https://github.com/linuxforhealth/fhir/issues/3716 , we now issue a warning (or an error if validateSecurityContext=true) when the user tries to specify a securityContext through the Binary.securityContext element.

However, in fhir-smart we already have logic for enforcing access control based on patient compartment membership. Thus it should be possible to add support for Binary.securityContext if the securityContext target is a Patient.

For all other securityContext target types we should continue to either log a warning message or throw an error based on validateSecurityContext configuration.(Please check https://github.com/linuxforhealth/fhir/issues/3716 for more details)

Environment Which version of LinuxForHealth FHIR Server? 6.0

To Reproduce Steps to reproduce the behavior:

  1. Go to 'AuthzPolicyEnforcementPersistenceInterceptor.validateSecurityContext()' method.

Expected behavior If Binary.securityContext is populated and points to a Patient resource, then access to this Binary resource (create, update, delete, etc.) should be restricted in the same manner as other resources that are associated with that patient. For example, if fhir-smart is configured, and the request has the patient/Binary.read scope, then the user can only access this resource if they can also access the corresponding Patient resource.

Additional context Related to https://github.com/linuxforhealth/fhir/issues/3716

Workaround / alternative idea: consider a Binary resource to be part of a Patient Compartment. I believe that this could be done today just by registering a modified version of the Patient CompartmentDefinition (thanks to #2143).

PrasannaHegde1 avatar Oct 20 '22 20:10 PrasannaHegde1

thinking about this a little more, at least in the context of fhir-smart, I think we should be able to make this work with a securityContext that references ANY resource type. if the securityContext references a resource, we'd attempt to dereference (i.e. read) the target resource and then check if the user has access to that resource or not (and has passed scopes to allow the interaction). Then we'd use the result of that decision to decide whether the interaction with the Binary is permitted or not.

We'd need to decide

  1. whether the patient/Binary.read scope would be needed to read the Binary resource, or if its access would be solely tied to the securityContext target
  2. what to do if the target resource doesn't exist on the server...maybe we only need to dereference it if its a patient compartment resource and the user has passed one or more patient/ access scopes?

lmsurpre avatar Oct 21 '22 02:10 lmsurpre

NOTE: its not just about read access. We also shouldn't allow a user to create a Binary resource that has a securityContext which references another resource that they do not have access to. But we need to be careful there not to leak information about the presence/absence of that target.

whether the patient/Binary.read scope would be needed to read the Binary resource, or if its access would be solely tied to the securityContext target

Dag and I discussed this one and had a slight difference of opinion. The way I saw it is this would be an additional check. On top of requiring Binary.read permissions, the user also needs access to the resource referenced by the securityContext. The way Dag saw it is that the proxy resource should be checked instead of performing this Binary.read permission check.

It might be an interesting question for the community because its really about the intersection of this "securityContext" feature in the fhir spec and the definition of these scope strings in the SMART App Launch spec.

lmsurpre avatar Nov 02 '22 13:11 lmsurpre