Launchpad itself

Merge lp:~wgrant/launchpad/structural-subscription-security into lp:launchpad

  1. structural-subscription-security
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Jonathan Lange
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/structural-subscription-security
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~wgrant/launchpad/structural-subscription-security
Reviewer Review Type Date Requested Status
Julian Edwards (community) last revision only Approve
Gary Poster (community) Approve
Review via email: mp+10776@code.launchpad.net

Commit message

[r=gary][ui=none] (wgrant) Move structural subscription security to the model, rather than the view.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

= Summary =
Structural subscriptions want to be exported through the API. This is the third of four branches: moving access control into the model, while it currently just resides in the view.

== Proposed fix ==
Implement access control in IStructuralSubscriptionTarget.{add,remove}BugSubscription.

== Implementation details ==
IStructuralSubscriptionTarget.addBugSubscription already took the person performing the action as an argument, but removeBugSubscription needed it to be added.

addBugSubscription calls addSubscription, so access control was implemented there and in removeBugSubscription. I threw the privilege checking code into SST._userCanAlterSubscriptionForPerson.

The only other behavioral change is defaulting the subscriber in addSubscription, removeSubscription to subscribed_by. This will be useful for the API.

== Tests ==

 $ bin/test -vvt structural-subscriptions.txt -t initial-bug-contacts.txt -t has-bug-supervisor.txt -t bugs-page.txt

Revision history for this message
Gary Poster (gary) wrote :

This looks very good. Thank you.

I'm sorely tempted to suggest microoptimizations in _userCanAlterSubscription--in particular, optimizing for what feels like the common case by moving the "subscriber is subscribed_by" check as the very first one (that is, before the distribution source package stuff). If you object on principal (premature microoptimization), I'm fine with it. OTOH, if you like it, go for it.

Do I understand correctly that the next branch will expose this in the REST API? That's a curiosity question.

And are you going to request a UI review for the error behavior? That is, if I read the code correctly, as of your current branch, if I try to unsubscribe someone for whom I do not have sufficient privileges through the browser, I will get an OOPS page. That's mostly OK if I never get the option in the UI, but not so good otherwise. Is that also going to be addressed, or already addressed in a way that I don't see? That's a more serious question.

If the answer to my UI question is that my concerns are addressed, are unfounded, or will be addressed before this lands, I approve. Otherwise, let's talk.

I'll be optimistic and mark this approved so I'm not blocking you.

review: Approve
Revision history for this message
William Grant (wgrant) wrote :

> This looks very good. Thank you.
>
> I'm sorely tempted to suggest microoptimizations in _userCanAlterSubscription
> --in particular, optimizing for what feels like the common case by moving the
> "subscriber is subscribed_by" check as the very first one (that is, before the
> distribution source package stuff). If you object on principal (premature
> microoptimization), I'm fine with it. OTOH, if you like it, go for it.

I think it's more readable as it is, and only a tiny tiny bit less efficient.

> Do I understand correctly that the next branch will expose this in the REST
> API? That's a curiosity question.

That's correct.

> And are you going to request a UI review for the error behavior? That is, if
> I read the code correctly, as of your current branch, if I try to unsubscribe
> someone for whom I do not have sufficient privileges through the browser, I
> will get an OOPS page. That's mostly OK if I never get the option in the UI,
> but not so good otherwise. Is that also going to be addressed, or already
> addressed in a way that I don't see? That's a more serious question.

The view code implements this same restriction. The form that removes
structural subscriptions gives you checkboxes for each team that you administer.
The code behind it also removes from the submitted list of teams any that you
lack privileges for, before calling removeBugSubscription.

So, your evil browser unsubscription request will simply be ignored as before.

> If the answer to my UI question is that my concerns are addressed, are
> unfounded, or will be addressed before this lands, I approve. Otherwise,
> let's talk.
>
> I'll be optimistic and mark this approved so I'm not blocking you.

Thanks for the review. I might try to convince somebody else to merge it,
since you're gone.

Revision history for this message
Gary Poster (gary) wrote :

Thanks for the reply. Sounds good.

> I might try to convince somebody else to merge it,
> since you're gone.

I checked with wgrant, and mwhudson is helping with the merge. Thanks to both.

Revision history for this message
William Grant (wgrant) wrote :

> Thanks for the reply. Sounds good.
>
> > I might try to convince somebody else to merge it,
> > since you're gone.
>
> I checked with wgrant, and mwhudson is helping with the merge. Thanks to
> both.

So far mwhudson, thumper and jml have tried to ec2test it.

Each time, no email has arrived and the instance has vanished. I am scared.

jml is apparently going to run the tests on EC2 without going headless,
so he can see what is going wrong...

Revision history for this message
William Grant (wgrant) wrote :

ec2test eventually became happy with my branch, but there was a test failure. I've fixed it in r9216.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

<bigjools> wgrant: addSubscription needs an admin?
<wgrant> bigjools: Needs a member of the team.
<wgrant> bigjools: Or an admin.
<wgrant> bigjools: There were no readily accessible team members, so I used an admin.
<bigjools> wgrant: is this fact pertinent to the test, or is it just setting up test conditions?
<wgrant> bigjools: The latter.

review: Approve (last revision only)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/browser/structuralsubscription.py'
2--- lib/canonical/launchpad/browser/structuralsubscription.py 2009-08-24 01:09:07 +0000
3+++ lib/canonical/launchpad/browser/structuralsubscription.py 2009-08-25 11:08:42 +0000
4@@ -167,7 +167,7 @@
5 'e-mail each time someone reports or changes one of '
6 'its public bugs.' % target.displayname)
7 elif is_subscribed and not subscribe:
8- target.removeBugSubscription(self.user)
9+ target.removeBugSubscription(self.user, self.user)
10 self.request.response.addNotification(
11 'You have unsubscribed from "%s". You '
12 'will no longer automatically receive e-mail about '
13@@ -197,7 +197,7 @@
14 team.displayname, self.context.displayname))
15
16 for team in subscriptions - form_selected_teams:
17- target.removeBugSubscription(team)
18+ target.removeBugSubscription(team, self.user)
19 self.request.response.addNotification(
20 'The %s team will no longer automatically receive '
21 'e-mail about changes to public bugs in "%s".' % (
22@@ -220,7 +220,7 @@
23
24 subscriptions_to_remove = data.get('remove_other_subscriptions', [])
25 for subscription in subscriptions_to_remove:
26- target.removeBugSubscription(subscription)
27+ target.removeBugSubscription(subscription, self.user)
28 self.request.response.addNotification(
29 '%s will no longer automatically receive e-mail about '
30 'public bugs in "%s".' % (
31
32=== modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
33--- lib/canonical/launchpad/database/structuralsubscription.py 2009-07-23 13:44:13 +0000
34+++ lib/canonical/launchpad/database/structuralsubscription.py 2009-08-25 12:04:52 +0000
35@@ -5,6 +5,7 @@
36 __all__ = ['StructuralSubscription',
37 'StructuralSubscriptionTargetMixin']
38
39+from zope.component import getUtility
40 from zope.interface import implements
41
42 from sqlobject import ForeignKey
43@@ -16,9 +17,10 @@
44
45 from canonical.launchpad.interfaces import (
46 BlueprintNotificationLevel, BugNotificationLevel, DeleteSubscriptionError,
47- IDistribution, IDistributionSourcePackage, IDistroSeries, IMilestone,
48- IProduct, IProductSeries, IProject, IStructuralSubscription,
49- IStructuralSubscriptionTarget)
50+ IDistribution, IDistributionSourcePackage, IDistroSeries,
51+ ILaunchpadCelebrities, IMilestone, IProduct, IProductSeries, IProject,
52+ IStructuralSubscription, IStructuralSubscriptionTarget,
53+ UserCannotSubscribePerson)
54 from lp.registry.interfaces.person import (
55 validate_public_person, validate_person_not_private_membership)
56
57@@ -129,8 +131,35 @@
58 '%s is not a valid structural subscription target.')
59 return args
60
61+ def _userCanAlterSubscription(self, subscriber, subscribed_by):
62+ """Check if a user can change a subscription for a person."""
63+ # A Launchpad administrator or the user can subscribe a user.
64+ # A Launchpad or team admin can subscribe a team.
65+
66+ # Nobody else can, unless the context is a IDistributionSourcePackage,
67+ # in which case the drivers or owner can.
68+ if IDistributionSourcePackage.providedBy(self):
69+ for driver in self.distribution.drivers:
70+ if subscribed_by.inTeam(driver):
71+ return True
72+ if subscribed_by.inTeam(self.distribution.owner):
73+ return True
74+
75+ admins = getUtility(ILaunchpadCelebrities).admin
76+ return (subscriber is subscribed_by or
77+ subscriber in subscribed_by.getAdministratedTeams() or
78+ subscribed_by.inTeam(admins))
79+
80 def addSubscription(self, subscriber, subscribed_by):
81 """See `IStructuralSubscriptionTarget`."""
82+ if subscriber is None:
83+ subscriber = subscribed_by
84+
85+ if not self._userCanAlterSubscription(subscriber, subscribed_by):
86+ raise UserCannotSubscribePerson(
87+ '%s does not have permission to subscribe %s.' % (
88+ subscribed_by.name, subscriber.name))
89+
90 existing_subscription = self.getSubscription(subscriber)
91
92 if existing_subscription is not None:
93@@ -151,20 +180,28 @@
94 sub.bug_notification_level = BugNotificationLevel.COMMENTS
95 return sub
96
97- def removeBugSubscription(self, person):
98+ def removeBugSubscription(self, subscriber, unsubscribed_by):
99 """See `IStructuralSubscriptionTarget`."""
100+ if subscriber is None:
101+ subscriber = unsubscribed_by
102+
103+ if not self._userCanAlterSubscription(subscriber, unsubscribed_by):
104+ raise UserCannotSubscribePerson(
105+ '%s does not have permission to unsubscribe %s.' % (
106+ unsubscribed_by.name, subscriber.name))
107+
108 subscription_to_remove = None
109 for subscription in self.getSubscriptions(
110 min_bug_notification_level=BugNotificationLevel.METADATA):
111 # Only search for bug subscriptions
112- if subscription.subscriber == person:
113+ if subscription.subscriber == subscriber:
114 subscription_to_remove = subscription
115 break
116
117 if subscription_to_remove is None:
118 raise DeleteSubscriptionError(
119 "%s is not subscribed to %s." % (
120- person.name, self.displayname))
121+ subscriber.name, self.displayname))
122 else:
123 if (subscription_to_remove.blueprint_notification_level >
124 BlueprintNotificationLevel.NOTHING):
125
126=== modified file 'lib/canonical/launchpad/doc/structural-subscriptions.txt'
127--- lib/canonical/launchpad/doc/structural-subscriptions.txt 2009-04-17 10:32:16 +0000
128+++ lib/canonical/launchpad/doc/structural-subscriptions.txt 2009-08-25 11:08:42 +0000
129@@ -160,8 +160,8 @@
130
131 >>> evolution_sub.blueprint_notification_level = (
132 ... BlueprintNotificationLevel.METADATA)
133- >>> evolution_package.removeBugSubscription(sampleperson)
134- >>> ubuntu.removeBugSubscription(sampleperson)
135+ >>> evolution_package.removeBugSubscription(sampleperson, sampleperson)
136+ >>> ubuntu.removeBugSubscription(sampleperson, sampleperson)
137 >>> syncUpdate(evolution_sub)
138
139 Sample Person is no longer a subscriber to the package, but Foo Bar
140
141=== modified file 'lib/canonical/launchpad/interfaces/ftests/structural-subscription-target.txt'
142--- lib/canonical/launchpad/interfaces/ftests/structural-subscription-target.txt 2008-02-11 17:44:30 +0000
143+++ lib/canonical/launchpad/interfaces/ftests/structural-subscription-target.txt 2009-08-25 11:08:42 +0000
144@@ -48,31 +48,100 @@
145 >>> target.addBugSubscription(no_priv, no_priv)
146 <StructuralSubscription ...>
147
148-Let's add an ITeam as one of the subscribers:
149+People can only be subscribed by themselves, and only the team admins may
150+subscribe a team.
151+
152+no-priv, who has no relationship to ubuntu-team, cannot subscribe it.
153
154 >>> ubuntu_team = personset.getByName("ubuntu-team")
155- >>> target.addBugSubscription(ubuntu_team, ubuntu_team)
156- <StructuralSubscription ...>
157-
158- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
159- [u'no-priv', u'ubuntu-team']
160+ >>> target.addBugSubscription(ubuntu_team, no_priv)
161+ Traceback (most recent call last):
162+ ...
163+ UserCannotSubscribePerson: no-priv does not have permission to subscribe ubuntu-team.
164+
165+But kamion, an admin of the team, can.
166+
167+ >>> kamion = personset.getByName("kamion")
168+ >>> target.addBugSubscription(ubuntu_team, kamion)
169+ <StructuralSubscription ...>
170+
171+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
172+ [u'no-priv', u'ubuntu-team']
173+
174+foobar, a Launchpad administrator, can as well.
175+
176+ >>> foobar = personset.getByName("name16")
177+ >>> target.addBugSubscription(ubuntu_team, foobar)
178+ <StructuralSubscription ...>
179+
180+A non-admin cannot subscribe a person other than themselves.
181+
182+ >>> target.addBugSubscription(kamion, no_priv)
183+ Traceback (most recent call last):
184+ ...
185+ UserCannotSubscribePerson: no-priv does not have permission to subscribe kamion.
186+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
187+ [u'no-priv', u'ubuntu-team']
188+
189+But again, an admin can.
190+
191+ >>> target.addBugSubscription(kamion, foobar)
192+ <StructuralSubscription ...>
193+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
194+ [u'kamion', u'no-priv', u'ubuntu-team']
195
196 To remove a bug subscription, use
197 IStructuralSubscriptionTarget.removeBugSubscription:
198
199- >>> target.removeBugSubscription(no_priv)
200- >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
201- [u'ubuntu-team']
202+ >>> target.removeBugSubscription(no_priv, no_priv)
203+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
204+ [u'kamion', u'ubuntu-team']
205+
206+The subscription rules apply to unsubscription as well.
207+
208+An unprivileged user cannot unsubscribe a team.
209+
210+ >>> target.removeBugSubscription(ubuntu_team, no_priv)
211+ Traceback (most recent call last):
212+ ...
213+ UserCannotSubscribePerson: no-priv does not have permission to unsubscribe ubuntu-team.
214+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
215+ [u'kamion', u'ubuntu-team']
216+
217+But a team admin can.
218+
219+ >>> target.removeBugSubscription(ubuntu_team, kamion)
220+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
221+ [u'kamion']
222+
223+An unprivileged user also cannot unsubscribe another user.
224+
225+ >>> target.removeBugSubscription(kamion, no_priv)
226+ Traceback (most recent call last):
227+ ...
228+ UserCannotSubscribePerson: no-priv does not have permission to unsubscribe kamion.
229+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
230+ [u'kamion']
231+
232+But the user themselves can.
233+
234+ >>> target.removeBugSubscription(kamion, kamion)
235+ >>> sorted([sub.subscriber.name for sub in target.bug_subscriptions])
236+ []
237
238 Trying to remove a subscription that doesn't exist on a target raises a
239 DeleteSubscriptionError.
240
241- >>> foobar = personset.getByName("name16")
242- >>> target.removeBugSubscription(foobar)
243+ >>> target.removeBugSubscription(foobar, foobar)
244 Traceback (most recent call last):
245 ...
246 DeleteSubscriptionError: ...
247
248+Let's subscribe ubuntu-team again.
249+
250+ >>> target.addBugSubscription(ubuntu_team, foobar)
251+ <StructuralSubscription ...>
252+
253 Trying to remove a bug subscription when notification levels for other
254 applications are set, doesn't remove the subscription. Instead the
255 notification level for bugs is set to NOTHING.
256@@ -93,7 +162,7 @@
257 >>> print_subscriptions_list(target.getSubscriptions())
258 name16 COMMENTS LIFECYCLE
259 ubuntu-team COMMENTS NOTHING
260- >>> target.removeBugSubscription(foobar)
261+ >>> target.removeBugSubscription(foobar, foobar)
262 >>> print_subscriptions_list(target.getSubscriptions())
263 name16 NOTHING LIFECYCLE
264 ubuntu-team COMMENTS NOTHING
265
266=== modified file 'lib/canonical/launchpad/interfaces/structuralsubscription.py'
267--- lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-07-17 00:26:05 +0000
268+++ lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-08-25 11:08:42 +0000
269@@ -13,7 +13,8 @@
270 'DeleteSubscriptionError',
271 'IStructuralSubscription',
272 'IStructuralSubscriptionForm',
273- 'IStructuralSubscriptionTarget'
274+ 'IStructuralSubscriptionTarget',
275+ 'UserCannotSubscribePerson',
276 ]
277
278 from zope.interface import Attribute, Interface
279@@ -165,7 +166,7 @@
280 :return: The new bug subscription.
281 """
282
283- def removeBugSubscription(subscriber):
284+ def removeBugSubscription(subscriber, unsubscribed_by):
285 """Remove a subscription to bugs from this structure.
286
287 If subscription levels for other applications are set,
288@@ -173,6 +174,7 @@
289 `NOTHING`, otherwise, destroy the subscription.
290
291 :subscriber: The IPerson who will be subscribed.
292+ :unsubscribed_by: The IPerson removing the subscription.
293 """
294
295 def getSubscription(person):
296@@ -207,3 +209,7 @@
297
298 Raised when an error occurred trying to delete a
299 structural subscription."""
300+
301+
302+class UserCannotSubscribePerson(Exception):
303+ """User does not have permission to subscribe the person or team."""
304
305=== modified file 'lib/lp/bugs/doc/initial-bug-contacts.txt'
306--- lib/lp/bugs/doc/initial-bug-contacts.txt 2009-08-13 15:12:16 +0000
307+++ lib/lp/bugs/doc/initial-bug-contacts.txt 2009-08-25 11:08:42 +0000
308@@ -60,7 +60,7 @@
309 To remove a subscription, use
310 IStructuralSubscriptionTarget.removeBugSubscription:
311
312- >>> debian_firefox.removeBugSubscription(sample_person)
313+ >>> debian_firefox.removeBugSubscription(sample_person, sample_person)
314 >>> sorted([sub.subscriber.id for sub in debian_firefox.bug_subscriptions])
315 [17]
316
317@@ -68,7 +68,7 @@
318 DeleteSubscriptionError.
319
320 >>> foobar = personset.getByName("name16")
321- >>> debian_firefox.removeBugSubscription(foobar)
322+ >>> debian_firefox.removeBugSubscription(foobar, foobar)
323 Traceback (most recent call last):
324 ...
325 DeleteSubscriptionError: ...
326
327=== modified file 'lib/lp/bugs/tests/has-bug-supervisor.txt'
328--- lib/lp/bugs/tests/has-bug-supervisor.txt 2009-06-12 16:36:02 +0000
329+++ lib/lp/bugs/tests/has-bug-supervisor.txt 2009-08-25 11:08:42 +0000
330@@ -50,7 +50,7 @@
331
332 If no_priv unsubscribes, he is still set as the bug supervisor.
333
334- >>> target.removeBugSubscription(no_priv)
335+ >>> target.removeBugSubscription(no_priv, no_priv)
336 >>> print target.bug_supervisor.preferredemail.email
337 no-priv@canonical.com
338 >>> no_priv in bug.getIndirectSubscribers()