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.

Sign up for GitHub

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

Jump to bottom

Fix login error when user has an unsupported visibility type #23496

Closed
wants to merge 4 commits into from
Closed

Fix login error when user has an unsupported visibility type #23496

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2c3e3f
Fix login error when user has an unsupported visibility type
lunny Mar 15, 2023
42568b9
Merge branch 'main' into lunny/fix_visibility_types
lunny Mar 16, 2023
b30e590
Merge branch 'main' into lunny/fix_visibility_types
lunny Mar 25, 2023
07e2724
Fix swagger
lunny Mar 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions models/user/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ func CreateUser(u *User, overwriteDefault ...*CreateUserOverwriteOptions) (err e
}

// validate data
if err := validateUser(u); err != nil {
if err := validateUser(u, true); err != nil {
return err
}

Expand Down Expand Up @@ -804,8 +804,8 @@ func checkDupEmail(ctx context.Context, u *User) error {
}

// validateUser check if user is valid to insert / update into database
func validateUser(u *User) error {
if !setting.Service.AllowedUserVisibilityModesSlice.IsAllowedVisibility(u.Visibility) && !u.IsOrganization() {
func validateUser(u *User, checkVisibility bool) error {
if checkVisibility && !setting.Service.AllowedUserVisibilityModesSlice.IsAllowedVisibility(u.Visibility) && !u.IsOrganization() {
return fmt.Errorf("visibility Mode not allowed: %s", u.Visibility.String())
}

Expand All @@ -814,8 +814,8 @@ func validateUser(u *User) error {
}

// UpdateUser updates user's information.
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

That means the previous value which have stored in database should not be checked, only changed values need to be checked .

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

if err != nil {
return err
}
Expand Down Expand Up @@ -881,7 +881,8 @@ func UpdateUser(ctx context.Context, u *User, changePrimaryEmail bool, cols ...s

// UpdateUserCols update user according special columns
func UpdateUserCols(ctx context.Context, u *User, cols ...string) error {
if err := validateUser(u); err != nil {
checkVisibility := util.SliceContainsString(cols, "visibility", true)
if err := validateUser(u, checkVisibility); err != nil {
return err
}

Expand All @@ -890,7 +891,7 @@ func UpdateUserCols(ctx context.Context, u *User, cols ...string) error {
}

// UpdateUserSetting updates user's settings.
func UpdateUserSetting(u *User) (err error) {
func UpdateUserSetting(u *User, visibilityChanged bool) (err error) {
ctx, committer, err := db.TxContext(db.DefaultContext)
if err != nil {
return err
Expand All @@ -902,7 +903,7 @@ func UpdateUserSetting(u *User) (err error) {
return err
}
}
if err = UpdateUser(ctx, u, false); err != nil {
if err = UpdateUser(ctx, u, false, visibilityChanged); err != nil {
return err
}
return committer.Commit()
Expand Down
11 changes: 6 additions & 5 deletions models/user/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,25 +346,26 @@ func TestUpdateUser(t *testing.T) {
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})

user.KeepActivityPrivate = true
assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, false))
assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, false, false))
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
assert.True(t, user.KeepActivityPrivate)

setting.Service.AllowedUserVisibilityModesSlice = []bool{true, false, false}
user.KeepActivityPrivate = false
visibilityChanged := user.Visibility != structs.VisibleTypePrivate
user.Visibility = structs.VisibleTypePrivate
assert.Error(t, user_model.UpdateUser(db.DefaultContext, user, false))
assert.Error(t, user_model.UpdateUser(db.DefaultContext, user, false, visibilityChanged))
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
assert.True(t, user.KeepActivityPrivate)

newEmail := "new_" + user.Email
user.Email = newEmail
assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, true))
assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, true, false))
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
assert.Equal(t, newEmail, user.Email)

user.Email = "no mail@mail.org"
assert.Error(t, user_model.UpdateUser(db.DefaultContext, user, true))
assert.Error(t, user_model.UpdateUser(db.DefaultContext, user, true, false))
}

func TestUpdateUserEmailAlreadyUsed(t *testing.T) {
Expand All @@ -373,7 +374,7 @@ func TestUpdateUserEmailAlreadyUsed(t *testing.T) {
user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})

user2.Email = user3.Email
err := user_model.UpdateUser(db.DefaultContext, user2, true)
err := user_model.UpdateUser(db.DefaultContext, user2, true, false)
assert.True(t, user_model.IsErrEmailAlreadyUsed(err))
}

Expand Down
5 changes: 3 additions & 2 deletions modules/structs/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ type UserSettingsOptions struct {
Theme *string `json:"theme"`
DiffViewStyle *string `json:"diff_view_style"`
// Privacy
HideEmail *bool `json:"hide_email"`
HideActivity *bool `json:"hide_activity"`
HideEmail *bool `json:"hide_email"`
HideActivity *bool `json:"hide_activity"`
Visibility string `json:"visibility" binding:"In(,public,limited,private)"`
}

// RenameUserOption options when renaming a user
Expand Down
4 changes: 3 additions & 1 deletion routers/api/v1/admin/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,9 @@ func EditUser(ctx *context.APIContext) {
if form.Active != nil {
ctx.ContextUser.IsActive = *form.Active
}
visibilityChanged := false
if len(form.Visibility) != 0 {
visibilityChanged = ctx.ContextUser.Visibility != api.VisibilityModes[form.Visibility]
ctx.ContextUser.Visibility = api.VisibilityModes[form.Visibility]
}
if form.Admin != nil {
Expand All @@ -277,7 +279,7 @@ func EditUser(ctx *context.APIContext) {
ctx.ContextUser.IsRestricted = *form.Restricted
}

if err := user_model.UpdateUser(ctx, ctx.ContextUser, emailChanged); err != nil {
if err := user_model.UpdateUser(ctx, ctx.ContextUser, emailChanged, visibilityChanged); err != nil {
if user_model.IsErrEmailAlreadyUsed(err) ||
user_model.IsErrEmailCharIsNotSupported(err) ||
user_model.IsErrEmailInvalid(err) {
Expand Down
8 changes: 7 additions & 1 deletion routers/api/v1/user/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@ func UpdateUserSettings(ctx *context.APIContext) {
ctx.Doer.KeepActivityPrivate = *form.HideActivity
}

if err := user_model.UpdateUser(ctx, ctx.Doer, false); err != nil {
visibilityChanged := false
if len(form.Visibility) != 0 {
visibilityChanged = ctx.ContextUser.Visibility != api.VisibilityModes[form.Visibility]
ctx.ContextUser.Visibility = api.VisibilityModes[form.Visibility]
}

if err := user_model.UpdateUser(ctx, ctx.Doer, false, visibilityChanged); err != nil {
ctx.InternalServerError(err)
return
}
Expand Down
3 changes: 2 additions & 1 deletion routers/web/admin/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ func EditUserPost(ctx *context.Context) {
u.AllowImportLocal = form.AllowImportLocal
u.AllowCreateOrganization = form.AllowCreateOrganization

visibilityChanged := u.Visibility != form.Visibility
u.Visibility = form.Visibility

// skip self Prohibit Login
Expand All @@ -392,7 +393,7 @@ func EditUserPost(ctx *context.Context) {
u.ProhibitLogin = form.ProhibitLogin
}

if err := user_model.UpdateUser(ctx, u, emailChanged); err != nil {
if err := user_model.UpdateUser(ctx, u, emailChanged, visibilityChanged); err != nil {
if user_model.IsErrEmailAlreadyUsed(err) {
ctx.Data["Err_Email"] = true
ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserEdit, &form)
Expand Down
2 changes: 1 addition & 1 deletion routers/web/org/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func SettingsPost(ctx *context.Context) {
visibilityChanged := form.Visibility != org.Visibility
org.Visibility = form.Visibility

if err := user_model.UpdateUser(ctx, org.AsUser(), false); err != nil {
if err := user_model.UpdateUser(ctx, org.AsUser(), false, visibilityChanged); err != nil {
ctx.ServerError("UpdateUser", err)
return
}
Expand Down
5 changes: 3 additions & 2 deletions routers/web/user/setting/profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ func ProfilePost(ctx *context.Context) {
ctx.Doer.Location = form.Location
ctx.Doer.Description = form.Description
ctx.Doer.KeepActivityPrivate = form.KeepActivityPrivate
visibilityChanged := ctx.Doer.Visibility != form.Visibility
ctx.Doer.Visibility = form.Visibility
if err := user_model.UpdateUserSetting(ctx.Doer); err != nil {
if err := user_model.UpdateUserSetting(ctx.Doer, visibilityChanged); err != nil {
if _, ok := err.(user_model.ErrEmailAlreadyUsed); ok {
ctx.Flash.Error(ctx.Tr("form.email_been_used"))
ctx.Redirect(setting.AppSubURL + "/user/settings")
Expand Down Expand Up @@ -396,7 +397,7 @@ func UpdateUserLang(ctx *context.Context) {
ctx.Doer.Language = form.Language
}

if err := user_model.UpdateUserSetting(ctx.Doer); err != nil {
if err := user_model.UpdateUserSetting(ctx.Doer, false); err != nil {
ctx.ServerError("UpdateUserSetting", err)
return
}
Expand Down
2 changes: 1 addition & 1 deletion services/auth/source/ldap/source_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
}
usr.IsActive = true

err = user_model.UpdateUser(ctx, usr, emailChanged, "full_name", "email", "is_admin", "is_restricted", "is_active")
err = user_model.UpdateUser(ctx, usr, emailChanged, false, "full_name", "email", "is_admin", "is_restricted", "is_active")
if err != nil {
log.Error("SyncExternalUsers[%s]: Error updating user %s: %v", source.authSource.Name, usr.Name, err)
}
Expand Down
2 changes: 1 addition & 1 deletion services/user/rename.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func renameUser(ctx context.Context, u *user_model.User, newUserName string) err

u.Name = newUserName
u.LowerName = strings.ToLower(newUserName)
if err := user_model.UpdateUser(ctx, u, false); err != nil {
if err := user_model.UpdateUser(ctx, u, false, false); err != nil {
return err
}

Expand Down