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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

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.

[Guilherme Salgado (salgado)]

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

review: Needs Fixing

[Leonard Richardson (leonardr)]

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

[Guilherme Salgado (salgado)]

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

[Guilherme Salgado (salgado)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/doc/account.txt'
--- lib/canonical/launchpad/doc/account.txt	2009-07-16 19:12:39 +0000
+++ lib/canonical/launchpad/doc/account.txt	2010-01-04 16:07:15 +0000
@@ -86,10 +86,13 @@
 
 = The Account object =
 
-The account implements the IAccount interface.
+The account implements the IAccount interface but not all attributes are
+accessible for the owner.
 
+    >>> login('[e-mail]')
     >>> verifyObject(IAccount, account)
     True
+    >>> login('[e-mail]')
 
 An account has a displayname, and a preferred email address.
 
@@ -164,11 +167,12 @@
     u'No Privileges Person'
 
 When the status is changed, the date_status_set is updated in the
-database.
+database. Only an admin can change the status.
 
     >>> from canonical.launchpad.interfaces.account import AccountStatus
 
     >>> original_date_status_set = account.date_status_set
+    >>> login('[e-mail]')
     >>> account.status = AccountStatus.SUSPENDED
 
     # Shouldn't be necessary with Storm!
@@ -178,6 +182,7 @@
     True
 
     >>> account.status = AccountStatus.ACTIVE
+    >>> login('[e-mail]')
 
 The Account's displayname is synced to the Person's displayname if there
 is one. If the Person.displayname is changed, the Account.displayname is

=== modified file 'lib/canonical/launchpad/interfaces/account.py'
--- lib/canonical/launchpad/interfaces/account.py	2009-08-14 12:50:43 +0000
+++ lib/canonical/launchpad/interfaces/account.py	2010-01-04 16:07:15 +0000
@@ -254,14 +254,6 @@
         title=_("Rationale for this account's creation."), required=True,
         readonly=True, values=AccountCreationRationale.items)
 
-    date_status_set = Datetime(
-        title=_('Date status last modified.'),
-        required=True, readonly=False)
-
-    status_comment = Text(
-        title=_("Why are you deactivating your account?"),
-        required=False, readonly=False)
-
     openid_identifier = TextLine(
         title=_("Key used to generate opaque OpenID identities."),
         readonly=True, required=True)
@@ -283,6 +275,14 @@
 class IAccountSpecialRestricted(Interface):
     """Attributes of `IAccount` protected with launchpad.Special."""
 
+    date_status_set = Datetime(
+        title=_('Date status last modified.'),
+        required=True, readonly=False)
+
+    status_comment = Text(
+        title=_("Why are you deactivating your account?"),
+        required=False, readonly=False)
+
     # XXX sinzui 2008-07-14 bug=248518:
     # This method would assert the password is not None, but
     # setPreferredEmail() passes the Person's current password, which may

=== 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	2010-01-04 16:07:15 +0000
@@ -56,6 +56,77 @@
     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
+
+Normally, Launchpad will reject any call made with an unrecognized
+consumer key, because access tokens are registered with specific
+consumer keys.
+
+    >>> from canonical.launchpad.testing.pages import (
+    ...     LaunchpadWebServiceCaller)
+    >>> from canonical.launchpad.interfaces import IOAuthConsumerSet
+
+    >>> caller = LaunchpadWebServiceCaller('new-consumer', 'access-key')
+    >>> response = caller.get(root)
+    >>> print response.getheader('status')
+    401 Unauthorized
+    >>> print response.body
+    Unknown consumer (new-consumer).
+    ...
+
+But with anonymous access there is no registration step. The first
+time Launchpad sees a consumer key might be during an
+anonymous request, and it can't reject that request just because it
+doesn't recognize the client.
+
+    >>> login(ANONYMOUS)
+    >>> consumer_set = getUtility(IOAuthConsumerSet)
+    >>> print consumer_set.getByKey('another-new-consumer')
+    None
+    >>> logout()
+
+    >>> caller = LaunchpadWebServiceCaller('another-new-consumer', '')
+    >>> response = caller.get(root)
+    >>> print response.getheader('status')
+    200 Ok
+
+Launchpad automatically adds new consumer keys it sees to its database.
+
+    >>> login(ANONYMOUS)
+    >>> print consumer_set.getByKey('another-new-consumer').key
+    another-new-consumer
+    >>> logout()
+
+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
+    (<Person at...>, 'displayname', 'launchpad.Edit')
+    ...
 
 == API Requests to other hosts ==
 
@@ -108,3 +179,4 @@
     ...     headers={'Authorization': sample_auth})
     HTTP/1.1 401 Unauthorized
     ...
+    Request is missing an OAuth consumer key.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2009-12-24 13:33:41 +0000
+++ lib/canonical/launchpad/security.py	2010-01-04 16:07:15 +0000
@@ -49,12 +49,12 @@
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.translations.interfaces.distroserieslanguage import (
     IDistroSeriesLanguage)
-from lp.translations.utilities.permission_helpers import (
-    is_admin_or_rosetta_expert)
 from lp.registry.interfaces.entitlement import IEntitlement
 from canonical.launchpad.interfaces.hwdb import (
     IHWDBApplication, IHWDevice, IHWDeviceClass, IHWDriver, IHWDriverName,
     IHWDriverPackageName, IHWSubmission, IHWSubmissionDevice, IHWVendorID)
+from lp.services.permission_helpers import (
+    is_admin, is_admin_or_registry_expert, is_admin_or_rosetta_expert)
 from lp.services.worlddata.interfaces.language import ILanguage, ILanguageSet
 from lp.translations.interfaces.languagepack import ILanguagePack
 from canonical.launchpad.interfaces.launchpad import (
@@ -195,9 +195,7 @@
     usedfor = None
 
     def checkAuthenticated(self, user):
-        celebrities = getUtility(ILaunchpadCelebrities)
-        return (user.inTeam(celebrities.registry_experts)
-                or user.inTeam(celebrities.admin))
+        return is_admin_or_registry_expert(user)
 
 
 class ReviewProduct(ReviewByRegistryExpertsOrAdmins):
@@ -216,6 +214,11 @@
     usedfor = IProjectSet
 
 
+class ModeratePerson(ReviewByRegistryExpertsOrAdmins):
+    permission = 'launchpad.Moderate'
+    usedfor = IPerson
+
+
 class ViewPillar(AuthorizationBase):
     usedfor = IPillar
     permission = 'launchpad.View'
@@ -230,26 +233,43 @@
         else:
             celebrities = getUtility(ILaunchpadCelebrities)
             return (user.inTeam(celebrities.commercial_admin)
-                    or user.inTeam(celebrities.registry_experts)
-                    or user.inTeam(celebrities.admin))
-
-
-class EditAccount(AuthorizationBase):
+                    or is_admin_or_registry_expert(user))
+
+
+class EditAccountBySelfOrAdmin(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IAccount
 
     def checkAccountAuthenticated(self, account):
         if account == self.obj:
             return True
-        user = IPerson(account, None)
-        return (user is not None and
-                user.inTeam(getUtility(ILaunchpadCelebrities).admin))
-
-
-class ViewAccount(EditAccount):
+        return super(
+            EditAccountBySelfOrAdmin, self).checkAccountAuthenticated(account)
+
+    def checkAuthenticated(self, user):
+        return is_admin(user)
+
+
+class ViewAccount(EditAccountBySelfOrAdmin):
     permission = 'launchpad.View'
 
 
+class SpecialAccount(EditAccountBySelfOrAdmin):
+    permission = 'launchpad.Special'
+
+    def checkAuthenticated(self, user):
+        """Extend permission to registry experts."""
+        return is_admin_or_registry_expert(user)
+
+
+class ModerateAccountByRegistryExpert(AuthorizationBase):
+    usedfor = IAccount
+    permission = 'launchpad.Moderate'
+
+    def checkAuthenticated(self, user):
+        return is_admin_or_registry_expert(user)
+
+
 class EditOAuthAccessToken(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IOAuthAccessToken
@@ -647,15 +667,6 @@
         return self.obj.id == user.id
 
 
-class EditAccountBySelf(AuthorizationBase):
-    permission = 'launchpad.Special'
-    usedfor = IAccount
-
-    def checkAccountAuthenticated(self, account):
-        """A user can edit the Account who is herself."""
-        return self.obj == account
-
-
 class ViewPublicOrPrivateTeamMembers(AuthorizationBase):
     """Restrict viewing of private memberships of teams.
 

=== modified file 'lib/canonical/launchpad/testing/pages.py'
--- lib/canonical/launchpad/testing/pages.py	2009-12-21 17:18:12 +0000
+++ lib/canonical/launchpad/testing/pages.py	2010-01-04 16:07:15 +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
@@ -97,10 +98,33 @@
         """
         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 oauth_access_key == '':
+                # The client wants to make an anonymous request.
+                self.access_token = OAuthToken(oauth_access_key, '')
+                if self.consumer is None:
+                    # The client is trying to make an anonymous
+                    # request with a previously unknown consumer. This
+                    # is fine: we manually create a "fake"
+                    # OAuthConsumer (it's "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, '')
+            else:
+                if self.consumer is None:
+                    # Requests using this caller will be rejected by
+                    # the server, but we have a test that verifies
+                    # such requests _are_ rejected, so we'll create a
+                    # fake OAuthConsumer object.
+                    self.consumer = OAuthConsumer(oauth_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)
             logout()
         else:
             self.consumer = None
@@ -676,6 +700,8 @@
         'foobar123451432', 'salgado-read-nonprivate')
     test.globs['user_webservice'] = LaunchpadWebServiceCaller(
         'launchpad-library', 'nopriv-read-nonprivate')
+    test.globs['anon_webservice'] = LaunchpadWebServiceCaller(
+        'launchpad-library', '')
     test.globs['setupBrowser'] = setupBrowser
     test.globs['setupDTCBrowser'] = setupDTCBrowser
     test.globs['browser'] = setupBrowser()

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2009-10-22 13:16:06 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2010-01-04 16:07:15 +0000
@@ -1177,10 +1177,42 @@
         form = get_oauth_authorization(request)
 
         consumer_key = form.get('oauth_consumer_key')
-        consumer = getUtility(IOAuthConsumerSet).getByKey(consumer_key)
+        consumers = getUtility(IOAuthConsumerSet)
+        consumer = consumers.getByKey(consumer_key)
+        token_key = form.get('oauth_token')
+        anonymous_request = (token_key == '')
         if consumer is None:
-            raise Unauthorized('Unknown consumer (%s).' % consumer_key)
-        token_key = form.get('oauth_token')
+            if consumer_key is None:
+                # Most likely the user is trying to make a totally
+                # unauthenticated request.
+                raise Unauthorized(
+                    'Request is missing an OAuth consumer key.')
+            if anonymous_request:
+                # This is the first time anyone has tried to make an
+                # anonymous request using this consumer
+                # name. Dynamically create the consumer.
+                #
+                # In the normal website this wouldn't be possible
+                # because GET requests have their transactions rolled
+                # back. But webservice requests always have their
+                # transactions committed so that we can keep track of
+                # the OAuth nonces and prevent replay attacks.
+                consumer = consumers.new(consumer_key, '')
+            else:
+                # An unknown consumer can never make a non-anonymous
+                # request, because access tokens are registered with a
+                # specific, known consumer.
+                raise Unauthorized('Unknown consumer (%s).' % consumer_key)
+        if anonymous_request:
+            # Skip the OAuth verification step and let the user access the
+            # web service as an unauthenticated user.
+            #
+            # XXX leonardr 2009-12-15 bug=496964: Ideally we'd be
+            # auto-creating a token for the anonymous user the first
+            # time, passing it through the OAuth verification step,
+            # and using it on all subsequent anonymous requests.
+            auth_utility = getUtility(IPlacelessAuthUtility)
+            return auth_utility.unauthenticatedPrincipal()
         token = consumer.getAccessToken(token_key)
         if token is None:
             raise Unauthorized('Unknown access token (%s).' % token_key)

=== modified file 'lib/canonical/launchpad/zcml/account.zcml'
--- lib/canonical/launchpad/zcml/account.zcml	2009-07-13 18:15:02 +0000
+++ lib/canonical/launchpad/zcml/account.zcml	2010-01-04 16:07:15 +0000
@@ -15,8 +15,11 @@
             permission="launchpad.Special"
             interface="canonical.launchpad.interfaces.IAccountSpecialRestricted" />
         <require
+            permission="launchpad.Moderate"
+            set_attributes="status date_status_set status_comment" />
+        <require
             permission="launchpad.Edit"
-            set_schema="canonical.launchpad.interfaces.IAccount" />
+            set_attributes="displayname password" />
     </class>
 
     <securedutility

=== modified file 'lib/lp/bugs/scripts/checkwatches.py'
--- lib/lp/bugs/scripts/checkwatches.py	2009-12-15 16:28:20 +0000
+++ lib/lp/bugs/scripts/checkwatches.py	2010-01-04 16:07:15 +0000
@@ -789,7 +789,16 @@
             )
 
         for bug_id in all_remote_ids:
-            bug_watches = bug_watches_by_remote_bug[bug_id]
+            try:
+                bug_watches = bug_watches_by_remote_bug[bug_id]
+            except KeyError:
+                # If there aren't any bug watches for this remote bug,
+                # just log a warning and carry on.
+                self.warning(
+                    "Spurious remote bug ID: No watches found for "
+                    "remote bug %s on %s" % (bug_id, remotesystem.baseurl))
+                continue
+
             for bug_watch in bug_watches:
                 bug_watch.lastchecked = UTC_NOW
             if bug_id in unmodified_remote_ids:

=== modified file 'lib/lp/bugs/scripts/tests/test_checkwatches.py'
--- lib/lp/bugs/scripts/tests/test_checkwatches.py	2009-12-21 16:08:35 +0000
+++ lib/lp/bugs/scripts/tests/test_checkwatches.py	2010-01-04 16:07:15 +0000
@@ -15,6 +15,8 @@
 
 from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI
 from lp.bugs.scripts import checkwatches
+from lp.bugs.scripts.checkwatches import CheckWatchesErrorUtility
+from lp.bugs.tests.externalbugtracker import TestBugzillaAPIXMLRPCTransport
 from lp.testing import TestCaseWithFactory
 
 
@@ -23,6 +25,52 @@
     return BugzillaAPI(bugtracker.baseurl)
 
 
+class NonConnectingBugzillaAPI(BugzillaAPI):
+    """A non-connected version of the BugzillaAPI ExternalBugTracker."""
+
+    bugs = {
+        1: {'product': 'test-product'},
+        }
+
+    def getCurrentDBTime(self):
+        return None
+
+    def getExternalBugTrackerToUse(self):
+        return self
+
+    def getProductsForRemoteBugs(self, remote_bugs):
+        """Return the products for some remote bugs.
+
+        This method is basically the same as that of the superclass but
+        without the call to initializeRemoteBugDB().
+        """
+        bug_products = {}
+        for bug_id in bug_ids:
+            # If one of the bugs we're trying to get the product for
+            # doesn't exist, just skip it.
+            try:
+                actual_bug_id = self._getActualBugId(bug_id)
+            except BugNotFound:
+                continue
+
+            bug_dict = self._bugs[actual_bug_id]
+            bug_products[bug_id] = bug_dict['product']
+
+        return bug_products
+
+
+class NoBugWatchesByRemoteBugUpdater(checkwatches.BugWatchUpdater):
+    """A subclass of BugWatchUpdater with methods overridden for testing."""
+
+    def _getBugWatchesByRemoteBug(self, bug_watch_ids):
+        """Return an empty dict.
+
+        This method overrides _getBugWatchesByRemoteBug() so that bug
+        497141 can be regression-tested.
+        """
+        return {}
+
+
 class TestCheckwatchesWithSyncableGnomeProducts(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
@@ -63,5 +111,40 @@
             gnome_bugzilla, [bug_watch_1, bug_watch_2])
 
 
+class TestBugWatchUpdater(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_bug_497141(self):
+        # Regression test for bug 497141. KeyErrors raised in
+        # BugWatchUpdater.updateBugWatches() shouldn't cause
+        # checkwatches to abort.
+        updater = NoBugWatchesByRemoteBugUpdater(
+            transaction, QuietFakeLogger())
+
+        # Create a couple of bug watches for testing purposes.
+        bug_tracker = self.factory.makeBugTracker()
+        bug_watches = [
+            self.factory.makeBugWatch(bugtracker=bug_tracker)
+            for i in range(2)]
+
+        # Use a test XML-RPC transport to ensure no connections happen.
+        test_transport = TestBugzillaAPIXMLRPCTransport(bug_tracker.baseurl)
+        remote_system = NonConnectingBugzillaAPI(
+            bug_tracker.baseurl, xmlrpc_transport=test_transport)
+
+        # Calling updateBugWatches() shouldn't raise a KeyError, even
+        # though with our broken updater _getExternalBugTrackersAndWatches()
+        # will return an empty dict.
+        updater.updateBugWatches(remote_system, bug_watches)
+
+        # An error will have been logged instead of the KeyError being
+        # raised.
+        error_utility = CheckWatchesErrorUtility()
+        last_oops = error_utility.getLastOopsReport()
+        self.assertTrue(
+            last_oops.value.startswith('Spurious remote bug ID'))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-12-05 18:37:28 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-01-04 16:07:15 +0000
@@ -761,7 +761,7 @@
             name="+reviewaccount"
             for="lp.registry.interfaces.person.IPerson"
             class="lp.registry.browser.person.PersonAccountAdministerView"
-            permission="launchpad.Admin"
+            permission="launchpad.Moderate"
             template="../templates/person-review.pt"/>
         <browser:page
             name="+claimteam"

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2009-12-23 16:17:12 +0000
+++ lib/lp/registry/browser/person.py	2010-01-04 16:07:15 +0000
@@ -901,6 +901,12 @@
         text = 'Administer'
         return Link(target, text, icon='edit')
 
+    @enabled_with_permission('launchpad.Moderate')
+    def administer_account(self):
+        target = '+reviewaccount'
+        text = 'Administer Account'
+        return Link(target, text, icon='edit')
+
 
 class PersonOverviewMenu(ApplicationMenu, PersonMenuMixin):
 
@@ -910,9 +916,10 @@
              'editemailaddresses', 'editlanguages', 'editwikinames',
              'editircnicknames', 'editjabberids', 'editpassword',
              'editsshkeys', 'editpgpkeys', 'editlocation', 'memberships',
-             'codesofconduct', 'karma', 'administer', 'projects',
-             'activate_ppa', 'maintained', 'view_ppa_subscriptions',
-             'ppa', 'oauth_tokens', 'related_software_summary']
+             'codesofconduct', 'karma', 'administer', 'administer_account',
+             'projects', 'activate_ppa', 'maintained',
+             'view_ppa_subscriptions', 'ppa', 'oauth_tokens',
+             'related_software_summary']
 
     def related_software_summary(self):
         target = '+related-software'
@@ -1687,8 +1694,6 @@
     """Administer an `IAccount` belonging to an `IPerson`."""
     schema = IAccount
     label = "Review person's account"
-    field_names = [
-        'displayname', 'password', 'status', 'status_comment']
     custom_widget(
         'status_comment', TextAreaWidget, height=5, width=60)
     custom_widget('password', PasswordChangeWidget)
@@ -1697,9 +1702,14 @@
         """See `LaunchpadEditFormView`."""
         super(PersonAccountAdministerView, self).__init__(context, request)
         # Only the IPerson can be traversed to, so it provides the IAccount.
+        # It also means that permissions are checked on IAccount, not IPerson.
         self.person = self.context
         from canonical.launchpad.interfaces import IMasterObject
         self.context = IMasterObject(self.context.account)
+        # Set fields to be displayed.
+        self.field_names = ['status', 'status_comment']
+        if self.viewed_by_admin:
+            self.field_names = ['displayname', 'password'] + self.field_names
 
     @property
     def is_viewing_person(self):
@@ -1711,6 +1721,11 @@
         return False
 
     @property
+    def viewed_by_admin(self):
+        """Is the user a Launchpad admin?"""
+        return check_permission('launchpad.Admin', self.context)
+
+    @property
     def email_addresses(self):
         """A list of the user's preferred and validated email addresses."""
         emails = sorted(
@@ -1722,6 +1737,10 @@
     @property
     def next_url(self):
         """See `LaunchpadEditFormView`."""
+        is_suspended = self.context.status == AccountStatus.SUSPENDED
+        if is_suspended and not self.viewed_by_admin:
+            # Non-admins cannot see suspended persons.
+            return canonical_url(getUtility(IPersonSet))
         return canonical_url(self.person)
 
     @property
@@ -1739,6 +1758,9 @@
             # email is sent to the user.
             data['password'] = 'invalid'
             self.person.setPreferredEmail(None)
+            self.request.response.addNoticeNotification(
+                u'The account "%s" has been suspended.' % (
+                    self.context.displayname))
         if (data['status'] == AccountStatus.ACTIVE
             and self.context.status != AccountStatus.ACTIVE):
             self.request.response.addNoticeNotification(
@@ -5834,7 +5856,8 @@
     usedfor = IPersonIndexMenu
     facet = 'overview'
     title = 'Change person'
-    links = ('edit', 'administer', 'branding', 'editpassword')
+    links = ('edit', 'administer', 'administer_account',
+             'branding', 'editpassword')
 
 
 class ITeamIndexMenu(Interface):

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2009-08-13 19:03:36 +0000
+++ lib/lp/registry/doc/person-account.txt	2010-01-04 16:07:15 +0000
@@ -26,9 +26,9 @@
     None
 
 The account can only be activated by the user who is claiming
-the profile. Mark cannot claim it.
+the profile. Sample Person cannot claim it.
 
-    >>> login('[e-mail]')
+    >>> login('[e-mail]')
     >>> matsubara.account.activate(
     ...     comment="test", password='ok', preferred_email=emailaddress)
     Traceback (most recent call last):

=== modified file 'lib/lp/registry/stories/person/xx-admin-person-review.txt'
--- lib/lp/registry/stories/person/xx-admin-person-review.txt	2009-12-23 16:17:12 +0000
+++ lib/lp/registry/stories/person/xx-admin-person-review.txt	2010-01-04 16:07:15 +0000
@@ -51,7 +51,8 @@
     >>> print link.text
     edit[IMG] Review the user's Launchpad information
 
-The admin can update the user's account.
+The admin can update the user's account. Suspending an account will give a
+feedback message.
 
     >>> admin_browser.getControl(
     ...     'The status of this account').value = ['SUSPENDED']
@@ -61,6 +62,8 @@
 
     >>> print admin_browser.title
     The one and only Salgado does not use Launchpad
+    >>> print get_feedback_messages(admin_browser.contents)[0]
+    The account "The one and only Salgado" has been suspended.
 
 The admin can see the account information of a user that does not use
 Launchpad, and can change the account too. Note that all pages that belong
@@ -88,6 +91,57 @@
     The user is reactivated. He must use the "forgot password" to log in.
 
 
+Partial access for registry experts
+-----------------------------------
+
+Members of the registry team get partial access to the review account page to
+be able to suspend spam accounts.
+
+    >>> login('[e-mail]')
+    >>> registry_expert = factory.makePerson(email='[e-mail]',
+    ...                                      password='test')
+    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
+    >>> from zope.component import getUtility
+    >>> registry_team = getUtility(ILaunchpadCelebrities).registry_experts
+    >>> registry_team.addMember(registry_expert, registry_team.teamowner)
+    >>> print registry_expert.inTeam(registry_team)
+    True
+    >>> logout()
+
+    >>> expert_browser = setupBrowser(auth='Basic [e-mail]:test')
+    >>> expert_browser.open('http://launchpad.dev/~no-priv/+reviewaccount')
+    >>> print expert_browser.title
+    Review person's account...
+
+The +reviewaccount page does not display account information for registry
+experts. It also has no form elements to change the display name or the
+password.
+
+    >>> content = find_main_content(expert_browser.contents)
+    >>> print content.find(id='summary')
+    None
+    >>> print content.find(name='field.displayname')
+    None
+    >>> print content.find(name='field.password')
+    None
+
+The only option for registry experts is to change an account's status and to
+provide a reason why they did it. After suspending the account the
+page will only be visible to admins, so the registry expert will be
+redirected to the "people" main page. A feedback message informs the expert
+about the suspension.
+
+    >>> control = expert_browser.getControl(name='field.status_comment')
+    >>> control.value = 'This is SPAM!'
+    >>> expert_browser.getControl(
+    ...     'The status of this account').value = ['SUSPENDED']
+    >>> expert_browser.getControl('Change').click()
+    >>> print expert_browser.url
+    http://launchpad.dev/people
+    >>> print get_feedback_messages(expert_browser.contents)[0]
+    The account "No Privileges Person" has been suspended.
+
+
 Restricted to admins
 --------------------
 

=== modified file 'lib/lp/registry/templates/person-review.pt'
--- lib/lp/registry/templates/person-review.pt	2009-09-15 15:42:39 +0000
+++ lib/lp/registry/templates/person-review.pt	2010-01-04 16:07:15 +0000
@@ -33,7 +33,8 @@
             </p>
           </tal:review-person>
 
-          <tal:review-account tal:condition="not: view/is_viewing_person">
+          <tal:review-account
+               condition="python: not view.is_viewing_person and view.viewed_by_admin">
             <p>
               The account displayname is not always the same as the Launchpad
               displayname.

=== renamed file 'lib/lp/translations/utilities/permission_helpers.py' => 'lib/lp/services/permission_helpers.py'
--- lib/lp/translations/utilities/permission_helpers.py	2009-11-17 09:50:33 +0000
+++ lib/lp/services/permission_helpers.py	2010-01-04 16:07:15 +0000
@@ -6,7 +6,11 @@
 __metaclass__ = type
 
 __all__ = [
+    'is_admin',
+    'is_admin_or_registry_expert',
     'is_admin_or_rosetta_expert',
+    'is_registry_expert',
+    'is_rosetta_expert',
     ]
 
 from zope.component import getUtility
@@ -14,8 +18,29 @@
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 
 
+def is_admin(user):
+    """Check if the user is a Launchpad admin."""
+    celebrities = getUtility(ILaunchpadCelebrities)
+    return user.inTeam(celebrities.admin)
+
+
+def is_rosetta_expert(user):
+    """Check if the user is a Rosetta expert."""
+    celebrities = getUtility(ILaunchpadCelebrities)
+    return user.inTeam(celebrities.rosetta_experts)
+
+
+def is_registry_expert(user):
+    """Check if the user is a registry expert."""
+    celebrities = getUtility(ILaunchpadCelebrities)
+    return user.inTeam(celebrities.registry_experts)
+
+
 def is_admin_or_rosetta_expert(user):
-    """Check if the user is a Launchpad admin or a Rosettta expert."""
-    celebrities = getUtility(ILaunchpadCelebrities)
-    return (user.inTeam(celebrities.admin) or
-            user.inTeam(celebrities.rosetta_experts))
+    """Check if the user is a Launchpad admin or a Rosetta expert."""
+    return is_admin(user) or is_rosetta_expert(user)
+
+
+def is_admin_or_registry_expert(user):
+    """Check if the user is a Launchpad admin or a registry expert."""
+    return is_admin(user) or is_registry_expert(user)

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2009-12-18 09:25:14 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2010-01-04 16:07:15 +0000
@@ -43,6 +43,7 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.services.permission_helpers import is_admin_or_rosetta_expert
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
@@ -62,8 +63,6 @@
 from lp.translations.interfaces.translations import TranslationConstants
 from lp.translations.utilities.gettext_po_importer import (
     GettextPOImporter)
-from lp.translations.utilities.permission_helpers import (
-    is_admin_or_rosetta_expert)
 from canonical.librarian.interfaces import ILibrarianClient
 
 

=== modified file 'versions.cfg'
--- versions.cfg	2009-12-22 23:35:26 +0000
+++ versions.cfg	2010-01-04 16:07:15 +0000
@@ -19,7 +19,7 @@
 grokcore.component = 1.6
 httplib2 = 0.4.0
 ipython = 0.9.1
-launchpadlib = 1.5.3.1
+launchpadlib = 1.5.4
 lazr.authentication = 0.1.1
 lazr.batchnavigator = 1.1
 lazr.config = 1.1.3