From be1f9cb8b621107be997be95ff67916554a9e6e0 Mon Sep 17 00:00:00 2001 From: Gary Date: Tue, 10 Nov 2026 23:05:45 +0100 Subject: [PATCH] fix(sessions): a password change ends the sessions opened before it A refresh token is good for seven days and for a new pair each time, and the refresh never read the account again: a copy of a token went on working after the password was changed, and after the account was gone. The account now numbers its sessions (SessionVersion). The refresh token carries the number the row had when it was made, in the token's ID: Common's claims have no field for it, and every service reads them. The refresh reads the account again and answers 401 when it is gone or at another session. - A password change, and the reset from the command line, add one to the number: every session opened before ends. - The password change answers with the body of a login, so that the session that made the change goes on with a pair of the new session. - A new account starts at a number of its own, not at 0: the database gives the id of an account that is gone to the next one made, and a token of the first is not of the second. - A token made before sessions were numbered says nothing and is of session 0, like the accounts that were there: an update signs nobody out. - Saving a profile leaves the number alone. --- go.mod | 2 +- route/v1/user.go | 57 ++++++------ route/v1_2fa_test.go | 9 +- route/v1_sessions_test.go | 176 ++++++++++++++++++++++++++++++++++++++ service/model/o_user.go | 8 ++ service/user.go | 48 ++++++++++- 6 files changed, 267 insertions(+), 33 deletions(-) create mode 100644 route/v1_sessions_test.go diff --git a/go.mod b/go.mod index 2cb3cd8..b971d49 100644 --- a/go.mod +++ b/go.mod @@ -33,7 +33,7 @@ require ( github.com/go-openapi/jsonpointer v0.22.5 // indirect github.com/go-openapi/swag/jsonname v0.25.5 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect - github.com/golang-jwt/jwt/v4 v4.5.2 // indirect + github.com/golang-jwt/jwt/v4 v4.5.2 github.com/golang-jwt/jwt/v5 v5.3.0 // indirect github.com/google/uuid v1.5.0 // indirect github.com/gorilla/mux v1.8.0 // indirect diff --git a/route/v1/user.go b/route/v1/user.go index d729430..ddce149 100644 --- a/route/v1/user.go +++ b/route/v1/user.go @@ -236,26 +236,28 @@ func PostUserLogin(ctx echo.Context) error { return issueTokens(ctx, user) } -// issueTokens mints the access and refresh tokens and builds the login body; -// shared by /login (2FA off) and /2fa/verify (2FA on). -func issueTokens(ctx echo.Context, user model2.UserDBModel) error { +// newTokens is the pair of a session of user: an access token, three hours, and the refresh +// token of the session the row is at. +func newTokens(user model2.UserDBModel) (system_model.VerifyInformation, error) { privateKey, _ := service.MyService.User().GetKeyPair() - - token := system_model.VerifyInformation{} - - accessToken, err := jwt.GetAccessToken(user.Username, privateKey, user.Id) + access, err := jwt.GetAccessToken(user.Username, privateKey, user.Id) if err != nil { - return ctx.JSON(http.StatusInternalServerError, model.Result{Success: common_err.SERVICE_ERROR, Message: err.Error()}) + return system_model.VerifyInformation{}, err } - token.AccessToken = accessToken + refresh, err := service.MyService.User().RefreshToken(user) + if err != nil { + return system_model.VerifyInformation{}, err + } + return system_model.VerifyInformation{AccessToken: access, RefreshToken: refresh, ExpiresAt: time.Now().Add(3 * time.Hour).Unix()}, nil +} - refreshToken, err := jwt.GetRefreshToken(user.Username, privateKey, user.Id) +// issueTokens mints the access and refresh tokens and builds the login body; +// shared by /login (2FA off) and /2fa/verify (2FA on). +func issueTokens(ctx echo.Context, user model2.UserDBModel) error { + token, err := newTokens(user) if err != nil { return ctx.JSON(http.StatusInternalServerError, model.Result{Success: common_err.SERVICE_ERROR, Message: err.Error()}) } - token.RefreshToken = refreshToken - - token.ExpiresAt = time.Now().Add(3 * time.Hour * time.Duration(1)).Unix() data := make(map[string]interface{}, 2) user.Password = "" data["token"] = token @@ -431,8 +433,14 @@ func PutUserPassword(ctx echo.Context) error { if err := service.MyService.User().UpdateUserPassword(user); err != nil { return ctx.JSON(common_err.SERVICE_ERROR, model.Result{Success: common_err.SERVICE_ERROR, Message: common_err.GetMsg(common_err.SERVICE_ERROR), Data: err.Error()}) } - user.Password = "" - return ctx.JSON(common_err.SUCCESS, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: user}) + // The change ended every session of the account, this one with them: it goes on with the + // pair made here, of the session the row is at now. The body is the one of a login. + user = service.MyService.User().GetUserAllInfoById(id) + if user.Id == 0 { // gone since the change: no pair for an account that is not there + return ctx.JSON(common_err.SERVICE_ERROR, + model.Result{Success: common_err.USER_NOT_EXIST, Message: common_err.GetMsg(common_err.USER_NOT_EXIST)}) + } + return issueTokens(ctx, user) } // @Summary edit user nick @@ -827,8 +835,6 @@ func PostUserRefreshToken(ctx echo.Context) error { ctx.Bind(&js) refresh := js["refresh_token"] - privateKey, _ := service.MyService.User().GetKeyPair() - claims, err := jwt.ParseToken( refresh, func() (*ecdsa.PublicKey, error) { @@ -842,22 +848,19 @@ func PostUserRefreshToken(ctx echo.Context) error { return ctx.JSON(http.StatusUnauthorized, model.Result{Success: common_err.VERIFICATION_FAILURE, Message: common_err.GetMsg(common_err.VERIFICATION_FAILURE)}) } - newAccessToken, err := jwt.GetAccessToken(claims.Username, privateKey, claims.ID) - if err != nil { - return ctx.JSON(http.StatusInternalServerError, model.Result{Success: common_err.SERVICE_ERROR, Message: err.Error()}) + // The token is only a signature: the account is read again. It may be gone, a password + // change since the token was made ended its session, and an account made since under the + // same id has sessions of its own. + user := service.MyService.User().GetUserAllInfoById(strconv.Itoa(claims.ID)) + if user.Id == 0 || service.SessionOf(claims) != user.SessionVersion { + return ctx.JSON(http.StatusUnauthorized, model.Result{Success: common_err.VERIFICATION_FAILURE, Message: common_err.GetMsg(common_err.VERIFICATION_FAILURE)}) } - newRefreshToken, err := jwt.GetRefreshToken(claims.Username, privateKey, claims.ID) + verifyInfo, err := newTokens(user) if err != nil { return ctx.JSON(http.StatusInternalServerError, model.Result{Success: common_err.SERVICE_ERROR, Message: err.Error()}) } - verifyInfo := system_model.VerifyInformation{ - AccessToken: newAccessToken, - RefreshToken: newRefreshToken, - ExpiresAt: time.Now().Add(3 * time.Hour).Unix(), - } - return ctx.JSON(common_err.SUCCESS, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: verifyInfo}) } diff --git a/route/v1_2fa_test.go b/route/v1_2fa_test.go index e0cc2ac..58a24a6 100644 --- a/route/v1_2fa_test.go +++ b/route/v1_2fa_test.go @@ -124,6 +124,13 @@ func wrongCode(t *testing.T, secret string) string { // the per-user limiters are process-wide, so the map is reset here: a // budget spent on an id in one test is not inherited by the next. func newRig(t *testing.T) (service.UserService, client) { + t.Helper() + users, c, _ := newRigDB(t) + return users, c +} + +// newRigDB is newRig, with the database for a test that needs a row as an older version left it. +func newRigDB(t *testing.T) (service.UserService, client, *gorm.DB) { t.Helper() logger.LogInitConsoleOnly() // the handlers log; a test run on its own has no logger otherwise db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -141,7 +148,7 @@ func newRig(t *testing.T) (service.UserService, client) { v1.ResetLoginLimiters() v1.UserLimit = rate.Inf v1.ResetUserLimiters() - return users, client{t: t, h: route.InitRouter()} + return users, client{t: t, h: route.InitRouter()}, db } // TestLimiterResetPerRig: a budget exhausted on user id 1 in one rig is not diff --git a/route/v1_sessions_test.go b/route/v1_sessions_test.go new file mode 100644 index 0000000..de1411a --- /dev/null +++ b/route/v1_sessions_test.go @@ -0,0 +1,176 @@ +package route_test + +import ( + "net/http" + "strconv" + "testing" + "time" + + "github.com/ReCasaOS/CasaOS-Common/utils/jwt" + "github.com/ReCasaOS/CasaOS-UserService/pkg/utils/encryption" + model2 "github.com/ReCasaOS/CasaOS-UserService/service/model" + gojwt "github.com/golang-jwt/jwt/v4" +) + +func refreshStatus(c client, refresh string) (int, result) { + return c.do("POST", "/v1/users/refresh", "", map[string]string{"refresh_token": refresh}) +} + +// A refresh token is good for seven days and for a new pair each time, so a copy of one outlived +// everything its owner could do. Changing the password ends every session opened before it; the +// one that changed it goes on with the pair the change hands back. +func TestARefreshTokenStopsWorkingWhenThePasswordChanges(t *testing.T) { + users, c := newRig(t) + const password, changed = "correct horse", "battery staple" + users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.HashPassword(password), Role: "admin"}) + + access, copied := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password})) + // the copy is used once: the pair it gives is as old as the session + status, res := refreshStatus(c, copied) + if status != http.StatusOK { + t.Fatalf("a refresh before the change: %d", status) + } + renewed, _ := res.Data["refresh_token"].(string) + if renewed == "" { + t.Fatalf("no refresh token in %v", res.Data) + } + + data := c.expect(200, 200, "PUT", "/v1/users/current/password", access, map[string]string{"old_password": password, "password": changed}) + _, mine := tokens(t, data) + if user, _ := data["user"].(map[string]interface{}); user["username"] != "admin" || user["password"] != nil { + t.Fatalf("the user of the answer: %v", data["user"]) + } + + for name, token := range map[string]string{"the copied token": copied, "the one it was renewed into": renewed} { + if status, _ := refreshStatus(c, token); status != http.StatusUnauthorized { + t.Errorf("%s after the password changed: %d, want 401", name, status) + } + } + status, res = refreshStatus(c, mine) + if status != http.StatusOK { + t.Fatalf("the session that changed the password: %d, want 200", status) + } + // and what it is renewed into goes on working + if next, _ := res.Data["refresh_token"].(string); next == "" { + t.Fatalf("no refresh token in %v", res.Data) + } else if status, _ := refreshStatus(c, next); status != http.StatusOK { + t.Fatalf("the renewed token of that session: %d, want 200", status) + } + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": changed}) +} + +func TestARefreshTokenStopsWorkingWhenTheAccountIsGone(t *testing.T) { + users, c := newRig(t) + const password = "correct horse" + user := users.CreateUser(model2.UserDBModel{Username: "guest", Password: encryption.HashPassword(password), Role: "admin"}) + _, refresh := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "guest", "password": password})) + + users.DeleteUserById(strconv.Itoa(user.Id)) + + if status, _ := refreshStatus(c, refresh); status != http.StatusUnauthorized { + t.Fatalf("the token of an account that is gone: %d, want 401", status) + } +} + +// A box that updates keeps its sessions: a token made before this change says nothing of the +// session it belongs to, and is one of the sessions opened before any password change. +func TestARefreshTokenMadeBeforeSessionsWereCountedStillWorks(t *testing.T) { + users, c, db := newRigDB(t) + const password = "correct horse" + user := users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.HashPassword(password), Role: "admin"}) + // the row as the update finds it: the column is added with 0 in it + if err := db.Exec("UPDATE o_users SET session_version = 0").Error; err != nil { + t.Fatal(err) + } + private, _ := users.GetKeyPair() + old, err := jwt.GetRefreshToken("admin", private, user.Id) + if err != nil { + t.Fatal(err) + } + + if status, _ := refreshStatus(c, old); status != http.StatusOK { + t.Fatalf("a token of before the update: %d, want 200", status) + } + + access, _ := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password})) + c.expect(200, 200, "PUT", "/v1/users/current/password", access, map[string]string{"old_password": password, "password": "battery staple"}) + if status, _ := refreshStatus(c, old); status != http.StatusUnauthorized { + t.Fatalf("a token of before the update, after the password changed: %d, want 401", status) + } +} + +// The reset from the command line is what an owner runs when the account is not theirs any more: +// it ends the sessions too. +func TestARefreshTokenStopsWorkingWhenTheCredentialsAreReset(t *testing.T) { + users, c := newRig(t) + const password = "correct horse" + user := users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.HashPassword(password), Role: "admin"}) + _, refresh := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password})) + + user.Password = encryption.HashPassword("battery staple") + if err := users.ResetUserCredentials(user); err != nil { + t.Fatal(err) + } + + if status, _ := refreshStatus(c, refresh); status != http.StatusUnauthorized { + t.Fatalf("a token of before the reset: %d, want 401", status) + } + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "battery staple"}) +} + +// The database gives the number of an account that is gone to the next one that is made. A +// token of the first, an hour old, must not open the second: a token is never older than its +// account. +func TestARefreshTokenOfAnAccountThatIsGoneDoesNotOpenTheNextOne(t *testing.T) { + users, c := newRig(t) + gone := users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.HashPassword("correct horse"), Role: "admin"}) + private, _ := users.GetKeyPair() + issued := time.Now().Add(-time.Hour) + old, err := gojwt.NewWithClaims(gojwt.SigningMethodES256, jwt.Claims{Username: "admin", ID: gone.Id, RegisteredClaims: gojwt.RegisteredClaims{ + ExpiresAt: gojwt.NewNumericDate(issued.Add(7 * 24 * time.Hour)), + IssuedAt: gojwt.NewNumericDate(issued), + NotBefore: gojwt.NewNumericDate(issued), + Issuer: "refresh", + ID: "0", + }}).SignedString(private) + if err != nil { + t.Fatal(err) + } + + users.DeleteUserById(strconv.Itoa(gone.Id)) + next := users.CreateUser(model2.UserDBModel{Username: "owner", Password: encryption.HashPassword("battery staple"), Role: "admin"}) + if next.Id != gone.Id { + t.Skipf("the next account got another number (%d after %d): nothing to tell apart", next.Id, gone.Id) + } + + if status, _ := refreshStatus(c, old); status != http.StatusUnauthorized { + t.Fatalf("the token of the account that was there before: %d, want 401", status) + } + // and the next account's own session works + _, refresh := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "owner", "password": "battery staple"})) + if status, _ := refreshStatus(c, refresh); status != http.StatusOK { + t.Fatalf("the session of the next account: %d, want 200", status) + } +} + +// Saving the profile from a row read earlier must not put the sessions back to where they were. +func TestSavingAProfileDoesNotBringEndedSessionsBack(t *testing.T) { + users, c := newRig(t) + const password = "correct horse" + user := users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.HashPassword(password), Role: "admin"}) + if err := users.UpdateUserPassword(user); err != nil { // the row is at session 1 + t.Fatal(err) + } + _, copied := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password})) + read := users.GetUserAllInfoById(strconv.Itoa(user.Id)) // read while the copied token is good + if err := users.UpdateUserPassword(read); err != nil { // session 2: the copied token is ended + t.Fatal(err) + } + + read.Nickname = "Ada" + users.UpdateUser(read) + + if status, _ := refreshStatus(c, copied); status != http.StatusUnauthorized { + t.Fatalf("an ended session, after the profile was saved from an older read: %d, want 401", status) + } +} diff --git a/service/model/o_user.go b/service/model/o_user.go index c786239..d33e182 100644 --- a/service/model/o_user.go +++ b/service/model/o_user.go @@ -33,6 +33,14 @@ type UserDBModel struct { TotpEnabled bool `gorm:"default:false" json:"totp_enabled"` TotpLastStep int64 `gorm:"default:0" json:"-"` // last accepted 30 s step (replay guard) RecoveryCodes []string `gorm:"serializer:json" json:"-"` // bcrypt hashes, removed as consumed + + // SessionVersion numbers the sessions of the account: a refresh token carries the number the + // row had when it was made, and is refused once the row has another. A password change adds + // one, which ends every session opened before it. A new account starts at the instant it is + // made (CreateUser), not at 0: the database gives the id of an account that is gone to the + // next one, and a token of the first must not be of the second's session. An account from + // before sessions were numbered is at 0, like its tokens, which say nothing. + SessionVersion int64 `gorm:"default:0" json:"-"` } func (p *UserDBModel) TableName() string { diff --git a/service/user.go b/service/user.go index 9683c90..7e04528 100644 --- a/service/user.go +++ b/service/user.go @@ -16,12 +16,14 @@ import ( "io" "mime/multipart" "os" + "strconv" "sync" "time" "github.com/ReCasaOS/CasaOS-Common/utils/jwt" "github.com/ReCasaOS/CasaOS-Common/utils/logger" "github.com/ReCasaOS/CasaOS-UserService/service/model" + gojwt "github.com/golang-jwt/jwt/v4" "go.uber.org/zap" "gorm.io/gorm" ) @@ -31,7 +33,10 @@ type UserService interface { CreateUser(m model.UserDBModel) model.UserDBModel GetUserCount() (userCount int64) UpdateUser(m model.UserDBModel) + // UpdateUserPassword stores the hash and ends the sessions opened before (SessionVersion) UpdateUserPassword(m model.UserDBModel) error + // RefreshToken makes a refresh token of the session the row is at; SessionOf reads it back + RefreshToken(m model.UserDBModel) (string, error) GetUserInfoById(id string) (m model.UserDBModel) GetUserAllInfoById(id string) (m model.UserDBModel) GetUserAllInfoByName(userName string) (m model.UserDBModel) @@ -40,7 +45,8 @@ type UserService interface { GetUserInfoByUserName(userName string) (m model.UserDBModel) GetAllUserName() (list []model.UserDBModel) UpdateUserTOTP(m model.UserDBModel) error - // ResetUserCredentials writes the password and the four 2FA columns in one UPDATE: both, or neither + // ResetUserCredentials writes the password and the four 2FA columns, and ends the sessions + // opened before, in one transaction: all, or none ResetUserCredentials(m model.UserDBModel) error ReplacePasswordHash(id int, current, replacement string) bool EnableUserTOTP(id int, secret string, step int64, hashes []string) bool @@ -89,6 +95,9 @@ func (u *userService) GetAllUserName() (list []model.UserDBModel) { } func (u *userService) CreateUser(m model.UserDBModel) model.UserDBModel { + if m.SessionVersion == 0 { + m.SessionVersion = time.Now().UnixNano() // see the field: no token of an earlier account is of this one + } u.db.Create(&m) return m } @@ -99,7 +108,8 @@ func (u *userService) GetUserCount() (userCount int64) { } func (u *userService) UpdateUser(m model.UserDBModel) { - u.db.Model(&m).Omit("password", "totp_secret", "totp_enabled", "totp_last_step", "recovery_codes").Updates(&m) + // session_version: a profile saved from a row read earlier must not bring ended sessions back + u.db.Model(&m).Omit("password", "totp_secret", "totp_enabled", "totp_last_step", "recovery_codes", "session_version").Updates(&m) } // UpdateUserTOTP writes the four 2FA columns, zero values included: Updates @@ -193,11 +203,41 @@ func (u *userService) UseRecoveryCode(id int, code string) bool { } func (u *userService) ResetUserCredentials(m model.UserDBModel) error { - return u.db.Model(&m).Select("password", "totp_secret", "totp_enabled", "totp_last_step", "recovery_codes").Updates(&m).Error + // one transaction: the credentials and the end of the sessions opened with the old ones + return u.db.Transaction(func(tx *gorm.DB) error { + if err := tx.Model(&m).Select("password", "totp_secret", "totp_enabled", "totp_last_step", "recovery_codes").Updates(&m).Error; err != nil { + return err + } + return tx.Model(&m).Update("session_version", gorm.Expr("session_version + 1")).Error + }) } func (u *userService) UpdateUserPassword(m model.UserDBModel) error { - return u.db.Model(&m).Update("password", m.Password).Error + return u.db.Model(&m).Updates(map[string]interface{}{ + "password": m.Password, + "session_version": gorm.Expr("session_version + 1"), + }).Error +} + +// RefreshToken is Common's GetRefreshToken, with the session of the row in the token's ID (jti): +// Common's claims have no field of their own for it, and every service reads them. +func (u *userService) RefreshToken(m model.UserDBModel) (string, error) { + now := time.Now() + claims := jwt.Claims{Username: m.Username, ID: m.Id, RegisteredClaims: gojwt.RegisteredClaims{ + ExpiresAt: gojwt.NewNumericDate(now.Add(7 * 24 * time.Hour)), + IssuedAt: gojwt.NewNumericDate(now), + NotBefore: gojwt.NewNumericDate(now), + Issuer: "refresh", + ID: strconv.FormatInt(m.SessionVersion, 10), + }} + return gojwt.NewWithClaims(gojwt.SigningMethodES256, claims).SignedString(u.privateKey) +} + +// SessionOf is the session a refresh token belongs to. A token made before sessions were +// numbered says nothing, and is of the first: a box that updates keeps its sessions. +func SessionOf(claims *jwt.Claims) int64 { + session, _ := strconv.ParseInt(claims.RegisteredClaims.ID, 10, 64) + return session } func (u *userService) GetUserAllInfoById(id string) (m model.UserDBModel) {