Handle duplicate external ids when in migration mode
Before this change external ids cache keep case insensitive sha1 even if
external id is not migrated yet. This cause issue with duplicate cache
entries when loading a cache for external ids which differs only on user
name case sensitivity. To solve that issue keep case sensitive sha1 when
in migration mode. Also when getting external id from cache first check
for case sensitive sha1 to make sure to match correct account and
fallback to case insensitive sha1 to look for already migrated external
ids.
Bug: Issue 15328
Change-Id: I694de6ef43d2f2246b9535612e2f7c1db7d5e707
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdFactory.java b/java/com/google/gerrit/server/account/externalids/ExternalIdFactory.java
index fa68cad..bd9c7df 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdFactory.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdFactory.java
@@ -267,6 +267,8 @@
"Neither case sensitive nor case insensitive SHA1 of external ID '%s' match note ID '%s'",
externalIdKeyStr, noteId));
}
+ externalIdKey =
+ externalIdKeyFactory.create(externalIdKey.scheme(), externalIdKey.id(), false);
}
String email =
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIds.java b/java/com/google/gerrit/server/account/externalids/ExternalIds.java
index 4e1e524..9450ff5 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIds.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIds.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.server.config.AuthConfig;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -35,11 +36,19 @@
public class ExternalIds {
private final ExternalIdReader externalIdReader;
private final ExternalIdCache externalIdCache;
+ private final AuthConfig authConfig;
+ private final ExternalIdKeyFactory externalIdKeyFactory;
@Inject
- public ExternalIds(ExternalIdReader externalIdReader, ExternalIdCache externalIdCache) {
+ public ExternalIds(
+ ExternalIdReader externalIdReader,
+ ExternalIdCache externalIdCache,
+ ExternalIdKeyFactory externalIdKeyFactory,
+ AuthConfig authConfig) {
this.externalIdReader = externalIdReader;
this.externalIdCache = externalIdCache;
+ this.externalIdKeyFactory = externalIdKeyFactory;
+ this.authConfig = authConfig;
}
/** Returns all external IDs. */
@@ -54,7 +63,15 @@
/** Returns the specified external ID. */
public Optional<ExternalId> get(ExternalId.Key key) throws IOException {
- return externalIdCache.byKey(key);
+ Optional<ExternalId> externalId = Optional.empty();
+ if (authConfig.isUserNameCaseInsensitiveMigrationMode()) {
+ externalId =
+ externalIdCache.byKey(externalIdKeyFactory.create(key.scheme(), key.id(), false));
+ }
+ if (!externalId.isPresent()) {
+ externalId = externalIdCache.byKey(key);
+ }
+ return externalId;
}
/** Returns the specified external ID from the given revision. */
diff --git a/java/com/google/gerrit/server/account/externalids/PasswordVerifier.java b/java/com/google/gerrit/server/account/externalids/PasswordVerifier.java
index 33443c1..eb2bea9 100644
--- a/java/com/google/gerrit/server/account/externalids/PasswordVerifier.java
+++ b/java/com/google/gerrit/server/account/externalids/PasswordVerifier.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.account.HashedPassword;
+import com.google.gerrit.server.config.AuthConfig;
import com.google.inject.Inject;
import java.util.Collection;
@@ -29,9 +30,12 @@
private final ExternalIdKeyFactory externalIdKeyFactory;
+ private AuthConfig authConfig;
+
@Inject
- public PasswordVerifier(ExternalIdKeyFactory externalIdKeyFactory) {
+ public PasswordVerifier(ExternalIdKeyFactory externalIdKeyFactory, AuthConfig authConfig) {
this.externalIdKeyFactory = externalIdKeyFactory;
+ this.authConfig = authConfig;
}
/** Returns {@code true} if there is an external ID matching both the username and password. */
@@ -40,13 +44,23 @@
if (password == null) {
return false;
}
+
for (ExternalId id : externalIds) {
// Only process the "username:$USER" entry, which is unique.
- if (!id.isScheme(SCHEME_USERNAME)
- || !id.key().equals(externalIdKeyFactory.create(SCHEME_USERNAME, username))) {
+ if (!id.isScheme(SCHEME_USERNAME)) {
continue;
}
+ if (!id.key().equals(externalIdKeyFactory.create(SCHEME_USERNAME, username))) {
+ if (!authConfig.isUserNameCaseInsensitiveMigrationMode()) {
+ continue;
+ }
+
+ if (!id.key().equals(externalIdKeyFactory.create(SCHEME_USERNAME, username, false))) {
+ continue;
+ }
+ }
+
String hashedStr = id.password();
if (!Strings.isNullOrEmpty(hashedStr)) {
try {
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
index 7a76e09..1e89a85 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
@@ -94,6 +94,8 @@
public class ExternalIdIT extends AbstractDaemonTest {
private static final boolean IS_USER_NAME_CASE_INSENSITIVE_MIGRATION_MODE = false;
+ private static final boolean CASE_SENSITIVE_USERNAME = false;
+ private static final boolean CASE_INSENSITIVE_USERNAME = true;
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdateProvider;
@Inject private ExternalIds externalIds;
@@ -904,6 +906,34 @@
@Test
@GerritConfig(name = "auth.userNameCaseInsensitive", value = "true")
@GerritConfig(name = "auth.userNameCaseInsensitiveMigrationMode", value = "true")
+ public void shouldTolerateDuplicateExternalIdsWhenInMigrationMode() throws Exception {
+ Account.Id firstAccountId = Account.id(1);
+ Account.Id secondAccountId = Account.id(2);
+
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = externalIdNotesFactory.load(allUsersRepo);
+
+ createExternalId(
+ md, extIdNotes, SCHEME_GERRIT, "janedoe", firstAccountId, CASE_SENSITIVE_USERNAME);
+ createExternalId(
+ md, extIdNotes, SCHEME_GERRIT, "JaneDoe", secondAccountId, CASE_SENSITIVE_USERNAME);
+
+ ExternalId.Key firstAccountExternalId =
+ externalIdKeyFactory.create(SCHEME_GERRIT, "janedoe", CASE_INSENSITIVE_USERNAME);
+ assertThat(externalIds.get(firstAccountExternalId).get().accountId())
+ .isEqualTo(firstAccountId);
+
+ ExternalId.Key secondAccountExternalId =
+ externalIdKeyFactory.create(SCHEME_GERRIT, "JaneDoe", CASE_INSENSITIVE_USERNAME);
+ assertThat(externalIds.get(secondAccountExternalId).get().accountId())
+ .isEqualTo(secondAccountId);
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "auth.userNameCaseInsensitive", value = "true")
+ @GerritConfig(name = "auth.userNameCaseInsensitiveMigrationMode", value = "true")
public void createCaseInsensitiveMigrationModeExternalIdAccountDuringTheMigration()
throws Exception {
Account.Id accountId = Account.id(66);
@@ -982,6 +1012,12 @@
assertThat(getAccountId(extIdNotes, scheme, id.toLowerCase())).isEqualTo(accountId.get());
}
+ /**
+ * Create external id object
+ *
+ * <p>This method skips gerrit.config auth.userNameCaseInsensitiveMigrationMode and allow to
+ * create case sensitive/insensitive external id
+ */
protected void createExternalId(
MetaDataUpdate md,
ExternalIdNotes extIdNotes,
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index 28b7146..25334fd 100644
--- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
@@ -104,7 +104,7 @@
extIdKeyFactory = new ExternalIdKeyFactory(new ExternalIdKeyFactory.ConfigImpl(authConfig));
extIdFactory = new ExternalIdFactory(extIdKeyFactory, authConfig);
authRequestFactory = new AuthRequest.Factory(extIdKeyFactory);
- pwdVerifier = new PasswordVerifier(extIdKeyFactory);
+ pwdVerifier = new PasswordVerifier(extIdKeyFactory, authConfig);
authSuccessful =
new AuthResult(AUTH_ACCOUNT_ID, extIdKeyFactory.create("username", AUTH_USER), false);
