launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:snap-pillar-reconcile-access into launchpad:master with ~pappacena/launchpad:snap-pillar-accesspolicy as a prerequisite.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397693
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:snap-pillar-reconcile-access into launchpad:master.

diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
index eeaecfc..77c699e 100644
--- a/lib/lp/snappy/browser/snap.py
+++ b/lib/lp/snappy/browser/snap.py
@@ -719,6 +719,16 @@ class SnapAdminView(BaseSnapEditView):
                     'private',
                     '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)
+        if 'private' in data:
+            private = data.pop('private')
+            self.context.setPrivate(private)
+        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 d5f0c7d..d048778 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -654,6 +654,9 @@ 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)
         self.assertTrue(snap.require_virtualized)
         self.assertFalse(snap.private)
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 890b6f3..864af3c 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -568,6 +568,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?"""
+
 
 class ISnapEdit(IWebhookTarget):
     """`ISnap` methods that require launchpad.Edit permission."""
@@ -830,6 +833,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.
@@ -865,6 +871,9 @@ class ISnapAdminAttributes(Interface):
     def unsubscribe(person, unsubscribed_by):
         """Unsubscribe a person to this snap recipe."""
 
+    def setPrivate(private):
+        """Set the current snap recipe as public or private."""
+
 
 # XXX cjwatson 2015-07-17 bug=760849: "beta" is a lie to get WADL
 # generation working.  Individual attributes must set their version to
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index a23016e..ce73636 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -60,6 +60,7 @@ from lp.app.browser.tales import (
     ArchiveFormatterAPI,
     DateTimeFormatterAPI,
     )
+from lp.app.enums import InformationType
 from lp.app.errors import IncompatibleArguments
 from lp.app.interfaces.security import IAuthorization
 from lp.app.interfaces.services import IService
@@ -109,7 +110,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
@@ -1060,6 +1064,23 @@ class Snap(Storm, WebhookTargetMixin):
         order_by = Desc(SnapBuild.id)
         return self._getBuilds(filter_term, order_by)
 
+    def visibleByUser(self, user):
+        """See `IGitRepository`."""
+        if not self.private:
+            return True
+        if user is None:
+            return False
+        if user.inTeam(self.owner):
+            return True
+
+        store = IStore(self)
+        visibility_clause = removeSecurityProxy(getUtility(
+            ISnapSet))._findSnapVisibilityClause(user)
+        return not store.find(
+            Snap,
+            Snap.id == self.id,
+            visibility_clause).is_empty()
+
     def subscribe(self, person, subscribed_by):
         """See `ISnap`."""
         # XXX pappacena 2021-02-05: We may need a "SnapSubscription" here.
@@ -1073,6 +1094,27 @@ 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
+        info_type = (InformationType.PUBLIC if not self.private
+                     else InformationType.PROPRIETARY)
+        pillars = [self.project]
+        reconcile_access_for_artifact(self, info_type, pillars)
+
+    def setPrivate(self, private):
+        self.private = private
+        self._reconcileAccess()
+
+    def setProject(self, project):
+        self.project = project
+        self._reconcileAccess()
+
     def destroySelf(self):
         """See `ISnap`."""
         store = IStore(Snap)
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index e21efa1..c3b2fb5 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
@@ -65,9 +66,14 @@ from lp.code.tests.helpers import (
     GitHostingFixture,
     )
 from lp.registry.enums import 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 (
@@ -1324,6 +1330,101 @@ 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()
+        pillar = self.factory.makeProduct(owner=owner)
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, project=pillar, 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=team)
+        with person_logged_in(team):
+            self.assertTrue(snap.visibleByUser(person))
+
+    def test_subscribing_changes_visibility(self):
+        person = self.factory.makePerson()
+        owner = self.factory.makePerson()
+        pillar = self.factory.makeProduct(owner=owner)
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, project=pillar, 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.setPrivate(False)
+        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)
+
+        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(1, self.getSnapGrants(snap).count())
+
+            snap.setProject(new_project)
+            self.assertTrue(snap.visibleByUser(another_person))
+            self.assertEqual(1, self.getSnapGrants(snap).count())
+
+
 class TestSnapSet(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer