From f2c3e3ff50bc0a701d8b991dc6e3cdda6602eb58 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 15 Mar 2023 18:23:00 +0800 Subject: [PATCH 1/2] Fix login error when user has an unsupported visibility type --- models/user/user.go | 17 +++++++++-------- models/user/user_test.go | 11 ++++++----- modules/structs/user.go | 5 +++-- routers/api/v1/admin/user.go | 4 +++- routers/api/v1/user/settings.go | 8 +++++++- routers/web/admin/users.go | 3 ++- routers/web/org/setting.go | 2 +- routers/web/user/setting/profile.go | 5 +++-- services/auth/source/ldap/source_sync.go | 2 +- services/user/rename.go | 2 +- 10 files changed, 36 insertions(+), 23 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 82c2d3b6cdc10..e08315231d6c4 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -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 } @@ -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()) } @@ -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) if err != nil { return err } @@ -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 } @@ -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 @@ -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() diff --git a/models/user/user_test.go b/models/user/user_test.go index fc8f6b8d515ce..f22f80108624e 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -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) { @@ -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)) } diff --git a/modules/structs/user.go b/modules/structs/user.go index f68b92ac069e2..811582f07f2b8 100644 --- a/modules/structs/user.go +++ b/modules/structs/user.go @@ -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 diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 369d13943a329..18cc728eb9ade 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -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 { @@ -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) { diff --git a/routers/api/v1/user/settings.go b/routers/api/v1/user/settings.go index 53794c82f8385..7ec98b7723b12 100644 --- a/routers/api/v1/user/settings.go +++ b/routers/api/v1/user/settings.go @@ -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 } diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 1bb9d0480682e..5a86869e2d96a 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -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 @@ -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) diff --git a/routers/web/org/setting.go b/routers/web/org/setting.go index 654e9000fa1b5..a2ea36d912164 100644 --- a/routers/web/org/setting.go +++ b/routers/web/org/setting.go @@ -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 } diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index f500be7632336..fc7ae59ffb300 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -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") @@ -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 } diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 4571ff6540c3e..b74b4addfdb52 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -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) } diff --git a/services/user/rename.go b/services/user/rename.go index af195d7d76a26..b355416b0e19e 100644 --- a/services/user/rename.go +++ b/services/user/rename.go @@ -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 } From 07e2724f69d797bd9bc3385a8110b180ad4f711a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Mar 2023 15:39:04 +0800 Subject: [PATCH 2/2] Fix swagger --- templates/swagger/v1_json.tmpl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 0f7e60c598bf8..73a6a2a20fa86 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -20360,6 +20360,10 @@ "type": "string", "x-go-name": "Theme" }, + "visibility": { + "type": "string", + "x-go-name": "Visibility" + }, "website": { "type": "string", "x-go-name": "Website"