From cdd32a1307098120221da7a3213951a524396967 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 11 Apr 2023 18:46:28 +0800 Subject: [PATCH 01/10] refactor rename user and rename organization --- models/repo/repo.go | 8 ++ models/user/error.go | 25 +++-- models/user/user.go | 45 --------- routers/api/v1/admin/user.go | 6 +- routers/web/admin/users.go | 2 +- routers/web/org/setting.go | 36 +++---- routers/web/user/setting/profile.go | 17 ++-- services/org/org.go | 95 ++++++++++++++++++- services/user/avatar.go | 65 +++++++++++++ services/user/rename.go | 41 -------- services/user/user.go | 141 +++++++++++++++++----------- 11 files changed, 291 insertions(+), 190 deletions(-) create mode 100644 services/user/avatar.go delete mode 100644 services/user/rename.go diff --git a/models/repo/repo.go b/models/repo/repo.go index 3653dae015d6b..1b9539bc249c6 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -815,3 +815,11 @@ func FixNullArchivedRepository(ctx context.Context) (int64, error) { IsArchived: false, }) } + +// UpdateRepositoryOwnerName updates the owner name of all repositories owned by the user +func UpdateRepositoryOwnerName(ctx context.Context, oldUserName, newUserName string) error { + if _, err := db.GetEngine(ctx).Exec("UPDATE `repository` SET owner_name=? WHERE owner_name=?", newUserName, oldUserName); err != nil { + return fmt.Errorf("change repo owner name: %w", err) + } + return nil +} diff --git a/models/user/error.go b/models/user/error.go index 306b9ee9d9b8f..2fd435174158b 100644 --- a/models/user/error.go +++ b/models/user/error.go @@ -9,13 +9,6 @@ import ( "code.gitea.io/gitea/modules/util" ) -// ____ ___ -// | | \______ ___________ -// | | / ___// __ \_ __ \ -// | | /\___ \\ ___/| | \/ -// |______//____ >\___ >__| -// \/ \/ - // ErrUserAlreadyExist represents a "user already exists" error. type ErrUserAlreadyExist struct { Name string @@ -95,7 +88,23 @@ func (err ErrUserInactive) Error() string { return fmt.Sprintf("user is inactive [uid: %d, name: %s]", err.UID, err.Name) } -// Unwrap unwraps this error as a ErrPermission error +// Unwrap unwraps this error as a ErrUserInactive error func (err ErrUserInactive) Unwrap() error { return util.ErrPermissionDenied } + +// ErrUserIsNotLocal represents a "ErrUserIsNotLocal" kind of error. +type ErrUserIsNotLocal struct { + UID int64 + Name string +} + +func (err ErrUserIsNotLocal) Error() string { + return fmt.Sprintf("user is not local type [uid: %d, name: %s]", err.UID, err.Name) +} + +// IsErrUserIsNotLocal +func IsErrUserIsNotLocal(err error) bool { + _, ok := err.(ErrUserIsNotLocal) + return ok +} diff --git a/models/user/user.go b/models/user/user.go index 5709ed7ff27e9..e9d2e2730abba 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -9,7 +9,6 @@ import ( "encoding/hex" "fmt" "net/url" - "os" "path/filepath" "strings" "time" @@ -742,50 +741,6 @@ func VerifyUserActiveCode(code string) (user *User) { return nil } -// ChangeUserName changes all corresponding setting from old user name to new one. -func ChangeUserName(ctx context.Context, u *User, newUserName string) (err error) { - oldUserName := u.Name - if err = IsUsableUsername(newUserName); err != nil { - return err - } - - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - isExist, err := IsUserExist(ctx, 0, newUserName) - if err != nil { - return err - } else if isExist { - return ErrUserAlreadyExist{newUserName} - } - - if _, err = db.GetEngine(ctx).Exec("UPDATE `repository` SET owner_name=? WHERE owner_name=?", newUserName, oldUserName); err != nil { - return fmt.Errorf("Change repo owner name: %w", err) - } - - // Do not fail if directory does not exist - if err = util.Rename(UserPath(oldUserName), UserPath(newUserName)); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("Rename user directory: %w", err) - } - - if err = NewUserRedirect(ctx, u.ID, oldUserName, newUserName); err != nil { - return err - } - - if err = committer.Commit(); err != nil { - if err2 := util.Rename(UserPath(newUserName), UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) { - log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldUserName, newUserName, err, err2) - return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldUserName, newUserName, err, err2) - } - return err - } - - return nil -} - // checkDupEmail checks whether there are the same email with the user func checkDupEmail(ctx context.Context, u *User) error { u.Email = strings.ToLower(u.Email) diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 8649a982b08ee..bc093f79501b3 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -505,15 +505,15 @@ func RenameUser(ctx *context.APIContext) { } newName := web.GetForm(ctx).(*api.RenameUserOption).NewName - - if strings.EqualFold(newName, ctx.ContextUser.Name) { + nameChanged := newName != ctx.ContextUser.Name + if !nameChanged { // Noop as username is not changed ctx.Status(http.StatusNoContent) return } // Check if user name has been changed - if err := user_service.RenameUser(ctx, ctx.ContextUser, newName); err != nil { + if err := user_service.RenameUser(ctx, ctx.ContextUser, newName, strings.EqualFold(newName, ctx.ContextUser.Name)); err != nil { switch { case user_model.IsErrUserAlreadyExist(err): ctx.Error(http.StatusUnprocessableEntity, "", ctx.Tr("form.username_been_taken")) diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 531f14d08627e..453f6f6596701 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -337,7 +337,7 @@ func EditUserPost(ctx *context.Context) { } if len(form.UserName) != 0 && u.Name != form.UserName { - if err := user_setting.HandleUsernameChange(ctx, u, form.UserName); err != nil { + if err := user_setting.HandleUsernameChange(ctx, u, form.UserName, u.LowerName == strings.ToLower(form.UserName)); err != nil { if ctx.Written() { return } diff --git a/routers/web/org/setting.go b/routers/web/org/setting.go index db8fc728dffc7..3535b8fdae211 100644 --- a/routers/web/org/setting.go +++ b/routers/web/org/setting.go @@ -23,7 +23,7 @@ import ( user_setting "code.gitea.io/gitea/routers/web/user/setting" "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/org" - container_service "code.gitea.io/gitea/services/packages/container" + org_service "code.gitea.io/gitea/services/org" repo_service "code.gitea.io/gitea/services/repository" user_service "code.gitea.io/gitea/services/user" ) @@ -70,31 +70,23 @@ func SettingsPost(ctx *context.Context) { nameChanged := org.Name != form.Name // Check if organization name has been changed. - if org.LowerName != strings.ToLower(form.Name) { - isExist, err := user_model.IsUserExist(ctx, org.ID, form.Name) - if err != nil { - ctx.ServerError("IsUserExist", err) - return - } else if isExist { + if nameChanged { + err := org_service.RenameOrganization(ctx, org, form.Name, org.LowerName == strings.ToLower(form.Name)) + switch { + case user_model.IsErrUserAlreadyExist(err): ctx.Data["OrgName"] = true ctx.RenderWithErr(ctx.Tr("form.username_been_taken"), tplSettingsOptions, &form) return - } else if err = user_model.ChangeUserName(ctx, org.AsUser(), form.Name); err != nil { - switch { - case db.IsErrNameReserved(err): - ctx.Data["OrgName"] = true - ctx.RenderWithErr(ctx.Tr("repo.form.name_reserved", err.(db.ErrNameReserved).Name), tplSettingsOptions, &form) - case db.IsErrNamePatternNotAllowed(err): - ctx.Data["OrgName"] = true - ctx.RenderWithErr(ctx.Tr("repo.form.name_pattern_not_allowed", err.(db.ErrNamePatternNotAllowed).Pattern), tplSettingsOptions, &form) - default: - ctx.ServerError("ChangeUserName", err) - } + case db.IsErrNameReserved(err): + ctx.Data["OrgName"] = true + ctx.RenderWithErr(ctx.Tr("repo.form.name_reserved", err.(db.ErrNameReserved).Name), tplSettingsOptions, &form) return - } - - if err := container_service.UpdateRepositoryNames(ctx, org.AsUser(), form.Name); err != nil { - ctx.ServerError("UpdateRepositoryNames", err) + case db.IsErrNamePatternNotAllowed(err): + ctx.Data["OrgName"] = true + ctx.RenderWithErr(ctx.Tr("repo.form.name_pattern_not_allowed", err.(db.ErrNamePatternNotAllowed).Pattern), tplSettingsOptions, &form) + return + case err != nil: + ctx.ServerError("org_service.RenameOrganization", err) return } diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index f500be7632336..0b897dca7536b 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -48,16 +48,14 @@ func Profile(ctx *context.Context) { } // HandleUsernameChange handle username changes from user settings and admin interface -func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName string) error { - // Non-local users are not allowed to change their username. - if !user.IsLocal() { - ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user")) - return fmt.Errorf(ctx.Tr("form.username_change_not_local_user")) - } - +func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName string, onlyCapitalization bool) error { // rename user - if err := user_service.RenameUser(ctx, user, newName); err != nil { + if err := user_service.RenameUser(ctx, user, newName, onlyCapitalization); err != nil { switch { + // Non-local users are not allowed to change their username. + case user_model.IsErrUserIsNotLocal(err): + ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user")) + return fmt.Errorf(ctx.Tr("form.username_change_not_local_user")) case user_model.IsErrUserAlreadyExist(err): ctx.Flash.Error(ctx.Tr("form.username_been_taken")) case user_model.IsErrEmailAlreadyUsed(err): @@ -90,7 +88,8 @@ func ProfilePost(ctx *context.Context) { if len(form.Name) != 0 && ctx.Doer.Name != form.Name { log.Debug("Changing name for %s to %s", ctx.Doer.Name, form.Name) - if err := HandleUsernameChange(ctx, ctx.Doer, form.Name); err != nil { + if err := HandleUsernameChange(ctx, ctx.Doer, form.Name, ctx.Doer.LowerName == strings.ToLower(form.Name)); err != nil { + ctx.Flash.Error(ctx.Tr("form.email_been_used")) ctx.Redirect(setting.AppSubURL + "/user/settings") return } diff --git a/services/org/org.go b/services/org/org.go index e45fb305debe8..cb2055d4a5ebd 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -4,20 +4,26 @@ package org import ( + "context" "fmt" + "os" + "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/organization" + org_model "code.gitea.io/gitea/models/organization" packages_model "code.gitea.io/gitea/models/packages" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/agit" + container_service "code.gitea.io/gitea/services/packages/container" ) // DeleteOrganization completely and permanently deletes everything of organization. -func DeleteOrganization(org *organization.Organization) error { +func DeleteOrganization(org *org_model.Organization) error { ctx, commiter, err := db.TxContext(db.DefaultContext) if err != nil { return err @@ -39,7 +45,7 @@ func DeleteOrganization(org *organization.Organization) error { return models.ErrUserOwnPackages{UID: org.ID} } - if err := organization.DeleteOrganization(ctx, org); err != nil { + if err := org_model.DeleteOrganization(ctx, org); err != nil { return fmt.Errorf("DeleteOrganization: %w", err) } @@ -53,15 +59,94 @@ func DeleteOrganization(org *organization.Organization) error { path := user_model.UserPath(org.Name) if err := util.RemoveAll(path); err != nil { - return fmt.Errorf("Failed to RemoveAll %s: %w", path, err) + return fmt.Errorf("failed to RemoveAll %s: %w", path, err) } if len(org.Avatar) > 0 { avatarPath := org.CustomAvatarRelativePath() if err := storage.Avatars.Delete(avatarPath); err != nil { - return fmt.Errorf("Failed to remove %s: %w", avatarPath, err) + return fmt.Errorf("failed to remove %s: %w", avatarPath, err) } } return nil } + +// RenameOrganization renames an organization. +func RenameOrganization(ctx context.Context, org *org_model.Organization, newName string, onlyCapitalization bool) error { + if !org.AsUser().IsOrganization() { + return fmt.Errorf("cannot rename user") + } + + if err := user_model.IsUsableUsername(newName); err != nil { + return err + } + + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + + oldName := org.Name + + if onlyCapitalization { + org.Name = newName + if err := user_model.UpdateUserCols(ctx, org.AsUser(), "name"); err != nil { + org.Name = oldName + return err + } + if err := committer.Commit(); err != nil { + org.Name = oldName + return err + } + return nil + } + + isExist, err := user_model.IsUserExist(ctx, org.ID, newName) + if err != nil { + return err + } + if isExist { + return user_model.ErrUserAlreadyExist{ + Name: newName, + } + } + + if err = repo_model.UpdateRepositoryOwnerName(ctx, oldName, newName); err != nil { + return err + } + + if err = user_model.NewUserRedirect(ctx, org.ID, oldName, newName); err != nil { + return err + } + + if err := agit.UserNameChanged(ctx, org.AsUser(), newName); err != nil { + return err + } + if err := container_service.UpdateRepositoryNames(ctx, org.AsUser(), newName); err != nil { + return err + } + + org.Name = newName + org.LowerName = strings.ToLower(newName) + if err := user_model.UpdateUserCols(ctx, org.AsUser(), "name", "lower_name"); err != nil { + return err + } + + // Do not fail if directory does not exist + if err = util.Rename(user_model.UserPath(oldName), user_model.UserPath(newName)); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("rename user directory: %w", err) + } + + if err = committer.Commit(); err != nil { + if err2 := util.Rename(user_model.UserPath(newName), user_model.UserPath(oldName)); err2 != nil && !os.IsNotExist(err2) { + log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldName, newName, err, err2) + return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldName, newName, err, err2) + } + return err + } + + log.Trace("Org name changed: %s -> %s", oldName, newName) + return nil +} diff --git a/services/user/avatar.go b/services/user/avatar.go new file mode 100644 index 0000000000000..1db5018739e46 --- /dev/null +++ b/services/user/avatar.go @@ -0,0 +1,65 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package user + +import ( + "fmt" + "image/png" + "io" + + "code.gitea.io/gitea/models/db" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/avatar" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/storage" +) + +// UploadAvatar saves custom avatar for user. +func UploadAvatar(u *user_model.User, data []byte) error { + m, err := avatar.Prepare(data) + if err != nil { + return err + } + + ctx, committer, err := db.TxContext(db.DefaultContext) + if err != nil { + return err + } + defer committer.Close() + + u.UseCustomAvatar = true + u.Avatar = avatar.HashAvatar(u.ID, data) + if err = user_model.UpdateUserCols(ctx, u, "use_custom_avatar", "avatar"); err != nil { + return fmt.Errorf("updateUser: %w", err) + } + + if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { + if err := png.Encode(w, *m); err != nil { + log.Error("Encode: %v", err) + } + return err + }); err != nil { + return fmt.Errorf("Failed to create dir %s: %w", u.CustomAvatarRelativePath(), err) + } + + return committer.Commit() +} + +// DeleteAvatar deletes the user's custom avatar. +func DeleteAvatar(u *user_model.User) error { + aPath := u.CustomAvatarRelativePath() + log.Trace("DeleteAvatar[%d]: %s", u.ID, aPath) + if len(u.Avatar) > 0 { + if err := storage.Avatars.Delete(aPath); err != nil { + return fmt.Errorf("Failed to remove %s: %w", aPath, err) + } + } + + u.UseCustomAvatar = false + u.Avatar = "" + if _, err := db.GetEngine(db.DefaultContext).ID(u.ID).Cols("avatar, use_custom_avatar").Update(u); err != nil { + return fmt.Errorf("UpdateUser: %w", err) + } + return nil +} diff --git a/services/user/rename.go b/services/user/rename.go deleted file mode 100644 index af195d7d76a26..0000000000000 --- a/services/user/rename.go +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright 2023 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package user - -import ( - "context" - "fmt" - "strings" - - user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/services/agit" - container_service "code.gitea.io/gitea/services/packages/container" -) - -func renameUser(ctx context.Context, u *user_model.User, newUserName string) error { - if u.IsOrganization() { - return fmt.Errorf("cannot rename organization") - } - - if err := user_model.ChangeUserName(ctx, u, newUserName); err != nil { - return err - } - - if err := agit.UserNameChanged(ctx, u, newUserName); err != nil { - return err - } - if err := container_service.UpdateRepositoryNames(ctx, u, newUserName); err != nil { - return err - } - - u.Name = newUserName - u.LowerName = strings.ToLower(newUserName) - if err := user_model.UpdateUser(ctx, u, false); err != nil { - return err - } - - log.Trace("User name changed: %s -> %s", u.Name, newUserName) - return nil -} diff --git a/services/user/user.go b/services/user/user.go index d52a2f404bcf0..39babac0480a1 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -6,8 +6,8 @@ package user import ( "context" "fmt" - "image/png" - "io" + "os" + "strings" "time" "code.gitea.io/gitea/models" @@ -18,29 +18,107 @@ import ( repo_model "code.gitea.io/gitea/models/repo" system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/avatar" "code.gitea.io/gitea/modules/eventsource" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/agit" "code.gitea.io/gitea/services/packages" + container_service "code.gitea.io/gitea/services/packages/container" ) // RenameUser renames a user -func RenameUser(ctx context.Context, u *user_model.User, newUserName string) error { +func RenameUser(ctx context.Context, u *user_model.User, newUserName string, onlyCapitalization bool) error { + if u.IsOrganization() { + return fmt.Errorf("cannot rename organization") + } + + // Non-local users are not allowed to change their username. + if !u.IsLocal() { + return user_model.ErrUserIsNotLocal{ + UID: u.ID, + Name: u.Name, + } + } + + if err := user_model.IsUsableUsername(newUserName); err != nil { + return err + } + ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - if err := renameUser(ctx, u, newUserName); err != nil { + + oldUserName := u.Name + + if onlyCapitalization { + u.Name = newUserName + if err := user_model.UpdateUserCols(ctx, u, "name"); err != nil { + u.Name = oldUserName + return err + } + if err := committer.Commit(); err != nil { + u.Name = oldUserName + return err + } + return nil + } + + isExist, err := user_model.IsUserExist(ctx, u.ID, newUserName) + if err != nil { return err } - if err := committer.Commit(); err != nil { + if isExist { + return user_model.ErrUserAlreadyExist{ + Name: newUserName, + } + } + + if err = repo_model.UpdateRepositoryOwnerName(ctx, oldUserName, newUserName); err != nil { + return err + } + + if err = user_model.NewUserRedirect(ctx, u.ID, oldUserName, newUserName); err != nil { + return err + } + + if err := agit.UserNameChanged(ctx, u, newUserName); err != nil { + return err + } + if err := container_service.UpdateRepositoryNames(ctx, u, newUserName); err != nil { + return err + } + + u.Name = newUserName + u.LowerName = strings.ToLower(newUserName) + if err := user_model.UpdateUserCols(ctx, u, "name", "lower_name"); err != nil { + u.Name = oldUserName + u.LowerName = strings.ToLower(oldUserName) return err } - return err + + // Do not fail if directory does not exist + if err = util.Rename(user_model.UserPath(oldUserName), user_model.UserPath(newUserName)); err != nil && !os.IsNotExist(err) { + u.Name = oldUserName + u.LowerName = strings.ToLower(oldUserName) + return fmt.Errorf("rename user directory: %w", err) + } + + if err = committer.Commit(); err != nil { + u.Name = oldUserName + u.LowerName = strings.ToLower(oldUserName) + if err2 := util.Rename(user_model.UserPath(newUserName), user_model.UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) { + log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldUserName, newUserName, err, err2) + return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldUserName, newUserName, err, err2) + } + return err + } + + log.Trace("User name changed: %s -> %s", oldUserName, newUserName) + return nil } // DeleteUser completely and permanently deletes everything of a user, @@ -241,52 +319,3 @@ func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error { return user_model.DeleteInactiveEmailAddresses(ctx) } - -// UploadAvatar saves custom avatar for user. -func UploadAvatar(u *user_model.User, data []byte) error { - m, err := avatar.Prepare(data) - if err != nil { - return err - } - - ctx, committer, err := db.TxContext(db.DefaultContext) - if err != nil { - return err - } - defer committer.Close() - - u.UseCustomAvatar = true - u.Avatar = avatar.HashAvatar(u.ID, data) - if err = user_model.UpdateUserCols(ctx, u, "use_custom_avatar", "avatar"); err != nil { - return fmt.Errorf("updateUser: %w", err) - } - - if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { - if err := png.Encode(w, *m); err != nil { - log.Error("Encode: %v", err) - } - return err - }); err != nil { - return fmt.Errorf("Failed to create dir %s: %w", u.CustomAvatarRelativePath(), err) - } - - return committer.Commit() -} - -// DeleteAvatar deletes the user's custom avatar. -func DeleteAvatar(u *user_model.User) error { - aPath := u.CustomAvatarRelativePath() - log.Trace("DeleteAvatar[%d]: %s", u.ID, aPath) - if len(u.Avatar) > 0 { - if err := storage.Avatars.Delete(aPath); err != nil { - return fmt.Errorf("Failed to remove %s: %w", aPath, err) - } - } - - u.UseCustomAvatar = false - u.Avatar = "" - if _, err := db.GetEngine(db.DefaultContext).ID(u.ID).Cols("avatar, use_custom_avatar").Update(u); err != nil { - return fmt.Errorf("UpdateUser: %w", err) - } - return nil -} From 7f5af9c012fd32ddf0599fae06e3390933827138 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 11 Apr 2023 22:04:06 +0800 Subject: [PATCH 02/10] Add integration test for admin rename user --- routers/web/org/setting.go | 3 +-- tests/integration/api_admin_test.go | 40 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/routers/web/org/setting.go b/routers/web/org/setting.go index 3535b8fdae211..213e4491354e1 100644 --- a/routers/web/org/setting.go +++ b/routers/web/org/setting.go @@ -22,7 +22,6 @@ import ( "code.gitea.io/gitea/modules/web" user_setting "code.gitea.io/gitea/routers/web/user/setting" "code.gitea.io/gitea/services/forms" - "code.gitea.io/gitea/services/org" org_service "code.gitea.io/gitea/services/org" repo_service "code.gitea.io/gitea/services/repository" user_service "code.gitea.io/gitea/services/user" @@ -181,7 +180,7 @@ func SettingsDelete(ctx *context.Context) { return } - if err := org.DeleteOrganization(ctx.Org.Organization); err != nil { + if err := org_service.DeleteOrganization(ctx.Org.Organization); err != nil { if models.IsErrUserOwnRepos(err) { ctx.Flash.Error(ctx.Tr("form.org_still_own_repo")) ctx.Redirect(ctx.Org.OrgLink + "/settings/delete") diff --git a/tests/integration/api_admin_test.go b/tests/integration/api_admin_test.go index b608c26f6ee50..aa9b5719e7f57 100644 --- a/tests/integration/api_admin_test.go +++ b/tests/integration/api_admin_test.go @@ -241,3 +241,43 @@ func TestAPICreateRepoForUser(t *testing.T) { ) MakeRequest(t, req, http.StatusCreated) } + +func TestAPIRenameUser(t *testing.T) { + defer tests.PrepareTestEnv(t)() + adminUsername := "user1" + token := getUserToken(t, adminUsername, auth_model.AccessTokenScopeSudo) + urlStr := fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "user2", token) + req := NewRequestWithValues(t, "POST", urlStr, map[string]string{ + // required + "new_username": "User2", + }) + MakeRequest(t, req, http.StatusOK) + + urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2", token) + req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ + // required + "new_username": "User2-2-2", + }) + MakeRequest(t, req, http.StatusOK) + + urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2", token) + req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ + // required + "new_username": "user1", + }) + MakeRequest(t, req, http.StatusNotFound) + + urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2-2-2", token) + req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ + // required + "new_username": "user1", + }) + MakeRequest(t, req, http.StatusUnprocessableEntity) + + urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2-2-2", token) + req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ + // required + "new_username": "user2", + }) + MakeRequest(t, req, http.StatusOK) +} From 07445ed0d9393615601315584a87a4d5d256b6fe Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 12 Apr 2023 22:23:42 +0800 Subject: [PATCH 03/10] Fix test --- tests/integration/api_admin_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/api_admin_test.go b/tests/integration/api_admin_test.go index aa9b5719e7f57..97a0112872d57 100644 --- a/tests/integration/api_admin_test.go +++ b/tests/integration/api_admin_test.go @@ -249,35 +249,35 @@ func TestAPIRenameUser(t *testing.T) { urlStr := fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "user2", token) req := NewRequestWithValues(t, "POST", urlStr, map[string]string{ // required - "new_username": "User2", + "new_name": "User2", }) MakeRequest(t, req, http.StatusOK) urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2", token) req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ // required - "new_username": "User2-2-2", + "new_name": "User2-2-2", }) MakeRequest(t, req, http.StatusOK) urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2", token) req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ // required - "new_username": "user1", + "new_name": "user1", }) MakeRequest(t, req, http.StatusNotFound) urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2-2-2", token) req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ // required - "new_username": "user1", + "new_name": "user1", }) MakeRequest(t, req, http.StatusUnprocessableEntity) urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2-2-2", token) req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ // required - "new_username": "user2", + "new_name": "user2", }) MakeRequest(t, req, http.StatusOK) } From 9edd5928aa8f9ffcdd6ae7ca902e41a0e509cc48 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 13 Apr 2023 16:42:38 +0800 Subject: [PATCH 04/10] Fix bug --- routers/api/v1/admin/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index bc093f79501b3..3000310c77336 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -528,5 +528,5 @@ func RenameUser(ctx *context.APIContext) { } return } - ctx.Status(http.StatusNoContent) + ctx.Status(http.StatusOK) } From 3889aee698a09508d8d22d0d837ef02c9aa5d727 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 17 Apr 2023 12:18:53 +0800 Subject: [PATCH 05/10] Fix test --- tests/integration/api_admin_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/api_admin_test.go b/tests/integration/api_admin_test.go index 97a0112872d57..cf4f17a5ab729 100644 --- a/tests/integration/api_admin_test.go +++ b/tests/integration/api_admin_test.go @@ -265,7 +265,8 @@ func TestAPIRenameUser(t *testing.T) { // required "new_name": "user1", }) - MakeRequest(t, req, http.StatusNotFound) + // the old user name still be used by with a redirect + MakeRequest(t, req, http.StatusTemporaryRedirect) urlStr = fmt.Sprintf("/api/v1/admin/users/%s/rename?token=%s", "User2-2-2", token) req = NewRequestWithValues(t, "POST", urlStr, map[string]string{ From 4f630eb576dc73ef7317aaa3eebd150f371ee916 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 20 Apr 2023 18:51:04 +0800 Subject: [PATCH 06/10] Fix test --- routers/web/user/setting/profile.go | 1 - 1 file changed, 1 deletion(-) diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index 0b897dca7536b..418b533e164dc 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -89,7 +89,6 @@ func ProfilePost(ctx *context.Context) { if len(form.Name) != 0 && ctx.Doer.Name != form.Name { log.Debug("Changing name for %s to %s", ctx.Doer.Name, form.Name) if err := HandleUsernameChange(ctx, ctx.Doer, form.Name, ctx.Doer.LowerName == strings.ToLower(form.Name)); err != nil { - ctx.Flash.Error(ctx.Tr("form.email_been_used")) ctx.Redirect(setting.AppSubURL + "/user/settings") return } From 7eeb6ca0c16012bef0c55a5d5e987012d4e870f2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 4 May 2023 12:22:45 +0800 Subject: [PATCH 07/10] Follow yp05327's suggestion --- routers/api/v1/admin/user.go | 3 +-- routers/web/user/setting/profile.go | 1 - services/org/org.go | 6 ++++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 3000310c77336..0ff84134debf9 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -505,8 +505,7 @@ func RenameUser(ctx *context.APIContext) { } newName := web.GetForm(ctx).(*api.RenameUserOption).NewName - nameChanged := newName != ctx.ContextUser.Name - if !nameChanged { + if newName == ctx.ContextUser.Name { // Noop as username is not changed ctx.Status(http.StatusNoContent) return diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index 418b533e164dc..0e3616220b4ba 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -55,7 +55,6 @@ func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName s // Non-local users are not allowed to change their username. case user_model.IsErrUserIsNotLocal(err): ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user")) - return fmt.Errorf(ctx.Tr("form.username_change_not_local_user")) case user_model.IsErrUserAlreadyExist(err): ctx.Flash.Error(ctx.Tr("form.username_been_taken")) case user_model.IsErrEmailAlreadyUsed(err): diff --git a/services/org/org.go b/services/org/org.go index cb2055d4a5ebd..8db783ff26e09 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -131,15 +131,21 @@ func RenameOrganization(ctx context.Context, org *org_model.Organization, newNam org.Name = newName org.LowerName = strings.ToLower(newName) if err := user_model.UpdateUserCols(ctx, org.AsUser(), "name", "lower_name"); err != nil { + org.Name = oldName + org.LowerName = strings.ToLower(oldName) return err } // Do not fail if directory does not exist if err = util.Rename(user_model.UserPath(oldName), user_model.UserPath(newName)); err != nil && !os.IsNotExist(err) { + org.Name = oldName + org.LowerName = strings.ToLower(oldName) return fmt.Errorf("rename user directory: %w", err) } if err = committer.Commit(); err != nil { + org.Name = oldName + org.LowerName = strings.ToLower(oldName) if err2 := util.Rename(user_model.UserPath(newName), user_model.UserPath(oldName)); err2 != nil && !os.IsNotExist(err2) { log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldName, newName, err, err2) return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldName, newName, err, err2) From 6d9870ac8f8b2c826ab828273c79fa7fbae64dd4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 May 2023 10:49:49 +0800 Subject: [PATCH 08/10] Follow reviewers' suggestion --- models/user/error.go | 17 ++++++++++++++++- options/locale/locale_en-US.ini | 1 + routers/api/v1/admin/user.go | 10 ++++------ routers/web/admin/users.go | 2 +- routers/web/org/setting.go | 2 +- routers/web/user/setting/profile.go | 9 ++++++--- services/org/org.go | 19 ++++++++----------- services/user/user.go | 11 ++++++++++- 8 files changed, 47 insertions(+), 24 deletions(-) diff --git a/models/user/error.go b/models/user/error.go index 2fd435174158b..f512994169566 100644 --- a/models/user/error.go +++ b/models/user/error.go @@ -88,7 +88,7 @@ func (err ErrUserInactive) Error() string { return fmt.Sprintf("user is inactive [uid: %d, name: %s]", err.UID, err.Name) } -// Unwrap unwraps this error as a ErrUserInactive error +// Unwrap unwraps this error as a ErrPermission error func (err ErrUserInactive) Unwrap() error { return util.ErrPermissionDenied } @@ -108,3 +108,18 @@ func IsErrUserIsNotLocal(err error) bool { _, ok := err.(ErrUserIsNotLocal) return ok } + +type ErrUsernameNotChanged struct { + UID int64 + Name string +} + +func (err ErrUsernameNotChanged) Error() string { + return fmt.Sprintf("username hasn't been changed[uid: %d, name: %s]", err.UID, err.Name) +} + +// IsErrUsernameNotChanged +func IsErrUsernameNotChanged(err error) bool { + _, ok := err.(ErrUsernameNotChanged) + return ok +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 180fd1c18d0e9..03a99f140f498 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -511,6 +511,7 @@ lang_select_error = Select a language from the list. username_been_taken = The username is already taken. username_change_not_local_user = Non-local users are not allowed to change their username. +username_has_not_been_changed = Username has not been changed repo_name_been_taken = The repository name is already used. repository_force_private = Force Private is enabled: private repositories cannot be made public. repository_files_already_exist = Files already exist for this repository. Contact the system administrator. diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 0ff84134debf9..b66fdf60ed244 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -505,15 +505,13 @@ func RenameUser(ctx *context.APIContext) { } newName := web.GetForm(ctx).(*api.RenameUserOption).NewName - if newName == ctx.ContextUser.Name { - // Noop as username is not changed - ctx.Status(http.StatusNoContent) - return - } // Check if user name has been changed - if err := user_service.RenameUser(ctx, ctx.ContextUser, newName, strings.EqualFold(newName, ctx.ContextUser.Name)); err != nil { + if err := user_service.RenameUser(ctx, ctx.ContextUser, newName); err != nil { switch { + case user_model.IsErrUsernameNotChanged(err): + // Noop as username is not changed + ctx.Status(http.StatusNoContent) case user_model.IsErrUserAlreadyExist(err): ctx.Error(http.StatusUnprocessableEntity, "", ctx.Tr("form.username_been_taken")) case db.IsErrNameReserved(err): diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 40e5e77fe2815..2150bc42f7baf 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -332,7 +332,7 @@ func EditUserPost(ctx *context.Context) { } if len(form.UserName) != 0 && u.Name != form.UserName { - if err := user_setting.HandleUsernameChange(ctx, u, form.UserName, u.LowerName == strings.ToLower(form.UserName)); err != nil { + if err := user_setting.HandleUsernameChange(ctx, u, form.UserName); err != nil { if ctx.Written() { return } diff --git a/routers/web/org/setting.go b/routers/web/org/setting.go index 833ad36dcf5e3..3937b1a708172 100644 --- a/routers/web/org/setting.go +++ b/routers/web/org/setting.go @@ -66,7 +66,7 @@ func SettingsPost(ctx *context.Context) { // Check if organization name has been changed. if nameChanged { - err := org_service.RenameOrganization(ctx, org, form.Name, org.LowerName == strings.ToLower(form.Name)) + err := org_service.RenameOrganization(ctx, org, form.Name) switch { case user_model.IsErrUserAlreadyExist(err): ctx.Data["OrgName"] = true diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index d389a0db4c033..8a8fb3337266a 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -48,10 +48,13 @@ func Profile(ctx *context.Context) { } // HandleUsernameChange handle username changes from user settings and admin interface -func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName string, onlyCapitalization bool) error { +func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName string) error { // rename user - if err := user_service.RenameUser(ctx, user, newName, onlyCapitalization); err != nil { + if err := user_service.RenameUser(ctx, user, newName); err != nil { switch { + // Noop as username is not changed + case user_model.IsErrUsernameNotChanged(err): + ctx.Flash.Error(ctx.Tr("form.username_has_not_been_changed")) // Non-local users are not allowed to change their username. case user_model.IsErrUserIsNotLocal(err): ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user")) @@ -87,7 +90,7 @@ func ProfilePost(ctx *context.Context) { if len(form.Name) != 0 && ctx.Doer.Name != form.Name { log.Debug("Changing name for %s to %s", ctx.Doer.Name, form.Name) - if err := HandleUsernameChange(ctx, ctx.Doer, form.Name, ctx.Doer.LowerName == strings.ToLower(form.Name)); err != nil { + if err := HandleUsernameChange(ctx, ctx.Doer, form.Name); err != nil { ctx.Redirect(setting.AppSubURL + "/user/settings") return } diff --git a/services/org/org.go b/services/org/org.go index 8db783ff26e09..dffb59f84dae5 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -73,7 +73,7 @@ func DeleteOrganization(org *org_model.Organization) error { } // RenameOrganization renames an organization. -func RenameOrganization(ctx context.Context, org *org_model.Organization, newName string, onlyCapitalization bool) error { +func RenameOrganization(ctx context.Context, org *org_model.Organization, newName string) error { if !org.AsUser().IsOrganization() { return fmt.Errorf("cannot rename user") } @@ -82,12 +82,7 @@ func RenameOrganization(ctx context.Context, org *org_model.Organization, newNam return err } - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - + onlyCapitalization := strings.EqualFold(org.Name, newName) oldName := org.Name if onlyCapitalization { @@ -96,13 +91,15 @@ func RenameOrganization(ctx context.Context, org *org_model.Organization, newNam org.Name = oldName return err } - if err := committer.Commit(); err != nil { - org.Name = oldName - return err - } return nil } + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + isExist, err := user_model.IsUserExist(ctx, org.ID, newName) if err != nil { return err diff --git a/services/user/user.go b/services/user/user.go index 39babac0480a1..e6be299ede233 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -29,7 +29,7 @@ import ( ) // RenameUser renames a user -func RenameUser(ctx context.Context, u *user_model.User, newUserName string, onlyCapitalization bool) error { +func RenameUser(ctx context.Context, u *user_model.User, newUserName string) error { if u.IsOrganization() { return fmt.Errorf("cannot rename organization") } @@ -42,6 +42,15 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string, onl } } + if newUserName == u.Name { + return user_model.ErrUsernameNotChanged{ + UID: u.ID, + Name: u.Name, + } + } + + onlyCapitalization := strings.EqualFold(newUserName, u.Name) + if err := user_model.IsUsableUsername(newUserName); err != nil { return err } From c6f01e3c7e8b23a915207ac51c0b3e10c55048fd Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 May 2023 11:05:17 +0800 Subject: [PATCH 09/10] Merge duplicated functions --- services/org/org.go | 78 ++----------------------------------------- services/user/user.go | 25 +++++--------- 2 files changed, 10 insertions(+), 93 deletions(-) diff --git a/services/org/org.go b/services/org/org.go index dffb59f84dae5..baf23bf9b1768 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -6,8 +6,6 @@ package org import ( "context" "fmt" - "os" - "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" @@ -18,8 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" - "code.gitea.io/gitea/services/agit" - container_service "code.gitea.io/gitea/services/packages/container" + user_service "code.gitea.io/gitea/services/user" ) // DeleteOrganization completely and permanently deletes everything of organization. @@ -74,79 +71,8 @@ func DeleteOrganization(org *org_model.Organization) error { // RenameOrganization renames an organization. func RenameOrganization(ctx context.Context, org *org_model.Organization, newName string) error { - if !org.AsUser().IsOrganization() { - return fmt.Errorf("cannot rename user") - } - - if err := user_model.IsUsableUsername(newName); err != nil { - return err - } - - onlyCapitalization := strings.EqualFold(org.Name, newName) oldName := org.Name - - if onlyCapitalization { - org.Name = newName - if err := user_model.UpdateUserCols(ctx, org.AsUser(), "name"); err != nil { - org.Name = oldName - return err - } - return nil - } - - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - isExist, err := user_model.IsUserExist(ctx, org.ID, newName) - if err != nil { - return err - } - if isExist { - return user_model.ErrUserAlreadyExist{ - Name: newName, - } - } - - if err = repo_model.UpdateRepositoryOwnerName(ctx, oldName, newName); err != nil { - return err - } - - if err = user_model.NewUserRedirect(ctx, org.ID, oldName, newName); err != nil { - return err - } - - if err := agit.UserNameChanged(ctx, org.AsUser(), newName); err != nil { - return err - } - if err := container_service.UpdateRepositoryNames(ctx, org.AsUser(), newName); err != nil { - return err - } - - org.Name = newName - org.LowerName = strings.ToLower(newName) - if err := user_model.UpdateUserCols(ctx, org.AsUser(), "name", "lower_name"); err != nil { - org.Name = oldName - org.LowerName = strings.ToLower(oldName) - return err - } - - // Do not fail if directory does not exist - if err = util.Rename(user_model.UserPath(oldName), user_model.UserPath(newName)); err != nil && !os.IsNotExist(err) { - org.Name = oldName - org.LowerName = strings.ToLower(oldName) - return fmt.Errorf("rename user directory: %w", err) - } - - if err = committer.Commit(); err != nil { - org.Name = oldName - org.LowerName = strings.ToLower(oldName) - if err2 := util.Rename(user_model.UserPath(newName), user_model.UserPath(oldName)); err2 != nil && !os.IsNotExist(err2) { - log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldName, newName, err, err2) - return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldName, newName, err, err2) - } + if err := user_service.RenameUser(ctx, org.AsUser(), newName); err != nil { return err } diff --git a/services/user/user.go b/services/user/user.go index e6be299ede233..5df1ca0c6da65 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -30,12 +30,8 @@ import ( // RenameUser renames a user func RenameUser(ctx context.Context, u *user_model.User, newUserName string) error { - if u.IsOrganization() { - return fmt.Errorf("cannot rename organization") - } - // Non-local users are not allowed to change their username. - if !u.IsLocal() { + if !u.IsOrganization() && !u.IsLocal() { return user_model.ErrUserIsNotLocal{ UID: u.ID, Name: u.Name, @@ -49,18 +45,11 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string) err } } - onlyCapitalization := strings.EqualFold(newUserName, u.Name) - if err := user_model.IsUsableUsername(newUserName); err != nil { return err } - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - + onlyCapitalization := strings.EqualFold(newUserName, u.Name) oldUserName := u.Name if onlyCapitalization { @@ -69,13 +58,15 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string) err u.Name = oldUserName return err } - if err := committer.Commit(); err != nil { - u.Name = oldUserName - return err - } return nil } + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + isExist, err := user_model.IsUserExist(ctx, u.ID, newUserName) if err != nil { return err From 67ec820238ba199a19372674c153b1379aae5947 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 May 2023 11:14:49 +0800 Subject: [PATCH 10/10] Improve trace --- routers/api/v1/admin/user.go | 3 +++ routers/web/user/setting/profile.go | 3 ++- services/org/org.go | 9 +-------- services/user/user.go | 2 -- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 59ff29384575a..c3af5dc90a75e 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -502,6 +502,7 @@ func RenameUser(ctx *context.APIContext) { return } + oldName := ctx.ContextUser.Name newName := web.GetForm(ctx).(*api.RenameUserOption).NewName // Check if user name has been changed @@ -523,5 +524,7 @@ func RenameUser(ctx *context.APIContext) { } return } + + log.Trace("User name changed: %s -> %s", oldName, newName) ctx.Status(http.StatusOK) } diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index 8a8fb3337266a..47066d5e384ef 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -49,6 +49,7 @@ func Profile(ctx *context.Context) { // HandleUsernameChange handle username changes from user settings and admin interface func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName string) error { + oldName := user.Name // rename user if err := user_service.RenameUser(ctx, user, newName); err != nil { switch { @@ -73,7 +74,7 @@ func HandleUsernameChange(ctx *context.Context, user *user_model.User, newName s } return err } - + log.Trace("User name changed: %s -> %s", oldName, newName) return nil } diff --git a/services/org/org.go b/services/org/org.go index baf23bf9b1768..a62e5b6fc8f6d 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -13,7 +13,6 @@ import ( packages_model "code.gitea.io/gitea/models/packages" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" user_service "code.gitea.io/gitea/services/user" @@ -71,11 +70,5 @@ func DeleteOrganization(org *org_model.Organization) error { // RenameOrganization renames an organization. func RenameOrganization(ctx context.Context, org *org_model.Organization, newName string) error { - oldName := org.Name - if err := user_service.RenameUser(ctx, org.AsUser(), newName); err != nil { - return err - } - - log.Trace("Org name changed: %s -> %s", oldName, newName) - return nil + return user_service.RenameUser(ctx, org.AsUser(), newName) } diff --git a/services/user/user.go b/services/user/user.go index 5df1ca0c6da65..e0815bd860791 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -116,8 +116,6 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string) err } return err } - - log.Trace("User name changed: %s -> %s", oldUserName, newUserName) return nil }