From 376fa0d8c49ca8a290ebb328281a56af346f5785 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 16 Jan 2024 09:51:46 +0800 Subject: [PATCH] Forbid removing the last admin user (#28337) (#28793) Backport #28337 by @yp05327 Co-authored-by: yp05327 <576951401@qq.com> Co-authored-by: Lunny Xiao --- models/error.go | 15 +++++++++++++++ models/user/user.go | 29 +++++++++++++++++++++++++---- options/locale/locale_en-US.ini | 3 +++ routers/api/v1/admin/user.go | 9 ++++++++- routers/web/admin/users.go | 11 ++++++++++- routers/web/user/setting/account.go | 10 ++++++++++ services/user/user.go | 7 ++++++- templates/swagger/v1_json.tmpl | 3 +++ 8 files changed, 80 insertions(+), 7 deletions(-) diff --git a/models/error.go b/models/error.go index b7bb967b73..83dfe29805 100644 --- a/models/error.go +++ b/models/error.go @@ -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 { diff --git a/models/user/user.go b/models/user/user.go index 2c4e94953b..509547296b 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -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. @@ -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 } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 0454eb68ca..c846180371 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -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 list of stolen passwords 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 @@ -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 diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 15c0d894e2..cfa74d25f2 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -183,6 +183,8 @@ func EditUser(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/User" + // "400": + // "$ref": "#/responses/error" // "403": // "$ref": "#/responses/forbidden" // "422": @@ -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 { @@ -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) diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index f77af3175f..9c95347580 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -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) @@ -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) } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 5c14f3ad4b..266f86fc94 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -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): @@ -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) } diff --git a/services/user/user.go b/services/user/user.go index cebd510f1e..ceb4cda2c3 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -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. @@ -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 diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 644fe6aa26..c88fcfb8bd 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -660,6 +660,9 @@ "200": { "$ref": "#/responses/User" }, + "400": { + "$ref": "#/responses/error" + }, "403": { "$ref": "#/responses/forbidden" },