Make external_id primary key of account_external_ids table

Having the primary key be (account_id, external_id) tuple was wrong,
we required exactly one match from an account_id in order to login a
user to their account.  If multiple matches were found we failed with
an exception and denied access.

When we move to a git based backend we really need external_id to be
the proper primary key of this entity, so we should fix it now before
more duplicates show up in real world databases.

Change-Id: Idd44dd67574fedb48e3e0bbd43526e1e67392dfc
Signed-off-by: Shawn O. Pearce <sop@google.com>
Reviewed-by: Grzegorz Kossakowski <grek@google.com>

src/main/java/com/google/gerrit/client/reviewdb/AccountExternalId.java

@@ -26,29 +26,19 @@ public final class AccountExternalId {
   public static final String SCHEME_MAILTO = "mailto:";
   public static final String LEGACY_GAE = "Google Account ";
 
-  public static class Key extends StringKey<Account.Id> {
+  public static class Key extends StringKey<com.google.gwtorm.client.Key<?>> {
     private static final long serialVersionUID = 1L;
 
-    @Column
-    protected Account.Id accountId;
-
     @Column
     protected String externalId;
 
     protected Key() {
-      accountId = new Account.Id();
     }
 
-    public Key(final Account.Id a, final String e) {
+    public Key(final String e) {
-      accountId = a;
       externalId = e;
     }
 
-    @Override
-    public Account.Id getParentKey() {
-      return accountId;
-    }
-
     @Override
     public String get() {
       return externalId;
@@ -94,6 +84,9 @@ public final class AccountExternalId {
   @Column(name = Column.NONE)
   protected Key key;
 
+  @Column
+  protected Account.Id accountId;
+
   @Column(notNull = false)
   protected String emailAddress;
 
@@ -109,9 +102,11 @@ public final class AccountExternalId {
   /**
    * Create a new binding to an external identity.
    *
+   * @param who the account this binds to.
    * @param k the binding key.
    */
-  public AccountExternalId(final AccountExternalId.Key k) {
+  public AccountExternalId(final Account.Id who, final AccountExternalId.Key k) {
+    accountId = who;
     key = k;
   }
 
@@ -121,7 +116,7 @@ public final class AccountExternalId {
 
   /** Get local id of this account, to link with in other entities */
   public Account.Id getAccountId() {
-    return key.accountId;
+    return accountId;
   }
 
   public String getExternalId() {

src/main/java/com/google/gerrit/client/reviewdb/AccountExternalIdAccess.java

@@ -25,13 +25,10 @@ public interface AccountExternalIdAccess extends
   @PrimaryKey("key")
   AccountExternalId get(AccountExternalId.Key key) throws OrmException;
 
-  @Query("WHERE key.externalId = ? LIMIT 2")
+  @Query("WHERE accountId = ?")
-  ResultSet<AccountExternalId> byExternal(String id) throws OrmException;
-
-  @Query("WHERE key.accountId = ?")
   ResultSet<AccountExternalId> byAccount(Account.Id id) throws OrmException;
 
-  @Query("WHERE key.accountId = ? AND emailAddress = ?")
+  @Query("WHERE accountId = ? AND emailAddress = ?")
   ResultSet<AccountExternalId> byAccountEmail(Account.Id id, String email)
       throws OrmException;
 

src/main/java/com/google/gerrit/server/account/AccountManager.java

@@ -56,9 +56,8 @@ public class AccountManager {
     try {
       final ReviewDb db = schema.open();
       try {
-        final List<AccountExternalId> matches =
+        return db.accountExternalIds().get(
-            db.accountExternalIds().byExternal(externalId).toList();
+            new AccountExternalId.Key(externalId)) != null;
-        return !matches.isEmpty();
       } finally {
         db.close();
       }
@@ -80,33 +79,19 @@ public class AccountManager {
     try {
       final ReviewDb db = schema.open();
       try {
-        final List<AccountExternalId> matches =
+        final AccountExternalId id =
-            db.accountExternalIds().byExternal(who.getExternalId()).toList();
+            db.accountExternalIds().get(
-        switch (matches.size()) {
+                new AccountExternalId.Key(who.getExternalId()));
-          case 0:
+        if (id == null) {
-            // New account, automatically create and return.
+          // New account, automatically create and return.
-            //
+          //
-            return create(db, who);
+          return create(db, who);
 
-          case 1: {
+        } else {
-            // Account exists, return the identity to the caller.
+          // Account exists, return the identity to the caller.
-            //
+          //
-            final AccountExternalId id = matches.get(0);
+          update(db, who, id);
-            update(db, who, id);
+          return new AuthResult(id.getAccountId(), false);
-            return new AuthResult(id.getAccountId(), false);
-          }
-
-          default: {
-            final StringBuilder r = new StringBuilder();
-            r.append("Multiple accounts match \"");
-            r.append(who.getExternalId());
-            r.append("\":");
-            for (AccountExternalId e : matches) {
-              r.append(' ');
-              r.append(e.getAccountId());
-            }
-            throw new AccountException(r.toString());
-          }
         }
 
       } finally {
@@ -270,7 +255,7 @@ public class AccountManager {
   private static AccountExternalId createId(final Account.Id newId,
       final AuthRequest who) {
     final String ext = who.getExternalId();
-    return new AccountExternalId(new AccountExternalId.Key(newId, ext));
+    return new AccountExternalId(newId, new AccountExternalId.Key(ext));
   }
 
   /**
@@ -286,32 +271,23 @@ public class AccountManager {
     try {
       final ReviewDb db = schema.open();
       try {
-        final List<AccountExternalId> matches =
+        AccountExternalId extId =
-            db.accountExternalIds().byExternal(who.getExternalId()).toList();
+            db.accountExternalIds().get(
-        switch (matches.size()) {
+                new AccountExternalId.Key(who.getExternalId()));
-          case 0: {
+        if (extId != null) {
-            final AccountExternalId extId = createId(to, who);
+          if (!extId.getAccountId().equals(to)) {
-            extId.setEmailAddress(who.getEmailAddress());
+            throw new AccountException("Identity in use by another account");
-            extId.setLastUsedOn();
-            db.accountExternalIds().insert(Collections.singleton(extId));
-            if (who.getEmailAddress() != null) {
-              byEmailCache.evict(who.getEmailAddress());
-              byIdCache.evict(to);
-            }
-            break;
           }
-
+          update(db, who, extId);
-          case 1: {
+        } else {
-            final AccountExternalId extId = matches.get(0);
+          extId = createId(to, who);
-            if (!extId.getAccountId().equals(to)) {
+          extId.setEmailAddress(who.getEmailAddress());
-              throw new AccountException("Identity already used");
+          extId.setLastUsedOn();
-            }
+          db.accountExternalIds().insert(Collections.singleton(extId));
-            update(db, who, extId);
+          if (who.getEmailAddress() != null) {
-            break;
+            byEmailCache.evict(who.getEmailAddress());
+            byIdCache.evict(to);
           }
-
-          default:
-            throw new AccountException("Identity already used");
         }
 
       } finally {

src/main/java/com/google/gerrit/server/ldap/LdapRealm.java

@@ -442,13 +442,10 @@ class LdapRealm implements Realm {
     try {
       final ReviewDb db = schema.open();
       try {
-        final List<AccountExternalId> candidates =
+        final String id = AccountExternalId.SCHEME_GERRIT + username;
-            db.accountExternalIds().byExternal(
+        final AccountExternalId extId =
-                AccountExternalId.SCHEME_GERRIT + username).toList();
+            db.accountExternalIds().get(new AccountExternalId.Key(id));
-        if (candidates.size() == 1) {
+        return extId != null ? extId.getAccountId() : null;
-          return candidates.get(0).getAccountId();
-        }
-        return null;
       } finally {
         db.close();
       }

src/main/java/com/google/gerrit/server/rpc/ChangeListServiceImpl.java

@@ -450,7 +450,7 @@ public class ChangeListServiceImpl extends BaseServiceImplementation implements
     addAll(result, db.accounts().suggestByFullName(a, b, 10));
     for (AccountExternalId extId : db.accountExternalIds()
         .suggestByEmailAddress(a, b, 10)) {
-      result.add(extId.getKey().getParentKey());
+      result.add(extId.getAccountId());
     }
     return result;
   }

src/main/java/com/google/gerrit/server/rpc/account/AccountSecurityImpl.java

@@ -234,19 +234,20 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
     run(callback, new Action<Set<AccountExternalId.Key>>() {
       public Set<AccountExternalId.Key> run(final ReviewDb db)
           throws OrmException, Failure {
-        // Don't permit deletes unless they are for our own account
-        //
-        final Account.Id me = getAccountId();
-        for (final AccountExternalId.Key keyId : keys) {
-          if (!me.equals(keyId.getParentKey()))
-            throw new Failure(new NoSuchEntityException());
-        }
-
         // Determine the records we will allow the user to remove.
         //
+        final Account.Id me = getAccountId();
         final Map<AccountExternalId.Key, AccountExternalId> all =
             db.accountExternalIds()
                 .toMap(db.accountExternalIds().byAccount(me));
+
+        // Don't permit deletes unless they are for our own account
+        //
+        for (final AccountExternalId.Key keyId : keys) {
+          if (!all.containsKey(keyId))
+            throw new Failure(new NoSuchEntityException());
+        }
+
         final AccountExternalId mostRecent =
             AccountExternalId.mostRecent(all.values());
         final Set<AccountExternalId.Key> removed =

src/main/webapp/WEB-INF/sql/index_generic.sql

@@ -22,10 +22,9 @@ ON accounts (full_name);
 
 -- *********************************************************************
 -- AccountExternalIdAccess
---    @PrimaryKey covers: byAccount
+--    covers:             byAccount
---    covers:             byExternal
+CREATE INDEX account_external_ids_byAccount
-CREATE INDEX account_external_ids_byExt
+ON account_external_ids (account_id);
-ON account_external_ids (external_id);
 
 --    covers:             byEmailAddress, suggestByEmailAddress
 CREATE INDEX account_external_ids_byEmail

src/main/webapp/WEB-INF/sql/index_postgres.sql

@@ -57,10 +57,9 @@ ON accounts (full_name);
 
 -- *********************************************************************
 -- AccountExternalIdAccess
---    @PrimaryKey covers: byAccount
+--    covers:             byAccount
---    covers:             byExternal
+CREATE INDEX account_external_ids_byAccount
-CREATE INDEX account_external_ids_byExt
+ON account_external_ids (account_id);
-ON account_external_ids (external_id);
 
 --    covers:             byEmailAddress, suggestByEmailAddress
 CREATE INDEX account_external_ids_byEmail

src/main/webapp/WEB-INF/sql/upgrade017_018_mysql.sql

@@ -28,6 +28,18 @@ AND NOT EXISTS (SELECT 1 FROM copy_patch_comments1 r
 DROP TEMPORARY TABLE copy_patch_comments1;
 DROP TEMPORARY TABLE copy_patch_comments2;
 
+
+-- account_external_ids
+--
+DROP INDEX account_external_ids_byExt;
+
+CREATE INDEX account_external_ids_byAccount
+ON account_external_ids (account_id);
+
+ALTER TABLE account_external_ids DROP PRIMARY KEY;
+ALTER TABLE account_external_ids ADD PRIMARY KEY (external_id);
+
+
 DROP TABLE patches;
 
 UPDATE schema_version SET version_nbr = 18;

src/main/webapp/WEB-INF/sql/upgrade017_018_postgres.sql

@@ -27,6 +27,18 @@ AND NOT EXISTS (SELECT 1 FROM patch_comments p
                 AND p.file_name = patch_comments.file_name
                 AND p.uuid = patch_comments.parent_uuid);
 
+
+-- account_external_ids
+--
+DROP INDEX account_external_ids_byExt;
+
+CREATE INDEX account_external_ids_byAccount
+ON account_external_ids (account_id);
+
+ALTER TABLE account_external_ids DROP CONSTRAINT account_external_ids_pkey;
+ALTER TABLE account_external_ids ADD PRIMARY KEY (external_id);
+
+
 DROP TABLE patches;
 
 UPDATE schema_version SET version_nbr = 18;