This repository was archived by the owner on Mar 6, 2026. It is now read-only.
Token lifecycle updates#50
Merged
tylercreller merged 1 commit intoproject-kessel:mainfrom Mar 6, 2026
Merged
Conversation
Reviewer's GuideRefactors the token client to use a reusable HTTP client with configurable timeout, introduces context-aware token retrieval with proper cancellation, and updates token caching to honor SSO-provided expiry times while adding comprehensive unit tests around token lifecycle behavior. Sequence diagram for GetTokenWithContext token retrieval and cachingsequenceDiagram
actor Caller
participant TokenClient
participant Cache as cache.Cache
participant HTTPClient as http.Client
participant SSO as AuthServer
Caller->>TokenClient: GetTokenWithContext(ctx)
TokenClient->>TokenClient: build cachedTokenKey
TokenClient->>Cache: Get(cachedTokenKey)
Cache-->>TokenClient: token, found?
alt token present and not expired
TokenClient-->>Caller: TokenResponse(AccessToken from cache)
else cache miss or expired
TokenClient->>TokenClient: build form data
TokenClient->>HTTPClient: Do(POST url, ctx)
HTTPClient->>SSO: POST /token
SSO-->>HTTPClient: 200 OK, JSON token
HTTPClient-->>TokenClient: http.Response
TokenClient->>TokenClient: json.Unmarshal(body, TokenResponse)
TokenClient->>TokenClient: cacheDuration = ExpiresIn - tokenExpiryMargin
TokenClient->>Cache: Set(cachedTokenKey, AccessToken, cacheDuration)
TokenClient-->>Caller: TokenResponse
end
Class diagram for updated token client and configclassDiagram
class Config {
+string clientId
+string clientSecret
+string authServerTokenUrl
+*tls.Config TlsConfig
+time.Duration Timeout
+time.Duration TokenHTTPTimeout
}
class TokenClient {
-string clientId
-string clientSecret
-string url
-bool EnableOIDCAuth
-bool Insecure
-cache.Cache cache
-http.Client httpClient
+TokenClient(config *Config)
+GetCachedToken(tokenKey string) (string, error)
+GetToken() (*TokenResponse, error)
+GetTokenWithContext(ctx context.Context) (*TokenResponse, error)
}
class TokenResponse {
+string AccessToken
+int ExpiresIn
}
class cache.Cache {
+Set(key string, value interface, duration time.Duration)
+Get(key string) (interface, bool)
}
class http.Client {
+time.Duration Timeout
+Do(req *http.Request) (*http.Response, error)
}
Config --> TokenClient : used to construct
TokenClient --> cache.Cache : uses
TokenClient --> http.Client : holds reusable
TokenClient --> TokenResponse : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When
tokenResponse.ExpiresInis zero or missing,cacheDurationbecomes 0 andgo-cachewill treat this asNoExpiration, meaning the token may be cached indefinitely even if the JWT has an expiry; consider explicitly handlingExpiresIn <= 0(e.g. by not caching, or falling back to a sane default duration). - The cache is now created with
NoExpirationas the default and per-item TTL derived fromexpires_in; it may be worth guarding against very largeexpires_invalues (e.g. hours/days) to avoid holding stale tokens too long if the server misconfigures or changes expiry semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When `tokenResponse.ExpiresIn` is zero or missing, `cacheDuration` becomes 0 and `go-cache` will treat this as `NoExpiration`, meaning the token may be cached indefinitely even if the JWT has an expiry; consider explicitly handling `ExpiresIn <= 0` (e.g. by not caching, or falling back to a sane default duration).
- The cache is now created with `NoExpiration` as the default and per-item TTL derived from `expires_in`; it may be worth guarding against very large `expires_in` values (e.g. hours/days) to avoid holding stale tokens too long if the server misconfigures or changes expiry semantics.
## Individual Comments
### Comment 1
<location path="common/token.go" line_range="155-158" />
<code_context>
}
- a.cache.Set(cachedTokenKey, tokenResponse.AccessToken, cacheCleanupInterval)
+
+ // Cache based on actual token lifetime from SSO, with a safety margin
+ // to ensure we refresh before expiry.
+ cacheDuration := time.Duration(tokenResponse.ExpiresIn) * time.Second
+ if cacheDuration > tokenExpiryMargin {
+ cacheDuration -= tokenExpiryMargin
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Handling of short-lived tokens can lead to them being cached indefinitely
When `ExpiresIn <= tokenExpiryMargin`, `cacheDuration` becomes zero or negative. go-cache treats any negative (non-zero) duration as "no expiration", so very short-lived tokens would be cached indefinitely, which is a behavior change from the previous fixed TTL and likely unintended.
Consider either:
- not caching when `ExpiresIn <= tokenExpiryMargin`, or
- clamping to a minimal positive TTL when `cacheDuration <= 0`, or
- using `cache.DefaultExpiration` in that case and configuring a sane default TTL instead of `NoExpiration`.
</issue_to_address>
### Comment 2
<location path="common/token_test.go" line_range="14-18" />
<code_context>
+ "github.com/golang-jwt/jwt/v5"
+)
+
+func createTestJWT(exp time.Time) string {
+ token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+ "exp": exp.Unix(),
+ })
+ signed, _ := token.SignedString([]byte("test-secret"))
+ return signed
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Handle potential error from JWT signing in tests instead of ignoring it
`createTestJWT` currently discards the error from `SignedString`, which could mask test setup failures if the signing method or key changes. Update this helper to either return `(string, error)` or accept a `*testing.T` and call `t.Fatalf` (marking it as `t.Helper()`), so any signing error fails the test clearly.
Suggested implementation:
```golang
func createTestJWT(t *testing.T, exp time.Time) string {
t.Helper()
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
"exp": exp.Unix(),
})
signed, err := token.SignedString([]byte("test-secret"))
if err != nil {
t.Fatalf("failed to sign test JWT: %v", err)
}
return signed
}
```
Update all call sites of `createTestJWT` in `common/token_test.go` (and any other test files if present) to pass a `*testing.T`:
- Change calls like `createTestJWT(exp)` to `createTestJWT(t, exp)`, where `t` is the test's `*testing.T`.
- If `createTestJWT` is used inside helper functions that don't currently receive `*testing.T`, update those helpers to accept `*testing.T` and thread it through to `createTestJWT`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tylercreller
force-pushed
the
token-optimizations
branch
2 times, most recently
from
c93692b to
441a90f
Compare
tylercreller
force-pushed
the
token-optimizations
branch
from
441a90f to
7be2021
Compare
akoserwal
approved these changes
Mar 6, 2026
akoserwal
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
http.ClientperTokenClientfor TLS connection poolingGetTokenWithContext()for caller controlled cancellation and timeoutsexpires_infrom SSO instead of hardcoded 5 minutesExpiresIn<= 30s are not cachedTokenHTTPTimeout(default 10s)GetToken()still backwards compatible, wrapsGetTokenWithContext()CI is not setup in this repo yet, pasting test runs