Skip to content

Commit 0082cb5

Browse files
wxiaoguangwxiaoguang
authored
Fix last admin check when syncing users (#34649)
Fix #34358
1 parent 92e7e98 commit 0082cb5

File tree

8 files changed

+49
-20
lines changed

8 files changed

+49
-20
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ func EditUser(ctx *context.APIContext) {
239239
Location: optional.FromPtr(form.Location),
240240
Description: optional.FromPtr(form.Description),
241241
IsActive: optional.FromPtr(form.Active),
242-
IsAdmin: optional.FromPtr(form.Admin),
242+
IsAdmin: user_service.UpdateOptionFieldFromPtr(form.Admin),
243243
Visibility: optional.FromNonDefault(api.VisibilityModes[form.Visibility]),
244244
AllowGitHook: optional.FromPtr(form.AllowGitHook),
245245
AllowImportLocal: optional.FromPtr(form.AllowImportLocal),

‎routers/web/admin/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ func EditUserPost(ctx *context.Context) {
432432
Website: optional.Some(form.Website),
433433
Location: optional.Some(form.Location),
434434
IsActive: optional.Some(form.Active),
435-
IsAdmin: optional.Some(form.Admin),
435+
IsAdmin: user_service.UpdateOptionFieldFromValue(form.Admin),
436436
AllowGitHook: optional.Some(form.AllowGitHook),
437437
AllowImportLocal: optional.Some(form.AllowImportLocal),
438438
MaxRepoCreation: optional.Some(form.MaxRepoCreation),

‎routers/web/auth/auth.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -613,7 +613,7 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth.
613613
if user_model.CountUsers(ctx, nil) == 1 {
614614
opts := &user_service.UpdateOptions{
615615
IsActive: optional.Some(true),
616-
IsAdmin: optional.Some(true),
616+
IsAdmin: user_service.UpdateOptionFieldFromValue(true),
617617
SetLastLogin: true,
618618
}
619619
if err := user_service.UpdateUser(ctx, u, opts); err != nil {

‎routers/web/auth/oauth.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,8 +193,8 @@ func SignInOAuthCallback(ctx *context.Context) {
193193
source := authSource.Cfg.(*oauth2.Source)
194194

195195
isAdmin, isRestricted := getUserAdminAndRestrictedFromGroupClaims(source, &gothUser)
196-
u.IsAdmin = isAdmin.ValueOrDefault(false)
197-
u.IsRestricted = isRestricted.ValueOrDefault(false)
196+
u.IsAdmin = isAdmin.ValueOrDefault(user_service.UpdateOptionField[bool]{FieldValue: false}).FieldValue
197+
u.IsRestricted = isRestricted.ValueOrDefault(setting.Service.DefaultUserIsRestricted)
198198

199199
if !createAndHandleCreatedUser(ctx, templates.TplName(""), nil, u, overwriteDefault, &gothUser, setting.OAuth2Client.AccountLinking != setting.OAuth2AccountLinkingDisabled) {
200200
// error already handled
@@ -258,11 +258,11 @@ func getClaimedGroups(source *oauth2.Source, gothUser *goth.User) container.Set[
258258
return claimValueToStringSet(groupClaims)
259259
}
260260

261-
func getUserAdminAndRestrictedFromGroupClaims(source *oauth2.Source, gothUser *goth.User) (isAdmin, isRestricted optional.Option[bool]) {
261+
func getUserAdminAndRestrictedFromGroupClaims(source *oauth2.Source, gothUser *goth.User) (isAdmin optional.Option[user_service.UpdateOptionField[bool]], isRestricted optional.Option[bool]) {
262262
groups := getClaimedGroups(source, gothUser)
263263

264264
if source.AdminGroup != "" {
265-
isAdmin = optional.Some(groups.Contains(source.AdminGroup))
265+
isAdmin = user_service.UpdateOptionFieldFromSync(groups.Contains(source.AdminGroup))
266266
}
267267
if source.RestrictedGroup != "" {
268268
isRestricted = optional.Some(groups.Contains(source.RestrictedGroup))

‎services/auth/source/ldap/source_authenticate.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (source *Source) Authenticate(ctx context.Context, user *user_model.User, u
5858
opts := &user_service.UpdateOptions{}
5959
if source.AdminFilter != "" && user.IsAdmin != sr.IsAdmin {
6060
// Change existing admin flag only if AdminFilter option is set
61-
opts.IsAdmin = optional.Some(sr.IsAdmin)
61+
opts.IsAdmin = user_service.UpdateOptionFieldFromSync(sr.IsAdmin)
6262
}
6363
if !sr.IsAdmin && source.RestrictedFilter != "" && user.IsRestricted != sr.IsRestricted {
6464
// Change existing restricted flag only if RestrictedFilter option is set

‎services/auth/source/ldap/source_sync.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
162162
IsActive: optional.Some(true),
163163
}
164164
if source.AdminFilter != "" {
165-
opts.IsAdmin = optional.Some(su.IsAdmin)
165+
opts.IsAdmin = user_service.UpdateOptionFieldFromSync(su.IsAdmin)
166166
}
167167
// Change existing restricted flag only if RestrictedFilter option is set
168168
if !su.IsAdmin && source.RestrictedFilter != "" {

‎services/user/update.go

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,26 @@ import (
1515
"code.gitea.io/gitea/modules/structs"
1616
)
1717

18+
type UpdateOptionField[T any] struct {
19+
FieldValue T
20+
FromSync bool
21+
}
22+
23+
func UpdateOptionFieldFromValue[T any](value T) optional.Option[UpdateOptionField[T]] {
24+
return optional.Some(UpdateOptionField[T]{FieldValue: value})
25+
}
26+
27+
func UpdateOptionFieldFromSync[T any](value T) optional.Option[UpdateOptionField[T]] {
28+
return optional.Some(UpdateOptionField[T]{FieldValue: value, FromSync: true})
29+
}
30+
31+
func UpdateOptionFieldFromPtr[T any](value *T) optional.Option[UpdateOptionField[T]] {
32+
if value == nil {
33+
return optional.None[UpdateOptionField[T]]()
34+
}
35+
return UpdateOptionFieldFromValue(*value)
36+
}
37+
1838
type UpdateOptions struct {
1939
KeepEmailPrivate optional.Option[bool]
2040
FullName optional.Option[string]
@@ -32,7 +52,7 @@ type UpdateOptions struct {
3252
DiffViewStyle optional.Option[string]
3353
AllowCreateOrganization optional.Option[bool]
3454
IsActive optional.Option[bool]
35-
IsAdmin optional.Option[bool]
55+
IsAdmin optional.Option[UpdateOptionField[bool]]
3656
EmailNotificationsPreference optional.Option[string]
3757
SetLastLogin bool
3858
RepoAdminChangeTeamAccess optional.Option[bool]
@@ -111,13 +131,18 @@ func UpdateUser(ctx context.Context, u *user_model.User, opts *UpdateOptions) er
111131
cols = append(cols, "is_restricted")
112132
}
113133
if opts.IsAdmin.Has() {
114-
if !opts.IsAdmin.Value() && user_model.IsLastAdminUser(ctx, u) {
115-
return user_model.ErrDeleteLastAdminUser{UID: u.ID}
134+
if opts.IsAdmin.Value().FieldValue /* true */ {
135+
u.IsAdmin = opts.IsAdmin.Value().FieldValue // set IsAdmin=true
136+
cols = append(cols, "is_admin")
137+
} else if !user_model.IsLastAdminUser(ctx, u) /* not the last admin */ {
138+
u.IsAdmin = opts.IsAdmin.Value().FieldValue // it's safe to change it from false to true (not the last admin)
139+
cols = append(cols, "is_admin")
140+
} else /* IsAdmin=false but this is the last admin user */ { //nolint
141+
if !opts.IsAdmin.Value().FromSync {
142+
return user_model.ErrDeleteLastAdminUser{UID: u.ID}
143+
}
144+
// else: syncing from external-source, this user is the last admin, so skip the "IsAdmin=false" change
116145
}
117-
118-
u.IsAdmin = opts.IsAdmin.Value()
119-
120-
cols = append(cols, "is_admin")
121146
}
122147

123148
if opts.Visibility.Has() {

‎services/user/update_test.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ func TestUpdateUser(t *testing.T) {
2222
admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
2323

2424
assert.Error(t, UpdateUser(db.DefaultContext, admin, &UpdateOptions{
25-
IsAdmin: optional.Some(false),
25+
IsAdmin: UpdateOptionFieldFromValue(false),
26+
}))
27+
28+
assert.NoError(t, UpdateUser(db.DefaultContext, admin, &UpdateOptions{
29+
IsAdmin: UpdateOptionFieldFromSync(false),
2630
}))
2731

2832
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
@@ -38,7 +42,7 @@ func TestUpdateUser(t *testing.T) {
3842
MaxRepoCreation: optional.Some(10),
3943
IsRestricted: optional.Some(true),
4044
IsActive: optional.Some(false),
41-
IsAdmin: optional.Some(true),
45+
IsAdmin: UpdateOptionFieldFromValue(true),
4246
Visibility: optional.Some(structs.VisibleTypePrivate),
4347
KeepActivityPrivate: optional.Some(true),
4448
Language: optional.Some("lang"),
@@ -60,7 +64,7 @@ func TestUpdateUser(t *testing.T) {
6064
assert.Equal(t, opts.MaxRepoCreation.Value(), user.MaxRepoCreation)
6165
assert.Equal(t, opts.IsRestricted.Value(), user.IsRestricted)
6266
assert.Equal(t, opts.IsActive.Value(), user.IsActive)
63-
assert.Equal(t, opts.IsAdmin.Value(), user.IsAdmin)
67+
assert.Equal(t, opts.IsAdmin.Value().FieldValue, user.IsAdmin)
6468
assert.Equal(t, opts.Visibility.Value(), user.Visibility)
6569
assert.Equal(t, opts.KeepActivityPrivate.Value(), user.KeepActivityPrivate)
6670
assert.Equal(t, opts.Language.Value(), user.Language)
@@ -80,7 +84,7 @@ func TestUpdateUser(t *testing.T) {
8084
assert.Equal(t, opts.MaxRepoCreation.Value(), user.MaxRepoCreation)
8185
assert.Equal(t, opts.IsRestricted.Value(), user.IsRestricted)
8286
assert.Equal(t, opts.IsActive.Value(), user.IsActive)
83-
assert.Equal(t, opts.IsAdmin.Value(), user.IsAdmin)
87+
assert.Equal(t, opts.IsAdmin.Value().FieldValue, user.IsAdmin)
8488
assert.Equal(t, opts.Visibility.Value(), user.Visibility)
8589
assert.Equal(t, opts.KeepActivityPrivate.Value(), user.KeepActivityPrivate)
8690
assert.Equal(t, opts.Language.Value(), user.Language)

0 commit comments

Comments
 (0)