- Notifications
You must be signed in to change notification settings - Fork1k
feat: ensure OAuth2 refresh tokens outlive access tokens#19769
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
feat: ensure OAuth2 refresh tokens outlive access tokens#19769
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c43e727
to459f676
Compare// This should generally be longer than access token lifetimes to allow | ||
// refreshing after access token expiry. | ||
RefreshDefaultDuration serpent.Duration`json:"refresh_default_duration,omitempty" typescript:",notnull"` |
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.
In what case would it make sense to set the default refresh duration to be the same as the access token lifetime? In that case you may as well just disable refresh tokens entirely. If we allow setting this equal to or very close to access token lifetime, I foresee customers running into issues. Would it make sense to validate thatRefreshDefaultDuration
is strictly greater than access token lifetime, and raise an error if this is not the case?
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.
Yeah, that makes sense. I've added a validation to catch those cases.
It errors out if the constraint of refresh token lifetime < access token lifetime, since debugging why refresh tokens aren't issued would become even harder if we fail silently.
932dbe8
to0baaed6
Comparecodersdk/deployment.go Outdated
} | ||
// Validate checks cross-field constraints for deployment values. | ||
// It should be called after all values are loaded from flags/env/YAML. |
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.
s/should/must
It would also be nice to know if there were a way to catch if this were not the case, and error if so.
For now, I suppose we could just check for a zero-valuedDeploymentValues
and complain?
ThomasK33Sep 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 closest I can think of is checking if a session's default duration is not equal to 0 (the default)?
Since a session duration of 0 doesn't make sense conceptually.
So prepending something like
// Check if values appear uninitializedifaccess==0 {return xerrors.New("sessions configurationappearsuninitialized-ensureallvaluesareloadedbeforevalidation") }
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
0baaed6
to720a83d
Compare720a83d
toceb24de
CompareThere 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 below. Recommend a second approval as well as my OAuth knowledge is rusty.
codersdk/deployment.go Outdated
// Check if values appear uninitialized | ||
ifaccess==0 { | ||
returnxerrors.New("sessions configuration appears uninitialized - ensure all values are loaded before validation") |
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.
returnxerrors.New("sessions configuration appears uninitialized - ensure all values are loaded before validation") | |
returnxerrors.New("developer error:sessions configuration appears uninitialized - ensure all values are loaded before validation") |
dv:=mk(1*time.Hour,48*time.Hour) | ||
err:=dv.Validate() | ||
require.NoError(t,err) | ||
}) |
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 forget to also add a test to hit the code path you added to check for uninitialized default duration
ceb24de
to0fb122c
CompareChange-Id: I988093e8fc7328a09d2a0b2c5d476bad75e064c8Signed-off-by: Thomas Kosiewski <tk@coder.com>
0fb122c
to805bc4d
Compare088d149
intomainUh oh!
There was an error while loading.Please reload this page.
Implement OAuth2 Refresh Token Expiry Configuration
This PR adds a dedicated configuration option for OAuth2 refresh token lifetimes, allowing refresh tokens to outlive their associated access tokens. This is important for OAuth2 flows where refresh tokens should remain valid after access tokens expire.
Key changes:
RefreshDefaultDuration
toSessionLifetime
struct to control refresh token expiryThe PR ensures that refresh tokens can have a longer lifetime than access tokens, which is a standard OAuth2 pattern that allows clients to obtain new access tokens without requiring user re-authentication.