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

issue #2777 - bulkdata authz enforcement in fhir-smart #2785

Merged
merged 9 commits into from
Oct 22, 2021
Merged

issue #2777 - bulkdata authz enforcement in fhir-smart #2785

merged 9 commits into from
Oct 22, 2021

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Sep 20, 2021 •
edited
Loading

  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 (and all its implementations).
    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
    Support for FHIR Subscription mechanism #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 double-check the scopes before we return the download urls from $bulkdata-status.

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

@lmsurpre lmsurpre force-pushed the issue-2777 branch 5 times, most recently from 3559081 to c824e60 Compare September 21, 2021 16:44
@lmsurpre lmsurpre marked this pull request as ready for review October 18, 2021 20:30
@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
Guard MyInterceptor methods from interactions from other tests
8aef9e0 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@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 lmsurpre force-pushed the issue-2777 branch 2 times, most recently from e34f78b to f8e9156 Compare October 19, 2021 01:45
@lmsurpre
post-merge updates
adcd131 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
prb112
prb112 reviewed Oct 19, 2021
View reviewed changes
fhir-server/src/main/java/com/ibm/fhir/server/interceptor/FHIRPersistenceInterceptorMgr.java Outdated
*
* To register an interceptor implementation, develop a class that implements the FHIRPersistenceInterceptor interface,
* and then insert your implementation class name into a file called
* META-INF/services/com.ibm.fhir.persistence.FHIRPersistenceInterceptor and store that file in your jar.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the interceptor itself going to remain the same canonical name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. I moved it to the com.ibm.fhir.server.interceptor package and so I'll update this doc to reflect that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@prb112
Copy link
Contributor

prb112 commented Oct 19, 2021

Q: Modularity, this code seems to move all the code into the fhir-server. The modularity of the project is really blurring.
Have you considered moving it to a separate intermediate package?

Q: Re 4, do you need the operationName for downstream decisions?

@lmsurpre
Copy link
Member Author

Q: Modularity, this code seems to move all the code into the fhir-server. The modularity of the project is really blurring.
Have you considered moving it to a separate intermediate package?

I tried to address this in point number 2 of the pull request description.
I originally attempted to break the dependency that we have from fhir-server to fhir-notification and its specific implementations, but this proved to be quite difficult/ugly because our FHIRServletContextListener is doing special work to initialize the websocket notification impl.

Q: Re 4, do you need the operationName for downstream decisions?

I need the operation code in the beforeInvoke and afterInvoke methods. I was surprised that it wasn't already in the FHIROperationContext and so I added it. It could have been "yet another property in the map" but I thought it was first-class enough to warrant its own member.

@lmsurpre
fix javadoc for new FHIRPersistenceInterceptorMgr
c9a3784 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Introduce fhir-server-spi
4e62a94 
Currently, this module houses the SPI-related classes for:
* Interceptors
* Custom Operations

As part of that effort, I moved the Interaction enum from FHIRRestHelper
to its interface, FHIRResourceHelpers.

I updated fhir-smart to depend on fhir-server-spi instead of
fhir-server.

I also updated all fhir-operation modules to depend on a
'provided'-scope fhir-server-spi instead of fhir-server, with the
following exceptions:
1. fhir-operation-erase because that one still depends on
RestAuditLogger which I decided not to move into the spi module
2. fhir-operation-term-cache because that depends on
ServerRegistryResourceProvider (to clear its internal cache)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
update META-INF/services names to match the new spi packages
cae989f 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
michaelwschroeder
michaelwschroeder approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@michaelwschroeder michaelwschroeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - a few minor comments

fhir-server/src/main/java/com/ibm/fhir/server/operation/spi/FHIROperationContext.java Outdated Show resolved Hide resolved
fhir-server/src/main/java/com/ibm/fhir/server/operation/spi/FHIRRestOperationResponse.java Outdated Show resolved Hide resolved
fhir-server-spi/src/main/java/com/ibm/fhir/server/spi/operation/FHIROperationContext.java Outdated Show resolved Hide resolved
fhir-server-spi/src/main/java/com/ibm/fhir/server/spi/operation/FHIRRestOperationResponse.java Outdated Show resolved Hide resolved
fhir-server/src/main/java/com/ibm/fhir/server/resources/Operation.java Show resolved Hide resolved
fhir-server/src/main/java/com/ibm/fhir/server/rest/FHIRRestInteractionInvoke.java Show resolved Hide resolved
@lmsurpre
Updates per PR review
a8d616f 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre merged commit 66c52fa into main Oct 22, 2021
@lmsurpre lmsurpre deleted the issue-2777 branch October 22, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwschroeder michaelwschroeder michaelwschroeder approved these changes

@prb112 prb112 prb112 left review comments

@punktilious punktilious Awaiting requested review from punktilious

@tbieste tbieste Awaiting requested review from tbieste

Assignees
No one assigned
Labels
deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lmsurpre @prb112 @michaelwschroeder