Use AppBearerTokenAuth in login_as_app_installation #1009
Conversation
|
Starting third-party-check jobs. |
|
This took me a long time to track down (and it's no fault of github3, really). But just using authentication consistently throughout the library seems better (behavior is consistent in every method). I marked it as draft because I'm unsure if there might be any unwanted side-effects (I don't think so, but...). Many thanks for considering. |
|
Also unsure if the |
|
Build failed (third-party-check pipeline) integration testing on
Warning: |
src/github3/apps.py
Outdated
| @@ -171,26 +171,3 @@ def create_token(private_key_pem, app_id, expire_in=TEN_MINUTES_AS_SECONDS): | |||
| ) | |||
| token.make_signed_token(key) | |||
| return token.serialize() | |||
|
|
|||
|
|
|||
| def create_jwt_headers( | |||
There was a problem hiding this comment.
This is a publicly named function in a publicly named module. We can't just get rid of it
|
This looks generally fine. The question I have is why on earth is GitHub sending you a redirect here? |
50573f1 to
898a9d7
Compare
|
Starting third-party-check jobs. |
|
Oops, good point about the public function. I've reverted that part and pushed a new version of the commit. Re: redirects, I poked it a bit further, and it turns out it wasn't hitting the redirect path, but this other path: (At least, if I comment out that block, an unmodified github3.py works for me.) I've marked the PR as ready for review, but the commit description could still be amended to reflect this last fact. I could do that if you'd like. |
|
Build failed (third-party-check pipeline) integration testing on
Warning: |
Please do. That would make it clearer for future folks what is happening. Otherwise, this looks great. Thanks! |
Previously, this method used an Authorization header via `headers` parameter, instead of an AuthBase instance via the `auth` parameter. But the requests library has this behavior where it will try to pick up authentication credentials from ~/.netrc if neither session nor request has an associated AuthBase object. (A sole Authorization header will not prevent this behavior.) Since all of github3.py uses AuthBase instances to authenticate, but login_as_app_installation didn't, this makes for an incredibly difficult issue to debug, since the latter is the only place in which the issue can manifest, if a user's ~/.netrc has an entry for api.github.com. By using AppBearerTokenAuth, this can no longer happen, since the ~/.netrc path is not hit any more.
898a9d7 to
655c8e1
Compare
|
Starting third-party-check jobs. |
Done. (I see you tend to merge PRs, and not squash, so I didn't update the PR description in the web UI.) Thanks! |
|
Build failed (third-party-check pipeline) integration testing on
Warning: |
|
Closing and re-opening to try to retrigger GHA |
|
Starting third-party-check jobs. |
|
Build failed (third-party-check pipeline) integration testing on
Warning: |
|
Updated our github actions to build on pull requests too which they weren't before. Let me close/reopen one last time. |
|
Starting third-party-check jobs. |
|
Build failed (third-party-check pipeline) integration testing on
Warning: |
|
Thank you @dato ! |
Previously, this method used an Authorization header via
headersparameter, instead of an AuthBase instance via
authparameter.But the requests library has this behavior where it will strip the
Authorization header on redirects, and then pick up fresh credentials
from other sources (~/.netrc in particular, if present).
Any AuthBase object, though, is still attached to the request, so this
makes for an incredibly difficult issue to debug: all of github3.py
uses AuthBase to authenticate, except login_as_app_installation. The
latter thus being the only place in which the issue can manifest, when
~/.netrc has an entry for api.github.com. By using AppBearerTokenAuth,
this can no longer happen.