launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:snap-privacy-is-never-stale into launchpad:master.

Commit message:
Allow private snaps to be marked as stale

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1927950 in Launchpad itself: "private snap recipes do not trigger on branch update"
  https://bugs.launchpad.net/launchpad/+bug/1927950

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/402557

Use the standard `check_permissions` flags to allow system scripts to avoid visibility checks and correctly mark private snaps as stale.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:snap-privacy-is-never-stale into launchpad:master.

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index f6de617..452bf4e 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -685,7 +685,10 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
 
     def markSnapsStale(self):
         """See `IBranch`."""
-        for snap in getUtility(ISnapSet).findByBranch(self):
+        snaps = getUtility(ISnapSet).findByBranch(
+            self,
+            check_permissions=False)
+        for snap in snaps:
             snap.is_stale = True
 
     def addToLaunchBag(self, launchbag):
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index ffe0749..2ba9491 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1267,7 +1267,11 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
     def markSnapsStale(self, paths):
         """See `IGitRepository`."""
         snap_set = getUtility(ISnapSet)
-        for snap in snap_set.findByGitRepository(self, paths=paths):
+        snaps = snap_set.findByGitRepository(
+            self,
+            paths=paths,
+            check_permissions=False)
+        for snap in snaps:
             snap.is_stale = True
 
     def _markProposalMerged(self, proposal, merged_revision_id, logger=None):
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index a29c474..008c640 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -168,6 +168,7 @@ from lp.services.utils import seconds_since_epoch
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.webapp.snapshot import notify_modified
+from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -2758,6 +2759,16 @@ class TestGitRepositoryMarkSnapsStale(TestCaseWithFactory):
             {ref.path: {"sha1": "0" * 40, "type": GitObjectType.COMMIT}})
         self.assertFalse(snap.is_stale)
 
+    def test_private_snap(self):
+        # A private snap should be able to be marked stale
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        [ref] = self.factory.makeGitRefs()
+        snap = self.factory.makeSnap(git_ref=ref, private=True)
+        removeSecurityProxy(snap).is_stale = False
+        ref.repository.createOrUpdateRefs(
+            {ref.path: {"sha1": "0" * 40, "type": GitObjectType.COMMIT}})
+        self.assertTrue(snap.is_stale)
+
 
 class TestGitRepositoryFork(TestCaseWithFactory):
 
diff --git a/lib/lp/code/tests/test_branch.py b/lib/lp/code/tests/test_branch.py
index c95515e..034dbe3 100644
--- a/lib/lp/code/tests/test_branch.py
+++ b/lib/lp/code/tests/test_branch.py
@@ -4,6 +4,7 @@
 """Unit tests for methods of Branch and BranchSet."""
 
 from __future__ import absolute_import, print_function, unicode_literals
+from os import remove
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -23,7 +24,7 @@ from lp.testing import (
     run_with_login,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import DatabaseFunctionalLayer, ZopelessDatabaseLayer
 
 
 class PermissionTest(TestCaseWithFactory):
diff --git a/lib/lp/codehosting/scanner/tests/test_bzrsync.py b/lib/lp/codehosting/scanner/tests/test_bzrsync.py
index 3fb56df..931af4e 100644
--- a/lib/lp/codehosting/scanner/tests/test_bzrsync.py
+++ b/lib/lp/codehosting/scanner/tests/test_bzrsync.py
@@ -58,6 +58,7 @@ from lp.services.database.interfaces import IStore
 from lp.services.features.testing import FeatureFixture
 from lp.services.osutils import override_environ
 from lp.services.webhooks.testing import LogsScheduledWebhooks
+from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import (
     dbuser,
@@ -783,6 +784,17 @@ class TestMarkSnapsStale(BzrSyncTestCase):
         self.makeBzrSync(self.db_branch).syncBranchAndClose()
         self.assertFalse(snap.is_stale)
 
+    @run_as_db_user(config.launchpad.dbuser)
+    def test_mark_private_snap_stale(self):
+        # Private snaps should be correctly marked as stale.
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        snap = self.factory.makeSnap(
+            branch=self.db_branch, private=True)
+        removeSecurityProxy(snap).is_stale = False
+        switch_dbuser("branchscanner")
+        self.makeBzrSync(self.db_branch).syncBranchAndClose()
+        self.assertTrue(snap.is_stale)
+
 
 class TestTriggerWebhooks(BzrSyncTestCase):
     """Test triggering of webhooks."""
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 9d82962..f2ce156 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -1003,10 +1003,10 @@ class ISnapSet(Interface):
             this user; otherwise, only return publicly-visible packages.
         """
 
-    def findByBranch(branch):
+    def findByBranch(branch, check_permissions=True):
         """Return all snap packages for the given Bazaar branch."""
 
-    def findByGitRepository(repository, paths=None):
+    def findByGitRepository(repository, paths=None, check_permissions=True):
         """Return all snap packages for the given Git repository.
 
         :param repository: An `IGitRepository`.
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 1a108b0..81aaf0e 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1494,20 +1494,24 @@ class SnapSet:
         return snaps_for_project.union(
             _getSnaps(bzr_collection)).union(_getSnaps(git_collection))
 
-    def findByBranch(self, branch, visible_by_user=None):
+    def findByBranch(self, branch, visible_by_user=None,
+                     check_permissions=True):
         """See `ISnapSet`."""
+        query = [Snap.branch == branch]
+        if check_permissions:
+            query.append(get_snap_privacy_filter(visible_by_user))
         return IStore(Snap).find(
             Snap,
-            Snap.branch == branch,
-            get_snap_privacy_filter(visible_by_user))
+            *query)
 
     def findByGitRepository(self, repository, paths=None,
-                            visible_by_user=None):
+                            visible_by_user=None, check_permissions=True):
         """See `ISnapSet`."""
         clauses = [Snap.git_repository == repository]
         if paths is not None:
             clauses.append(Snap.git_path.is_in(paths))
-        clauses.append(get_snap_privacy_filter(visible_by_user))
+        if check_permissions:
+            clauses.append(get_snap_privacy_filter(visible_by_user))
         return IStore(Snap).find(Snap, *clauses)
 
     def findByGitRef(self, ref, visible_by_user=None):