launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-details-delete2-966641 into lp:launchpad with lp:~wallyworld/launchpad/sharing-details-delete-966641 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #966641 in Launchpad itself: "Delete button on sharing details page does nothing"
  https://bugs.launchpad.net/launchpad/+bug/966641

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-delete2-966641/+merge/100723

== Implementation ==

Add new API to sharing service:

    def revokeAccessGrants(pillar, sharee, branches=None, bugs=None):
        """Remove a sharee's access to the specified artifacts.

This is used by the delete button on the sharing details page to revoke a user's access to a bug or branch.

The findByArtifact and revokeByArtifact methods on IAccessArtifactGrantSource had to be extended to allow an option list of grantees to be passed in, to only find/revoke access for the specified peope.

== Tests ==

Add new revokeAccessGrants tests to test_sharingservice
Add new tests for the findByArtifact and revokeByArtifact methods to test_accesspolicy

== Lint ==

Linting changed files:
  lib/lp/registry/interfaces/accesspolicy.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/model/accesspolicy.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/tests/test_accesspolicy.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-delete2-966641/+merge/100723
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-details-delete2-966641 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py	2012-03-31 18:22:38 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py	2012-04-04 04:21:22 +0000
@@ -124,11 +124,21 @@
             pairs.
         """
 
-    def findByArtifact(artifacts):
-        """Return all `IAccessArtifactGrant` objects for the artifacts."""
-
-    def revokeByArtifact(artifacts):
-        """Delete all `IAccessArtifactGrant` objects for the artifacts."""
+    def findByArtifact(artifacts, grantees=None):
+        """Return `IAccessArtifactGrant` objects for the artifacts.
+
+        :param artifacts: the artifacts for which to find any grants.
+        :param grantees: find grants for the specified grantees only,
+            else find all grants.
+        """
+
+    def revokeByArtifact(artifacts, grantees=None):
+        """Delete `IAccessArtifactGrant` objects for the artifacts.
+
+        :param artifacts: the artifacts to which revoke access.
+        :param grantees: revoke access for the specified grantees only,
+            else delete all grants.
+        """
 
 
 class IAccessPolicyArtifactSource(Interface):

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-04-04 04:21:22 +0000
@@ -28,6 +28,8 @@
 
 from lp import _
 from lp.app.interfaces.services import IService
+from lp.bugs.interfaces.bug import IBug
+from lp.code.interfaces.branch import IBranch
 from lp.registry.enums import (
     InformationType,
     SharingPermission,
@@ -119,3 +121,21 @@
         :param information_types: if None, remove all access, otherwise just
                                    remove the specified access_policies
         """
+
+    @export_write_operation()
+    @operation_parameters(
+        pillar=Reference(IPillar, title=_('Pillar'), required=True),
+        sharee=Reference(IPerson, title=_('Sharee'), required=True),
+        bugs=List(
+            Reference(IBug, title=_('Bugs'), required=False)),
+        branches=List(
+            Reference(IBranch), title=_('Branches'), required=False))
+    @operation_for_version('devel')
+    def revokeAccessGrants(pillar, sharee, branches=None, bugs=None):
+        """Remove a sharee's access to the specified artifacts.
+
+        :param pillar: the pillar from which to remove access
+        :param sharee: the person or team for whom to revoke access
+        :param bugs: the bugs for which to revoke access
+        :param branches: the branches for which to revoke access
+        """

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-03-31 18:22:38 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-04-04 04:21:22 +0000
@@ -283,15 +283,19 @@
                 for (artifact, grantee) in grants)))
 
     @classmethod
-    def findByArtifact(cls, artifacts):
+    def findByArtifact(cls, artifacts, grantees=None):
         """See `IAccessArtifactGrantSource`."""
-        ids = [artifact.id for artifact in artifacts]
-        return IStore(cls).find(cls, cls.abstract_artifact_id.is_in(ids))
+        artifact_ids = [artifact.id for artifact in artifacts]
+        constraints = [cls.abstract_artifact_id.is_in(artifact_ids)]
+        if grantees:
+            grantee_ids = [grantee.id for grantee in grantees]
+            constraints.append(cls.grantee_id.is_in(grantee_ids))
+        return IStore(cls).find(cls, *constraints)
 
     @classmethod
-    def revokeByArtifact(cls, artifacts):
+    def revokeByArtifact(cls, artifacts, grantees=None):
         """See `IAccessPolicyGrantSource`."""
-        cls.findByArtifact(artifacts).remove()
+        cls.findByArtifact(artifacts, grantees).remove()
 
 
 class AccessPolicyGrant(StormBase):

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-04-04 04:21:22 +0000
@@ -21,6 +21,7 @@
     )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
+    IAccessArtifactSource,
     IAccessPolicyGrantFlatSource,
     IAccessPolicyGrantSource,
     IAccessPolicySource,
@@ -242,3 +243,23 @@
             accessartifact_grant_source = getUtility(
                 IAccessArtifactGrantSource)
             accessartifact_grant_source.revokeByArtifact(to_delete)
+
+    @available_with_permission('launchpad.Edit', 'pillar')
+    def revokeAccessGrants(self, pillar, sharee, branches=None, bugs=None):
+        """See `ISharingService`."""
+
+        if not self.write_enabled:
+            raise Unauthorized("This feature is not yet enabled.")
+
+        artifacts = []
+        if branches:
+            artifacts.extend(branches)
+        if bugs:
+            artifacts.extend(bugs)
+        # Find the access artifacts associated with the bugs and branches.
+        accessartifact_source = getUtility(IAccessArtifactSource)
+        artifacts_to_delete = accessartifact_source.find(artifacts)
+        # Revoke access to bugs/branches for the specified sharee.
+        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
+        accessartifact_grant_source.revokeByArtifact(
+            artifacts_to_delete, [sharee])

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-04-04 04:21:22 +0000
@@ -18,6 +18,8 @@
     SharingPermission,
     )
 from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessPolicyGrantFlatSource,
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
@@ -542,6 +544,101 @@
             Unauthorized, self.service.deletePillarSharee,
             product, [InformationType.USERDATA])
 
+    def _assert_revokeAccessGrants(self, pillar, bugs, branches):
+        artifacts = []
+        if bugs:
+            artifacts.extend(bugs)
+        if branches:
+            artifacts.extend(branches)
+        policy = self.factory.makeAccessPolicy(pillar=pillar)
+        # Grant access to a grantee and another person.
+        grantee = self.factory.makePerson()
+        someone = self.factory.makePerson()
+        access_artifacts = []
+        for artifact in artifacts:
+            access_artifact = self.factory.makeAccessArtifact(
+                concrete=artifact)
+            access_artifacts.append(access_artifact)
+            self.factory.makeAccessPolicyArtifact(
+                artifact=access_artifact, policy=policy)
+            for person in [grantee, someone]:
+                self.factory.makeAccessArtifactGrant(
+                    artifact=access_artifact, grantee=person,
+                    grantor=pillar.owner)
+
+        # Check that grantee has expected access grants.
+        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
+        grants = accessartifact_grant_source.findByArtifact(
+            access_artifacts, [grantee])
+        apgfs = getUtility(IAccessPolicyGrantFlatSource)
+        self.assertEqual(1, grants.count())
+
+        with FeatureFixture(WRITE_FLAG):
+            self.service.revokeAccessGrants(
+                pillar, grantee, bugs=bugs, branches=branches)
+
+        # The grantee now has no access to anything.
+        permission_info = apgfs.findGranteePermissionsByPolicy(
+            [policy], [grantee])
+        self.assertEqual(0, permission_info.count())
+
+        # Someone else still has access to the bugs and branches.
+        grants = accessartifact_grant_source.findByArtifact(
+            access_artifacts, [someone])
+        self.assertEqual(1, grants.count())
+
+    def test_revokeAccessGrantsBugs(self):
+        # Users with launchpad.Edit can delete all access for a sharee.
+        owner = self.factory.makePerson()
+        distro = self.factory.makeDistribution(owner=owner)
+        login_person(owner)
+        bug = self.factory.makeBug(
+            distribution=distro, owner=owner, private=True)
+        self._assert_revokeAccessGrants(distro, [bug], None)
+
+    def test_revokeAccessGrantsBranches(self):
+        # Users with launchpad.Edit can delete all access for a sharee.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        branch = self.factory.makeBranch(
+            product=product, owner=owner, private=True)
+        self._assert_revokeAccessGrants(product, None, [branch])
+
+    def _assert_revokeAccessGrantsUnauthorized(self):
+        # revokeAccessGrants raises an Unauthorized exception if the user
+        # is not permitted to do so.
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product, private=True)
+        sharee = self.factory.makePerson()
+        with FeatureFixture(WRITE_FLAG):
+            self.assertRaises(
+                Unauthorized, self.service.revokeAccessGrants,
+                product, sharee, bugs=[bug])
+
+    def test_revokeAccessGrantsAnonymous(self):
+        # Anonymous users are not allowed.
+        with FeatureFixture(WRITE_FLAG):
+            login(ANONYMOUS)
+            self._assert_revokeAccessGrantsUnauthorized()
+
+    def test_revokeAccessGrantsAnyone(self):
+        # Unauthorized users are not allowed.
+        with FeatureFixture(WRITE_FLAG):
+            login_person(self.factory.makePerson())
+            self._assert_revokeAccessGrantsUnauthorized()
+
+    def test_revokeAccessGrants_without_flag(self):
+        # The feature flag needs to be enabled.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        bug = self.factory.makeBug(product=product, private=True)
+        sharee = self.factory.makePerson()
+        login_person(owner)
+        self.assertRaises(
+            Unauthorized, self.service.revokeAccessGrants,
+            product, sharee, bugs=[bug])
+
 
 class ApiTestMixin:
     """Common tests for launchpadlib and webservice."""

=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py	2012-03-31 18:22:38 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py	2012-04-04 04:21:22 +0000
@@ -290,6 +290,21 @@
             grants,
             getUtility(IAccessArtifactGrantSource).findByArtifact([artifact]))
 
+    def test_findByArtifact_specified_grantees(self):
+        # findByArtifact() finds only the relevant grants for the specified
+        # grantees.
+        artifact = self.factory.makeAccessArtifact()
+        grantees = [self.factory.makePerson() for i in range(3)]
+        grants = [
+            self.factory.makeAccessArtifactGrant(
+                artifact=artifact, grantee=grantee)
+            for grantee in grantees]
+        self.factory.makeAccessArtifactGrant()
+        self.assertContentEqual(
+            grants[:2],
+            getUtility(IAccessArtifactGrantSource).findByArtifact(
+                [artifact], grantees=grantees[:2]))
+
     def test_revokeByArtifact(self):
         # revokeByArtifact() removes the relevant grants.
         artifact = self.factory.makeAccessArtifact()
@@ -300,6 +315,25 @@
         self.assertRaises(LostObjectError, getattr, grant, 'grantor')
         self.assertIsNot(None, other_grant.grantor)
 
+    def test_revokeByArtifact_specified_grantees(self):
+        # revokeByArtifact() removes the relevant grants for the specified
+        # grantees.
+        artifact = self.factory.makeAccessArtifact()
+        grantee = self.factory.makePerson()
+        someone_else = self.factory.makePerson()
+        grant = self.factory.makeAccessArtifactGrant(
+            artifact=artifact, grantee=grantee)
+        someone_else_grant = self.factory.makeAccessArtifactGrant(
+            artifact=artifact, grantee=someone_else)
+        other_grant = self.factory.makeAccessArtifactGrant()
+        aags = getUtility(IAccessArtifactGrantSource)
+        aags.revokeByArtifact([artifact], [grantee])
+        IStore(grant).invalidate()
+        self.assertRaises(LostObjectError, getattr, grant, 'grantor')
+        self.assertEqual(
+            someone_else_grant, aags.findByArtifact([artifact])[0])
+        self.assertIsNot(None, other_grant.grantor)
+
 
 class TestAccessPolicyArtifact(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer