Merge lp:~bac/launchpad/bug-750984 into lp:launchpad

[Brad Crittenden]

[r=jcsackett][bug=750984] Don't OOPS on bad API token/nonce/timestamp.

= Summary =

Previously problems with API token, nonce, or timestamp resulted in an
Unauthorized exception being raised which caused an OOPS to be
generated. Since the likely cause of those problems is bad client data
(though server side processing could be at fault) it is inappropriate to
generated an OOPS.

== Proposed fix ==

Rather than returning Unauthorized return custom exceptions that have
been marked with webservice_error as 401. The client still sees an HTTP
401 but no OOPS is reported.

== Pre-implementation notes ==

Talks with Gary and lots of conversation on the bug between Robert and
Martin.

== Tests ==

bin/test -vvt webapp-publication.txt

== Demo and Q/A ==

It should be possible to force bad client data but the details are not
obvious at the moment.

= Launchpad lint =

The lint issues are not fixable.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/interfaces/oauth.py
  lib/canonical/launchpad/doc/webapp-publication.txt
  lib/canonical/launchpad/webapp/servers.py

./lib/canonical/launchpad/doc/webapp-publication.txt
     195: want exceeds 78 characters.
     203: want exceeds 78 characters.
     325: want exceeds 78 characters.

[j.c.sackett (jcsackett)]

This looks good to land.

review: Approve

=== modified file 'lib/canonical/launchpad/doc/webapp-publication.txt'
--- lib/canonical/launchpad/doc/webapp-publication.txt	2011-04-21 01:30:30 +0000
+++ lib/canonical/launchpad/doc/webapp-publication.txt	2011-05-26 19:11:35 +0000
@@ -1,4 +1,5 @@
-= Launchpad Publication =
+Launchpad Publication
+=====================
 
 Launchpad uses the generic Zope3 publisher. It registers several
 factories that are responsible for instantiating the appropriate
@@ -6,7 +7,8 @@
 zope.publisher.IPublication for the request.
 
 
-== Virtual host configurations ==
+Virtual host configurations
+---------------------------
 
 The configuration defines a number of domains, one for the main
 Launchpad site and one for the sites of the various applications.
@@ -118,7 +120,8 @@
     xmlrpc.launchpad.dev
 
 
-== VirtualHostRequestPublicationFactory ==
+VirtualHostRequestPublicationFactory
+------------------------------------
 
 A number of VirtualHostRequestPublicationFactories are registered with
 Zope to handle requests for a particular vhost, port, and set of HTTP
@@ -322,7 +325,8 @@
     <class 'canonical.launchpad.webapp.publication.LaunchpadBrowserPublication'>
 
 
-== Zope Publisher integration ==
+Zope Publisher integration
+--------------------------
 
 A factory is registered for each of our available virtual host. This
 is done by the register_launchpad_request_publication_factories
@@ -417,7 +421,8 @@
       Allow: POST
 
     >>> print_request_and_publication(
-    ...     'xmlrpc.launchpad.dev', method='POST', mime_type='application/xml')
+    ...     'xmlrpc.launchpad.dev', method='POST',
+    ...     mime_type='application/xml')
     ProtocolErrorRequest
     ProtocolErrorPublication: status=415
 
@@ -483,7 +488,8 @@
     >>> login(ANONYMOUS)
 
 
-== ILaunchpadBrowserApplicationRequest ==
+ILaunchpadBrowserApplicationRequest
+-----------------------------------
 
 All Launchpad requests provides the ILaunchpadBrowserApplicationRequest
 interface. That interface is an extension of the zope standard
@@ -497,7 +503,8 @@
     True
 
 
-== Handling form data using IBrowserFormNG ==
+Handling form data using IBrowserFormNG
+---------------------------------------
 
 Submitted form data is available in the form_ng request attribute. This
 is an object providing the IBrowserFormNG interface which offers two
@@ -582,7 +589,8 @@
     items_field
 
 
-== Page ID ==
+Page ID
+-------
 
 Our publication implementation sets a WSGI variable 'launchpad.pageid'.
 This is an identifier of the form ContextName:ViewName.
@@ -654,7 +662,8 @@
     ''
 
 
-== Tick counts ==
+Tick counts
+-----------
 
 Similarly to our page IDs, our publication implementation will store the
 tick counts for the traversal and object publication processes in WSGI
@@ -832,7 +841,8 @@
     >>> login(ANONYMOUS)
 
 
-== Transaction Logging ==
+Transaction Logging
+-------------------
 
 The publication implementation is responsible for putting the name
 of the logged in user in the transaction. (The afterCall() hook is
@@ -872,7 +882,8 @@
      / 16
 
 
-== Read-Only Requests ==
+Read-Only Requests
+------------------
 
 Our publication implementation make sure that requests supposed to be
 read-only (GET and HEAD) don't change anything in the database.
@@ -943,7 +954,8 @@
     Darwin
 
 
-=== GET requests on api.launchpad.net ===
+GET requests on api.launchpad.net
+.................................
 
 In the case of WebServicePublication, though, we have to commit the
 transaction after GET requests in order to persist new entries added to
@@ -992,13 +1004,14 @@
     ...     'api.launchpad.dev', 'GET',
     ...     extra_environment=dict(QUERY_STRING=urlencode(form)))
     >>> test_request.processInputs()
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Invalid nonce/timestamp: This nonce has been used already.
-
-
-== Doomed transactions are aborted ==
+    NonceAlreadyUsed: This nonce has been used already.
+
+
+Doomed transactions are aborted
+-------------------------------
 
 Doomed transactions are aborted.
 
@@ -1032,7 +1045,8 @@
     >>> del txn.abort # Clean up test fixture.
 
 
-== Requests on Python C Methods succeed ==
+Requests on Python C Methods succeed
+------------------------------------
 
 Rarely but occasionally, it is possible to traverse to a Python C method.
 For instance, an XMLRPC proxy might allow a traversal to __repr__.
@@ -1048,7 +1062,8 @@
     '{}'
 
 
-== HEAD requests have empty body ==
+HEAD requests have empty body
+-----------------------------
 
 The publication implementation also makes sure that no body is
 returned as part of HEAD requests. (Again this is handled by the
@@ -1085,7 +1100,8 @@
     Some boring content.
 
 
-== Authentication of requests ==
+Authentication of requests
+--------------------------
 
 In LaunchpadBrowserPublication, authentication happens in the
 beforeTraversal() hook. Our publication will set the principal to
@@ -1183,10 +1199,11 @@
     >>> form2 = form.copy()
     >>> form2['oauth_nonce'] = '1764572616e48616d6d65724c61686'
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Expired token...
+    TokenException: Expired token...
+
     >>> access_token.date_expires = now + timedelta(days=1)
     >>> syncUpdate(access_token)
 
@@ -1194,10 +1211,10 @@
     >>> form2['oauth_token'] += 'z'
     >>> form2['oauth_nonce'] = '4572616e48616d6d65724c61686176'
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Unknown access token...
+    TokenException: Unknown access token...
 
 The consumer must be registered as well, and the signature must be
 correct.
@@ -1205,7 +1222,7 @@
     >>> form2 = form.copy()
     >>> form2['oauth_consumer_key'] += 'z'
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
     Unauthorized: Unknown consumer (foobar123451432z).
@@ -1214,10 +1231,10 @@
     >>> form2['oauth_signature'] += 'z'
     >>> form2['oauth_nonce'] = '2616e48616d6d65724c61686176457'
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Invalid signature.
+    TokenException: Invalid signature.
 
 The nonce specified in the request must not have been used before in a request
 with this same token and timestamp.
@@ -1229,10 +1246,10 @@
     Guilherme Salgado
 
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Invalid nonce/timestamp: This nonce has been used already.
+    NonceAlreadyUsed: This nonce has been used already.
 
 The timestamp must not be older than TIMESTAMP_ACCEPTANCE_WINDOW from the most
 recent request for this token.
@@ -1247,10 +1264,10 @@
 
     >>> form2['oauth_timestamp'] -= 10
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Invalid nonce/timestamp: Timestamp too old compared ...
+    TimestampOrderingError: Timestamp too old compared ...
 
 Last but not least, the timestamp must not be too far in the future, as
 defined by TIMESTAMP_SKEW_WINDOW.
@@ -1260,10 +1277,10 @@
     >>> form2 = form.copy()
     >>> form2['oauth_timestamp'] += (TIMESTAMP_SKEW_WINDOW+10)
     >>> test_request = LaunchpadTestRequest(form=form2)
-    >>> print publication.getPrincipal(test_request).title
+    >>> publication.getPrincipal(test_request)
     Traceback (most recent call last):
     ...
-    Unauthorized: Invalid nonce/timestamp: Timestamp ... from bad system clock
+    ClockSkew: Timestamp ... from bad system clock
 
 Close the bogus request that was started by the call to
 beforeTraversal, in order to ensure we leave our state sane.
 
=== modified file 'lib/canonical/launchpad/interfaces/oauth.py'
--- lib/canonical/launchpad/interfaces/oauth.py	2010-10-18 11:34:31 +0000
+++ lib/canonical/launchpad/interfaces/oauth.py	2011-05-26 19:11:35 +0000
@@ -20,8 +20,10 @@
     'NonceAlreadyUsed',
     'TimestampOrderingError',
     'ClockSkew',
+    'TokenException',
     ]
 
+import httplib
 from zope.interface import (
     Attribute,
     Interface,
@@ -34,6 +36,8 @@
     TextLine,
     )
 
+from lazr.restful.declarations import webservice_error
+
 from canonical.launchpad import _
 from canonical.launchpad.webapp.interfaces import (
     AccessLevel,
@@ -300,14 +304,26 @@
     """Marker interface for a request signed with OAuth credentials."""
 
 
-# Note that these three exceptions are converted to Unauthorized (equating to
-# 401 status) in webapp/servers.py, WebServicePublication.getPrincipal.
-
-class NonceAlreadyUsed(Exception):
+# Note that these exceptions are marked as UNAUTHORIZED (401 status)
+# so they may be raised but will not cause an OOPS to be generated.  The
+# client will see them as an UNAUTHORIZED error.
+
+class _TokenException(Exception):
+    """Base class for token, nonce, and timestamp exceptions."""
+    webservice_error(httplib.UNAUTHORIZED)
+
+
+class NonceAlreadyUsed(_TokenException):
     """Nonce has been used together with same token but another timestamp."""
 
-class TimestampOrderingError(Exception):
+
+class TimestampOrderingError(_TokenException):
     """Timestamp is too old, compared to the last request."""
 
-class ClockSkew(Exception):
+
+class ClockSkew(_TokenException):
     """Timestamp is too far off from server's clock."""
+
+
+class TokenException(_TokenException):
+    """Token has expired."""
 
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2011-05-20 19:35:04 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2011-05-26 19:11:35 +0000
@@ -67,11 +67,9 @@
     IWebServiceApplication,
     )
 from canonical.launchpad.interfaces.oauth import (
-    ClockSkew,
     IOAuthConsumerSet,
     IOAuthSignedRequest,
-    NonceAlreadyUsed,
-    TimestampOrderingError,
+    TokenException,
     )
 import canonical.launchpad.layers
 from canonical.launchpad.webapp.authentication import (
@@ -1025,7 +1023,7 @@
 
 
 http = wsgi.ServerType(
-    ZServerTracelogServer, # subclass of WSGIHTTPServer
+    ZServerTracelogServer,  # subclass of WSGIHTTPServer
     WSGIPublisherApplication,
     LaunchpadAccessLogger,
     8080,
@@ -1039,7 +1037,7 @@
     True)
 
 debughttp = wsgi.ServerType(
-    ZServerTracelogServer, # subclass of WSGIHTTPServer
+    ZServerTracelogServer,  # subclass of WSGIHTTPServer
     WSGIPublisherApplication,
     LaunchpadAccessLogger,
     8082,
@@ -1047,7 +1045,7 @@
     requestFactory=DebugLayerRequestFactory)
 
 privatexmlrpc = wsgi.ServerType(
-    ZServerTracelogServer, # subclass of WSGIHTTPServer
+    ZServerTracelogServer,  # subclass of WSGIHTTPServer
     WSGIPublisherApplication,
     LaunchpadAccessLogger,
     8080,
@@ -1208,6 +1206,14 @@
 
         Web service requests are authenticated using OAuth, except for the
         one made using (presumably) JavaScript on the /api override path.
+
+        Raises a variety of token errors (ClockSkew, NonceAlreadyUsed,
+        TimestampOrderingError, TokenException) which have a webservice error
+        status of Unauthorized - 401.  All of these exceptions represent
+        errors on the part of the client.
+
+        Raises Unauthorized directly in the case where the consumer is None
+        for a non-anonymous request as it may represent a server error.
         """
         # Use the regular HTTP authentication, when the request is not
         # on the API virtual host but comes through the path_override on
@@ -1250,7 +1256,7 @@
                 # transactions committed so that we can keep track of
                 # the OAuth nonces and prevent replay attacks.
                 if consumer_key == '' or consumer_key is None:
-                    raise Unauthorized("No consumer key specified.")
+                    raise TokenException("No consumer key specified.")
                 consumer = consumers.new(consumer_key, '')
             else:
                 # An unknown consumer can never make a non-anonymous
@@ -1270,19 +1276,16 @@
             return auth_utility.unauthenticatedPrincipal()
         token = consumer.getAccessToken(token_key)
         if token is None:
-            raise Unauthorized('Unknown access token (%s).' % token_key)
+            raise TokenException('Unknown access token (%s).' % token_key)
         nonce = form.get('oauth_nonce')
         timestamp = form.get('oauth_timestamp')
-        try:
-            token.checkNonceAndTimestamp(nonce, timestamp)
-        except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e:
-            raise Unauthorized('Invalid nonce/timestamp: %s' % e)
+        token.checkNonceAndTimestamp(nonce, timestamp)
         if token.permission == OAuthPermission.UNAUTHORIZED:
-            raise Unauthorized('Unauthorized token (%s).' % token.key)
+            raise TokenException('Unauthorized token (%s).' % token.key)
         elif token.is_expired:
-            raise Unauthorized('Expired token (%s).' % token.key)
+            raise TokenException('Expired token (%s).' % token.key)
         elif not check_oauth_signature(request, consumer, token):
-            raise Unauthorized('Invalid signature.')
+            raise TokenException('Invalid signature.')
         else:
             # Everything is fine, let's return the principal.
             pass
@@ -1529,7 +1532,8 @@
     # len(factories)+1.
     for priority, factory in enumerate(factories):
         publisher_factory_registry.register(
-            "*", "*", factory.vhost_name, len(factories)-priority+1, factory)
+            "*", "*", factory.vhost_name, len(factories) - priority + 1,
+            factory)
 
     # Register a catch-all "not found" handler at the lowest priority.
     publisher_factory_registry.register(