From 70cf025bc42a01b39dd024aab2e2ee9ae36160f3 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 Sep 2009 16:53:00 -0700 Subject: [PATCH] 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 Reviewed-by: Grzegorz Kossakowski --- .../client/reviewdb/AccountExternalId.java | 23 ++--- .../reviewdb/AccountExternalIdAccess.java | 7 +- .../gerrit/server/account/AccountManager.java | 84 +++++++------------ .../google/gerrit/server/ldap/LdapRealm.java | 11 +-- .../server/rpc/ChangeListServiceImpl.java | 2 +- .../rpc/account/AccountSecurityImpl.java | 17 ++-- src/main/webapp/WEB-INF/sql/index_generic.sql | 7 +- .../webapp/WEB-INF/sql/index_postgres.sql | 7 +- .../WEB-INF/sql/upgrade017_018_mysql.sql | 12 +++ .../WEB-INF/sql/upgrade017_018_postgres.sql | 12 +++ 10 files changed, 85 insertions(+), 97 deletions(-) diff --git a/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalId.java b/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalId.java index a68bcd735f..c7ad3e0a84 100644 --- a/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalId.java +++ b/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 { + public static class Key extends StringKey> { 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) { - accountId = a; + public Key(final String e) { 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() { diff --git a/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalIdAccess.java b/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalIdAccess.java index a6ac5ce667..4b65724e02 100644 --- a/src/main/java/com/google/gerrit/client/reviewdb/AccountExternalIdAccess.java +++ b/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") - ResultSet byExternal(String id) throws OrmException; - - @Query("WHERE key.accountId = ?") + @Query("WHERE accountId = ?") ResultSet byAccount(Account.Id id) throws OrmException; - @Query("WHERE key.accountId = ? AND emailAddress = ?") + @Query("WHERE accountId = ? AND emailAddress = ?") ResultSet byAccountEmail(Account.Id id, String email) throws OrmException; diff --git a/src/main/java/com/google/gerrit/server/account/AccountManager.java b/src/main/java/com/google/gerrit/server/account/AccountManager.java index 0e957a24bd..8461c538b9 100644 --- a/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/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 matches = - db.accountExternalIds().byExternal(externalId).toList(); - return !matches.isEmpty(); + return db.accountExternalIds().get( + new AccountExternalId.Key(externalId)) != null; } finally { db.close(); } @@ -80,33 +79,19 @@ public class AccountManager { try { final ReviewDb db = schema.open(); try { - final List matches = - db.accountExternalIds().byExternal(who.getExternalId()).toList(); - switch (matches.size()) { - case 0: - // New account, automatically create and return. - // - return create(db, who); + final AccountExternalId id = + db.accountExternalIds().get( + new AccountExternalId.Key(who.getExternalId())); + if (id == null) { + // New account, automatically create and return. + // + return create(db, who); - case 1: { - // Account exists, return the identity to the caller. - // - final AccountExternalId id = matches.get(0); - update(db, who, id); - 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()); - } + } else { + // Account exists, return the identity to the caller. + // + update(db, who, id); + return new AuthResult(id.getAccountId(), false); } } 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 matches = - db.accountExternalIds().byExternal(who.getExternalId()).toList(); - switch (matches.size()) { - case 0: { - final AccountExternalId extId = createId(to, who); - extId.setEmailAddress(who.getEmailAddress()); - extId.setLastUsedOn(); - db.accountExternalIds().insert(Collections.singleton(extId)); - if (who.getEmailAddress() != null) { - byEmailCache.evict(who.getEmailAddress()); - byIdCache.evict(to); - } - break; + AccountExternalId extId = + db.accountExternalIds().get( + new AccountExternalId.Key(who.getExternalId())); + if (extId != null) { + if (!extId.getAccountId().equals(to)) { + throw new AccountException("Identity in use by another account"); } - - case 1: { - final AccountExternalId extId = matches.get(0); - if (!extId.getAccountId().equals(to)) { - throw new AccountException("Identity already used"); - } - update(db, who, extId); - break; + update(db, who, extId); + } else { + extId = createId(to, who); + extId.setEmailAddress(who.getEmailAddress()); + extId.setLastUsedOn(); + db.accountExternalIds().insert(Collections.singleton(extId)); + if (who.getEmailAddress() != null) { + byEmailCache.evict(who.getEmailAddress()); + byIdCache.evict(to); } - - default: - throw new AccountException("Identity already used"); } } finally { diff --git a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java b/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java index a6fbf851b7..cb992f764e 100644 --- a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java +++ b/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 candidates = - db.accountExternalIds().byExternal( - AccountExternalId.SCHEME_GERRIT + username).toList(); - if (candidates.size() == 1) { - return candidates.get(0).getAccountId(); - } - return null; + final String id = AccountExternalId.SCHEME_GERRIT + username; + final AccountExternalId extId = + db.accountExternalIds().get(new AccountExternalId.Key(id)); + return extId != null ? extId.getAccountId() : null; } finally { db.close(); } diff --git a/src/main/java/com/google/gerrit/server/rpc/ChangeListServiceImpl.java b/src/main/java/com/google/gerrit/server/rpc/ChangeListServiceImpl.java index c107b42303..fe5c30d9d8 100644 --- a/src/main/java/com/google/gerrit/server/rpc/ChangeListServiceImpl.java +++ b/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; } diff --git a/src/main/java/com/google/gerrit/server/rpc/account/AccountSecurityImpl.java b/src/main/java/com/google/gerrit/server/rpc/account/AccountSecurityImpl.java index d9f4db8d93..87a7f9d4ac 100644 --- a/src/main/java/com/google/gerrit/server/rpc/account/AccountSecurityImpl.java +++ b/src/main/java/com/google/gerrit/server/rpc/account/AccountSecurityImpl.java @@ -234,19 +234,20 @@ class AccountSecurityImpl extends BaseServiceImplementation implements run(callback, new Action>() { public Set 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 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 removed = diff --git a/src/main/webapp/WEB-INF/sql/index_generic.sql b/src/main/webapp/WEB-INF/sql/index_generic.sql index 1cf13de2f2..cfdb23681e 100644 --- a/src/main/webapp/WEB-INF/sql/index_generic.sql +++ b/src/main/webapp/WEB-INF/sql/index_generic.sql @@ -22,10 +22,9 @@ ON accounts (full_name); -- ********************************************************************* -- AccountExternalIdAccess --- @PrimaryKey covers: byAccount --- covers: byExternal -CREATE INDEX account_external_ids_byExt -ON account_external_ids (external_id); +-- covers: byAccount +CREATE INDEX account_external_ids_byAccount +ON account_external_ids (account_id); -- covers: byEmailAddress, suggestByEmailAddress CREATE INDEX account_external_ids_byEmail diff --git a/src/main/webapp/WEB-INF/sql/index_postgres.sql b/src/main/webapp/WEB-INF/sql/index_postgres.sql index 7fc8b219b1..b007a3b246 100644 --- a/src/main/webapp/WEB-INF/sql/index_postgres.sql +++ b/src/main/webapp/WEB-INF/sql/index_postgres.sql @@ -57,10 +57,9 @@ ON accounts (full_name); -- ********************************************************************* -- AccountExternalIdAccess --- @PrimaryKey covers: byAccount --- covers: byExternal -CREATE INDEX account_external_ids_byExt -ON account_external_ids (external_id); +-- covers: byAccount +CREATE INDEX account_external_ids_byAccount +ON account_external_ids (account_id); -- covers: byEmailAddress, suggestByEmailAddress CREATE INDEX account_external_ids_byEmail diff --git a/src/main/webapp/WEB-INF/sql/upgrade017_018_mysql.sql b/src/main/webapp/WEB-INF/sql/upgrade017_018_mysql.sql index 3278a3b4ef..11d1d79a63 100644 --- a/src/main/webapp/WEB-INF/sql/upgrade017_018_mysql.sql +++ b/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; diff --git a/src/main/webapp/WEB-INF/sql/upgrade017_018_postgres.sql b/src/main/webapp/WEB-INF/sql/upgrade017_018_postgres.sql index 63cd16f7f9..04cf4c3292 100644 --- a/src/main/webapp/WEB-INF/sql/upgrade017_018_postgres.sql +++ b/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;