Allow admins to rename non-local users (#35970)

Presently, attempting to rename a non-local (e.g. Oauth2 or LDAP) user
results in an error, even if the requester is an administrator. As far
as I can tell, this is a security feature, not architectural in nature,
as automatic account linking could be used to take control of another
user's account. This is not a concern for an administrator, who we
should trust to know what they are doing.

This patch allows admins, and only admins, to rename non-local users.

Fixes #18308 (sort of)

---------

Co-authored-by: wxiaoguang <[email protected]>

Author: meln5674

routers/api/v1/admin/user.go

@@ -480,7 +480,7 @@ func RenameUser(ctx *context.APIContext) {
 	newName := web.GetForm(ctx).(*api.RenameUserOption).NewName
 
 	// Check if username has been changed
-	if err := user_service.RenameUser(ctx, ctx.ContextUser, newName); err != nil {
+	if err := user_service.RenameUser(ctx, ctx.ContextUser, newName, ctx.Doer); err != nil {
 		if user_model.IsErrUserAlreadyExist(err) || db.IsErrNameReserved(err) || db.IsErrNamePatternNotAllowed(err) || db.IsErrNameCharsNotAllowed(err) {
 			ctx.APIError(http.StatusUnprocessableEntity, err)
 		} else {

routers/api/v1/org/org.go

@@ -340,7 +340,7 @@ func Rename(ctx *context.APIContext) {
 
 	form := web.GetForm(ctx).(*api.RenameOrgOption)
 	orgUser := ctx.Org.Organization.AsUser()
-	if err := user_service.RenameUser(ctx, orgUser, form.NewName); err != nil {
+	if err := user_service.RenameUser(ctx, orgUser, form.NewName, ctx.Doer); err != nil {
 		if user_model.IsErrUserAlreadyExist(err) || db.IsErrNameReserved(err) || db.IsErrNamePatternNotAllowed(err) || db.IsErrNameCharsNotAllowed(err) {
 			ctx.APIError(http.StatusUnprocessableEntity, err)
 		} else {

routers/web/admin/users.go

@@ -345,7 +345,7 @@ func EditUserPost(ctx *context.Context) {
 	}
 
 	if form.UserName != "" {
-		if err := user_service.RenameUser(ctx, u, form.UserName); err != nil {
+		if err := user_service.RenameUser(ctx, u, form.UserName, ctx.Doer); err != nil {
 			switch {
 			case user_model.IsErrUserIsNotLocal(err):
 				ctx.Data["Err_UserName"] = true

routers/web/org/setting.go

@@ -213,7 +213,7 @@ func SettingsRenamePost(ctx *context.Context) {
 		return
 	}
 
-	if err := user_service.RenameUser(ctx, ctx.Org.Organization.AsUser(), newOrgName); err != nil {
+	if err := user_service.RenameUser(ctx, ctx.Org.Organization.AsUser(), newOrgName, ctx.Doer); err != nil {
 		if user_model.IsErrUserAlreadyExist(err) {
 			ctx.JSONError(ctx.Tr("org.form.name_been_taken", newOrgName))
 		} else if db.IsErrNameReserved(err) {

routers/web/user/setting/profile.go

@@ -75,7 +75,7 @@ func ProfilePost(ctx *context.Context) {
 			ctx.Redirect(setting.AppSubURL + "/user/settings")
 			return
 		}
-		if err := user_service.RenameUser(ctx, ctx.Doer, form.Name); err != nil {
+		if err := user_service.RenameUser(ctx, ctx.Doer, form.Name, ctx.Doer); err != nil {
 			switch {
 			case user_model.IsErrUserIsNotLocal(err):
 				ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user"))

services/user/user.go

@@ -31,17 +31,15 @@ import (
 )
 
 // 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, doer *user_model.User) error {
 	if newUserName == u.Name {
 		return nil
 	}
 
-	// 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,
-		}
+	// Non-local users are not allowed to change their own username, but admins are
+	isExternalUser := !u.IsOrganization() && !u.IsLocal()
+	if isExternalUser && !doer.IsAdmin {
+		return user_model.ErrUserIsNotLocal{UID: u.ID, Name: u.Name}
 	}
 
 	if err := user_model.IsUsableUsername(newUserName); err != nil {

services/user/user_test.go

@@ -20,6 +20,7 @@ import (
 	org_service "code.gitea.io/gitea/services/org"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMain(m *testing.M) {
@@ -101,23 +102,31 @@ func TestRenameUser(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 21})
 
-	t.Run("Non-Local", func(t *testing.T) {
-		u := &user_model.User{
-			Type:      user_model.UserTypeIndividual,
-			LoginType: auth.OAuth2,
+	t.Run("External user", func(t *testing.T) {
+		adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1, IsAdmin: true})
+		externalUser := &user_model.User{
+			Name:      "external_user",
+			Email:     "[email protected]",
+			LoginType: auth.LDAP,
 		}
-		assert.ErrorIs(t, RenameUser(t.Context(), u, "user_rename"), user_model.ErrUserIsNotLocal{})
+		require.NoError(t, user_model.CreateUser(t.Context(), externalUser, &user_model.Meta{}))
+
+		err := RenameUser(t.Context(), externalUser, externalUser.Name+"_changed", externalUser)
+		assert.True(t, user_model.IsErrUserIsNotLocal(err), "external user is not allowed to rename themselves")
+
+		err = RenameUser(t.Context(), externalUser, externalUser.Name+"_changed", adminUser)
+		assert.NoError(t, err, "admin can rename external user")
 	})
 
 	t.Run("Same username", func(t *testing.T) {
-		assert.NoError(t, RenameUser(t.Context(), user, user.Name))
+		assert.NoError(t, RenameUser(t.Context(), user, user.Name, user))
 	})
 
 	t.Run("Non usable username", func(t *testing.T) {
 		usernames := []string{"--diff", ".well-known", "gitea-actions", "aaa.atom", "aa.png"}
 		for _, username := range usernames {
 			assert.Error(t, user_model.IsUsableUsername(username), "non-usable username: %s", username)
-			assert.Error(t, RenameUser(t.Context(), user, username), "non-usable username: %s", username)
+			assert.Error(t, RenameUser(t.Context(), user, username, user), "non-usable username: %s", username)
 		}
 	})
 
@@ -126,7 +135,7 @@ func TestRenameUser(t *testing.T) {
 		unittest.AssertNotExistsBean(t, &user_model.User{ID: user.ID, Name: caps})
 		unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID, OwnerName: user.Name})
 
-		assert.NoError(t, RenameUser(t.Context(), user, caps))
+		assert.NoError(t, RenameUser(t.Context(), user, caps, user))
 
 		unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID, Name: caps})
 		unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID, OwnerName: caps})
@@ -135,17 +144,17 @@ func TestRenameUser(t *testing.T) {
 	t.Run("Already exists", func(t *testing.T) {
 		existUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 
-		assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.Name), user_model.ErrUserAlreadyExist{Name: existUser.Name})
-		assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.LowerName), user_model.ErrUserAlreadyExist{Name: existUser.LowerName})
+		assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.Name, user), user_model.ErrUserAlreadyExist{Name: existUser.Name})
+		assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.LowerName, user), user_model.ErrUserAlreadyExist{Name: existUser.LowerName})
 		newUsername := fmt.Sprintf("uSEr%d", existUser.ID)
-		assert.ErrorIs(t, RenameUser(t.Context(), user, newUsername), user_model.ErrUserAlreadyExist{Name: newUsername})
+		assert.ErrorIs(t, RenameUser(t.Context(), user, newUsername, user), user_model.ErrUserAlreadyExist{Name: newUsername})
 	})
 
 	t.Run("Normal", func(t *testing.T) {
 		oldUsername := user.Name
 		newUsername := "User_Rename"
 
-		assert.NoError(t, RenameUser(t.Context(), user, newUsername))
+		assert.NoError(t, RenameUser(t.Context(), user, newUsername, user))
 		unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID, Name: newUsername, LowerName: strings.ToLower(newUsername)})
 
 		redirectUID, err := user_model.LookupUserRedirect(t.Context(), oldUsername)

templates/admin/user/edit.tmpl

@@ -9,7 +9,7 @@
 				{{.CsrfTokenHtml}}
 				<div class="field {{if .Err_UserName}}error{{end}}">
 					<label for="user_name">{{ctx.Locale.Tr "username"}}</label>
-					<input id="user_name" name="user_name" value="{{.User.Name}}" {{if not .User.IsLocal}}disabled{{end}} maxlength="40">
+					<input id="user_name" name="user_name" value="{{.User.Name}}" maxlength="40">
 				</div>
 				<!-- Types and name -->
 				<div class="inline required field {{if .Err_LoginType}}error{{end}}">