Merge lp:~wallyworld/launchpad/subscribe-grants-access-1000045 into lp:launchpad

Proposed by Ian Booth
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 15359
Proposed branch: lp:~wallyworld/launchpad/subscribe-grants-access-1000045
Merge into: lp:launchpad
Prerequisite: lp:~wallyworld/launchpad/legacy-unsubscribe-revokes-access-997425
Diff against target: 463 lines (+273/-21)
9 files modified
configs/testrunner/launchpad-lazr.conf (+0/-15)
lib/lp/bugs/model/bug.py (+10/-0)
lib/lp/bugs/tests/test_bugvisibility.py (+22/-0)
lib/lp/registry/interfaces/accesspolicy.py (+1/-1)
lib/lp/registry/interfaces/sharingservice.py (+30/-0)
lib/lp/registry/services/sharingservice.py (+62/-4)
lib/lp/registry/services/tests/test_sharingservice.py (+135/-1)
lib/lp/services/features/flags.py (+7/-0)
lib/lp/testing/factory.py (+6/-0)
To merge this branch: bzr merge lp:~wallyworld/launchpad/subscribe-grants-access-1000045
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+106278@code.launchpad.net

Commit message

Add model code to grant access to bugs when adding a subscriber and add required sharing service methods

Description of the change

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

To post a comment you must log in.
Curtis Hovey (sinzui) wrote :

I think this is good to land. We need a bug number for the two XXX, which might be bug 933768 or bug 933938.

I think we will need to revisit the permission rules in a few weeks. The PM team delegates the running of the project to the drivers, who work with the contractors. Drivers do not get to set project-level sharing, but they probably need to manage bugs and branch sharing. We need Matthew to get this answer.

review: Approve (code)
Ian Booth (wallyworld) wrote :

On 18/05/12 08:51, Curtis Hovey wrote:
> Review: Approve code
>
> I think this is good to land. We need a bug number for the two XXX, which might be bug 933768 or bug 933938.

I created a new bug 1001042 "Update BranchCollection filtering to use
branch information_type" since it is pretty much a self contained body
of work.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configs/testrunner/launchpad-lazr.conf'
2--- configs/testrunner/launchpad-lazr.conf 2012-05-21 00:05:24 +0000
3+++ configs/testrunner/launchpad-lazr.conf 2012-05-15 07:22:45 +0000
4@@ -150,21 +150,6 @@
5 # processes spawned through some other mechanism.
6 port: 11242
7
8-<<<<<<< TREE
9-=======
10-[merge_proposal_jobs]
11-error_dir: /var/tmp/codehosting.test
12-
13-[packaging_translations]
14-error_dir: /var/tmp/lperr.test
15-
16-[sharing_jobs]
17-error_dir: /var/tmp/sharing.test
18-
19-[upgrade_branches]
20-error_dir: /var/tmp/codehosting.test
21-
22->>>>>>> MERGE-SOURCE
23 [personalpackagearchive]
24 root: /var/tmp/ppa.test/
25
26
27=== modified file 'lib/lp/bugs/model/bug.py'
28--- lib/lp/bugs/model/bug.py 2012-05-21 00:05:24 +0000
29+++ lib/lp/bugs/model/bug.py 2012-05-21 00:05:24 +0000
30@@ -4,6 +4,7 @@
31 # pylint: disable-msg=E0611,W0212
32
33 """Launchpad bug-related database table classes."""
34+from lp.app.interfaces.services import IService
35
36 __metaclass__ = type
37
38@@ -839,6 +840,15 @@
39 # Ensure that the subscription has been flushed.
40 Store.of(sub).flush()
41
42+ # Grant the subscriber access if they can't see the bug (if the
43+ # database triggers aren't going to do it for us).
44+ trigger_flag = 'disclosure.access_mirror_triggers.removed'
45+ if bool(getFeatureFlag(trigger_flag)):
46+ service = getUtility(IService, 'sharing')
47+ bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
48+ if not bugs:
49+ service.createAccessGrants(subscribed_by, person, bugs=[self])
50+
51 # In some cases, a subscription should be created without
52 # email notifications. suppress_notify determines if
53 # notifications are sent.
54
55=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
56--- lib/lp/bugs/tests/test_bugvisibility.py 2012-05-21 00:05:24 +0000
57+++ lib/lp/bugs/tests/test_bugvisibility.py 2012-05-21 00:05:24 +0000
58@@ -18,6 +18,8 @@
59
60 LEGACY_VISIBILITY_FLAG = {
61 u"disclosure.legacy_subscription_visibility.enabled": u"true"}
62+TRIGGERS_REMOVED_FLAG = {
63+ u"disclosure.access_mirror_triggers.removed": u"true"}
64
65
66 class TestPublicBugVisibility(TestCaseWithFactory):
67@@ -123,3 +125,23 @@
68 with self.disable_trigger_fixture:
69 self.bug.unsubscribe(user, self.owner)
70 self.assertTrue(self.bug.userCanView(user))
71+
72+ def test_subscribeGrantsVisibilityWithTriggersRemoved(self):
73+ # When a user is subscribed to a bug, they are granted access. In this
74+ # test, the database triggers are removed and so model code is used.
75+ with FeatureFixture(TRIGGERS_REMOVED_FLAG):
76+ with self.disable_trigger_fixture:
77+ user = self.factory.makePerson()
78+ self.assertFalse(self.bug.userCanView(user))
79+ with celebrity_logged_in('admin'):
80+ self.bug.subscribe(user, self.owner)
81+ self.assertTrue(self.bug.userCanView(user))
82+
83+ def test_subscribeGrantsVisibilityUsingTriggers(self):
84+ # When a user is subscribed to a bug, they are granted access. In this
85+ # test, the database triggers are used.
86+ user = self.factory.makePerson()
87+ self.assertFalse(self.bug.userCanView(user))
88+ with celebrity_logged_in('admin'):
89+ self.bug.subscribe(user, self.owner)
90+ self.assertTrue(self.bug.userCanView(user))
91
92=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
93--- lib/lp/registry/interfaces/accesspolicy.py 2012-05-21 00:05:24 +0000
94+++ lib/lp/registry/interfaces/accesspolicy.py 2012-05-21 00:05:24 +0000
95@@ -283,5 +283,5 @@
96 """Find the `IAccessArtifact`s for grantee and policies.
97
98 :param grantee: the access artifact grantee.
99- :param policies: a collection of `IAccesPolicy`s.
100+ :param policies: a collection of `IAccessPolicy`s.
101 """
102
103=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
104--- lib/lp/registry/interfaces/sharingservice.py 2012-05-21 00:05:24 +0000
105+++ lib/lp/registry/interfaces/sharingservice.py 2012-05-21 00:05:24 +0000
106@@ -58,6 +58,18 @@
107 :return: a (bugtasks, branches) tuple
108 """
109
110+ def getVisibleArtifacts(person, branches=None, bugs=None):
111+ """Return the artifacts shared with person.
112+
113+ Given lists of artifacts, return those a person has access to either
114+ via a policy grant or artifact grant.
115+
116+ :param person: the person whose access is being checked.
117+ :param branches: the branches to check for which a person has access.
118+ :param bugs: the bugs to check for which a person has access.
119+ :return: a collection of artifacts the person can see.
120+ """
121+
122 def getInformationTypes(pillar):
123 """Return the allowed information types for the given pillar."""
124
125@@ -149,3 +161,21 @@
126 :param bugs: the bugs for which to revoke access
127 :param branches: the branches for which to revoke access
128 """
129+
130+ @export_write_operation()
131+ @call_with(user=REQUEST_USER)
132+ @operation_parameters(
133+ sharee=Reference(IPerson, title=_('Sharee'), required=True),
134+ bugs=List(
135+ Reference(schema=IBug), title=_('Bugs'), required=False),
136+ branches=List(
137+ Reference(schema=IBranch), title=_('Branches'), required=False))
138+ @operation_for_version('devel')
139+ def createAccessGrants(user, sharee, branches=None, bugs=None):
140+ """Grant a sharee access to the specified artifacts.
141+
142+ :param user: the user making the request
143+ :param sharee: the person or team for whom to grant access
144+ :param bugs: the bugs for which to grant access
145+ :param branches: the branches for which to grant access
146+ """
147
148=== modified file 'lib/lp/registry/services/sharingservice.py'
149--- lib/lp/registry/services/sharingservice.py 2012-05-21 00:05:24 +0000
150+++ lib/lp/registry/services/sharingservice.py 2012-05-21 00:05:24 +0000
151@@ -8,6 +8,8 @@
152 'SharingService',
153 ]
154
155+from itertools import product
156+
157 from lazr.restful.interfaces import IWebBrowserOriginatingRequest
158 from lazr.restful.utils import get_current_web_service_request
159 from zope.component import getUtility
160@@ -31,7 +33,6 @@
161 IAccessPolicyGrantSource,
162 IAccessPolicySource,
163 )
164-from lp.registry.interfaces.distribution import IDistribution
165 from lp.registry.interfaces.product import IProduct
166 from lp.registry.interfaces.projectgroup import IProjectGroup
167 from lp.registry.interfaces.sharingjob import IRemoveSubscriptionsJobSource
168@@ -39,7 +40,10 @@
169 from lp.registry.model.person import Person
170 from lp.services.features import getFeatureFlag
171 from lp.services.searchbuilder import any
172-from lp.services.webapp.authorization import available_with_permission
173+from lp.services.webapp.authorization import (
174+ available_with_permission,
175+ check_permission,
176+ )
177
178
179 class SharingService:
180@@ -58,8 +62,11 @@
181
182 @property
183 def write_enabled(self):
184- return bool(getFeatureFlag(
185- 'disclosure.enhanced_sharing.writable'))
186+ return (
187+ bool(getFeatureFlag(
188+ 'disclosure.enhanced_sharing.writable') or
189+ bool(getFeatureFlag(
190+ 'disclosure.access_mirror_triggers.removed'))))
191
192 def getSharedArtifacts(self, pillar, person, user):
193 """See `ISharingService`."""
194@@ -89,6 +96,33 @@
195
196 return bugtasks, branches
197
198+ def getVisibleArtifacts(self, person, branches=None, bugs=None):
199+ """See `ISharingService`."""
200+ bugs_by_id = {}
201+ branches_by_id = {}
202+ for bug in bugs or []:
203+ bugs_by_id[bug.id] = bug
204+ for branch in branches or []:
205+ branches_by_id[branch.id] = branch
206+
207+ # Load the bugs.
208+ visible_bug_ids = []
209+ if bugs_by_id:
210+ param = BugTaskSearchParams(
211+ user=person, bug=any(*bugs_by_id.keys()))
212+ visible_bug_ids = list(getUtility(IBugTaskSet).searchBugIds(param))
213+ visible_bugs = [bugs_by_id[bug_id] for bug_id in visible_bug_ids]
214+
215+ # Load the branches.
216+ visible_branches = []
217+ if branches_by_id:
218+ all_branches = getUtility(IAllBranches)
219+ wanted_branches = all_branches.visibleByUser(person).withIds(
220+ *branches_by_id.keys())
221+ visible_branches = list(wanted_branches.getBranches())
222+
223+ return visible_bugs, visible_branches
224+
225 def getInformationTypes(self, pillar):
226 """See `ISharingService`."""
227 allowed_types = [
228@@ -313,3 +347,27 @@
229 # longer see.
230 getUtility(IRemoveSubscriptionsJobSource).create(
231 pillar, sharee, user, bugs=bugs, branches=branches)
232+
233+ def createAccessGrants(self, user, sharee, branches=None, bugs=None):
234+ """See `ISharingService`."""
235+
236+ if not self.write_enabled:
237+ raise Unauthorized("This feature is not yet enabled.")
238+
239+ artifacts = []
240+ if branches:
241+ artifacts.extend(branches)
242+ if bugs:
243+ artifacts.extend(bugs)
244+ # The user needs to have launchpad.Edit permission on all supplied
245+ # bugs and branches or else we raise an Unauthorized exception.
246+ for artifact in artifacts or []:
247+ if not check_permission('launchpad.Edit', artifact):
248+ raise Unauthorized
249+
250+ # Ensure there are access artifacts associated with the bugs and
251+ # branches.
252+ artifacts = getUtility(IAccessArtifactSource).ensure(artifacts)
253+ # Create access to bugs/branches for the specified sharee.
254+ getUtility(IAccessArtifactGrantSource).grant(
255+ list(product(artifacts, [sharee], [user])))
256
257=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
258--- lib/lp/registry/services/tests/test_sharingservice.py 2012-05-21 00:05:24 +0000
259+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-05-21 00:05:24 +0000
260@@ -13,11 +13,13 @@
261 from zope.traversing.browser.absoluteurl import absoluteURL
262
263 from lp.app.interfaces.services import IService
264+from lp.bugs.interfaces.bug import IBug
265 from lp.code.enums import (
266 BranchSubscriptionDiffSize,
267 BranchSubscriptionNotificationLevel,
268 CodeReviewNotificationLevel,
269 )
270+from lp.code.interfaces.branch import IBranch
271 from lp.registry.enums import (
272 InformationType,
273 SharingPermission,
274@@ -786,6 +788,89 @@
275 Unauthorized, self.service.revokeAccessGrants,
276 product, product.owner, sharee, bugs=[bug])
277
278+ def _assert_createAccessGrants(self, user, bugs, branches):
279+ # Creating access grants works as expected.
280+ grantee = self.factory.makePerson()
281+ with FeatureFixture(WRITE_FLAG):
282+ self.service.createAccessGrants(
283+ user, grantee, bugs=bugs, branches=branches)
284+
285+ # Check that grantee has expected access grants.
286+ shared_bugs = []
287+ shared_branches = []
288+ all_pillars = []
289+ for bug in bugs or []:
290+ all_pillars.extend(bug.affected_pillars)
291+ for branch in branches or []:
292+ all_pillars.append(branch.target.context)
293+ policies = getUtility(IAccessPolicySource).findByPillar(all_pillars)
294+
295+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
296+ access_artifacts = apgfs.findArtifactsByGrantee(grantee, policies)
297+ for a in access_artifacts:
298+ if IBug.providedBy(a.concrete_artifact):
299+ shared_bugs.append(a.concrete_artifact)
300+ elif IBranch.providedBy(a.concrete_artifact):
301+ shared_branches.append(a.concrete_artifact)
302+ self.assertContentEqual(bugs or [], shared_bugs)
303+ self.assertContentEqual(branches or [], shared_branches)
304+
305+ def test_createAccessGrantsBugs(self):
306+ # Access grants can be created for bugs.
307+ owner = self.factory.makePerson()
308+ distro = self.factory.makeDistribution(owner=owner)
309+ login_person(owner)
310+ bug = self.factory.makeBug(
311+ distribution=distro, owner=owner,
312+ information_type=InformationType.USERDATA)
313+ self._assert_createAccessGrants(owner, [bug], None)
314+
315+ def test_createAccessGrantsBranches(self):
316+ # Access grants can be created for branches.
317+ owner = self.factory.makePerson()
318+ product = self.factory.makeProduct(owner=owner)
319+ login_person(owner)
320+ branch = self.factory.makeBranch(
321+ product=product, owner=owner, private=True)
322+ self._assert_createAccessGrants(owner, None, [branch])
323+
324+ def _assert_createAccessGrantsUnauthorized(self, user):
325+ # createAccessGrants raises an Unauthorized exception if the user
326+ # is not permitted to do so.
327+ product = self.factory.makeProduct()
328+ bug = self.factory.makeBug(
329+ product=product, information_type=InformationType.USERDATA)
330+ sharee = self.factory.makePerson()
331+ with FeatureFixture(WRITE_FLAG):
332+ self.assertRaises(
333+ Unauthorized, self.service.createAccessGrants,
334+ user, sharee, bugs=[bug])
335+
336+ def test_createAccessGrantsAnonymous(self):
337+ # Anonymous users are not allowed.
338+ with FeatureFixture(WRITE_FLAG):
339+ login(ANONYMOUS)
340+ self._assert_createAccessGrantsUnauthorized(ANONYMOUS)
341+
342+ def test_createAccessGrantsAnyone(self):
343+ # Unauthorized users are not allowed.
344+ with FeatureFixture(WRITE_FLAG):
345+ anyone = self.factory.makePerson()
346+ login_person(anyone)
347+ self._assert_createAccessGrantsUnauthorized(anyone)
348+
349+ def test_createAccessGrants_without_flag(self):
350+ # The feature flag needs to be enabled.
351+ owner = self.factory.makePerson()
352+ product = self.factory.makeProduct(owner=owner)
353+ bug = self.factory.makeBug(
354+ product=product, information_type=InformationType.USERDATA)
355+ sharee = self.factory.makePerson()
356+ login_person(owner)
357+ self.assertRaises(
358+ Unauthorized, self.service.createAccessGrants,
359+ product.owner, sharee, bugs=[bug])
360+
361 def test_getSharedArtifacts(self):
362 # Test the getSharedArtifacts method.
363 owner = self.factory.makePerson()
364@@ -829,7 +914,8 @@
365 [(product, InformationType.USERDATA)])
366 for i, branch in enumerate(branches):
367 artifact = grant_access(branch, i == 9)
368- # XXX for now we need to subscribe users to the branch in order
369+ # XXX bug=1001042 wallyworld 2012-05-18
370+ # for now we need to subscribe users to the branch in order
371 # for the underlying BranchCollection to allow access. This will
372 # no longer be the case when BranchCollection supports the new
373 # access policy framework.
374@@ -848,6 +934,54 @@
375 self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
376 self.assertContentEqual(branches[:9], shared_branches)
377
378+ def test_getVisibleArtifacts(self):
379+ # Test the getVisibleArtifacts method.
380+ owner = self.factory.makePerson()
381+ product = self.factory.makeProduct(owner=owner)
382+ grantee = self.factory.makePerson()
383+ login_person(owner)
384+
385+ bugs = []
386+ for x in range(0, 10):
387+ bug = self.factory.makeBug(
388+ product=product, owner=owner,
389+ information_type=InformationType.USERDATA)
390+ bugs.append(bug)
391+ branches = []
392+ for x in range(0, 10):
393+ branch = self.factory.makeBranch(
394+ product=product, owner=owner, private=True)
395+ branches.append(branch)
396+
397+ def grant_access(artifact):
398+ access_artifact = self.factory.makeAccessArtifact(
399+ concrete=artifact)
400+ self.factory.makeAccessArtifactGrant(
401+ artifact=access_artifact, grantee=grantee, grantor=owner)
402+ return access_artifact
403+
404+ # Grant access to some of the bugs and branches.
405+ for bug in bugs[:5]:
406+ grant_access(bug)
407+ for branch in branches[:5]:
408+ grant_access(branch)
409+ # XXX bug=1001042 wallyworld 2012-05-18
410+ # for now we need to subscribe users to the branch in order
411+ # for the underlying BranchCollection to allow access. This will
412+ # no longer be the case when BranchCollection supports the new
413+ # access policy framework.
414+ branch.subscribe(
415+ grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
416+ BranchSubscriptionDiffSize.NODIFF,
417+ CodeReviewNotificationLevel.NOEMAIL,
418+ owner)
419+
420+ # Check the results.
421+ shared_bugs, shared_branches = self.service.getVisibleArtifacts(
422+ grantee, branches, bugs)
423+ self.assertContentEqual(bugs[:5], shared_bugs)
424+ self.assertContentEqual(branches[:5], shared_branches)
425+
426
427 class ApiTestMixin:
428 """Common tests for launchpadlib and webservice."""
429
430=== modified file 'lib/lp/services/features/flags.py'
431--- lib/lp/services/features/flags.py 2012-05-21 00:05:24 +0000
432+++ lib/lp/services/features/flags.py 2012-05-21 00:05:24 +0000
433@@ -295,6 +295,13 @@
434 '',
435 'Sharing management',
436 ''),
437+ ('disclosure.access_mirror_triggers.removed',
438+ 'boolean',
439+ ('If true, the database triggers which cause subscribing to grant'
440+ 'access to a bug or branch have been removed.'),
441+ '',
442+ '',
443+ ''),
444 ('garbo.workitem_migrator.enabled',
445 'boolean',
446 ('If true, garbo will try to migrate work items from the whiteboard of '
447
448=== modified file 'lib/lp/testing/factory.py'
449--- lib/lp/testing/factory.py 2012-05-04 14:52:44 +0000
450+++ lib/lp/testing/factory.py 2012-05-21 00:05:24 +0000
451@@ -1122,6 +1122,12 @@
452 if private:
453 removeSecurityProxy(branch).explicitly_private = True
454 removeSecurityProxy(branch).transitively_private = True
455+ # XXX this is here till branch properly supports information_type
456+ [artifact] = getUtility(IAccessArtifactSource).ensure([branch])
457+ [policy] = getUtility(IAccessPolicySource).find(
458+ [(branch.target.context, InformationType.USERDATA)])
459+ getUtility(IAccessPolicyArtifactSource).create([(artifact, policy)])
460+
461 if stacked_on is not None:
462 removeSecurityProxy(branch).stacked_on = stacked_on
463 if reviewer is not None: