From 2a7d6b463cd37b52706528b7302692b798867128 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sat, 14 Oct 2023 15:14:40 +0000 Subject: [PATCH 1/2] Unify password changing and invalidate auth tokens. --- cmd/admin_user_change_password.go | 6 ++---- models/auth/auth_token.go | 5 +++++ routers/api/v1/admin/user.go | 15 ++++++--------- routers/web/admin/users.go | 8 ++------ routers/web/auth/password.go | 20 ++++---------------- routers/web/user/setting/account.go | 11 ++++------- services/auth/source/db/authenticate.go | 5 +---- services/user/user.go | 16 ++++++++++++++++ 8 files changed, 40 insertions(+), 46 deletions(-) diff --git a/cmd/admin_user_change_password.go b/cmd/admin_user_change_password.go index eebbfb3b6710..9e964e47b0d5 100644 --- a/cmd/admin_user_change_password.go +++ b/cmd/admin_user_change_password.go @@ -11,6 +11,7 @@ import ( user_model "code.gitea.io/gitea/models/user" pwd "code.gitea.io/gitea/modules/auth/password" "code.gitea.io/gitea/modules/setting" + user_service "code.gitea.io/gitea/services/user" "github.com/urfave/cli/v2" ) @@ -65,11 +66,8 @@ func runChangePassword(c *cli.Context) error { if err != nil { return err } - if err = user.SetPassword(c.String("password")); err != nil { - return err - } - if err = user_model.UpdateUserCols(ctx, user, "passwd", "passwd_hash_algo", "salt"); err != nil { + if err := user_service.ChangePassword(ctx, user, c.String("password"), user.MustChangePassword); err != nil { return err } diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 65f1b169eb2a..81f07d1a8382 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -54,6 +54,11 @@ func DeleteAuthTokenByID(ctx context.Context, id string) error { return err } +func DeleteAuthTokensByUserID(ctx context.Context, uid int64) error { + _, err := db.GetEngine(ctx).Where(builder.Eq{"user_id": uid}).Delete(&AuthToken{}) + return err +} + func DeleteExpiredAuthTokens(ctx context.Context) error { _, err := db.GetEngine(ctx).Where(builder.Lt{"expires_unix": timeutil.TimeStampNow()}).Delete(&AuthToken{}) return err diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 09d7c1a9403a..9ec4544bbc98 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -185,6 +185,10 @@ func EditUser(ctx *context.APIContext) { return } + if form.MustChangePassword != nil { + ctx.ContextUser.MustChangePassword = *form.MustChangePassword + } + if len(form.Password) != 0 { if len(form.Password) < setting.MinPasswordLength { ctx.Error(http.StatusBadRequest, "PasswordTooShort", fmt.Errorf("password must be at least %d characters", setting.MinPasswordLength)) @@ -203,20 +207,13 @@ func EditUser(ctx *context.APIContext) { ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned")) return } - if ctx.ContextUser.Salt, err = user_model.GetUserSalt(); err != nil { - ctx.Error(http.StatusInternalServerError, "UpdateUser", err) - return - } - if err = ctx.ContextUser.SetPassword(form.Password); err != nil { + + if err := user_service.ChangePassword(ctx, ctx.ContextUser, form.Password, ctx.ContextUser.MustChangePassword); err != nil { ctx.InternalServerError(err) return } } - if form.MustChangePassword != nil { - ctx.ContextUser.MustChangePassword = *form.MustChangePassword - } - ctx.ContextUser.LoginName = form.LoginName if form.FullName != nil { diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 91a578fb5548..daadaf26bf97 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -381,12 +381,8 @@ func EditUserPost(ctx *context.Context) { return } - if u.Salt, err = user_model.GetUserSalt(); err != nil { - ctx.ServerError("UpdateUser", err) - return - } - if err = u.SetPassword(form.Password); err != nil { - ctx.ServerError("SetPassword", err) + if err := user_service.ChangePassword(ctx, u, form.Password, u.MustChangePassword); err != nil { + ctx.ServerError("ChangePassword", err) return } } diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index bdfa8c40258f..ad5b0fb14354 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -228,13 +228,8 @@ func ResetPasswdPost(ctx *context.Context) { ctx.ServerError("UpdateUser", err) return } - if err = u.SetPassword(passwd); err != nil { - ctx.ServerError("UpdateUser", err) - return - } - u.MustChangePassword = false - if err := user_model.UpdateUserCols(ctx, u, "must_change_password", "passwd", "passwd_hash_algo", "rands", "salt"); err != nil { - ctx.ServerError("UpdateUser", err) + if err := user_service.ChangePassword(ctx, u, passwd, false); err != nil { + ctx.ServerError("ChangePassword", err) return } @@ -321,15 +316,8 @@ func MustChangePasswordPost(ctx *context.Context) { return } - if err = u.SetPassword(form.Password); err != nil { - ctx.ServerError("UpdateUser", err) - return - } - - u.MustChangePassword = false - - if err := user_model.UpdateUserCols(ctx, u, "must_change_password", "passwd", "passwd_hash_algo", "salt"); err != nil { - ctx.ServerError("UpdateUser", err) + if err := user_service.ChangePassword(ctx, u, form.Password, false); err != nil { + ctx.ServerError("ChangePassword", err) return } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 5c14f3ad4b52..dea091888eaf 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -69,16 +69,13 @@ func AccountPost(ctx *context.Context) { } ctx.Flash.Error(errMsg) } else { - var err error - if err = ctx.Doer.SetPassword(form.Password); err != nil { - ctx.ServerError("UpdateUser", err) - return - } - if err := user_model.UpdateUserCols(ctx, ctx.Doer, "salt", "passwd_hash_algo", "passwd"); err != nil { - ctx.ServerError("UpdateUser", err) + if err := user_service.ChangePassword(ctx, ctx.Doer, form.Password, false); err != nil { + ctx.ServerError("ChangePassword", err) return } + log.Trace("User password updated: %s", ctx.Doer.Name) + ctx.Flash.Success(ctx.Tr("settings.change_password_success")) } diff --git a/services/auth/source/db/authenticate.go b/services/auth/source/db/authenticate.go index 8160141863a1..84768115d0ed 100644 --- a/services/auth/source/db/authenticate.go +++ b/services/auth/source/db/authenticate.go @@ -58,10 +58,7 @@ func Authenticate(ctx context.Context, user *user_model.User, login, password st // Or update the password when the salt length doesn't match the current // recommended salt length, this in order to migrate user's salts to a more secure salt. if user.PasswdHashAlgo != setting.PasswordHashAlgo || len(user.Salt) != user_model.SaltByteLength*2 { - if err := user.SetPassword(password); err != nil { - return nil, err - } - if err := user_model.UpdateUserCols(ctx, user, "passwd", "passwd_hash_algo", "salt"); err != nil { + if err := user_service.ChangePassword(ctx, user, password, user.MustChangePassword); err != nil { return nil, err } } diff --git a/services/user/user.go b/services/user/user.go index b95a7e0639d0..72cf7fab3153 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" packages_model "code.gitea.io/gitea/models/packages" @@ -120,6 +121,21 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string) err return nil } +// ChangePassword sets the users password and invalidates all existing auth tokens +func ChangePassword(ctx context.Context, u *user_model.User, password string, mustChangePassword bool) error { + if err := u.SetPassword(password); err != nil { + return err + } + + u.MustChangePassword = mustChangePassword + + if err := user_model.UpdateUserCols(ctx, u, "must_change_password", "passwd", "passwd_hash_algo", "rands", "salt"); err != nil { + return err + } + + return auth_model.DeleteAuthTokensByUserID(ctx, u.ID) +} + // DeleteUser completely and permanently deletes everything of a user, // but issues/comments/pulls will be kept and shown as someone has been deleted, // unless the user is younger than USER_DELETE_WITH_COMMENTS_MAX_DAYS. From 9c30760ed7578ffda555a661b9d23e090c894b9e Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sat, 14 Oct 2023 15:42:56 +0000 Subject: [PATCH 2/2] Delete auth tokens when deleting user. --- services/user/delete.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/user/delete.go b/services/user/delete.go index 01e3c37b39f3..41cfedd47d83 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -183,6 +183,10 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) } // ***** END: ExternalLoginUser ***** + if err := auth_model.DeleteAuthTokensByUserID(ctx, u.ID); err != nil { + return fmt.Errorf("DeleteAuthTokensByUserID: %w", err) + } + if _, err = db.DeleteByID(ctx, u.ID, new(user_model.User)); err != nil { return fmt.Errorf("delete: %w", err) }