When in compartment scopes are used, token should not need to have read access to the parent resource #528

Closed
mattiuusitalo opened this Issue Dec 12, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@mattiuusitalo

We have Patient/Observation.read scope. We create a rule with

Rulebuilder builder;
IdDt userIdPatientId;
builder.allow("read:" + scope).read().resourcesOfType("Patient/Observation.read").inCompartment("Patient", userIdPatientId);

Reading Observation resources fails because of the comparison done below:

if (appliesToResourceId != null && appliesToResourceId.hasResourceType() && appliesToResourceId.hasIdPart()) {

It checks if token's scope is Patient.read.

If we add for example ValueSet.read which is not an in compartment scope, reading succeeds.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 12, 2016

Owner

The issue there is that the AuthorizationInterceptor isn't designed to handle SMART scopes. It's a bit lower-level than that. Essentially you need to pass in Observation as the resource type, not Patient/Observation.read. This should actually fail since the resource type is invalid.

One of the things I'm hoping we'll create (or someone will create first and contribute) is an adapter that sits on top of it which allows the SMART specific scopes.. but for now you'd need to parse the scope and call the builder appropriately. E.g. the scope Patient/Observation.read would translate to the following builder code:

builder.allow("read:" + scope).read().resourcesOfType("Observation").inCompartment("Patient", userIdPatientId);
Owner

jamesagnew commented Dec 12, 2016

The issue there is that the AuthorizationInterceptor isn't designed to handle SMART scopes. It's a bit lower-level than that. Essentially you need to pass in Observation as the resource type, not Patient/Observation.read. This should actually fail since the resource type is invalid.

One of the things I'm hoping we'll create (or someone will create first and contribute) is an adapter that sits on top of it which allows the SMART specific scopes.. but for now you'd need to parse the scope and call the builder appropriately. E.g. the scope Patient/Observation.read would translate to the following builder code:

builder.allow("read:" + scope).read().resourcesOfType("Observation").inCompartment("Patient", userIdPatientId);
@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 12, 2016

Owner

..actually, looking at this, there is no resourcesAsType(String) method. I don't understand how the code you posted compiles.

Could you post a more complete code snippet?

Owner

jamesagnew commented Dec 12, 2016

..actually, looking at this, there is no resourcesAsType(String) method. I don't understand how the code you posted compiles.

Could you post a more complete code snippet?

@mattiuusitalo

This comment has been minimized.

Show comment
Hide comment
@mattiuusitalo

mattiuusitalo Dec 13, 2016

Sorry, I missed something while taking the snippet. Here's a more complete part.

private void createPatientReadScope(RuleBuilder builder, String scope, IdDt userIdPatientId, Class<? extends IBaseResource> type) {

	builder.allow("read:" + scope).read().resourcesOfType(type).inCompartment(PhrConstants.RESOURCE_PATIENT, userIdPatientId);
	LOGGER.info("allow read:" + scope);

    }

I will try your earlier suggestion about modifying the scope little bit.

mattiuusitalo commented Dec 13, 2016

Sorry, I missed something while taking the snippet. Here's a more complete part.

private void createPatientReadScope(RuleBuilder builder, String scope, IdDt userIdPatientId, Class<? extends IBaseResource> type) {

	builder.allow("read:" + scope).read().resourcesOfType(type).inCompartment(PhrConstants.RESOURCE_PATIENT, userIdPatientId);
	LOGGER.info("allow read:" + scope);

    }

I will try your earlier suggestion about modifying the scope little bit.

@eevaturkka

This comment has been minimized.

Show comment
Hide comment
@eevaturkka

eevaturkka Dec 13, 2016

We've evaluated this futher and it seems like the interceptor tries to evaluate the compartment for incoming request already - the code itself is valid and does compile and worked for earlier (summer) version of Hapi. Does this have something to do with that new outgoing resource id parameter in api?

We've evaluated this futher and it seems like the interceptor tries to evaluate the compartment for incoming request already - the code itself is valid and does compile and worked for earlier (summer) version of Hapi. Does this have something to do with that new outgoing resource id parameter in api?

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 13, 2016

Owner

Can you supply more details about exactly what is happening? I'm still not getting it, sorry. I'd like to create a unit test to recreate what you're seeing.

Ideally:

  • What parameters are you using to build the rule list (for the code snippet you posted, what does the PhrConstants.RESOURCE_PATIENT contain, what is the value of the type parameter etc)
  • What URL are you invoking the server with
  • What does the expected response contain
  • What does the actual response contain (resource type, resource ID, fields relevant to the compartment)
Owner

jamesagnew commented Dec 13, 2016

Can you supply more details about exactly what is happening? I'm still not getting it, sorry. I'd like to create a unit test to recreate what you're seeing.

Ideally:

  • What parameters are you using to build the rule list (for the code snippet you posted, what does the PhrConstants.RESOURCE_PATIENT contain, what is the value of the type parameter etc)
  • What URL are you invoking the server with
  • What does the expected response contain
  • What does the actual response contain (resource type, resource ID, fields relevant to the compartment)
@mattiuusitalo

This comment has been minimized.

Show comment
Hide comment
@mattiuusitalo

mattiuusitalo Dec 13, 2016

Here's what goes to the call:
builder.allow("read:" + scope).read().resourcesOfType(Observation.class).inCompartment("Patient", userIdPatientId);

Later on in the chain we always add a final rule which denies all. This way, if no scope allows the activity, default action is to deny the action.

We got rid of the problem by overriding this method:
https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java#L222

In case it is a READ request, skip checking the rules. They get validated in the outgoingResponse call.

Here's what goes to the call:
builder.allow("read:" + scope).read().resourcesOfType(Observation.class).inCompartment("Patient", userIdPatientId);

Later on in the chain we always add a final rule which denies all. This way, if no scope allows the activity, default action is to deny the action.

We got rid of the problem by overriding this method:
https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java#L222

In case it is a READ request, skip checking the rules. They get validated in the outgoingResponse call.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 13, 2016

Owner

Thanks. Can you please also provide:

  • What URL are you invoking the server with
  • What does the expected response contain
  • What does the actual response contain (resource type, resource ID, fields relevant to the compartment)
Owner

jamesagnew commented Dec 13, 2016

Thanks. Can you please also provide:

  • What URL are you invoking the server with
  • What does the expected response contain
  • What does the actual response contain (resource type, resource ID, fields relevant to the compartment)
@mattiuusitalo

This comment has been minimized.

Show comment
Hide comment
@mattiuusitalo

mattiuusitalo Dec 13, 2016

The request:

GET http://localhost:9082/kanta-phr-sandbox-oauth/baseDstu2/CarePlan/135154 HTTP/1.1

The expected response:

{
   "resourceType": "CarePlan",
   "id": "135154",
   "meta":    {
      "versionId": "268",
      "lastUpdated": "2016-11-17T07:49:11.805+02:00"
   },
   "identifier": [   {
      "use": "temp",
      "type": {"text": "Type text"},
      "value": "UHV8I7zN3jZ8h9oecEfBdw==",
      "period":       {
         "start": "2016-08-07T01:47:06.250Z",
         "end": "2016-08-07T01:47:06.250Z"
      },
      "assigner":       {
         "reference": "Organization/121156",
         "display": "Potilas ite"
      }
   }],
   "subject": {"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"},
   "status": "active",
   "context": {"display": "Tähän voisi laittaa Encounter referencen"},
   "period":    {
      "start": "2016-08-07T01:47:06.265Z",
      "end": "2016-08-07T01:47:06.265Z"
   },
   "author": [{"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"}],
   "modified": "2016-08-07T01:47:08.602Z",
   "category": [   {
      "id": "32322",
      "coding": [      {
         "id": "00922",
         "system": "http://loinc.org",
         "version": "00922",
         "code": "2.16.840.1.113883.3.88.12.80.62",
         "display": "waist circumference",
         "userSelected": true
      }],
      "text": "category text"
   }],
   "description": "careplan desc",
   "addresses": [{"reference": "Condition/121154"}],
   "support": [{"display": "Tähän myös reference resurssiin"}],
   "relatedPlan": [   {
      "code": "replaces",
      "plan":       {
         "reference": "CarePlan/122257",
         "display": "Tähän reference toiseen CarePlaniin"
      }
   }],
   "participant": [   {
      "role": {"text": "Role text"},
      "member":       {
         "reference": "Organization/121156",
         "display": "Tähän reference ORG, Patient, RelatedPerson tai Practioneriin"
      }
   }],
   "goal": [{"reference": "Goal/121155"}],
   "activity": [   {
      "actionResulting": [{"display": "actionResulting"}],
      "progress": [      {
         "time": "2016-08-07T01:47:06.314Z",
         "text": "Progress text"
      }],
      "detail":       {
         "category": {"text": "category FTqzimL6VGaBpXSrhlND+g=="},
         "code": {"text": "code jl1nnFTvxxzxsjuSzLrtcg=="},
         "reasonCode": [{"text": "reasonCode ODj51GwdlgQJUDkyqDsq1g=="}],
         "reasonReference": [{"display": "reasonReference dyh3XvcX4FRLhqJIu+mFpA=="}],
         "goal": [{"reference": "Goal/121155"}],
         "status": "on-hold",
         "statusReason": {"text": "statusReason l+1+knGytXTfehwaWyQesw=="},
         "prohibited": false,
         "location":          {
            "reference": "Location/121157",
            "display": "location fGeOnXwe8/Mc38eE3MXdRQ=="
         },
         "performer": [{"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"}],
         "dailyAmount":          {
            "value": 0.14753114774732656,
            "unit": "EqzIxE8XvKKGsHHjAFJpDw=="
         },
         "quantity":          {
            "value": 0.8384859726955787,
            "unit": "vs1y1wnU7dzX6FUfAcYWsQ=="
         },
         "description": "p/9qoIk9CahoYIebCJKVmA=="
      }
   }],
   "note":    {
      "time": "2016-08-07T01:47:06.356Z",
      "text": "o0jZZeG0/0vl19kOgWnn/A=="
   }
}

Actual response is the output from our aforementioned Deny all final default rule.

I suppose I should mention that while I have been talking about Observations before, the same happens with CarePlans. Of course we add a CarePlan rule as well as Observation rule.

mattiuusitalo commented Dec 13, 2016

The request:

GET http://localhost:9082/kanta-phr-sandbox-oauth/baseDstu2/CarePlan/135154 HTTP/1.1

The expected response:

{
   "resourceType": "CarePlan",
   "id": "135154",
   "meta":    {
      "versionId": "268",
      "lastUpdated": "2016-11-17T07:49:11.805+02:00"
   },
   "identifier": [   {
      "use": "temp",
      "type": {"text": "Type text"},
      "value": "UHV8I7zN3jZ8h9oecEfBdw==",
      "period":       {
         "start": "2016-08-07T01:47:06.250Z",
         "end": "2016-08-07T01:47:06.250Z"
      },
      "assigner":       {
         "reference": "Organization/121156",
         "display": "Potilas ite"
      }
   }],
   "subject": {"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"},
   "status": "active",
   "context": {"display": "Tähän voisi laittaa Encounter referencen"},
   "period":    {
      "start": "2016-08-07T01:47:06.265Z",
      "end": "2016-08-07T01:47:06.265Z"
   },
   "author": [{"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"}],
   "modified": "2016-08-07T01:47:08.602Z",
   "category": [   {
      "id": "32322",
      "coding": [      {
         "id": "00922",
         "system": "http://loinc.org",
         "version": "00922",
         "code": "2.16.840.1.113883.3.88.12.80.62",
         "display": "waist circumference",
         "userSelected": true
      }],
      "text": "category text"
   }],
   "description": "careplan desc",
   "addresses": [{"reference": "Condition/121154"}],
   "support": [{"display": "Tähän myös reference resurssiin"}],
   "relatedPlan": [   {
      "code": "replaces",
      "plan":       {
         "reference": "CarePlan/122257",
         "display": "Tähän reference toiseen CarePlaniin"
      }
   }],
   "participant": [   {
      "role": {"text": "Role text"},
      "member":       {
         "reference": "Organization/121156",
         "display": "Tähän reference ORG, Patient, RelatedPerson tai Practioneriin"
      }
   }],
   "goal": [{"reference": "Goal/121155"}],
   "activity": [   {
      "actionResulting": [{"display": "actionResulting"}],
      "progress": [      {
         "time": "2016-08-07T01:47:06.314Z",
         "text": "Progress text"
      }],
      "detail":       {
         "category": {"text": "category FTqzimL6VGaBpXSrhlND+g=="},
         "code": {"text": "code jl1nnFTvxxzxsjuSzLrtcg=="},
         "reasonCode": [{"text": "reasonCode ODj51GwdlgQJUDkyqDsq1g=="}],
         "reasonReference": [{"display": "reasonReference dyh3XvcX4FRLhqJIu+mFpA=="}],
         "goal": [{"reference": "Goal/121155"}],
         "status": "on-hold",
         "statusReason": {"text": "statusReason l+1+knGytXTfehwaWyQesw=="},
         "prohibited": false,
         "location":          {
            "reference": "Location/121157",
            "display": "location fGeOnXwe8/Mc38eE3MXdRQ=="
         },
         "performer": [{"reference": "Patient/845bd9f1-3635-4866-a6c8-1ca085df5c1a"}],
         "dailyAmount":          {
            "value": 0.14753114774732656,
            "unit": "EqzIxE8XvKKGsHHjAFJpDw=="
         },
         "quantity":          {
            "value": 0.8384859726955787,
            "unit": "vs1y1wnU7dzX6FUfAcYWsQ=="
         },
         "description": "p/9qoIk9CahoYIebCJKVmA=="
      }
   }],
   "note":    {
      "time": "2016-08-07T01:47:06.356Z",
      "text": "o0jZZeG0/0vl19kOgWnn/A=="
   }
}

Actual response is the output from our aforementioned Deny all final default rule.

I suppose I should mention that while I have been talking about Observations before, the same happens with CarePlans. Of course we add a CarePlan rule as well as Observation rule.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 13, 2016

Owner

Perfect, thanks! I'll see if I can get a unit test going which recreates this.

Owner

jamesagnew commented Dec 13, 2016

Perfect, thanks! I'll see if I can get a unit test going which recreates this.

@eevaturkka

This comment has been minimized.

Show comment
Hide comment
@eevaturkka

eevaturkka Dec 13, 2016

The value of patientId is attached to request in the bearer token, we get the value from the Oauth server and based on that response we build the rulelist - so it is not explicity as compartment in the read url.

The value of patientId is attached to request in the bearer token, we get the value from the Oauth server and based on that response we build the rulelist - so it is not explicity as compartment in the read url.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Dec 13, 2016

Owner

Ok, I think I have this one nailed. Basically the interceptor tries to be a bit clever by preventing the implementing method from ever being called if there is no chance the compartment rules could allow the call to proceed. It was being a bit overzealous.

I'm going to check in a fix now.

Thanks for reporting, and supplying all the details, that was just what was needed.

Owner

jamesagnew commented Dec 13, 2016

Ok, I think I have this one nailed. Basically the interceptor tries to be a bit clever by preventing the implementing method from ever being called if there is no chance the compartment rules could allow the call to proceed. It was being a bit overzealous.

I'm going to check in a fix now.

Thanks for reporting, and supplying all the details, that was just what was needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment