Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

ThomasK33
Copy link
Member

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:

  • AddedRefreshDefaultDuration toSessionLifetime struct to control refresh token expiry
  • Modified token generation to use this new setting when creating refresh tokens
  • Added a test to verify refresh tokens outlive access tokens
  • Set default refresh token lifetime to 30 days

The 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.

@ThomasK33Graphite App
Copy link
MemberAuthor

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch 2 times, most recently fromc43e727 to459f676CompareSeptember 10, 2025 19:50
@ThomasK33ThomasK33 marked this pull request as ready for reviewSeptember 11, 2025 08:42
Comment on lines +570 to +572
// 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"`
Copy link
Member

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?

ThomasK33 reacted with thumbs up emoji
Copy link
MemberAuthor

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.

@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch 2 times, most recently from932dbe8 to0baaed6CompareSeptember 11, 2025 10:58
}

// Validate checks cross-field constraints for deployment values.
// It should be called after all values are loaded from flags/env/YAML.
Copy link
Member

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?

ThomasK33 reacted with thumbs up emoji
Copy link
MemberAuthor

@ThomasK33ThomasK33Sep 11, 2025
edited
Loading

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")      }

@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch from0baaed6 to720a83dCompareSeptember 11, 2025 11:13
@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch from720a83d toceb24deCompareSeptember 12, 2025 07:35
Copy link
Member

@johnstcnjohnstcn left a 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.


// Check if values appear uninitialized
ifaccess==0 {
returnxerrors.New("sessions configuration appears uninitialized - ensure all values are loaded before validation")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
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)
})
Copy link
Member

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

@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch fromceb24de to0fb122cCompareSeptember 12, 2025 11:47
Change-Id: I988093e8fc7328a09d2a0b2c5d476bad75e064c8Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33ThomasK33force-pushed thethomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branch from0fb122c to805bc4dCompareSeptember 12, 2025 12:12
@ThomasK33ThomasK33 merged commit088d149 intomainSep 13, 2025
37 checks passed
@ThomasK33ThomasK33 deleted the thomask33/09-10-feat_oauth2_add_configurable_refresh_token_lifetime branchSeptember 13, 2025 06:57
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsSep 13, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@code-ashercode-ashercode-asher approved these changes

@EmyrkEmyrkAwaiting requested review from Emyrk

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ThomasK33@johnstcn@code-asher

[8]ページ先頭

©2009-2025 Movatter.jp