feat: add session max lifetime and fix refresh logic#559
Conversation
…for non-envoy proxies
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughSession maximum lifetime tracking and enforcement are added: sessions now record Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthService
participant Repository
participant Database
rect rgba(200,220,255,0.18)
note over AuthService,Repository: Session Creation flow (new)
Client->>AuthService: CreateSession(credentials)
AuthService->>Repository: CreateSession(params + CreatedAt)
Repository->>Database: INSERT INTO sessions(..., created_at, ...)
Database-->>Repository: INSERT result (including created_at)
Repository-->>AuthService: Session{CreatedAt, Expiry, ...}
AuthService-->>Client: Set-Cookie (expiry)
end
rect rgba(220,255,200,0.18)
note over Client,AuthService: Refresh with dynamic threshold
Client->>AuthService: RefreshSession(cookie)
AuthService->>Repository: GetSession(sessionID)
Repository->>Database: SELECT ..., created_at FROM sessions WHERE id=...
Database-->>Repository: Session{CreatedAt, Expiry, ...}
Repository-->>AuthService: Session{CreatedAt, Expiry, ...}
AuthService->>AuthService: calculate refreshThreshold = f(SessionExpiry)
AuthService->>AuthService: newExpiry = currentExpiry + refreshThreshold
AuthService-->>Client: Updated Set-Cookie (newExpiry)
end
rect rgba(255,230,200,0.18)
note over Client,AuthService: Max-lifetime enforcement on access
Client->>AuthService: GetSessionCookie(cookie)
AuthService->>Repository: GetSession(sessionID)
Repository->>Database: SELECT created_at, expiry, ...
Database-->>Repository: Session{CreatedAt, ...}
Repository-->>AuthService: Session{CreatedAt, ...}
AuthService->>AuthService: if SessionMaxLifetime != 0 && CreatedAt + SessionMaxLifetime < now
alt max lifetime exceeded
AuthService->>Repository: DeleteSession(sessionID)
Repository->>Database: DELETE FROM sessions WHERE id=...
AuthService-->>Client: Error (session expired)
else valid
AuthService-->>Client: SessionCookie (valid)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #559 +/- ##
==========================================
- Coverage 19.44% 19.25% -0.19%
==========================================
Files 39 39
Lines 2273 2295 +22
==========================================
Hits 442 442
- Misses 1803 1825 +22
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/service/auth_service.go (1)
207-219: LGTM!The
CreatedAttimestamp is correctly captured at session creation time using Unix epoch seconds, consistent with theExpiryfield format.Optional nit: Consider capturing
time.Now()once to ensureExpiryandCreatedAtuse the exact same instant:🔎 Suggested improvement
+ now := time.Now() + session := repository.CreateSessionParams{ UUID: uuid.String(), Username: data.Username, Email: data.Email, Name: data.Name, Provider: data.Provider, TotpPending: data.TotpPending, OAuthGroups: data.OAuthGroups, - Expiry: time.Now().Add(time.Duration(expiry) * time.Second).Unix(), - CreatedAt: time.Now().Unix(), + Expiry: now.Add(time.Duration(expiry) * time.Second).Unix(), + CreatedAt: now.Unix(), OAuthName: data.OAuthName, OAuthSub: data.OAuthSub, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.env.examplecmd/tinyauth/tinyauth.goconfig.example.yamlinternal/assets/migrations/000004_created_at.down.sqlinternal/assets/migrations/000004_created_at.up.sqlinternal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/repository/models.gointernal/repository/query.sql.gointernal/service/auth_service.goquery.sqlschema.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:02:49.689Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 326
File: internal/service/database_service.go:7-11
Timestamp: 2025-08-27T11:02:49.689Z
Learning: In internal/service/database_service.go, using glebarez/sqlite (modernc) for GORM with sqlite3 (mattn) driver for golang-migrate is a valid pattern that works correctly, despite appearing to be a driver mismatch. The golang-migrate sqlite driver causes panics when used with modernc.org/sqlite backend, so the mixed approach should be maintained.
Applied to files:
internal/bootstrap/db_bootstrap.go
🧬 Code graph analysis (3)
internal/bootstrap/service_bootstrap.go (1)
internal/config/config.go (1)
SessionCookieName(11-11)
internal/controller/user_controller_test.go (1)
internal/config/config.go (1)
SessionCookieName(11-11)
internal/controller/proxy_controller_test.go (1)
internal/config/config.go (1)
SessionCookieName(11-11)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 40-40: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 40-40: [UnorderedKey] The TINYAUTH_AUTH_SESSIONEXPIRY key should go before the TINYAUTH_AUTH_USERS key
(UnorderedKey)
[warning] 42-42: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 42-42: [UnorderedKey] The TINYAUTH_AUTH_SESSIONMAXLIFETIME key should go before the TINYAUTH_AUTH_USERS key
(UnorderedKey)
🔇 Additional comments (20)
config.example.yaml (1)
41-42: LGTM! Clear configuration addition.The new
sessionMaxLifetimeparameter is well-documented with a sensible default of 0 (unlimited), ensuring backward compatibility while enabling hard session lifetime limits when configured..env.example (1)
41-42: Good addition of the session max lifetime configuration.The new
TINYAUTH_AUTH_SESSIONMAXLIFETIMEenvironment variable aligns well with the auth configuration namespace.Note: The static analysis warnings about quote characters and key ordering are false positives—quotes are standard practice in
.envfiles, and alphabetical ordering is optional.internal/assets/migrations/000004_created_at.down.sql (1)
1-1: LGTM! Rollback migration is correct.The down migration properly reverses the addition of the
created_atcolumn.query.sql (2)
27-39: Correctly preservescreated_atas immutable.The
UpdateSessionquery appropriately excludescreated_atfrom the SET clause, ensuring the session creation timestamp remains unchanged throughout the session's lifetime. This is the correct behavior for tracking session age.
1-17: Code correctly setscreated_atto current timestamp.The application properly sets
CreatedAttotime.Now().Unix()in theCreateSessionCookiemethod withininternal/service/auth_service.gowhen populating theCreateSessionParamsbefore calling the repository'sCreateSessionfunction. The implementation matches the SQL parameter addition.internal/bootstrap/app_bootstrap.go (1)
45-48: LGTM! Session configuration validation is correctly implemented.The validation ensures that when
SessionMaxLifetimeis configured (non-zero), it must be at least as long asSessionExpiry. This prevents an invalid configuration where sessions would be forced to re-authenticate before they could be refreshed.internal/repository/models.go (1)
16-16: LGTM! Auto-generated field correctly reflects the schema change.The
CreatedAtfield is properly generated by sqlc from the schema definition, with the correctint64type for Unix timestamp storage.internal/config/config.go (1)
47-47: LGTM! SessionMaxLifetime field correctly implements hard session lifetime limit.The new
SessionMaxLifetimeconfiguration field enables enforcement of an absolute maximum session duration, preventing indefinite session extension through activity. This is a common security best practice.The field works alongside
SessionExpiry(line 46):
SessionExpiry: Controls refresh interval for active sessionsSessionMaxLifetime: Enforces hard limit requiring re-authenticationschema.sql (1)
10-10: The migration properly handles theNOT NULLconstraint withDEFAULT 0for existing rows. No issues found.cmd/tinyauth/tinyauth.go (1)
28-31: LGTM! SessionMaxLifetime field added with sensible default.The new
SessionMaxLifetime: 0field is correctly initialized with a default value of 0 (unlimited), which maintains backward compatibility. This aligns with the PR objectives to add an optional hard session lifetime limit.internal/controller/proxy_controller_test.go (1)
60-67: LGTM! Test configuration updated correctly.The test setup properly includes the new
SessionMaxLifetimefield with the default value of 0, maintaining consistency with the updatedAuthServiceConfigstructure.internal/controller/user_controller_test.go (1)
63-70: LGTM! Test configuration updated consistently.The
SessionMaxLifetimefield is correctly added to the test setup, matching the service configuration structure changes across the codebase.internal/bootstrap/service_bootstrap.go (1)
61-69: LGTM! Configuration properly wired to service layer.The
SessionMaxLifetimefield is correctly propagated from the application config to theAuthServiceConfig, ensuring the new session lifetime limit is available to the authentication service.internal/repository/query.sql.go (3)
12-74: LGTM! Generated CreateSession query correctly handles CreatedAt.The
CreatedAtfield is properly integrated into the session creation flow:
- Added to the INSERT column list (line 22)
- Included in VALUES placeholders (line 26)
- Returned via RETURNING clause (line 28)
- Added to
CreateSessionParamsstruct (line 40)- Correctly passed and scanned (lines 55, 69)
96-118: LGTM! GetSession query correctly retrieves CreatedAt.The
CreatedAtfield is properly included in the SELECT query and correctly scanned into the Session struct.
120-176: LGTM! UpdateSession correctly preserves CreatedAt immutability.The implementation correctly handles
CreatedAt:
- NOT included in the UPDATE SET clause (lines 121-130), ensuring immutability
- Included in RETURNING clause (line 132) to return the existing value
- Properly scanned (line 171)
This ensures that
created_atremains unchanged after session creation, which is the correct behavior for tracking session age.internal/service/auth_service.go (4)
28-38: LGTM!The
SessionMaxLifetimefield addition is clean and follows the existing pattern for time-based configuration (usingintfor seconds, consistent withSessionExpiry).
247-259: LGTM!The dynamic refresh threshold calculation is well-designed:
- For short sessions (≤1 hour): threshold = half the session expiry (prevents threshold from exceeding session duration)
- For longer sessions: capped at 1 hour
The incremental extension via
session.Expiry + refreshThreshold(line 259) combined with the max lifetime enforcement inGetSessionCookieprovides the intended behavior: active users get extended sessions, butSessionMaxLifetimeenforces the hard cap.
278-279: LGTM!The cookie's
MaxAgeis correctly set to the actual remaining time until the new expiry, ensuring browser cookie expiration aligns with server-side session state. The trace log is helpful for debugging.
320-328: LGTM!The max lifetime enforcement is well-implemented:
- Gracefully handles disabled feature (
SessionMaxLifetime == 0) and legacy sessions (CreatedAt == 0)- Correctly positioned before the regular expiry check
- Clear error message distinguishes max lifetime expiration from normal expiry
- Delete failure is logged but doesn't block the rejection, ensuring security even if cleanup fails
2 participants
Session Refresh Logic Improvements
=> Fixed refresh threshold calculation
SessionExpiry < 1 hour: usesSessionExpiry / 2as the refresh thresholdSessionExpiry >= 1 hour: uses 1 hour as the refresh thresholdThis prevents the refresh threshold from exceeding the session expiry duration
=> Fixed refresh timing to prevent excessive database writes
Changed from
currentTime + refreshThresholdtosession.Expiry + refreshThreshold.Using
currentTime + refreshThresholdcaused continuous refreshes once the threshold was reached (e.g., with 1hr threshold: session drops to 59min → refresh adds 1hr → next request at 59min triggers another refresh). Adding to the existing expiry (session.Expiry + refreshThreshold) ensures refresh only happens once when crossing the threshold, while still extending sessions incrementally for active users.=> Added hard session lifetime limit
SessionMaxLifetime(defaults to 0 for unlimited)created_atfield to sessions table to track session ageSummary by CodeRabbit
New Features
Configuration Changes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.