launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/lp-signing:allow-duplicate-inject-call into lp-signing:master.

Commit message:
Making /inject idempotent by avoiding 500 errors when adding authorization for a client to use a given key.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/lp-signing/+git/lp-signing/+merge/383961
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/lp-signing:allow-duplicate-inject-call into lp-signing:master.

diff --git a/lp_signing/model/key.py b/lp_signing/model/key.py
index 451b3b2..d72449a 100644
--- a/lp_signing/model/key.py
+++ b/lp_signing/model/key.py
@@ -378,10 +378,17 @@ class Key(Storm):
                 KeyAuthorization, KeyAuthorization.key == self)]
 
     def addAuthorization(self, client):
-        """Authorize a client to sign with this key.
+        """Authorize a client to sign with this key. If the client is
+        already authorized, this method does nothing.
 
         :param client: A `Client`.
         """
+        existing_auth = store.find(
+            KeyAuthorization,
+            KeyAuthorization.key == self,
+            KeyAuthorization.client == client).one()
+        if existing_auth:
+            return
         key_authorization = KeyAuthorization(self, client)
         store.add(key_authorization)
 
diff --git a/lp_signing/model/tests/test_key.py b/lp_signing/model/tests/test_key.py
index 8b7d1f1..8b945ec 100644
--- a/lp_signing/model/tests/test_key.py
+++ b/lp_signing/model/tests/test_key.py
@@ -330,6 +330,18 @@ class TestKey(TestCase):
         self.assertTrue(key.isAuthorized(clients[0]))
         self.assertTrue(key.isAuthorized(clients[1]))
 
+    def test_addAuthorization_duplicated(self):
+        client = factory.create_client()
+        key = factory.create_key()
+        self.assertFalse(key.isAuthorized(client))
+
+        key.addAuthorization(client)
+        self.assertTrue(key.isAuthorized(client))
+
+        # Try again, same client.
+        key.addAuthorization(client)
+        self.assertTrue(key.isAuthorized(client))
+
     def test_removeAuthorization(self):
         key = factory.create_key()
         clients = [factory.create_client() for _ in range(2)]
diff --git a/lp_signing/tests/test_webapi.py b/lp_signing/tests/test_webapi.py
index 19e66bf..2ad027a 100644
--- a/lp_signing/tests/test_webapi.py
+++ b/lp_signing/tests/test_webapi.py
@@ -1530,6 +1530,55 @@ class TestInjectView(TestCase):
             fingerprint=resp.json["fingerprint"],
             authorizations=[self.clients[0], self.clients[1]]))
 
+    def test_inject_duplicate_key_from_same_client(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)
+        private_key = bytes(private_key)
+        public_key = bytes(public_key)
+        client = self.clients[0]
+
+        resp = self.post_inject({
+            "key-type": "FIT",
+            "private-key": base64.b64encode(private_key).decode("UTF-8"),
+            "public-key": base64.b64encode(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=[client]))
+
+        # Regenerate nonces (consumed by the previous request).
+        self.nonces = [Nonce.generate().nonce for _ in range(2)]
+        store.commit()
+
+        resp = self.post_inject({
+            "key-type": "FIT",
+            "private-key": base64.b64encode(private_key).decode("UTF-8"),
+            "public-key": base64.b64encode(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=[client]))
+
     def test_inject_update_private_public_material(self):
         common_name = Key._generateKeyCommonName(
             "PPA test-owner test-archive", 'FIT')