fix: always attempt external auth refresh when fetching #11762
Conversation
| if !link.Expiry.IsZero() && link.Expiry.Before(dbtime.Now()) { | ||
| return false, nil, nil | ||
| } |
There was a problem hiding this comment.
Expired tokens should return invalid
| @@ -2030,78 +2030,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ | |||
| return | |||
| } | |||
|
|
|||
| if listen { | |||
There was a problem hiding this comment.
I hope my new flow is a bit easier to understand.
Basically there is 2 flows, listen and not listen. If listen 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).
Before listen and not 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.
e998ccb to
189c8ae
Compare
| 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.
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.
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.
That's probably a big one. IIRC GitHub often just gives you tokens with no expiry.
|
@johnstcn got it all passing |
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.
Comment does not agree with below require.Equal -- do we only expect 1 or 2?
There was a problem hiding this comment.
I forgot to commit my comment diff 🤦.
It was two, then I decided to fix it and make it 1, since lower == better.
| // 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.
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.