Fix login error when user has an unsupported visibility type #23496
Conversation
lunny
added
type/bug
outdated/backport/v1.18
GiteaBot
added
the
lgtm/need 1
|
Need |
| func UpdateUser(ctx context.Context, u *User, changePrimaryEmail bool, cols ...string) error { | ||
| err := validateUser(u) | ||
| func UpdateUser(ctx context.Context, u *User, changePrimaryEmail, visibilityChanged bool, cols ...string) error { | ||
| err := validateUser(u, visibilityChanged) |
There was a problem hiding this comment.
It confuses me, why visibilityChanged means checkVisibility? Even if it is correct, it's hard to understand.
As a quick patch, this PR might work.
But I think it reduces maintainability, I can't imagine what does UpdateUser(ctx, u, false, false) or UpdateUser(ctx, u, false, true) do without reading all related code.
There was a problem hiding this comment.
How about add an error handling after calling validateUser instead of add checkVisibility to validateUser?
if err := validateUser(u); err != nil {
if !(ignoreVisibilityNotAllowed && IsVisibilityNotAllowed(err)) {
return err
}
}There was a problem hiding this comment.
It confuses me, why
visibilityChangedmeanscheckVisibility? Even if it is correct, it's hard to understand.As a quick patch, this PR might work.
But I think it reduces maintainability, I can't imagine what does
UpdateUser(ctx, u, false, false)orUpdateUser(ctx, u, false, true)do without reading all related code.
That means the previous value which have stored in database should not be checked, only changed values need to be checked .
There was a problem hiding this comment.
I can understand after I spend much time on reading code, while I do not think it is a good design.
Now we have (..., true, false), do we want to have (..., true, false, true) in the future?
lunny
removed
the
outdated/backport/v1.18
|
replaced by #24867 |
…itea#24903) Backport go-gitea#24867 by @lunny Fix go-gitea#23211 Replace go-gitea#23496 --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> (cherry picked from commit 275abd6)
Validated user methods should not always check user's visibility type because users maybe have a visibility type which not allowed in a later configuration change.
Fix #23211