feat: github app refresh tokens

[giovanni-guidini]

Adding support for github app refresh tokens.
This is currently an opt-in feature for github apps (what we use to auth users).

closes codecov/engineering-team#163

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

[codecov]

Codecov Report

Merging #27 (dd65a8e) into main (5fc16f4) will increase coverage by 0.05%.
Report is 2 commits behind head on main.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   91.69%   91.75%   +0.05%     
==========================================
  Files          93       93              
  Lines        8107     8135      +28     
  Branches     1228     1234       +6     
==========================================
+ Hits         7434     7464      +30     
+ Misses        671      669       -2     
  Partials        2        2              

Flag	Coverage Δ
python3.10.5	88.69% <93.33%> (+0.07%)	⬆️
python3.7.13	88.53% <90.00%> (+0.05%)	⬆️
python3.8.13	88.76% <90.00%> (+0.05%)	⬆️
python3.9.12	89.11% <90.00%> (+0.05%)	⬆️
rust	89.75% <ø> (ø)
smart-labels	92.27% <96.66%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
shared/encryption/token.py	100.00% <ø> (ø)
shared/torngit/github.py	95.89% <96.66%> (+0.61%)	⬆️

[giovanni-guidini]

The one "issue" we have is that I don't know a good way to check if the token is expired or not. Gitlab had a specific way of telling us "this request failed because the token expired", but Github just say "Bad credentials". (I couldn't find in the docs reference to this, I just tested it with a revoked token)

So we try to refresh on all 401 errors :E

[scott-codecov]

This looks good to me. Like we chatted about earlier I'm just a little curious about the sequence of releasing this. Current tokens never expire and don't have a refresh token. What's the smoothest way to roll this out?

If the currently issued tokens still never expire (and only new logins have expiring tokens + refresh tokens) then it might make sense to not raise TorngitCantRefreshTokenError when missing a refresh_token. Might be worth testing out the transition from non-expiring token GitHub app -> expiring token GitHub app and see what happens w/ old tokens.

[giovanni-guidini]

I agree that we need to test the transition.

About

If the currently issued tokens still never expire (and only new logins have expiring tokens + refresh tokens) then it might make sense to not raise TorngitCantRefreshTokenError when missing a refresh_token.

Well we only raise that if we try to refresh the token. If it never expires it shouldn't 401 and therefore we won't ever try to refresh it :E

EDIT
As I was testing this a new situation appeared. If you go in the UI and try to see your code we make a request to get content. That fails with 404 instead of 401 😅 so I'll have to add that detail in (retry on 404 if content is in the URL). In light of 404 being more common than 401 I will not raise exception and just return the current token. It's an extra retry on a fail request, but maybe we can tolerate that?

[matt-codecov]

seconding scott's Q about rolling this out. do we want to phase out the non-expiring tokens? did we do anything like that for GitLab? looks like you addressed scott's feedback about raising TorngitCantRefreshTokenError, but there remain a couple dead references to it in the PR

if multiple api requests/worker tasks (particularly the latter) are spinning up at once, i'm worried they'll get stuck in a cycle of invalidating each others' tokens which will spawn a bunch of unnecessary API requests and hurt performance. i think that's worth solving, but maybe in a separate PR as it would involve changing the GitLab implementation as well. this PR is equivalent to the GitLab implementation and the tests are thorough so i'll approve

incidentally, i noticed that _on_token_refresh()/get_token_refresh_callback() is only populated in worker/services/repository.py and worker/services/repo_providers.py and is missing from worker/services/owner.py (and maybe other places). didn't check API, but that's a bug in worker at least