Support deleting multiple GPG keys in a single request

This is important for the web UI where a user might want to clear out
multiple GPG keys in a row. If we were to do this by firing off
multiple requests, we would introduce write contention on the gpg-keys
ref, likely causing lock failures on the update since we don't
(currently) spin and retry.

Rather than adding a new endpoint, reuse the existing POST endpoint
for this purpose. The response from this endpoint is now a map similar
to listing all keys, except it only lists keys that were modified by
the operation. Deleted keys have no fields in the response.

Change-Id: Id4b7ec651380720249f7623e9824e8b116962f7d

Documentation/rest-api-accounts.txt

@@ -714,7 +714,7 @@ link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID.
         "John Doe \u003cjohn.doe@example.com\u003e"
       ],
       "key": "-----BEGIN PGP PUBLIC KEY BLOCK-----\nVersion: BCPG v1.52\n\nmQENBFXUpNcBCACv4paCiyKxZ0EcKy8VaWVNkJlNebRBiyw9WxU85wPOq5Gz/3GT\nRQwKqeY0SxVdQT8VNBw2sBe2m6eqcfZ2iKmesSlbXMe15DA7k8Bg4zEpQ0tXNG1L\nhceZDVQ1Xk06T2sgkunaiPsXi82nwN3UWYtDXxX4is5e6xBNL48Jgz4lbqo6+8D5\nvsVYiYMx4AwRkJyt/oA3IZAtSlY8Yd445nY14VPcnsGRwGWTLyZv9gxKHRUppVhQ\nE3o6ePXKEVgmONnQ4CjqmkGwWZvjMF2EPtAxvQLAuFa8Hqtkq5cgfgVkv/Vrcln4\nnQZVoMm3a3f5ODii2tQzNh6+7LL1bpqAmVEtABEBAAG0H0pvaG4gRG9lIDxqb2hu\nLmRvZUBleGFtcGxlLmNvbT6JATgEEwECACIFAlXUpNcCGwMGCwkIBwMCBhUIAgkK\nCwQWAgMBAh4BAheAAAoJEJNQnkuvyKSbfjoH/2OcSQOu1kJ20ndjhgY2yNChm7gd\ntU7TEBbB0TsLeazkrrLtKvrpW5+CRe07ZAG9HOtp3DikwAyrhSxhlYgVsQDhgB8q\nG0tYiZtQ88YyYrncCQ4hwknrcWXVW9bK3V4ZauxzPv3ADSloyR9tMURw5iHCIeL5\nfIw/pLvA3RjPMx4Sfow/bqRCUELua39prGw5Tv8a2ZRFbj2sgP5j8lUFegyJPQ4z\ntJhe6zZvKOzvIyxHO8llLmdrImsXRL9eqroWGs0VYqe6baQpY6xpSjbYK0J5HYcg\nTO+/u80JI+ROTMHE6unGp5Pgh/xIz6Wd34E0lWL1eOyNfGiPLyRWn1d0"
-    }
+    },
   }
 ----
 
@@ -751,16 +751,16 @@ describes the GPG key.
   }
 ----
 
-[[add-gpg-keys]]
+[[add-delete-gpg-keys]]
-=== Add GPG Keys
+=== Add/Delete GPG Keys
 --
 'POST /accounts/link:#account-id[\{account-id\}]/gpgkeys'
 --
 
-Add one or more GPG keys for a user.
+Add or delete one or more GPG keys for a user.
 
-The new keys must be provided in the request body as a
+The changes must be provided in the request body as a
-link:#gpg-key-input[GpgKeyInput] entity. Each GPG key is provided in
+link:#gpg-key-input[GpgKeyInput] entity. Each new GPG key is provided in
 ASCII armored format, and must contain a self-signed certification
 matching a registered email or other identity of the user.
 
@@ -772,12 +772,16 @@ matching a registered email or other identity of the user.
   {
     "add": [
       "-----BEGIN PGP PUBLIC KEY BLOCK-----\nVersion: GnuPG v1\n\nmQENBFXUpNcBCACv4paCiyKxZ0EcKy8VaWVNkJlNebRBiyw9WxU85wPOq5Gz/3GT\nRQwKqeY0SxVdQT8VNBw2sBe2m6eqcfZ2iKmesSlbXMe15DA7k8Bg4zEpQ0tXNG1L\nhceZDVQ1Xk06T2sgkunaiPsXi82nwN3UWYtDXxX4is5e6xBNL48Jgz4lbqo6+8D5\nvsVYiYMx4AwRkJyt/oA3IZAtSlY8Yd445nY14VPcnsGRwGWTLyZv9gxKHRUppVhQ\nE3o6ePXKEVgmONnQ4CjqmkGwWZvjMF2EPtAxvQLAuFa8Hqtkq5cgfgVkv/Vrcln4\nnQZVoMm3a3f5ODii2tQzNh6+7LL1bpqAmVEtABEBAAG0H0pvaG4gRG9lIDxqb2hu\nLmRvZUBleGFtcGxlLmNvbT6JATgEEwECACIFAlXUpNcCGwMGCwkIBwMCBhUIAgkK\nCwQWAgMBAh4BAheAAAoJEJNQnkuvyKSbfjoH/2OcSQOu1kJ20ndjhgY2yNChm7gd\ntU7TEBbB0TsLeazkrrLtKvrpW5+CRe07ZAG9HOtp3DikwAyrhSxhlYgVsQDhgB8q\nG0tYiZtQ88YyYrncCQ4hwknrcWXVW9bK3V4ZauxzPv3ADSloyR9tMURw5iHCIeL5\nfIw/pLvA3RjPMx4Sfow/bqRCUELua39prGw5Tv8a2ZRFbj2sgP5j8lUFegyJPQ4z\ntJhe6zZvKOzvIyxHO8llLmdrImsXRL9eqroWGs0VYqe6baQpY6xpSjbYK0J5HYcg\nTO+/u80JI+ROTMHE6unGp5Pgh/xIz6Wd34E0lWL1eOyNfGiPLyRWn1d0yZO5AQ0E\nVdSk1wEIALUycrH2HK9zQYdR/KJo1yJJuaextLWsYYn881yDQo/p06U5vXOZ28lG\nAq/Xs96woVZPbgME6FyQzhf20Z2sbr+5bNo3OcEKaKX3Eo/sWwSJ7bXbGLDxMf4S\netfY1WDC+4rTqE30JuC++nQviPRdCcZf0AEgM6TxVhYEMVYwV787YO1IH62EBICM\nSkIONOfnusNZ4Skgjq9OzakOOpROZ4tki5cH/5oSDgdcaGPy1CFDpL9fG6er2zzk\nsw3qCbraqZrrlgpinWcAduiao67U/dV18O6OjYzrt33fTKZ0+bXhk1h1gloC21MQ\nya0CXlnfR/FOQhvuK0RlbR3cMfhZQscAEQEAAYkBHwQYAQIACQUCVdSk1wIbDAAK\nCRCTUJ5Lr8ikm8+QB/4uE+AlvFQFh9W8koPdfk7CJF7wdgZZ2NDtktvLL71WuMK8\nPOmf9f5JtcLCX4iJxGzcWogAR5ed20NgUoHUg7jn9Xm3fvP+kiqL6WqPhjazd89h\nk06v9hPE65kp4wb0fQqDrtWfP1lFGuh77rQgISt3Y4QutDl49vXS183JAfGPxFxx\n8FgGcfNwL2LVObvqCA0WLqeIrQVbniBPFGocE3yA/0W9BB/xtolpKfgMMsqGRMeu\n9oIsNxB2oE61OsqjUtGsnKQi8k5CZbhJaql4S89vwS+efK0R+mo+0N55b0XxRlCS\nfaURgAcjarQzJnG0hUps2GNO/+nM7UyyJAGfHlh5\n=EdXO\n-----END PGP PUBLIC KEY BLOCK-----\n"
+    ],
+    "delete": [
+      "DEADBEEF",
     ]
   }'
 ----
 
-As a response, the added GPG keys are returned as a map of
+As a response, the modified GPG keys are returned as a map of
-link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID.
+link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID. Deleted keys are
+represented by an empty object.
 
 .Response
 ----
@@ -794,6 +798,7 @@ link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID.
       ],
       "key": "-----BEGIN PGP PUBLIC KEY BLOCK-----\nVersion: BCPG v1.52\n\nmQENBFXUpNcBCACv4paCiyKxZ0EcKy8VaWVNkJlNebRBiyw9WxU85wPOq5Gz/3GT\nRQwKqeY0SxVdQT8VNBw2sBe2m6eqcfZ2iKmesSlbXMe15DA7k8Bg4zEpQ0tXNG1L\nhceZDVQ1Xk06T2sgkunaiPsXi82nwN3UWYtDXxX4is5e6xBNL48Jgz4lbqo6+8D5\nvsVYiYMx4AwRkJyt/oA3IZAtSlY8Yd445nY14VPcnsGRwGWTLyZv9gxKHRUppVhQ\nE3o6ePXKEVgmONnQ4CjqmkGwWZvjMF2EPtAxvQLAuFa8Hqtkq5cgfgVkv/Vrcln4\nnQZVoMm3a3f5ODii2tQzNh6+7LL1bpqAmVEtABEBAAG0H0pvaG4gRG9lIDxqb2hu\nLmRvZUBleGFtcGxlLmNvbT6JATgEEwECACIFAlXUpNcCGwMGCwkIBwMCBhUIAgkK\nCwQWAgMBAh4BAheAAAoJEJNQnkuvyKSbfjoH/2OcSQOu1kJ20ndjhgY2yNChm7gd\ntU7TEBbB0TsLeazkrrLtKvrpW5+CRe07ZAG9HOtp3DikwAyrhSxhlYgVsQDhgB8q\nG0tYiZtQ88YyYrncCQ4hwknrcWXVW9bK3V4ZauxzPv3ADSloyR9tMURw5iHCIeL5\nfIw/pLvA3RjPMx4Sfow/bqRCUELua39prGw5Tv8a2ZRFbj2sgP5j8lUFegyJPQ4z\ntJhe6zZvKOzvIyxHO8llLmdrImsXRL9eqroWGs0VYqe6baQpY6xpSjbYK0J5HYcg\nTO+/u80JI+ROTMHE6unGp5Pgh/xIz6Wd34E0lWL1eOyNfGiPLyRWn1d0"
     }
+    "DEADBEEF": {}
   }
 ----
 
@@ -1723,11 +1728,11 @@ The `GpgKeyInfo` entity contains information about a GPG public key.
 |========================
 |Field Name   ||Description
 |`id`         |Not set in map context|The 8-char hex GPG key ID.
-|`fingerprint`||The 40-char (plus spaces) hex GPG key fingerprint.
+|`fingerprint`|Not set for deleted keys|The 40-char (plus spaces) hex GPG key fingerprint.
-|`user_ids`   ||
+|`user_ids`   |Not set for deleted keys|
 link:https://tools.ietf.org/html/rfc4880#section-5.11[OpenPGP User IDs]
 associated with the public key.
-|`key`        ||ASCII armored public key material.
+|`key`        |Not set for deleted keys|ASCII armored public key material.
 |========================
 
 [[gpg-key-input]]
@@ -1738,6 +1743,7 @@ The `GpgKeyInput` entity contains information for adding GPG keys.
 |========================
 |Field Name|Description
 |`add`     |List of ASCII armored public key strings to add.
+|`delete`  |List of link:#gpg-key-id[`\{gpg-key-id\}`]s to delete.
 |========================
 
 [[http-password-input]]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java

@@ -162,6 +162,10 @@ public class AcceptanceTestRequestScope {
     return old;
   }
 
+  public Context get() {
+    return current.get();
+  }
+
   public Context disableDb() {
     Context old = current.get();
     SchemaFactory<ReviewDb> sf = new SchemaFactory<ReviewDb>() {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -18,10 +18,13 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assert_;
 import static com.google.gerrit.server.git.gpg.PublicKeyStore.fingerprintToString;
-import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyIdToString;
+import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyToString;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.base.Function;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
+import com.google.common.io.BaseEncoding;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
@@ -31,8 +34,11 @@ import com.google.gerrit.extensions.common.GpgKeyInfo;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 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.client.AccountExternalId;
 import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.GpgKeys;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.gpg.PublicKeyStore;
 import com.google.gerrit.server.git.gpg.TestKey;
@@ -52,6 +58,7 @@ import org.junit.Test;
 
 import java.io.ByteArrayOutputStream;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
@@ -189,19 +196,11 @@ public class AccountIT extends AbstractDaemonTest {
   @Test
   public void addGpgKey() throws Exception {
     TestKey key = TestKey.key1();
-    String id = keyIdToString(key.getKeyId());
+    String id = key.getKeyIdString();
     addExternalIdEmail(admin, "test1@example.com");
 
-    GpgKeyInfo info = gApi.accounts().self()
+    assertKeyMapContains(key, addGpgKey(key.getPublicKeyArmored()));
-        .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
+    assertKeys(key);
-        .get(id);
-    info.id = id;
-    assertKeyEquals(key, info);
-    assertKeyEquals(key, gApi.accounts().self().gpgKey(id).get());
-
-    PGPPublicKey stored = getOnlyKeyFromStore(key);
-    assertThat(stored.getFingerprint())
-        .isEqualTo(key.getPublicKey().getFingerprint());
 
     setApiUser(user);
     exception.expect(ResourceNotFoundException.class);
@@ -213,19 +212,15 @@ public class AccountIT extends AbstractDaemonTest {
   public void reAddExistingGpgKey() throws Exception {
     addExternalIdEmail(admin, "test5@example.com");
     TestKey key = TestKey.key5();
-    String id = keyIdToString(key.getKeyId());
+    String id = key.getKeyIdString();
     PGPPublicKey pk = key.getPublicKey();
 
-    GpgKeyInfo info = gApi.accounts().self()
+    GpgKeyInfo info = addGpgKey(armor(pk)).get(id);
-        .putGpgKeys(ImmutableList.of(armor(pk)))
-        .get(id);
     assertThat(info.userIds).hasSize(2);
     assertIteratorSize(2, getOnlyKeyFromStore(key).getUserIDs());
 
     pk = PGPPublicKey.removeCertification(pk, "foo:myId");
-    info = gApi.accounts().self()
+    info = addGpgKey(armor(pk)).get(id);
-        .putGpgKeys(ImmutableList.of(armor(pk)))
-        .get(id);
     assertThat(info.userIds).hasSize(1);
     assertIteratorSize(1, getOnlyKeyFromStore(key).getUserIDs());
   }
@@ -240,17 +235,12 @@ public class AccountIT extends AbstractDaemonTest {
     db.accountExternalIds().insert(Collections.singleton(extId));
 
     TestKey key = TestKey.key5();
-    String id = keyIdToString(key.getKeyId());
+    addGpgKey(key.getPublicKeyArmored());
-    gApi.accounts().self()
-        .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
-        .get(id);
     setApiUser(user);
 
     exception.expect(ResourceConflictException.class);
     exception.expectMessage("GPG key already associated with another account");
-    gApi.accounts().self()
+    addGpgKey(key.getPublicKeyArmored());
-        .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
-        .get(id);
   }
 
   @Test
@@ -262,37 +252,62 @@ public class AccountIT extends AbstractDaemonTest {
           PushCertificateIdent.parse(key.getFirstUserId()).getEmailAddress());
       toAdd.add(key.getPublicKeyArmored());
     }
-    gApi.accounts().self().putGpgKeys(toAdd);
+    gApi.accounts().self().putGpgKeys(toAdd, ImmutableList.<String> of());
-
+    assertKeys(keys);
-    Map<String, GpgKeyInfo> actual = gApi.accounts().self().listGpgKeys();
-    assertThat(actual).hasSize(keys.size());
-    for (TestKey k : keys) {
-      String id = keyIdToString(k.getKeyId());
-      GpgKeyInfo info = actual.get(id);
-      assertThat(info).named(id).isNotNull();
-      assertThat(info.id).named(id).isNull();
-      info.id = id;
-      assertKeyEquals(k, info);
-    }
   }
 
   @Test
   public void deleteGpgKey() throws Exception {
     TestKey key = TestKey.key1();
-    String id = keyIdToString(key.getKeyId());
+    String id = key.getKeyIdString();
     addExternalIdEmail(admin, "test1@example.com");
-    gApi.accounts().self()
+    addGpgKey(key.getPublicKeyArmored());
-        .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()));
+    assertKeys(key);
-    assertKeyEquals(key, gApi.accounts().self().gpgKey(id).get());
 
     gApi.accounts().self().gpgKey(id).delete();
-    assertThat(gApi.accounts().self().listGpgKeys()).isEmpty();
+    assertKeys();
 
     exception.expect(ResourceNotFoundException.class);
     exception.expectMessage(id);
     gApi.accounts().self().gpgKey(id).get();
   }
 
+  @Test
+  public void addAndRemoveGpgKeys() throws Exception {
+    for (TestKey key : TestKey.allValidKeys()) {
+      addExternalIdEmail(admin,
+          PushCertificateIdent.parse(key.getFirstUserId()).getEmailAddress());
+    }
+    TestKey key1 = TestKey.key1();
+    TestKey key2 = TestKey.key2();
+    TestKey key5 = TestKey.key5();
+
+    Map<String, GpgKeyInfo> infos = gApi.accounts().self().putGpgKeys(
+        ImmutableList.of(
+          key1.getPublicKeyArmored(),
+          key2.getPublicKeyArmored()),
+        ImmutableList.of(key5.getKeyIdString()));
+    assertThat(infos.keySet())
+        .containsExactly(key1.getKeyIdString(), key2.getKeyIdString());
+    assertKeys(key1, key2);
+
+    infos = gApi.accounts().self().putGpgKeys(
+        ImmutableList.of(key5.getPublicKeyArmored()),
+        ImmutableList.of(key1.getKeyIdString()));
+    assertThat(infos.keySet())
+        .containsExactly(key1.getKeyIdString(), key5.getKeyIdString());
+    assertKeyMapContains(key5, infos);
+    assertThat(infos.get(key1.getKeyIdString()).key).isNull();
+    assertKeys(key2, key5);
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("Cannot both add and delete key: "
+        + keyToString(key2.getPublicKey()));
+    infos = gApi.accounts().self().putGpgKeys(
+        ImmutableList.of(key2.getPublicKeyArmored()),
+        ImmutableList.of(key2.getKeyIdString()));
+  }
+
   private PGPPublicKey getOnlyKeyFromStore(TestKey key) throws Exception {
     try (PublicKeyStore store = publicKeyStoreProvider.get()) {
       Iterable<PGPPublicKeyRing> keys = store.get(key.getKeyId());
@@ -314,8 +329,70 @@ public class AccountIT extends AbstractDaemonTest {
     assertThat(ImmutableList.copyOf(it)).hasSize(size);
   }
 
+  private static void assertKeyMapContains(TestKey expected,
+      Map<String, GpgKeyInfo> actualMap) {
+    GpgKeyInfo actual = actualMap.get(expected.getKeyIdString());
+    assertThat(actual).isNotNull();
+    assertThat(actual.id).isNull();
+    actual.id = expected.getKeyIdString();
+    assertKeyEquals(expected, actual);
+  }
+
+  private void assertKeys(TestKey... expectedKeys) throws Exception {
+    assertKeys(Arrays.asList(expectedKeys));
+  }
+
+  private void assertKeys(Iterable<TestKey> expectedKeys) throws Exception {
+    // Check via API.
+    FluentIterable<TestKey> expected = FluentIterable.from(expectedKeys);
+    Map<String, GpgKeyInfo> keyMap = gApi.accounts().self().listGpgKeys();
+    assertThat(keyMap.keySet())
+        .named("keys returned by listGpgKeys()")
+        .containsExactlyElementsIn(
+          expected.transform(new Function<TestKey, String>() {
+            @Override
+            public String apply(TestKey in) {
+              return in.getKeyIdString();
+            }
+          }));
+
+    for (TestKey key : expected) {
+      assertKeyEquals(key, gApi.accounts().self().gpgKey(
+          key.getKeyIdString()).get());
+      assertKeyEquals(key, gApi.accounts().self().gpgKey(
+          fingerprintToString(key.getPublicKey().getFingerprint())).get());
+      assertKeyMapContains(key, keyMap);
+    }
+
+    // Check raw external IDs.
+    Account.Id currAccountId =
+        ((IdentifiedUser) atrScope.get().getCurrentUser()).getAccountId();
+    assertThat(
+        GpgKeys.getGpgExtIds(db, currAccountId)
+          .transform(new Function<AccountExternalId, String>() {
+            @Override
+            public String apply(AccountExternalId in) {
+              return in.getSchemeRest();
+            }
+          }))
+        .named("external IDs in database")
+        .containsExactlyElementsIn(
+            expected.transform(new Function<TestKey, String>() {
+              @Override
+              public String apply(TestKey in) {
+                return BaseEncoding.base16().encode(
+                    in.getPublicKey().getFingerprint());
+              }
+            }));
+
+    // Check raw stored keys.
+    for (TestKey key : expected) {
+      getOnlyKeyFromStore(key);
+    }
+  }
+
   private static void assertKeyEquals(TestKey expected, GpgKeyInfo actual) {
-    String id = keyIdToString(expected.getKeyId());
+    String id = expected.getKeyIdString();
     assertThat(actual.id).named(id).isEqualTo(id);
     assertThat(actual.fingerprint).named(id).isEqualTo(
         fingerprintToString(expected.getPublicKey().getFingerprint()));
@@ -338,4 +415,10 @@ public class AccountIT extends AbstractDaemonTest {
     accountCache.evict(account.getId());
     setApiUser(account);
   }
+
+  private Map<String, GpgKeyInfo> addGpgKey(String armored) throws Exception {
+    return gApi.accounts().self().putGpgKeys(
+        ImmutableList.of(armored),
+        ImmutableList.<String> of());
+  }
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java

@@ -30,7 +30,8 @@ public interface AccountApi {
   void addEmail(EmailInput input) throws RestApiException;
 
   Map<String, GpgKeyInfo> listGpgKeys() throws RestApiException;
-  Map<String, GpgKeyInfo> putGpgKeys(List<String> add) throws RestApiException;
+  Map<String, GpgKeyInfo> putGpgKeys(List<String> add, List<String> remove)
+      throws RestApiException;
   GpgKeyApi gpgKey(String id) throws RestApiException;
 
   /**
@@ -59,8 +60,8 @@ public interface AccountApi {
     }
 
     @Override
-    public Map<String, GpgKeyInfo> putGpgKeys(List<String> add)
+    public Map<String, GpgKeyInfo> putGpgKeys(List<String> add,
-        throws RestApiException {
+        List<String> remove) throws RestApiException {
       throw new NotImplementedException();
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/account/GpgKeys.java

@@ -95,23 +95,7 @@ public class GpgKeys implements
       throw new ResourceNotFoundException(id);
     }
 
-    byte[] fp = null;
+    byte[] fp = parseFingerprint(id.get(), getGpgExtIds(parent));
-    for (AccountExternalId extId : getGpgExtIds(parent)) {
-      String fpStr = extId.getSchemeRest();
-      if (!fpStr.endsWith(str)) {
-        continue;
-      } else if (fp != null) {
-        throw new ResourceNotFoundException("Multiple keys found for " + id);
-      }
-      fp = BaseEncoding.base16().decode(fpStr);
-      if (str.length() == 40) {
-        break;
-      }
-    }
-    if (fp == null) {
-      throw new ResourceNotFoundException(id);
-    }
-
     try (PublicKeyStore store = storeProvider.get()) {
       long keyId = keyId(fp);
       for (PGPPublicKeyRing keyRing : store.get(keyId)) {
@@ -125,6 +109,33 @@ public class GpgKeys implements
     throw new ResourceNotFoundException(id);
   }
 
+  static byte[] parseFingerprint(String str,
+      Iterable<AccountExternalId> existingExtIds)
+      throws ResourceNotFoundException {
+    str = CharMatcher.WHITESPACE.removeFrom(str).toUpperCase();
+    if ((str.length() != 8 && str.length() != 40)
+        || !CharMatcher.anyOf("0123456789ABCDEF").matchesAllOf(str)) {
+      throw new ResourceNotFoundException(str);
+    }
+    byte[] fp = null;
+    for (AccountExternalId extId : existingExtIds) {
+      String fpStr = extId.getSchemeRest();
+      if (!fpStr.endsWith(str)) {
+        continue;
+      } else if (fp != null) {
+        throw new ResourceNotFoundException("Multiple keys found for " + str);
+      }
+      fp = BaseEncoding.base16().decode(fpStr);
+      if (str.length() == 40) {
+        break;
+      }
+    }
+    if (fp == null) {
+      throw new ResourceNotFoundException(str);
+    }
+    return fp;
+  }
+
   @Override
   public DynamicMap<RestView<GpgKey>> views() {
     return views;
@@ -167,7 +178,7 @@ public class GpgKeys implements
   }
 
   @VisibleForTesting
-  public static Iterable<AccountExternalId> getGpgExtIds(ReviewDb db,
+  public static FluentIterable<AccountExternalId> getGpgExtIds(ReviewDb db,
       Account.Id accountId) throws OrmException {
     return FluentIterable
         .from(db.accountExternalIds().byAccount(accountId))

gerrit-server/src/main/java/com/google/gerrit/server/account/PostGpgKeys.java

@@ -14,13 +14,18 @@
 
 package com.google.gerrit.server.account;
 
+import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyIdToString;
 import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyToString;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import com.google.common.io.BaseEncoding;
 import com.google.gerrit.extensions.common.GpgKeyInfo;
 import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -32,6 +37,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.account.PostGpgKeys.Input;
 import com.google.gerrit.server.git.gpg.CheckResult;
+import com.google.gerrit.server.git.gpg.Fingerprint;
 import com.google.gerrit.server.git.gpg.PublicKeyChecker;
 import com.google.gerrit.server.git.gpg.PublicKeyStore;
 import com.google.gwtorm.server.OrmException;
@@ -55,11 +61,13 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 @Singleton
 public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
   public static class Input {
     public List<String> add;
+    public List<String> delete;
   }
 
   private final Provider<PersonIdent> serverIdent;
@@ -84,34 +92,64 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
       ResourceConflictException, PGPException, OrmException, IOException {
     GpgKeys.checkEnabled();
 
-    List<PGPPublicKeyRing> newKeys = readKeys(input);
+    List<AccountExternalId> existingExtIds =
-    List<AccountExternalId> newExtIds = new ArrayList<>(newKeys.size());
+        GpgKeys.getGpgExtIds(db.get(), rsrc.getUser().getAccountId()).toList();
 
-    for (PGPPublicKeyRing keyRing : newKeys) {
+    try (PublicKeyStore store = storeProvider.get()) {
-      PGPPublicKey key = keyRing.getPublicKey();
+      Set<Fingerprint> toRemove = readKeysToRemove(input, existingExtIds);
-      AccountExternalId.Key extIdKey = new AccountExternalId.Key(
+      List<PGPPublicKeyRing> newKeys = readKeysToAdd(input, toRemove);
-          AccountExternalId.SCHEME_GPGKEY,
+      List<AccountExternalId> newExtIds = new ArrayList<>(existingExtIds.size());
-          BaseEncoding.base16().encode(key.getFingerprint()));
+
-      AccountExternalId existing = db.get().accountExternalIds().get(extIdKey);
+      for (PGPPublicKeyRing keyRing : newKeys) {
-      if (existing != null) {
+        PGPPublicKey key = keyRing.getPublicKey();
-        if (!existing.getAccountId().equals(rsrc.getUser().getAccountId())) {
+        AccountExternalId.Key extIdKey = toExtIdKey(key.getFingerprint());
-          throw new ResourceConflictException(
+        AccountExternalId existing = db.get().accountExternalIds().get(extIdKey);
-              "GPG key already associated with another account");
+        if (existing != null) {
+          if (!existing.getAccountId().equals(rsrc.getUser().getAccountId())) {
+            throw new ResourceConflictException(
+                "GPG key already associated with another account");
+          }
+        } else {
+          newExtIds.add(
+              new AccountExternalId(rsrc.getUser().getAccountId(), extIdKey));
         }
-      } else {
-        newExtIds.add(
-            new AccountExternalId(rsrc.getUser().getAccountId(), extIdKey));
       }
-    }
 
-    storeKeys(rsrc, newKeys);
+      storeKeys(rsrc, newKeys, toRemove);
-    if (!newExtIds.isEmpty()) {
+      if (!newExtIds.isEmpty()) {
-      db.get().accountExternalIds().insert(newExtIds);
+        db.get().accountExternalIds().insert(newExtIds);
+      }
+      db.get().accountExternalIds().deleteKeys(Iterables.transform(toRemove,
+          new Function<Fingerprint, AccountExternalId.Key>() {
+            @Override
+            public AccountExternalId.Key apply(Fingerprint fp) {
+              return toExtIdKey(fp.get());
+            }
+          }));
+      return toJson(newKeys, toRemove);
     }
-    return toJson(newKeys);
   }
 
-  private List<PGPPublicKeyRing> readKeys(Input input)
+  private Set<Fingerprint> readKeysToRemove(Input input,
+      List<AccountExternalId> existingExtIds) {
+    if (input.delete == null || input.delete.isEmpty()) {
+      return ImmutableSet.of();
+    }
+    Set<Fingerprint> fingerprints =
+        Sets.newHashSetWithExpectedSize(input.delete.size());
+    for (String id : input.delete) {
+      try {
+        fingerprints.add(new Fingerprint(
+            GpgKeys.parseFingerprint(id, existingExtIds)));
+      } catch (ResourceNotFoundException e) {
+        // Skip removal.
+      }
+    }
+    return fingerprints;
+  }
+
+  private List<PGPPublicKeyRing> readKeysToAdd(Input input,
+      Set<Fingerprint> toRemove)
       throws BadRequestException, IOException {
     if (input.add == null || input.add.isEmpty()) {
       return ImmutableList.of();
@@ -125,15 +163,21 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
         if (objs.size() != 1 || !(objs.get(0) instanceof PGPPublicKeyRing)) {
           throw new BadRequestException("Expected exactly one PUBLIC KEY BLOCK");
         }
-        keyRings.add((PGPPublicKeyRing) objs.get(0));
+        PGPPublicKeyRing keyRing = (PGPPublicKeyRing) objs.get(0);
+        if (toRemove.contains(
+            new Fingerprint(keyRing.getPublicKey().getFingerprint()))) {
+          throw new BadRequestException("Cannot both add and delete key: "
+              + keyToString(keyRing.getPublicKey()));
+        }
+        keyRings.add(keyRing);
       }
     }
     return keyRings;
   }
 
-  private void storeKeys(AccountResource rsrc, List<PGPPublicKeyRing> keyRings)
+  private void storeKeys(AccountResource rsrc, List<PGPPublicKeyRing> keyRings,
-      throws BadRequestException, ResourceConflictException, PGPException,
+      Set<Fingerprint> toRemove) throws BadRequestException,
-      IOException {
+      ResourceConflictException, PGPException, IOException {
     try (PublicKeyStore store = storeProvider.get()) {
       for (PGPPublicKeyRing keyRing : keyRings) {
         PGPPublicKey key = keyRing.getPublicKey();
@@ -145,6 +189,9 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
         }
         store.add(keyRing);
       }
+      for (Fingerprint fp : toRemove) {
+        store.remove(fp.get());
+      }
       CommitBuilder cb = new CommitBuilder();
       PersonIdent committer = serverIdent.get();
       cb.setAuthor(rsrc.getUser().newCommitterIdent(
@@ -156,24 +203,35 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
         case NEW:
         case FAST_FORWARD:
         case FORCED:
+        case NO_CHANGE:
           break;
         default:
           // TODO(dborowitz): Backoff and retry on LOCK_FAILURE.
           throw new ResourceConflictException(
-              "Failed to save public key: " + saveResult);
+              "Failed to save public keys: " + saveResult);
       }
     }
   }
 
+  private final AccountExternalId.Key toExtIdKey(byte[] fp) {
+    return new AccountExternalId.Key(
+        AccountExternalId.SCHEME_GPGKEY,
+        BaseEncoding.base16().encode(fp));
+  }
+
   private static Map<String, GpgKeyInfo> toJson(
-      Collection<PGPPublicKeyRing> keyRings) throws IOException {
+      Collection<PGPPublicKeyRing> keys,
+      Set<Fingerprint> deleted) throws IOException {
     Map<String, GpgKeyInfo> infos =
-        Maps.newHashMapWithExpectedSize(keyRings.size());
+        Maps.newHashMapWithExpectedSize(keys.size() + deleted.size());
-    for (PGPPublicKeyRing keyRing : keyRings) {
+    for (PGPPublicKeyRing keyRing : keys) {
       GpgKeyInfo info = GpgKeys.toJson(keyRing);
       infos.put(info.id, info);
       info.id = null;
     }
+    for (Fingerprint fp : deleted) {
+      infos.put(keyIdToString(fp.getId()), new GpgKeyInfo());
+    }
     return infos;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java

@@ -138,10 +138,11 @@ public class AccountApiImpl implements AccountApi {
   }
 
   @Override
-  public Map<String, GpgKeyInfo> putGpgKeys(List<String> add)
+  public Map<String, GpgKeyInfo> putGpgKeys(List<String> add,
-      throws RestApiException {
+      List<String> delete) throws RestApiException {
     PostGpgKeys.Input in = new PostGpgKeys.Input();
     in.add = add;
+    in.delete = delete;
     try {
       return postGpgKeys.apply(account, in);
     } catch (PGPException | OrmException | IOException e) {

gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java

@@ -21,10 +21,10 @@ import org.eclipse.jgit.util.NB;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 
-class Fingerprint {
+public class Fingerprint {
   private final byte[] fp;
 
-  Fingerprint(byte[] fp) {
+  public Fingerprint(byte[] fp) {
     // Don't bother with defensive copies; PGPPublicKey#getFingerprint() already
     // does so.
     checkArgument(fp.length == 20,
@@ -32,11 +32,11 @@ class Fingerprint {
     this.fp = fp;
   }
 
-  byte[] get() {
+  public byte[] get() {
     return fp;
   }
 
-  boolean equalsBytes(byte[] bytes) {
+  public boolean equalsBytes(byte[] bytes) {
     return Arrays.equals(fp, bytes);
   }
 
@@ -61,7 +61,7 @@ class Fingerprint {
         buf.getShort(), buf.getShort());
   }
 
-  long getId() {
+  public long getId() {
     return ByteBuffer.wrap(fp).getLong(12);
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/TestKey.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.git.gpg;
 
+import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyIdToString;
+
 import com.google.common.collect.ImmutableList;
 
 import org.bouncycastle.bcpg.ArmoredInputStream;
@@ -597,6 +599,10 @@ public class TestKey {
     return getPublicKey().getKeyID();
   }
 
+  public String getKeyIdString() {
+    return keyIdToString(getPublicKey().getKeyID());
+  }
+
   public String getFirstUserId() {
     return (String) getPublicKey().getUserIDs().next();
   }