Merge lp:~leonardr/launchpad/anonymous-oauth into lp:launchpad/db-devel

Proposed by Leonard Richardson
Status: Merged
Approved by: Guilherme Salgado
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~leonardr/launchpad/anonymous-oauth
Merge into: lp:launchpad/db-devel
Diff against target: 837 lines (+406/-63)
17 files modified
lib/canonical/launchpad/doc/account.txt (+7/-2)
lib/canonical/launchpad/interfaces/account.py (+8/-8)
lib/canonical/launchpad/pagetests/webservice/xx-service.txt (+72/-0)
lib/canonical/launchpad/security.py (+36/-25)
lib/canonical/launchpad/testing/pages.py (+31/-5)
lib/canonical/launchpad/webapp/servers.py (+35/-3)
lib/canonical/launchpad/zcml/account.zcml (+4/-1)
lib/lp/bugs/scripts/checkwatches.py (+10/-1)
lib/lp/bugs/scripts/tests/test_checkwatches.py (+83/-0)
lib/lp/registry/browser/configure.zcml (+1/-1)
lib/lp/registry/browser/person.py (+29/-6)
lib/lp/registry/doc/person-account.txt (+2/-2)
lib/lp/registry/stories/person/xx-admin-person-review.txt (+55/-1)
lib/lp/registry/templates/person-review.pt (+2/-1)
lib/lp/services/permission_helpers.py (+29/-4)
lib/lp/translations/model/translationimportqueue.py (+1/-2)
versions.cfg (+1/-1)
To merge this branch: bzr merge lp:~leonardr/launchpad/anonymous-oauth
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) code Approve
Review via email: mp+16199@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

This branch fixes bug 385517 (but not all of it: see bug 496964). It allows OAuth requests to bypass the normal verification process (and enjoy the privileges of the unauthenticated principal) if their token key is the empty string. All the real data the client has to provide is a consumer key.

If a normal request comes in with an unrecognized consumer key, the request is still rejected. Access tokens are associated with specific known consumers, so there's no way that request can be valid. But an anonymous request is valid even if it mentions a consumer key never seen before. If that happens, my branch automatically creates a consumer object associated with the consumer key, so it can be recognized later.

I created an 'anonymous_webservice' LaunchpadWebServiceCaller for use in testing anonymous access to the web service, and added some basic tests to xx-service.txt.

Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (6.8 KiB)

Hi Leonard,

This looks good; I have only a couple suggestions.

 review needsfixing

On Tue, 2009-12-15 at 15:13 +0000, Leonard Richardson wrote:
> === modified file 'lib/canonical/launchpad/pagetests/webservice/xx-service.txt'
> --- lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2009-08-20 04:46:48 +0000
> +++ lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2009-12-15 15:13:21 +0000
> @@ -56,6 +56,39 @@
> HTTP/1.1 404 Not Found
> ...
>
> +== Anonymous requests ==
> +
> +A properly signed web service request whose OAuth token key is empty
> +is treated as an anonymous request.
> +
> + >>> root = 'http://api.launchpad.dev/beta'
> + >>> body = anon_webservice.get(root).jsonBody()
> + >>> print body['projects_collection_link']
> + http://api.launchpad.dev/beta/projects
> + >>> print body['me_link']
> + http://api.launchpad.dev/beta/people/+me
> +
> +Anonymous requests can't access certain data:
> +
> + >>> response = anon_webservice.get(body['me_link'])
> + >>> print response.getheader('status')
> + 401 Unauthorized
> + >>> print response.body
> + You need to be logged in to view this URL.
> + ...
> +
> +Anonymous requests can't change the dataset.
> +
> + >>> import simplejson
> + >>> data = simplejson.dumps({'display_name' : "This won't work"})
> + >>> response = anon_webservice.patch(root + "/~salgado",
> + ... 'application/json', data)
> + >>> print response.getheader('status')
> + 401 Unauthorized
> + >>> print response.body
> + ...
> + Unauthorized: (<Person at...>, 'displayname', 'launchpad.Edit')
> + ...
>
> == API Requests to other hosts ==
>
>
> === modified file 'lib/canonical/launchpad/testing/pages.py'
> --- lib/canonical/launchpad/testing/pages.py 2009-10-16 17:14:42 +0000
> +++ lib/canonical/launchpad/testing/pages.py 2009-12-15 15:13:21 +0000
> @@ -20,7 +20,8 @@
> from BeautifulSoup import (
> BeautifulSoup, CData, Comment, Declaration, NavigableString, PageElement,
> ProcessingInstruction, SoupStrainer, Tag)
> -from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT
> +from contrib.oauth import (
> + OAuthConsumer, OAuthRequest, OAuthSignatureMethod_PLAINTEXT, OAuthToken)
> from urlparse import urljoin
>
> from zope.app.testing.functional import HTTPCaller, SimpleCookie
> @@ -95,10 +96,15 @@
> """
> if oauth_consumer_key is not None and oauth_access_key is not None:
> login(ANONYMOUS)
> - self.consumer = getUtility(IOAuthConsumerSet).getByKey(
> - oauth_consumer_key)
> - self.access_token = self.consumer.getAccessToken(
> - oauth_access_key)
> + consumers = getUtility(IOAuthConsumerSet)
> + self.consumer = consumers.getByKey(oauth_consumer_key)
> + if self.consumer is None:

I find it really confusing that you rely on a non-registered consumer to
identify anonymous access here -- I was expecting you'd use an empty
token key for that.

To do that, though, you'll need to use the 'launchpad-library' as the
consumer key of your anon_webservice below, but when you do th...

Read more...

review: Needs Fixing
Revision history for this message
Leonard Richardson (leonardr) wrote :

Check the incremental diff: http://pastebin.ubuntu.com/342820/

Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (4.9 KiB)

> === modified file 'lib/canonical/launchpad/testing/pages.py'
> --- lib/canonical/launchpad/testing/pages.py 2009-12-15 14:43:33 +0000
> +++ lib/canonical/launchpad/testing/pages.py 2009-12-16 15:52:00 +0000
> @@ -99,12 +99,24 @@
> consumers = getUtility(IOAuthConsumerSet)
> self.consumer = consumers.getByKey(oauth_consumer_key)
> if self.consumer is None:
> - # Set up for anonymous access
> + # The client wants to make a request using an
> + # unrecognized consumer key. Only an anonymous request
> + # (one in which oauth_access_key is the empty string)
> + # will succeed, but we run this code in either case,
> + # so that we can verify that a non-anonymous request
> + # fails.
> self.consumer = OAuthConsumer(oauth_consumer_key, '')
> - self.access_token = OAuthToken('', '')
> + self.access_token = OAuthToken(oauth_access_key, '')
> else:
> - self.access_token = self.consumer.getAccessToken(
> - oauth_access_key)
> + if oauth_access_key == '':
> + # The client wants to make an anonymous request
> + # using a recognized consumer key.
> + self.access_token = OAuthToken(oauth_access_key, '')
> + else:
> + # The client wants to make an authorized request
> + # using a recognized consumer key.
> + self.access_token = self.consumer.getAccessToken(
> + oauth_access_key)

Although this is now making it clear that we identify anonymous requests
by the lack of an access key, I think this can be simplified a bit.
This is what I had in mind when I first commented on this change.

    if oauth_access_key == '':
        # The client wants to make an anonymous request.
        if self.consumer is None:
            # Anonymous requests don't need a registered consumer, so we
            # manually create a "fake" OauthConsumer (we call it "fake"
            # because it's not really an IOAuthConsumer as returned by
            # IOAuthConsumerSet.getByKey) to be used in the requests we make.
            self.consumer = OAuthConsumer(oauth_consumer_key, '')
        self.access_token = OAuthToken(oauth_access_key, '')
    else:
        assert self.consumer is not None, (
            "Need a registered consumer key for authenticated requests.")
        self.access_token = self.consumer.getAccessToken(
            oauth_access_key)

It's untested but I think it's equivalent to what you have.

> logout()
> else:
> self.consumer = None
> @@ -660,7 +672,7 @@
> test.globs['user_webservice'] = LaunchpadWebServiceCaller(
> 'launchpad-library', 'nopriv-read-nonprivate')
> test.globs['anon_webservice'] = LaunchpadWebServiceCaller(
> - 'anonymous-access', '')
> + 'launchpad-library', '')
> test.globs['setupBrowser'] = setupBrowser
> test.globs['browser'] = setupBrowser()
> test.globs['a...

Read more...

Revision history for this message
Guilherme Salgado (salgado) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/account.txt'
2--- lib/canonical/launchpad/doc/account.txt 2009-07-16 19:12:39 +0000
3+++ lib/canonical/launchpad/doc/account.txt 2010-01-04 16:07:15 +0000
4@@ -86,10 +86,13 @@
5
6 = The Account object =
7
8-The account implements the IAccount interface.
9+The account implements the IAccount interface but not all attributes are
10+accessible for the owner.
11
12+ >>> login('foo.bar@canonical.com')
13 >>> verifyObject(IAccount, account)
14 True
15+ >>> login('no-priv@canonical.com')
16
17 An account has a displayname, and a preferred email address.
18
19@@ -164,11 +167,12 @@
20 u'No Privileges Person'
21
22 When the status is changed, the date_status_set is updated in the
23-database.
24+database. Only an admin can change the status.
25
26 >>> from canonical.launchpad.interfaces.account import AccountStatus
27
28 >>> original_date_status_set = account.date_status_set
29+ >>> login('foo.bar@canonical.com')
30 >>> account.status = AccountStatus.SUSPENDED
31
32 # Shouldn't be necessary with Storm!
33@@ -178,6 +182,7 @@
34 True
35
36 >>> account.status = AccountStatus.ACTIVE
37+ >>> login('no-priv@canonical.com')
38
39 The Account's displayname is synced to the Person's displayname if there
40 is one. If the Person.displayname is changed, the Account.displayname is
41
42=== modified file 'lib/canonical/launchpad/interfaces/account.py'
43--- lib/canonical/launchpad/interfaces/account.py 2009-08-14 12:50:43 +0000
44+++ lib/canonical/launchpad/interfaces/account.py 2010-01-04 16:07:15 +0000
45@@ -254,14 +254,6 @@
46 title=_("Rationale for this account's creation."), required=True,
47 readonly=True, values=AccountCreationRationale.items)
48
49- date_status_set = Datetime(
50- title=_('Date status last modified.'),
51- required=True, readonly=False)
52-
53- status_comment = Text(
54- title=_("Why are you deactivating your account?"),
55- required=False, readonly=False)
56-
57 openid_identifier = TextLine(
58 title=_("Key used to generate opaque OpenID identities."),
59 readonly=True, required=True)
60@@ -283,6 +275,14 @@
61 class IAccountSpecialRestricted(Interface):
62 """Attributes of `IAccount` protected with launchpad.Special."""
63
64+ date_status_set = Datetime(
65+ title=_('Date status last modified.'),
66+ required=True, readonly=False)
67+
68+ status_comment = Text(
69+ title=_("Why are you deactivating your account?"),
70+ required=False, readonly=False)
71+
72 # XXX sinzui 2008-07-14 bug=248518:
73 # This method would assert the password is not None, but
74 # setPreferredEmail() passes the Person's current password, which may
75
76=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-service.txt'
77--- lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2009-08-20 04:46:48 +0000
78+++ lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2010-01-04 16:07:15 +0000
79@@ -56,6 +56,77 @@
80 HTTP/1.1 404 Not Found
81 ...
82
83+== Anonymous requests ==
84+
85+A properly signed web service request whose OAuth token key is empty
86+is treated as an anonymous request.
87+
88+ >>> root = 'http://api.launchpad.dev/beta'
89+ >>> body = anon_webservice.get(root).jsonBody()
90+ >>> print body['projects_collection_link']
91+ http://api.launchpad.dev/beta/projects
92+ >>> print body['me_link']
93+ http://api.launchpad.dev/beta/people/+me
94+
95+Normally, Launchpad will reject any call made with an unrecognized
96+consumer key, because access tokens are registered with specific
97+consumer keys.
98+
99+ >>> from canonical.launchpad.testing.pages import (
100+ ... LaunchpadWebServiceCaller)
101+ >>> from canonical.launchpad.interfaces import IOAuthConsumerSet
102+
103+ >>> caller = LaunchpadWebServiceCaller('new-consumer', 'access-key')
104+ >>> response = caller.get(root)
105+ >>> print response.getheader('status')
106+ 401 Unauthorized
107+ >>> print response.body
108+ Unknown consumer (new-consumer).
109+ ...
110+
111+But with anonymous access there is no registration step. The first
112+time Launchpad sees a consumer key might be during an
113+anonymous request, and it can't reject that request just because it
114+doesn't recognize the client.
115+
116+ >>> login(ANONYMOUS)
117+ >>> consumer_set = getUtility(IOAuthConsumerSet)
118+ >>> print consumer_set.getByKey('another-new-consumer')
119+ None
120+ >>> logout()
121+
122+ >>> caller = LaunchpadWebServiceCaller('another-new-consumer', '')
123+ >>> response = caller.get(root)
124+ >>> print response.getheader('status')
125+ 200 Ok
126+
127+Launchpad automatically adds new consumer keys it sees to its database.
128+
129+ >>> login(ANONYMOUS)
130+ >>> print consumer_set.getByKey('another-new-consumer').key
131+ another-new-consumer
132+ >>> logout()
133+
134+Anonymous requests can't access certain data.
135+
136+ >>> response = anon_webservice.get(body['me_link'])
137+ >>> print response.getheader('status')
138+ 401 Unauthorized
139+ >>> print response.body
140+ You need to be logged in to view this URL.
141+ ...
142+
143+Anonymous requests can't change the dataset.
144+
145+ >>> import simplejson
146+ >>> data = simplejson.dumps({'display_name' : "This won't work"})
147+ >>> response = anon_webservice.patch(root + "/~salgado",
148+ ... 'application/json', data)
149+ >>> print response.getheader('status')
150+ 401 Unauthorized
151+ >>> print response.body
152+ (<Person at...>, 'displayname', 'launchpad.Edit')
153+ ...
154
155 == API Requests to other hosts ==
156
157@@ -108,3 +179,4 @@
158 ... headers={'Authorization': sample_auth})
159 HTTP/1.1 401 Unauthorized
160 ...
161+ Request is missing an OAuth consumer key.
162
163=== modified file 'lib/canonical/launchpad/security.py'
164--- lib/canonical/launchpad/security.py 2009-12-24 13:33:41 +0000
165+++ lib/canonical/launchpad/security.py 2010-01-04 16:07:15 +0000
166@@ -49,12 +49,12 @@
167 from lp.registry.interfaces.distroseries import IDistroSeries
168 from lp.translations.interfaces.distroserieslanguage import (
169 IDistroSeriesLanguage)
170-from lp.translations.utilities.permission_helpers import (
171- is_admin_or_rosetta_expert)
172 from lp.registry.interfaces.entitlement import IEntitlement
173 from canonical.launchpad.interfaces.hwdb import (
174 IHWDBApplication, IHWDevice, IHWDeviceClass, IHWDriver, IHWDriverName,
175 IHWDriverPackageName, IHWSubmission, IHWSubmissionDevice, IHWVendorID)
176+from lp.services.permission_helpers import (
177+ is_admin, is_admin_or_registry_expert, is_admin_or_rosetta_expert)
178 from lp.services.worlddata.interfaces.language import ILanguage, ILanguageSet
179 from lp.translations.interfaces.languagepack import ILanguagePack
180 from canonical.launchpad.interfaces.launchpad import (
181@@ -195,9 +195,7 @@
182 usedfor = None
183
184 def checkAuthenticated(self, user):
185- celebrities = getUtility(ILaunchpadCelebrities)
186- return (user.inTeam(celebrities.registry_experts)
187- or user.inTeam(celebrities.admin))
188+ return is_admin_or_registry_expert(user)
189
190
191 class ReviewProduct(ReviewByRegistryExpertsOrAdmins):
192@@ -216,6 +214,11 @@
193 usedfor = IProjectSet
194
195
196+class ModeratePerson(ReviewByRegistryExpertsOrAdmins):
197+ permission = 'launchpad.Moderate'
198+ usedfor = IPerson
199+
200+
201 class ViewPillar(AuthorizationBase):
202 usedfor = IPillar
203 permission = 'launchpad.View'
204@@ -230,26 +233,43 @@
205 else:
206 celebrities = getUtility(ILaunchpadCelebrities)
207 return (user.inTeam(celebrities.commercial_admin)
208- or user.inTeam(celebrities.registry_experts)
209- or user.inTeam(celebrities.admin))
210-
211-
212-class EditAccount(AuthorizationBase):
213+ or is_admin_or_registry_expert(user))
214+
215+
216+class EditAccountBySelfOrAdmin(AuthorizationBase):
217 permission = 'launchpad.Edit'
218 usedfor = IAccount
219
220 def checkAccountAuthenticated(self, account):
221 if account == self.obj:
222 return True
223- user = IPerson(account, None)
224- return (user is not None and
225- user.inTeam(getUtility(ILaunchpadCelebrities).admin))
226-
227-
228-class ViewAccount(EditAccount):
229+ return super(
230+ EditAccountBySelfOrAdmin, self).checkAccountAuthenticated(account)
231+
232+ def checkAuthenticated(self, user):
233+ return is_admin(user)
234+
235+
236+class ViewAccount(EditAccountBySelfOrAdmin):
237 permission = 'launchpad.View'
238
239
240+class SpecialAccount(EditAccountBySelfOrAdmin):
241+ permission = 'launchpad.Special'
242+
243+ def checkAuthenticated(self, user):
244+ """Extend permission to registry experts."""
245+ return is_admin_or_registry_expert(user)
246+
247+
248+class ModerateAccountByRegistryExpert(AuthorizationBase):
249+ usedfor = IAccount
250+ permission = 'launchpad.Moderate'
251+
252+ def checkAuthenticated(self, user):
253+ return is_admin_or_registry_expert(user)
254+
255+
256 class EditOAuthAccessToken(AuthorizationBase):
257 permission = 'launchpad.Edit'
258 usedfor = IOAuthAccessToken
259@@ -647,15 +667,6 @@
260 return self.obj.id == user.id
261
262
263-class EditAccountBySelf(AuthorizationBase):
264- permission = 'launchpad.Special'
265- usedfor = IAccount
266-
267- def checkAccountAuthenticated(self, account):
268- """A user can edit the Account who is herself."""
269- return self.obj == account
270-
271-
272 class ViewPublicOrPrivateTeamMembers(AuthorizationBase):
273 """Restrict viewing of private memberships of teams.
274
275
276=== modified file 'lib/canonical/launchpad/testing/pages.py'
277--- lib/canonical/launchpad/testing/pages.py 2009-12-21 17:18:12 +0000
278+++ lib/canonical/launchpad/testing/pages.py 2010-01-04 16:07:15 +0000
279@@ -20,7 +20,8 @@
280 from BeautifulSoup import (
281 BeautifulSoup, CData, Comment, Declaration, NavigableString, PageElement,
282 ProcessingInstruction, SoupStrainer, Tag)
283-from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT
284+from contrib.oauth import (
285+ OAuthConsumer, OAuthRequest, OAuthSignatureMethod_PLAINTEXT, OAuthToken)
286 from urlparse import urljoin
287
288 from zope.app.testing.functional import HTTPCaller, SimpleCookie
289@@ -97,10 +98,33 @@
290 """
291 if oauth_consumer_key is not None and oauth_access_key is not None:
292 login(ANONYMOUS)
293- self.consumer = getUtility(IOAuthConsumerSet).getByKey(
294- oauth_consumer_key)
295- self.access_token = self.consumer.getAccessToken(
296- oauth_access_key)
297+ consumers = getUtility(IOAuthConsumerSet)
298+ self.consumer = consumers.getByKey(oauth_consumer_key)
299+ if oauth_access_key == '':
300+ # The client wants to make an anonymous request.
301+ self.access_token = OAuthToken(oauth_access_key, '')
302+ if self.consumer is None:
303+ # The client is trying to make an anonymous
304+ # request with a previously unknown consumer. This
305+ # is fine: we manually create a "fake"
306+ # OAuthConsumer (it's "fake" because it's not
307+ # really an IOAuthConsumer as returned by
308+ # IOAuthConsumerSet.getByKey) to be used in the
309+ # requests we make.
310+ self.consumer = OAuthConsumer(oauth_consumer_key, '')
311+ else:
312+ if self.consumer is None:
313+ # Requests using this caller will be rejected by
314+ # the server, but we have a test that verifies
315+ # such requests _are_ rejected, so we'll create a
316+ # fake OAuthConsumer object.
317+ self.consumer = OAuthConsumer(oauth_consumer_key, '')
318+ self.access_token = OAuthToken(oauth_access_key, '')
319+ else:
320+ # The client wants to make an authorized request
321+ # using a recognized consumer key.
322+ self.access_token = self.consumer.getAccessToken(
323+ oauth_access_key)
324 logout()
325 else:
326 self.consumer = None
327@@ -676,6 +700,8 @@
328 'foobar123451432', 'salgado-read-nonprivate')
329 test.globs['user_webservice'] = LaunchpadWebServiceCaller(
330 'launchpad-library', 'nopriv-read-nonprivate')
331+ test.globs['anon_webservice'] = LaunchpadWebServiceCaller(
332+ 'launchpad-library', '')
333 test.globs['setupBrowser'] = setupBrowser
334 test.globs['setupDTCBrowser'] = setupDTCBrowser
335 test.globs['browser'] = setupBrowser()
336
337=== modified file 'lib/canonical/launchpad/webapp/servers.py'
338--- lib/canonical/launchpad/webapp/servers.py 2009-10-22 13:16:06 +0000
339+++ lib/canonical/launchpad/webapp/servers.py 2010-01-04 16:07:15 +0000
340@@ -1177,10 +1177,42 @@
341 form = get_oauth_authorization(request)
342
343 consumer_key = form.get('oauth_consumer_key')
344- consumer = getUtility(IOAuthConsumerSet).getByKey(consumer_key)
345+ consumers = getUtility(IOAuthConsumerSet)
346+ consumer = consumers.getByKey(consumer_key)
347+ token_key = form.get('oauth_token')
348+ anonymous_request = (token_key == '')
349 if consumer is None:
350- raise Unauthorized('Unknown consumer (%s).' % consumer_key)
351- token_key = form.get('oauth_token')
352+ if consumer_key is None:
353+ # Most likely the user is trying to make a totally
354+ # unauthenticated request.
355+ raise Unauthorized(
356+ 'Request is missing an OAuth consumer key.')
357+ if anonymous_request:
358+ # This is the first time anyone has tried to make an
359+ # anonymous request using this consumer
360+ # name. Dynamically create the consumer.
361+ #
362+ # In the normal website this wouldn't be possible
363+ # because GET requests have their transactions rolled
364+ # back. But webservice requests always have their
365+ # transactions committed so that we can keep track of
366+ # the OAuth nonces and prevent replay attacks.
367+ consumer = consumers.new(consumer_key, '')
368+ else:
369+ # An unknown consumer can never make a non-anonymous
370+ # request, because access tokens are registered with a
371+ # specific, known consumer.
372+ raise Unauthorized('Unknown consumer (%s).' % consumer_key)
373+ if anonymous_request:
374+ # Skip the OAuth verification step and let the user access the
375+ # web service as an unauthenticated user.
376+ #
377+ # XXX leonardr 2009-12-15 bug=496964: Ideally we'd be
378+ # auto-creating a token for the anonymous user the first
379+ # time, passing it through the OAuth verification step,
380+ # and using it on all subsequent anonymous requests.
381+ auth_utility = getUtility(IPlacelessAuthUtility)
382+ return auth_utility.unauthenticatedPrincipal()
383 token = consumer.getAccessToken(token_key)
384 if token is None:
385 raise Unauthorized('Unknown access token (%s).' % token_key)
386
387=== modified file 'lib/canonical/launchpad/zcml/account.zcml'
388--- lib/canonical/launchpad/zcml/account.zcml 2009-07-13 18:15:02 +0000
389+++ lib/canonical/launchpad/zcml/account.zcml 2010-01-04 16:07:15 +0000
390@@ -15,8 +15,11 @@
391 permission="launchpad.Special"
392 interface="canonical.launchpad.interfaces.IAccountSpecialRestricted" />
393 <require
394+ permission="launchpad.Moderate"
395+ set_attributes="status date_status_set status_comment" />
396+ <require
397 permission="launchpad.Edit"
398- set_schema="canonical.launchpad.interfaces.IAccount" />
399+ set_attributes="displayname password" />
400 </class>
401
402 <securedutility
403
404=== modified file 'lib/lp/bugs/scripts/checkwatches.py'
405--- lib/lp/bugs/scripts/checkwatches.py 2009-12-15 16:28:20 +0000
406+++ lib/lp/bugs/scripts/checkwatches.py 2010-01-04 16:07:15 +0000
407@@ -789,7 +789,16 @@
408 )
409
410 for bug_id in all_remote_ids:
411- bug_watches = bug_watches_by_remote_bug[bug_id]
412+ try:
413+ bug_watches = bug_watches_by_remote_bug[bug_id]
414+ except KeyError:
415+ # If there aren't any bug watches for this remote bug,
416+ # just log a warning and carry on.
417+ self.warning(
418+ "Spurious remote bug ID: No watches found for "
419+ "remote bug %s on %s" % (bug_id, remotesystem.baseurl))
420+ continue
421+
422 for bug_watch in bug_watches:
423 bug_watch.lastchecked = UTC_NOW
424 if bug_id in unmodified_remote_ids:
425
426=== modified file 'lib/lp/bugs/scripts/tests/test_checkwatches.py'
427--- lib/lp/bugs/scripts/tests/test_checkwatches.py 2009-12-21 16:08:35 +0000
428+++ lib/lp/bugs/scripts/tests/test_checkwatches.py 2010-01-04 16:07:15 +0000
429@@ -15,6 +15,8 @@
430
431 from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI
432 from lp.bugs.scripts import checkwatches
433+from lp.bugs.scripts.checkwatches import CheckWatchesErrorUtility
434+from lp.bugs.tests.externalbugtracker import TestBugzillaAPIXMLRPCTransport
435 from lp.testing import TestCaseWithFactory
436
437
438@@ -23,6 +25,52 @@
439 return BugzillaAPI(bugtracker.baseurl)
440
441
442+class NonConnectingBugzillaAPI(BugzillaAPI):
443+ """A non-connected version of the BugzillaAPI ExternalBugTracker."""
444+
445+ bugs = {
446+ 1: {'product': 'test-product'},
447+ }
448+
449+ def getCurrentDBTime(self):
450+ return None
451+
452+ def getExternalBugTrackerToUse(self):
453+ return self
454+
455+ def getProductsForRemoteBugs(self, remote_bugs):
456+ """Return the products for some remote bugs.
457+
458+ This method is basically the same as that of the superclass but
459+ without the call to initializeRemoteBugDB().
460+ """
461+ bug_products = {}
462+ for bug_id in bug_ids:
463+ # If one of the bugs we're trying to get the product for
464+ # doesn't exist, just skip it.
465+ try:
466+ actual_bug_id = self._getActualBugId(bug_id)
467+ except BugNotFound:
468+ continue
469+
470+ bug_dict = self._bugs[actual_bug_id]
471+ bug_products[bug_id] = bug_dict['product']
472+
473+ return bug_products
474+
475+
476+class NoBugWatchesByRemoteBugUpdater(checkwatches.BugWatchUpdater):
477+ """A subclass of BugWatchUpdater with methods overridden for testing."""
478+
479+ def _getBugWatchesByRemoteBug(self, bug_watch_ids):
480+ """Return an empty dict.
481+
482+ This method overrides _getBugWatchesByRemoteBug() so that bug
483+ 497141 can be regression-tested.
484+ """
485+ return {}
486+
487+
488 class TestCheckwatchesWithSyncableGnomeProducts(TestCaseWithFactory):
489
490 layer = LaunchpadZopelessLayer
491@@ -63,5 +111,40 @@
492 gnome_bugzilla, [bug_watch_1, bug_watch_2])
493
494
495+class TestBugWatchUpdater(TestCaseWithFactory):
496+
497+ layer = LaunchpadZopelessLayer
498+
499+ def test_bug_497141(self):
500+ # Regression test for bug 497141. KeyErrors raised in
501+ # BugWatchUpdater.updateBugWatches() shouldn't cause
502+ # checkwatches to abort.
503+ updater = NoBugWatchesByRemoteBugUpdater(
504+ transaction, QuietFakeLogger())
505+
506+ # Create a couple of bug watches for testing purposes.
507+ bug_tracker = self.factory.makeBugTracker()
508+ bug_watches = [
509+ self.factory.makeBugWatch(bugtracker=bug_tracker)
510+ for i in range(2)]
511+
512+ # Use a test XML-RPC transport to ensure no connections happen.
513+ test_transport = TestBugzillaAPIXMLRPCTransport(bug_tracker.baseurl)
514+ remote_system = NonConnectingBugzillaAPI(
515+ bug_tracker.baseurl, xmlrpc_transport=test_transport)
516+
517+ # Calling updateBugWatches() shouldn't raise a KeyError, even
518+ # though with our broken updater _getExternalBugTrackersAndWatches()
519+ # will return an empty dict.
520+ updater.updateBugWatches(remote_system, bug_watches)
521+
522+ # An error will have been logged instead of the KeyError being
523+ # raised.
524+ error_utility = CheckWatchesErrorUtility()
525+ last_oops = error_utility.getLastOopsReport()
526+ self.assertTrue(
527+ last_oops.value.startswith('Spurious remote bug ID'))
528+
529+
530 def test_suite():
531 return unittest.TestLoader().loadTestsFromName(__name__)
532
533=== modified file 'lib/lp/registry/browser/configure.zcml'
534--- lib/lp/registry/browser/configure.zcml 2009-12-05 18:37:28 +0000
535+++ lib/lp/registry/browser/configure.zcml 2010-01-04 16:07:15 +0000
536@@ -761,7 +761,7 @@
537 name="+reviewaccount"
538 for="lp.registry.interfaces.person.IPerson"
539 class="lp.registry.browser.person.PersonAccountAdministerView"
540- permission="launchpad.Admin"
541+ permission="launchpad.Moderate"
542 template="../templates/person-review.pt"/>
543 <browser:page
544 name="+claimteam"
545
546=== modified file 'lib/lp/registry/browser/person.py'
547--- lib/lp/registry/browser/person.py 2009-12-23 16:17:12 +0000
548+++ lib/lp/registry/browser/person.py 2010-01-04 16:07:15 +0000
549@@ -901,6 +901,12 @@
550 text = 'Administer'
551 return Link(target, text, icon='edit')
552
553+ @enabled_with_permission('launchpad.Moderate')
554+ def administer_account(self):
555+ target = '+reviewaccount'
556+ text = 'Administer Account'
557+ return Link(target, text, icon='edit')
558+
559
560 class PersonOverviewMenu(ApplicationMenu, PersonMenuMixin):
561
562@@ -910,9 +916,10 @@
563 'editemailaddresses', 'editlanguages', 'editwikinames',
564 'editircnicknames', 'editjabberids', 'editpassword',
565 'editsshkeys', 'editpgpkeys', 'editlocation', 'memberships',
566- 'codesofconduct', 'karma', 'administer', 'projects',
567- 'activate_ppa', 'maintained', 'view_ppa_subscriptions',
568- 'ppa', 'oauth_tokens', 'related_software_summary']
569+ 'codesofconduct', 'karma', 'administer', 'administer_account',
570+ 'projects', 'activate_ppa', 'maintained',
571+ 'view_ppa_subscriptions', 'ppa', 'oauth_tokens',
572+ 'related_software_summary']
573
574 def related_software_summary(self):
575 target = '+related-software'
576@@ -1687,8 +1694,6 @@
577 """Administer an `IAccount` belonging to an `IPerson`."""
578 schema = IAccount
579 label = "Review person's account"
580- field_names = [
581- 'displayname', 'password', 'status', 'status_comment']
582 custom_widget(
583 'status_comment', TextAreaWidget, height=5, width=60)
584 custom_widget('password', PasswordChangeWidget)
585@@ -1697,9 +1702,14 @@
586 """See `LaunchpadEditFormView`."""
587 super(PersonAccountAdministerView, self).__init__(context, request)
588 # Only the IPerson can be traversed to, so it provides the IAccount.
589+ # It also means that permissions are checked on IAccount, not IPerson.
590 self.person = self.context
591 from canonical.launchpad.interfaces import IMasterObject
592 self.context = IMasterObject(self.context.account)
593+ # Set fields to be displayed.
594+ self.field_names = ['status', 'status_comment']
595+ if self.viewed_by_admin:
596+ self.field_names = ['displayname', 'password'] + self.field_names
597
598 @property
599 def is_viewing_person(self):
600@@ -1711,6 +1721,11 @@
601 return False
602
603 @property
604+ def viewed_by_admin(self):
605+ """Is the user a Launchpad admin?"""
606+ return check_permission('launchpad.Admin', self.context)
607+
608+ @property
609 def email_addresses(self):
610 """A list of the user's preferred and validated email addresses."""
611 emails = sorted(
612@@ -1722,6 +1737,10 @@
613 @property
614 def next_url(self):
615 """See `LaunchpadEditFormView`."""
616+ is_suspended = self.context.status == AccountStatus.SUSPENDED
617+ if is_suspended and not self.viewed_by_admin:
618+ # Non-admins cannot see suspended persons.
619+ return canonical_url(getUtility(IPersonSet))
620 return canonical_url(self.person)
621
622 @property
623@@ -1739,6 +1758,9 @@
624 # email is sent to the user.
625 data['password'] = 'invalid'
626 self.person.setPreferredEmail(None)
627+ self.request.response.addNoticeNotification(
628+ u'The account "%s" has been suspended.' % (
629+ self.context.displayname))
630 if (data['status'] == AccountStatus.ACTIVE
631 and self.context.status != AccountStatus.ACTIVE):
632 self.request.response.addNoticeNotification(
633@@ -5834,7 +5856,8 @@
634 usedfor = IPersonIndexMenu
635 facet = 'overview'
636 title = 'Change person'
637- links = ('edit', 'administer', 'branding', 'editpassword')
638+ links = ('edit', 'administer', 'administer_account',
639+ 'branding', 'editpassword')
640
641
642 class ITeamIndexMenu(Interface):
643
644=== modified file 'lib/lp/registry/doc/person-account.txt'
645--- lib/lp/registry/doc/person-account.txt 2009-08-13 19:03:36 +0000
646+++ lib/lp/registry/doc/person-account.txt 2010-01-04 16:07:15 +0000
647@@ -26,9 +26,9 @@
648 None
649
650 The account can only be activated by the user who is claiming
651-the profile. Mark cannot claim it.
652+the profile. Sample Person cannot claim it.
653
654- >>> login('mark@example.com')
655+ >>> login('test@canonical.com')
656 >>> matsubara.account.activate(
657 ... comment="test", password='ok', preferred_email=emailaddress)
658 Traceback (most recent call last):
659
660=== modified file 'lib/lp/registry/stories/person/xx-admin-person-review.txt'
661--- lib/lp/registry/stories/person/xx-admin-person-review.txt 2009-12-23 16:17:12 +0000
662+++ lib/lp/registry/stories/person/xx-admin-person-review.txt 2010-01-04 16:07:15 +0000
663@@ -51,7 +51,8 @@
664 >>> print link.text
665 edit[IMG] Review the user's Launchpad information
666
667-The admin can update the user's account.
668+The admin can update the user's account. Suspending an account will give a
669+feedback message.
670
671 >>> admin_browser.getControl(
672 ... 'The status of this account').value = ['SUSPENDED']
673@@ -61,6 +62,8 @@
674
675 >>> print admin_browser.title
676 The one and only Salgado does not use Launchpad
677+ >>> print get_feedback_messages(admin_browser.contents)[0]
678+ The account "The one and only Salgado" has been suspended.
679
680 The admin can see the account information of a user that does not use
681 Launchpad, and can change the account too. Note that all pages that belong
682@@ -88,6 +91,57 @@
683 The user is reactivated. He must use the "forgot password" to log in.
684
685
686+Partial access for registry experts
687+-----------------------------------
688+
689+Members of the registry team get partial access to the review account page to
690+be able to suspend spam accounts.
691+
692+ >>> login('foo.bar@canonical.com')
693+ >>> registry_expert = factory.makePerson(email='expert@example.com',
694+ ... password='test')
695+ >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
696+ >>> from zope.component import getUtility
697+ >>> registry_team = getUtility(ILaunchpadCelebrities).registry_experts
698+ >>> registry_team.addMember(registry_expert, registry_team.teamowner)
699+ >>> print registry_expert.inTeam(registry_team)
700+ True
701+ >>> logout()
702+
703+ >>> expert_browser = setupBrowser(auth='Basic expert@example.com:test')
704+ >>> expert_browser.open('http://launchpad.dev/~no-priv/+reviewaccount')
705+ >>> print expert_browser.title
706+ Review person's account...
707+
708+The +reviewaccount page does not display account information for registry
709+experts. It also has no form elements to change the display name or the
710+password.
711+
712+ >>> content = find_main_content(expert_browser.contents)
713+ >>> print content.find(id='summary')
714+ None
715+ >>> print content.find(name='field.displayname')
716+ None
717+ >>> print content.find(name='field.password')
718+ None
719+
720+The only option for registry experts is to change an account's status and to
721+provide a reason why they did it. After suspending the account the
722+page will only be visible to admins, so the registry expert will be
723+redirected to the "people" main page. A feedback message informs the expert
724+about the suspension.
725+
726+ >>> control = expert_browser.getControl(name='field.status_comment')
727+ >>> control.value = 'This is SPAM!'
728+ >>> expert_browser.getControl(
729+ ... 'The status of this account').value = ['SUSPENDED']
730+ >>> expert_browser.getControl('Change').click()
731+ >>> print expert_browser.url
732+ http://launchpad.dev/people
733+ >>> print get_feedback_messages(expert_browser.contents)[0]
734+ The account "No Privileges Person" has been suspended.
735+
736+
737 Restricted to admins
738 --------------------
739
740
741=== modified file 'lib/lp/registry/templates/person-review.pt'
742--- lib/lp/registry/templates/person-review.pt 2009-09-15 15:42:39 +0000
743+++ lib/lp/registry/templates/person-review.pt 2010-01-04 16:07:15 +0000
744@@ -33,7 +33,8 @@
745 </p>
746 </tal:review-person>
747
748- <tal:review-account tal:condition="not: view/is_viewing_person">
749+ <tal:review-account
750+ condition="python: not view.is_viewing_person and view.viewed_by_admin">
751 <p>
752 The account displayname is not always the same as the Launchpad
753 displayname.
754
755=== renamed file 'lib/lp/translations/utilities/permission_helpers.py' => 'lib/lp/services/permission_helpers.py'
756--- lib/lp/translations/utilities/permission_helpers.py 2009-11-17 09:50:33 +0000
757+++ lib/lp/services/permission_helpers.py 2010-01-04 16:07:15 +0000
758@@ -6,7 +6,11 @@
759 __metaclass__ = type
760
761 __all__ = [
762+ 'is_admin',
763+ 'is_admin_or_registry_expert',
764 'is_admin_or_rosetta_expert',
765+ 'is_registry_expert',
766+ 'is_rosetta_expert',
767 ]
768
769 from zope.component import getUtility
770@@ -14,8 +18,29 @@
771 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
772
773
774+def is_admin(user):
775+ """Check if the user is a Launchpad admin."""
776+ celebrities = getUtility(ILaunchpadCelebrities)
777+ return user.inTeam(celebrities.admin)
778+
779+
780+def is_rosetta_expert(user):
781+ """Check if the user is a Rosetta expert."""
782+ celebrities = getUtility(ILaunchpadCelebrities)
783+ return user.inTeam(celebrities.rosetta_experts)
784+
785+
786+def is_registry_expert(user):
787+ """Check if the user is a registry expert."""
788+ celebrities = getUtility(ILaunchpadCelebrities)
789+ return user.inTeam(celebrities.registry_experts)
790+
791+
792 def is_admin_or_rosetta_expert(user):
793- """Check if the user is a Launchpad admin or a Rosettta expert."""
794- celebrities = getUtility(ILaunchpadCelebrities)
795- return (user.inTeam(celebrities.admin) or
796- user.inTeam(celebrities.rosetta_experts))
797+ """Check if the user is a Launchpad admin or a Rosetta expert."""
798+ return is_admin(user) or is_rosetta_expert(user)
799+
800+
801+def is_admin_or_registry_expert(user):
802+ """Check if the user is a Launchpad admin or a registry expert."""
803+ return is_admin(user) or is_registry_expert(user)
804
805=== modified file 'lib/lp/translations/model/translationimportqueue.py'
806--- lib/lp/translations/model/translationimportqueue.py 2009-12-18 09:25:14 +0000
807+++ lib/lp/translations/model/translationimportqueue.py 2010-01-04 16:07:15 +0000
808@@ -43,6 +43,7 @@
809 from lp.registry.interfaces.product import IProduct
810 from lp.registry.interfaces.productseries import IProductSeries
811 from lp.registry.interfaces.sourcepackage import ISourcePackage
812+from lp.services.permission_helpers import is_admin_or_rosetta_expert
813 from lp.services.worlddata.interfaces.language import ILanguageSet
814 from lp.translations.interfaces.pofile import IPOFileSet
815 from lp.translations.interfaces.potemplate import IPOTemplateSet
816@@ -62,8 +63,6 @@
817 from lp.translations.interfaces.translations import TranslationConstants
818 from lp.translations.utilities.gettext_po_importer import (
819 GettextPOImporter)
820-from lp.translations.utilities.permission_helpers import (
821- is_admin_or_rosetta_expert)
822 from canonical.librarian.interfaces import ILibrarianClient
823
824
825
826=== modified file 'versions.cfg'
827--- versions.cfg 2009-12-22 23:35:26 +0000
828+++ versions.cfg 2010-01-04 16:07:15 +0000
829@@ -19,7 +19,7 @@
830 grokcore.component = 1.6
831 httplib2 = 0.4.0
832 ipython = 0.9.1
833-launchpadlib = 1.5.3.1
834+launchpadlib = 1.5.4
835 lazr.authentication = 0.1.1
836 lazr.batchnavigator = 1.1
837 lazr.config = 1.1.3

Subscribers

People subscribed via source and target branches

to status/vote changes: