Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify user update methods #28733

Merged
merged 12 commits into from
Feb 4, 2024
Merged

Unify user update methods #28733

merged 12 commits into from
Feb 4, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Jan 8, 2024

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.

@KN4CK3R KN4CK3R added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 8, 2024
@GiteaBot 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 pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 8, 2024
@github-actions 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
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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"code.gitea.io/gitea/modules/util"
)

func AddOrSetPrimaryEmailAddress(ctx context.Context, u *user_model.User, emailStr string) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
modules/optional/option.go Show resolved Hide resolved
modules/auth/password/pwn/pwn.go Show resolved Hide resolved
modules/auth/password/pwn.go Outdated Show resolved Hide resolved
modules/auth/password/pwn.go Outdated Show resolved Hide resolved
routers/api/v1/org/org.go Outdated Show resolved Hide resolved
routers/web/admin/users.go Show resolved Hide resolved
routers/web/auth/auth.go Show resolved Hide resolved
routers/web/auth/oauth.go Show resolved Hide resolved
routers/web/auth/oauth.go Show resolved Hide resolved
modules/auth/password/pwn/pwn.go Show resolved Hide resolved
modules/optional/option.go Show resolved Hide resolved
routers/api/v1/admin/user.go Show resolved Hide resolved
routers/web/auth/oauth.go Show resolved Hide resolved
services/user/email.go Show resolved Hide resolved
services/user/email.go Show resolved Hide resolved
Comment on lines -381 to -386
if len(passwd) == 0 {
u.Passwd = ""
u.Salt = ""
u.PasswdHashAlgo = ""
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
models/user/email_address.go Outdated Show resolved Hide resolved
@GiteaBot 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
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be deleted?

@delvh
Copy link
Member

delvh commented Feb 2, 2024

Ping for reviewers

@GiteaBot 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 delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024
@GiteaBot
Copy link
Contributor

GiteaBot commented Feb 3, 2024

@KN4CK3R please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024
@KN4CK3R KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024
@lunny lunny enabled auto-merge (squash) February 4, 2024 13:12
@lunny lunny merged commit f8b471a into go-gitea:main Feb 4, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Feb 4, 2024
@GiteaBot 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 KN4CK3R deleted the refactor-user-service branch February 4, 2024 14:07
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 5, 2024
* 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)
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
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.
wxiaoguang pushed a commit that referenced this pull request Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E-mail address change in LDAP creates separate address in gitea
6 participants