launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/lp-signing:inject-duplicate-keys into lp-signing:master.

Commit message:
Authorize multiple clients in the inject endpoint

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/lp-signing/+git/lp-signing/+merge/379390

Added ability to authorize multiple clients for the same key 
in the inject endpoint. If the client already injected a key into 
the Signing Service we return the same (200 and fingerprint) as if
the key was not there but we don't actually perform any operations.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/lp-signing:inject-duplicate-keys into lp-signing:master.

diff --git a/lp_signing/model/key.py b/lp_signing/model/key.py
index cb2097c..c3a04e7 100644
--- a/lp_signing/model/key.py
+++ b/lp_signing/model/key.py
@@ -288,9 +288,12 @@ class Key(Storm):
             raise KeyGenerationError.single(
                     f"Failed to get fingerprint of new key: {e}")
         _log.info("Injected new key with fingerprint %s", fingerprint)
-
-        return cls.new(key_type, fingerprint, private_key, public_key,
-                       created_at)
+        try:
+            key = Key.getByTypeAndFingerprint(key_type, fingerprint)
+            return key
+        except NoSuchKey:
+            return cls.new(key_type, fingerprint, private_key, public_key,
+                           created_at)
 
     @property
     def authorizations(self):
diff --git a/lp_signing/tests/test_webapi.py b/lp_signing/tests/test_webapi.py
index 23dc0e5..2da68ad 100644
--- a/lp_signing/tests/test_webapi.py
+++ b/lp_signing/tests/test_webapi.py
@@ -1094,6 +1094,12 @@ class TestInjectView(TestCase):
         self.private_key = PrivateKey.generate()
         self.client.registerPublicKey(self.private_key.public_key)
         self.nonce = Nonce.generate().nonce
+        self.second_client = factory.create_client()
+        self.second_private_key = PrivateKey.generate()
+        self.second_client.registerPublicKey(
+            self.second_private_key.public_key)
+        self.second_nonce = Nonce.generate().nonce
+
         store.commit()
 
     def test_unauthenticated(self):
@@ -1173,6 +1179,11 @@ class TestInjectView(TestCase):
             "/inject", json_data=json_data,
             private_key=self.private_key, nonce=self.nonce)
 
+    def second_post_inject(self, json_data=None):
+        return self.fixture.second_client.post(
+            "/inject", json_data=json_data,
+            private_key=self.second_private_key, nonce=self.second_nonce)
+
     def test_missing_key_type(self):
         resp = self.post_inject({"private-key": "",
                                  "public-key": "",
@@ -1494,3 +1505,56 @@ class TestInjectView(TestCase):
             r"Command .*'-fingerprint'.* returned non-zero exit status 1")
         self.assertThat(resp, HasAPIError(MatchesRegex(error_re), 500))
         self.assertNonceConsumed()
+
+    def test_inject_duplicate_key_different_clients(self):
+        common_name = Key._generateKeyCommonName(
+            "PPA test-owner test-archive", 'FIT')
+        with TemporaryDirectory() as tmp:
+            private_key, public_key = Key._generateKeyCertPair(
+                Path(tmp), KeyType.FIT, common_name)
+
+        resp = self.post_inject(
+            {
+                "key-type": "FIT",
+                "private-key": base64.b64encode(
+                    bytes(private_key)).decode("UTF-8"),
+                "public-key": base64.b64encode(
+                    bytes(public_key)).decode("UTF-8"),
+                "created-at": (datetime.utcnow()).isoformat(),
+                "description": "PPA test-owner test-archive",
+                })
+        self.assertThat(resp, IsJSONResponse(
+            MatchesDict({
+                "fingerprint": HasLength(40),
+                }),
+            expected_status=200))
+        self.assertNonceConsumed()
+        key = Key.getByTypeAndFingerprint(
+            KeyType.FIT, resp.json["fingerprint"])
+        self.assertThat(key, MatchesStructure.byEquality(
+            fingerprint=resp.json["fingerprint"],
+            authorizations=[self.client]))
+
+        resp = self.second_post_inject(
+            {
+                "key-type": "FIT",
+                "private-key": base64.b64encode(
+                    bytes(private_key)).decode("UTF-8"),
+                "public-key": base64.b64encode(
+                    bytes(public_key)).decode("UTF-8"),
+                "timestamp": str(datetime.utcnow()),
+                "created-at": (datetime.utcnow()).isoformat(),
+                "description": "PPA test-owner test-archive",
+                })
+        self.assertThat(resp, IsJSONResponse(
+            MatchesDict({
+                "fingerprint": HasLength(40),
+                }),
+            expected_status=200))
+        self.assertNonceConsumed()
+
+        key = Key.getByTypeAndFingerprint(
+            KeyType.FIT, resp.json["fingerprint"])
+        self.assertThat(key, MatchesStructure.byEquality(
+            fingerprint=resp.json["fingerprint"],
+            authorizations=[self.client, self.second_client]))
diff --git a/lp_signing/tests/testfixtures.py b/lp_signing/tests/testfixtures.py
index 433b0b8..333a54e 100644
--- a/lp_signing/tests/testfixtures.py
+++ b/lp_signing/tests/testfixtures.py
@@ -117,6 +117,7 @@ class AppFixture(Fixture):
         self.app.testing = True
         self.app.test_client_class = TestClient
         self.client = self.app.test_client()
+        self.second_client = self.app.test_client()
         app_context = self.app.app_context()
         app_context.push()
         self.addCleanup(app_context.pop)