Fix GetUsersByEmails (#34643)

models/fixtures/email_address.yml

@@ -81,7 +81,7 @@
 -
   id: 11
   uid: 4
-  email: user4@example.com
+  email: User4@Example.Com
   lower_email: user4@example.com
   is_activated: true
   is_primary: true

models/user/user.go

@@ -1151,8 +1151,8 @@ func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) ([
 	}
 
 	for _, c := range oldCommits {
-		user, ok := emailUserMap[c.Author.Email]
+		user := emailUserMap.GetByEmail(c.Author.Email) // FIXME: why ValidateCommitsWithEmails uses "Author", but ParseCommitsWithSignature uses "Committer"?
-		if !ok {
+		if user == nil {
 			user = &User{
 				Name:  c.Author.Name,
 				Email: c.Author.Email,
@@ -1166,7 +1166,15 @@ func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) ([
 	return newCommits, nil
 }
 
-func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, error) {
+type EmailUserMap struct {
+	m map[string]*User
+}
+
+func (eum *EmailUserMap) GetByEmail(email string) *User {
+	return eum.m[strings.ToLower(email)]
+}
+
+func GetUsersByEmails(ctx context.Context, emails []string) (*EmailUserMap, error) {
 	if len(emails) == 0 {
 		return nil, nil
 	}
@@ -1176,7 +1184,7 @@ func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, e
 	for _, email := range emails {
 		if strings.HasSuffix(email, "@"+setting.Service.NoReplyAddress) {
 			username := strings.TrimSuffix(email, "@"+setting.Service.NoReplyAddress)
-			needCheckUserNames.Add(username)
+			needCheckUserNames.Add(strings.ToLower(username))
 		} else {
 			needCheckEmails.Add(strings.ToLower(email))
 		}
@@ -1203,8 +1211,7 @@ func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, e
 		for _, email := range emailAddresses {
 			user := users[email.UID]
 			if user != nil {
-				results[user.Email] = user
+				results[email.LowerEmail] = user
-				results[user.GetPlaceholderEmail()] = user
 			}
 		}
 	}
@@ -1214,10 +1221,9 @@ func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, e
 		return nil, err
 	}
 	for _, user := range users {
-		results[user.Email] = user
+		results[strings.ToLower(user.GetPlaceholderEmail())] = user
-		results[user.GetPlaceholderEmail()] = user
 	}
-	return results, nil
+	return &EmailUserMap{results}, nil
 }
 
 // GetUserByEmail returns the user object by given e-mail if exists.

models/user/user_test.go

@@ -58,13 +58,33 @@ func TestUserEmails(t *testing.T) {
 		assert.ElementsMatch(t, []string{"user8@example.com"}, user_model.GetUserEmailsByNames(db.DefaultContext, []string{"user8", "org7"}))
 	})
 	t.Run("GetUsersByEmails", func(t *testing.T) {
-		m, err := user_model.GetUsersByEmails(db.DefaultContext, []string{"user1@example.com", "user2@" + setting.Service.NoReplyAddress})
+		defer test.MockVariableValue(&setting.Service.NoReplyAddress, "NoReply.gitea.internal")()
-		require.NoError(t, err)
+		testGetUserByEmail := func(t *testing.T, email string, uid int64) {
-		require.Len(t, m, 4)
+			m, err := user_model.GetUsersByEmails(db.DefaultContext, []string{email})
-		assert.EqualValues(t, 1, m["user1@example.com"].ID)
+			require.NoError(t, err)
-		assert.EqualValues(t, 1, m["user1@"+setting.Service.NoReplyAddress].ID)
+			user := m.GetByEmail(email)
-		assert.EqualValues(t, 2, m["user2@example.com"].ID)
+			if uid == 0 {
-		assert.EqualValues(t, 2, m["user2@"+setting.Service.NoReplyAddress].ID)
+				require.Nil(t, user)
+				return
+			}
+			require.NotNil(t, user)
+			assert.Equal(t, uid, user.ID)
+		}
+		cases := []struct {
+			Email string
+			UID   int64
+		}{
+			{"UseR1@example.com", 1},
+			{"user1-2@example.COM", 1},
+			{"USER2@" + setting.Service.NoReplyAddress, 2},
+			{"user4@example.com", 4},
+			{"no-such", 0},
+		}
+		for _, c := range cases {
+			t.Run(c.Email, func(t *testing.T) {
+				testGetUserByEmail(t, c.Email, c.UID)
+			})
+		}
 	})
 }
 

services/git/commit.go

@@ -34,9 +34,9 @@ func ParseCommitsWithSignature(ctx context.Context, repo *repo_model.Repository,
 	}
 
 	for _, c := range oldCommits {
-		committer, ok := emailUsers[c.Committer.Email]
+		committerUser := emailUsers.GetByEmail(c.Committer.Email) // FIXME: why ValidateCommitsWithEmails uses "Author", but ParseCommitsWithSignature uses "Committer"?
-		if !ok && c.Committer != nil {
+		if committerUser == nil {
-			committer = &user_model.User{
+			committerUser = &user_model.User{
 				Name:  c.Committer.Name,
 				Email: c.Committer.Email,
 			}
@@ -44,7 +44,7 @@ func ParseCommitsWithSignature(ctx context.Context, repo *repo_model.Repository,
 
 		signCommit := &asymkey_model.SignCommit{
 			UserCommit:   c,
-			Verification: asymkey_service.ParseCommitWithSignatureCommitter(ctx, c.Commit, committer),
+			Verification: asymkey_service.ParseCommitWithSignatureCommitter(ctx, c.Commit, committerUser),
 		}
 
 		isOwnerMemberCollaborator := func(user *user_model.User) (bool, error) {