Conversation
After a Google account is disabled or password reset, we need ADB access to expire in less than 30 days which is the current behavior. However, this change avoids expiring the session too quickly in order to avoid data loss until we can set up sessions to renew automatically before they expire if the user is still working or verify this is already happening.
jakehobbs
left a comment
jakehobbs
left a comment
There was a problem hiding this comment.
lgtm but you are gonna make almira's life harder 😆
|
Still thinking about this. In an emergency we could shut down the ADB and start it back up with different keys, and I don't want someone to be in the middle of taking attendance without any indication that their session is expiring imminently. |
|
(In addition to rotating the server key in the event of an emergency) Maybe we can make the cookie expire at 5AM to avoid people getting signed out while trying to do something like taking attendance. Might update this PR for that eventually. ChatGPT and I co-authored this function (not tested) package main
import (
"fmt"
"time"
)
func next5AMPTAfter24Hours() time.Time {
// Get the current time in PT
loc, err := time.LoadLocation("America/Los_Angeles")
if err != nil {
panic(err)
}
now := time.Now().In(loc)
// Calculate 5 AM PT tomorrow
tomorrow := now.AddDate(0, 0, 1)
tomorrow5AM := time.Date(tomorrow.Year(), tomorrow.Month(), tomorrow.Day(), 5, 0, 0, 0, loc)
// If tomorrow's 5 AM PT is more than 24 hours from now, use it
if tomorrow5AM.After(now.Add(24 * time.Hour)) {
return tomorrow5AM
}
// Otherwise, calculate 5 AM PT the day after tomorrow
dayAfterTomorrow := now.AddDate(0, 0, 2)
dayAfterTomorrow5AM := time.Date(dayAfterTomorrow.Year(), dayAfterTomorrow.Month(), dayAfterTomorrow.Day(), 5, 0, 0, 0, loc)
return dayAfterTomorrow5AM
} |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe auth-session cookie MaxAge in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.go`:
- Around line 143-146: The session uses a fixed 12-hour MaxAge and is not
refreshed on authenticated requests; update authRoleMiddleware and
apiRoleMiddleware so they perform rolling renewal by calling setAuthSession(w,
r, user) after authentication succeeds (i.e., immediately after getAuthedADBUser
returns a valid user in authRoleMiddleware, and immediately after successful
authentication in apiRoleMiddleware), ensuring the existing cookie/session is
reset to extend the 12-hour window on each validated request.
| // MaxAge is 12 hours in seconds | ||
| MaxAge: 12 * // hours | ||
| 60 * // minutes | ||
| 60, // seconds |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== setAuthSession definition + call sites =="
rg -nP --type=go '\bfunc\s+setAuthSession\b|\bsetAuthSession\s*\(' -C2
echo
echo "== auth middlewares and session persistence usage =="
rg -nP --type=go 'authRoleMiddleware|apiRoleMiddleware|sessionStore\.Save\(' -C3Repository: dxe/adb
Length of output: 3291
🏁 Script executed:
sed -n '311,337p' main.go && echo "---" && sed -n '396,412p' main.goRepository: dxe/adb
Length of output: 1261
Implement rolling session renewal; current 12-hour MaxAge is a hard cutoff.
The session is only set during sign-in (line 514) with a fixed 12-hour expiry and is never refreshed on authenticated requests. Both authRoleMiddleware and apiRoleMiddleware validate the existing session but do not renew it. This means active users will be logged out 12 hours from login time regardless of ongoing activity, which conflicts with the stated goal of avoiding mid-session disruption.
Add session renewal in both auth middlewares on successful authentication:
- In
authRoleMiddlewareafter thegetAuthedADBUsercheck succeeds, callsetAuthSession(w, r, user)to extend the 12-hour window. - In
apiRoleMiddlewareimmediately after successful authentication, callsetAuthSession(w, r, user).
Proposed fix example (rolling renewal on authenticated requests)
func (c MainController) authRoleMiddleware(h http.Handler, allowedRoles []string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
user, authed := getAuthedADBUser(c.db, r)
if !authed {
// Delete the cookie if it doesn't auth.
c := &http.Cookie{
Name: "auth-session",
Path: "/",
MaxAge: -1,
HttpOnly: true,
SameSite: http.SameSiteLaxMode,
}
http.SetCookie(w, c)
http.Redirect(w, r, "/login", http.StatusFound)
return
}
+ // Rolling renewal: keep active sessions alive.
+ if err := setAuthSession(w, r, user); err != nil {
+ log.Println("failed to refresh auth session:", err)
+ }
+
if !userIsAllowed(allowedRoles, user) {
http.Redirect(w, r.WithContext(setUserContext(r, user)), "/403", http.StatusFound)
return
}
// Request is authed at this point.
h.ServeHTTP(w, r.WithContext(setUserContext(r, user)))
})
}
func (c MainController) apiRoleMiddleware(h http.Handler, allowedRoles []string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
user, authed := getAuthedADBUser(c.db, r)
if !authed {
http.Error(w, http.StatusText(400), 400)
return
}
+
+ // Rolling renewal for API activity too.
+ if err := setAuthSession(w, r, user); err != nil {
+ log.Println("failed to refresh auth session:", err)
+ }
if !userIsAllowed(allowedRoles, user) {
http.Error(w, http.StatusText(403), 403)
return
}
// Request is authed at this point.
h.ServeHTTP(w, r)
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.go` around lines 143 - 146, The session uses a fixed 12-hour MaxAge and
is not refreshed on authenticated requests; update authRoleMiddleware and
apiRoleMiddleware so they perform rolling renewal by calling setAuthSession(w,
r, user) after authentication succeeds (i.e., immediately after getAuthedADBUser
returns a valid user in authRoleMiddleware, and immediately after successful
authentication in apiRoleMiddleware), ensuring the existing cookie/session is
reset to extend the 12-hour window on each validated request.
2 participants
After a Google account is disabled or password reset, we need ADB access to expire in less than 30 days which is the current behavior. However, this change avoids expiring the session too quickly in order to avoid data loss until we can set up sessions to renew automatically before they expire if the user is still working or verify this is already happening.
Summary by CodeRabbit