From 755f997b0d1c154293d3a41f4c4a4665447e0f3d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 6 Oct 2026 08:32:39 -0400 Subject: [PATCH] feat(auth): sessions expire server-side; password change and reset end other sessions (M462 #4978) Sessions had no server-side expiry: only the web cookie's 30-day Max-Age limited them, and a bearer token (Android) lived until revoked by hand. GetSessionByTokenHash and ListSessionsForUser now ignore sessions idle for 30 days or older than a year, and the GC worker deletes them hourly. A password change was a plain UPDATE, so a session opened with the old password survived it. Now: - self-service change signs out every other device and keeps this one; - reset by email ends every session the account has; - an admin reset ends the target's sessions (keeping the admin's own when they reset themselves). The success copy on web and Android says the other devices were signed out. Co-Authored-By: Claude Opus 5.5 --- .../minstrel/settings/ui/PasswordViewModel.kt | 2 +- internal/api/admin_users.go | 13 ++ internal/api/auth_reset.go | 7 + internal/api/me_password.go | 14 ++ internal/api/session_lifecycle_test.go | 139 ++++++++++++++++++ internal/db/dbq/gc.sql.go | 16 ++ internal/db/dbq/sessions.sql.go | 40 ++++- internal/db/queries/gc.sql | 7 + internal/db/queries/sessions.sql | 27 +++- internal/gc/worker.go | 2 + web/src/routes/settings/+page.svelte | 2 +- 11 files changed, 262 insertions(+), 7 deletions(-) create mode 100644 internal/api/session_lifecycle_test.go diff --git a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/PasswordViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/PasswordViewModel.kt index 2745ea02..ea50c42d 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/PasswordViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/PasswordViewModel.kt @@ -57,7 +57,7 @@ class PasswordViewModel @Inject constructor( try { repository.changePassword(current = s.current, next = s.next) internal.update { - PasswordUiState(message = "Password changed.") + PasswordUiState(message = "Password changed. Your other devices have been signed out.") } } catch ( @Suppress("TooGenericExceptionCaught") e: Throwable, diff --git a/internal/api/admin_users.go b/internal/api/admin_users.go index 57eb8010..c8fbc95a 100644 --- a/internal/api/admin_users.go +++ b/internal/api/admin_users.go @@ -285,6 +285,19 @@ func (h *handlers) handleAdminResetPassword(w http.ResponseWriter, r *http.Reque return } + // The target's existing sessions end with the old password. An admin + // resetting their own password keeps the session they're using, the + // same as a self-service change. + sessID, ownSession := auth.SessionIDFromContext(r.Context()) + if targetID == caller.ID && ownSession { + _, err = q.DeleteOtherSessionsForUser(r.Context(), dbq.DeleteOtherSessionsForUserParams{UserID: targetID, ID: sessID}) + } else { + _, err = q.DeleteSessionsForUser(r.Context(), targetID) + } + if err != nil { + h.logger.Error("admin reset password: revoke sessions failed", "err", err) + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, caller.ID, targetID, audit.ActionPasswordResetAdmin, nil) w.WriteHeader(http.StatusNoContent) diff --git a/internal/api/auth_reset.go b/internal/api/auth_reset.go index 3982d918..00196349 100644 --- a/internal/api/auth_reset.go +++ b/internal/api/auth_reset.go @@ -104,6 +104,13 @@ func (h *handlers) handleResetPassword(w http.ResponseWriter, r *http.Request) { return } + // End every session the account has. Whoever needed a reset isn't signed + // in anywhere they rely on, and whoever may have learned the old password + // must not stay signed in on it. + if _, err := q.DeleteSessionsForUser(r.Context(), reset.UserID); err != nil { + h.logger.Error("reset password: revoke sessions failed", "err", err) + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, reset.UserID, reset.UserID, audit.ActionPasswordResetByEmail, nil) w.WriteHeader(http.StatusNoContent) diff --git a/internal/api/me_password.go b/internal/api/me_password.go index 97ee42d7..22c9afe1 100644 --- a/internal/api/me_password.go +++ b/internal/api/me_password.go @@ -8,6 +8,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) @@ -61,6 +62,19 @@ func (h *handlers) handleChangePassword(w http.ResponseWriter, r *http.Request) return } + // Sign out every other device. A password change is often the response + // to suspecting someone else has it, and their session would otherwise + // outlive the password it was opened with. The device making the change + // stays signed in. + if sessID, ok := auth.SessionIDFromContext(r.Context()); ok { + if _, err := q.DeleteOtherSessionsForUser(r.Context(), dbq.DeleteOtherSessionsForUserParams{ + UserID: user.ID, + ID: sessID, + }); err != nil { + h.logger.Error("change password: revoke other sessions failed", "err", err) + } + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, user.ID, user.ID, audit.ActionPasswordChangeSelf, nil) w.WriteHeader(http.StatusNoContent) diff --git a/internal/api/session_lifecycle_test.go b/internal/api/session_lifecycle_test.go new file mode 100644 index 00000000..ceb4ab69 --- /dev/null +++ b/internal/api/session_lifecycle_test.go @@ -0,0 +1,139 @@ +package api + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +func sessionExists(t *testing.T, pool *pgxpool.Pool, id pgtype.UUID) bool { + t.Helper() + var exists bool + if err := pool.QueryRow(context.Background(), + `SELECT EXISTS (SELECT 1 FROM sessions WHERE id = $1)`, id, + ).Scan(&exists); err != nil { + t.Fatalf("exists check: %v", err) + } + return exists +} + +// Sessions expire server-side on two limits, and an expired one stops +// authenticating at once rather than when the GC sweep gets to it. +func TestSessionExpiry_IdleAndAbsoluteLimits(t *testing.T) { + _, pool := testHandlers(t) + user := seedUser(t, pool, "alice", "hunter2", false) + q := dbq.New(pool) + + mint := func(setup string) []byte { + token, err := auth.MintSessionToken() + if err != nil { + t.Fatalf("mint: %v", err) + } + hash := auth.HashSessionToken(token) + sess, err := q.InsertSession(context.Background(), dbq.InsertSessionParams{ + UserID: user.ID, TokenHash: hash, UserAgent: "test", Ip: "192.0.2.1", + }) + if err != nil { + t.Fatalf("insert: %v", err) + } + if setup != "" { + if _, err := pool.Exec(context.Background(), `UPDATE sessions SET `+setup+` WHERE id = $1`, sess.ID); err != nil { + t.Fatalf("age session: %v", err) + } + } + return hash + } + + fresh := mint("") + idle := mint(`last_seen_at = now() - interval '31 days'`) + old := mint(`created_at = now() - interval '366 days'`) + + if _, err := q.GetSessionByTokenHash(context.Background(), fresh); err != nil { + t.Errorf("fresh session: %v, want found", err) + } + for name, hash := range map[string][]byte{"idle": idle, "absolute": old} { + if _, err := q.GetSessionByTokenHash(context.Background(), hash); !errors.Is(err, pgx.ErrNoRows) { + t.Errorf("%s-expired session: err = %v, want ErrNoRows", name, err) + } + } + + listed, err := q.ListSessionsForUser(context.Background(), user.ID) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(listed) != 1 { + t.Errorf("listed %d sessions, want only the live one", len(listed)) + } + + swept, err := q.GcDeleteExpiredSessions(context.Background()) + if err != nil { + t.Fatalf("gc: %v", err) + } + if swept != 2 { + t.Errorf("gc swept %d, want the 2 expired rows", swept) + } +} + +// Changing your password signs out every other device and keeps this one. +func TestHandleChangePassword_RevokesOtherSessions(t *testing.T) { + h, pool := testHandlers(t) + user := seedUser(t, pool, "alice", "hunter2", false) + current := seedSession(t, pool, user.ID, "192.0.2.1") + other := seedSession(t, pool, user.ID, "198.51.100.7") + + body := strings.NewReader(`{"current_password":"hunter2","new_password":"correct-horse"}`) + req := httptest.NewRequest(http.MethodPut, "/api/me/password", body) + req.Header.Set("Content-Type", "application/json") + req = withSession(req, user, current) + w := httptest.NewRecorder() + h.handleChangePassword(w, req) + + if w.Code != http.StatusNoContent { + t.Fatalf("status = %d body = %s", w.Code, w.Body.String()) + } + if !sessionExists(t, pool, current) { + t.Error("the device that changed the password was signed out") + } + if sessionExists(t, pool, other) { + t.Error("another device's session survived the password change") + } +} + +// A reset by email ends every session the account has. +func TestHandleResetPassword_RevokesAllSessions(t *testing.T) { + h, pool := testHandlers(t) + user := seedUser(t, pool, "alice", "hunter2", false) + s1 := seedSession(t, pool, user.ID, "192.0.2.1") + s2 := seedSession(t, pool, user.ID, "198.51.100.7") + + const token = "reset-token-for-session-lifecycle-test" + if _, err := pool.Exec(context.Background(), + `INSERT INTO password_resets (token, user_id, expires_at) VALUES ($1, $2, now() + interval '1 hour')`, + token, user.ID, + ); err != nil { + t.Fatalf("seed reset: %v", err) + } + + body := strings.NewReader(`{"token":"` + token + `","new_password":"correct-horse"}`) + req := httptest.NewRequest(http.MethodPost, "/api/auth/reset-password", body) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + h.handleResetPassword(w, req) + + if w.Code != http.StatusNoContent { + t.Fatalf("status = %d body = %s", w.Code, w.Body.String()) + } + if sessionExists(t, pool, s1) || sessionExists(t, pool, s2) { + t.Error("a session survived the password reset") + } +} diff --git a/internal/db/dbq/gc.sql.go b/internal/db/dbq/gc.sql.go index cb150262..957a6ef5 100644 --- a/internal/db/dbq/gc.sql.go +++ b/internal/db/dbq/gc.sql.go @@ -84,6 +84,22 @@ func (q *Queries) GcDeleteExpiredPasswordResets(ctx context.Context) (int64, err return result.RowsAffected(), nil } +const gcDeleteExpiredSessions = `-- name: GcDeleteExpiredSessions :execrows +DELETE FROM sessions +WHERE last_seen_at <= now() - interval '30 days' + OR created_at <= now() - interval '365 days' +` + +// Sessions past their idle (30 days) or absolute (1 year) limit. They already +// fail auth through GetSessionByTokenHash's filter; this only clears the rows. +func (q *Queries) GcDeleteExpiredSessions(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteExpiredSessions) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const gcExpireScrobbleQueueFailedRows = `-- name: GcExpireScrobbleQueueFailedRows :execrows DELETE FROM scrobble_queue WHERE status = 'failed' diff --git a/internal/db/dbq/sessions.sql.go b/internal/db/dbq/sessions.sql.go index 95c05d84..307af3e7 100644 --- a/internal/db/dbq/sessions.sql.go +++ b/internal/db/dbq/sessions.sql.go @@ -69,10 +69,38 @@ func (q *Queries) DeleteSessionForUser(ctx context.Context, arg DeleteSessionFor return result.RowsAffected(), nil } -const getSessionByTokenHash = `-- name: GetSessionByTokenHash :one -SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions WHERE token_hash = $1 +const deleteSessionsForUser = `-- name: DeleteSessionsForUser :execrows +DELETE FROM sessions WHERE user_id = $1 ` +// Every session the user has, the caller's included. A password reset uses +// it: whoever forgot the password is not signed in anywhere they trust, and +// whoever may have learned it must be signed out everywhere. +func (q *Queries) DeleteSessionsForUser(ctx context.Context, userID pgtype.UUID) (int64, error) { + result, err := q.db.Exec(ctx, deleteSessionsForUser, userID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const getSessionByTokenHash = `-- name: GetSessionByTokenHash :one +SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions +WHERE token_hash = $1 + AND last_seen_at > now() - interval '30 days' + AND created_at > now() - interval '365 days' +` + +// Expired sessions are invisible here, so they fail auth the moment they +// lapse rather than whenever the GC sweep next runs. Two limits: +// +// idle 30 days — a token nobody has used in a month is abandoned, and +// matches the web cookie's lifetime; +// absolute 1 year — even a token in daily use is re-issued yearly, so a +// stolen one that is being used quietly does not live +// forever. +// +// Keep in step with ListSessionsForUser and GcDeleteExpiredSessions. func (q *Queries) GetSessionByTokenHash(ctx context.Context, tokenHash []byte) (Session, error) { row := q.db.QueryRow(ctx, getSessionByTokenHash, tokenHash) var i Session @@ -127,11 +155,17 @@ func (q *Queries) InsertSession(ctx context.Context, arg InsertSessionParams) (S } const listSessionsForUser = `-- name: ListSessionsForUser :many -SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions WHERE user_id = $1 ORDER BY last_seen_at DESC +SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions +WHERE user_id = $1 + AND last_seen_at > now() - interval '30 days' + AND created_at > now() - interval '365 days' +ORDER BY last_seen_at DESC ` // Most-recently-active first: the row a user is most likely to act on is the // one that moved last, and an unfamiliar entry at the top is the alarm. +// Expired rows are left out: they no longer authenticate, so listing them +// as active would be wrong. Same limits as GetSessionByTokenHash. func (q *Queries) ListSessionsForUser(ctx context.Context, userID pgtype.UUID) ([]Session, error) { rows, err := q.db.Query(ctx, listSessionsForUser, userID) if err != nil { diff --git a/internal/db/queries/gc.sql b/internal/db/queries/gc.sql index 26c0c202..6934ca41 100644 --- a/internal/db/queries/gc.sql +++ b/internal/db/queries/gc.sql @@ -68,3 +68,10 @@ UPDATE system_playlist_runs DELETE FROM password_resets WHERE (used_at IS NOT NULL AND used_at < now() - INTERVAL '7 days') OR (used_at IS NULL AND expires_at < now() - INTERVAL '1 hour'); + +-- name: GcDeleteExpiredSessions :execrows +-- Sessions past their idle (30 days) or absolute (1 year) limit. They already +-- fail auth through GetSessionByTokenHash's filter; this only clears the rows. +DELETE FROM sessions +WHERE last_seen_at <= now() - interval '30 days' + OR created_at <= now() - interval '365 days'; diff --git a/internal/db/queries/sessions.sql b/internal/db/queries/sessions.sql index 9c809053..a17245fd 100644 --- a/internal/db/queries/sessions.sql +++ b/internal/db/queries/sessions.sql @@ -7,7 +7,18 @@ VALUES ($1, $2, $3, sqlc.arg(ip), sqlc.arg(ip)) RETURNING *; -- name: GetSessionByTokenHash :one -SELECT * FROM sessions WHERE token_hash = $1; +-- Expired sessions are invisible here, so they fail auth the moment they +-- lapse rather than whenever the GC sweep next runs. Two limits: +-- idle 30 days — a token nobody has used in a month is abandoned, and +-- matches the web cookie's lifetime; +-- absolute 1 year — even a token in daily use is re-issued yearly, so a +-- stolen one that is being used quietly does not live +-- forever. +-- Keep in step with ListSessionsForUser and GcDeleteExpiredSessions. +SELECT * FROM sessions +WHERE token_hash = $1 + AND last_seen_at > now() - interval '30 days' + AND created_at > now() - interval '365 days'; -- name: TouchSessionLastSeen :exec UPDATE sessions SET last_seen_at = now(), last_ip = $2 WHERE id = $1; @@ -15,7 +26,13 @@ UPDATE sessions SET last_seen_at = now(), last_ip = $2 WHERE id = $1; -- name: ListSessionsForUser :many -- Most-recently-active first: the row a user is most likely to act on is the -- one that moved last, and an unfamiliar entry at the top is the alarm. -SELECT * FROM sessions WHERE user_id = $1 ORDER BY last_seen_at DESC; +-- Expired rows are left out: they no longer authenticate, so listing them +-- as active would be wrong. Same limits as GetSessionByTokenHash. +SELECT * FROM sessions +WHERE user_id = $1 + AND last_seen_at > now() - interval '30 days' + AND created_at > now() - interval '365 days' +ORDER BY last_seen_at DESC; -- name: DeleteSession :exec DELETE FROM sessions WHERE id = $1; @@ -34,3 +51,9 @@ DELETE FROM sessions WHERE id = $1 AND user_id = $2; -- "Log out everywhere else." Excludes the caller's own session so the action -- doesn't log them out of the page they just used to invoke it. DELETE FROM sessions WHERE user_id = $1 AND id <> $2; + +-- name: DeleteSessionsForUser :execrows +-- Every session the user has, the caller's included. A password reset uses +-- it: whoever forgot the password is not signed in anywhere they trust, and +-- whoever may have learned it must be signed out everywhere. +DELETE FROM sessions WHERE user_id = $1; diff --git a/internal/gc/worker.go b/internal/gc/worker.go index 39ca9a3c..e0d31068 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -16,6 +16,7 @@ // - GcExpireScrobbleQueueFailedRows (#567) // - GcResetStuckSystemPlaylistRuns (#574) // - GcDeleteExpiredPasswordResets (#575) +// - GcDeleteExpiredSessions (M462 #4978 — idle 30d / absolute 1y) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) // - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) // - GcDeleteOrphanedCandidateArtistTags(+State) (#2376 — the similarity @@ -86,6 +87,7 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "expire_scrobble_failed", q.GcExpireScrobbleQueueFailedRows) w.runSweep(ctx, "reset_stuck_system_runs", q.GcResetStuckSystemPlaylistRuns) w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) + w.runSweep(ctx, "delete_expired_sessions", q.GcDeleteExpiredSessions) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) // Tags before state: if the process dies between the two, a candidate left diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 17fbabc5..30fb0aef 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -153,7 +153,7 @@ passwordSaving = true; try { await changePassword(passwordForm.current, passwordForm.new); - pushToast('Password changed.'); + pushToast('Password changed. Your other devices have been signed out.'); passwordForm = { current: '', new: '', confirm: '' }; } catch (e: unknown) { const code = errCode(e);