From 6a219f5faf7622f15f9f14e09bf716a4c437b2ba Mon Sep 17 00:00:00 2001 From: Alexander Shimchik Date: Thu, 29 Sep 2022 15:36:29 +0300 Subject: [PATCH 1/3] Check if email is used when updating user (#21289) Backport #21289 Fix #21075 When updating user data should check if email is used by other users --- models/user/user.go | 17 +++++++++++------ models/user/user_test.go | 16 ++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index a96232e386145..1d7772cf4169d 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -891,14 +891,19 @@ func UpdateUser(ctx context.Context, u *User, changePrimaryEmail bool, cols ...s if err != nil { return err } - if !has { - // 1. Update old primary email - if _, err = e.Where("uid=? AND is_primary=?", u.ID, true).Cols("is_primary").Update(&EmailAddress{ - IsPrimary: false, - }); err != nil { - return err + if has && emailAddress.UID != u.ID { + return ErrEmailAlreadyUsed{ + Email: u.Email, } + } + // 1. Update old primary email + if _, err = e.Where("uid=? AND is_primary=?", u.ID, true).Cols("is_primary").Update(&EmailAddress{ + IsPrimary: false, + }); err != nil { + return err + } + if !has { emailAddress.Email = u.Email emailAddress.UID = u.ID emailAddress.IsActivated = true diff --git a/models/user/user_test.go b/models/user/user_test.go index 4994ac53ab3df..29e0ab29687cd 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -299,10 +299,26 @@ func TestUpdateUser(t *testing.T) { user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) assert.True(t, user.KeepActivityPrivate) + newEmail := "new_" + user.Email + user.Email = newEmail + assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, true)) + 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)) } +func TestUpdateUserEmailAlreadyUsed(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + + user2.Email = user3.Email + err := user_model.UpdateUser(db.DefaultContext, user2, true) + assert.True(t, user_model.IsErrEmailAlreadyUsed(err)) +} + func TestNewUserRedirect(t *testing.T) { // redirect to a completely new name assert.NoError(t, unittest.PrepareTestDatabase()) From f0b38a946446bdafdf4ff2ca47cab674ed84660c Mon Sep 17 00:00:00 2001 From: Xinyu Zhou Date: Fri, 25 Nov 2022 22:41:15 +0800 Subject: [PATCH 2/3] check if email is activated before throw error in `func UpdateUser` Signed-off-by: Xinyu Zhou --- models/user/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user/user.go b/models/user/user.go index 1d7772cf4169d..0a422b3e0e343 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -891,7 +891,7 @@ func UpdateUser(ctx context.Context, u *User, changePrimaryEmail bool, cols ...s if err != nil { return err } - if has && emailAddress.UID != u.ID { + if has && emailAddress.IsActivated && emailAddress.UID != u.ID { return ErrEmailAlreadyUsed{ Email: u.Email, } From 5d53836f74b30cac0fabcd0d97c27ff7b6e23be5 Mon Sep 17 00:00:00 2001 From: Xinyu Zhou Date: Fri, 25 Nov 2022 23:12:43 +0800 Subject: [PATCH 3/3] fix test Signed-off-by: Xinyu Zhou --- models/user/user_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/user/user_test.go b/models/user/user_test.go index 29e0ab29687cd..d4dcd588f4f1f 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -302,7 +302,7 @@ func TestUpdateUser(t *testing.T) { newEmail := "new_" + user.Email user.Email = newEmail assert.NoError(t, user_model.UpdateUser(db.DefaultContext, user, true)) - user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) assert.Equal(t, newEmail, user.Email) user.Email = "no mail@mail.org" @@ -311,8 +311,8 @@ func TestUpdateUser(t *testing.T) { func TestUpdateUserEmailAlreadyUsed(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) + user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}).(*user_model.User) user2.Email = user3.Email err := user_model.UpdateUser(db.DefaultContext, user2, true)