feat(login): add remember-me checkbox for 30-day sessions#116
Conversation
- Add "Remember Me" checkbox to login form with custom styled indicator - Extend session MaxAge and skip idle timeout for remember-me sessions - Extract SessionOptions helper to consolidate session cookie settings - Refactor AuthHandler to accept *config.Config directly - Support dark mode, reduced-motion, and high-contrast accessibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds an optional “Remember me for N days” login checkbox that extends session lifetime (via new config) and introduces middleware/helpers to standardize session cookie options.
Changes:
- Add remember-me UI on the login page (with dark-mode styling) and plumb new template props.
- Add session remember-me support in auth handler and middleware (cookie MaxAge override + idle-timeout bypass).
- Add new config knobs:
SESSION_REMEMBER_ME_ENABLEDandSESSION_REMEMBER_ME_MAX_AGE, plus refactorAuthHandlerconstruction to take*config.Config.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/templates/static/css/pages/login.css | Adds styles/animations for the remember-me checkbox. |
| internal/templates/static/css/components/dark-mode.css | Adds dark-theme focus ring for remember-me checkbox. |
| internal/templates/props.go | Extends LoginPageProps with remember-me fields. |
| internal/templates/login_page.templ | Renders remember-me checkbox + label when enabled. |
| internal/middleware/auth.go | Adds SessionOptions helper, remember-me middleware, and idle-timeout bypass for remember-me sessions. |
| internal/handlers/auth.go | Refactors handler to accept *config.Config; sets remember-me session flag and cookie options on login; passes props to template. |
| internal/config/config.go | Adds remember-me configuration fields and env var loading defaults. |
| internal/bootstrap/router.go | Uses middleware.SessionOptions and wires remember-me middleware into the global chain. |
| internal/bootstrap/handlers.go | Updates NewAuthHandler call site to pass *config.Config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/templates/login_page.templ
Outdated
| <polyline points="2.5 6 5 8.5 9.5 3.5"></polyline> | ||
| </svg> | ||
| </span> | ||
| <span class="login-remember-text">{ fmt.Sprintf("Remember me for %d days", props.RememberMeDays) }</span> |
There was a problem hiding this comment.
The label text will read "Remember me for 1 days" when props.RememberMeDays == 1. Please pluralize the unit (e.g., day vs days) so the UI reads correctly.
| <span class="login-remember-text">{ fmt.Sprintf("Remember me for %d days", props.RememberMeDays) }</span> | |
| <span class="login-remember-text"> | |
| if props.RememberMeDays == 1 { | |
| { fmt.Sprintf("Remember me for %d day", props.RememberMeDays) } | |
| } else { | |
| { fmt.Sprintf("Remember me for %d days", props.RememberMeDays) } | |
| } | |
| </span> |
internal/middleware/auth.go
Outdated
| if session.Get(SessionRememberMe) != nil { | ||
| session.Options(opts) |
There was a problem hiding this comment.
SessionRememberMeMiddleware sets session.Options(opts) but never saves the session. With gin-contrib/sessions the cookie Max-Age/Expires is only updated when session.Save() is called, so this won’t actually produce a sliding expiration on requests that don’t otherwise save the session.
| if session.Get(SessionRememberMe) != nil { | |
| session.Options(opts) | |
| if session.Get(SessionRememberMe) != nil { | |
| // Apply remember-me cookie options | |
| session.Options(opts) | |
| // Proceed with the rest of the handlers so any additional session changes are captured | |
| c.Next() | |
| // Ensure the updated options (and any changes) are persisted to the cookie | |
| _ = session.Save() | |
| return |
internal/middleware/auth.go
Outdated
| // Skip idle timeout for "remember me" sessions | ||
| if session.Get(SessionRememberMe) != nil { |
There was a problem hiding this comment.
In SessionIdleTimeout, the early return for remember-me sessions skips the SessionLastActivity update and the session.Save() call. If the goal is a sliding cookie expiration for remember-me, you still need a save somewhere on each request (even if you don’t enforce idle timeout).
| // Skip idle timeout for "remember me" sessions | |
| if session.Get(SessionRememberMe) != nil { | |
| // For "remember me" sessions, skip idle-timeout enforcement | |
| // but still update last activity and save the session so that | |
| // cookie expiration can slide on each request. | |
| if session.Get(SessionRememberMe) != nil { | |
| session.Set(SessionLastActivity, time.Now().Unix()) | |
| _ = session.Save() |
| // SessionRememberMeMiddleware overrides cookie MaxAge for "remember me" sessions. | ||
| // Must run after sessions.Sessions() and before SessionIdleTimeout. | ||
| func SessionRememberMeMiddleware(rememberMeMaxAge int, isProduction bool) gin.HandlerFunc { | ||
| opts := SessionOptions(rememberMeMaxAge, isProduction) | ||
| return func(c *gin.Context) { | ||
| session := sessions.Default(c) | ||
| if session.Get(SessionRememberMe) != nil { | ||
| session.Options(opts) | ||
| } | ||
| c.Next() | ||
| } | ||
| } |
There was a problem hiding this comment.
No tests cover the new remember-me behavior (setting remember_me should bypass idle timeout and should refresh cookie Max-Age if sliding expiration is intended). Since internal/middleware/auth.go already has auth_test.go, please add unit tests exercising the remember-me branch.
internal/handlers/auth.go
Outdated
|
|
||
| // rememberMeDays converts SessionRememberMeMaxAge (seconds) to days for display. | ||
| func (h *AuthHandler) rememberMeDays() int { | ||
| return h.cfg.SessionRememberMeMaxAge / 86400 |
There was a problem hiding this comment.
rememberMeDays() uses integer division by 86400, which can produce 0 (or generally a truncated value) for non-day-aligned SESSION_REMEMBER_ME_MAX_AGE, leading to a misleading UI label. Consider rounding up and enforcing a minimum of 1 day (or rendering hours when < 1 day).
| return h.cfg.SessionRememberMeMaxAge / 86400 | |
| maxAge := h.cfg.SessionRememberMeMaxAge | |
| if maxAge <= 0 { | |
| return 0 | |
| } | |
| // Use ceiling division so that any positive, non-day-aligned duration | |
| // is rounded up to the next whole day for display. | |
| return (maxAge + 86400 - 1) / 86400 |
- Pluralize day/days label based on RememberMeDays value - Use ceiling division in rememberMeDays() to handle non-day-aligned values - Call session.Save() after c.Next() in SessionRememberMeMiddleware for sliding expiration - Update SessionLastActivity for remember-me sessions in idle timeout middleware - Add unit tests for remember-me middleware, idle timeout bypass, and SessionOptions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot /review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestSessionRememberMeMiddleware_SetsOptions(t *testing.T) { | ||
| r := setupTestRouter() | ||
|
|
||
| // Set up session with remember_me flag | ||
| r.Use(func(c *gin.Context) { | ||
| session := sessions.Default(c) | ||
| session.Set(SessionUserID, "user123") | ||
| session.Set(SessionRememberMe, true) | ||
| _ = session.Save() | ||
| c.Next() | ||
| }) | ||
|
|
||
| r.Use(SessionRememberMeMiddleware(2592000, false)) | ||
|
|
||
| handlerCalled := false | ||
| r.GET("/test", func(c *gin.Context) { | ||
| handlerCalled = true | ||
| c.String(http.StatusOK, "OK") | ||
| }) | ||
|
|
||
| w := httptest.NewRecorder() | ||
| req, _ := http.NewRequestWithContext(context.Background(), http.MethodGet, "/test", nil) | ||
| r.ServeHTTP(w, req) | ||
|
|
||
| assert.Equal(t, http.StatusOK, w.Code) | ||
| assert.True(t, handlerCalled) | ||
| } |
There was a problem hiding this comment.
TestSessionRememberMeMiddleware_SetsOptions / _NoRememberMe currently only assert that the handler ran, but they don't verify the actual behavior (that a Set-Cookie is emitted with the configured Max-Age for remember-me, and unchanged when not set). Adding assertions against the response Set-Cookie header (or a follow-up request using the returned cookie) would make these tests catch regressions in sliding-expiration behavior.
| sessionStore := cookie.NewStore([]byte(cfg.SessionSecret)) | ||
| sessionStore.Options(sessions.Options{ | ||
| Path: "/", | ||
| MaxAge: cfg.SessionMaxAge, | ||
| HttpOnly: true, | ||
| Secure: cfg.IsProduction, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) | ||
| opts := middleware.SessionOptions(cfg.SessionMaxAge, cfg.IsProduction) | ||
| sessionStore.Options(opts) | ||
| r.Use(sessions.Sessions("oauth_session", sessionStore)) | ||
| r.Use(middleware.SessionRememberMeMiddleware(cfg.SessionRememberMeMaxAge, cfg.IsProduction)) | ||
| r.Use(middleware.SessionIdleTimeout(cfg.SessionIdleTimeout)) |
There was a problem hiding this comment.
Because the session store is cookie-based, the underlying securecookie decoder also has a max-age limit (default is typically 30 days). If SESSION_REMEMBER_ME_MAX_AGE is configured > 30 days, browsers may keep sending the cookie but decoding can fail unless the store's codec max age is increased too (often via sessionStore.MaxAge(...)). Consider setting the store max-age to at least max(cfg.SessionMaxAge, cfg.SessionRememberMeMaxAge) to ensure remember-me durations above 30 days actually work.
internal/middleware/auth.go
Outdated
| return func(c *gin.Context) { | ||
| session := sessions.Default(c) | ||
| if session.Get(SessionRememberMe) != nil { | ||
| session.Options(opts) | ||
| c.Next() | ||
| // Ensure the updated cookie options are persisted (sliding expiration) | ||
| _ = session.Save() | ||
| return |
There was a problem hiding this comment.
SessionRememberMeMiddleware calls session.Save() after c.Next(). If the handler has already written the response (e.g., c.String, c.JSON, templates), headers are committed and the updated Set-Cookie (sliding expiration) won't be sent. Consider saving before the response is written (e.g., call session.Save() before c.Next(), or wrap the ResponseWriter to save on first write) and handle/log save errors.
| // For "remember me" sessions, skip idle-timeout enforcement | ||
| // but still update last activity for metrics/audit. | ||
| if session.Get(SessionRememberMe) != nil { | ||
| session.Set(SessionLastActivity, time.Now().Unix()) |
There was a problem hiding this comment.
The remember-me path in SessionIdleTimeout updates SessionLastActivity but never saves the session, so the new timestamp won't persist to the cookie/store for subsequent requests. Mirror the non-remember path by saving after setting SessionLastActivity (ideally before the response is written).
| session.Set(SessionLastActivity, time.Now().Unix()) | |
| session.Set(SessionLastActivity, time.Now().Unix()) | |
| _ = session.Save() |
internal/middleware/auth.go
Outdated
| opts := SessionOptions(rememberMeMaxAge, isProduction) | ||
| return func(c *gin.Context) { | ||
| session := sessions.Default(c) | ||
| if session.Get(SessionRememberMe) != nil { |
There was a problem hiding this comment.
SessionRememberMeMiddleware checks session.Get(SessionRememberMe) != nil, which will enable remember-me even if the value is present but false/invalid. Since this key is intended to be boolean, consider type-asserting to bool and requiring it to be true before overriding options.
| if session.Get(SessionRememberMe) != nil { | |
| if remember, ok := session.Get(SessionRememberMe).(bool); ok && remember { |
internal/handlers/auth.go
Outdated
| func (h *AuthHandler) rememberMeDays() int { | ||
| maxAge := h.cfg.SessionRememberMeMaxAge | ||
| if maxAge <= 0 { | ||
| return 0 |
There was a problem hiding this comment.
rememberMeDays() can return 0 when SessionRememberMeMaxAge <= 0, but the UI still renders the checkbox when SessionRememberMeEnabled is true, resulting in a confusing label like "Remember me for 0 days". Consider clamping the computed value to at least 1 (or hiding/disable the checkbox when the max-age is not positive).
| func (h *AuthHandler) rememberMeDays() int { | |
| maxAge := h.cfg.SessionRememberMeMaxAge | |
| if maxAge <= 0 { | |
| return 0 | |
| // Clamps the result to a minimum of 1 day to avoid confusing "0 days" labels. | |
| func (h *AuthHandler) rememberMeDays() int { | |
| maxAge := h.cfg.SessionRememberMeMaxAge | |
| if maxAge <= 0 { | |
| return 1 |
| SessionRememberMeEnabled: getEnvBool("SESSION_REMEMBER_ME_ENABLED", true), | ||
| SessionRememberMeMaxAge: getEnvInt("SESSION_REMEMBER_ME_MAX_AGE", 2592000), // 30 days | ||
| DeviceCodeExpiration: 30 * time.Minute, |
There was a problem hiding this comment.
SESSION_REMEMBER_ME_MAX_AGE is loaded directly into SessionRememberMeMaxAge without validation. A zero/negative value can lead to non-persistent sessions or cookie deletion semantics depending on the sessions backend. Consider adding validation in Config.Validate() when remember-me is enabled (e.g., require a positive max age, and possibly enforce reasonable bounds).
- Save session before c.Next() in SessionRememberMeMiddleware so sliding-expiration cookie is sent before headers are committed - Type-assert SessionRememberMe to bool instead of nil check - Save session in idle timeout remember-me path to persist LastActivity - Clamp rememberMeDays() minimum to 1 to avoid "0 days" label - Add config validation for SESSION_REMEMBER_ME_MAX_AGE when enabled - Improve tests with Set-Cookie header assertions for Max-Age Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
SESSION_REMEMBER_ME_MAX_AGE)SessionRememberMeMiddlewaremiddleware.SessionOptions()helper to consolidate duplicatedsessions.Optionsstructs across router, middleware, and handlerAuthHandlerto accept*config.Configdirectly (was 7 positional params, now 3), matching the pattern used by other handlersNew Configuration
SESSION_REMEMBER_ME_ENABLEDtrueSESSION_REMEMBER_ME_MAX_AGE2592000(30 days)Test plan
Max-Ageis ~2592000 when "Remember Me" is checked (browser DevTools → Application → Cookies)SESSION_REMEMBER_ME_ENABLED=false→ checkbox is hiddenSESSION_REMEMBER_ME_MAX_AGE=604800→ label shows "Remember me for 7 days"make testpasses🤖 Generated with Claude Code