Unify user update methods (#28733)

Fixes #28660
Fixes an admin api bug related to `user.LoginSource`
Fixed `/user/emails` response not identical to GitHub api

This PR unifies the user update methods. The goal is to keep the logic
only at one place (having audit logs in mind). For example, do the
password checks only in one method not everywhere a password is updated.

After that PR is merged, the user creation should be next.

routers/api/v1/admin/user.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"strings"
 
 	"code.gitea.io/gitea/models"
 	asymkey_model "code.gitea.io/gitea/models/asymkey"
@@ -18,6 +17,7 @@ import (
 	"code.gitea.io/gitea/modules/auth/password"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
@@ -107,9 +107,8 @@ func CreateUser(ctx *context.APIContext) {
 			return
 		}
 
-		pwned, err := password.IsPwned(ctx, form.Password)
-		if pwned {
-			if err != nil {
+		if err := password.IsPwned(ctx, form.Password); err != nil {
+			if password.IsErrIsPwnedRequest(err) {
 				log.Error(err.Error())
 			}
 			ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned"))
@@ -192,115 +191,65 @@ func EditUser(ctx *context.APIContext) {
 
 	form := web.GetForm(ctx).(*api.EditUserOption)
 
-	parseAuthSource(ctx, ctx.ContextUser, form.SourceID, form.LoginName)
-	if ctx.Written() {
+	authOpts := &user_service.UpdateAuthOptions{
+		LoginSource:        optional.FromNonDefault(form.SourceID),
+		LoginName:          optional.Some(form.LoginName),
+		Password:           optional.FromNonDefault(form.Password),
+		MustChangePassword: optional.FromPtr(form.MustChangePassword),
+		ProhibitLogin:      optional.FromPtr(form.ProhibitLogin),
+	}
+	if err := user_service.UpdateAuth(ctx, ctx.ContextUser, authOpts); err != nil {
+		switch {
+		case errors.Is(err, password.ErrMinLength):
+			ctx.Error(http.StatusBadRequest, "PasswordTooShort", fmt.Errorf("password must be at least %d characters", setting.MinPasswordLength))
+		case errors.Is(err, password.ErrComplexity):
+			ctx.Error(http.StatusBadRequest, "PasswordComplexity", err)
+		case errors.Is(err, password.ErrIsPwned), password.IsErrIsPwnedRequest(err):
+			ctx.Error(http.StatusBadRequest, "PasswordIsPwned", err)
+		default:
+			ctx.Error(http.StatusInternalServerError, "UpdateAuth", err)
+		}
 		return
 	}
 
-	if len(form.Password) != 0 {
-		if len(form.Password) < setting.MinPasswordLength {
-			ctx.Error(http.StatusBadRequest, "PasswordTooShort", fmt.Errorf("password must be at least %d characters", setting.MinPasswordLength))
-			return
-		}
-		if !password.IsComplexEnough(form.Password) {
-			err := errors.New("PasswordComplexity")
-			ctx.Error(http.StatusBadRequest, "PasswordComplexity", err)
-			return
-		}
-		pwned, err := password.IsPwned(ctx, form.Password)
-		if pwned {
-			if err != nil {
-				log.Error(err.Error())
-			}
-			ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned"))
-			return
-		}
-		if ctx.ContextUser.Salt, err = user_model.GetUserSalt(); err != nil {
-			ctx.Error(http.StatusInternalServerError, "UpdateUser", err)
-			return
-		}
-		if err = ctx.ContextUser.SetPassword(form.Password); err != nil {
-			ctx.InternalServerError(err)
-			return
-		}
-	}
-
-	if form.MustChangePassword != nil {
-		ctx.ContextUser.MustChangePassword = *form.MustChangePassword
-	}
-
-	ctx.ContextUser.LoginName = form.LoginName
-
-	if form.FullName != nil {
-		ctx.ContextUser.FullName = *form.FullName
-	}
-	var emailChanged bool
 	if form.Email != nil {
-		email := strings.TrimSpace(*form.Email)
-		if len(email) == 0 {
-			ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("email is not allowed to be empty string"))
+		if err := user_service.AddOrSetPrimaryEmailAddress(ctx, ctx.ContextUser, *form.Email); err != nil {
+			switch {
+			case user_model.IsErrEmailCharIsNotSupported(err), user_model.IsErrEmailInvalid(err):
+				ctx.Error(http.StatusBadRequest, "EmailInvalid", err)
+			case user_model.IsErrEmailAlreadyUsed(err):
+				ctx.Error(http.StatusBadRequest, "EmailUsed", err)
+			default:
+				ctx.Error(http.StatusInternalServerError, "AddOrSetPrimaryEmailAddress", err)
+			}
 			return
 		}
-
-		if err := user_model.ValidateEmail(email); err != nil {
-			ctx.InternalServerError(err)
-			return
-		}
-
-		emailChanged = !strings.EqualFold(ctx.ContextUser.Email, email)
-		ctx.ContextUser.Email = email
-	}
-	if form.Website != nil {
-		ctx.ContextUser.Website = *form.Website
-	}
-	if form.Location != nil {
-		ctx.ContextUser.Location = *form.Location
-	}
-	if form.Description != nil {
-		ctx.ContextUser.Description = *form.Description
-	}
-	if form.Active != nil {
-		ctx.ContextUser.IsActive = *form.Active
-	}
-	if len(form.Visibility) != 0 {
-		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 {
-		ctx.ContextUser.AllowGitHook = *form.AllowGitHook
-	}
-	if form.AllowImportLocal != nil {
-		ctx.ContextUser.AllowImportLocal = *form.AllowImportLocal
-	}
-	if form.MaxRepoCreation != nil {
-		ctx.ContextUser.MaxRepoCreation = *form.MaxRepoCreation
-	}
-	if form.AllowCreateOrganization != nil {
-		ctx.ContextUser.AllowCreateOrganization = *form.AllowCreateOrganization
-	}
-	if form.ProhibitLogin != nil {
-		ctx.ContextUser.ProhibitLogin = *form.ProhibitLogin
-	}
-	if form.Restricted != nil {
-		ctx.ContextUser.IsRestricted = *form.Restricted
 	}
 
-	if err := user_model.UpdateUser(ctx, ctx.ContextUser, emailChanged); err != nil {
-		if user_model.IsErrEmailAlreadyUsed(err) ||
-			user_model.IsErrEmailCharIsNotSupported(err) ||
-			user_model.IsErrEmailInvalid(err) {
-			ctx.Error(http.StatusUnprocessableEntity, "", err)
+	opts := &user_service.UpdateOptions{
+		FullName:                optional.FromPtr(form.FullName),
+		Website:                 optional.FromPtr(form.Website),
+		Location:                optional.FromPtr(form.Location),
+		Description:             optional.FromPtr(form.Description),
+		IsActive:                optional.FromPtr(form.Active),
+		IsAdmin:                 optional.FromPtr(form.Admin),
+		Visibility:              optional.FromNonDefault(api.VisibilityModes[form.Visibility]),
+		AllowGitHook:            optional.FromPtr(form.AllowGitHook),
+		AllowImportLocal:        optional.FromPtr(form.AllowImportLocal),
+		MaxRepoCreation:         optional.FromPtr(form.MaxRepoCreation),
+		AllowCreateOrganization: optional.FromPtr(form.AllowCreateOrganization),
+		IsRestricted:            optional.FromPtr(form.Restricted),
+	}
+
+	if err := user_service.UpdateUser(ctx, ctx.ContextUser, opts); err != nil {
+		if models.IsErrDeleteLastAdminUser(err) {
+			ctx.Error(http.StatusBadRequest, "LastAdmin", err)
 		} else {
 			ctx.Error(http.StatusInternalServerError, "UpdateUser", err)
 		}
 		return
 	}
+
 	log.Trace("Account profile updated by admin (%s): %s", ctx.Doer.Name, ctx.ContextUser.Name)
 
 	ctx.JSON(http.StatusOK, convert.ToUser(ctx, ctx.ContextUser, ctx.Doer))
@@ -527,9 +476,6 @@ func RenameUser(ctx *context.APIContext) {
 	// Check if user name has been changed
 	if err := user_service.RenameUser(ctx, ctx.ContextUser, newName); err != nil {
 		switch {
-		case user_model.IsErrUsernameNotChanged(err):
-			// Noop as username is not changed
-			ctx.Status(http.StatusNoContent)
 		case user_model.IsErrUserAlreadyExist(err):
 			ctx.Error(http.StatusUnprocessableEntity, "", ctx.Tr("form.username_been_taken"))
 		case db.IsErrNameReserved(err):
@@ -545,5 +491,5 @@ func RenameUser(ctx *context.APIContext) {
 	}
 
 	log.Trace("User name changed: %s -> %s", oldName, newName)
-	ctx.Status(http.StatusOK)
+	ctx.Status(http.StatusNoContent)
 }

routers/api/v1/org/org.go

@@ -13,12 +13,14 @@ import (
 	"code.gitea.io/gitea/models/perm"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/optional"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/user"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	"code.gitea.io/gitea/services/convert"
 	"code.gitea.io/gitea/services/org"
+	user_service "code.gitea.io/gitea/services/user"
 )
 
 func listUserOrgs(ctx *context.APIContext, u *user_model.User) {
@@ -337,28 +339,30 @@ func Edit(ctx *context.APIContext) {
 	//     "$ref": "#/responses/Organization"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+
 	form := web.GetForm(ctx).(*api.EditOrgOption)
-	org := ctx.Org.Organization
-	org.FullName = form.FullName
-	org.Email = form.Email
-	org.Description = form.Description
-	org.Website = form.Website
-	org.Location = form.Location
-	if form.Visibility != "" {
-		org.Visibility = api.VisibilityModes[form.Visibility]
+
+	if form.Email != "" {
+		if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.Org.Organization.AsUser(), form.Email); err != nil {
+			ctx.Error(http.StatusInternalServerError, "ReplacePrimaryEmailAddress", err)
+			return
+		}
 	}
-	if form.RepoAdminChangeTeamAccess != nil {
-		org.RepoAdminChangeTeamAccess = *form.RepoAdminChangeTeamAccess
+
+	opts := &user_service.UpdateOptions{
+		FullName:                  optional.Some(form.FullName),
+		Description:               optional.Some(form.Description),
+		Website:                   optional.Some(form.Website),
+		Location:                  optional.Some(form.Location),
+		Visibility:                optional.FromNonDefault(api.VisibilityModes[form.Visibility]),
+		RepoAdminChangeTeamAccess: optional.FromPtr(form.RepoAdminChangeTeamAccess),
 	}
-	if err := user_model.UpdateUserCols(ctx, org.AsUser(),
-		"full_name", "description", "website", "location",
-		"visibility", "repo_admin_change_team_access",
-	); err != nil {
-		ctx.Error(http.StatusInternalServerError, "EditOrganization", err)
+	if err := user_service.UpdateUser(ctx, ctx.Org.Organization.AsUser(), opts); err != nil {
+		ctx.Error(http.StatusInternalServerError, "UpdateUser", err)
 		return
 	}
 
-	ctx.JSON(http.StatusOK, convert.ToOrganization(ctx, org))
+	ctx.JSON(http.StatusOK, convert.ToOrganization(ctx, ctx.Org.Organization))
 }
 
 // Delete an organization

routers/api/v1/user/email.go

@@ -9,10 +9,10 @@ import (
 
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/context"
-	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/services/convert"
+	user_service "code.gitea.io/gitea/services/user"
 )
 
 // ListEmails list all of the authenticated user's email addresses
@@ -56,22 +56,14 @@ func AddEmail(ctx *context.APIContext) {
 	//     "$ref": "#/responses/EmailList"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
+
 	form := web.GetForm(ctx).(*api.CreateEmailOption)
 	if len(form.Emails) == 0 {
 		ctx.Error(http.StatusUnprocessableEntity, "", "Email list empty")
 		return
 	}
 
-	emails := make([]*user_model.EmailAddress, len(form.Emails))
-	for i := range form.Emails {
-		emails[i] = &user_model.EmailAddress{
-			UID:         ctx.Doer.ID,
-			Email:       form.Emails[i],
-			IsActivated: !setting.Service.RegisterEmailConfirm,
-		}
-	}
-
-	if err := user_model.AddEmailAddresses(ctx, emails); err != nil {
+	if err := user_service.AddEmailAddresses(ctx, ctx.Doer, form.Emails); err != nil {
 		if user_model.IsErrEmailAlreadyUsed(err) {
 			ctx.Error(http.StatusUnprocessableEntity, "", "Email address has been used: "+err.(user_model.ErrEmailAlreadyUsed).Email)
 		} else if user_model.IsErrEmailCharIsNotSupported(err) || user_model.IsErrEmailInvalid(err) {
@@ -91,11 +83,17 @@ func AddEmail(ctx *context.APIContext) {
 		return
 	}
 
-	apiEmails := make([]*api.Email, len(emails))
-	for i := range emails {
-		apiEmails[i] = convert.ToEmail(emails[i])
+	emails, err := user_model.GetEmailAddresses(ctx, ctx.Doer.ID)
+	if err != nil {
+		ctx.Error(http.StatusInternalServerError, "GetEmailAddresses", err)
+		return
 	}
-	ctx.JSON(http.StatusCreated, &apiEmails)
+
+	apiEmails := make([]*api.Email, 0, len(emails))
+	for _, email := range emails {
+		apiEmails = append(apiEmails, convert.ToEmail(email))
+	}
+	ctx.JSON(http.StatusCreated, apiEmails)
 }
 
 // DeleteEmail delete email
@@ -115,26 +113,19 @@ func DeleteEmail(ctx *context.APIContext) {
 	//     "$ref": "#/responses/empty"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+
 	form := web.GetForm(ctx).(*api.DeleteEmailOption)
 	if len(form.Emails) == 0 {
 		ctx.Status(http.StatusNoContent)
 		return
 	}
 
-	emails := make([]*user_model.EmailAddress, len(form.Emails))
-	for i := range form.Emails {
-		emails[i] = &user_model.EmailAddress{
-			Email: form.Emails[i],
-			UID:   ctx.Doer.ID,
-		}
-	}
-
-	if err := user_model.DeleteEmailAddresses(ctx, emails); err != nil {
+	if err := user_service.DeleteEmailAddresses(ctx, ctx.Doer, form.Emails); err != nil {
 		if user_model.IsErrEmailAddressNotExist(err) {
 			ctx.Error(http.StatusNotFound, "DeleteEmailAddresses", err)
-			return
+		} else {
+			ctx.Error(http.StatusInternalServerError, "DeleteEmailAddresses", err)
 		}
-		ctx.Error(http.StatusInternalServerError, "DeleteEmailAddresses", err)
 		return
 	}
 	ctx.Status(http.StatusNoContent)

routers/api/v1/user/settings.go

@@ -6,11 +6,12 @@ package user
 import (
 	"net/http"
 
-	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/optional"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/services/convert"
+	user_service "code.gitea.io/gitea/services/user"
 )
 
 // GetUserSettings returns user settings
@@ -44,36 +45,18 @@ func UpdateUserSettings(ctx *context.APIContext) {
 
 	form := web.GetForm(ctx).(*api.UserSettingsOptions)
 
-	if form.FullName != nil {
-		ctx.Doer.FullName = *form.FullName
+	opts := &user_service.UpdateOptions{
+		FullName:            optional.FromPtr(form.FullName),
+		Description:         optional.FromPtr(form.Description),
+		Website:             optional.FromPtr(form.Website),
+		Location:            optional.FromPtr(form.Location),
+		Language:            optional.FromPtr(form.Language),
+		Theme:               optional.FromPtr(form.Theme),
+		DiffViewStyle:       optional.FromPtr(form.DiffViewStyle),
+		KeepEmailPrivate:    optional.FromPtr(form.HideEmail),
+		KeepActivityPrivate: optional.FromPtr(form.HideActivity),
 	}
-	if form.Description != nil {
-		ctx.Doer.Description = *form.Description
-	}
-	if form.Website != nil {
-		ctx.Doer.Website = *form.Website
-	}
-	if form.Location != nil {
-		ctx.Doer.Location = *form.Location
-	}
-	if form.Language != nil {
-		ctx.Doer.Language = *form.Language
-	}
-	if form.Theme != nil {
-		ctx.Doer.Theme = *form.Theme
-	}
-	if form.DiffViewStyle != nil {
-		ctx.Doer.DiffViewStyle = *form.DiffViewStyle
-	}
-
-	if form.HideEmail != nil {
-		ctx.Doer.KeepEmailPrivate = *form.HideEmail
-	}
-	if form.HideActivity != nil {
-		ctx.Doer.KeepActivityPrivate = *form.HideActivity
-	}
-
-	if err := user_model.UpdateUser(ctx, ctx.Doer, false); err != nil {
+	if err := user_service.UpdateUser(ctx, ctx.Doer, opts); err != nil {
 		ctx.InternalServerError(err)
 		return
 	}