Skip to content

Fix last admin check when syncing users #34649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion routers/api/v1/admin/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func EditUser(ctx *context.APIContext) {
Location: optional.FromPtr(form.Location),
Description: optional.FromPtr(form.Description),
IsActive: optional.FromPtr(form.Active),
IsAdmin: optional.FromPtr(form.Admin),
IsAdmin: user_service.UpdateOptionFieldFromPtr(form.Admin),
Visibility: optional.FromNonDefault(api.VisibilityModes[form.Visibility]),
AllowGitHook: optional.FromPtr(form.AllowGitHook),
AllowImportLocal: optional.FromPtr(form.AllowImportLocal),
Expand Down
2 changes: 1 addition & 1 deletion routers/web/admin/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func EditUserPost(ctx *context.Context) {
Website: optional.Some(form.Website),
Location: optional.Some(form.Location),
IsActive: optional.Some(form.Active),
IsAdmin: optional.Some(form.Admin),
IsAdmin: user_service.UpdateOptionFieldFromValue(form.Admin),
AllowGitHook: optional.Some(form.AllowGitHook),
AllowImportLocal: optional.Some(form.AllowImportLocal),
MaxRepoCreation: optional.Some(form.MaxRepoCreation),
Expand Down
2 changes: 1 addition & 1 deletion routers/web/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth.
if user_model.CountUsers(ctx, nil) == 1 {
opts := &user_service.UpdateOptions{
IsActive: optional.Some(true),
IsAdmin: optional.Some(true),
IsAdmin: user_service.UpdateOptionFieldFromValue(true),
SetLastLogin: true,
}
if err := user_service.UpdateUser(ctx, u, opts); err != nil {
Expand Down
8 changes: 4 additions & 4 deletions routers/web/auth/oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,8 @@ func SignInOAuthCallback(ctx *context.Context) {
source := authSource.Cfg.(*oauth2.Source)

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

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

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

if source.AdminGroup != "" {
isAdmin = optional.Some(groups.Contains(source.AdminGroup))
isAdmin = user_service.UpdateOptionFieldFromSync(groups.Contains(source.AdminGroup))
}
if source.RestrictedGroup != "" {
isRestricted = optional.Some(groups.Contains(source.RestrictedGroup))
Expand Down
2 changes: 1 addition & 1 deletion services/auth/source/ldap/source_authenticate.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (source *Source) Authenticate(ctx context.Context, user *user_model.User, u
opts := &user_service.UpdateOptions{}
if source.AdminFilter != "" && user.IsAdmin != sr.IsAdmin {
// Change existing admin flag only if AdminFilter option is set
opts.IsAdmin = optional.Some(sr.IsAdmin)
opts.IsAdmin = user_service.UpdateOptionFieldFromSync(sr.IsAdmin)
}
if !sr.IsAdmin && source.RestrictedFilter != "" && user.IsRestricted != sr.IsRestricted {
// Change existing restricted flag only if RestrictedFilter option is set
Expand Down
2 changes: 1 addition & 1 deletion services/auth/source/ldap/source_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
IsActive: optional.Some(true),
}
if source.AdminFilter != "" {
opts.IsAdmin = optional.Some(su.IsAdmin)
opts.IsAdmin = user_service.UpdateOptionFieldFromSync(su.IsAdmin)
}
// Change existing restricted flag only if RestrictedFilter option is set
if !su.IsAdmin && source.RestrictedFilter != "" {
Expand Down
39 changes: 32 additions & 7 deletions services/user/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,26 @@ import (
"code.gitea.io/gitea/modules/structs"
)

type UpdateOptionField[T any] struct {
FieldValue T
FromSync bool
}

func UpdateOptionFieldFromValue[T any](value T) optional.Option[UpdateOptionField[T]] {
return optional.Some(UpdateOptionField[T]{FieldValue: value})
}

func UpdateOptionFieldFromSync[T any](value T) optional.Option[UpdateOptionField[T]] {
return optional.Some(UpdateOptionField[T]{FieldValue: value, FromSync: true})
}

func UpdateOptionFieldFromPtr[T any](value *T) optional.Option[UpdateOptionField[T]] {
if value == nil {
return optional.None[UpdateOptionField[T]]()
}
return UpdateOptionFieldFromValue(*value)
}

type UpdateOptions struct {
KeepEmailPrivate optional.Option[bool]
FullName optional.Option[string]
Expand All @@ -32,7 +52,7 @@ type UpdateOptions struct {
DiffViewStyle optional.Option[string]
AllowCreateOrganization optional.Option[bool]
IsActive optional.Option[bool]
IsAdmin optional.Option[bool]
IsAdmin optional.Option[UpdateOptionField[bool]]
EmailNotificationsPreference optional.Option[string]
SetLastLogin bool
RepoAdminChangeTeamAccess optional.Option[bool]
Expand Down Expand Up @@ -111,13 +131,18 @@ func UpdateUser(ctx context.Context, u *user_model.User, opts *UpdateOptions) er
cols = append(cols, "is_restricted")
}
if opts.IsAdmin.Has() {
if !opts.IsAdmin.Value() && user_model.IsLastAdminUser(ctx, u) {
return user_model.ErrDeleteLastAdminUser{UID: u.ID}
if opts.IsAdmin.Value().FieldValue /* true */ {
u.IsAdmin = opts.IsAdmin.Value().FieldValue // set IsAdmin=true
cols = append(cols, "is_admin")
} else if !user_model.IsLastAdminUser(ctx, u) /* not the last admin */ {
u.IsAdmin = opts.IsAdmin.Value().FieldValue // it's safe to change it from false to true (not the last admin)
cols = append(cols, "is_admin")
} else /* IsAdmin=false but this is the last admin user */ { //nolint
if !opts.IsAdmin.Value().FromSync {
return user_model.ErrDeleteLastAdminUser{UID: u.ID}
}
// else: syncing from external-source, this user is the last admin, so skip the "IsAdmin=false" change
}

u.IsAdmin = opts.IsAdmin.Value()

cols = append(cols, "is_admin")
}

if opts.Visibility.Has() {
Expand Down
12 changes: 8 additions & 4 deletions services/user/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ func TestUpdateUser(t *testing.T) {
admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})

assert.Error(t, UpdateUser(db.DefaultContext, admin, &UpdateOptions{
IsAdmin: optional.Some(false),
IsAdmin: UpdateOptionFieldFromValue(false),
}))

assert.NoError(t, UpdateUser(db.DefaultContext, admin, &UpdateOptions{
IsAdmin: UpdateOptionFieldFromSync(false),
}))

user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
Expand All @@ -38,7 +42,7 @@ func TestUpdateUser(t *testing.T) {
MaxRepoCreation: optional.Some(10),
IsRestricted: optional.Some(true),
IsActive: optional.Some(false),
IsAdmin: optional.Some(true),
IsAdmin: UpdateOptionFieldFromValue(true),
Visibility: optional.Some(structs.VisibleTypePrivate),
KeepActivityPrivate: optional.Some(true),
Language: optional.Some("lang"),
Expand All @@ -60,7 +64,7 @@ func TestUpdateUser(t *testing.T) {
assert.Equal(t, opts.MaxRepoCreation.Value(), user.MaxRepoCreation)
assert.Equal(t, opts.IsRestricted.Value(), user.IsRestricted)
assert.Equal(t, opts.IsActive.Value(), user.IsActive)
assert.Equal(t, opts.IsAdmin.Value(), user.IsAdmin)
assert.Equal(t, opts.IsAdmin.Value().FieldValue, user.IsAdmin)
assert.Equal(t, opts.Visibility.Value(), user.Visibility)
assert.Equal(t, opts.KeepActivityPrivate.Value(), user.KeepActivityPrivate)
assert.Equal(t, opts.Language.Value(), user.Language)
Expand All @@ -80,7 +84,7 @@ func TestUpdateUser(t *testing.T) {
assert.Equal(t, opts.MaxRepoCreation.Value(), user.MaxRepoCreation)
assert.Equal(t, opts.IsRestricted.Value(), user.IsRestricted)
assert.Equal(t, opts.IsActive.Value(), user.IsActive)
assert.Equal(t, opts.IsAdmin.Value(), user.IsAdmin)
assert.Equal(t, opts.IsAdmin.Value().FieldValue, user.IsAdmin)
assert.Equal(t, opts.Visibility.Value(), user.Visibility)
assert.Equal(t, opts.KeepActivityPrivate.Value(), user.KeepActivityPrivate)
assert.Equal(t, opts.Language.Value(), user.Language)
Expand Down