-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: Support Bearer Token Authentication #165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is generally fine as a workaround, but I would like us to consider putting in more design work around this, especially if we foresee adding more authentication types (e.g. OAuth) in the future.
ba23435
to
79b1e4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a discussion where it was decided that its fine to overload the token field for now.
The main decision point here is that we do not have upcoming authentication types in the horizon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for adding in the validations
I'll take a final round of review on this when you get a chance, @yangchoo. I'm getting some lint errors on Travis, but I did not change these files and I have latest master merged into my branch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the core logic seems straightforward enough (adding to the authorization headers).
Minor nits on some extraneous keys. Additional clarification/messaging around specification of multiple tokens would be good as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing nits. New behavior for handling token/bearer-token case looks good to me.
Please rebase w/ master to address the CI-lint errors.
Hmm, after installing the extra dependencies (e.g. pip install '.[docs]'`), the pre-commit passes locally without issue:
|
Can confirm that I'm unable to reproduce this issue locally. After doing some further digging, I suspect it may be related to pylint concurrency. Based on pylint-dev/pylint#3611 I pushed a commit to limit the concurrency to 1 for now. Let's see if this works. There's a big backlog on travis right now, so things are slower than usual on that front. |
Seems like the concurrency limitations didn't fix the issue. For now, adding an explicit ignore so that it doesn't block this diff. Something to continue to look into |
Ah, that's too bad. Can you approve once again so I can merge? |
78c5f9e
to
887f3cc
Compare
887f3cc
to
7226275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formal approval. Lint error seems to have been fixed after reverting accidental comment change.
We will soon be passing a client_credentials (service-to-service) bearer token to programs when executing in the context of Igor. Web will validate the bearer token (and only the bearer token, no user token needed) when receiving an API request.
Programs internally call the transcriptic library (this repo) to form an API
Connection
.The change in this PR involves changing the Connection instantiation routine to recognize if the token is a
Bearer
token and, if so, set the standard OAuthAuthorization
header (and noX-User-Token
header). This is opposed to a non-bearer token, which will result in the usualX-User-Token
header.Jira Issue: SEA-271