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
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 5e93c7e..e64fa7e 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -714,7 +714,7 @@
"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 @@
}
----
-[[add-gpg-keys]]
-=== Add GPG Keys
+[[add-delete-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
-link:#gpg-key-input[GpgKeyInput] entity. Each GPG key is provided in
+The changes must be provided in the request body as a
+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 @@
{
"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
-link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID.
+As a response, the modified GPG keys are returned as a map of
+link:#gpg-key-info[GpgKeyInfo] entities, keyed by ID. Deleted keys are
+represented by an empty object.
.Response
----
@@ -794,6 +798,7 @@
],
"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 @@
|========================
|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.
-|`user_ids` ||
+|`fingerprint`|Not set for deleted keys|The 40-char (plus spaces) hex GPG key fingerprint.
+|`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 @@
|========================
|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]]
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
index 0ff4709..d978c70 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
@@ -162,6 +162,10 @@
return old;
}
+ public Context get() {
+ return current.get();
+ }
+
public Context disableDb() {
Context old = current.get();
SchemaFactory<ReviewDb> sf = new SchemaFactory<ReviewDb>() {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 189a283..e1b98af 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -18,10 +18,13 @@
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.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 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 @@
@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()
- .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
- .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());
+ assertKeyMapContains(key, addGpgKey(key.getPublicKeyArmored()));
+ assertKeys(key);
setApiUser(user);
exception.expect(ResourceNotFoundException.class);
@@ -213,19 +212,15 @@
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()
- .putGpgKeys(ImmutableList.of(armor(pk)))
- .get(id);
+ GpgKeyInfo info = addGpgKey(armor(pk)).get(id);
assertThat(info.userIds).hasSize(2);
assertIteratorSize(2, getOnlyKeyFromStore(key).getUserIDs());
pk = PGPPublicKey.removeCertification(pk, "foo:myId");
- info = gApi.accounts().self()
- .putGpgKeys(ImmutableList.of(armor(pk)))
- .get(id);
+ info = addGpgKey(armor(pk)).get(id);
assertThat(info.userIds).hasSize(1);
assertIteratorSize(1, getOnlyKeyFromStore(key).getUserIDs());
}
@@ -240,17 +235,12 @@
db.accountExternalIds().insert(Collections.singleton(extId));
TestKey key = TestKey.key5();
- String id = keyIdToString(key.getKeyId());
- gApi.accounts().self()
- .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
- .get(id);
+ addGpgKey(key.getPublicKeyArmored());
setApiUser(user);
exception.expect(ResourceConflictException.class);
exception.expectMessage("GPG key already associated with another account");
- gApi.accounts().self()
- .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()))
- .get(id);
+ addGpgKey(key.getPublicKeyArmored());
}
@Test
@@ -262,37 +252,62 @@
PushCertificateIdent.parse(key.getFirstUserId()).getEmailAddress());
toAdd.add(key.getPublicKeyArmored());
}
- gApi.accounts().self().putGpgKeys(toAdd);
-
- 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);
- }
+ gApi.accounts().self().putGpgKeys(toAdd, ImmutableList.<String> of());
+ assertKeys(keys);
}
@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()
- .putGpgKeys(ImmutableList.of(key.getPublicKeyArmored()));
- assertKeyEquals(key, gApi.accounts().self().gpgKey(id).get());
+ addGpgKey(key.getPublicKeyArmored());
+ assertKeys(key);
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 @@
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 @@
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());
+ }
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
index 0d10316..a356ab6 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
@@ -30,7 +30,8 @@
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 @@
}
@Override
- public Map<String, GpgKeyInfo> putGpgKeys(List<String> add)
- throws RestApiException {
+ public Map<String, GpgKeyInfo> putGpgKeys(List<String> add,
+ List<String> remove) throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GpgKeys.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GpgKeys.java
index 84bd17f..846db54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GpgKeys.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GpgKeys.java
@@ -95,23 +95,7 @@
throw new ResourceNotFoundException(id);
}
- byte[] fp = null;
- 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);
- }
-
+ byte[] fp = parseFingerprint(id.get(), getGpgExtIds(parent));
try (PublicKeyStore store = storeProvider.get()) {
long keyId = keyId(fp);
for (PGPPublicKeyRing keyRing : store.get(keyId)) {
@@ -125,6 +109,33 @@
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 @@
}
@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))
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostGpgKeys.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PostGpgKeys.java
index ee6a746..776f983 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostGpgKeys.java
+++ b/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.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.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 @@
ResourceConflictException, PGPException, OrmException, IOException {
GpgKeys.checkEnabled();
- List<PGPPublicKeyRing> newKeys = readKeys(input);
- List<AccountExternalId> newExtIds = new ArrayList<>(newKeys.size());
+ List<AccountExternalId> existingExtIds =
+ GpgKeys.getGpgExtIds(db.get(), rsrc.getUser().getAccountId()).toList();
- for (PGPPublicKeyRing keyRing : newKeys) {
- PGPPublicKey key = keyRing.getPublicKey();
- AccountExternalId.Key extIdKey = new AccountExternalId.Key(
- AccountExternalId.SCHEME_GPGKEY,
- BaseEncoding.base16().encode(key.getFingerprint()));
- AccountExternalId existing = db.get().accountExternalIds().get(extIdKey);
- if (existing != null) {
- if (!existing.getAccountId().equals(rsrc.getUser().getAccountId())) {
- throw new ResourceConflictException(
- "GPG key already associated with another account");
+ try (PublicKeyStore store = storeProvider.get()) {
+ Set<Fingerprint> toRemove = readKeysToRemove(input, existingExtIds);
+ List<PGPPublicKeyRing> newKeys = readKeysToAdd(input, toRemove);
+ List<AccountExternalId> newExtIds = new ArrayList<>(existingExtIds.size());
+
+ for (PGPPublicKeyRing keyRing : newKeys) {
+ PGPPublicKey key = keyRing.getPublicKey();
+ AccountExternalId.Key extIdKey = toExtIdKey(key.getFingerprint());
+ AccountExternalId existing = db.get().accountExternalIds().get(extIdKey);
+ 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);
- if (!newExtIds.isEmpty()) {
- db.get().accountExternalIds().insert(newExtIds);
+ storeKeys(rsrc, newKeys, toRemove);
+ if (!newExtIds.isEmpty()) {
+ 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 @@
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)
- throws BadRequestException, ResourceConflictException, PGPException,
- IOException {
+ private void storeKeys(AccountResource rsrc, List<PGPPublicKeyRing> keyRings,
+ Set<Fingerprint> toRemove) throws BadRequestException,
+ ResourceConflictException, PGPException, IOException {
try (PublicKeyStore store = storeProvider.get()) {
for (PGPPublicKeyRing keyRing : keyRings) {
PGPPublicKey key = keyRing.getPublicKey();
@@ -145,6 +189,9 @@
}
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 @@
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());
- for (PGPPublicKeyRing keyRing : keyRings) {
+ Maps.newHashMapWithExpectedSize(keys.size() + deleted.size());
+ 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;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
index fbc0774..85e519b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
@@ -138,10 +138,11 @@
}
@Override
- public Map<String, GpgKeyInfo> putGpgKeys(List<String> add)
- throws RestApiException {
+ public Map<String, GpgKeyInfo> putGpgKeys(List<String> add,
+ 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) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java
index 6209a14..0649b93 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java
@@ -21,10 +21,10 @@
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 @@
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 @@
buf.getShort(), buf.getShort());
}
- long getId() {
+ public long getId() {
return ByteBuffer.wrap(fp).getLong(12);
}
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/TestKey.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/TestKey.java
index 483964a..321f01b 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/TestKey.java
+++ b/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 @@
return getPublicKey().getKeyID();
}
+ public String getKeyIdString() {
+ return keyIdToString(getPublicKey().getKeyID());
+ }
+
public String getFirstUserId() {
return (String) getPublicKey().getUserIDs().next();
}
