From b51f97e97d55a8d55a2975d2ebcadff0d26638f1 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 6 Aug 2025 20:25:13 +0200 Subject: [PATCH] feat: add option to allow non-local users to change usernames (#8714) Add a new config option for OAuth2 authentication sources: allow users to change their username. In the case where OAuth2 is more like a social OAuth2 login there's no need to not allow users to change their username. The information how the user is linked to the authentication source is stored in different fields. Resolves forgejo/forgejo#687 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8714 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Reviewed-by: Earl Warren Co-authored-by: Gusted Co-committed-by: Gusted --- cmd/admin_auth_oauth.go | 9 +++++ cmd/admin_auth_oauth_test.go | 2 ++ models/user/fixtures/login_source.yml | 8 +++++ models/user/fixtures/user.yml | 1 + options/locale_next/locale_en-US.json | 2 ++ routers/web/admin/auths.go | 1 + services/auth/source/oauth2/source.go | 1 + services/forms/auth_form.go | 1 + services/user/user.go | 25 +++++++++++-- services/user/user_test.go | 40 +++++++++++++-------- templates/admin/auth/edit.tmpl | 7 ++++ templates/admin/auth/source/oauth.tmpl | 7 ++++ tests/integration/admin_auth_source_test.go | 32 +++++++++++++++++ 13 files changed, 119 insertions(+), 17 deletions(-) create mode 100644 models/user/fixtures/login_source.yml create mode 100644 tests/integration/admin_auth_source_test.go diff --git a/cmd/admin_auth_oauth.go b/cmd/admin_auth_oauth.go index 8a756480cd..ef5d9116e3 100644 --- a/cmd/admin_auth_oauth.go +++ b/cmd/admin_auth_oauth.go @@ -125,6 +125,10 @@ func oauthCLIFlags() []cli.Flag { Name: "group-team-map-removal", Usage: "Activate automatic team membership removal depending on groups", }, + &cli.BoolFlag{ + Name: "allow-username-change", + Usage: "Allow users to change their username", + }, } } @@ -176,6 +180,7 @@ func parseOAuth2Config(_ context.Context, c *cli.Command) *oauth2.Source { RestrictedGroup: c.String("restricted-group"), GroupTeamMap: c.String("group-team-map"), GroupTeamMapRemoval: c.Bool("group-team-map-removal"), + AllowUsernameChange: c.Bool("allow-username-change"), } } @@ -277,6 +282,10 @@ func (a *authService) updateOauth(ctx context.Context, c *cli.Command) error { oAuth2Config.GroupTeamMapRemoval = c.Bool("group-team-map-removal") } + if c.IsSet("allow-username-change") { + oAuth2Config.AllowUsernameChange = c.Bool("allow-username-change") + } + // update custom URL mapping customURLMapping := &oauth2.CustomURLMapping{} diff --git a/cmd/admin_auth_oauth_test.go b/cmd/admin_auth_oauth_test.go index ea5442e62d..3430ad1f56 100644 --- a/cmd/admin_auth_oauth_test.go +++ b/cmd/admin_auth_oauth_test.go @@ -55,6 +55,7 @@ func TestAddOauth(t *testing.T) { "--restricted-group", "restricted", "--group-team-map", `{"org_a_team_1": {"organization-a": ["Team 1"]}, "org_a_all_teams": {"organization-a": ["Team 1", "Team 2", "Team 3"]}}`, "--group-team-map-removal", + "--allow-username-change", }, source: &auth.Source{ Type: auth.OAuth2, @@ -83,6 +84,7 @@ func TestAddOauth(t *testing.T) { GroupTeamMapRemoval: true, RestrictedGroup: "restricted", SkipLocalTwoFA: true, + AllowUsernameChange: true, }, }, }, diff --git a/models/user/fixtures/login_source.yml b/models/user/fixtures/login_source.yml new file mode 100644 index 0000000000..3950f85964 --- /dev/null +++ b/models/user/fixtures/login_source.yml @@ -0,0 +1,8 @@ +- + id: 1001 + type: 6 # OAuth2 + name: OAuth2 authentication source + is_active: 1 + cfg: '{"Provider":"invalid","ClientID":"invalid","ClientSecret":"invalid","AllowUsernameChange":true}' + created_unix: 1753740851 + updated_unix: 1753740851 diff --git a/models/user/fixtures/user.yml b/models/user/fixtures/user.yml index b1892f331b..137064a368 100644 --- a/models/user/fixtures/user.yml +++ b/models/user/fixtures/user.yml @@ -11,6 +11,7 @@ must_change_password: false login_source: 1001 login_name: 123 + login_type: 6 type: 5 salt: ZogKvWdyEx max_repo_creation: -1 diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index e730c84726..aa8d811bd3 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -106,6 +106,8 @@ "discussion.sidebar.reference": "Reference", "editor.textarea.tab_hint": "Line already indented. Press Tab again or Escape to leave the editor.", "editor.textarea.shift_tab_hint": "No indentation on this line. Press Shift + Tab again or Escape to leave the editor.", + "admin.auths.allow_username_change": "Allow username change", + "admin.auths.allow_username_change.description": "Allow users to change their username in the profile settings", "admin.dashboard.cleanup_offline_runners": "Cleanup offline runners", "admin.dashboard.remove_resolved_reports": "Remove resolved reports", "settings.visibility.description": "Profile visibility affects others' ability to access your non-private repositories. Learn more.", diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go index 2c6dc76305..c352b6ad1a 100644 --- a/routers/web/admin/auths.go +++ b/routers/web/admin/auths.go @@ -190,6 +190,7 @@ func parseOAuth2Config(form forms.AuthenticationForm) *oauth2.Source { AdminGroup: form.Oauth2AdminGroup, GroupTeamMap: form.Oauth2GroupTeamMap, GroupTeamMapRemoval: form.Oauth2GroupTeamMapRemoval, + AllowUsernameChange: form.AllowUsernameChange, } } diff --git a/services/auth/source/oauth2/source.go b/services/auth/source/oauth2/source.go index 5245f88270..a3126cf353 100644 --- a/services/auth/source/oauth2/source.go +++ b/services/auth/source/oauth2/source.go @@ -29,6 +29,7 @@ type Source struct { GroupTeamMapRemoval bool RestrictedGroup string SkipLocalTwoFA bool `json:",omitempty"` + AllowUsernameChange bool // reference to the authSource authSource *auth.Source diff --git a/services/forms/auth_form.go b/services/forms/auth_form.go index e665ca0d19..b89e87f749 100644 --- a/services/forms/auth_form.go +++ b/services/forms/auth_form.go @@ -79,6 +79,7 @@ type AuthenticationForm struct { SkipLocalTwoFA bool GroupTeamMap string `binding:"ValidGroupTeamMap"` GroupTeamMapRemoval bool + AllowUsernameChange bool } // Validate validates fields diff --git a/services/user/user.go b/services/user/user.go index d682d5a434..9cb6858f0c 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -13,6 +13,7 @@ import ( "forgejo.org/models" asymkey_model "forgejo.org/models/asymkey" + "forgejo.org/models/auth" "forgejo.org/models/db" "forgejo.org/models/organization" packages_model "forgejo.org/models/packages" @@ -25,6 +26,7 @@ import ( "forgejo.org/modules/storage" "forgejo.org/modules/util" "forgejo.org/services/agit" + "forgejo.org/services/auth/source/oauth2" org_service "forgejo.org/services/org" "forgejo.org/services/packages" container_service "forgejo.org/services/packages/container" @@ -49,9 +51,26 @@ func renameUser(ctx context.Context, u *user_model.User, newUserName string, doe // 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() { - return user_model.ErrUserIsNotLocal{ - UID: u.ID, - Name: u.Name, + // 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 err != nil { + return err + } + sourceCfg := source.Cfg.(*oauth2.Source) + if !sourceCfg.AllowUsernameChange { + return user_model.ErrUserIsNotLocal{ + UID: u.ID, + Name: u.Name, + } } } diff --git a/services/user/user_test.go b/services/user/user_test.go index f1cab60a6d..0747833557 100644 --- a/services/user/user_test.go +++ b/services/user/user_test.go @@ -25,6 +25,7 @@ import ( "forgejo.org/modules/setting" "forgejo.org/modules/test" "forgejo.org/modules/timeutil" + "forgejo.org/services/auth/source/oauth2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -140,23 +141,10 @@ func TestCreateUser(t *testing.T) { } func TestRenameUser(t *testing.T) { + defer unittest.OverrideFixtures("models/user/fixtures/")() require.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{ - ID: 2, - Name: "old-name", - Type: user_model.UserTypeIndividual, - LoginType: auth.OAuth2, - } - require.ErrorIs(t, RenameUser(db.DefaultContext, u, "user_rename2"), user_model.ErrUserIsNotLocal{UID: 2, Name: "old-name"}) - - t.Run("Admin", func(t *testing.T) { - require.NoError(t, AdminRenameUser(t.Context(), u, "user_rename2")) - }) - }) - t.Run("Same username", func(t *testing.T) { require.NoError(t, RenameUser(db.DefaultContext, user, user.Name)) }) @@ -225,6 +213,30 @@ func TestRenameUser(t *testing.T) { unittest.AssertExistsIf(t, true, &user_model.Redirect{LowerName: "redirect-1"}) unittest.AssertExistsIf(t, true, &user_model.Redirect{LowerName: "redirect-2"}) }) + + t.Run("Non-local", func(t *testing.T) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1041, LoginSource: 1001}) + authSource := unittest.AssertExistsAndLoadBean(t, &auth.Source{ID: user.LoginSource}) + assert.False(t, user.IsLocal()) + assert.True(t, user.IsOAuth2()) + + t.Run("Allowed", func(t *testing.T) { + require.NoError(t, RenameUser(t.Context(), user, "I-am-a-local-username")) + }) + + t.Run("Not allowed", func(t *testing.T) { + authSourceCfg := authSource.Cfg.(*oauth2.Source) + authSourceCfg.AllowUsernameChange = false + authSource.Cfg = authSourceCfg + _, err := db.GetEngine(t.Context()).Cols("cfg").ID(authSource.ID).Update(authSource) + require.NoError(t, err) + + require.ErrorIs(t, RenameUser(t.Context(), user, "Another-username-change"), user_model.ErrUserIsNotLocal{UID: user.ID, Name: user.Name}) + t.Run("Admin", func(t *testing.T) { + require.NoError(t, AdminRenameUser(t.Context(), user, "Another-username-change")) + }) + }) + }) } func TestCreateUser_Issue5882(t *testing.T) { diff --git a/templates/admin/auth/edit.tmpl b/templates/admin/auth/edit.tmpl index 1ca5573cae..11be03acc5 100644 --- a/templates/admin/auth/edit.tmpl +++ b/templates/admin/auth/edit.tmpl @@ -299,6 +299,13 @@

{{ctx.Locale.Tr "admin.auths.skip_local_two_fa_helper"}}

+
+
+ + +

{{ctx.Locale.Tr "admin.auths.allow_username_change.description"}}

+
+
diff --git a/templates/admin/auth/source/oauth.tmpl b/templates/admin/auth/source/oauth.tmpl index 7d0a64d269..0cb9ea56bc 100644 --- a/templates/admin/auth/source/oauth.tmpl +++ b/templates/admin/auth/source/oauth.tmpl @@ -35,6 +35,13 @@

{{ctx.Locale.Tr "admin.auths.skip_local_two_fa_helper"}}

+
+
+ + +

{{ctx.Locale.Tr "admin.auths.allow_username_change.description"}}

+
+
diff --git a/tests/integration/admin_auth_source_test.go b/tests/integration/admin_auth_source_test.go new file mode 100644 index 0000000000..4b46541318 --- /dev/null +++ b/tests/integration/admin_auth_source_test.go @@ -0,0 +1,32 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later. + +package integration + +import ( + "fmt" + "net/http" + "testing" + + "forgejo.org/models/auth" + "forgejo.org/tests" +) + +func TestAdminAuthAllowUsernameChangeSetting(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user1") + + source := addAuthSource(t, map[string]string{ + "type": fmt.Sprintf("%d", auth.OAuth2), + "name": "some-name", + "is_active": "on", + "allow_username_change": "on", + "oauth2_provider": "gitlab", + }) + + response := session.MakeRequest(t, NewRequestf(t, "GET", "/admin/auths/%d", source.ID), http.StatusOK) + htmlDoc := NewHTMLParser(t, response.Body) + + htmlDoc.AssertElement(t, "#allow_username_change[checked]", true) +}