Unify user update methods #28733

KN4CK3R · 2024-01-08T18:59:21Z

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.

…r-user-service

KN4CK3R · 2024-01-08T19:01:52Z

services/auth/source/ldap/source_sync.go

 					log.Error("SyncExternalUsers[%s]: Error updating user %s: %v", source.authSource.Name, usr.Name, err)
 				}
+
+				if err := user_service.ReplacePrimaryEmailAddress(ctx, usr, su.Mail); err != nil {


This fixes #28660. If we decide this is not a bug but a breaking change, I will change this to AddOrSetPrimaryEmailAddress and make a separate PR.

I think this isn't expected behavior, so this shouldn't be considered as breaking

KN4CK3R · 2024-01-08T19:04:39Z

services/user/email.go

+	"code.gitea.io/gitea/modules/util"
+)
+
+func AddOrSetPrimaryEmailAddress(ctx context.Context, u *user_model.User, emailStr string) error {


Currently you can't really edit the user emails as admin. We only can set the primary address. This method mimics this behaviour but I propose to remove the email field from the admin user forms in future in favor of extended email edit endpoints.

templates/swagger/v1_json.tmpl

modules/optional/option.go

modules/auth/password/pwn/pwn.go

modules/auth/password/pwn.go

routers/api/v1/org/org.go

routers/web/admin/users.go

routers/web/auth/auth.go

routers/web/auth/oauth.go

…r-user-service

modules/auth/password/pwn/pwn.go

modules/optional/option.go

routers/api/v1/admin/user.go

routers/web/auth/oauth.go

services/user/email.go

KN4CK3R · 2024-01-09T11:10:27Z

models/user/user.go

-	if len(passwd) == 0 {
-		u.Passwd = ""
-		u.Salt = ""
-		u.PasswdHashAlgo = ""
-		return nil
-	}


I could not find a place where this is used in the current code base so I did not replicate it. Even if we use something like that, we should add a new method with a more descriptive name.
The only related place is

gitea/services/user/user.go

Lines 133 to 147 in a80debc

// 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.

if err := user_model.UpdateUserCols(ctx, &user_model.User{

ID: u.ID,

IsActive: false,

IsRestricted: true,

IsAdmin: false,

ProhibitLogin: true,

Passwd: "",

Salt: "",

PasswdHashAlgo: "",

MaxRepoCreation: 0,

}, "is_active", "is_restricted", "is_admin", "prohibit_login", "max_repo_creation", "passwd", "salt", "passwd_hash_algo"); err != nil {

return fmt.Errorf("unable to disable user: %s[%d] prior to purge. UpdateUserCols: %w", u.Name, u.ID, err)

}

models/user/email_address.go

…r-user-service

lunny · 2024-01-23T08:45:44Z

models/user/user.go

 // ValidateUser check if user is valid to insert / update into database
-func ValidateUser(u *User, cols ...string) error {
+func ValidateUser(u *User, cols ...string) error { // TODO delete


Should this be deleted?

…r-user-service

delvh · 2024-02-02T22:46:04Z

Ping for reviewers

GiteaBot · 2024-02-03T17:53:39Z

@KN4CK3R please fix the merge conflicts. 🍵

…r-user-service

* giteaofficial/main: [skip ci] Updated licenses and gitignores Show whether a PR is WIP inside popups (go-gitea#28975) Unify password changing and invalidate auth tokens (go-gitea#27625) Unify user update methods (go-gitea#28733) Do not render empty comments (go-gitea#29039)

Fixes go-gitea#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.

Fix #29278. Caused by a small typo in #28733

KN4CK3R added 2 commits January 8, 2024 18:40

Unify user update methods.

ef477dd

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

386fc5a

…r-user-service

KN4CK3R added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 8, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 8, 2024

pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 8, 2024

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Jan 8, 2024

KN4CK3R commented Jan 8, 2024

View reviewed changes

delvh reviewed Jan 9, 2024

View reviewed changes

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

89f71f6

…r-user-service

KN4CK3R force-pushed the refactor-user-service branch from d28906b to 89f71f6 Compare January 9, 2024 08:08

Add suggestions.

2b97dbc

KN4CK3R commented Jan 9, 2024

View reviewed changes

KN4CK3R added 3 commits January 9, 2024 11:33

Fix return.

15b6e90

Fix test.

d8d87b3

Use db.DeleteByID.

84aa00e

delvh approved these changes Jan 11, 2024

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 11, 2024

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

b50590e

…r-user-service

KN4CK3R mentioned this pull request Jan 18, 2024

Make LoginName optional for admin edit user API #28834

Open

KN4CK3R added the type/bug label Jan 19, 2024

lunny reviewed Jan 23, 2024

View reviewed changes

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

f339316

…r-user-service

wxiaoguang approved these changes Feb 3, 2024

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 3, 2024

delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024

Merge branch 'main' into refactor-user-service

f04b039

KN4CK3R mentioned this pull request Feb 3, 2024

Unify password changing and invalidate auth tokens #27625

Merged

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

5792d59

…r-user-service

KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024

Merge branch 'main' into refactor-user-service

3550833

lunny enabled auto-merge (squash) February 4, 2024 13:12

lunny merged commit f8b471a into go-gitea:main Feb 4, 2024
25 checks passed

GiteaBot added this to the 1.22.0 milestone Feb 4, 2024

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 4, 2024

KN4CK3R deleted the refactor-user-service branch February 4, 2024 14:07

wolfogre mentioned this pull request Feb 21, 2024

Do not use ctx.Doer when reset password #29289

Merged

wxiaoguang pushed a commit that referenced this pull request Feb 21, 2024

Do not use ctx.Doer when reset password (#29289)

22b8de8

Fix #29278. Caused by a small typo in #28733

This was referenced Feb 23, 2024

Start to migrate from util.OptionalBool to optional.Option[bool] #29329

Merged

Introduce generic type values.NullableValue and reimplement OptionalBool #29341

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify user update methods #28733

Unify user update methods #28733

KN4CK3R commented Jan 8, 2024

KN4CK3R Jan 8, 2024

denyskon Jan 14, 2024

KN4CK3R Jan 8, 2024

KN4CK3R Jan 9, 2024

lunny Jan 23, 2024

delvh commented Feb 2, 2024

GiteaBot commented Feb 3, 2024

	// 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.
	if err := user_model.UpdateUserCols(ctx, &user_model.User{
	ID: u.ID,
	IsActive: false,
	IsRestricted: true,
	IsAdmin: false,
	ProhibitLogin: true,
	Passwd: "",
	Salt: "",
	PasswdHashAlgo: "",
	MaxRepoCreation: 0,
	}, "is_active", "is_restricted", "is_admin", "prohibit_login", "max_repo_creation", "passwd", "salt", "passwd_hash_algo"); err != nil {
	return fmt.Errorf("unable to disable user: %s[%d] prior to purge. UpdateUserCols: %w", u.Name, u.ID, err)
	}

Unify user update methods #28733

Unify user update methods #28733

Conversation

KN4CK3R commented Jan 8, 2024

KN4CK3R Jan 8, 2024

Choose a reason for hiding this comment

denyskon Jan 14, 2024

Choose a reason for hiding this comment

KN4CK3R Jan 8, 2024

Choose a reason for hiding this comment

KN4CK3R Jan 9, 2024

Choose a reason for hiding this comment

lunny Jan 23, 2024

Choose a reason for hiding this comment

delvh commented Feb 2, 2024

GiteaBot commented Feb 3, 2024