Skip to content

Bulk Data Authorization #6

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

Merged
merged 5 commits into from
Jan 12, 2023
Merged

Bulk Data Authorization #6

merged 5 commits into from
Jan 12, 2023

Conversation

radamson
Copy link
Contributor

Summary

Enables interaction with secured bulk data servers using SMART Backend Services.

Note: This PR is based off #2
Note: This PR also updates the bulk-client dependency to address smart-on-fhir/bulk-data-client#10

Motivation

Bulk Data Systems use SMART Backend Services for authorization and authentication. This PR enables the bulk-data-export-client to interact with these secured systems.

Paragraph (g)(10)(v)(B)

Authentication and authorization must occur during the process of granting an application access to patient data in accordance with the “SMART Backend Services: Authorization Guide”

New Behaviors

Users may provide a token URL, client ID, and location of a private key file to complete the authorization handshake and retrieve the bulk data resources requested.

Code Changes

  • New flags:
    • --token-url <tokenUrl> Bulk Token Authorization Endpoint
    • --client-id <clientId> Bulk Data Client ID
    • --private-key <url> File containing private key used to sign authentication tokens
  • Updates bulk-client dependency to a version which addresses Files Fail to Download smart-on-fhir/bulk-data-client#10
  • Adds a new function for extracting the private key from the file. (Providing the JWK as a string is not sufficient as it causes the underlying josev2.0.0 library to recognize the key as symmetric.

Testing Guidance

I tested against the secured inferno-reference server and the unsecured SMART BCH test server.

To test against the inferno reference server:

  • Extract one of the test signing keys into it's own file in this repo (e.g., inferno_es384_jwks.json)
  • Run the following:
npm run cli -- -f https://inferno.healthit.gov/reference-server/r4 -g 1a --token-url https://inferno.healthit.gov/reference-server/oauth/bulk-token --client-id eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6InJlZ2lzdHJhdGlvbi10b2tlbiJ9.eyJqd2tzX3VybCI6Imh0dHA6Ly8xMC4xNS4yNTIuNzMvaW5mZXJuby8ud2VsbC1rbm93bi9qd2tzLmpzb24iLCJhY2Nlc3NUb2tlbnNFeHBpcmVJbiI6MTUsImlhdCI6MTU5NzQxMzE5NX0.q4v4Msc74kN506KTZ0q_minyapJw0gwlT6M_uiL73S4 --private-key ./inferno_es384_jwks.json

To test against the SMART BCH test server to ensure there are no regressions for unsecured servers try:

npm run cli -- -f https://bulk-data.smarthealthit.org/eyJlcnIiOiIiLCJwYWdlIjoxMDAwMCwiZHVyIjoxMCwidGx0IjoxNSwibSI6MSwic3R1IjozLCJkZWwiOjB9/fhir -g 048d4683-703a-4311-963d-48e515a6372b

Copy link
Contributor

@sarahmcdougall sarahmcdougall left a comment

Choose a reason for hiding this comment

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

This looks great and works well. Tested with the SMART test server and the Inferno reference server. I have a few in-line comments but regardless this PR seems to be in a good state to merge.

package.json Outdated
@@ -51,11 +51,12 @@
"typescript": "^4.8.4"
},
"dependencies": {
"bulk-data-client": "git+https://[email protected]/smart-on-fhir/bulk-data-client",
"bulk-data-client": "git+https://[email protected]/bulk-dqm/bulk-data-client#fix-download",
Copy link
Contributor

Choose a reason for hiding this comment

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

The ndjson files are populated when I test with both the SMART test server and the inferno reference server, and based upon inspection of this branch the changes look good. Should we merge the changes from the fix-download branch into the main branch of our fork before merging this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it really matters either way, but I'm leaning towards leaving it in the branch for now and just opening a PR back into the upstream repo. I don't have a strong opinion on this though.

package.json Outdated
"colors": "^1.4.0",
"commander": "^9.4.1",
"fqm-execution": "^1.0.1",
"ts-node": "^10.9.1",
"ts-node-dev": "^2.0.0"
"ts-node-dev": "^2.0.0",
"jose": "^2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the SMART client uses node-jose instead of jose for auth (see package.json). Is there any reason that we should use node-jose over jose? I do not have prior experience with auth implementation and jose seems to work here, but wanted to bring it to your attention if you have a preference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. I definitely looked in the SMART client package.json, but I must have grabbed the wrong library. I'll swap them out so we don't have an additional unnecessary dependency.

Good catch!

@@ -39,8 +43,13 @@ const main = async () => {
},
};

if (program.opts().privateKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on adding handling in the CLI for the case where a token URL is provided but a private key is not? I tested this scenario with the inferno reference server. Looks like a 401 error gets thrown if no private key is provided. Let me know if you think the error returned is descriptive enough or if we should use the CLI to be more specific to the user as to why a 401 was returned:

> ts-node --files src/cli.ts "-f" "https://inferno.healthit.gov/reference-server/r4" "-g" "1a" "--token-url" "https://inferno.healthit.gov/reference-server/oauth/bulk-token" "--client-id" "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6InJlZ2lzdHJhdGlvbi10b2tlbiJ9.eyJqd2tzX3VybCI6Imh0dHA6Ly8xMC4xNS4yNTIuNzMvaW5mZXJuby8ud2VsbC1rbm93bi9qd2tzLmpzb24iLCJhY2Nlc3NUb2tlbnNFeHBpcmVJbiI6MTUsImlhdCI6MTU5NzQxMzE5NX0.q4v4Msc74kN506KTZ0q_minyapJw0gwlT6M_uiL73S4"

Kick-off started
Kick-off completed
/Users/smcdougall/onc/github/bulk-data-export-client/node_modules/got/dist/source/as-promise/index.js:118
                    request._beforeError(new types_1.HTTPError(response));
                                         ^
HTTPError: Response code 401 (Unauthorized)
    at Request.<anonymous> (/Users/smcdougall/onc/github/bulk-data-export-client/node_modules/got/dist/source/as-promise/index.js:118:42)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'ERR_NON_2XX_3XX_RESPONSE',
  timings: {
    start: 1673473772940,
    socket: 1673473772941,
    lookup: 1673473772941,
    connect: 1673473772967,
    secureConnect: 1673473773034,
    upload: 1673473773034,
    response: 1673473773065,
    end: 1673473773066,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 1,
      dns: 0,
      tcp: 26,
      tls: 67,
      request: 0,
      firstByte: 31,
      download: 1,
      total: 126
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice if the logs indicated which specific request received a 401 (e.g., was this on the initially discovery request that doesn't use the key at all?). If an incorrect key is used I'd expect to see a similar error. I think we can add that as a separate ticket though.

Initially, I was thinking it makes sense to allow the existing behavior because this is a prototype / testing tool and this could allow us to test unusual behavior, but testing against https://bulk-data.smarthealthit.org/?err=token_invalid_token I think the auth request is just skipped when the key isn't present.

I'll add a check that ensures that token URL, client ID, and private key are all provided if one is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error is now thrown unless all or none of the auth options are provided:

npm run cli -- -f https://inferno.healthit.gov/reference-server/r4 -g 1a --token-url https://inferno.healthit.gov/reference-server/oauth/bulk-token

> [email protected] cli
> ts-node --files src/cli.ts "-f" "https://inferno.healthit.gov/reference-server/r4" "-g" "1a" "--token-url" "https://inferno.healthit.gov/reference-server/oauth/bulk-token"

/Users/radamson/code/bulk-dqm/bulk-data-export-client/src/cli.ts:42
      throw new Error(
            ^
Error: Token URL, Client ID, and Private Key must all be provided or all omitted. Missing Client ID, Private Key
    at validateInputs (/Users/radamson/code/bulk-dqm/bulk-data-export-client/src/cli.ts:42:13)
    at main (/Users/radamson/code/bulk-dqm/bulk-data-export-client/src/cli.ts:61:3)
    at Object.<anonymous> (/Users/radamson/code/bulk-dqm/bulk-data-export-client/src/cli.ts:96:1)
    at Module._compile (node:internal/modules/cjs/loader:1099:14)
    at Module.m._compile (/Users/radamson/code/bulk-dqm/bulk-data-export-client/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Object.require.extensions.<computed> [as .ts] (/Users/radamson/code/bulk-dqm/bulk-data-export-client/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)

@sarahmcdougall sarahmcdougall merged commit f5a9a75 into main Jan 12, 2023
@sarahmcdougall sarahmcdougall deleted the auth branch January 12, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants