onc-healthit / inferno-program

Archived source code for the Inferno Testing Tool and the Program Edition set of tests. No longer maintained.
https://inferno.healthit.gov/
Apache License 2.0
38 stars 12 forks source link

Bulk Testing Client Does Not Follow Redirects (FI-1371) #398

Closed bforbis closed 2 years ago

bforbis commented 2 years ago

I am trying to pass the multi-patient test sequences for BDGV-06 through BDGV-23. Initially our service was using requiresAccessToken=false and using signed URLs for data access, however we recently got instructions from ONC that this is not allowed per ONC rulings (see #303 ).

Our next approach for implementing bulk data access validations of our file servers was to:

In practice, we expect that most HTTP clients will be able to follow these 301 redirects, however the inferno client does not.

Would Inferno be willing to make a change to have the client follow redirects?

yunwwang commented 2 years ago

This is interesting. What is your interpretation on Bulk Data Export Section [5.4] (http://hl7.org/fhir/uv/bulkdata/STU1.0.1/export/index.html#file-request)?

bforbis commented 2 years ago

The spec does not classify whether 3xx are considered an error or a success. However, in my experience many http clients will happily follow redirects and it is not unreasonable to assume clients can handle this. Would this need to be raised to HL7 to decide on?

yunwwang commented 2 years ago

We had a brief discussion at zulip. One question raised is about security headers. Does your S3 bucket require additional headers?

bforbis commented 2 years ago

No, just the signed url is required.

On Sat, Nov 20, 2021, 2:26 PM yunwwang @.***> wrote:

We had a brief discussion at zulip https://chat.fhir.org/#narrow/stream/179250-bulk-data/topic/301.20redirect. One question raised is about security headers. Does your S3 bucket require additional headers?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/onc-healthit/inferno-program/issues/398#issuecomment-974699732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEBVCCTVW6J67NA5CF7BKTTUM7Y6JANCNFSM5H5KQYQA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

bforbis commented 2 years ago

FYI @yunwwang, when dealing with Amazon S3 in particular, there is a requirement to use only one Authorization mechanism. This means that on handling of the 30[1|2|7] redirect, the client must remove the Authorization header.

More details on that requirement here: https://stackoverflow.com/questions/31514336/only-one-auth-mechanism-allowed-only-the-x-amz-algorithm-query-parameter

yunwwang commented 2 years ago

Thank you for the latest comment. Looks like our current redirect solution has a side effect. The team will investigate this issue again.

bforbis commented 2 years ago

Hi @yunwwang, what is the status with this ticket? Are we going to proceed with this fix, or do you think we may revisit the conversation with allowing for requiresAccessToken=false so that we can return signed URLs directly in the status endpoint?

yunwwang commented 2 years ago

@bforbis We are revisiting the requiresAccessToken. You can follow the discussion on zulip https://chat.fhir.org/#narrow/stream/179250-bulk-data/topic/301.20redirect

bforbis commented 2 years ago

@yunwwang, last conversation from Zulip was that requiresAccessToken=false would be supported by inferno, though that seems to more be in line with issue #303. Will that ticket be re-opened?

yunwwang commented 2 years ago

This issue is fixed in ONC Certification(g)(10) Test Kit v2.0.0. PR: https://github.com/onc-healthit/onc-certification-g10-test-kit/pull/16