launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/subscribe-grants-access-1000045 into lp:launchpad with lp:~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425 as a prerequisite.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #1000045 in Launchpad itself: "Ensure access when subscribing a user to a bug "
  https://bugs.launchpad.net/launchpad/+bug/1000045

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/subscribe-grants-access-1000045/+merge/106278

== Implementation ==

This branch provides code to grant access to a bug when a user is subscribed (if the bug is not already visible to the user). This is done right now by triggers, but the triggers will be removed at some point.

Key implementation points:

Add feature flag disclosure.access_mirror_triggers.removed which needs to be turned on for everybody during the FDT when the triggers are deleted.

Add 2 new methods to the sharing service:
- getVisibleArtifacts(person, branches=None, bugs=None)
- createAccessGrants(user, sharee, branches=None, bugs=None)

The bug subscribe code checks that triggers are removed and if so, checks whether the bug is visible to the subscriber, and if not, grants access.

The permission check in the createAccessGrants() method checks for launchpad.Edit on each bug/branch. This matches the permission required to subscribe the user in the first place. However, other writeable operations on the sharing service required launchpad.Edit on the pillar. The check on the createAccessGrants() method could be tightened, but then it may not allow a user to subscribe someone since it could raise Unauthorised when a user has launchpad.Edit on the bug but not launchpad.Edit on a target pillar.


== Tests ==

Add tests for the new sharing service methods.
- test_createAccessGrantsXXXX
- test_getVisibleArtifacts

Add tests for the bug subscription behaviour:
- test_subscribeGrantsVisibilityWithTriggersRemoved
- test_subscribeGrantsVisibilityUsingTriggers


== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bugvisibility.py
  lib/lp/registry/interfaces/accesspolicy.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/services/features/flags.py
  lib/lp/testing/factory.py

./lib/lp/testing/factory.py
    1129: E501 line too long (80 characters)
-- 
https://code.launchpad.net/~wallyworld/launchpad/subscribe-grants-access-1000045/+merge/106278
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-05-17 22:17:26 +0000
+++ lib/lp/bugs/model/bug.py	2012-05-17 22:17:27 +0000
@@ -4,6 +4,7 @@
 # pylint: disable-msg=E0611,W0212
 
 """Launchpad bug-related database table classes."""
+from lp.app.interfaces.services import IService
 
 __metaclass__ = type
 
@@ -842,6 +843,15 @@
         # Ensure that the subscription has been flushed.
         Store.of(sub).flush()
 
+        # Grant the subscriber access if they can't see the bug (if the
+        # database triggers aren't going to do it for us).
+        trigger_flag = 'disclosure.access_mirror_triggers.removed'
+        if bool(getFeatureFlag(trigger_flag)):
+            service = getUtility(IService, 'sharing')
+            bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
+            if not bugs:
+                service.createAccessGrants(subscribed_by, person, bugs=[self])
+
         # In some cases, a subscription should be created without
         # email notifications.  suppress_notify determines if
         # notifications are sent.

=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py	2012-05-17 22:17:26 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py	2012-05-17 22:17:27 +0000
@@ -18,6 +18,8 @@
 
 LEGACY_VISIBILITY_FLAG = {
     u"disclosure.legacy_subscription_visibility.enabled": u"true"}
+TRIGGERS_REMOVED_FLAG = {
+    u"disclosure.access_mirror_triggers.removed": u"true"}
 
 
 class TestPublicBugVisibility(TestCaseWithFactory):
@@ -123,3 +125,23 @@
             with self.disable_trigger_fixture:
                 self.bug.unsubscribe(user, self.owner)
         self.assertTrue(self.bug.userCanView(user))
+
+    def test_subscribeGrantsVisibilityWithTriggersRemoved(self):
+        # When a user is subscribed to a bug, they are granted access. In this
+        # test, the database triggers are removed and so model code is used.
+        with FeatureFixture(TRIGGERS_REMOVED_FLAG):
+            with self.disable_trigger_fixture:
+                user = self.factory.makePerson()
+                self.assertFalse(self.bug.userCanView(user))
+                with celebrity_logged_in('admin'):
+                    self.bug.subscribe(user, self.owner)
+                    self.assertTrue(self.bug.userCanView(user))
+
+    def test_subscribeGrantsVisibilityUsingTriggers(self):
+        # When a user is subscribed to a bug, they are granted access. In this
+        # test, the database triggers are used.
+        user = self.factory.makePerson()
+        self.assertFalse(self.bug.userCanView(user))
+        with celebrity_logged_in('admin'):
+            self.bug.subscribe(user, self.owner)
+            self.assertTrue(self.bug.userCanView(user))

=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py	2012-05-17 22:17:26 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py	2012-05-17 22:17:27 +0000
@@ -283,5 +283,5 @@
         """Find the `IAccessArtifact`s for grantee and policies.
 
         :param grantee: the access artifact grantee.
-        :param policies: a collection of `IAccesPolicy`s.
+        :param policies: a collection of `IAccessPolicy`s.
         """

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-05-17 22:17:26 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-05-17 22:17:27 +0000
@@ -58,6 +58,18 @@
         :return: a (bugtasks, branches) tuple
         """
 
+    def getVisibleArtifacts(person, branches=None, bugs=None):
+        """Return the artifacts shared with person.
+
+        Given lists of artifacts, return those a person has access to either
+        via a policy grant or artifact grant.
+
+        :param person: the person whose access is being checked.
+        :param branches: the branches to check for which a person has access.
+        :param bugs: the bugs to check for which a person has access.
+        :return: a collection of artifacts the person can see.
+        """
+
     def getInformationTypes(pillar):
         """Return the allowed information types for the given pillar."""
 
@@ -149,3 +161,21 @@
         :param bugs: the bugs for which to revoke access
         :param branches: the branches for which to revoke access
         """
+
+    @export_write_operation()
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
+        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 createAccessGrants(user, sharee, branches=None, bugs=None):
+        """Grant a sharee access to the specified artifacts.
+
+        :param user: the user making the request
+        :param sharee: the person or team for whom to grant access
+        :param bugs: the bugs for which to grant access
+        :param branches: the branches for which to grant access
+        """

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-05-17 22:17:26 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-05-17 22:17:27 +0000
@@ -8,6 +8,8 @@
     'SharingService',
     ]
 
+from itertools import product
+
 from lazr.restful.interfaces import IWebBrowserOriginatingRequest
 from lazr.restful.utils import get_current_web_service_request
 from zope.component import getUtility
@@ -31,7 +33,6 @@
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
-from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.sharingjob import IRemoveSubscriptionsJobSource
@@ -39,7 +40,10 @@
 from lp.registry.model.person import Person
 from lp.services.features import getFeatureFlag
 from lp.services.searchbuilder import any
-from lp.services.webapp.authorization import available_with_permission
+from lp.services.webapp.authorization import (
+    available_with_permission,
+    check_permission,
+    )
 
 
 class SharingService:
@@ -58,8 +62,11 @@
 
     @property
     def write_enabled(self):
-        return bool(getFeatureFlag(
-            'disclosure.enhanced_sharing.writable'))
+        return (
+            bool(getFeatureFlag(
+            'disclosure.enhanced_sharing.writable') or
+            bool(getFeatureFlag(
+            'disclosure.access_mirror_triggers.removed'))))
 
     def getSharedArtifacts(self, pillar, person, user):
         """See `ISharingService`."""
@@ -89,6 +96,33 @@
 
         return bugtasks, branches
 
+    def getVisibleArtifacts(self, person, branches=None, bugs=None):
+        """See `ISharingService`."""
+        bugs_by_id = {}
+        branches_by_id = {}
+        for bug in bugs or []:
+            bugs_by_id[bug.id] = bug
+        for branch in branches or []:
+            branches_by_id[branch.id] = branch
+
+        # Load the bugs.
+        visible_bug_ids = []
+        if bugs_by_id:
+            param = BugTaskSearchParams(
+                user=person, bug=any(*bugs_by_id.keys()))
+            visible_bug_ids = list(getUtility(IBugTaskSet).searchBugIds(param))
+        visible_bugs = [bugs_by_id[bug_id] for bug_id in visible_bug_ids]
+
+        # Load the branches.
+        visible_branches = []
+        if branches_by_id:
+            all_branches = getUtility(IAllBranches)
+            wanted_branches = all_branches.visibleByUser(person).withIds(
+                *branches_by_id.keys())
+            visible_branches = list(wanted_branches.getBranches())
+
+        return visible_bugs, visible_branches
+
     def getInformationTypes(self, pillar):
         """See `ISharingService`."""
         allowed_types = [
@@ -313,3 +347,27 @@
         # longer see.
         getUtility(IRemoveSubscriptionsJobSource).create(
             pillar, sharee, user, bugs=bugs, branches=branches)
+
+    def createAccessGrants(self, user, 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)
+        # The user needs to have launchpad.Edit permission on all supplied
+        # bugs and branches or else we raise an Unauthorized exception.
+        for artifact in artifacts or []:
+            if not check_permission('launchpad.Edit', artifact):
+                raise Unauthorized
+
+        # Ensure there are access artifacts associated with the bugs and
+        # branches.
+        artifacts = getUtility(IAccessArtifactSource).ensure(artifacts)
+        # Create access to bugs/branches for the specified sharee.
+        getUtility(IAccessArtifactGrantSource).grant(
+            list(product(artifacts, [sharee], [user])))

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-05-17 22:17:26 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-05-17 22:17:27 +0000
@@ -13,11 +13,13 @@
 from zope.traversing.browser.absoluteurl import absoluteURL
 
 from lp.app.interfaces.services import IService
+from lp.bugs.interfaces.bug import IBug
 from lp.code.enums import (
     BranchSubscriptionDiffSize,
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
     )
+from lp.code.interfaces.branch import IBranch
 from lp.registry.enums import (
     InformationType,
     SharingPermission,
@@ -786,6 +788,89 @@
             Unauthorized, self.service.revokeAccessGrants,
             product, product.owner, sharee, bugs=[bug])
 
+    def _assert_createAccessGrants(self, user, bugs, branches):
+        # Creating access grants works as expected.
+        grantee = self.factory.makePerson()
+        with FeatureFixture(WRITE_FLAG):
+            self.service.createAccessGrants(
+                user, grantee, bugs=bugs, branches=branches)
+
+        # Check that grantee has expected access grants.
+        shared_bugs = []
+        shared_branches = []
+        all_pillars = []
+        for bug in bugs or []:
+            all_pillars.extend(bug.affected_pillars)
+        for branch in branches or []:
+            all_pillars.append(branch.target.context)
+        policies = getUtility(IAccessPolicySource).findByPillar(all_pillars)
+
+        apgfs = getUtility(IAccessPolicyGrantFlatSource)
+        access_artifacts = apgfs.findArtifactsByGrantee(grantee, policies)
+        for a in access_artifacts:
+            if IBug.providedBy(a.concrete_artifact):
+                shared_bugs.append(a.concrete_artifact)
+            elif IBranch.providedBy(a.concrete_artifact):
+                shared_branches.append(a.concrete_artifact)
+        self.assertContentEqual(bugs or [], shared_bugs)
+        self.assertContentEqual(branches or [], shared_branches)
+
+    def test_createAccessGrantsBugs(self):
+        # Access grants can be created for bugs.
+        owner = self.factory.makePerson()
+        distro = self.factory.makeDistribution(owner=owner)
+        login_person(owner)
+        bug = self.factory.makeBug(
+            distribution=distro, owner=owner,
+            information_type=InformationType.USERDATA)
+        self._assert_createAccessGrants(owner, [bug], None)
+
+    def test_createAccessGrantsBranches(self):
+        # Access grants can be created for branches.
+        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_createAccessGrants(owner, None, [branch])
+
+    def _assert_createAccessGrantsUnauthorized(self, user):
+        # createAccessGrants raises an Unauthorized exception if the user
+        # is not permitted to do so.
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(
+            product=product, information_type=InformationType.USERDATA)
+        sharee = self.factory.makePerson()
+        with FeatureFixture(WRITE_FLAG):
+            self.assertRaises(
+                Unauthorized, self.service.createAccessGrants,
+                user, sharee, bugs=[bug])
+
+    def test_createAccessGrantsAnonymous(self):
+        # Anonymous users are not allowed.
+        with FeatureFixture(WRITE_FLAG):
+            login(ANONYMOUS)
+            self._assert_createAccessGrantsUnauthorized(ANONYMOUS)
+
+    def test_createAccessGrantsAnyone(self):
+        # Unauthorized users are not allowed.
+        with FeatureFixture(WRITE_FLAG):
+            anyone = self.factory.makePerson()
+            login_person(anyone)
+            self._assert_createAccessGrantsUnauthorized(anyone)
+
+    def test_createAccessGrants_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, information_type=InformationType.USERDATA)
+        sharee = self.factory.makePerson()
+        login_person(owner)
+        self.assertRaises(
+            Unauthorized, self.service.createAccessGrants,
+            product.owner, sharee, bugs=[bug])
+
     def test_getSharedArtifacts(self):
         # Test the getSharedArtifacts method.
         owner = self.factory.makePerson()
@@ -848,6 +933,53 @@
         self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
         self.assertContentEqual(branches[:9], shared_branches)
 
+    def test_getVisibleArtifacts(self):
+        # Test the getVisibleArtifacts method.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        grantee = self.factory.makePerson()
+        login_person(owner)
+
+        bugs = []
+        for x in range(0, 10):
+            bug = self.factory.makeBug(
+                product=product, owner=owner,
+                information_type=InformationType.USERDATA)
+            bugs.append(bug)
+        branches = []
+        for x in range(0, 10):
+            branch = self.factory.makeBranch(
+                product=product, owner=owner, private=True)
+            branches.append(branch)
+
+        def grant_access(artifact):
+            access_artifact = self.factory.makeAccessArtifact(
+                concrete=artifact)
+            self.factory.makeAccessArtifactGrant(
+                artifact=access_artifact, grantee=grantee, grantor=owner)
+            return access_artifact
+
+        # Grant access to some of the bugs and branches.
+        for bug in bugs[:5]:
+            grant_access(bug)
+        for branch in branches[:5]:
+            grant_access(branch)
+            # XXX for now we need to subscribe users to the branch in order
+            # for the underlying BranchCollection to allow access. This will
+            # no longer be the case when BranchCollection supports the new
+            # access policy framework.
+            branch.subscribe(
+                grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
+                BranchSubscriptionDiffSize.NODIFF,
+                CodeReviewNotificationLevel.NOEMAIL,
+                owner)
+
+        # Check the results.
+        shared_bugs, shared_branches = self.service.getVisibleArtifacts(
+            grantee, branches, bugs)
+        self.assertContentEqual(bugs[:5], shared_bugs)
+        self.assertContentEqual(branches[:5], shared_branches)
+
 
 class ApiTestMixin:
     """Common tests for launchpadlib and webservice."""

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-05-17 22:17:26 +0000
+++ lib/lp/services/features/flags.py	2012-05-17 22:17:27 +0000
@@ -295,6 +295,13 @@
      '',
      'Sharing management',
      ''),
+    ('disclosure.access_mirror_triggers.removed',
+     'boolean',
+     ('If true, the database triggers which cause subscribing to grant'
+      'access to a bug or branch have been removed.'),
+     '',
+     '',
+     ''),
     ('garbo.workitem_migrator.enabled',
      'boolean',
      ('If true, garbo will try to migrate work items from the whiteboard of '

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-05-04 14:52:44 +0000
+++ lib/lp/testing/factory.py	2012-05-17 22:17:27 +0000
@@ -1122,6 +1122,12 @@
         if private:
             removeSecurityProxy(branch).explicitly_private = True
             removeSecurityProxy(branch).transitively_private = True
+            # XXX this is here till branch properly supports information_type
+            [artifact] = getUtility(IAccessArtifactSource).ensure([branch])
+            [policy] = getUtility(IAccessPolicySource).find(
+                [(branch.target.context, InformationType.USERDATA)])
+            getUtility(IAccessPolicyArtifactSource).create([(artifact, policy)])
+
         if stacked_on is not None:
             removeSecurityProxy(branch).stacked_on = stacked_on
         if reviewer is not None: