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

@@ -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 <user21@example.com> //secondary and not activated
+	//User2 <user2-2@example.com> //secondary and not activated
 	testCreateGPGKey(t, makeRequest, token, expected, `-----BEGIN PGP PUBLIC KEY BLOCK-----
 
-mQENBFmGWN4BCAC18V4tVGO65VLCV7p14FuXJlUtZ5CuYMvgEkcOqrvRaBSW9ao4
+mQGNBGC2K2cBDAC1+Xgk+8UfhASVgRngQi4rnQ8k0t+bWsBz4Czd26+cxVDRwlTT
-PGESOhJpfWpnW3QgJniYndLzPpsmdHEclEER6aZjiNgReWPOjHD5tykWocZAJqXD
+8PALdrbrY/e9iXjcVcZ8Npo4UYe7/LfnL57dc7tgbenRGYYrWyVoNNv58BVw4xCY
-eY1ym59gvVMLcfbV2yQsyR2hbJlc+dJsl16tigSEe3nwxZSw2IsW92pgEzT9JNUr
+RmgvdHWIIPGuz3aME0smHxbJ2KewYTqjTPuVKF/wrHTwCpVWdjYKC5KDo3yx0mro
-Q+mC8dw4dqY0tYmFazYUGNxufUc/twgQT/Or1aNs0az5Q6Jft4rrTRsh/S7We0VB
+xf9vOJOnkWNMiEw7TiZfkrbUqxyA53BVsSNKRX5C3b4FJcVT7eiAq7sDAaFxjEHy
-COKGkdcQyYgAls7HJBuPjQRi6DM9VhgBSHLAgSLyaUcZvhZBJr8Qe/q4PP3/kYDJ
+ahZslmvg7XZxWzSVzxDNesR7f4xuop8HBjzaluJoVuwiyWculTvz1b6hyHVQr+ad
-wm4RMnjOLz2pFZPgtRqgcAwpmFtLrACbEB3JABEBAAG0GlVzZXIyIDx1c2VyMjFA
+h8JGjj1tySI65OTFsTuptsfHXjtjl/NR4P6BXkf+FVwweaTQaEzpHkv0m9b9pY43
-ZXhhbXBsZS5jb20+iQFUBBMBCAA+FiEEPOLHOjPSO42DWM57+r85c5/h6ScFAlmG
+CY/8XtS4uNPermiLG/Z0BB1eOCdoOQVHpjOa55IXQWhxXB6NZVyowiUbrR7jLDQy
-WN4CGwMFCQPCZwAFCwkIBwIGFQgJCgsCBBYCAwECHgECF4AACgkQ+r85c5/h6Sfx
+5JP7D1HmErTR8JRm3VDqGbSaCgugRgFX+lb/fpgFp9k02OeK+JQudolZOt1mVk+T
-Lgf/dq64NBV8+X9an3seaLxePRviva48e4K67/wV/JxtXNO5Z/DhMGz5kHXCsG9D
+C4xmEWxfiH15/JMAEQEAAbQbdXNlcjIgPHVzZXIyLTJAZXhhbXBsZS5jb20+iQHU
-CXuWYO8ehlTjEnMZ6qqdDnY+H6bQsb2OS5oPn4RwpPXslAjEKtojPAr0dDsMS2DB
+BBMBCAA+FiEEB/Y4DM3Ba2H9iXmlPO9G70C+/D4FAmC2K2cCGwMFCQPCZwAFCwkI
-dUuIm1AoOnewOVO0OFRf1EqX1bivxnN0FVMcO0m8AczfnKDaGb0y/qg/Y9JAsKqp
+BwIGFQoJCAsCBBYCAwECHgECF4AACgkQPO9G70C+/D59/Av/XZIhCH4X2FpxCO3d
-j5pZNMWUkntRtGySeJ4CVJMmkVKJAHsa1Qj6MKdFeid4h4y94cBJ4ZdyBxNdpQOx
+oCa+sbYkBL5xeUoPfAx5ThXzqL/tllO88TKTMEGZF3k5pocXWH0xmhqlvDTcdb0i
-ydf0doicovfeqGNO4oWzsGP4RBK2CqGPCUT+EFl20jPvMkKwOjxgqc8p0z3b2UT9
+W3O0CN8FLmuotU51c0JC1mt9zwJP9PeJNyqxrMm01Yzj55z/Dz3QHSTlDjrWTWjn
-+9bnmCGHgF/fW1HJ3iKmfFPqnLkBDQRZhljeAQgA5AirU/NJGgm19ZJYFOiHftjS
+YBqDf2HfdM177oydfSYmevZni1aDmBalWpFPRvqISCO7uFnvg1hJQ5mD/0qie663
-azbrPxGeD3cSqmvDPIMc1DNZGfQV5D4EVumnVbQBtL6xHFoGKz9KisUMbe4a/X2J
+QJ8LAAANg32H9DyPnYi9wU62WX0DMUVTjKctT3cnYCbirjjJ7ZlCCm+cf61CRX1B
-S8JmIphQWG0vMJX1DaZIzr2gT71MnPD7JMGsSUCh5dIKpTNTZX4w+oGPGOu0/UlL
+E1Ng/Ef3ZcUfXWitZSjfET/pKEMSNjsQawFpZ/LPCBl+UPHzaTPAASeGJvcbZ3py
-x0448AryKwp30J2p6D4GeI0nb03n35S2lTOpnHDn1wj7Jl/8LS2fdFOdNaNHXSZe
+wZQLQc1MCu2hmMBQ8zHQTdS2Pp0RISxCQLYvVQL6DrcJDNiSqn9p9RQt5c5r5Pjx
-twdSwJKhyBEiScgeHBDyKqo8zWkYoSb9eA2HiYlbVaiNtp24KP1mIEpiUdrRjWno
+80BIPcjj3glOVP7PYE2azQAkt6reEjhimwCfjeDpiPnkBTY7Av2jCcUFhhemDY/j
-zauYSZGHZlOFMgF4dKWuetPiuH9m7UYZGKyMLfQ9vYFb+xcPh2bLCQHJ1OEmMQAR
+TRXK1paLphhJ36zC22SeHGxNNakjjuUakqB85DEUeoWuVm6ouQGNBGC2K2cBDADx
-AQABiQE8BBgBCAAmFiEEPOLHOjPSO42DWM57+r85c5/h6ScFAlmGWN4CGwwFCQPC
+G2rIAgMjdPtofhkEZXwv6zdNwmYOlIIM+59bam9Ep/vFq8F5f+xldevm5dvM8SeR
-ZwAACgkQ+r85c5/h6Sfjfwf+O4WEjRdvPJLxNy7mfAGoAqDMHIwyH/tVzYgyVhnG
+pNwDGSOUf5OKBWBdsJFhlYBl7+EcKd/Tent/XS6JoA9ffF33b+r04L543+ykiKON
-h/+cfRxJbGc3rpjYdr8dmvghzjEAout8uibPWaIqs63RCAPGPqgWLfxNO5c8+y8V
+WYeYi0F4WwYTIQgqZHJze1sPVkYGR5F0bL8PAcLuwd5dzZVi/q2HakrGdg29N8oY
-LZMVOTV26l2olkkdBWAuhLqKTNh6TiQva03yhOgHWj4XDvFfxICWPFXVd6t5ELpD
+b/XnoR7FflPrNYdzO6hawi5Inx7KS7aWa0ZkARb0F4HSct+/m6nAZVsoJINLudyQ
-iApGu1OAj8JfhmzbG03Yzx+Ku7bWDxMonx3V/IDEu5LS5zrboHYDKCA53bXXghoi
+ut2NWeU8rWIm1hqyIxQFvuQJy46umq++10J/sWA98bkg41Rx+72+eP7DM5v8IgUp
-Aceqql+PKrDwEjoY4bptwMHLmcjGjdCQ//Qx1neho7nZcS7xjTucY8gQuulwCyXF
+clJsfljRXIBWbmRAVZvtNI7PX9fwMMhf4M7wHO7G2WV39o1exKps5xFFcn8PUQiX
-y6wM+wMz8dunIG9gw4+Re6c4Rz9tX1kzxLrU7Pl21tMqfg==
+jCSR81M145CgCdmLUR1y0pdkN/WIqjXBhkPIvO2dxEcodMNHb1aUUuUOnww6+xIP
-=0N/9
+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-----`)
 }

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)

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

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: user101@example.com
+  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

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
 			}

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

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

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

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

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
+// EmailAddress is the list of all email addresses of a user. It also contains the
-// primary email address, but is not obligatory.
+// 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 {
+	// 1. Update user table
-		formerPrimaryEmail.UID = user.ID
-		formerPrimaryEmail.IsActivated = user.IsActive
-		if _, err = sess.Insert(formerPrimaryEmail); err != nil {
-			return err
-		}
-	}
-
 	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"
+	SearchEmailOrderByEmail        SearchEmailOrderBy = "email_address.lower_email ASC, email_address.is_primary DESC, email_address.id ASC"
-	SearchEmailOrderByEmailReverse SearchEmailOrderBy = "emails.email DESC, is_primary ASC, sortid DESC"
+	SearchEmailOrderByEmailReverse SearchEmailOrderBy = "email_address.lower_email DESC, email_address.is_primary ASC, email_address.id DESC"
-	SearchEmailOrderByName         SearchEmailOrderBy = "`user`.lower_name ASC, is_primary DESC, sortid ASC"
+	SearchEmailOrderByName         SearchEmailOrderBy = "`user`.lower_name ASC, email_address.is_primary DESC, email_address.id ASC"
-	SearchEmailOrderByNameReverse  SearchEmailOrderBy = "`user`.lower_name DESC, is_primary ASC, sortid DESC"
+	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
+	var cond builder.Cond = builder.Eq{"user.`type`": UserTypeIndividual}
-	// 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)
-
 	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)
+		cond = cond.And(builder.Or(
-		args = append(args, likeStr)
+			builder.Like{"lower(`user`.full_name)", likeStr},
-		args = append(args, 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 = ?")
+		cond = cond.And(builder.Eq{"email_address.is_primary": true})
-		args = append(args, true)
 	case opts.IsPrimary.IsFalse():
-		where = append(where, "emails.is_primary = ?")
+		cond = cond.And(builder.Eq{"email_address.is_primary": false})
-		args = append(args, false)
 	}
 
 	switch {
 	case opts.IsActivated.IsTrue():
-		where = append(where, "emails.is_activated = ?")
+		cond = cond.And(builder.Eq{"email_address.is_activated": true})
-		args = append(args, true)
 	case opts.IsActivated.IsFalse():
-		where = append(where, "emails.is_activated = ?")
+		cond = cond.And(builder.Eq{"email_address.is_activated": false})
-		args = append(args, false)
 	}
 
-	var whereStr string
+	count, err := x.Join("INNER", "`user`", "`user`.ID = email_address.uid").
-	if len(where) > 0 {
+		Where(cond).Count(new(EmailAddress))
-		whereStr = "WHERE " + strings.Join(where, " AND ")
-	}
-
-	joinSQL := "FROM " + emailsSQL + " INNER JOIN `user` ON `user`.id = emails.uid " + whereStr
-
-	count, err := x.SQL("SELECT count(*) "+joinSQL, args...).Count()
 	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
+	err = x.Table("email_address").
-
+		Select("email_address.*, `user`.name, `user`.full_name").
-	for rows.Next() {
+		Join("INNER", "`user`", "`user`.ID = email_address.uid").
-		var email SearchEmailResult
+		Where(cond).
-		if err := rows.Scan(&email); err != nil {
+		OrderBy(orderby).
-			return nil, 0, err
+		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
-		}
+		Find(&emails)
-		if skip > 0 {
-			skip--
-			continue
-		}
-		emails = append(emails, &email)
-		if len(emails) == opts.PageSize {
-			break
-		}
-	}
 
 	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()
 }

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),
+		UID:        int64(1),
-		ID:    int64(1),
+		ID:         int64(33),
-		Email: "user11@example.com",
+		Email:      "user1-2@example.com",
+		LowerEmail: "user1-2@example.com",
 	}))
 
 	assert.NoError(t, DeleteEmailAddress(&EmailAddress{
-		UID:   int64(1),
+		UID:        int64(1),
-		Email: "user12@example.com",
+		Email:      "user1-3@example.com",
+		LowerEmail: "user1-3@example.com",
 	}))
 
 	// Email address does not exist
 	err := DeleteEmailAddress(&EmailAddress{
-		UID:   int64(1),
+		UID:        int64(1),
-		Email: "user1234567890@example.com",
+		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),
+		UID:        int64(2),
-		ID:    int64(3),
+		ID:         int64(3),
-		Email: "user2@example.com",
+		Email:      "user2@example.com",
+		LowerEmail: "user2@example.com",
 	}
 	emails[1] = &EmailAddress{
-		UID:   int64(2),
+		UID:        int64(2),
-		Email: "user21@example.com",
+		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)