opendev/gerrit
Code Issues Proposed changes

Prevent preferred emails without external ID email

Browse Source 
Normally a user can only set an email as preferred email if this email
is assigned to the user's account via an external ID. Before invoking
the PutPreferred REST endpoint EmailsCollection verifies that the email
belongs to the user and rejects the request with 404 Not Found if not.
Whether an email belongs to a user is decided by the Realm. By default
Realm implementations (including DefaultRealm) check the external IDs of
the user account for this. However a realm may also consider additional
emails from external sources. If email assignments in the external
sources change over time it can happen that in Gerrit we get multiple
accounts with the same preferred email. This makes the preferred email
ambiguous. Ambigouos emails are OK for Gerrit but they are at least
confusing to users that use emails as account identifiers in the REST
API because Gerrit returns 404 Not Found if an account identifier is
ambiguous.

To prevent that this leads to multiple accounts with the same preferred
email we now make sure that an external ID for the preferred email
exists. If an external ID with the email doesn't exist yet but the realm
says that the email belongs to the user we now create a MAILTO external
ID with that email automatically unless the email is already assigned to
another account via an external ID. In the latter case we reject setting
the preferred email by returning 409 Conflict.

Note that with this change newly set preferred emails will now always
exactly match an email of the external IDs of the user (case-sensitive
match). Before it was possible to set a preferred email that was only a
case-insensitive match of an external ID email. If the given email only
has an case-insensitive match with an external ID email we now
automatically choose the correct case for setting the preferred email.

Change-Id: Icb6299b7fbceadb4029a7b645ddd65d096e1fe27
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-04-09 16:04:03 +02:00
parent c7d68163d9
commit e522ebd40e
5 changed files with 196 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.InternalAccountUpdate.Builder;
import com.google.gerrit.server.account.externalids.ExternalIdNotes;
import com.google.gerrit.server.account.externalids.ExternalIdNotes.ExternalIdNotesLoader;
import com.google.gerrit.server.account.externalids.ExternalIds;
@@ -146,10 +147,17 @@ public class AccountsUpdate {
* @param accountState the account that is being updated
* @param update account update builder
*/
void update(AccountState accountState, InternalAccountUpdate.Builder update);
void update(AccountState accountState, InternalAccountUpdate.Builder update) throws IOException;
static AccountUpdater join(List<AccountUpdater> updaters) {
return (a, u) -> updaters.stream().forEach(updater -> updater.update(a, u));
return new AccountUpdater() {
@Override
public void update(AccountState accountState, Builder update) throws IOException {
for (AccountUpdater updater : updaters) {
updater.update(accountState, update);
}
}
};
}
static AccountUpdater joinConsumers(List<Consumer<InternalAccountUpdate.Builder>> consumers) {

4
java/com/google/gerrit/server/account/DefaultRealm.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.account;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.extensions.client.AuthType;
@@ -33,7 +34,8 @@ public class DefaultRealm extends AbstractRealm {
private final AuthConfig authConfig;
@Inject
DefaultRealm(EmailExpander emailExpander, Provider<Emails> emails, AuthConfig authConfig) {
@VisibleForTesting
public DefaultRealm(EmailExpander emailExpander, Provider<Emails> emails, AuthConfig authConfig) {
this.emailExpander = emailExpander;
this.emails = emails;
this.authConfig = authConfig;

13
java/com/google/gerrit/server/restapi/account/CreateEmail.java
View File

@@ -20,12 +20,11 @@ import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
@@ -90,9 +89,8 @@ public class CreateEmail implements RestModifyView<AccountResource, EmailInput>
@Override
public Response<EmailInfo> apply(AccountResource rsrc, EmailInput input)
throws AuthException, BadRequestException, ResourceConflictException,
ResourceNotFoundException, OrmException, EmailException, MethodNotAllowedException,
IOException, ConfigInvalidException, PermissionBackendException {
throws RestApiException, OrmException, EmailException, MethodNotAllowedException, IOException,
ConfigInvalidException, PermissionBackendException {
if (input == null) {
input = new EmailInput();
}
@@ -110,9 +108,8 @@ public class CreateEmail implements RestModifyView<AccountResource, EmailInput>
/** To be used from plugins that want to create emails without permission checks. */
public Response<EmailInfo> apply(IdentifiedUser user, EmailInput input)
throws AuthException, BadRequestException, ResourceConflictException,
ResourceNotFoundException, OrmException, EmailException, MethodNotAllowedException,
IOException, ConfigInvalidException, PermissionBackendException {
throws RestApiException, OrmException, EmailException, MethodNotAllowedException, IOException,
ConfigInvalidException, PermissionBackendException {
if (input == null) {
input = new EmailInput();
}

88
java/com/google/gerrit/server/restapi/account/PutPreferred.java
View File

@@ -14,16 +14,22 @@
package com.google.gerrit.server.restapi.account;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -32,38 +38,49 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class PutPreferred implements RestModifyView<AccountResource.Email, Input> {
private static final Logger log = LoggerFactory.getLogger(PutPreferred.class);
private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
private final Provider<AccountsUpdate> accountsUpdateProvider;
private final ExternalIds externalIds;
@Inject
PutPreferred(
Provider<CurrentUser> self,
PermissionBackend permissionBackend,
@ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider) {
@ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider,
ExternalIds externalIds) {
this.self = self;
this.permissionBackend = permissionBackend;
this.accountsUpdateProvider = accountsUpdateProvider;
this.externalIds = externalIds;
}
@Override
public Response<String> apply(AccountResource.Email rsrc, Input input)
throws AuthException, ResourceNotFoundException, OrmException, IOException,
PermissionBackendException, ConfigInvalidException {
throws RestApiException, OrmException, IOException, PermissionBackendException,
ConfigInvalidException {
if (self.get() != rsrc.getUser()) {
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
}
return apply(rsrc.getUser(), rsrc.getEmail());
}
public Response<String> apply(IdentifiedUser user, String email)
throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
public Response<String> apply(IdentifiedUser user, String preferredEmail)
throws RestApiException, IOException, ConfigInvalidException, OrmException {
AtomicReference<Optional<RestApiException>> exception = new AtomicReference<>(Optional.empty());
AtomicBoolean alreadyPreferred = new AtomicBoolean(false);
accountsUpdateProvider
.get()
@@ -71,13 +88,68 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
"Set Preferred Email via API",
user.getAccountId(),
(a, u) -> {
if (email.equals(a.getAccount().getPreferredEmail())) {
if (preferredEmail.equals(a.getAccount().getPreferredEmail())) {
alreadyPreferred.set(true);
} else {
u.setPreferredEmail(email);
// check if the user has a matching email
String matchingEmail = null;
for (String email :
a.getExternalIds()
.stream()
.map(ExternalId::email)
.filter(Objects::nonNull)
.collect(toSet())) {
if (email.equals(preferredEmail)) {
// we have an email that matches exactly, prefer this one
matchingEmail = email;
break;
} else if (matchingEmail == null && email.equalsIgnoreCase(preferredEmail)) {
// we found an email that matches but has a different case
matchingEmail = email;
}
}
if (matchingEmail == null) {
// user doesn't have an external ID for this email
if (user.hasEmailAddress(preferredEmail)) {
// but Realm says the user is allowed to use this email
Set<ExternalId> existingExtIdsWithThisEmail =
externalIds.byEmail(preferredEmail);
if (!existingExtIdsWithThisEmail.isEmpty()) {
// but the email is already assigned to another account
log.warn(
"Cannot set preferred email {} for account {} because it is owned"
+ " by the following account(s): {}",
preferredEmail,
user.getAccountId(),
existingExtIdsWithThisEmail
.stream()
.map(ExternalId::accountId)
.collect(toList()));
exception.set(
Optional.of(
new ResourceConflictException("email in use by another account")));
return;
}
// claim the email now
u.addExternalId(ExternalId.createEmail(a.getAccount().getId(), preferredEmail));
matchingEmail = preferredEmail;
} else {
// Realm says that the email doesn't belong to the user. This can only happen as
// a race condition because EmailsCollection would have thrown
// ResourceNotFoundException already before invoking this REST endpoint.
exception.set(Optional.of(new ResourceNotFoundException(preferredEmail)));
return;
}
}
u.setPreferredEmail(matchingEmail);
}
})
.orElseThrow(() -> new ResourceNotFoundException("account not found"));
if (exception.get().isPresent()) {
throw exception.get().get();
}
return alreadyPreferred.get() ? Response.ok("") : Response.created("");
}
}

99
javatests/com/google/gerrit/acceptance/rest/account/EmailIT.java
View File

@@ -15,27 +15,54 @@
package com.google.gerrit.acceptance.rest.account;
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.ImmutableMultimap;
import com.google.common.collect.Multimap;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.extensions.api.accounts.EmailApi;
import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.DefaultRealm;
import com.google.gerrit.server.account.EmailExpander;
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.DisableReverseDnsLookup;
import com.google.gson.reflect.TypeToken;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.junit.Test;
public class EmailIT extends AbstractDaemonTest {
@Inject private @ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider;
@Inject private ExternalIds externalIds;
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
@Inject private AuthConfig authConfig;
@Inject private @AnonymousCowardName String anonymousCowardName;
@Inject private @CanonicalWebUrl Provider<String> canonicalUrl;
@Inject private @DisableReverseDnsLookup Boolean disableReverseDnsLookup;
@Inject private EmailExpander emailExpander;
@Inject private Provider<Emails> emails;
@Test
public void addEmail() throws Exception {
@@ -144,7 +171,43 @@ public class EmailIT extends AbstractDaemonTest {
resetCurrentApiUser();
String emailOtherCase = email.toUpperCase();
gApi.accounts().self().email(emailOtherCase).setPreferred();
assertThat(gApi.accounts().self().get().email).isEqualTo(emailOtherCase);
assertThat(gApi.accounts().self().get().email).isEqualTo(email);
}
@Test
public void setPreferredEmailToEmailFromCustomRealmThatDoesntExistAsExternalId()
throws Exception {
String email = "foo@example.com";
ExternalId.Key mailtoExtIdKey = ExternalId.Key.create(ExternalId.SCHEME_MAILTO, email);
assertThat(externalIds.get(mailtoExtIdKey)).isEmpty();
assertThat(gApi.accounts().self().get().email).isNotEqualTo(email);
Context oldCtx = createContextWithCustomRealm(new RealmWithAdditionalEmails(admin.id, email));
try {
gApi.accounts().self().email(email).setPreferred();
Optional<ExternalId> mailtoExtId = externalIds.get(mailtoExtIdKey);
assertThat(mailtoExtId).isPresent();
assertThat(mailtoExtId.get().accountId()).isEqualTo(admin.id);
assertThat(gApi.accounts().self().get().email).isEqualTo(email);
} finally {
atrScope.set(oldCtx);
}
}
@Test
public void setPreferredEmailToEmailFromCustomRealmThatBelongsToOtherAccount() throws Exception {
ExternalId mailToExtId = ExternalId.createEmail(user.id, user.email);
assertThat(externalIds.get(mailToExtId.key())).isPresent();
Context oldCtx =
createContextWithCustomRealm(new RealmWithAdditionalEmails(admin.id, user.email));
try {
exception.expect(ResourceConflictException.class);
exception.expectMessage("email in use by another account");
gApi.accounts().self().email(user.email).setPreferred();
} finally {
atrScope.set(oldCtx);
}
}
@Test
@@ -205,4 +268,38 @@ public class EmailIT extends AbstractDaemonTest {
RestResponse r = adminRestSession.put("/accounts/self/emails/" + email, input);
r.assertCreated();
}
private Context createContextWithCustomRealm(Realm realm) {
IdentifiedUser.GenericFactory userFactory =
new IdentifiedUser.GenericFactory(
authConfig,
realm,
anonymousCowardName,
canonicalUrl,
disableReverseDnsLookup,
accountCache,
groupBackend);
return atrScope.set(atrScope.newContext(reviewDbProvider, null, userFactory.create(admin.id)));
}
private class RealmWithAdditionalEmails extends DefaultRealm {
private final Multimap<Account.Id, String> additionalEmails;
public RealmWithAdditionalEmails(Account.Id accountId, String email) {
this(ImmutableMultimap.of(accountId, email));
}
public RealmWithAdditionalEmails(Multimap<Account.Id, String> additionalEmails) {
super(emailExpander, emails, authConfig);
this.additionalEmails = additionalEmails;
}
@Override
public boolean hasEmailAddress(IdentifiedUser user, String email) {
if (additionalEmails.containsEntry(user.getAccountId(), email)) {
return true;
}
return super.hasEmailAddress(user, email);
}
}
}