Skip to content

Commit ffdd25e

Browse files
meln5674meln5674
committed
Allow admins to rename non-local users
1 parent ed977d9 commit ffdd25e

File tree

12 files changed

+117
-27
lines changed

12 files changed

+117
-27
lines changed

‎models/fixtures/user.yml‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1556,3 +1556,77 @@
15561556
repo_admin_change_team_access: false
15571557
theme: ""
15581558
keep_activity_private: false
1559+
1560+
-
1561+
id: 43
1562+
lower_name: user43
1563+
name: user43
1564+
full_name: Non-Local User One
1565+
email: [email protected]
1566+
keep_email_private: false
1567+
email_notifications_preference: enabled
1568+
passwd: ZogKvWdyEx:password
1569+
passwd_hash_algo: dummy
1570+
must_change_password: false
1571+
login_source: 1
1572+
login_name: user43
1573+
type: 0
1574+
salt: ZogKvWdyEx
1575+
max_repo_creation: -1
1576+
is_active: true
1577+
is_admin: true
1578+
is_restricted: false
1579+
allow_git_hook: false
1580+
allow_import_local: false
1581+
allow_create_organization: true
1582+
prohibit_login: false
1583+
avatar: ""
1584+
avatar_email: [email protected]
1585+
use_custom_avatar: true
1586+
num_followers: 0
1587+
num_following: 0
1588+
num_stars: 0
1589+
num_repos: 0
1590+
num_teams: 0
1591+
num_members: 0
1592+
visibility: 0
1593+
repo_admin_change_team_access: false
1594+
theme: ""
1595+
keep_activity_private: false
1596+
1597+
-
1598+
id: 44
1599+
lower_name: user44
1600+
name: user44
1601+
full_name: Non-Local User Two
1602+
email: [email protected]
1603+
keep_email_private: false
1604+
email_notifications_preference: enabled
1605+
passwd: ZogKvWdyEx:password
1606+
passwd_hash_algo: dummy
1607+
must_change_password: false
1608+
login_source: 1
1609+
login_name: user44
1610+
type: 0
1611+
salt: ZogKvWdyEx
1612+
max_repo_creation: -1
1613+
is_active: true
1614+
is_admin: false
1615+
is_restricted: false
1616+
allow_git_hook: false
1617+
allow_import_local: false
1618+
allow_create_organization: true
1619+
prohibit_login: false
1620+
avatar: ""
1621+
avatar_email: [email protected]
1622+
use_custom_avatar: true
1623+
num_followers: 0
1624+
num_following: 0
1625+
num_stars: 0
1626+
num_repos: 0
1627+
num_teams: 0
1628+
num_members: 0
1629+
visibility: 0
1630+
repo_admin_change_team_access: false
1631+
theme: ""
1632+
keep_activity_private: false

‎models/user/user_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,13 +155,13 @@ func TestSearchUsers(t *testing.T) {
155155
}
156156

157157
testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}},
158-
[]int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40})
158+
[]int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40, 43, 44})
159159

160160
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(false)},
161161
[]int64{9})
162162

163163
testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
164-
[]int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40})
164+
[]int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40, 43, 44})
165165

166166
testUserSuccess(user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
167167
[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
@@ -171,7 +171,7 @@ func TestSearchUsers(t *testing.T) {
171171
[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
172172

173173
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: optional.Some(true)},
174-
[]int64{1})
174+
[]int64{1, 43})
175175

176176
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: optional.Some(true)},
177177
[]int64{29})

‎routers/api/v1/admin/user.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ func RenameUser(ctx *context.APIContext) {
480480
newName := web.GetForm(ctx).(*api.RenameUserOption).NewName
481481

482482
// Check if username has been changed
483-
if err := user_service.RenameUser(ctx, ctx.ContextUser, newName); err != nil {
483+
if err := user_service.RenameUser(ctx, ctx.ContextUser, newName, ctx.Doer); err != nil {
484484
if user_model.IsErrUserAlreadyExist(err) || db.IsErrNameReserved(err) || db.IsErrNamePatternNotAllowed(err) || db.IsErrNameCharsNotAllowed(err) {
485485
ctx.APIError(http.StatusUnprocessableEntity, err)
486486
} else {

‎routers/api/v1/org/org.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func Rename(ctx *context.APIContext) {
340340

341341
form := web.GetForm(ctx).(*api.RenameOrgOption)
342342
orgUser := ctx.Org.Organization.AsUser()
343-
if err := user_service.RenameUser(ctx, orgUser, form.NewName); err != nil {
343+
if err := user_service.RenameUser(ctx, orgUser, form.NewName, ctx.Doer); err != nil {
344344
if user_model.IsErrUserAlreadyExist(err) || db.IsErrNameReserved(err) || db.IsErrNamePatternNotAllowed(err) || db.IsErrNameCharsNotAllowed(err) {
345345
ctx.APIError(http.StatusUnprocessableEntity, err)
346346
} else {

‎routers/web/admin/users.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ func EditUserPost(ctx *context.Context) {
345345
}
346346

347347
if form.UserName != "" {
348-
if err := user_service.RenameUser(ctx, u, form.UserName); err != nil {
348+
if err := user_service.RenameUser(ctx, u, form.UserName, ctx.Doer); err != nil {
349349
switch {
350350
case user_model.IsErrUserIsNotLocal(err):
351351
ctx.Data["Err_UserName"] = true

‎routers/web/org/setting.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ func SettingsRenamePost(ctx *context.Context) {
213213
return
214214
}
215215

216-
if err := user_service.RenameUser(ctx, ctx.Org.Organization.AsUser(), newOrgName); err != nil {
216+
if err := user_service.RenameUser(ctx, ctx.Org.Organization.AsUser(), newOrgName, ctx.Doer); err != nil {
217217
if user_model.IsErrUserAlreadyExist(err) {
218218
ctx.JSONError(ctx.Tr("org.form.name_been_taken", newOrgName))
219219
} else if db.IsErrNameReserved(err) {

‎routers/web/user/setting/profile.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func ProfilePost(ctx *context.Context) {
7575
ctx.Redirect(setting.AppSubURL + "/user/settings")
7676
return
7777
}
78-
if err := user_service.RenameUser(ctx, ctx.Doer, form.Name); err != nil {
78+
if err := user_service.RenameUser(ctx, ctx.Doer, form.Name, ctx.Doer); err != nil {
7979
switch {
8080
case user_model.IsErrUserIsNotLocal(err):
8181
ctx.Flash.Error(ctx.Tr("form.username_change_not_local_user"))

‎services/user/update_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ func TestUpdateUser(t *testing.T) {
1919
assert.NoError(t, unittest.PrepareTestDatabase())
2020

2121
admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
22+
nonLocalAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 43})
23+
24+
// Need to make the nonlocal admin not an admin for the following checks to make sense
25+
assert.NoError(t, UpdateUser(t.Context(), nonLocalAdmin, &UpdateOptions{
26+
IsAdmin: UpdateOptionFieldFromValue(false),
27+
}))
2228

2329
assert.Error(t, UpdateUser(t.Context(), admin, &UpdateOptions{
2430
IsAdmin: UpdateOptionFieldFromValue(false),

‎services/user/user.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,13 @@ import (
3131
)
3232

3333
// RenameUser renames a user
34-
func RenameUser(ctx context.Context, u *user_model.User, newUserName string) error {
34+
func RenameUser(ctx context.Context, u *user_model.User, newUserName string, doer *user_model.User) error {
3535
if newUserName == u.Name {
3636
return nil
3737
}
3838

39-
// Non-local users are not allowed to change their username.
40-
if !u.IsOrganization() && !u.IsLocal() {
39+
// Non-local users are not allowed to change their own username, but admins are
40+
if !u.IsOrganization() && !u.IsLocal() && !doer.IsAdmin {
4141
return user_model.ErrUserIsNotLocal{
4242
UID: u.ID,
4343
Name: u.Name,

‎services/user/user_test.go‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -100,24 +100,34 @@ func TestCreateUser(t *testing.T) {
100100
func TestRenameUser(t *testing.T) {
101101
assert.NoError(t, unittest.PrepareTestDatabase())
102102
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 21})
103-
104-
t.Run("Non-Local", func(t *testing.T) {
105-
u := &user_model.User{
106-
Type: user_model.UserTypeIndividual,
107-
LoginType: auth.OAuth2,
108-
}
109-
assert.ErrorIs(t, RenameUser(t.Context(), u, "user_rename"), user_model.ErrUserIsNotLocal{})
103+
adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1, IsAdmin: true})
104+
nonLocalAdminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 43, IsAdmin: true}) //, LoginType: auth.OAuth2}) ???
105+
nonLocalAdminUser.LoginType = auth.OAuth2
106+
nonLocalUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 44, IsAdmin: false}) // , LoginType: auth.OAuth2}) ???
107+
nonLocalUser.LoginType = auth.OAuth2
108+
109+
t.Run("Non-Local, Self, Non-admin", func(t *testing.T) {
110+
assert.ErrorIs(t, RenameUser(t.Context(), nonLocalUser, "nonlocal_user_rename", nonLocalUser), user_model.ErrUserIsNotLocal{UID: nonLocalUser.ID, Name: nonLocalUser.Name})
111+
})
112+
t.Run("Non-Local, Self, Admin", func(t *testing.T) {
113+
assert.NoError(t, RenameUser(t.Context(), nonLocalAdminUser, "nonlocal_user_user_rename", nonLocalAdminUser))
114+
})
115+
t.Run("Non-Local, Other, Non-admin", func(t *testing.T) {
116+
assert.ErrorIs(t, RenameUser(t.Context(), nonLocalUser, "user_rename", user), user_model.ErrUserIsNotLocal{UID: nonLocalUser.ID, Name: nonLocalUser.Name})
117+
})
118+
t.Run("Non-Local, Other, Admin", func(t *testing.T) {
119+
assert.NoError(t, RenameUser(t.Context(), nonLocalAdminUser, "nonlocal_user_rename_2", adminUser))
110120
})
111121

112122
t.Run("Same username", func(t *testing.T) {
113-
assert.NoError(t, RenameUser(t.Context(), user, user.Name))
123+
assert.NoError(t, RenameUser(t.Context(), user, user.Name, user))
114124
})
115125

116126
t.Run("Non usable username", func(t *testing.T) {
117127
usernames := []string{"--diff", ".well-known", "gitea-actions", "aaa.atom", "aa.png"}
118128
for _, username := range usernames {
119129
assert.Error(t, user_model.IsUsableUsername(username), "non-usable username: %s", username)
120-
assert.Error(t, RenameUser(t.Context(), user, username), "non-usable username: %s", username)
130+
assert.Error(t, RenameUser(t.Context(), user, username, user), "non-usable username: %s", username)
121131
}
122132
})
123133

@@ -126,7 +136,7 @@ func TestRenameUser(t *testing.T) {
126136
unittest.AssertNotExistsBean(t, &user_model.User{ID: user.ID, Name: caps})
127137
unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID, OwnerName: user.Name})
128138

129-
assert.NoError(t, RenameUser(t.Context(), user, caps))
139+
assert.NoError(t, RenameUser(t.Context(), user, caps, user))
130140

131141
unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID, Name: caps})
132142
unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID, OwnerName: caps})
@@ -135,17 +145,17 @@ func TestRenameUser(t *testing.T) {
135145
t.Run("Already exists", func(t *testing.T) {
136146
existUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
137147

138-
assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.Name), user_model.ErrUserAlreadyExist{Name: existUser.Name})
139-
assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.LowerName), user_model.ErrUserAlreadyExist{Name: existUser.LowerName})
148+
assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.Name, user), user_model.ErrUserAlreadyExist{Name: existUser.Name})
149+
assert.ErrorIs(t, RenameUser(t.Context(), user, existUser.LowerName, user), user_model.ErrUserAlreadyExist{Name: existUser.LowerName})
140150
newUsername := fmt.Sprintf("uSEr%d", existUser.ID)
141-
assert.ErrorIs(t, RenameUser(t.Context(), user, newUsername), user_model.ErrUserAlreadyExist{Name: newUsername})
151+
assert.ErrorIs(t, RenameUser(t.Context(), user, newUsername, user), user_model.ErrUserAlreadyExist{Name: newUsername})
142152
})
143153

144154
t.Run("Normal", func(t *testing.T) {
145155
oldUsername := user.Name
146156
newUsername := "User_Rename"
147157

148-
assert.NoError(t, RenameUser(t.Context(), user, newUsername))
158+
assert.NoError(t, RenameUser(t.Context(), user, newUsername, user))
149159
unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID, Name: newUsername, LowerName: strings.ToLower(newUsername)})
150160

151161
redirectUID, err := user_model.LookupUserRedirect(t.Context(), oldUsername)

0 commit comments

Comments
 (0)