From 0a6e793eea583dc887746f0f9fc62a9cb2d2ff5f Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 15 May 2020 01:10:16 +0100 Subject: [PATCH] Block the removal of the Realm primary external ids Do not allow removing e-mails associated with the primary external id of a Realm as it would result in effectively removing the account id in that realm. Removing accounts is not allowed in Gerrit and therefore should not be allowed in Realms as well, especially when that is done unconsciously while removing an e-mail. This problem has been in the Gerrit code-base for years but it was hidden by the front-end check done in the GWT UI that was checking if the realm allowed or not that deletion. The advent of PolyGerrit UI has changed how things work and delegated the responsibility of making the decision to the backend, which was broken. The attempt to remove the e-mail associated with the primary external id scheme of the realm will now result in a ResourceConflictException, which is one of the error conditions that should be already known to the PolyGerrit UI. Bug: Issue 12755 Change-Id: I06f3414b02d00cdeaacb36e22f756560d70c5b45 --- .../server/restapi/account/DeleteEmail.java | 15 +++- .../acceptance/api/accounts/AccountIT.java | 70 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/restapi/account/DeleteEmail.java b/java/com/google/gerrit/server/restapi/account/DeleteEmail.java index 36788c754b..26fd438a3d 100644 --- a/java/com/google/gerrit/server/restapi/account/DeleteEmail.java +++ b/java/com/google/gerrit/server/restapi/account/DeleteEmail.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.restapi.account; import static java.util.stream.Collectors.toSet; import com.google.gerrit.extensions.client.AccountFieldName; +import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.extensions.common.Input; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; @@ -33,6 +34,7 @@ import com.google.gerrit.server.account.AccountResource; 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.AuthConfig; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -52,6 +54,7 @@ public class DeleteEmail implements RestModifyView private final PermissionBackend permissionBackend; private final AccountManager accountManager; private final ExternalIds externalIds; + private final AuthType authType; @Inject DeleteEmail( @@ -59,12 +62,14 @@ public class DeleteEmail implements RestModifyView Realm realm, PermissionBackend permissionBackend, AccountManager accountManager, - ExternalIds externalIds) { + ExternalIds externalIds, + AuthConfig authConfig) { this.self = self; this.realm = realm; this.permissionBackend = permissionBackend; this.accountManager = accountManager; this.externalIds = externalIds; + this.authType = authConfig.getAuthType(); } @Override @@ -95,6 +100,14 @@ public class DeleteEmail implements RestModifyView throw new ResourceNotFoundException(email); } + if (realm.accountBelongsToRealm(extIds)) { + String errorMsg = + String.format( + "Cannot remove e-mail '%s' which is directly associated with %s authentication", + email, authType); + throw new ResourceConflictException(errorMsg); + } + try { accountManager.unlink( user.getAccountId(), extIds.stream().map(ExternalId::key).collect(toSet())); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 9846b4c4a6..7975e08da6 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -1250,6 +1250,76 @@ public class AccountIT extends AbstractDaemonTest { .containsNoneOf(extId1, extId2); } + @Test + @GerritConfig(name = "auth.type", value = "LDAP") + public void deleteEmailShouldNotRemoveLdapExternalIdScheme() throws Exception { + String ldapEmail = "foo@company.com"; + String ldapExternalId = ExternalId.SCHEME_GERRIT + ":foo"; + accountsUpdateProvider + .get() + .update( + "Add External IDs", + admin.id, + u -> + u.addExternalId( + ExternalId.createWithEmail( + ExternalId.Key.parse(ldapExternalId), admin.id, ldapEmail))); + accountIndexedCounter.assertReindexOf(admin); + assertThat( + gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet())) + .contains(ldapExternalId); + + resetCurrentApiUser(); + assertThat(getEmails()).contains(ldapEmail); + + ResourceConflictException exception = + assertThrows( + ResourceConflictException.class, () -> gApi.accounts().self().deleteEmail(ldapEmail)); + assertThat(exception).hasMessageThat().contains(ldapEmail); + + resetCurrentApiUser(); + assertThat(getEmails()).contains(ldapEmail); + assertThat( + gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet())) + .contains(ldapExternalId); + } + + @Test + @GerritConfig(name = "auth.type", value = "LDAP") + public void deleteEmailShouldRemoveNonLdapExternalIdScheme() throws Exception { + String ldapEmail = "foo@company.com"; + String ldapExternalId = ExternalId.SCHEME_GERRIT + ":foo"; + String nonLdapEMail = "foo@example.com"; + String nonLdapExternalId = ExternalId.SCHEME_MAILTO + ":foo@example.com"; + accountsUpdateProvider + .get() + .update( + "Add External IDs", + admin.id, + u -> + u.addExternalId( + ExternalId.createWithEmail( + ExternalId.Key.parse(nonLdapExternalId), admin.id, nonLdapEMail)) + .addExternalId( + ExternalId.createWithEmail( + ExternalId.Key.parse(ldapExternalId), admin.id, ldapEmail))); + accountIndexedCounter.assertReindexOf(admin); + assertThat( + gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet())) + .containsAllOf(ldapExternalId, nonLdapExternalId); + + resetCurrentApiUser(); + assertThat(getEmails()).containsAllOf(ldapEmail, nonLdapEMail); + + gApi.accounts().self().deleteEmail(nonLdapEMail); + + resetCurrentApiUser(); + assertThat(getEmails()).doesNotContain(nonLdapEMail); + assertThat( + gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet())) + .contains(ldapExternalId); + } + @Test public void deleteEmailOfOtherUser() throws Exception { String email = "foo.bar@example.com";