Friendly JSON exceptions (bug 928061, bug 928062)

Example http://pastie.org/3338663

Change-Id: I26f53488c062ebfb6e49cfcf82e0b8179a683ea8

keystone/common/wsgi.py

@@ -32,6 +32,7 @@
 import webob.dec
 import webob.exc
 
+from keystone import exception
 from keystone.common import utils
 
 
@@ -153,24 +154,26 @@ class Application(BaseApplication):
     @webob.dec.wsgify
     def __call__(self, req):
         arg_dict = req.environ['wsgiorg.routing_args'][1]
-        action = arg_dict['action']
-        del arg_dict['action']
+        action = arg_dict.pop('action')
         del arg_dict['controller']
         logging.debug('arg_dict: %s', arg_dict)
 
+        # allow middleware up the stack to provide context & params
         context = req.environ.get('openstack.context', {})
-        # allow middleware up the stack to override the params
-        params = {}
-        if 'openstack.params' in req.environ:
-            params = req.environ['openstack.params']
+        params = req.environ.get('openstack.params', {})
         params.update(arg_dict)
 
         # TODO(termie): do some basic normalization on methods
         method = getattr(self, action)
 
         # NOTE(vish): make sure we have no unicode keys for py2.6.
         params = self._normalize_dict(params)
-        result = method(context, **params)
+
+        try:
+            result = method(context, **params)
+        except exception.Error as e:
+            logging.warning(e)
+            return render_exception(e)
 
         if result is None or type(result) is str or type(result) is unicode:
             return result
@@ -435,3 +438,22 @@ def _factory(app):
             conf.update(local_config)
             return cls(app)
         return _factory
+
+
+def render_exception(error):
+    """Forms a WSGI response based on the current error."""
+    resp = webob.Response()
+    resp.status = '%s %s' % (error.code, error.title)
+    resp.headerlist = [('Content-Type', 'application/json')]
+
+    body = {
+        'error': {
+            'code': error.code,
+            'title': error.title,
+            'message': str(error),
+        }
+    }
+
+    resp.body = json.dumps(body)
+
+    return resp

keystone/exception.py

@@ -0,0 +1,54 @@
+import re
+
+
+class Error(StandardError):
+    """Base error class.
+
+    Child classes should define an HTTP status code, title, and a doc string.
+
+    """
+    code = None
+    title = None
+
+    def __init__(self, message=None, **kwargs):
+        """Use the doc string as the error message by default."""
+        message = message or self.__doc__ % kwargs
+        super(Error, self).__init__(message)
+
+    def __str__(self):
+        """Cleans up line breaks and indentation from doc strings."""
+        string = super(Error, self).__str__()
+        string = re.sub('[ \n]+', ' ', string)
+        string = string.strip()
+        return string
+
+
+class ValidationError(Error):
+    """Expecting to find %(attribute)s in %(target)s.
+
+    The server could not comply with the request since it is either malformed
+    or otherwise incorrect.
+
+    The client is assumed to be in error.
+
+    """
+    code = 400
+    title = 'Bad Request'
+
+
+class Unauthorized(Error):
+    """The request you have made requires authentication."""
+    code = 401
+    title = 'Not Authorized'
+
+
+class Forbidden(Error):
+    """You are not authorized to perform the requested action: %(action)s"""
+    code = 403
+    title = 'Not Authorized'
+
+
+class NotFound(Error):
+    """Could not find: %(target)s"""
+    code = 404
+    title = 'Not Found'

keystone/identity/core.py

@@ -10,6 +10,7 @@
 
 from keystone import catalog
 from keystone import config
+from keystone import exception
 from keystone import policy
 from keystone import token
 from keystone.common import manager
@@ -233,7 +234,9 @@ def get_tenants_for_token(self, context, **kw):
         """
         token_ref = self.token_api.get_token(context=context,
                                              token_id=context['token_id'])
-        assert token_ref is not None
+
+        if token_ref is None:
+            raise exception.Unauthorized()
 
         user_ref = token_ref['user']
         tenant_ids = self.identity_api.get_tenants_for_user(

keystone/service.py

@@ -10,6 +10,7 @@
 import webob.exc
 
 from keystone import catalog
+from keystone import exception
 from keystone import identity
 from keystone import policy
 from keystone import token
@@ -196,6 +197,10 @@ def authenticate(self, context, auth=None):
 
             old_token_ref = self.token_api.get_token(context=context,
                                                      token_id=token)
+
+            if old_token_ref is None:
+                raise exception.Unauthorized()
+
             user_ref = old_token_ref['user']
 
             tenants = self.identity_api.get_tenants_for_user(context,
@@ -247,6 +252,10 @@ def validate_token(self, context, token_id, belongs_to=None):
 
         token_ref = self.token_api.get_token(context=context,
                                              token_id=token_id)
+
+        if token_ref is None:
+            raise exception.NotFound(target='token')
+
         if belongs_to:
             assert token_ref['tenant']['id'] == belongs_to
 

tests/test_exception.py

@@ -0,0 +1,53 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+import uuid
+import json
+
+from keystone.common import wsgi
+from keystone import exception
+from keystone import test
+
+
+class ExceptionTestCase(test.TestCase):
+    def setUp(self):
+        pass
+
+    def tearDown(self):
+        pass
+
+    def assertValidJsonRendering(self, e):
+        resp = wsgi.render_exception(e)
+        self.assertEqual(resp.status_int, e.code)
+        self.assertEqual(resp.status, '%s %s' % (e.code, e.title))
+
+        j = json.loads(resp.body)
+        self.assertIsNotNone(j.get('error'))
+        self.assertIsNotNone(j['error'].get('code'))
+        self.assertIsNotNone(j['error'].get('title'))
+        self.assertIsNotNone(j['error'].get('message'))
+        self.assertNotIn('\n', j['error']['message'])
+        self.assertNotIn('  ', j['error']['message'])
+        self.assertTrue(type(j['error']['code']) is int)
+
+    def test_validation_error(self):
+        target = uuid.uuid4().hex
+        attribute = uuid.uuid4().hex
+        e = exception.ValidationError(target=target, attribute=attribute)
+        self.assertValidJsonRendering(e)
+        self.assertIn(target, str(e))
+        self.assertIn(attribute, str(e))
+
+    def test_unauthorized(self):
+        e = exception.Unauthorized()
+        self.assertValidJsonRendering(e)
+
+    def test_forbidden(self):
+        action = uuid.uuid4().hex
+        e = exception.Forbidden(action=action)
+        self.assertValidJsonRendering(e)
+        self.assertIn(action, str(e))
+
+    def test_not_found(self):
+        target = uuid.uuid4().hex
+        e = exception.NotFound(target=target)
+        self.assertValidJsonRendering(e)
+        self.assertIn(target, str(e))

tests/test_keystoneclient.py

@@ -125,6 +125,8 @@ def test_authenticate_token_tenant_name(self):
         self.assertEquals(tenants[0].id, self.tenant_bar['id'])
 
     def test_authenticate_and_delete_token(self):
+        from keystoneclient import exceptions as client_exceptions
+
         client = self.get_client()
         token = client.auth_token
         token_client = self._client(token=token)
@@ -133,10 +135,26 @@ def test_authenticate_and_delete_token(self):
 
         client.tokens.delete(token_client.auth_token)
 
-        # FIXME(dolph): this should raise unauthorized
-        # from keystoneclient import exceptions as client_exceptions
-        # with self.assertRaises(client_exceptions.Unauthorized):
-        self.assertRaises(Exception, token_client.tenants.list)
+        self.assertRaises(client_exceptions.Unauthorized,
+                          token_client.tenants.list)
+
+    def test_authenticate_no_password(self):
+        from keystoneclient import exceptions as client_exceptions
+
+        user_ref = self.user_foo.copy()
+        user_ref['password'] = None
+        self.assertRaises(client_exceptions.AuthorizationFailure,
+                          self.get_client,
+                          user_ref)
+
+    def test_authenticate_no_username(self):
+        from keystoneclient import exceptions as client_exceptions
+
+        user_ref = self.user_foo.copy()
+        user_ref['name'] = None
+        self.assertRaises(client_exceptions.AuthorizationFailure,
+                          self.get_client,
+                          user_ref)
 
     # TODO(termie): I'm not really sure that this is testing much
     def test_endpoints(self):