Refactor rename user and rename organization (#24052)

This PR is a refactor at the beginning. And now it did 4 things.
- [x] Move renaming organizaiton and user logics into services layer and
merged as one function
- [x] Support rename a user capitalization only. For example, rename the
user from `Lunny` to `lunny`. We just need to change one table `user`
and others should not be touched.
- [x] Before this PR, some renaming were missed like `agit`
- [x] Fix bug the API reutrned from `http.StatusNoContent` to `http.StatusOK`

Author: lunny

models/repo/repo.go

@@ -832,3 +832,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
+}

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
@@ -99,3 +92,34 @@ func (err ErrUserInactive) Error() string {
 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
+}
+
+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
+}

models/user/user.go

@@ -9,7 +9,6 @@ import (
 	"encoding/hex"
 	"fmt"
 	"net/url"
-	"os"
 	"path/filepath"
 	"strings"
 	"time"
@@ -756,50 +755,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)

options/locale/locale_en-US.ini

@@ -520,6 +520,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.

routers/api/v1/admin/user.go

@@ -502,17 +502,15 @@ func RenameUser(ctx *context.APIContext) {
 		return
 	}
 
+	oldName := ctx.ContextUser.Name
 	newName := web.GetForm(ctx).(*api.RenameUserOption).NewName
 
-	if strings.EqualFold(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); 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):
@@ -526,5 +524,7 @@ func RenameUser(ctx *context.APIContext) {
 		}
 		return
 	}
-	ctx.Status(http.StatusNoContent)
+
+	log.Trace("User name changed: %s -> %s", oldName, newName)
+	ctx.Status(http.StatusOK)
 }

routers/web/org/setting.go

@@ -22,8 +22,7 @@ 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"
-	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"
 )
@@ -67,31 +66,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)
+		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
 		}
 
@@ -186,7 +177,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")

routers/web/user/setting/profile.go

@@ -49,15 +49,16 @@ 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"))
-	}
-
+	oldName := user.Name
 	// rename user
 	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"))
 		case user_model.IsErrUserAlreadyExist(err):
 			ctx.Flash.Error(ctx.Tr("form.username_been_taken"))
 		case user_model.IsErrEmailAlreadyUsed(err):
@@ -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
 }
 

services/org/org.go

@@ -4,20 +4,22 @@
 package org
 
 import (
+	"context"
 	"fmt"
 
 	"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/storage"
 	"code.gitea.io/gitea/modules/util"
+	user_service "code.gitea.io/gitea/services/user"
 )
 
 // 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 +41,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 +55,20 @@ 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) error {
+	return user_service.RenameUser(ctx, org.AsUser(), newName)
+}

services/user/avatar.go

@@ -0,0 +1,62 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package user
+
+import (
+	"fmt"
+	"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 {
+	avatarData, err := avatar.ProcessAvatarImage(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 {
+		_, err := w.Write(avatarData)
+		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
+}

services/user/rename.go

@@ -6,7 +6,8 @@ package user
 import (
 	"context"
 	"fmt"
-	"io"
+	"os"
+	"strings"
 	"time"
 
 	"code.gitea.io/gitea/models"
@@ -17,29 +18,105 @@ 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 {
+	// Non-local users are not allowed to change their username.
+	if !u.IsOrganization() && !u.IsLocal() {
+		return user_model.ErrUserIsNotLocal{
+			UID:  u.ID,
+			Name: u.Name,
+		}
+	}
+
+	if newUserName == u.Name {
+		return user_model.ErrUsernameNotChanged{
+			UID:  u.ID,
+			Name: u.Name,
+		}
+	}
+
+	if err := user_model.IsUsableUsername(newUserName); err != nil {
+		return err
+	}
+
+	onlyCapitalization := strings.EqualFold(newUserName, u.Name)
+	oldUserName := u.Name
+
+	if onlyCapitalization {
+		u.Name = newUserName
+		if err := user_model.UpdateUserCols(ctx, u, "name"); err != nil {
+			u.Name = oldUserName
+			return err
+		}
+		return nil
+	}
+
 	ctx, committer, err := db.TxContext(ctx)
 	if err != nil {
 		return err
 	}
 	defer committer.Close()
-	if err := renameUser(ctx, u, newUserName); err != 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
+	}
+	return nil
 }
 
 // DeleteUser completely and permanently deletes everything of a user,
@@ -240,50 +317,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 {
-	avatarData, err := avatar.ProcessAvatarImage(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 {
-		_, err := w.Write(avatarData)
-		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
-}

services/user/user.go

@@ -241,3 +241,44 @@ 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_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_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_name": "user1",
+	})
+	// 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{
+		// required
+		"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_name": "user2",
+	})
+	MakeRequest(t, req, http.StatusOK)
+}