0
0
mirror of https://github.com/go-gitea/gitea.git synced 2026-05-11 13:35:24 +02:00

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) <copilot@github.com>
This commit is contained in:
Eric Lesiuta 2026-05-08 22:10:24 -04:00
parent 093061d3ba
commit 3e2fe1fed4
No known key found for this signature in database
5 changed files with 51 additions and 46 deletions

View File

@ -76,6 +76,21 @@ func GetUserSessionsByUserID(ctx context.Context, userID int64) ([]*UserSession,
Desc("created_unix").Find(&sessions) 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 // InvalidateUserSession marks a session as logged out
func InvalidateUserSession(ctx context.Context, sessionID string) error { func InvalidateUserSession(ctx context.Context, sessionID string) error {
_, err := db.GetEngine(ctx).Where("id = ? AND logout_unix = 0", sessionID). _, err := db.GetEngine(ctx).Where("id = ? AND logout_unix = 0", sessionID).

View File

@ -17,6 +17,11 @@ import (
postgres "gitea.com/go-chi/session/postgres" 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. // VirtualSessionProvider represents a shadowed session provider implementation.
// It wraps a real session provider and adds "tombstone" tracking for destroyed // It wraps a real session provider and adds "tombstone" tracking for destroyed
// sessions so that concurrent requests (e.g. EventSource) cannot accidentally // 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. // a FileStore reference may call Release() and recreate the file.
// By tracking destroyed IDs, Read() returns an inert VirtualStore // By tracking destroyed IDs, Read() returns an inert VirtualStore
// that prevents re-authentication and avoids recreating the file. // 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 destroyedSIDs sync.Map // sid -> time.Time
} }
@ -100,6 +107,10 @@ func (o *VirtualSessionProvider) Destroy(sid string) error {
o.lock.Lock() o.lock.Lock()
defer o.lock.Unlock() defer o.lock.Unlock()
o.destroyedSIDs.Store(sid, time.Now()) 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) return o.provider.Destroy(sid)
} }
@ -124,28 +135,13 @@ func (o *VirtualSessionProvider) GC() {
o.provider.GC() o.provider.GC()
// Clean up tombstone entries and re-destroy any files that may have // Re-destroy any sessions that may have been recreated by concurrent
// been recreated by concurrent requests releasing after destruction. // requests releasing after destruction. Tombstones themselves expire
cutoff := time.Now().Add(-10 * time.Minute) // via time.AfterFunc in Destroy() so no manual cleanup is needed here.
var stale []string o.destroyedSIDs.Range(func(key, _ any) bool {
var active []string _ = o.provider.Destroy(key.(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)
}
return true return true
}) })
for _, sid := range stale {
o.destroyedSIDs.Delete(sid)
}
if len(active) > 0 {
for _, sid := range active {
_ = o.provider.Destroy(sid)
}
}
} }
func init() { func init() {

View File

@ -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["Users"] = orgs // needed to be able to use explore/user_list template
ctx.Data["OrgsTotal"] = len(orgs) ctx.Data["OrgsTotal"] = len(orgs)
userSessions, err := auth.GetUserSessionsByUserID(ctx, u.ID) sessionsTotal, sessionsActive, err := auth.CountUserSessionsByUserID(ctx, u.ID)
if err != nil { if err != nil {
ctx.ServerError("GetUserSessionsByUserID", err) ctx.ServerError("CountUserSessionsByUserID", err)
return return
} }
ctx.Data["SessionsTotal"] = len(userSessions) ctx.Data["SessionsTotal"] = sessionsTotal
activeCount := 0 ctx.Data["SessionsActive"] = sessionsActive
for _, s := range userSessions {
if s.LogoutUnix == 0 {
activeCount++
}
}
ctx.Data["SessionsActive"] = activeCount
ctx.HTML(http.StatusOK, tplUserView) ctx.HTML(http.StatusOK, tplUserView)
} }

View File

@ -14,14 +14,14 @@
</h4> </h4>
<div class="ui attached segment"> <div class="ui attached segment">
{{if .Sessions}} {{if .Sessions}}
<div class="flex-list"> <div class="flex-divided-list items-with-main">
{{range .Sessions}} {{range .Sessions}}
<div class="flex-item"> <div class="item">
<div class="flex-item-leading"> <div class="item-leading">
{{svg "octicon-device-desktop" 32}} {{svg "octicon-device-desktop" 32}}
</div> </div>
<div class="flex-item-main"> <div class="item-main">
<div class="flex-item-title"> <div class="item-title">
{{StringUtils.EllipsisString .UserAgent 60}} {{StringUtils.EllipsisString .UserAgent 60}}
{{if .LogoutUnix}} {{if .LogoutUnix}}
<span class="ui grey label">{{ctx.Locale.Tr "settings.sessions.ended"}}</span> <span class="ui grey label">{{ctx.Locale.Tr "settings.sessions.ended"}}</span>
@ -29,7 +29,7 @@
<span class="ui green label">{{ctx.Locale.Tr "settings.sessions.active"}}</span> <span class="ui green label">{{ctx.Locale.Tr "settings.sessions.active"}}</span>
{{end}} {{end}}
</div> </div>
<div class="flex-item-body"> <div class="item-body">
<span class="flex-text-inline">{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}}</span> <span class="flex-text-inline">{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}}</span>
{{if .LastIP}} {{if .LastIP}}
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}}</span>
@ -38,14 +38,14 @@
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}}</span>
{{end}} {{end}}
</div> </div>
<div class="flex-item-body"> <div class="item-body">
<span class="flex-text-inline">{{svg "octicon-key"}} {{.LoginMethod}}</span> <span class="flex-text-inline">{{svg "octicon-key"}} {{.LoginMethod}}</span>
<span class="flex-text-inline">| {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}}</span> <span class="flex-text-inline">| {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}}</span>
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}</span>
</div> </div>
</div> </div>
{{if not .LogoutUnix}} {{if not .LogoutUnix}}
<div class="flex-item-trailing"> <div class="item-trailing">
<form action="{{AppSubUrl}}/-/admin/users/{{$.User.ID}}/sessions/revoke" method="post"> <form action="{{AppSubUrl}}/-/admin/users/{{$.User.ID}}/sessions/revoke" method="post">
<input type="hidden" name="session_id" value="{{.ID}}"> <input type="hidden" name="session_id" value="{{.ID}}">
<button class="ui red tiny button">{{ctx.Locale.Tr "settings.sessions.revoke"}}</button> <button class="ui red tiny button">{{ctx.Locale.Tr "settings.sessions.revoke"}}</button>

View File

@ -14,14 +14,14 @@
<div class="ui attached segment"> <div class="ui attached segment">
<p>{{ctx.Locale.Tr "settings.sessions_desc"}}</p> <p>{{ctx.Locale.Tr "settings.sessions_desc"}}</p>
{{if .Sessions}} {{if .Sessions}}
<div class="flex-list"> <div class="flex-divided-list items-with-main">
{{range .Sessions}} {{range .Sessions}}
<div class="flex-item {{if eq .ID $.CurrentSessionID}}tw-bg-green-50{{end}}"> <div class="item {{if eq .ID $.CurrentSessionID}}tw-bg-green-50{{end}}">
<div class="flex-item-leading"> <div class="item-leading">
{{svg "octicon-device-desktop" 32}} {{svg "octicon-device-desktop" 32}}
</div> </div>
<div class="flex-item-main"> <div class="item-main">
<div class="flex-item-title"> <div class="item-title">
{{StringUtils.EllipsisString .UserAgent 60}} {{StringUtils.EllipsisString .UserAgent 60}}
{{if eq .ID $.CurrentSessionID}} {{if eq .ID $.CurrentSessionID}}
<span class="ui green label">{{ctx.Locale.Tr "settings.sessions.current"}}</span> <span class="ui green label">{{ctx.Locale.Tr "settings.sessions.current"}}</span>
@ -30,7 +30,7 @@
<span class="ui grey label">{{ctx.Locale.Tr "settings.sessions.ended"}}</span> <span class="ui grey label">{{ctx.Locale.Tr "settings.sessions.ended"}}</span>
{{end}} {{end}}
</div> </div>
<div class="flex-item-body"> <div class="item-body">
<span class="flex-text-inline">{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}}</span> <span class="flex-text-inline">{{svg "octicon-globe"}} {{ctx.Locale.Tr "settings.sessions.login_ip"}}: {{.LoginIP}}</span>
{{if .LastIP}} {{if .LastIP}}
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_ip"}}: {{.LastIP}}</span>
@ -39,14 +39,14 @@
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.prev_ip"}}: {{.PrevIP}}</span>
{{end}} {{end}}
</div> </div>
<div class="flex-item-body"> <div class="item-body">
<span class="flex-text-inline">{{svg "octicon-key"}} {{.LoginMethod}}</span> <span class="flex-text-inline">{{svg "octicon-key"}} {{.LoginMethod}}</span>
<span class="flex-text-inline">| {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}}</span> <span class="flex-text-inline">| {{svg "octicon-calendar"}} {{DateUtils.AbsoluteShort .CreatedUnix}}</span>
<span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}</span> <span class="flex-text-inline">| {{ctx.Locale.Tr "settings.sessions.last_active"}}: {{DateUtils.TimeSince .LastAccessUnix}}</span>
</div> </div>
</div> </div>
{{if and (not .LogoutUnix) (ne .ID $.CurrentSessionID)}} {{if and (not .LogoutUnix) (ne .ID $.CurrentSessionID)}}
<div class="flex-item-trailing"> <div class="item-trailing">
<form action="{{AppSubUrl}}/user/settings/security/sessions/revoke" method="post"> <form action="{{AppSubUrl}}/user/settings/security/sessions/revoke" method="post">
<input type="hidden" name="session_id" value="{{.ID}}"> <input type="hidden" name="session_id" value="{{.ID}}">
<button class="ui red tiny button">{{ctx.Locale.Tr "settings.sessions.revoke"}}</button> <button class="ui red tiny button">{{ctx.Locale.Tr "settings.sessions.revoke"}}</button>