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.

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

fhir-smart not enforced for JSONPatch and FHIRPathPatch #4101

Closed
jornvanwier opened this issue Nov 28, 2022 · 1 comment
Closed

fhir-smart not enforced for JSONPatch and FHIRPathPatch #4101

jornvanwier opened this issue Nov 28, 2022 · 1 comment
Assignees
Labels
bug Something isn't working P1 Priority 1 - Must Have

Comments

@jornvanwier
Copy link
Contributor

Describe the bug

fhir-smart does appear to enforce JSONPatch and FHIRPathPatch requests. When using a JWT with ff94628809f34970a7a8199bccf2f23c as one of the patient_ids and a "patient/*.read" scope the following happens:

This PUT request gives a 403 as expected:

curl 'http://localhost:9080/fhir-server/api/v4/Patient/ff94628809f34970a7a8199bccf2f23c' -H "Authorization: Bearer $bearer" -H 'Content-Type: application/json' -d '{"resourceType": "Patient", "id": "ff94628809f34970a7a8199bccf2f23c", "gender":"female"}' -X PUT

However, if I do what is essentially the same request using JSONPatch, the change is allowed:

curl 'http://localhost:9080/fhir-server/api/v4/Patient/ff94628809f34970a7a8199bccf2f23c' -H "Authorization: Bearer $bearer" -H 'Content-Type: application/json-patch+json' -d '[{ "op": "add", "path": "/gender", "value": "female" }]' -X PATCH

With FHIRPathPatch the same can be observed.

I initially suspected that it was because org.linuxforhealth.fhir.smart.AuthzPolicyEnforcementPersistenceInterceptor does not have a beforePatch method, but after adding this method (with the same implementation as beforeUpdate) the problem persisted.

Am I missing something, or is this a bug in fhir-smart?

Environment

I'm currently still using 4.11.1. Looking through the changelogs and relevant code I don't believe this has already been fixed in the newer versions.

To Reproduce

Steps to reproduce the behavior:

  1. Have a JWT with read-only access to a patient (I'm using "patient/*.read")
  2. Perform a JSONPatch or FHIRPathPatch request for data belonging to the current patient.
  3. The request will succeed.

Expected behavior

The request should not succeed unless the JWT has a write scope for the resource.

@jornvanwier jornvanwier added the bug Something isn't working label Nov 28, 2022
@lmsurpre lmsurpre added the P1 Priority 1 - Must Have label Nov 29, 2022
@lmsurpre lmsurpre self-assigned this Nov 30, 2022
lmsurpre added a commit that referenced this issue Nov 30, 2022
and add corresponding tests to AuthzPolicyEnforcementTest.java

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Nov 30, 2022
…pdate is skippable

Previously, a skippable update (one that would not affect the contents of the resource being updated)
would result in a success without invoking beforeUpdate or beforePatch interceptor methods.

For users of fhir-smart, this behavior is confusing because the user may not have write access to this
resource type, but the server is "allowing" the write anyway (even though its being optimized away).

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Nov 30, 2022
…pdate is skippable

Previously, a skippable update (one that would not affect the contents of the resource being updated)
would result in a success without invoking beforeUpdate or beforePatch interceptor methods.

For users of fhir-smart, this behavior is confusing because the user may not have write access to this
resource type, but the server is "allowing" the write anyway (even though its being optimized away).

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Dec 9, 2022
* issue #4101 - handle patch in fhir-smart interceptor

and add corresponding tests to AuthzPolicyEnforcementTest.java

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>

* updated comment

as suggested

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>

* add clarifying comments

as requested in review

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Dec 9, 2022
…pdate is skippable

Previously, a skippable update (one that would not affect the contents of the resource being updated)
would result in a success without invoking beforeUpdate or beforePatch interceptor methods.

For users of fhir-smart, this behavior is confusing because the user may not have write access to this
resource type, but the server is "allowing" the write anyway (even though its being optimized away).

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
lmsurpre added a commit that referenced this issue Dec 12, 2022
issue #4101 - call beforeX interceptor methods prior to checking if update is skippable
@PrasannaHegde1
Copy link
Collaborator

PrasannaHegde1 commented Dec 14, 2022

Generated a token with scope "patient/*.read" for patient_id 1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9

  1. The JSONPatch request for patient_id 1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9 resulted in a 403 error as expected.
https://localhost:9443/fhir-server/api/v4/Patient/1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9
HTTP Method : PATCH
Content-Type : application/json-patch+json
[{ "op": "add", "path": "/gender", "value": "male" }]

image

  1. The FHIRPathPatch request for patient_id 1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9 resulted in a 403 error as expected.
https://localhost:9443/fhir-server/api/v4/Patient/1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9
HTTP Method : PATCH
Content-Type : application/fhir+json
{
  "resourceType": "Parameters",
  "parameter": [ {
    "name": "operation",
    "part": [ {
      "name": "type",
      "valueCode": "add"
    }, {
      "name": "path",
      "valueString": "Patient"
    }, {
      "name": "name",
      "valueString": "gender"
    }, {
      "name": "value",
      "valueCode": "male"
    } ]
  } ]
}

image

  1. The update Patient(id = 1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9) request resulted in a 403 error as expected.
https://localhost:9443/fhir-server/api/v4/Patient/1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9
HTTP Method : PUT
Content-Type : application/json
{
	 "id" : "1850f9523a4-c7c2cc41-6544-404b-843a-7620eac731d9",
    "resourceType" : "Patient",
    "active" : true,
    "name" : [ {
        "family" : "Doe2",
        "given" : [ "John" ]
    } ],
    "gender" : "male"
}

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1 Priority 1 - Must Have
Projects
None yet
Development

No branches or pull requests

3 participants