launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+433778@xxxxxxxxxxxxxxxxxx>]

Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:snap-store-auth-error into launchpad:master.

Commit message:
Display info message when user tries to authorize store upload for snap that isn't registered


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1997565 in Launchpad itself: "snap /+authorize for brand new snap package name results in OOPS"
  https://bugs.launchpad.net/launchpad/+bug/1997565

For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/433778

Previously, that would cause an Internal Server Error

LP: #1997565

Example: https://people.canonical.com/~afedoseev/%231997565/Screenshot_20221129_154226.png
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:snap-store-auth-error into launchpad:master.

diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index 6bde0c8..4c2fbdd 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -1828,6 +1828,26 @@ class TestSnapAuthorizeView(BaseTestSnapView):
             browser.headers["Location"],
         )
 
+    @responses.activate
+    def test_begin_authorization__snap_not_registered(self):
+        snap_url = canonical_url(self.snap)
+        self.pushConfig("snappy", store_url="http://sca.example/";)
+        responses.add("POST", "http://sca.example/dev/api/acl/";, status=404)
+        browser = self.getUserBrowser(
+            url=snap_url + "/+authorize", user=self.snap.owner
+        )
+        browser.getControl("Begin authorization").click()
+        self.assertEqual(snap_url, browser.url)
+        messages = find_tags_by_class(
+            browser.contents, "informational message"
+        )
+        self.assertEqual(1, len(messages))
+        self.assertStartsWith(
+            extract_text(messages[0]),
+            "The requested snap name '{}' is not registered in the "
+            "snap store".format(removeSecurityProxy(self.snap).store_name),
+        )
+
     def test_complete_authorization_missing_discharge_macaroon(self):
         # If the form does not include a discharge macaroon, the "complete"
         # action fails.
diff --git a/lib/lp/snappy/interfaces/snapstoreclient.py b/lib/lp/snappy/interfaces/snapstoreclient.py
index e45e8a9..81cc293 100644
--- a/lib/lp/snappy/interfaces/snapstoreclient.py
+++ b/lib/lp/snappy/interfaces/snapstoreclient.py
@@ -10,6 +10,7 @@ __all__ = [
     "ISnapStoreClient",
     "NeedsRefreshResponse",
     "ScanFailedResponse",
+    "SnapNotFoundResponse",
     "SnapStoreError",
     "UnauthorizedUploadResponse",
     "UploadFailedResponse",
@@ -38,6 +39,10 @@ class BadRequestPackageUploadResponse(SnapStoreError):
     pass
 
 
+class SnapNotFoundResponse(SnapStoreError):
+    pass
+
+
 class UploadFailedResponse(SnapStoreError):
     pass
 
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 35c54d3..e0b4975 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -174,7 +174,10 @@ from lp.snappy.interfaces.snapbase import ISnapBaseSet, NoSuchSnapBase
 from lp.snappy.interfaces.snapbuild import ISnapBuild, ISnapBuildSet
 from lp.snappy.interfaces.snapjob import ISnapRequestBuildsJobSource
 from lp.snappy.interfaces.snappyseries import ISnappyDistroSeriesSet
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
+from lp.snappy.interfaces.snapstoreclient import (
+    ISnapStoreClient,
+    SnapNotFoundResponse,
+)
 from lp.snappy.model.snapbase import SnapBase
 from lp.snappy.model.snapbuild import SnapBuild
 from lp.snappy.model.snapjob import SnapJob
@@ -737,9 +740,18 @@ class Snap(Storm, WebhookTargetMixin):
                 "name."
             )
         snap_store_client = getUtility(ISnapStoreClient)
-        root_macaroon_raw = snap_store_client.requestPackageUploadPermission(
-            self.store_series, self.store_name
-        )
+        try:
+            root_macaroon_raw = (
+                snap_store_client.requestPackageUploadPermission(
+                    self.store_series, self.store_name
+                )
+            )
+        except SnapNotFoundResponse:
+            raise CannotAuthorizeStoreUploads(
+                "The requested snap name '{}' is not registered in the snap "
+                "store. You can register it at "
+                "https://snapcraft.io/register-snap".format(self.store_name)
+            )
         sso_caveats = self.extractSSOCaveats(
             Macaroon.deserialize(root_macaroon_raw)
         )
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index 387c678..536ba17 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -34,6 +34,7 @@ from lp.snappy.interfaces.snapstoreclient import (
     ISnapStoreClient,
     NeedsRefreshResponse,
     ScanFailedResponse,
+    SnapNotFoundResponse,
     UnauthorizedUploadResponse,
     UploadFailedResponse,
     UploadNotScannedYetResponse,
@@ -245,6 +246,8 @@ class SnapStoreClient:
                 raise BadRequestPackageUploadResponse(response.text)
             return response_data["macaroon"]
         except requests.HTTPError as e:
+            if e.response.status_code == 404:
+                raise cls._makeSnapStoreError(SnapNotFoundResponse, e)
             raise cls._makeSnapStoreError(BadRequestPackageUploadResponse, e)
         finally:
             timeline_action.finish()
diff --git a/lib/lp/snappy/tests/test_snapstoreclient.py b/lib/lp/snappy/tests/test_snapstoreclient.py
index 91dbf15..3dc546a 100644
--- a/lib/lp/snappy/tests/test_snapstoreclient.py
+++ b/lib/lp/snappy/tests/test_snapstoreclient.py
@@ -43,6 +43,7 @@ from lp.snappy.interfaces.snapstoreclient import (
     BadScanStatusResponse,
     ISnapStoreClient,
     ScanFailedResponse,
+    SnapNotFoundResponse,
     UnauthorizedUploadResponse,
     UploadFailedResponse,
     UploadNotScannedYetResponse,
@@ -415,7 +416,7 @@ class TestSnapStoreClient(TestCaseWithFactory):
         snappy_series = self.factory.makeSnappySeries()
         responses.add("POST", "http://sca.example/dev/api/acl/";, status=404)
         self.assertRaisesWithContent(
-            BadRequestPackageUploadResponse,
+            SnapNotFoundResponse,
             "404 Client Error: Not Found",
             self.client.requestPackageUploadPermission,
             snappy_series,