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.

Sign up for GitHub

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

Jump to bottom

Support authz policy enforcement in the bulkdata operation #2777

Closed
lmsurpre opened this issue Sep 17, 2021 · 2 comments
Closed

Support authz policy enforcement in the bulkdata operation #2777

lmsurpre opened this issue Sep 17, 2021 · 2 comments
Assignees
@lmsurpre
Labels
enhancement New feature or request
Milestone
Sprint 2021-14

Comments

@lmsurpre
Copy link
Member

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like

  1. Expand the FHIRPersistenceInterceptor interface to include beforeInvoke and afterInvoke methods
  2. Call these methods from FHIRRestHelper (or FHIRRestBundleHelper after robin's PR)
  3. Expand the fhir-smart module with support for enforcing the SMART scopes defined in the "SMART Backend Services" part of the bulkdata export spec.

Describe alternatives you've considered
Put the auth code right into fhir-operation-bulkdata.

Acceptance Criteria

  1. GIVEN [a precondition]
    AND [another precondition]
    WHEN [test step]
    AND [test step]
    THEN [verification step]
    AND [verification step]

Additional context
Add any other context or screenshots about the feature request here.
@prb112 prb112 added the enhancement New feature or request label Sep 17, 2021
@lmsurpre lmsurpre self-assigned this Sep 20, 2021
@lmsurpre lmsurpre added this to the Sprint 2021-13 milestone Sep 20, 2021
lmsurpre added a commit that referenced this issue Sep 20, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
b2bfd8e 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 20, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
061d82f 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 21, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
4f7f7fe 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 21, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
9f762f6 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 21, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
ab05dab 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 21, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
3559081 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 21, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
c824e60 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 22, 2021
@lmsurpre
issue #2777 - intercept the other bulkdata operations as well
4df7d28 
Previously, it was only enabled for export. Now, it will check for
system/[resourceType].write access for each resource type being
imported.

For bulkdata-status, we intercept the request just before the response
is returned and verify that the token has scopes that cover all the
various resource types in the output of the export job. For import
status, there's only OperationOutcome and so there's not much to check.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 18, 2021
@lmsurpre
issue #2777 - bulkdata authz enforcement in fhir-smart
543e9f5 
1. Introduce beforeInvoke and afterInvoke to the
FHIRPersistenceInterceptor interface. I wanted these to take the
FHIROperationContext which is in fhir-server, so that lead to:

2. Move the FHIRPersistenceInterceptor (and related classes) from
fhir-persistence to fhir-server (where they are called from).
Unfortunately, fhir-notification implements FHIRPersistenceInterceptor,
but fhir-server depends on fhir-notification. I tried breaking this
dependency but it was difficult because fhir-server currently
initializes the websocket notification impl differently than the kafka
and nats ones. So that lead to:

3. Move fhir-notification and all 3 implementations into fhir-server. At
first I didn't like this, but this was the easiest way out of the
dependency mess. If we want to break them back out, we should introduce
a proper notification spi. This should get revisited when we get to
#1289

4. Finally, I made the change I actually wanted to make, which is to add
support for `system/[resourceType].[read|write]` scopes in fhir-smart
and check these scopes before we allow requests to `$export`, `$import`,
and `$bulkdata-status`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 18, 2021
@lmsurpre
issue #2777 - intercept the other bulkdata operations as well
ff520bf 
Previously, it was only enabled for export. Now, it will check for
system/[resourceType].write access for each resource type being
imported.

For bulkdata-status, we intercept the request just before the response
is returned and verify that the token has scopes that cover all the
various resource types in the output of the export job. For import
status, there's only OperationOutcome and so there's not much to check.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 22, 2021
@lmsurpre
Merge pull request #2785 from IBM/issue-2777
66c52fa 
issue #2777 - bulkdata authz enforcement in fhir-smart
@lmsurpre
Copy link
Member Author

I am able to invoke a system export when I use a scope like system/*.read.
Unfortunately, I cannot do an export like [base]/$export?_type=Patient when I have a scope like system/Patient.read.

Additionally, I found that the list of scopes in the error message only includes system scopes which is confusing if the client has obtained a token with patient/ and/or user/ scopes.

lmsurpre added a commit that referenced this issue Oct 26, 2021
@lmsurpre
issue #2777 - fix parameter name and improve error message
c7b335d 
1. `_types` should have been `_type`
2. introduce new checkScopes variant that clarifies the scopes we're
echoing back are just the 'system/' ones

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 26, 2021
@lmsurpre
issue #2777 - fix parameter name and improve error message
5cff56b 
1. `_types` should have been `_type`
2. introduce new checkScopes variant that clarifies the scopes we're
echoing back are just the 'system/' ones

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 27, 2021
@lmsurpre
issue #2777 - allow access to Patient resource(s) if covered by user/…
b9ccc1f 
… or system/ scope

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre mentioned this issue Oct 27, 2021
Merged
lmsurpre added a commit that referenced this issue Oct 27, 2021
@lmsurpre
issue #2777 - allow access to Patient resource(s) if covered by user/…
69c3071 
… or system/ scope

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Oct 28, 2021
@lmsurpre
Merge pull request #2909 from IBM/issue-2777
373f6b3 
issue #2777 - allow access to Patient resource(s) if covered by user/ or system/ scope
@d0roppe
Copy link
Collaborator

d0roppe commented Oct 28, 2021

Tested with Keycloak env and verified that different scopes worked correctly with bulk data export and import

@d0roppe d0roppe closed this as completed Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmsurpre lmsurpre

Labels
enhancement New feature or request
Projects
None yet
Milestone
Sprint 2021-14
Development

No branches or pull requests
3 participants
@prb112 @lmsurpre @d0roppe