Skip to content

Commit

Permalink
Forbid removing the last admin user (#28337) (#28793)
Browse files Browse the repository at this point in the history
Backport #28337 by @yp05327

Co-authored-by: yp05327 <576951401@qq.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
  • Loading branch information
3 people authored Jan 16, 2024
1 parent be541d9 commit 376fa0d
Show file tree
Hide file tree
Showing 8 changed files with 80 additions and 7 deletions.
15 changes: 15 additions & 0 deletions models/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,21 @@ func (err ErrUserOwnPackages) Error() string {
return fmt.Sprintf("user still has ownership of packages [uid: %d]", err.UID)
}

// ErrDeleteLastAdminUser represents a "DeleteLastAdminUser" kind of error.
type ErrDeleteLastAdminUser struct {
UID int64
}

// IsErrDeleteLastAdminUser checks if an error is a ErrDeleteLastAdminUser.
func IsErrDeleteLastAdminUser(err error) bool {
_, ok := err.(ErrDeleteLastAdminUser)
return ok
}

func (err ErrDeleteLastAdminUser) Error() string {
return fmt.Sprintf("can not delete the last admin user [uid: %d]", err.UID)
}

// ErrNoPendingRepoTransfer is an error type for repositories without a pending
// transfer request
type ErrNoPendingRepoTransfer struct {
Expand Down
29 changes: 25 additions & 4 deletions models/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -705,9 +705,18 @@ func CreateUser(ctx context.Context, u *User, overwriteDefault ...*CreateUserOve
return committer.Commit()
}

// IsLastAdminUser check whether user is the last admin
func IsLastAdminUser(ctx context.Context, user *User) bool {
if user.IsAdmin && CountUsers(ctx, &CountUserFilter{IsAdmin: util.OptionalBoolTrue}) <= 1 {
return true
}
return false
}

// CountUserFilter represent optional filters for CountUsers
type CountUserFilter struct {
LastLoginSince *int64
IsAdmin util.OptionalBool
}

// CountUsers returns number of users.
Expand All @@ -716,13 +725,25 @@ func CountUsers(ctx context.Context, opts *CountUserFilter) int64 {
}

func countUsers(ctx context.Context, opts *CountUserFilter) int64 {
sess := db.GetEngine(ctx).Where(builder.Eq{"type": "0"})
sess := db.GetEngine(ctx)
cond := builder.NewCond()
cond = cond.And(builder.Eq{"type": UserTypeIndividual})

if opts != nil && opts.LastLoginSince != nil {
sess = sess.Where(builder.Gte{"last_login_unix": *opts.LastLoginSince})
if opts != nil {
if opts.LastLoginSince != nil {
cond = cond.And(builder.Gte{"last_login_unix": *opts.LastLoginSince})
}

if !opts.IsAdmin.IsNone() {
cond = cond.And(builder.Eq{"is_admin": opts.IsAdmin.IsTrue()})
}
}

count, err := sess.Where(cond).Count(new(User))
if err != nil {
log.Error("user.countUsers: %v", err)
}

count, _ := sess.Count(new(User))
return count
}

Expand Down
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ authorization_failed_desc = The authorization failed because we detected an inva
sspi_auth_failed = SSPI authentication failed
password_pwned = The password you chose is on a <a target="_blank" rel="noopener noreferrer" href="https://haveibeenpwned.com/Passwords">list of stolen passwords</a> previously exposed in public data breaches. Please try again with a different password and consider changing this password elsewhere too.
password_pwned_err = Could not complete request to HaveIBeenPwned
last_admin = You cannot remove the last admin. There must be at least one admin.
[mail]
view_it_on = View it on %s
Expand Down Expand Up @@ -587,6 +588,8 @@ org_still_own_packages = "This organization still owns one or more packages, del
target_branch_not_exist = Target branch does not exist.
admin_cannot_delete_self = You cannot delete yourself when you are an admin. Please remove your admin privileges first.
[user]
change_avatar = Change your avatar…
joined_on = Joined on %s
Expand Down
9 changes: 8 additions & 1 deletion routers/api/v1/admin/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ func EditUser(ctx *context.APIContext) {
// responses:
// "200":
// "$ref": "#/responses/User"
// "400":
// "$ref": "#/responses/error"
// "403":
// "$ref": "#/responses/forbidden"
// "422":
Expand Down Expand Up @@ -264,6 +266,10 @@ func EditUser(ctx *context.APIContext) {
ctx.ContextUser.Visibility = api.VisibilityModes[form.Visibility]
}
if form.Admin != nil {
if !*form.Admin && user_model.IsLastAdminUser(ctx, ctx.ContextUser) {
ctx.Error(http.StatusBadRequest, "LastAdmin", ctx.Tr("auth.last_admin"))
return
}
ctx.ContextUser.IsAdmin = *form.Admin
}
if form.AllowGitHook != nil {
Expand Down Expand Up @@ -341,7 +347,8 @@ func DeleteUser(ctx *context.APIContext) {
if err := user_service.DeleteUser(ctx, ctx.ContextUser, ctx.FormBool("purge")); err != nil {
if models.IsErrUserOwnRepos(err) ||
models.IsErrUserHasOrgs(err) ||
models.IsErrUserOwnPackages(err) {
models.IsErrUserOwnPackages(err) ||
models.IsErrDeleteLastAdminUser(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
} else {
ctx.Error(http.StatusInternalServerError, "DeleteUser", err)
Expand Down
11 changes: 10 additions & 1 deletion routers/web/admin/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,12 @@ func EditUserPost(ctx *context.Context) {

}

// Check whether user is the last admin
if !form.Admin && user_model.IsLastAdminUser(ctx, u) {
ctx.RenderWithErr(ctx.Tr("auth.last_admin"), tplUserEdit, &form)
return
}

u.LoginName = form.LoginName
u.FullName = form.FullName
emailChanged := !strings.EqualFold(u.Email, form.Email)
Expand Down Expand Up @@ -496,7 +502,10 @@ func DeleteUser(ctx *context.Context) {
ctx.Redirect(setting.AppSubURL + "/admin/users/" + url.PathEscape(ctx.Params(":userid")))
case models.IsErrUserOwnPackages(err):
ctx.Flash.Error(ctx.Tr("admin.users.still_own_packages"))
ctx.Redirect(setting.AppSubURL + "/admin/users/" + ctx.Params(":userid"))
ctx.Redirect(setting.AppSubURL + "/admin/users/" + url.PathEscape(ctx.Params(":userid")))
case models.IsErrDeleteLastAdminUser(err):
ctx.Flash.Error(ctx.Tr("auth.last_admin"))
ctx.Redirect(setting.AppSubURL + "/admin/users/" + url.PathEscape(ctx.Params(":userid")))
default:
ctx.ServerError("DeleteUser", err)
}
Expand Down
10 changes: 10 additions & 0 deletions routers/web/user/setting/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ func DeleteAccount(ctx *context.Context) {
return
}

// admin should not delete themself
if ctx.Doer.IsAdmin {
ctx.Flash.Error(ctx.Tr("form.admin_cannot_delete_self"))
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}

if err := user.DeleteUser(ctx, ctx.Doer, false); err != nil {
switch {
case models.IsErrUserOwnRepos(err):
Expand All @@ -257,6 +264,9 @@ func DeleteAccount(ctx *context.Context) {
case models.IsErrUserOwnPackages(err):
ctx.Flash.Error(ctx.Tr("form.still_own_packages"))
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
case models.IsErrDeleteLastAdminUser(err):
ctx.Flash.Error(ctx.Tr("auth.last_admin"))
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
default:
ctx.ServerError("DeleteUser", err)
}
Expand Down
7 changes: 6 additions & 1 deletion services/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
return fmt.Errorf("%s is an organization not a user", u.Name)
}

if user_model.IsLastAdminUser(ctx, u) {
return models.ErrDeleteLastAdminUser{UID: u.ID}
}

if purge {
// Disable the user first
// NOTE: This is deliberately not within a transaction as it must disable the user immediately to prevent any further action by the user to be purged.
Expand Down Expand Up @@ -295,7 +299,8 @@ func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error {
}
if err := DeleteUser(ctx, u, false); err != nil {
// Ignore users that were set inactive by admin.
if models.IsErrUserOwnRepos(err) || models.IsErrUserHasOrgs(err) || models.IsErrUserOwnPackages(err) {
if models.IsErrUserOwnRepos(err) || models.IsErrUserHasOrgs(err) ||
models.IsErrUserOwnPackages(err) || models.IsErrDeleteLastAdminUser(err) {
continue
}
return err
Expand Down
3 changes: 3 additions & 0 deletions templates/swagger/v1_json.tmpl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 376fa0d

Please sign in to comment.