fix & improve

Author: yp05327

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 {

models/user/user.go

@@ -705,6 +705,14 @@ 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

routers/api/v1/admin/user.go

@@ -255,13 +255,10 @@ func EditUser(ctx *context.APIContext) {
 	if len(form.Visibility) != 0 {
 		ctx.ContextUser.Visibility = api.VisibilityModes[form.Visibility]
 	}
-	if form.Admin != nil {
-		// Check whether user is the last admin
-		if ctx.ContextUser.IsAdmin && !*form.Admin &&
-			user_model.CountUsers(ctx, &user_model.CountUserFilter{IsAdmin: util.OptionalBoolTrue}) <= 1 {
-			ctx.Error(http.StatusBadRequest, "LastAdmin", ctx.Tr("auth.last_admin"))
-			return
-		}
+	if form.Admin != nil && !*form.Admin && user_model.IsLastAdminUser(ctx, ctx.ContextUser) {
+		ctx.Error(http.StatusBadRequest, "LastAdmin", ctx.Tr("auth.last_admin"))
+		return
+	} else {
 		ctx.ContextUser.IsAdmin = *form.Admin
 	}
 	if form.AllowGitHook != nil {
@@ -339,7 +336,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)

routers/web/admin/users.go

@@ -437,8 +437,7 @@ func EditUserPost(ctx *context.Context) {
 	}
 
 	// Check whether user is the last admin
-	if u.IsAdmin && !form.Admin &&
-		user_model.CountUsers(ctx, &user_model.CountUserFilter{IsAdmin: util.OptionalBoolTrue}) <= 1 {
+	if !form.Admin && user_model.IsLastAdminUser(ctx, u) {
 		ctx.RenderWithErr(ctx.Tr("auth.last_admin"), tplUserEdit, &form)
 		return
 	}
@@ -510,7 +509,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)
 		}

routers/web/user/setting/account.go

@@ -246,6 +246,13 @@ func DeleteAccount(ctx *context.Context) {
 		return
 	}
 
+	// admin should not delete themself
+	if ctx.ContextUser.ID == ctx.Doer.ID {
+		ctx.Flash.Error(ctx.Tr("admin.users.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)
 		}

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