-
Notifications
You must be signed in to change notification settings - Fork 0
Use direct device flow authentication. #67
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
Conversation
|
You can test against the live Packit server with the following: from pyorderly.outpack.location import *
loc = outpack_location_packit("https://reside-dev.packit-dev.dide.ic.ac.uk/")
print(loc.list())See mrc-ide/orderly#218 for the R counterpart. |
| @@ -0,0 +1,136 @@ | |||
| import time | |||
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.
This is almost identical to the existing version. Just moving it out because it is a very generic and standalone bit of code compared to the rest of location_packit.py.
The only difference is that I removed the scope parameter from authenticate() since we don't need it anymore.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
=======================================
Coverage 99.23% 99.23%
=======================================
Files 55 57 +2
Lines 4307 4326 +19
Branches 285 285
=======================================
+ Hits 4274 4293 +19
Misses 18 18
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EmmaLRussell
left a comment
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.
Works for me! Couple of tiny suggestions.
| return {"Authorization": f"Bearer {token}"} | ||
| if token is not None: | ||
| if re.match("^gh._", token): | ||
| msg = "Using a GitHub token to login to Packit isn't supported anymore." |
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.
Might be nice to include the additional advice which is in the R: "Either use a Packit token or omit the token to use interactive authentication.."
| else: | ||
| interval = parameters.interval |
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.
Don't need this as already set interval on line 115?
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.
Good catch. I've removed the line 115 assignment.
We replaced GitHub authentication in Packit with a native device flow authentication. Instead of authenticating against GitHub and then trading a GitHub PAT for a Packit token, pyorderly can now authenticate directly against Packit. This simplifies the implementation and allows us to login to Packit instances that do not use GitHub authentication (such as Montagu). This flow has been in use in the R orderly package for a while now and is working quite well.
We replaced GitHub authentication in Packit with a native device flow authentication. Instead of authenticating against GitHub and then trading a GitHub PAT for a Packit token, pyorderly can now authenticate directly against Packit.
This simplifies the implementation and allows us to login to Packit instances that do not use GitHub authentication (such as Montagu).
This flow has been in use in the R orderly package for a while now and is working quite well.