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.

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

allow optional headers on access_url (resolves issue #239) #248

Merged
merged 7 commits into from
Apr 19, 2019

Conversation

dglazer
Copy link
Member

@dglazer dglazer commented Apr 3, 2019

This PR resolves issues #239. Changes:

  1. introduce a new AccessURL object that contains a url string and an optional headers array
  2. replace the access_url string with the new object, in AccessMethod and in the /access method
  3. small documentation cleanup

Staged documentation: https://ga4gh.github.io/data-repository-service-schemas/preview/feature/issue-239-access-credentials/docs/

@dglazer dglazer requested review from briandoconnor and sarpera April 3, 2019 02:28
@dglazer dglazer self-assigned this Apr 3, 2019
@dglazer
Copy link
Member Author

dglazer commented Apr 4, 2019

@kemp-google @philloooo -- fyi -- this PR allows DRS servers to optionally return auth headers along with access URLs. It's meant to finish the plan discussed in #239 -- comments welcome

@briandoconnor
Copy link
Contributor

Copy link
Contributor

@briandoconnor briandoconnor left a comment

Choose a reason for hiding this comment

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

I left one comment that basically gave ideas for future PRs but let's start with this if our Drivers are happy

items:
type: string
description: >-
An optional list of headers to include in the HTTP request to `url`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a great first step. It improves what we have right now and we should go for it. As an implementer/user of DRS I'd like to know 1) what are the possible headers here, do we define them in the /service-info end point for example or in the spec as a list of acceptable ones? And 2) it would be great to see examples of how these work for each of the types of access methods DRS 1.0 would support (in the docs for example). Totally fine to address in a future PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Re (1): As a user, you shouldn't have to know what headers are possible; you just use the ones you're given. As an implementor, you can return any headers you want. The most likely use case is to return an Authorization header, as is shown in the example in the YAML.

Re (2): this PR already includes beefed-up documentation on access methods (in front_matter.adoc); happy to add more in future PRs

@briandoconnor
Copy link
Contributor

See what the Drivers think after @sarpera takes a look and gives thumbs up. Does it work for others using things like aspera or globus

Copy link

@jorizci jorizci left a comment

Choose a reason for hiding this comment

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

@dglazer although I see the point as a general pupose, I don't see it working well in a scenario where the content server requires a token generated for the user that can expire. Are we expecting that in those cases the final client will do the whole process of fetching, et al whenever we need to resume or restart?

@briandoconnor
Copy link
Contributor

briandoconnor commented Apr 15, 2019

Notes from call: @jorizci mentioned expiration of tokens... will work well for static system but not sure how this will work systems that expire their tokens quickly.

Does this work for EGA/ENA/EVA?

  • now? doesn't work right now
  • expand in the future to better accommodate? Can we do this during the F2F?

@dglazer
Copy link
Member Author

dglazer commented Apr 15, 2019

Summarizing conversation with @jorizci on the workstream call -- this PR is not sufficient to fully meet EGA's use case, but he's a "-0", not a "-1". From his perspective, it's okay to merge, and we should discuss further enhancements in Hinxton.

@junjun-zhang
Copy link

+1 from ICGC-ARGO

@dglazer dglazer merged commit d58d3ea into develop Apr 19, 2019
@dglazer dglazer deleted the feature/issue-239-access-credentials branch April 21, 2019 14:53
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.

6 participants