launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-edit-better-reauth into lp:launchpad.

Commit message:
Reauthorize store uploads in Snap:+edit if store_upload is newly-enabled.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1603755 in Launchpad itself: "enabling Snap.store_upload (and no other changes) does not authorize store upload"
  https://bugs.launchpad.net/launchpad/+bug/1603755

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-edit-better-reauth/+merge/300303

Reauthorize store uploads in Snap:+edit if store_upload is newly-enabled.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-edit-better-reauth into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2016-07-16 07:46:23 +0000
+++ lib/lp/snappy/browser/snap.py	2016-07-18 10:01:08 +0000
@@ -567,6 +567,8 @@
         if (not store_upload or
                 store_distro_series is None or store_name is None):
             return False
+        if not self.context.store_upload:
+            return True
         if store_distro_series.snappy_series != self.context.store_series:
             return True
         if store_name != self.context.store_name:

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2016-07-16 07:46:23 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2016-07-18 10:01:08 +0000
@@ -63,6 +63,7 @@
     SNAP_TESTING_FLAGS,
     SnapPrivateFeatureDisabled,
     )
+from lp.snappy.interfaces.snappyseries import ISnappyDistroSeriesSet
 from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
 from lp.testing import (
     admin_logged_in,
@@ -439,7 +440,7 @@
 
     def test_create_new_snap_display_processors(self):
         branch = self.factory.makeAnyBranch()
-        distroseries = self.setUpDistroSeries()
+        self.setUpDistroSeries()
         browser = self.getViewBrowser(
             branch, view_name="+new-snap", user=self.person)
         processors = browser.getControl(name="field.processors")
@@ -466,7 +467,7 @@
 
     def test_create_new_snap_processors(self):
         branch = self.factory.makeAnyBranch()
-        distroseries = self.setUpDistroSeries()
+        self.setUpDistroSeries()
         browser = self.getViewBrowser(
             branch, view_name="+new-snap", user=self.person)
         processors = browser.getControl(name="field.processors")
@@ -859,6 +860,47 @@
         login_person(self.person)
         self.assertSnapProcessors(snap, ["386", "armhf"])
 
+    def assertNeedStoreReauth(self, expected, initial_kwargs, data):
+        initial_kwargs.setdefault("store_upload", True)
+        initial_kwargs.setdefault("store_series", self.snappyseries)
+        initial_kwargs.setdefault("store_name", u"one")
+        snap = self.factory.makeSnap(
+            registrant=self.person, owner=self.person,
+            distroseries=self.distroseries, **initial_kwargs)
+        view = create_initialized_view(snap, "+edit", principal=self.person)
+        data.setdefault("store_upload", snap.store_upload)
+        data.setdefault("store_distro_series", snap.store_distro_series)
+        data.setdefault("store_name", snap.store_name)
+        self.assertEqual(expected, view._needStoreReauth(data))
+
+    def test__needStoreReauth_no_change(self):
+        # If the user didn't change any store settings, no reauthorization
+        # is needed.
+        self.assertNeedStoreReauth(False, {}, {})
+
+    def test__needStoreReauth_different_series(self):
+        # Changing the store series requires reauthorization.
+        with admin_logged_in():
+            new_snappyseries = self.factory.makeSnappySeries(
+                usable_distro_series=[self.distroseries])
+        sds = getUtility(ISnappyDistroSeriesSet).getByBothSeries(
+            new_snappyseries, self.distroseries)
+        self.assertNeedStoreReauth(True, {}, {"store_distro_series": sds})
+
+    def test__needStoreReauth_different_name(self):
+        # Changing the store name requires reauthorization.
+        self.assertNeedStoreReauth(True, {}, {"store_name": u"two"})
+
+    def test__needStoreReauth_enable_upload(self):
+        # Enabling store upload requires reauthorization.  (This can happen
+        # on its own if both store_series and store_name were set to begin
+        # with, which is especially plausible for Git-based snap packages,
+        # or if this option is disabled and then re-enabled.  In the latter
+        # case, we can't tell if store_series or store_name were also
+        # changed in between, so reauthorizing is the conservative course.)
+        self.assertNeedStoreReauth(
+            True, {"store_upload": False}, {"store_upload": True})
+
     def test_edit_store_upload(self):
         # Changing store upload settings on a snap sets all the appropriate
         # fields and redirects to SSO for reauthorization.