- Notifications
You must be signed in to change notification settings - Fork923
fix: always attempt external auth refresh when fetching#11762
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if !link.Expiry.IsZero() && link.Expiry.Before(dbtime.Now()) { | ||
| return false, nil, nil | ||
| } |
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.
Expired tokens should returninvalid
| @@ -2030,78 +2030,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ | |||
| return | |||
| } | |||
| if listen { | |||
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 hope my new flow is a bit easier to understand.
Basically there is 2 flows,listen and not listen. Iflisten is set to true, then the http handler will block until a valid auth token is found (this should probably be a websocket or something else idk).
Beforelisten andnot listen did not share any code, which is dumb because on a success, their code paths should be identical. So this refactor makes the two cases share the same code execution path when a valid auth token is found.
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 change makes sense to me overall. Just need to make the tests pass.
e998ccb to189c8aeCompare| if !valid { | ||
| // Set the previous token so the retry logic will skip validating the | ||
| // same token again. This should only be set if the token is invalid and there | ||
| // was no error. If it is invalid because of an error, then we should recheck. | ||
| previousToken = &externalAuthLink | ||
| handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{ | ||
| URL: redirectURL.String(), | ||
| }) |
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 to do this to prevent re-validating the same token after the refresh attempt
| // If expiresIn is 0, then the token never expires. | ||
| expires := dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second) | ||
| if body.ExpiresIn == 0 { | ||
| expires = time.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 was another bug I found. If zero time was passed in, the token was considered immediately expired on our end.
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.
That's probably a big one. IIRC GitHub often just gives you tokens with no expiry.
@johnstcn got it all passing |
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, some comments
coderd/workspaceagents_test.go Outdated
| // We expect only 2. One from the initial "Refresh" attempt, and | ||
| // another from the first tick. Ideally this would be just one. | ||
| // In a failed test, you will likely see 9, as the last one | ||
| // gets canceled. | ||
| require.Equal(t, 1, validateCalls, "validate calls duplicated on same token") |
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.
Comment does not agree with belowrequire.Equal -- do we only expect 1 or 2?
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 forgot to commit my comment diff 🤦.
It was two, then I decided to fix it and make it 1, since lower == better.
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.
Fixed the typo...
| // If expiresIn is 0, then the token never expires. | ||
| expires := dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second) | ||
| if body.ExpiresIn == 0 { | ||
| expires = time.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.
That's probably a big one. IIRC GitHub often just gives you tokens with no expiry.
They do in the regular oauth flow for sure. I would guess this would be a big issue, but if it was, then wouldn't it break spectacularly?? So maybe in the device flow method there is an expiry 🤔 |
What this does
Refactors the external auth code to:
ValidateTokentakes into accountExpiraryinformation.