Merge lp:~wallyworld/launchpad/sharing-details-delete2-966641 into lp:launchpad

[Ian Booth]

Add sharing service api for revoking access to bugs and branches

== 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 people.

I also had to change the sharing details view - it was putting bugtask urls into the view data model but instead needed to use bug urls. Our disclosure data model deals with bugs, not bugtasks.

== 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

[Curtis Hovey (sinzui)]

I do not understand line 23. user-data and security bugs can be shared by projects. I may only maintain the second or third project, so the following code
    importance = bug.default_bugtask.importance.title.lower()
returns the importance the oldest bugtask found. Since my project is not the oldest task, it is not the default -- that importance is wrong.

Also the links are made to the bug, which I think chooses the first/oldest bugtask as well -- the context is not my project so the bugtask index page renders the links in the side portlets for the other project, not mine. I think test_pillar_sharing could have a test with for a second task that demonstrates that this code is providing data for the wrong task.

review: Needs Information (code)

[Ian Booth (wallyworld)]

> I do not understand line 23. user-data and security bugs can be shared by
> projects. I may only maintain the second or third project, so the following
> code
> importance = bug.default_bugtask.importance.title.lower()
> returns the importance the oldest bugtask found. Since my project is not the
> oldest task, it is not the default -- that importance is wrong.
>
> Also the links are made to the bug, which I think chooses the first/oldest
> bugtask as well -- the context is not my project so the bugtask index page
> renders the links in the side portlets for the other project, not mine. I
> think test_pillar_sharing could have a test with for a second task that
> demonstrates that this code is providing data for the wrong task.

The core issue was that the view needs to display bugtask information but when it makes the API call to revoke access when the user hits "Delete", it needs to send the bug url not the bugtask url. I incorrectly implemented this change. The view data model has been corrected to include the bugtask attributes for the importance and rendered url and the self_link attribute is set to that of the bug which is what is sent through to the API call. Essentially, the code was reverted to how it used to be; only the self_link attribute was added.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-04-04 23:34:24 +0000
+++ lib/lp/registry/browser/pillar.py	2012-04-04 23:34:24 +0000
@@ -431,7 +431,7 @@
                             task.target == self.pillar]
             if bugtask is not None:
                 web_link = canonical_url(bugtask, path_only_if_possible=True)
-                self_link = absoluteURL(bugtask, request)
+                self_link = absoluteURL(bug, request)
                 importance = bugtask.importance.title.lower()
             else:
                 # This shouldn't ever happen, but if it does there's no reason

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-04 23:34:24 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-04 23:34:24 +0000
@@ -149,7 +149,7 @@
                 'bug_importance': bugtask.importance.title.lower(),
                 'web_link': canonical_url(
                     bugtask, path_only_if_possible=True),
-                'self_link': absoluteURL(bugtask, request),
+                'self_link': absoluteURL(self.bug, request),
             }, cache.objects.get('bugs')[0])
             if self.pillar_type == 'product':
                 self.assertEqual({

=== 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 23:34:24 +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 23:34:24 +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(schema=IBug), title=_('Bugs'), required=False),
+        branches=List(
+            Reference(schema=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 23:34:24 +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-04-02 03:12:58 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-04-04 23:34:24 +0000
@@ -21,6 +21,7 @@
     )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
+    IAccessArtifactSource,
     IAccessPolicyGrantFlatSource,
     IAccessPolicyGrantSource,
     IAccessPolicySource,
@@ -249,3 +250,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-04-04 04:31:31 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-04-04 23:34:24 +0000
@@ -18,6 +18,8 @@
     SharingPermission,
     )
 from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessPolicyGrantFlatSource,
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
@@ -586,6 +588,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 23:34:24 +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