Merge ~pappacena/launchpad:snap-pillar-reconcile-access into launchpad:master

[Thiago F. Pappacena]

Adding reconcile for snap access policy when changing Snap privacy or project

[Colin Watson (cjwatson)]

review: Approve

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
index 2ac9785..f434bbf 100644
--- a/lib/lp/snappy/browser/snap.py
+++ b/lib/lp/snappy/browser/snap.py
@@ -758,6 +758,13 @@ class SnapAdminView(BaseSnapEditView):
                     'information_type',
                     'You do not have permission to create private snaps.')
 
+    def updateContextFromData(self, data, context=None, notify_modified=True):
+        if 'project' in data:
+            project = data.pop('project')
+            self.context.setProject(project)
+        super(SnapAdminView, self).updateContextFromData(
+            data, context, notify_modified)
+
 
 class SnapEditView(BaseSnapEditView, EnableProcessorsMixin):
     """View for editing snap packages."""
diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index c5b2f2a..10caef9 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -56,7 +56,10 @@ from lp.code.tests.helpers import (
     BranchHostingFixture,
     GitHostingFixture,
     )
-from lp.registry.enums import PersonVisibility
+from lp.registry.enums import (
+    BranchSharingPolicy,
+    PersonVisibility,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
@@ -713,13 +716,17 @@ class TestSnapAdminView(BaseTestSnapView):
         commercial_admin = self.factory.makePerson(
             member_of=[getUtility(ILaunchpadCelebrities).commercial_admin])
         login_person(self.person)
+        project = self.factory.makeProduct(name="my-project")
+        with person_logged_in(project.owner):
+            project.information_type = InformationType.PROPRIETARY
         snap = self.factory.makeSnap(registrant=self.person)
-        project = self.factory.makeProduct(name='my-project')
         self.assertTrue(snap.require_virtualized)
         self.assertIsNone(snap.project)
         self.assertFalse(snap.private)
         self.assertTrue(snap.allow_internet)
 
+        self.factory.makeAccessPolicy(
+            pillar=project, type=InformationType.PRIVATESECURITY)
         private = InformationType.PRIVATESECURITY.name
         browser = self.getViewBrowser(snap, user=commercial_admin)
         browser.getLink("Administer snap package").click()
@@ -756,8 +763,12 @@ class TestSnapAdminView(BaseTestSnapView):
         login_person(self.person)
         team = self.factory.makeTeam(
             owner=self.person, visibility=PersonVisibility.PRIVATE)
+        project = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC,
+            branch_sharing_policy=BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
         snap = self.factory.makeSnap(
-            registrant=self.person, owner=team, private=True)
+            registrant=self.person, owner=team, project=project,
+            information_type=InformationType.PRIVATESECURITY)
         # Note that only LP admins or, in this case, commercial_admins
         # can reach this snap because it's owned by a private team.
         commercial_admin = self.factory.makePerson(
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index df95533..31708c3 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -571,6 +571,9 @@ class ISnapView(Interface):
         # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
         value_type=Reference(schema=Interface), readonly=True)))
 
+    def visibleByUser(user):
+        """Can the specified user see this snap recipe?"""
+
     def getAllowedInformationTypes(user):
         """Get a list of acceptable `InformationType`s for this snap recipe.
 
@@ -839,6 +842,9 @@ class ISnapEditableAttributes(IHasOwner):
             "'2.1/stable/fix-123', '2.1/stable', 'stable/fix-123', or "
             "'stable'.")))
 
+    def setProject(project):
+        """Set the pillar project of this snap recipe."""
+
 
 class ISnapAdminAttributes(Interface):
     """`ISnap` attributes that can be edited by admins.
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 20dbf3d..52e2d33 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -121,7 +121,10 @@ from lp.registry.interfaces.role import (
     IHasOwner,
     IPersonRoles,
     )
-from lp.registry.model.accesspolicy import AccessPolicyGrant
+from lp.registry.model.accesspolicy import (
+    AccessPolicyGrant,
+    reconcile_access_for_artifact,
+    )
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.series import ACTIVE_STATUSES
 from lp.registry.model.teammembership import TeamParticipation
@@ -420,8 +423,11 @@ class Snap(Storm, WebhookTargetMixin):
         # Set the information type first so that other validators can perform
         # suitable privacy checks, but pillar should also be set, since it's
         # mandatory for private snaps.
+        # Note that we set self._information_type (not self.information_type)
+        # to avoid the call to self._reconcileAccess() while building the
+        # Snap instance.
         self.project = project
-        self.information_type = information_type
+        self._information_type = information_type
 
         self.registrant = registrant
         self.owner = owner
@@ -458,6 +464,7 @@ class Snap(Storm, WebhookTargetMixin):
     @information_type.setter
     def information_type(self, information_type):
         self._information_type = information_type
+        self._reconcileAccess()
 
     @property
     def private(self):
@@ -1121,6 +1128,14 @@ class Snap(Storm, WebhookTargetMixin):
         order_by = Desc(SnapBuild.id)
         return self._getBuilds(filter_term, order_by)
 
+    def visibleByUser(self, user):
+        """See `ISnap`."""
+        store = IStore(self)
+        return not store.find(
+            Snap,
+            Snap.id == self.id,
+            get_snap_privacy_filter(user)).is_empty()
+
     def subscribe(self, person, subscribed_by, ignore_permissions=False):
         """See `ISnap`."""
         # XXX pappacena 2021-02-05: We may need a "SnapSubscription" here.
@@ -1136,6 +1151,21 @@ class Snap(Storm, WebhookTargetMixin):
             self.pillar, person, unsubscribed_by, snaps=[self])
         IStore(self).flush()
 
+    def _reconcileAccess(self):
+        """Reconcile the snap's sharing information.
+
+        Takes the privacy and pillar and makes the related AccessArtifact
+        and AccessPolicyArtifacts match.
+        """
+        if self.project is None:
+            return
+        pillars = [self.project]
+        reconcile_access_for_artifact(self, self.information_type, pillars)
+
+    def setProject(self, project):
+        self.project = project
+        self._reconcileAccess()
+
     def _deleteAccessGrants(self):
         """Delete access grants for this snap recipe prior to deleting it."""
         getUtility(IAccessArtifactSource).delete([self])
@@ -1263,6 +1293,7 @@ class SnapSet:
             store_name=store_name, store_secrets=store_secrets,
             store_channels=store_channels, project=project)
         store.add(snap)
+        snap._reconcileAccess()
 
         # Automatically subscribe the owner to the Snap.
         snap.subscribe(snap.owner, registrant, ignore_permissions=True)
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index a68b116..e8ea61e 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -42,6 +42,7 @@ from testtools.matchers import (
     )
 import transaction
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
@@ -64,10 +65,18 @@ from lp.code.tests.helpers import (
     BranchHostingFixture,
     GitHostingFixture,
     )
-from lp.registry.enums import PersonVisibility
+from lp.registry.enums import (
+    BranchSharingPolicy,
+    PersonVisibility,
+    )
+from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.registry.model.accesspolicy import (
+    AccessArtifact,
+    AccessArtifactGrant,
+    )
 from lp.services.config import config
 from lp.services.crypto.interfaces import IEncryptedContainer
 from lp.services.database.constants import (
@@ -1327,6 +1336,105 @@ class TestSnapDeleteWithBuilds(TestCaseWithFactory):
             self.assertRaises(LostObjectError, getattr, webhook, "target")
 
 
+class TestSnapVisibility(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestSnapVisibility, self).setUp()
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+
+    def getSnapGrants(self, snap, person=None):
+        conditions = [AccessArtifact.snap == snap]
+        if person is not None:
+            conditions.append(AccessArtifactGrant.grantee == person)
+        return IStore(AccessArtifactGrant).find(
+            AccessArtifactGrant,
+            AccessArtifactGrant.abstract_artifact_id == AccessArtifact.id,
+            *conditions)
+
+    def test_only_owner_can_grant_access(self):
+        owner = self.factory.makePerson()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, private=True)
+        other_person = self.factory.makePerson()
+        with person_logged_in(owner):
+            snap.subscribe(other_person, owner)
+        with person_logged_in(other_person):
+            self.assertRaises(Unauthorized, getattr, snap, 'subscribe')
+
+    def test_private_is_invisible_by_default(self):
+        owner = self.factory.makePerson()
+        person = self.factory.makePerson()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, private=True)
+        with person_logged_in(owner):
+            self.assertFalse(snap.visibleByUser(person))
+
+    def test_private_is_visible_by_team_member(self):
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        snap = self.factory.makeSnap(private=True, owner=team,
+                                     registrant=person)
+        with person_logged_in(team):
+            self.assertTrue(snap.visibleByUser(person))
+
+    def test_subscribing_changes_visibility(self):
+        person = self.factory.makePerson()
+        owner = self.factory.makePerson()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, private=True)
+
+        with person_logged_in(owner):
+            self.assertFalse(snap.visibleByUser(person))
+            snap.subscribe(person, snap.owner)
+            # Calling again should be a no-op.
+            snap.subscribe(person, snap.owner)
+            self.assertTrue(snap.visibleByUser(person))
+            snap.unsubscribe(person, snap.owner)
+            self.assertFalse(snap.visibleByUser(person))
+
+    def test_reconcile_set_public(self):
+        owner = self.factory.makePerson()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, private=True)
+        another_user = self.factory.makePerson()
+        with admin_logged_in():
+            snap.subscribe(another_user, snap.owner)
+
+        self.assertEqual(1, self.getSnapGrants(snap, another_user).count())
+        with admin_logged_in():
+            snap.information_type = InformationType.PUBLIC
+        self.assertEqual(0, self.getSnapGrants(snap, another_user).count())
+
+    def test_reconcile_permissions_setting_project(self):
+        owner = self.factory.makePerson()
+        old_project = self.factory.makeProduct()
+        getUtility(IAccessPolicySource).create(
+            [(old_project, InformationType.PROPRIETARY)])
+
+        snap = self.factory.makeSnap(
+            project=old_project, private=True, registrant=owner, owner=owner)
+
+        # Owner automatically gets a grant.
+        with person_logged_in(owner):
+            self.assertTrue(snap.visibleByUser(snap.owner))
+            self.assertEqual(1, self.getSnapGrants(snap).count())
+
+        new_project = self.factory.makeProduct()
+        getUtility(IAccessPolicySource).create(
+            [(new_project, InformationType.PROPRIETARY)])
+        another_person = self.factory.makePerson()
+        with person_logged_in(owner):
+            snap.subscribe(another_person, owner)
+            self.assertTrue(snap.visibleByUser(another_person))
+            self.assertEqual(2, self.getSnapGrants(snap).count())
+
+            snap.setProject(new_project)
+            self.assertTrue(snap.visibleByUser(another_person))
+            self.assertEqual(2, self.getSnapGrants(snap).count())
+
+
 class TestSnapSet(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -1438,7 +1546,9 @@ class TestSnapSet(TestCaseWithFactory):
         [ref] = self.factory.makeGitRefs()
         components = self.makeSnapComponents(git_ref=ref)
         components['information_type'] = InformationType.PROPRIETARY
-        components['project'] = self.factory.makeProduct()
+        components['project'] = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
         snap = getUtility(ISnapSet).new(**components)
         with person_logged_in(components['owner']):
             self.assertTrue(snap.private)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index bc89da6..6948419 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4748,11 +4748,19 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                  auto_build_archive=None, auto_build_pocket=None,
                  auto_build_channels=None, is_stale=None,
                  require_virtualized=True, processors=None,
-                 date_created=DEFAULT, private=False, information_type=None,
+                 date_created=DEFAULT, private=None, information_type=None,
                  allow_internet=True, build_source_tarball=False,
                  store_upload=False, store_series=None, store_name=None,
                  store_secrets=None, store_channels=None, project=_DEFAULT):
         """Make a new Snap."""
+        assert information_type is None or private is None
+        if information_type is None:
+            # Defaults to public information type, unless "private" flag was
+            # passed.
+            information_type = (InformationType.PUBLIC if not private
+                                else InformationType.PROPRIETARY)
+        if private is None:
+            private = information_type not in PUBLIC_INFORMATION_TYPES
         if registrant is None:
             registrant = self.makePerson()
         if owner is None:
@@ -4772,13 +4780,15 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         if private and project is _DEFAULT:
             # If we are creating a private snap and didn't explictly set a
             # pillar for it, we must create a pillar.
-            project = self.makeProduct()
+            branch_sharing = (
+                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY if not private
+                else BranchSharingPolicy.PROPRIETARY)
+            project = self.makeProduct(
+                owner=registrant, registrant=registrant,
+                information_type=information_type,
+                branch_sharing_policy=branch_sharing)
         if project is _DEFAULT:
             project = None
-        assert information_type is None or private is None
-        if private is not None:
-            information_type = (InformationType.PUBLIC if not private
-                                else InformationType.PROPRIETARY)
         snap = getUtility(ISnapSet).new(
             registrant, owner, distroseries, name,
             require_virtualized=require_virtualized, processors=processors,