From b9d611e917d9bd10e0d8be8fc61e057d5936993c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 8 Jun 2021 11:52:51 +0800 Subject: [PATCH] Always store primary email address into email_address table and also the state (#15956) * Always store primary email address into email_address table and also the state * Add lower_email to not convert email to lower as what's added * Fix fixture * Fix tests * Use BeforeInsert to save lower email * Fix v180 migration * fix tests * Fix test * Remove wrong submited codes * Fix test * Fix test * Fix test * Add test for v181 migration * remove change user's email to lower * Revert change on user's email column * Fix lower email * Fix test * Fix test --- integrations/api_gpg_keys_test.go | 76 +++++---- integrations/api_user_email_test.go | 10 +- models/error.go | 15 ++ models/fixtures/email_address.yml | 254 +++++++++++++++++++++++++++- models/gpg_key.go | 2 +- models/migrations/migrations.go | 2 + models/migrations/v181.go | 92 ++++++++++ models/migrations/v181_test.go | 54 ++++++ models/user.go | 23 ++- models/user_mail.go | 250 ++++++++++----------------- models/user_mail_test.go | 51 ++++-- 11 files changed, 593 insertions(+), 236 deletions(-) create mode 100644 models/migrations/v181.go create mode 100644 models/migrations/v181_test.go diff --git a/integrations/api_gpg_keys_test.go b/integrations/api_gpg_keys_test.go index 76a4a1afa..b4f19031a 100644 --- a/integrations/api_gpg_keys_test.go +++ b/integrations/api_gpg_keys_test.go @@ -36,7 +36,6 @@ func TestGPGKeys(t *testing.T) { } for _, tc := range tt { - //Basic test on result code t.Run(tc.name, func(t *testing.T) { t.Run("ViewOwnGPGKeys", func(t *testing.T) { @@ -87,9 +86,9 @@ func TestGPGKeys(t *testing.T) { assert.Empty(t, subKey.Emails) primaryKey2 := keys[1] //Primary key 2 - assert.EqualValues(t, "FABF39739FE1E927", primaryKey2.KeyID) + assert.EqualValues(t, "3CEF46EF40BEFC3E", primaryKey2.KeyID) assert.Len(t, primaryKey2.Emails, 1) - assert.EqualValues(t, "user21@example.com", primaryKey2.Emails[0].Email) + assert.EqualValues(t, "user2-2@example.com", primaryKey2.Emails[0].Email) assert.False(t, primaryKey2.Emails[0].Verified) var key api.GPGKey @@ -110,9 +109,9 @@ func TestGPGKeys(t *testing.T) { req = NewRequest(t, "GET", "/api/v1/user/gpg_keys/"+strconv.FormatInt(primaryKey2.ID, 10)+"?token="+token) //Primary key 2 resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &key) - assert.EqualValues(t, "FABF39739FE1E927", key.KeyID) + assert.EqualValues(t, "3CEF46EF40BEFC3E", key.KeyID) assert.Len(t, key.Emails, 1) - assert.EqualValues(t, "user21@example.com", key.Emails[0].Email) + assert.EqualValues(t, "user2-2@example.com", key.Emails[0].Email) assert.False(t, key.Emails[0].Verified) }) @@ -231,35 +230,46 @@ uy6MA3VSB99SK9ducGmE1Jv8mcziREroz2TEGr0zPs6h } func testCreateValidSecondaryEmailGPGKey(t *testing.T, makeRequest makeRequestFunc, token string, expected int) { - //User2 //secondary and not activated + //User2 //secondary and not activated testCreateGPGKey(t, makeRequest, token, expected, `-----BEGIN PGP PUBLIC KEY BLOCK----- -mQENBFmGWN4BCAC18V4tVGO65VLCV7p14FuXJlUtZ5CuYMvgEkcOqrvRaBSW9ao4 -PGESOhJpfWpnW3QgJniYndLzPpsmdHEclEER6aZjiNgReWPOjHD5tykWocZAJqXD -eY1ym59gvVMLcfbV2yQsyR2hbJlc+dJsl16tigSEe3nwxZSw2IsW92pgEzT9JNUr -Q+mC8dw4dqY0tYmFazYUGNxufUc/twgQT/Or1aNs0az5Q6Jft4rrTRsh/S7We0VB -COKGkdcQyYgAls7HJBuPjQRi6DM9VhgBSHLAgSLyaUcZvhZBJr8Qe/q4PP3/kYDJ -wm4RMnjOLz2pFZPgtRqgcAwpmFtLrACbEB3JABEBAAG0GlVzZXIyIDx1c2VyMjFA -ZXhhbXBsZS5jb20+iQFUBBMBCAA+FiEEPOLHOjPSO42DWM57+r85c5/h6ScFAlmG -WN4CGwMFCQPCZwAFCwkIBwIGFQgJCgsCBBYCAwECHgECF4AACgkQ+r85c5/h6Sfx -Lgf/dq64NBV8+X9an3seaLxePRviva48e4K67/wV/JxtXNO5Z/DhMGz5kHXCsG9D -CXuWYO8ehlTjEnMZ6qqdDnY+H6bQsb2OS5oPn4RwpPXslAjEKtojPAr0dDsMS2DB -dUuIm1AoOnewOVO0OFRf1EqX1bivxnN0FVMcO0m8AczfnKDaGb0y/qg/Y9JAsKqp -j5pZNMWUkntRtGySeJ4CVJMmkVKJAHsa1Qj6MKdFeid4h4y94cBJ4ZdyBxNdpQOx -ydf0doicovfeqGNO4oWzsGP4RBK2CqGPCUT+EFl20jPvMkKwOjxgqc8p0z3b2UT9 -+9bnmCGHgF/fW1HJ3iKmfFPqnLkBDQRZhljeAQgA5AirU/NJGgm19ZJYFOiHftjS -azbrPxGeD3cSqmvDPIMc1DNZGfQV5D4EVumnVbQBtL6xHFoGKz9KisUMbe4a/X2J -S8JmIphQWG0vMJX1DaZIzr2gT71MnPD7JMGsSUCh5dIKpTNTZX4w+oGPGOu0/UlL -x0448AryKwp30J2p6D4GeI0nb03n35S2lTOpnHDn1wj7Jl/8LS2fdFOdNaNHXSZe -twdSwJKhyBEiScgeHBDyKqo8zWkYoSb9eA2HiYlbVaiNtp24KP1mIEpiUdrRjWno -zauYSZGHZlOFMgF4dKWuetPiuH9m7UYZGKyMLfQ9vYFb+xcPh2bLCQHJ1OEmMQAR -AQABiQE8BBgBCAAmFiEEPOLHOjPSO42DWM57+r85c5/h6ScFAlmGWN4CGwwFCQPC -ZwAACgkQ+r85c5/h6Sfjfwf+O4WEjRdvPJLxNy7mfAGoAqDMHIwyH/tVzYgyVhnG -h/+cfRxJbGc3rpjYdr8dmvghzjEAout8uibPWaIqs63RCAPGPqgWLfxNO5c8+y8V -LZMVOTV26l2olkkdBWAuhLqKTNh6TiQva03yhOgHWj4XDvFfxICWPFXVd6t5ELpD -iApGu1OAj8JfhmzbG03Yzx+Ku7bWDxMonx3V/IDEu5LS5zrboHYDKCA53bXXghoi -Aceqql+PKrDwEjoY4bptwMHLmcjGjdCQ//Qx1neho7nZcS7xjTucY8gQuulwCyXF -y6wM+wMz8dunIG9gw4+Re6c4Rz9tX1kzxLrU7Pl21tMqfg== -=0N/9 +mQGNBGC2K2cBDAC1+Xgk+8UfhASVgRngQi4rnQ8k0t+bWsBz4Czd26+cxVDRwlTT +8PALdrbrY/e9iXjcVcZ8Npo4UYe7/LfnL57dc7tgbenRGYYrWyVoNNv58BVw4xCY +RmgvdHWIIPGuz3aME0smHxbJ2KewYTqjTPuVKF/wrHTwCpVWdjYKC5KDo3yx0mro +xf9vOJOnkWNMiEw7TiZfkrbUqxyA53BVsSNKRX5C3b4FJcVT7eiAq7sDAaFxjEHy +ahZslmvg7XZxWzSVzxDNesR7f4xuop8HBjzaluJoVuwiyWculTvz1b6hyHVQr+ad +h8JGjj1tySI65OTFsTuptsfHXjtjl/NR4P6BXkf+FVwweaTQaEzpHkv0m9b9pY43 +CY/8XtS4uNPermiLG/Z0BB1eOCdoOQVHpjOa55IXQWhxXB6NZVyowiUbrR7jLDQy +5JP7D1HmErTR8JRm3VDqGbSaCgugRgFX+lb/fpgFp9k02OeK+JQudolZOt1mVk+T +C4xmEWxfiH15/JMAEQEAAbQbdXNlcjIgPHVzZXIyLTJAZXhhbXBsZS5jb20+iQHU +BBMBCAA+FiEEB/Y4DM3Ba2H9iXmlPO9G70C+/D4FAmC2K2cCGwMFCQPCZwAFCwkI +BwIGFQoJCAsCBBYCAwECHgECF4AACgkQPO9G70C+/D59/Av/XZIhCH4X2FpxCO3d +oCa+sbYkBL5xeUoPfAx5ThXzqL/tllO88TKTMEGZF3k5pocXWH0xmhqlvDTcdb0i +W3O0CN8FLmuotU51c0JC1mt9zwJP9PeJNyqxrMm01Yzj55z/Dz3QHSTlDjrWTWjn +YBqDf2HfdM177oydfSYmevZni1aDmBalWpFPRvqISCO7uFnvg1hJQ5mD/0qie663 +QJ8LAAANg32H9DyPnYi9wU62WX0DMUVTjKctT3cnYCbirjjJ7ZlCCm+cf61CRX1B +E1Ng/Ef3ZcUfXWitZSjfET/pKEMSNjsQawFpZ/LPCBl+UPHzaTPAASeGJvcbZ3py +wZQLQc1MCu2hmMBQ8zHQTdS2Pp0RISxCQLYvVQL6DrcJDNiSqn9p9RQt5c5r5Pjx +80BIPcjj3glOVP7PYE2azQAkt6reEjhimwCfjeDpiPnkBTY7Av2jCcUFhhemDY/j +TRXK1paLphhJ36zC22SeHGxNNakjjuUakqB85DEUeoWuVm6ouQGNBGC2K2cBDADx +G2rIAgMjdPtofhkEZXwv6zdNwmYOlIIM+59bam9Ep/vFq8F5f+xldevm5dvM8SeR +pNwDGSOUf5OKBWBdsJFhlYBl7+EcKd/Tent/XS6JoA9ffF33b+r04L543+ykiKON +WYeYi0F4WwYTIQgqZHJze1sPVkYGR5F0bL8PAcLuwd5dzZVi/q2HakrGdg29N8oY +b/XnoR7FflPrNYdzO6hawi5Inx7KS7aWa0ZkARb0F4HSct+/m6nAZVsoJINLudyQ +ut2NWeU8rWIm1hqyIxQFvuQJy46umq++10J/sWA98bkg41Rx+72+eP7DM5v8IgUp +clJsfljRXIBWbmRAVZvtNI7PX9fwMMhf4M7wHO7G2WV39o1exKps5xFFcn8PUQiX +jCSR81M145CgCdmLUR1y0pdkN/WIqjXBhkPIvO2dxEcodMNHb1aUUuUOnww6+xIP +8rGVw+a2DUiALc8Qr5RP21AYKRctfiwhSQh2KODveMtyLI3U9C/eLRPp+QM3XB8A +EQEAAYkBvAQYAQgAJhYhBAf2OAzNwWth/Yl5pTzvRu9Avvw+BQJgtitnAhsMBQkD +wmcAAAoJEDzvRu9Avvw+3FcMAJBwupyJ4zwQFxTJ5BkDlusG3U2FXEf3bDrXhvNd +qi8eS8Vo/vRiH/w/my5JFpz1o2tJToryF71D+uF5DTItalKquhsQ9reAEmXggqOh +9Jd9mWJIEEWcRORiLNDKENKvE8bouw4U4hRaSF0IaGzAe5mO+oOvwal8L97wFxrZ +4leM1GzkopiuNfbkkBBw2KJcMjYBHzzXSCALnVwhjbgkBEWPIg38APT3cr9KfnMM +q8+tvsGLj4piAl3Lww7+GhSsDOUXH8btR41BSAQDrbO5q6oi/h4nuxoNmQIDW/Ug +s+dd5hnY2FtHRjb4FCR9kAjdTE6stc8wzohWfbg1N+12TTA2ylByAumICVXixavH +RJ7l0OiWJk388qw9mqh3k8HcBxL7OfDlFC9oPmCS0iYiIwW/Yc80kBhoxcvl/Xa7 +mIMMn8taHIaQO7v9ln2EVQYTzbNCmwTw9ovTM0j/Pbkg2EftfP1TCoxQHvBnsCED +6qgtsUdi5eviONRkBgeZtN3oxA== +=MgDv -----END PGP PUBLIC KEY BLOCK-----`) } diff --git a/integrations/api_user_email_test.go b/integrations/api_user_email_test.go index 8d0a0cdf1..9d2b7485d 100644 --- a/integrations/api_user_email_test.go +++ b/integrations/api_user_email_test.go @@ -33,7 +33,7 @@ func TestAPIListEmails(t *testing.T) { Primary: true, }, { - Email: "user21@example.com", + Email: "user2-2@example.com", Verified: false, Primary: false, }, @@ -55,7 +55,7 @@ func TestAPIAddEmail(t *testing.T) { session.MakeRequest(t, req, http.StatusUnprocessableEntity) opts = api.CreateEmailOption{ - Emails: []string{"user22@example.com"}, + Emails: []string{"user2-3@example.com"}, } req = NewRequestWithJSON(t, "POST", "/api/v1/user/emails?token="+token, &opts) resp := session.MakeRequest(t, req, http.StatusCreated) @@ -64,7 +64,7 @@ func TestAPIAddEmail(t *testing.T) { DecodeJSON(t, resp, &emails) assert.EqualValues(t, []*api.Email{ { - Email: "user22@example.com", + Email: "user2-3@example.com", Verified: true, Primary: false, }, @@ -79,13 +79,13 @@ func TestAPIDeleteEmail(t *testing.T) { token := getTokenForLoggedInUser(t, session) opts := api.DeleteEmailOption{ - Emails: []string{"user22@example.com"}, + Emails: []string{"user2-3@example.com"}, } req := NewRequestWithJSON(t, "DELETE", "/api/v1/user/emails?token="+token, &opts) session.MakeRequest(t, req, http.StatusNotFound) opts = api.DeleteEmailOption{ - Emails: []string{"user21@example.com"}, + Emails: []string{"user2-2@example.com"}, } req = NewRequestWithJSON(t, "DELETE", "/api/v1/user/emails?token="+token, &opts) session.MakeRequest(t, req, http.StatusNoContent) diff --git a/models/error.go b/models/error.go index 48cba57a8..501bf8686 100644 --- a/models/error.go +++ b/models/error.go @@ -237,6 +237,21 @@ func (err ErrEmailAddressNotExist) Error() string { return fmt.Sprintf("Email address does not exist [email: %s]", err.Email) } +// ErrPrimaryEmailCannotDelete primary email address cannot be deleted +type ErrPrimaryEmailCannotDelete struct { + Email string +} + +// IsErrPrimaryEmailCannotDelete checks if an error is an ErrPrimaryEmailCannotDelete +func IsErrPrimaryEmailCannotDelete(err error) bool { + _, ok := err.(ErrPrimaryEmailCannotDelete) + return ok +} + +func (err ErrPrimaryEmailCannotDelete) Error() string { + return fmt.Sprintf("Primary email address cannot be deleted [email: %s]", err.Email) +} + // ErrOpenIDAlreadyUsed represents a "OpenIDAlreadyUsed" kind of error. type ErrOpenIDAlreadyUsed struct { OpenID string diff --git a/models/fixtures/email_address.yml b/models/fixtures/email_address.yml index c37d9a787..e7df5fdc5 100644 --- a/models/fixtures/email_address.yml +++ b/models/fixtures/email_address.yml @@ -1,35 +1,279 @@ - id: 1 - uid: 1 + uid: 11 email: user11@example.com + lower_email: user11@example.com is_activated: false + is_primary: true - id: 2 - uid: 1 + uid: 12 email: user12@example.com - is_activated: false + lower_email: user12@example.com + is_activated: true + is_primary: true - id: 3 uid: 2 email: user2@example.com + lower_email: user2@example.com is_activated: true + is_primary: true - id: 4 - uid: 2 + uid: 21 email: user21@example.com - is_activated: false + lower_email: user21@example.com + is_activated: true + is_primary: true - id: 5 uid: 9999999 email: user9999999@example.com + lower_email: user9999999@example.com is_activated: true + is_primary: false - id: 6 uid: 10 + email: user10@example.com + lower_email: user10@example.com + is_activated: true + is_primary: true + +- + id: 7 + uid: 10 email: user101@example.com + lower_email: user101@example.com + is_activated: true + is_primary: false + +- + id: 8 + uid: 9 + email: user9@example.com + lower_email: user9@example.com + is_activated: false + is_primary: true + +- + id: 9 + uid: 1 + email: user1@example.com + lower_email: user1@example.com + is_activated: true + is_primary: true + +- + id: 10 + uid: 3 + email: user3@example.com + lower_email: user3@example.com + is_activated: true + is_primary: true + +- + id: 11 + uid: 4 + email: user4@example.com + lower_email: user4@example.com + is_activated: true + is_primary: true + +- + id: 12 + uid: 5 + email: user5@example.com + lower_email: user5@example.com is_activated: true + is_primary: true + +- + id: 13 + uid: 6 + email: user6@example.com + lower_email: user6@example.com + is_activated: true + is_primary: true + +- + id: 14 + uid: 7 + email: user7@example.com + lower_email: user7@example.com + is_activated: true + is_primary: true + +- + id: 15 + uid: 8 + email: user8@example.com + lower_email: user8@example.com + is_activated: true + is_primary: true + +- + id: 16 + uid: 13 + email: user13@example.com + lower_email: user13@example.com + is_activated: true + is_primary: true + +- + id: 17 + uid: 14 + email: user14@example.com + lower_email: user14@example.com + is_activated: true + is_primary: true + +- + id: 18 + uid: 15 + email: user15@example.com + lower_email: user15@example.com + is_activated: true + is_primary: true + +- + id: 19 + uid: 16 + email: user16@example.com + lower_email: user16@example.com + is_activated: true + is_primary: true + +- + id: 20 + uid: 17 + email: user17@example.com + lower_email: user17@example.com + is_activated: true + is_primary: true + +- + id: 21 + uid: 18 + email: user18@example.com + lower_email: user18@example.com + is_activated: true + is_primary: true + +- + id: 22 + uid: 19 + email: user19@example.com + lower_email: user19@example.com + is_activated: true + is_primary: true + +- + id: 23 + uid: 20 + email: user20@example.com + lower_email: user20@example.com + is_activated: true + is_primary: true + +- + id: 24 + uid: 22 + email: limited_org@example.com + lower_email: limited_org@example.com + is_activated: true + is_primary: true + +- + id: 25 + uid: 23 + email: privated_org@example.com + lower_email: privated_org@example.com + is_activated: true + is_primary: true + +- + id: 26 + uid: 24 + email: user24@example.com + lower_email: user24@example.com + is_activated: true + is_primary: true + +- + id: 27 + uid: 25 + email: org25@example.com + lower_email: org25@example.com + is_activated: true + is_primary: true + +- + id: 28 + uid: 26 + email: org26@example.com + lower_email: org26@example.com + is_activated: true + is_primary: true + +- + id: 29 + uid: 27 + email: user27@example.com + lower_email: user27@example.com + is_activated: true + is_primary: true + +- + id: 30 + uid: 28 + email: user28@example.com + lower_email: user28@example.com + is_activated: true + is_primary: true + +- + id: 31 + uid: 29 + email: user29@example.com + lower_email: user29@example.com + is_activated: true + is_primary: true + +- + id: 32 + uid: 30 + email: user30@example.com + lower_email: user30@example.com + is_activated: true + is_primary: true + +- + id: 33 + uid: 1 + email: user1-2@example.com + lower_email: user1-2@example.com + is_activated: true + is_primary: false + +- + id: 34 + uid: 1 + email: user1-3@example.com + lower_email: user1-3@example.com + is_activated: true + is_primary: false + +- + id: 35 + uid: 2 + email: user2-2@example.com + lower_email: user2-2@example.com + is_activated: false + is_primary: false \ No newline at end of file diff --git a/models/gpg_key.go b/models/gpg_key.go index 2ffcf47ca..9530eacb0 100644 --- a/models/gpg_key.go +++ b/models/gpg_key.go @@ -301,7 +301,7 @@ func parseGPGKey(ownerID int64, e *openpgp.Entity) (*GPGKey, error) { } email := strings.ToLower(strings.TrimSpace(ident.UserId.Email)) for _, e := range userEmails { - if e.Email == email { + if e.LowerEmail == email { emails = append(emails, e) break } diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index d440722c9..df1bac4a1 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -311,6 +311,8 @@ var migrations = []Migration{ NewMigration("Convert avatar url to text", convertAvatarURLToText), // v180 -> v181 NewMigration("Delete credentials from past migrations", deleteMigrationCredentials), + // v181 -> v182 + NewMigration("Always save primary email on email address table", addPrimaryEmail2EmailAddress), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v181.go b/models/migrations/v181.go new file mode 100644 index 000000000..6ba4edc15 --- /dev/null +++ b/models/migrations/v181.go @@ -0,0 +1,92 @@ +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "strings" + + "xorm.io/xorm" +) + +func addPrimaryEmail2EmailAddress(x *xorm.Engine) (err error) { + type User struct { + ID int64 `xorm:"pk autoincr"` + Email string `xorm:"NOT NULL"` + IsActive bool `xorm:"INDEX"` // Activate primary email + } + + type EmailAddress1 struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX NOT NULL"` + Email string `xorm:"UNIQUE NOT NULL"` + LowerEmail string + IsActivated bool + IsPrimary bool `xorm:"DEFAULT(false) NOT NULL"` + } + + // Add lower_email and is_primary columns + if err = x.Table("email_address").Sync2(new(EmailAddress1)); err != nil { + return + } + + if _, err = x.Exec("UPDATE email_address SET lower_email=LOWER(email), is_primary=?", false); err != nil { + return + } + + type EmailAddress struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX NOT NULL"` + Email string `xorm:"UNIQUE NOT NULL"` + LowerEmail string `xorm:"UNIQUE NOT NULL"` + IsActivated bool + IsPrimary bool `xorm:"DEFAULT(false) NOT NULL"` + } + + // change lower_email as unique + if err = x.Sync2(new(EmailAddress)); err != nil { + return + } + + sess := x.NewSession() + defer sess.Close() + + const batchSize = 100 + + for start := 0; ; start += batchSize { + users := make([]*User, 0, batchSize) + if err = sess.Limit(batchSize, start).Find(&users); err != nil { + return + } + if len(users) == 0 { + break + } + + for _, user := range users { + var exist bool + exist, err = sess.Where("email=?", user.Email).Table("email_address").Exist() + if err != nil { + return + } + if !exist { + if _, err = sess.Insert(&EmailAddress{ + UID: user.ID, + Email: user.Email, + LowerEmail: strings.ToLower(user.Email), + IsActivated: user.IsActive, + IsPrimary: true, + }); err != nil { + return + } + } else { + if _, err = sess.Where("email=?", user.Email).Cols("is_primary").Update(&EmailAddress{ + IsPrimary: true, + }); err != nil { + return + } + } + } + } + + return nil +} diff --git a/models/migrations/v181_test.go b/models/migrations/v181_test.go new file mode 100644 index 000000000..b392a9b71 --- /dev/null +++ b/models/migrations/v181_test.go @@ -0,0 +1,54 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_addPrimaryEmail2EmailAddress(t *testing.T) { + type User struct { + ID int64 + Email string + IsActive bool + } + + // Prepare and load the testing database + x, deferable := prepareTestEnv(t, 0, new(User)) + if x == nil || t.Failed() { + defer deferable() + return + } + defer deferable() + + err := addPrimaryEmail2EmailAddress(x) + assert.NoError(t, err) + + type EmailAddress struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX NOT NULL"` + Email string `xorm:"UNIQUE NOT NULL"` + LowerEmail string `xorm:"UNIQUE NOT NULL"` + IsActivated bool + IsPrimary bool `xorm:"DEFAULT(false) NOT NULL"` + } + + var users = make([]User, 0, 20) + err = x.Find(&users) + assert.NoError(t, err) + + for _, user := range users { + var emailAddress EmailAddress + has, err := x.Where("lower_email=?", strings.ToLower(user.Email)).Get(&emailAddress) + assert.NoError(t, err) + assert.True(t, has) + assert.True(t, emailAddress.IsPrimary) + assert.EqualValues(t, user.IsActive, emailAddress.IsActivated) + assert.EqualValues(t, user.ID, emailAddress.UID) + } +} diff --git a/models/user.go b/models/user.go index c7b44bdb7..002c05065 100644 --- a/models/user.go +++ b/models/user.go @@ -74,9 +74,6 @@ const ( ) var ( - // ErrEmailNotExist e-mail does not exist error - ErrEmailNotExist = errors.New("E-mail does not exist") - // ErrEmailNotActivated e-mail address has not been activated error ErrEmailNotActivated = errors.New("E-mail address has not been activated") @@ -876,15 +873,6 @@ func CreateUser(u *User) (err error) { } u.Email = strings.ToLower(u.Email) - isExist, err = sess. - Where("email=?", u.Email). - Get(new(User)) - if err != nil { - return err - } else if isExist { - return ErrEmailAlreadyUsed{u.Email} - } - if err = ValidateEmail(u.Email); err != nil { return err } @@ -915,6 +903,17 @@ func CreateUser(u *User) (err error) { return err } + // insert email address + if _, err := sess.Insert(&EmailAddress{ + UID: u.ID, + Email: u.Email, + LowerEmail: strings.ToLower(u.Email), + IsActivated: u.IsActive, + IsPrimary: true, + }); err != nil { + return err + } + return sess.Commit() } diff --git a/models/user_mail.go b/models/user_mail.go index 1bdd6a423..2758dfb8e 100644 --- a/models/user_mail.go +++ b/models/user_mail.go @@ -17,14 +17,22 @@ import ( "xorm.io/builder" ) -// EmailAddress is the list of all email addresses of a user. Can contain the -// primary email address, but is not obligatory. +// EmailAddress is the list of all email addresses of a user. It also contains the +// primary email address which is saved in user table. type EmailAddress struct { ID int64 `xorm:"pk autoincr"` UID int64 `xorm:"INDEX NOT NULL"` Email string `xorm:"UNIQUE NOT NULL"` + LowerEmail string `xorm:"UNIQUE NOT NULL"` IsActivated bool - IsPrimary bool `xorm:"-"` + IsPrimary bool `xorm:"DEFAULT(false) NOT NULL"` +} + +// BeforeInsert will be invoked by XORM before inserting a record +func (email *EmailAddress) BeforeInsert() { + if email.LowerEmail == "" { + email.LowerEmail = strings.ToLower(email.Email) + } } // ValidateEmail check if email is a allowed address @@ -47,34 +55,10 @@ func GetEmailAddresses(uid int64) ([]*EmailAddress, error) { emails := make([]*EmailAddress, 0, 5) if err := x. Where("uid=?", uid). + Asc("id"). Find(&emails); err != nil { return nil, err } - - u, err := GetUserByID(uid) - if err != nil { - return nil, err - } - - isPrimaryFound := false - for _, email := range emails { - if email.Email == u.Email { - isPrimaryFound = true - email.IsPrimary = true - } else { - email.IsPrimary = false - } - } - - // We always want the primary email address displayed, even if it's not in - // the email address table (yet). - if !isPrimaryFound { - emails = append(emails, &EmailAddress{ - Email: u.Email, - IsActivated: u.IsActive, - IsPrimary: true, - }) - } return emails, nil } @@ -97,14 +81,13 @@ func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error) // Can't filter by boolean field unless it's explicit cond := builder.NewCond() - cond = cond.And(builder.Eq{"email": email}, builder.Neq{"id": emailID}) + cond = cond.And(builder.Eq{"lower_email": strings.ToLower(email)}, builder.Neq{"id": emailID}) if setting.Service.RegisterEmailConfirm { // Inactive (unvalidated) addresses don't count as active if email validation is required cond = cond.And(builder.Eq{"is_activated": true}) } - em := EmailAddress{} - + var em EmailAddress if has, err := e.Where(cond).Get(&em); has || err != nil { if has { log.Info("isEmailActive('%s',%d,%d) found duplicate in email ID %d", email, userID, emailID, em.ID) @@ -112,22 +95,6 @@ func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error) return has, err } - // Can't filter by boolean field unless it's explicit - cond = builder.NewCond() - cond = cond.And(builder.Eq{"email": email}, builder.Neq{"id": userID}) - if setting.Service.RegisterEmailConfirm { - cond = cond.And(builder.Eq{"is_active": true}) - } - - us := User{} - - if has, err := e.Where(cond).Get(&us); has || err != nil { - if has { - log.Info("isEmailActive('%s',%d,%d) found duplicate in user ID %d", email, userID, emailID, us.ID) - } - return has, err - } - return false, nil } @@ -136,7 +103,7 @@ func isEmailUsed(e Engine, email string) (bool, error) { return true, nil } - return e.Where("email=?", email).Get(&EmailAddress{}) + return e.Where("lower_email=?", strings.ToLower(email)).Get(&EmailAddress{}) } // IsEmailUsed returns true if the email has been used. @@ -145,7 +112,7 @@ func IsEmailUsed(email string) (bool, error) { } func addEmailAddress(e Engine, email *EmailAddress) error { - email.Email = strings.ToLower(strings.TrimSpace(email.Email)) + email.Email = strings.TrimSpace(email.Email) used, err := isEmailUsed(e, email.Email) if err != nil { return err @@ -174,7 +141,7 @@ func AddEmailAddresses(emails []*EmailAddress) error { // Check if any of them has been used for i := range emails { - emails[i].Email = strings.ToLower(strings.TrimSpace(emails[i].Email)) + emails[i].Email = strings.TrimSpace(emails[i].Email) used, err := IsEmailUsed(emails[i].Email) if err != nil { return err @@ -223,6 +190,10 @@ func (email *EmailAddress) updateActivation(e Engine, activate bool) error { // DeleteEmailAddress deletes an email address of given user. func DeleteEmailAddress(email *EmailAddress) (err error) { + if email.IsPrimary { + return ErrPrimaryEmailCannotDelete{Email: email.Email} + } + var deleted int64 // ask to check UID address := EmailAddress{ @@ -231,8 +202,11 @@ func DeleteEmailAddress(email *EmailAddress) (err error) { if email.ID > 0 { deleted, err = x.ID(email.ID).Delete(&address) } else { + if email.Email != "" && email.LowerEmail == "" { + email.LowerEmail = strings.ToLower(email.Email) + } deleted, err = x. - Where("email=?", email.Email). + Where("lower_email=?", email.LowerEmail). Delete(&address) } @@ -261,7 +235,7 @@ func MakeEmailPrimary(email *EmailAddress) error { if err != nil { return err } else if !has { - return ErrEmailNotExist + return ErrEmailAddressNotExist{Email: email.Email} } if !email.IsActivated { @@ -276,32 +250,31 @@ func MakeEmailPrimary(email *EmailAddress) error { return ErrUserNotExist{email.UID, "", 0} } - // Make sure the former primary email doesn't disappear. - formerPrimaryEmail := &EmailAddress{UID: user.ID, Email: user.Email} - has, err = x.Get(formerPrimaryEmail) - if err != nil { - return err - } - sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { return err } - if !has { - formerPrimaryEmail.UID = user.ID - formerPrimaryEmail.IsActivated = user.IsActive - if _, err = sess.Insert(formerPrimaryEmail); err != nil { - return err - } - } - + // 1. Update user table user.Email = email.Email if _, err = sess.ID(user.ID).Cols("email").Update(user); err != nil { return err } + // 2. Update old primary email + if _, err = sess.Where("uid=? AND is_primary=?", email.UID, true).Cols("is_primary").Update(&EmailAddress{ + IsPrimary: false, + }); err != nil { + return err + } + + // 3. update new primay email + email.IsPrimary = true + if _, err = sess.ID(email.ID).Cols("is_primary").Update(email); err != nil { + return err + } + return sess.Commit() } @@ -314,10 +287,10 @@ func (s SearchEmailOrderBy) String() string { // Strings for sorting result const ( - SearchEmailOrderByEmail SearchEmailOrderBy = "emails.email ASC, is_primary DESC, sortid ASC" - SearchEmailOrderByEmailReverse SearchEmailOrderBy = "emails.email DESC, is_primary ASC, sortid DESC" - SearchEmailOrderByName SearchEmailOrderBy = "`user`.lower_name ASC, is_primary DESC, sortid ASC" - SearchEmailOrderByNameReverse SearchEmailOrderBy = "`user`.lower_name DESC, is_primary ASC, sortid DESC" + SearchEmailOrderByEmail SearchEmailOrderBy = "email_address.lower_email ASC, email_address.is_primary DESC, email_address.id ASC" + SearchEmailOrderByEmailReverse SearchEmailOrderBy = "email_address.lower_email DESC, email_address.is_primary ASC, email_address.id DESC" + SearchEmailOrderByName SearchEmailOrderBy = "`user`.lower_name ASC, email_address.is_primary DESC, email_address.id ASC" + SearchEmailOrderByNameReverse SearchEmailOrderBy = "`user`.lower_name DESC, email_address.is_primary ASC, email_address.id DESC" ) // SearchEmailOptions are options to search e-mail addresses for the admin panel @@ -343,54 +316,32 @@ type SearchEmailResult struct { // SearchEmails takes options i.e. keyword and part of email name to search, // it returns results in given range and number of total results. func SearchEmails(opts *SearchEmailOptions) ([]*SearchEmailResult, int64, error) { - // Unfortunately, UNION support for SQLite in xorm is currently broken, so we must - // build the SQL ourselves. - where := make([]string, 0, 5) - args := make([]interface{}, 0, 5) - - emailsSQL := "(SELECT id as sortid, uid, email, is_activated, 0 as is_primary " + - "FROM email_address " + - "UNION ALL " + - "SELECT id as sortid, id AS uid, email, is_active AS is_activated, 1 as is_primary " + - "FROM `user` " + - "WHERE type = ?) AS emails" - args = append(args, UserTypeIndividual) - + var cond builder.Cond = builder.Eq{"user.`type`": UserTypeIndividual} if len(opts.Keyword) > 0 { - // Note: % can be injected in the Keyword parameter, but it won't do any harm. - where = append(where, "(lower(`user`.full_name) LIKE ? OR `user`.lower_name LIKE ? OR emails.email LIKE ?)") likeStr := "%" + strings.ToLower(opts.Keyword) + "%" - args = append(args, likeStr) - args = append(args, likeStr) - args = append(args, likeStr) + cond = cond.And(builder.Or( + builder.Like{"lower(`user`.full_name)", likeStr}, + builder.Like{"`user`.lower_name", likeStr}, + builder.Like{"email_address.lower_email", likeStr}, + )) } switch { case opts.IsPrimary.IsTrue(): - where = append(where, "emails.is_primary = ?") - args = append(args, true) + cond = cond.And(builder.Eq{"email_address.is_primary": true}) case opts.IsPrimary.IsFalse(): - where = append(where, "emails.is_primary = ?") - args = append(args, false) + cond = cond.And(builder.Eq{"email_address.is_primary": false}) } switch { case opts.IsActivated.IsTrue(): - where = append(where, "emails.is_activated = ?") - args = append(args, true) + cond = cond.And(builder.Eq{"email_address.is_activated": true}) case opts.IsActivated.IsFalse(): - where = append(where, "emails.is_activated = ?") - args = append(args, false) - } - - var whereStr string - if len(where) > 0 { - whereStr = "WHERE " + strings.Join(where, " AND ") + cond = cond.And(builder.Eq{"email_address.is_activated": false}) } - joinSQL := "FROM " + emailsSQL + " INNER JOIN `user` ON `user`.id = emails.uid " + whereStr - - count, err := x.SQL("SELECT count(*) "+joinSQL, args...).Count() + count, err := x.Join("INNER", "`user`", "`user`.ID = email_address.uid"). + Where(cond).Count(new(EmailAddress)) if err != nil { return nil, 0, fmt.Errorf("Count: %v", err) } @@ -400,36 +351,16 @@ func SearchEmails(opts *SearchEmailOptions) ([]*SearchEmailResult, int64, error) orderby = SearchEmailOrderByEmail.String() } - querySQL := "SELECT emails.uid, emails.email, emails.is_activated, emails.is_primary, " + - "`user`.name, `user`.full_name " + joinSQL + " ORDER BY " + orderby - opts.setDefaultValues() - rows, err := x.SQL(querySQL, args...).Rows(new(SearchEmailResult)) - if err != nil { - return nil, 0, fmt.Errorf("Emails: %v", err) - } - - // Page manually because xorm can't handle Limit() with raw SQL - defer rows.Close() - emails := make([]*SearchEmailResult, 0, opts.PageSize) - skip := (opts.Page - 1) * opts.PageSize - - for rows.Next() { - var email SearchEmailResult - if err := rows.Scan(&email); err != nil { - return nil, 0, err - } - if skip > 0 { - skip-- - continue - } - emails = append(emails, &email) - if len(emails) == opts.PageSize { - break - } - } + err = x.Table("email_address"). + Select("email_address.*, `user`.name, `user`.full_name"). + Join("INNER", "`user`", "`user`.ID = email_address.uid"). + Where(cond). + OrderBy(orderby). + Limit(opts.PageSize, (opts.Page-1)*opts.PageSize). + Find(&emails) return emails, count, err } @@ -442,6 +373,30 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err if err = sess.Begin(); err != nil { return err } + + // Activate/deactivate a user's secondary email address + // First check if there's another user active with the same address + addr := EmailAddress{UID: userID, LowerEmail: strings.ToLower(email)} + if has, err := sess.Get(&addr); err != nil { + return err + } else if !has { + return fmt.Errorf("no such email: %d (%s)", userID, email) + } + if addr.IsActivated == activate { + // Already in the desired state; no action + return nil + } + if activate { + if used, err := isEmailActive(sess, email, 0, addr.ID); err != nil { + return fmt.Errorf("isEmailActive(): %v", err) + } else if used { + return ErrEmailAlreadyUsed{Email: email} + } + } + if err = addr.updateActivation(sess, activate); err != nil { + return fmt.Errorf("updateActivation(): %v", err) + } + if primary { // Activate/deactivate a user's primary email address user := User{ID: userID, Email: email} @@ -454,13 +409,6 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err // Already in the desired state; no action return nil } - if activate { - if used, err := isEmailActive(sess, email, userID, 0); err != nil { - return fmt.Errorf("isEmailActive(): %v", err) - } else if used { - return ErrEmailAlreadyUsed{Email: email} - } - } user.IsActive = activate if user.Rands, err = GetUserSalt(); err != nil { return fmt.Errorf("generate salt: %v", err) @@ -468,29 +416,7 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil { return fmt.Errorf("updateUserCols(): %v", err) } - } else { - // Activate/deactivate a user's secondary email address - // First check if there's another user active with the same address - addr := EmailAddress{UID: userID, Email: email} - if has, err := sess.Get(&addr); err != nil { - return err - } else if !has { - return fmt.Errorf("no such email: %d (%s)", userID, email) - } - if addr.IsActivated == activate { - // Already in the desired state; no action - return nil - } - if activate { - if used, err := isEmailActive(sess, email, 0, addr.ID); err != nil { - return fmt.Errorf("isEmailActive(): %v", err) - } else if used { - return ErrEmailAlreadyUsed{Email: email} - } - } - if err = addr.updateActivation(sess, activate); err != nil { - return fmt.Errorf("updateActivation(): %v", err) - } } + return sess.Commit() } diff --git a/models/user_mail_test.go b/models/user_mail_test.go index 5a223dd04..829a38c18 100644 --- a/models/user_mail_test.go +++ b/models/user_mail_test.go @@ -17,9 +17,9 @@ func TestGetEmailAddresses(t *testing.T) { emails, _ := GetEmailAddresses(int64(1)) if assert.Len(t, emails, 3) { - assert.False(t, emails[0].IsPrimary) + assert.True(t, emails[0].IsPrimary) assert.True(t, emails[2].IsActivated) - assert.True(t, emails[2].IsPrimary) + assert.False(t, emails[2].IsPrimary) } emails, _ = GetEmailAddresses(int64(2)) @@ -45,13 +45,15 @@ func TestAddEmailAddress(t *testing.T) { assert.NoError(t, AddEmailAddress(&EmailAddress{ Email: "user1234567890@example.com", + LowerEmail: "user1234567890@example.com", IsPrimary: true, IsActivated: true, })) // ErrEmailAlreadyUsed err := AddEmailAddress(&EmailAddress{ - Email: "user1234567890@example.com", + Email: "user1234567890@example.com", + LowerEmail: "user1234567890@example.com", }) assert.Error(t, err) assert.True(t, IsErrEmailAlreadyUsed(err)) @@ -64,10 +66,12 @@ func TestAddEmailAddresses(t *testing.T) { emails := make([]*EmailAddress, 2) emails[0] = &EmailAddress{ Email: "user1234@example.com", + LowerEmail: "user1234@example.com", IsActivated: true, } emails[1] = &EmailAddress{ Email: "user5678@example.com", + LowerEmail: "user5678@example.com", IsActivated: true, } assert.NoError(t, AddEmailAddresses(emails)) @@ -82,20 +86,23 @@ func TestDeleteEmailAddress(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) assert.NoError(t, DeleteEmailAddress(&EmailAddress{ - UID: int64(1), - ID: int64(1), - Email: "user11@example.com", + UID: int64(1), + ID: int64(33), + Email: "user1-2@example.com", + LowerEmail: "user1-2@example.com", })) assert.NoError(t, DeleteEmailAddress(&EmailAddress{ - UID: int64(1), - Email: "user12@example.com", + UID: int64(1), + Email: "user1-3@example.com", + LowerEmail: "user1-3@example.com", })) // Email address does not exist err := DeleteEmailAddress(&EmailAddress{ - UID: int64(1), - Email: "user1234567890@example.com", + UID: int64(1), + Email: "user1234567890@example.com", + LowerEmail: "user1234567890@example.com", }) assert.Error(t, err) } @@ -106,13 +113,15 @@ func TestDeleteEmailAddresses(t *testing.T) { // delete multiple email address emails := make([]*EmailAddress, 2) emails[0] = &EmailAddress{ - UID: int64(2), - ID: int64(3), - Email: "user2@example.com", + UID: int64(2), + ID: int64(3), + Email: "user2@example.com", + LowerEmail: "user2@example.com", } emails[1] = &EmailAddress{ - UID: int64(2), - Email: "user21@example.com", + UID: int64(2), + Email: "user2-2@example.com", + LowerEmail: "user2-2@example.com", } assert.NoError(t, DeleteEmailAddresses(emails)) @@ -129,7 +138,7 @@ func TestMakeEmailPrimary(t *testing.T) { } err := MakeEmailPrimary(email) assert.Error(t, err) - assert.EqualError(t, err, ErrEmailNotExist.Error()) + assert.EqualError(t, err, ErrEmailAddressNotExist{email.Email}.Error()) email = &EmailAddress{ Email: "user11@example.com", @@ -168,15 +177,21 @@ func TestActivate(t *testing.T) { emails, _ := GetEmailAddresses(int64(1)) assert.Len(t, emails, 3) assert.True(t, emails[0].IsActivated) + assert.True(t, emails[0].IsPrimary) + assert.False(t, emails[1].IsPrimary) assert.True(t, emails[2].IsActivated) - assert.True(t, emails[2].IsPrimary) + assert.False(t, emails[2].IsPrimary) } func TestListEmails(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) // Must find all users and their emails - opts := &SearchEmailOptions{} + opts := &SearchEmailOptions{ + ListOptions: ListOptions{ + PageSize: 10000, + }, + } emails, count, err := SearchEmails(opts) assert.NoError(t, err) assert.NotEqual(t, int64(0), count)