From 3e2fe1fed46d04bc4a9611f928bb3eee5885fe57 Mon Sep 17 00:00:00 2001 From: Eric Lesiuta Date: Fri, 8 May 2026 22:10:24 -0400 Subject: [PATCH] fix(session): address review feedback - Use shared flex-divided-list/items-with-main classes in session templates so they pick up consistent styling instead of relying on non-existent flex-list/flex-item-* classes. - Add CountUserSessionsByUserID helper and use it in admin user view so we don't materialize the full session list just to compute total/active counts. - Bound the VirtualSessionProvider tombstone map by self-expiring entries via time.AfterFunc, so map size no longer depends on the session GC interval. Co-Authored-By: GitHub Copilot (Claude Opus 4.7) --- models/auth/user_session.go | 15 ++++++++ modules/session/virtual.go | 36 +++++++++---------- routers/web/admin/users.go | 14 +++----- templates/admin/user/sessions.tmpl | 16 ++++----- .../user/settings/security/sessions.tmpl | 16 ++++----- 5 files changed, 51 insertions(+), 46 deletions(-) diff --git a/models/auth/user_session.go b/models/auth/user_session.go index 475c812322..8179e8e58e 100644 --- a/models/auth/user_session.go +++ b/models/auth/user_session.go @@ -76,6 +76,21 @@ func GetUserSessionsByUserID(ctx context.Context, userID int64) ([]*UserSession, Desc("created_unix").Find(&sessions) } +// CountUserSessionsByUserID returns (total, active) session counts for a user +// without materializing the rows. +func CountUserSessionsByUserID(ctx context.Context, userID int64) (total, active int64, err error) { + e := db.GetEngine(ctx) + total, err = e.Where("user_id = ?", userID).Count(new(UserSession)) + if err != nil { + return 0, 0, err + } + active, err = e.Where("user_id = ? AND logout_unix = 0", userID).Count(new(UserSession)) + if err != nil { + return 0, 0, err + } + return total, active, nil +} + // InvalidateUserSession marks a session as logged out func InvalidateUserSession(ctx context.Context, sessionID string) error { _, err := db.GetEngine(ctx).Where("id = ? AND logout_unix = 0", sessionID). diff --git a/modules/session/virtual.go b/modules/session/virtual.go index 90434e18e2..642382466f 100644 --- a/modules/session/virtual.go +++ b/modules/session/virtual.go @@ -17,6 +17,11 @@ import ( postgres "gitea.com/go-chi/session/postgres" ) +// tombstoneTTL is how long a destroyed session ID is remembered so that +// concurrent requests releasing after destruction cannot recreate the session +// or be re-authenticated. +const tombstoneTTL = 10 * time.Minute + // VirtualSessionProvider represents a shadowed session provider implementation. // It wraps a real session provider and adds "tombstone" tracking for destroyed // sessions so that concurrent requests (e.g. EventSource) cannot accidentally @@ -30,6 +35,8 @@ type VirtualSessionProvider struct { // a FileStore reference may call Release() and recreate the file. // By tracking destroyed IDs, Read() returns an inert VirtualStore // that prevents re-authentication and avoids recreating the file. + // Entries self-expire after tombstoneTTL via time.AfterFunc so the map + // stays bounded regardless of session GC interval. destroyedSIDs sync.Map // sid -> time.Time } @@ -100,6 +107,10 @@ func (o *VirtualSessionProvider) Destroy(sid string) error { o.lock.Lock() defer o.lock.Unlock() o.destroyedSIDs.Store(sid, time.Now()) + // Self-expire the tombstone so the map stays bounded between session GCs. + time.AfterFunc(tombstoneTTL, func() { + o.destroyedSIDs.Delete(sid) + }) return o.provider.Destroy(sid) } @@ -124,28 +135,13 @@ func (o *VirtualSessionProvider) GC() { o.provider.GC() - // Clean up tombstone entries and re-destroy any files that may have - // been recreated by concurrent requests releasing after destruction. - cutoff := time.Now().Add(-10 * time.Minute) - var stale []string - var active []string - o.destroyedSIDs.Range(func(key, value any) bool { - sid := key.(string) - if value.(time.Time).Before(cutoff) { - stale = append(stale, sid) - } else { - active = append(active, sid) - } + // Re-destroy any sessions that may have been recreated by concurrent + // requests releasing after destruction. Tombstones themselves expire + // via time.AfterFunc in Destroy() so no manual cleanup is needed here. + o.destroyedSIDs.Range(func(key, _ any) bool { + _ = o.provider.Destroy(key.(string)) return true }) - for _, sid := range stale { - o.destroyedSIDs.Delete(sid) - } - if len(active) > 0 { - for _, sid := range active { - _ = o.provider.Destroy(sid) - } - } } func init() { diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 2e18f72f89..76cc78e67e 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -305,19 +305,13 @@ func ViewUser(ctx *context.Context) { ctx.Data["Users"] = orgs // needed to be able to use explore/user_list template ctx.Data["OrgsTotal"] = len(orgs) - userSessions, err := auth.GetUserSessionsByUserID(ctx, u.ID) + sessionsTotal, sessionsActive, err := auth.CountUserSessionsByUserID(ctx, u.ID) if err != nil { - ctx.ServerError("GetUserSessionsByUserID", err) + ctx.ServerError("CountUserSessionsByUserID", err) return } - ctx.Data["SessionsTotal"] = len(userSessions) - activeCount := 0 - for _, s := range userSessions { - if s.LogoutUnix == 0 { - activeCount++ - } - } - ctx.Data["SessionsActive"] = activeCount + ctx.Data["SessionsTotal"] = sessionsTotal + ctx.Data["SessionsActive"] = sessionsActive ctx.HTML(http.StatusOK, tplUserView) } diff --git a/templates/admin/user/sessions.tmpl b/templates/admin/user/sessions.tmpl index 75d3e197c3..adc4e37fd5 100644 --- a/templates/admin/user/sessions.tmpl +++ b/templates/admin/user/sessions.tmpl @@ -14,14 +14,14 @@
{{if .Sessions}} -
+
{{range .Sessions}} -
-
+
+
{{svg "octicon-device-desktop" 32}}
-
-
+
+
{{StringUtils.EllipsisString .UserAgent 60}} {{if .LogoutUnix}} {{ctx.Locale.Tr "settings.sessions.ended"}} @@ -29,7 +29,7 @@ {{ctx.Locale.Tr "settings.sessions.active"}} {{end}}
-
+
{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}} {{if .LastIP}} | {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}} @@ -38,14 +38,14 @@ | {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}} {{end}}
-
+
{{svg "octicon-key"}} {{.LoginMethod}} | {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}} | {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}
{{if not .LogoutUnix}} -
+
diff --git a/templates/user/settings/security/sessions.tmpl b/templates/user/settings/security/sessions.tmpl index 8784d9acf4..7c095b4b04 100644 --- a/templates/user/settings/security/sessions.tmpl +++ b/templates/user/settings/security/sessions.tmpl @@ -14,14 +14,14 @@

{{ctx.Locale.Tr "settings.sessions_desc"}}

{{if .Sessions}} -
+
{{range .Sessions}} -
-
+
+
{{svg "octicon-device-desktop" 32}}
-
-
+
+
{{StringUtils.EllipsisString .UserAgent 60}} {{if eq .ID $.CurrentSessionID}} {{ctx.Locale.Tr "settings.sessions.current"}} @@ -30,7 +30,7 @@ {{ctx.Locale.Tr "settings.sessions.ended"}} {{end}}
-
+
{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}} {{if .LastIP}} | {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}} @@ -39,14 +39,14 @@ | {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}} {{end}}
-
+
{{svg "octicon-key"}} {{.LoginMethod}} | {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}} | {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}
{{if and (not .LogoutUnix) (ne .ID $.CurrentSessionID)}} -
+