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 <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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'
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user