fix: reflect allowed username change in profile setting (#11171)

- When working forgejo/forgejo!8714 I did not touch the UI to remove the
note and `disabled` attribute. This was not intentional, and was likely
caused by me straight going for testing (as the backend code would allow
the username change).
- Slightly refactor the context to a common function, don't hard error
if `CanUserRename` fails but does default to that you cannot rename in
that case (which is the standard behavior of OAuth2 users anyway).

I already was aware that it seems !8714 wasn't working on Codeberg but someone at FOSDEM pointed it out again, thus the reason for this bug fix.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11171
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: Beowulf <beowulf@beocode.eu>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

routers/web/user/setting/profile.go

@@ -43,8 +43,8 @@ const (
 
 var commonPronouns = []string{"he/him", "she/her", "they/them", "it/its", "any pronouns"}
 
-// Profile render user's profile page
-func Profile(ctx *context.Context) {
+// profileContext sets common context for profile settings template.
+func profileContext(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("settings.profile")
 	ctx.Data["PageIsSettingsProfile"] = true
 	ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
@@ -55,20 +55,24 @@ func Profile(ctx *context.Context) {
 	ctx.Data["MaxAvatarWidth"] = setting.Avatar.MaxWidth
 	ctx.Data["MaxAvatarHeight"] = setting.Avatar.MaxHeight
 
+	canUserRename, err := user_service.CanUserRename(ctx, ctx.Doer)
+	if err != nil {
+		log.Error("CanUserRename for user[%d]: %v", ctx.Doer.ID, err)
+	}
+
+	ctx.Data["UserRenameDisabled"] = !canUserRename
+}
+
+// Profile render user's profile page
+func Profile(ctx *context.Context) {
+	profileContext(ctx)
+
 	ctx.HTML(http.StatusOK, tplSettingsProfile)
 }
 
 // ProfilePost response for change user's profile
 func ProfilePost(ctx *context.Context) {
-	ctx.Data["Title"] = ctx.Tr("settings")
-	ctx.Data["PageIsSettingsProfile"] = true
-	ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
-	ctx.Data["DisableGravatar"] = setting.Config().Picture.DisableGravatar.Value(ctx)
-	ctx.Data["CooldownPeriod"] = setting.Service.UsernameCooldownPeriod
-	ctx.Data["CommonPronouns"] = commonPronouns
-	ctx.Data["MaxAvatarFileSize"] = setting.Avatar.MaxFileSize
-	ctx.Data["MaxAvatarWidth"] = setting.Avatar.MaxWidth
-	ctx.Data["MaxAvatarHeight"] = setting.Avatar.MaxHeight
+	profileContext(ctx)
 
 	if ctx.HasError() {
 		ctx.HTML(http.StatusOK, tplSettingsProfile)

services/user/user.go

@@ -38,35 +38,47 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string) err
 	return renameUser(ctx, u, newUserName, false)
 }
 
-// RenameUser renames a user as an admin.
+// AdminRenameUser renames a user as an admin.
 func AdminRenameUser(ctx context.Context, u *user_model.User, newUserName string) error {
 	return renameUser(ctx, u, newUserName, true)
 }
 
+// CanUserRename returns if the given user can be renamed.
+//
+// This is merely a precondition, you likely want to use [RenameUser] or [AdminRenameUser]
+// which also takes into consideration username cooldown of a new username.
+func CanUserRename(ctx context.Context, user *user_model.User) (bool, error) {
+	// Non-local users are not allowed to change their username.
+	// Organizations can always be renamed.
+	if user.IsOrganization() || user.IsLocal() {
+		return true, nil
+	}
+
+	// If the user's authentication source is OAuth2 and that source allows for
+	// username changes then don't make a fuzz about it.
+	if !user.IsOAuth2() {
+		return false, nil
+	}
+
+	source, err := auth.GetSourceByID(ctx, user.LoginSource)
+	if err != nil {
+		return false, err
+	}
+	return source.Cfg.(*oauth2.Source).AllowUsernameChange, nil
+}
+
 func renameUser(ctx context.Context, u *user_model.User, newUserName string, doerIsAdmin bool) error {
 	if newUserName == u.Name {
 		return nil
 	}
 
-	// Non-local users are not allowed to change their username.
 	// If the doer is an admin, then allow the rename - they know better.
-	if !doerIsAdmin && !u.IsOrganization() && !u.IsLocal() {
-		// If the user's authentication source is OAuth2 and that source allows for
-		// username changes then don't make a fuzz about it.
-
-		if !u.IsOAuth2() {
-			return user_model.ErrUserIsNotLocal{
-				UID:  u.ID,
-				Name: u.Name,
-			}
-		}
-
-		source, err := auth.GetSourceByID(ctx, u.LoginSource)
+	if !doerIsAdmin {
+		canRenamed, err := CanUserRename(ctx, u)
 		if err != nil {
 			return err
 		}
-		sourceCfg := source.Cfg.(*oauth2.Source)
-		if !sourceCfg.AllowUsernameChange {
+		if !canRenamed {
 			return user_model.ErrUserIsNotLocal{
 				UID:  u.ID,
 				Name: u.Name,

templates/user/settings/profile.tmpl

@@ -9,8 +9,8 @@
 					<legend>{{ctx.Locale.Tr "settings.profile_desc"}}</legend>
 					<label {{if .Err_Name}}class="field error"{{end}}>
 						{{ctx.Locale.Tr "username"}}
-						<input name="name" value="{{.SignedUser.Name}}" data-name="{{.SignedUser.Name}}" autofocus required {{if or (not .SignedUser.IsLocal) .IsReverseProxy}}disabled{{end}} maxlength="40">
-						{{if or (not .SignedUser.IsLocal) .IsReverseProxy}}
+						<input name="name" value="{{.SignedUser.Name}}" data-name="{{.SignedUser.Name}}" autofocus required {{if or .UserRenameDisabled .IsReverseProxy}}disabled{{end}} maxlength="40">
+						{{if or .UserRenameDisabled .IsReverseProxy}}
 							<span class="help">{{ctx.Locale.Tr "settings.password_username_disabled"}}</span>
 						{{else}}
 							<span class="help">

tests/integration/fixtures/TestUserRename/login_source.yml

@@ -0,0 +1,10 @@
+-
+ id: 1001
+ type: 6
+ is_active: true
+ cfg: '{"Provider":"openidConnect","AllowUsernameChange":false}'
+-
+ id: 1002
+ type: 6
+ is_active: true
+ cfg: '{"Provider":"openidConnect","AllowUsernameChange":true}'

tests/integration/user_settings_test.go

@@ -7,10 +7,18 @@ import (
 	"net/http"
 	"testing"
 
+	"forgejo.org/models/auth"
+	"forgejo.org/models/db"
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/container"
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/test"
+	"forgejo.org/modules/translation"
 	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // TestUserSettingsAccount tests the contents of a user's account settings
@@ -124,3 +132,50 @@ func TestUserSettingsDelete(t *testing.T) {
 		session.MakeRequest(t, req, http.StatusNotFound)
 	})
 }
+
+func TestUserRename(t *testing.T) {
+	defer unittest.OverrideFixtures("tests/integration/fixtures/TestUserRename")()
+	defer tests.PrepareTestEnv(t)()
+
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	session := loginUser(t, "user2")
+	trMsg := translation.NewLocale("en-US").Tr("settings.password_username_disabled")
+
+	test := func(t *testing.T, session *TestSession, allowed bool) {
+		t.Helper()
+
+		resp := session.MakeRequest(t, NewRequest(t, "GET", "/user/settings"), http.StatusOK)
+		if allowed {
+			assert.NotContains(t, resp.Body.String(), trMsg)
+		} else {
+			assert.Contains(t, resp.Body.String(), trMsg)
+		}
+	}
+
+	t.Run("Local", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		test(t, session, true)
+	})
+
+	t.Run("OAuth2", func(t *testing.T) {
+		user.LoginSource = 1001
+		user.LoginType = auth.OAuth2
+		_, err := db.GetEngine(t.Context()).Cols("login_source", "login_type").Update(user)
+		require.NoError(t, err)
+
+		t.Run("Not allowed", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			test(t, session, false)
+		})
+
+		user.LoginSource = 1002
+		_, err = db.GetEngine(t.Context()).Cols("login_source", "login_type").Update(user)
+		require.NoError(t, err)
+
+		t.Run("Allowed", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			test(t, session, true)
+		})
+	})
+}