Also find accounts by preferred email

Historically (before change I2b3c5c9df) Gerrit was able to find accounts
by preferred email, even when the account had no external ID with that
email.

It turns out that the Gerrit integration with Google Accounts that is
used for Gerrit installations at Google depends on this feature. Each
Google Account can be associated with multiple email addresses, and one
is designated as "primary" email address. Users are intentionally
allowed to choose any of the email addresses that are associated with
their account as preferred email, not just the "primary" email address.
Because an ExternalId can store only a single email only the "primary"
email is part of the external ID and the secondary email addresses are
not stored in Gerrit's database. If a user chooses such a secondary
email address a preferred email we must ensure that its account is found
when a lookup by this secondary email is made. Hence we reenable the old
Gerrit behavior to also look for accounts with matching preferred email
when looking up accounts by email.

Change I757a4d065b added a new API to lookup accounts by email. Change
the implementation of these method to also find accounts by preferred
email.

To find accounts by preferred email use the the account index. To
prevent a circular dependency between the Accounts class and the
AccountCacheImpl.ByIdLoader class when using InternalAccountQuery the
methods to lookup accounts by email are moved to an own class.

As follow-up change I991d21b1ac adapts all Gerrit code to use the new
API for looking up accounts by email, so that the AccountByEmailCache
can be removed by change I3a4279f5ab.

Not being able to find (inconsistent) accounts by preferred email
currently blocks the migration of accounts to NoteDb.

Change-Id: I1c24da13786f81d6e59b0784586f0b53a16b0a28
Signed-off-by: Edwin Kempin <ekempin@google.com>

gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java

@@ -14,18 +14,13 @@
 
 package com.google.gerrit.server.account;
 
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
 import static java.util.Comparator.comparing;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -37,7 +32,6 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Stream;
@@ -56,20 +50,17 @@ public class Accounts {
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
   private final OutgoingEmailValidator emailValidator;
-  private final ExternalIds externalIds;
 
   @Inject
   Accounts(
       @GerritServerConfig Config cfg,
       GitRepositoryManager repoManager,
       AllUsersName allUsersName,
-      OutgoingEmailValidator emailValidator,
-      ExternalIds externalIds) {
+      OutgoingEmailValidator emailValidator) {
     this.readFromGit = cfg.getBoolean("user", null, "readAccountsFromGit", false);
     this.repoManager = repoManager;
     this.allUsersName = allUsersName;
     this.emailValidator = emailValidator;
-    this.externalIds = externalIds;
   }
 
   public Account get(ReviewDb db, Account.Id accountId)
@@ -98,44 +89,6 @@ public class Accounts {
     return db.accounts().get(accountIds).toList();
   }
 
-  /**
-   * Returns the accounts with the given email.
-   *
-   * <p>Each email should belong to a single account only. This means if more than one account is
-   * returned there is an inconsistency in the external IDs.
-   *
-   * <p>The accounts are retrieved via the external ID cache. Each access to the external ID cache
-   * requires reading the SHA1 of the refs/meta/external-ids branch. If accounts for multiple emails
-   * are needed it is more efficient to use {@link #byEmails(String...)} as this method reads the
-   * SHA1 of the refs/meta/external-ids branch only once (and not once per email).
-   *
-   * @see #byEmails(String...)
-   */
-  public ImmutableSet<Account.Id> byEmail(String email) throws IOException {
-    return externalIds.byEmail(email).stream().map(e -> e.accountId()).collect(toImmutableSet());
-  }
-
-  /**
-   * Returns the accounts for the given emails.
-   *
-   * <p>Each email should belong to a single account only. This means if more than one account for
-   * an email is returned there is an inconsistency in the external IDs.
-   *
-   * <p>The accounts are retrieved via the external ID cache. Each access to the external ID cache
-   * requires reading the SHA1 of the refs/meta/external-ids branch. If accounts for multiple emails
-   * are needed it is more efficient to use this method instead of {@link #byEmail(String)} as this
-   * method reads the SHA1 of the refs/meta/external-ids branch only once (and not once per email).
-   *
-   * @see #byEmail(String)
-   */
-  public ImmutableSetMultimap<String, Account.Id> byEmails(String... emails) throws IOException {
-    return externalIds
-        .byEmails(emails)
-        .entries()
-        .stream()
-        .collect(toImmutableSetMultimap(Map.Entry::getKey, e -> e.getValue().accountId()));
-  }
-
   /**
    * Returns all accounts.
    *

gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 The Android Open Source Project
+// Copyright (C) 2017 The Android Open Source Project
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,80 +14,85 @@
 
 package com.google.gerrit.server.account;
 
-import com.google.common.base.Strings;
-import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.extensions.restapi.AcceptsCreate;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.ChildCollection;
-import com.google.gerrit.extensions.restapi.IdString;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
-import com.google.gerrit.extensions.restapi.RestView;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.account.AccountResource.Email;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.Singleton;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.Streams;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.account.externalids.ExternalIds;
+import com.google.gerrit.server.query.account.InternalAccountQuery;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.List;
+
+/** Class to access accounts by email. */
 @Singleton
-public class Emails
-    implements ChildCollection<AccountResource, AccountResource.Email>,
-        AcceptsCreate<AccountResource> {
-  private final DynamicMap<RestView<AccountResource.Email>> views;
-  private final GetEmails list;
-  private final Provider<CurrentUser> self;
-  private final PermissionBackend permissionBackend;
-  private final CreateEmail.Factory createEmailFactory;
+public class Emails {
+  private final ExternalIds externalIds;
+  private final InternalAccountQuery accountQuery;
 
   @Inject
-  Emails(
-      DynamicMap<RestView<AccountResource.Email>> views,
-      GetEmails list,
-      Provider<CurrentUser> self,
-      PermissionBackend permissionBackend,
-      CreateEmail.Factory createEmailFactory) {
-    this.views = views;
-    this.list = list;
-    this.self = self;
-    this.permissionBackend = permissionBackend;
-    this.createEmailFactory = createEmailFactory;
+  public Emails(ExternalIds externalIds, InternalAccountQuery accountQuery) {
+    this.externalIds = externalIds;
+    this.accountQuery = accountQuery;
   }
 
-  @Override
-  public RestView<AccountResource> list() {
-    return list;
+  /**
+   * Returns the accounts with the given email.
+   *
+   * <p>Each email should belong to a single account only. This means if more than one account is
+   * returned there is an inconsistency in the external IDs.
+   *
+   * <p>The accounts are retrieved via the external ID cache. Each access to the external ID cache
+   * requires reading the SHA1 of the refs/meta/external-ids branch. If accounts for multiple emails
+   * are needed it is more efficient to use {@link #getAccountsFor(String...)} as this method reads
+   * the SHA1 of the refs/meta/external-ids branch only once (and not once per email).
+   *
+   * <p>In addition accounts are included that have the given email as preferred email even if they
+   * have no external ID for the preferred email. Having accounts with a preferred email that does
+   * not exist as external ID is an inconsistency, but existing functionality relies on still
+   * getting those accounts, which is why they are included. Accounts by preferred email are fetched
+   * from the account index.
+   *
+   * @see #getAccountsFor(String...)
+   */
+  public ImmutableSet<Account.Id> getAccountFor(String email) throws IOException, OrmException {
+    List<AccountState> byPreferredEmail = accountQuery.byPreferredEmail(email);
+    return Streams.concat(
+            externalIds.byEmail(email).stream().map(e -> e.accountId()),
+            byPreferredEmail
+                .stream()
+                // the index query also matches prefixes and emails with other case,
+                // filter those out
+                .filter(a -> email.equals(a.getAccount().getPreferredEmail()))
+                .map(a -> a.getAccount().getId()))
+        .collect(toImmutableSet());
   }
 
-  @Override
-  public AccountResource.Email parse(AccountResource rsrc, IdString id)
-      throws ResourceNotFoundException, PermissionBackendException, AuthException {
-    if (self.get() != rsrc.getUser()) {
-      permissionBackend.user(self).check(GlobalPermission.ADMINISTRATE_SERVER);
-    }
-
-    if ("preferred".equals(id.get())) {
-      String email = rsrc.getUser().getAccount().getPreferredEmail();
-      if (Strings.isNullOrEmpty(email)) {
-        throw new ResourceNotFoundException(id);
-      }
-      return new AccountResource.Email(rsrc.getUser(), email);
-    } else if (rsrc.getUser().hasEmailAddress(id.get())) {
-      return new AccountResource.Email(rsrc.getUser(), id.get());
-    } else {
-      throw new ResourceNotFoundException(id);
-    }
-  }
-
-  @Override
-  public DynamicMap<RestView<Email>> views() {
-    return views;
-  }
-
-  @SuppressWarnings("unchecked")
-  @Override
-  public CreateEmail create(AccountResource parent, IdString email) {
-    return createEmailFactory.create(email.get());
+  /**
+   * Returns the accounts for the given emails.
+   *
+   * @see #getAccountFor(String)
+   */
+  public ImmutableSetMultimap<String, Account.Id> getAccountsFor(String... emails)
+      throws IOException, OrmException {
+    ImmutableSetMultimap.Builder<String, Account.Id> builder = ImmutableSetMultimap.builder();
+    externalIds
+        .byEmails(emails)
+        .entries()
+        .stream()
+        .forEach(e -> builder.put(e.getKey(), e.getValue().accountId()));
+    accountQuery
+        .byPreferredEmail(emails)
+        .entries()
+        .stream()
+        // the index query also matches prefixes and emails with other case,
+        // filter those out
+        .filter(e -> e.getKey().equals(e.getValue().getAccount().getPreferredEmail()))
+        .forEach(e -> builder.put(e.getKey(), e.getValue().getAccount().getId()));
+    return builder.build();
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/account/EmailsCollection.java

@@ -0,0 +1,93 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.common.base.Strings;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.restapi.AcceptsCreate;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ChildCollection;
+import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.account.AccountResource.Email;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+@Singleton
+public class EmailsCollection
+    implements ChildCollection<AccountResource, AccountResource.Email>,
+        AcceptsCreate<AccountResource> {
+  private final DynamicMap<RestView<AccountResource.Email>> views;
+  private final GetEmails list;
+  private final Provider<CurrentUser> self;
+  private final PermissionBackend permissionBackend;
+  private final CreateEmail.Factory createEmailFactory;
+
+  @Inject
+  EmailsCollection(
+      DynamicMap<RestView<AccountResource.Email>> views,
+      GetEmails list,
+      Provider<CurrentUser> self,
+      PermissionBackend permissionBackend,
+      CreateEmail.Factory createEmailFactory) {
+    this.views = views;
+    this.list = list;
+    this.self = self;
+    this.permissionBackend = permissionBackend;
+    this.createEmailFactory = createEmailFactory;
+  }
+
+  @Override
+  public RestView<AccountResource> list() {
+    return list;
+  }
+
+  @Override
+  public AccountResource.Email parse(AccountResource rsrc, IdString id)
+      throws ResourceNotFoundException, PermissionBackendException, AuthException {
+    if (self.get() != rsrc.getUser()) {
+      permissionBackend.user(self).check(GlobalPermission.ADMINISTRATE_SERVER);
+    }
+
+    if ("preferred".equals(id.get())) {
+      String email = rsrc.getUser().getAccount().getPreferredEmail();
+      if (Strings.isNullOrEmpty(email)) {
+        throw new ResourceNotFoundException(id);
+      }
+      return new AccountResource.Email(rsrc.getUser(), email);
+    } else if (rsrc.getUser().hasEmailAddress(id.get())) {
+      return new AccountResource.Email(rsrc.getUser(), id.get());
+    } else {
+      throw new ResourceNotFoundException(id);
+    }
+  }
+
+  @Override
+  public DynamicMap<RestView<Email>> views() {
+    return views;
+  }
+
+  @SuppressWarnings("unchecked")
+  @Override
+  public CreateEmail create(AccountResource parent, IdString email) {
+    return createEmailFactory.create(email.get());
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/account/Module.java

@@ -51,7 +51,7 @@ public class Module extends RestApiModule {
     get(ACCOUNT_KIND, "active").to(GetActive.class);
     put(ACCOUNT_KIND, "active").to(PutActive.class);
     delete(ACCOUNT_KIND, "active").to(DeleteActive.class);
-    child(ACCOUNT_KIND, "emails").to(Emails.class);
+    child(ACCOUNT_KIND, "emails").to(EmailsCollection.class);
     get(EMAIL_KIND).to(GetEmail.class);
     put(EMAIL_KIND).to(PutEmail.class);
     delete(EMAIL_KIND).to(DeleteEmail.class);