diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index 5e93c7e2db..e64fa7ed74 100644 --- a/Documentation/rest-api-accounts.txt +++ b/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 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 @@ 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 -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 @@ 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. -|`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 @@ 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]] 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 0ff4709bc2..d978c70ffa 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 @@ public class AcceptanceTestRequestScope { return old; } + public Context get() { + return current.get(); + } + public Context disableDb() { Context old = current.get(); SchemaFactory sf = new SchemaFactory() { 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 189a283b6d..e1b98af446 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.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() - .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 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() - .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 @@ public class AccountIT extends AbstractDaemonTest { 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 @@ public class AccountIT extends AbstractDaemonTest { PushCertificateIdent.parse(key.getFirstUserId()).getEmailAddress()); toAdd.add(key.getPublicKeyArmored()); } - gApi.accounts().self().putGpgKeys(toAdd); - - Map 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. 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 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 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 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 expectedKeys) throws Exception { + // Check via API. + FluentIterable expected = FluentIterable.from(expectedKeys); + Map keyMap = gApi.accounts().self().listGpgKeys(); + assertThat(keyMap.keySet()) + .named("keys returned by listGpgKeys()") + .containsExactlyElementsIn( + expected.transform(new Function() { + @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() { + @Override + public String apply(AccountExternalId in) { + return in.getSchemeRest(); + } + })) + .named("external IDs in database") + .containsExactlyElementsIn( + expected.transform(new Function() { + @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 addGpgKey(String armored) throws Exception { + return gApi.accounts().self().putGpgKeys( + ImmutableList.of(armored), + ImmutableList. 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 0d10316eb9..a356ab6bd6 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 @@ public interface AccountApi { void addEmail(EmailInput input) throws RestApiException; Map listGpgKeys() throws RestApiException; - Map putGpgKeys(List add) throws RestApiException; + Map putGpgKeys(List add, List remove) + throws RestApiException; GpgKeyApi gpgKey(String id) throws RestApiException; /** @@ -59,8 +60,8 @@ public interface AccountApi { } @Override - public Map putGpgKeys(List add) - throws RestApiException { + public Map putGpgKeys(List add, + List 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 84bd17f75c..846db54c5a 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 @@ public class GpgKeys implements 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 @@ public class GpgKeys implements throw new ResourceNotFoundException(id); } + static byte[] parseFingerprint(String str, + Iterable 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> views() { return views; @@ -167,7 +178,7 @@ public class GpgKeys implements } @VisibleForTesting - public static Iterable getGpgExtIds(ReviewDb db, + public static FluentIterable 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 ee6a746fff..776f9831c8 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.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 { public static class Input { public List add; + public List delete; } private final Provider serverIdent; @@ -84,34 +92,64 @@ public class PostGpgKeys implements RestModifyView { ResourceConflictException, PGPException, OrmException, IOException { GpgKeys.checkEnabled(); - List newKeys = readKeys(input); - List newExtIds = new ArrayList<>(newKeys.size()); + List 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 toRemove = readKeysToRemove(input, existingExtIds); + List newKeys = readKeysToAdd(input, toRemove); + List 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() { + @Override + public AccountExternalId.Key apply(Fingerprint fp) { + return toExtIdKey(fp.get()); + } + })); + return toJson(newKeys, toRemove); } - return toJson(newKeys); } - private List readKeys(Input input) + private Set readKeysToRemove(Input input, + List existingExtIds) { + if (input.delete == null || input.delete.isEmpty()) { + return ImmutableSet.of(); + } + Set 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 readKeysToAdd(Input input, + Set toRemove) throws BadRequestException, IOException { if (input.add == null || input.add.isEmpty()) { return ImmutableList.of(); @@ -125,15 +163,21 @@ public class PostGpgKeys implements RestModifyView { 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 keyRings) - throws BadRequestException, ResourceConflictException, PGPException, - IOException { + private void storeKeys(AccountResource rsrc, List keyRings, + Set toRemove) throws BadRequestException, + 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 { } 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 { 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 toJson( - Collection keyRings) throws IOException { + Collection keys, + Set deleted) throws IOException { Map 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 fbc07742b6..85e519bdf2 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 @@ public class AccountApiImpl implements AccountApi { } @Override - public Map putGpgKeys(List add) - throws RestApiException { + public Map putGpgKeys(List add, + List 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 6209a1477f..0649b93f1b 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 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); } } 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 483964a097..321f01bba9 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 @@ public class TestKey { return getPublicKey().getKeyID(); } + public String getKeyIdString() { + return keyIdToString(getPublicKey().getKeyID()); + } + public String getFirstUserId() { return (String) getPublicKey().getUserIDs().next(); }