opendev/gerrit
Code Issues Proposed changes

AccountManager: Allow to duplicate emails per account

Browse Source 
Fix a recurring issue after the migration of external ids from ReviewDb
to NoteDb failing with the error 'Email <email> is already assigned to
account <>' and thus failing LDAP authentication.

The LDAP authentication may or may not have generated the preferred
email record in the main 'gerrit:<>' account. Consequently, users may
have added their external id as additional 'mailto:<>' record. That
was a normally supported use-case and, previously in v2.15, did not
create any problems or conflicts.

Starting with v2.16 the extra check for unique e-mails did go a bit too
far and blocked the ability to have both the 'mailto:' record and having
the same value also as preferred e-mail, which is incorrect because it is
a perfectly valid use-case (see Change-Id: Ie4c677365cd).

When linking or updating the preferred e-mail to an existing account,
accept duplications only if both external ids belongs to the target
account id. Because both e-mails (preferred e-mail and 'mailto:' record)
belongs to the same account, they are not really duplicated but just a
legacy of the way Gerrit was used to indicate an external 'mailto:'
identity that has been selected as primary e-mail afterwards.

Add unit-test that consistently reproduce the issue that many people
have reported after having migrated the accounts and external-ids to
All-Users.

NOTE: Even though the issue has been reported on the LDAP authentication
use-case, the problem lies in AccountManager and is more generic.

Fix the consistency checker for the e-mail external ids when
multiple duplicate entries are associated with the same account.

Bug: Issue 11246
Bug: Issue 9001
Change-Id: I78bc82faa2761bc0e56a9fa54a94225c82317275
This commit is contained in:
Luca Milanesio 2019-09-25 23:49:40 +01:00 committed by David Ostrovsky
parent e1587b5887
commit 91180b417e
4 changed files with 87 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
java/com/google/gerrit/server/account/AccountManager.java
View File

@ -226,7 +226,7 @@ public class AccountManager {
if (newEmail != null && !newEmail.equals(oldEmail)) {
ExternalId extIdWithNewEmail =
ExternalId.create(extId.key(), extId.accountId(), newEmail, extId.password());
checkEmailNotUsed(extIdWithNewEmail);
checkEmailNotUsed(extId.accountId(), extIdWithNewEmail);
accountUpdates.add(u -> u.replaceExternalId(extId, extIdWithNewEmail));
if (oldEmail != null && oldEmail.equals(user.getAccount().getPreferredEmail())) {
@ -278,7 +278,7 @@ public class AccountManager {
ExternalId extId =
ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress());
logger.atFine().log("Created external Id: %s", extId);
checkEmailNotUsed(extId);
checkEmailNotUsed(newId, extId);
ExternalId userNameExtId =
who.getUserName().isPresent() ? createUsername(newId, who.getUserName().get()) : null;
@ -353,7 +353,8 @@ public class AccountManager {
return ExternalId.create(SCHEME_USERNAME, username, accountId);
}
private void checkEmailNotUsed(ExternalId extIdToBeCreated) throws IOException, AccountException {
private void checkEmailNotUsed(Account.Id accountId, ExternalId extIdToBeCreated)
throws IOException, AccountException {
String email = extIdToBeCreated.email();
if (email == null) {
return;
@ -364,14 +365,18 @@ public class AccountManager {
return;
}
logger.atWarning().log(
"Email %s is already assigned to account %s;"
+ " cannot create external ID %s with the same email for account %s.",
email,
existingExtIdsWithEmail.iterator().next().accountId().get(),
extIdToBeCreated.key().get(),
extIdToBeCreated.accountId().get());
throw new AccountException("Email '" + email + "' in use by another account");
for (ExternalId externalId : existingExtIdsWithEmail) {
if (externalId.accountId().get() != accountId.get()) {
logger.atWarning().log(
"Email %s is already assigned to account %s;"
+ " cannot create external ID %s with the same email for account %s.",
email,
externalId.accountId().get(),
extIdToBeCreated.key().get(),
extIdToBeCreated.accountId().get());
throw new AccountException("Email '" + email + "' in use by another account");
}
}
}
private void addGroupMember(AccountGroup.UUID groupUuid, IdentifiedUser user)
@ -412,7 +417,7 @@ public class AccountManager {
} else {
ExternalId newExtId =
ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress());
checkEmailNotUsed(newExtId);
checkEmailNotUsed(to, newExtId);
accountsUpdateProvider
.get()
.update(

11
java/com/google/gerrit/server/account/externalids/ExternalIdsConsistencyChecker.java
View File

@ -73,8 +73,7 @@ public class ExternalIdsConsistencyChecker {
private List<ConsistencyProblemInfo> check(ExternalIdNotes extIdNotes) throws IOException {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
ListMultimap<String, ExternalId.Key> emails =
MultimapBuilder.hashKeys().arrayListValues().build();
ListMultimap<String, ExternalId> emails = MultimapBuilder.hashKeys().arrayListValues().build();
try (RevWalk rw = new RevWalk(extIdNotes.getRepository())) {
NoteMap noteMap = extIdNotes.getNoteMap();
@ -85,7 +84,11 @@ public class ExternalIdsConsistencyChecker {
problems.addAll(validateExternalId(extId));
if (extId.email() != null) {
emails.put(extId.email(), extId.key());
String email = extId.email();
if (emails.get(email).stream()
.noneMatch(e -> e.accountId().get() == extId.accountId().get())) {
emails.put(email, extId);
}
}
} catch (ConfigInvalidException e) {
addError(String.format(e.getMessage()), problems);
@ -102,7 +105,7 @@ public class ExternalIdsConsistencyChecker {
"Email '%s' is not unique, it's used by the following external IDs: %s",
e.getKey(),
e.getValue().stream()
.map(k -> "'" + k.get() + "'")
.map(k -> "'" + k.key().get() + "'")
.sorted()
.collect(joining(", "))),
problems));

62
javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
View File

@ -16,7 +16,9 @@ package com.google.gerrit.acceptance.api.accounts;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static java.util.stream.Collectors.toSet;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.common.Nullable;
@ -35,6 +37,7 @@ import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.inject.Inject;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@ -433,6 +436,44 @@ public class AccountManagerIT extends AbstractDaemonTest {
assertThat(accountState.get().getAccount().getPreferredEmail()).isEqualTo(email);
}
@Test
public void canFlagExistingExternalIdMailAsPreferred() throws Exception {
String email = "foo@example.com";
// Create an account with a SCHEME_GERRIT external ID
String username = "foo";
ExternalId.Key gerritExtIdKey = ExternalId.Key.create(ExternalId.SCHEME_GERRIT, username);
Account.Id accountId = new Account.Id(seq.nextAccountId());
accountsUpdate.insert(
"Create Test Account",
accountId,
u -> u.addExternalId(ExternalId.create(gerritExtIdKey, accountId)));
// Add the additional mail external ID with SCHEME_EMAIL
accountManager.link(accountId, AuthRequest.forEmail(email));
// Try to authenticate and update the email for the account.
// Expect that this to succeed because even if the email already exist
// it is associated to the same account-id and thus is not really
// a duplicate but simply a promotion of external id to preferred email.
AuthRequest who = AuthRequest.forUser(username);
who.setEmailAddress(email);
AuthResult authResult = accountManager.authenticate(who);
// Verify that no new accounts have been created
assertThat(authResult.isNew()).isFalse();
// Verify that the account external ids with scheme 'mailto:' contains the email
AccountState account = accounts.get(authResult.getAccountId()).get();
ImmutableSet<ExternalId> accountExternalIds = account.getExternalIds(ExternalId.SCHEME_MAILTO);
assertThat(accountExternalIds).isNotEmpty();
Set<String> emails = ExternalId.getEmails(accountExternalIds).collect(toSet());
assertThat(emails).contains(email);
// Verify the preferred email
assertThat(account.getAccount().getPreferredEmail()).isEqualTo(email);
}
@Test
public void linkNewExternalId() throws Exception {
// Create an account with a SCHEME_GERRIT external ID and no email
@ -535,7 +576,26 @@ public class AccountManagerIT extends AbstractDaemonTest {
AuthRequest who = AuthRequest.forEmail(email);
exception.expect(AccountException.class);
exception.expectMessage("Email 'foo@example.com' in use by another account");
accountManager.link(accountId, who);
accountManager.link(accountId2, who);
}
@Test
public void allowLinkingExistingExternalIdEmailAsPreferred() throws Exception {
String email = "foo@example.com";
// Create an account with an SCHEME_EXTERNAL external ID that occupies the email.
String username = "foo";
Account.Id accountId = new Account.Id(seq.nextAccountId());
ExternalId.Key externalExtIdKey = ExternalId.Key.create(ExternalId.SCHEME_EXTERNAL, username);
accountsUpdate.insert(
"Create Test Account",
accountId,
u -> u.addExternalId(ExternalId.createWithEmail(externalExtIdKey, accountId, email)));
AuthRequest who = AuthRequest.forEmail(email);
AuthResult result = accountManager.link(accountId, who);
assertThat(result.isNew()).isFalse();
assertThat(result.getAccountId().get()).isEqualTo(accountId.get());
}
private void assertNoSuchExternalIds(ExternalId.Key... extIdKeys) throws Exception {

3
javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
View File

@ -464,6 +464,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
admin.id,
"admin.other@example.com",
"secret-password"));
insertExtId(ExternalId.createEmail(admin.id, "admin.other@example.com"));
insertExtId(createExternalIdWithOtherCaseEmail(nextId(scheme, i)));
}
@ -690,7 +691,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
}
private ExternalId createExternalIdWithDuplicateEmail(String externalId) {
return ExternalId.createWithEmail(ExternalId.Key.parse(externalId), admin.id, admin.email);
return ExternalId.createWithEmail(ExternalId.Key.parse(externalId), user.id, admin.email);
}
private ExternalId createExternalIdWithBadPassword(String username) {