Wrapped unexpected exceptions (bug 955411)

- Replaced all webob.exc's (outside of middleware) with keystone.exception's - Raised 409 Conflict when creating/updating existing user/tenant ID/names (bug 955464) - Raised 501 Not Implemented for user-role-add w/o tenant_id (bug 955548) Change-Id: I9f16cac502c20dd35a6b8da778e85bf3d9cfae49
2012-03-14 15:06:16 -05:00 · 2012-03-14 15:06:16 -05:00 · 009d661a7e
commit 009d661a7e
parent 3a70a2f928
14 changed files with 123 additions and 57 deletions
--- a/keystone/catalog/backends/sql.py
+++ b/keystone/catalog/backends/sql.py
@ -125,7 +125,7 @@ class Catalog(sql.Base, catalog.Driver):
        endpoint_ref = session.query(Endpoint)
        endpoint_ref = endpoint_ref.filter_by(id=endpoint_id).first()
        if not endpoint_ref:
-            raise exception.NotFound('Endpoint not found')
+            raise exception.EndpointNotFound(endpoint_id=endpoint_id)
        with session.begin():
            session.delete(endpoint_ref)
            session.flush()
--- a/keystone/catalog/core.py
+++ b/keystone/catalog/core.py
@ -19,8 +19,6 @@

 import uuid

-import webob.exc
-
 from keystone import config
 from keystone import exception
 from keystone import identity
@ -131,7 +129,7 @@ class ServiceController(wsgi.Application):
    def get_service(self, context, service_id):
        service_ref = self.catalog_api.get_service(context, service_id)
        if not service_ref:
-            raise webob.exc.HTTPNotFound()
+            raise exception.ServiceNotFound(service_id=service_id)
        return {'OS-KSADM:service': service_ref}

    def delete_service(self, context, service_id):
@ -168,11 +166,8 @@ class EndpointController(wsgi.Application):
        endpoint_ref['id'] = endpoint_id

        service_id = endpoint_ref['service_id']
-        try:
-            service = self.catalog_api.get_service(context, service_id)
-        except exception.ServiceNotFound:
-            msg = 'No service exists with id %s' % service_id
-            raise webob.exc.HTTPBadRequest(msg)
+        if not self.catalog_api.service_exists(context, service_id):
+            raise exception.ServiceNotFound(service_id=service_id)

        new_endpoint_ref = self.catalog_api.create_endpoint(
                                context, endpoint_id, endpoint_ref)
--- a/keystone/common/ldap/core.py
+++ b/keystone/common/ldap/core.py
@ -16,6 +16,7 @@

 import ldap

+from keystone import exception
 from keystone.common import logging
 from keystone.common.ldap import fakeldap

@ -140,14 +141,16 @@ class BaseLdap(object):
        if values['name'] is not None:
            entity = self.get_by_name(values['name'])
            if entity is not None:
-                raise Exception('%s with id %s already exists'
-                                % (self.options_name, values['id']))
+                raise exception.Conflict(type=self.options_name,
+                                         details='Duplicate name, %s.' %
+                                                 values['name'])

        if values['id'] is not None:
            entity = self.get(values['id'])
            if entity is not None:
-                raise Exception('%s with id %s already exists'
-                                % (self.options_name, values['id']))
+                raise exception.Conflict(type=self.options_name,
+                                         details='Duplicate ID, %s.' %
+                                                 values['id'])

    def create(self, values):
        conn = self.get_connection()
--- a/keystone/common/sql/core.py
+++ b/keystone/common/sql/core.py
@ -43,6 +43,7 @@ Column = sql.Column
 String = sql.String
 ForeignKey = sql.ForeignKey
 DateTime = sql.DateTime
+IntegrityError = sql.exc.IntegrityError


 # Special Fields
--- a/keystone/common/wsgi.py
+++ b/keystone/common/wsgi.py
@ -185,6 +185,9 @@ class Application(BaseApplication):
        except exception.Error as e:
            LOG.warning(e)
            return render_exception(e)
+        except Exception as e:
+            logging.exception(e)
+            return render_exception(exception.UnexpectedError(exception=e))

        if result is None:
            return render_response(status=(204, 'No Content'))
--- a/keystone/contrib/ec2/core.py
+++ b/keystone/contrib/ec2/core.py
@ -36,8 +36,6 @@ glance to list images needed to perform the requested task.

 import uuid

-import webob.exc
-
 from keystone import catalog
 from keystone import config
 from keystone import exception
@ -114,12 +112,9 @@ class Ec2Controller(wsgi.Application):
            credentials['host'] = hostname
            signature = signer.generate(credentials)
            if not utils.auth_str_equal(credentials.signature, signature):
-                # TODO(termie): proper exception
-                msg = 'Invalid signature'
-                raise webob.exc.HTTPUnauthorized(explanation=msg)
+                raise exception.Unauthorized(message='Invalid EC2 signature.')
        else:
-            msg = 'Signature not supplied'
-            raise webob.exc.HTTPUnauthorized(explanation=msg)
+            raise exception.Unauthorized(message='EC2 signature not supplied.')

    def authenticate(self, context, credentials=None,
                         ec2Credentials=None):
@ -152,8 +147,7 @@ class Ec2Controller(wsgi.Application):
        creds_ref = self.ec2_api.get_credential(context,
                                                credentials['access'])
        if not creds_ref:
-            msg = 'Access key not found'
-            raise webob.exc.HTTPUnauthorized(explanation=msg)
+            raise exception.Unauthorized(message='EC2 access key not found.')

        self.check_signature(creds_ref, credentials)

@ -263,7 +257,7 @@ class Ec2Controller(wsgi.Application):

        :param context: standard context
        :param user_id: id of user
-        :raises webob.exc.HTTPForbidden: when token is invalid
+        :raises exception.Forbidden: when token is invalid

        """
        try:
@ -273,7 +267,7 @@ class Ec2Controller(wsgi.Application):
            raise exception.Unauthorized()
        token_user_id = token_ref['user'].get('id')
        if not token_user_id == user_id:
-            raise webob.exc.HTTPForbidden()
+            raise exception.Forbidden()

    def _is_admin(self, context):
        """Wrap admin assertion error return statement.
@ -294,9 +288,9 @@ class Ec2Controller(wsgi.Application):
        :param context: standard context
        :param user_id: expected credential owner
        :param credential_id: id of credential object
-        :raises webob.exc.HTTPForbidden: on failure
+        :raises exception.Forbidden: on failure

        """
        cred_ref = self.ec2_api.get_credential(context, credential_id)
        if not user_id == cred_ref['user_id']:
-            raise webob.exc.HTTPForbidden()
+            raise exception.Forbidden()
--- a/keystone/exception.py
+++ b/keystone/exception.py
@ -58,26 +58,66 @@ class Unauthorized(Error):


 class Forbidden(Error):
-    """You are not authorized to perform the requested action: %(action)s"""
+    """You are not authorized to perform the requested action."""
    code = 403
    title = 'Not Authorized'


+class ForbiddenAction(Forbidden):
+    """You are not authorized to perform the requested action: %(action)s"""
+
+
 class NotFound(Error):
    """Could not find: %(target)s"""
    code = 404
    title = 'Not Found'


-class NotImplemented(Error):
-    """The action you have requested has not been implemented."""
-    code = 501
-    action = 'Not Implemented'
+class EndpointNotFound(NotFound):
+    """Could not find endopint: %(endpoint_id)s"""
+
+
+class RoleNotFound(NotFound):
+    """Could not find role: %(role_id)s"""
+
+
+class ServiceNotFound(NotFound):
+    """Could not find service: %(service_id)s"""
+
+
+class TenantNotFound(NotFound):
+    """Could not find tenant: %(tenant_id)s"""


 class TokenNotFound(NotFound):
    """Could not find token: %(token_id)s"""


-class ServiceNotFound(NotFound):
-    """Could not find service: %(service_id)s"""
+class UserNotFound(NotFound):
+    """Could not find user: %(user_id)s"""
+
+
+class Conflict(Error):
+    """Conflict occurred attempting to store %(type)s.
+
+    %(details)s
+
+    """
+    code = 409
+    title = 'Conflict'
+
+
+class NotImplemented(Error):
+    """The action you have requested has not been implemented."""
+    code = 501
+    action = 'Not Implemented'
+
+
+class UnexpectedError(Error):
+    """An unexpected error prevented the server from fulfilling your request.
+
+    %(exception)s
+
+    """
+    code = 500
+    title = 'Internal Server Error'
--- a/keystone/identity/backends/kvs.py
+++ b/keystone/identity/backends/kvs.py
@ -14,6 +14,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.

+from keystone import exception
 from keystone import identity
 from keystone.common import kvs
 from keystone.common import utils
@ -151,9 +152,11 @@ class Identity(kvs.Base, identity.Driver):
    # CRUD
    def create_user(self, user_id, user):
        if self.get_user(user_id):
-            raise Exception('Duplicate id')
+            msg = 'Duplicate ID, %s.' % user_id
+            raise exception.Conflict(type='user', details=msg)
        if self.get_user_by_name(user['name']):
-            raise Exception('Duplicate name')
+            msg = 'Duplicate name, %s.' % user['name']
+            raise exception.Conflict(type='user', details=msg)
        user = _ensure_hashed_password(user)
        self.db.set('user-%s' % user_id, user)
        self.db.set('user_name-%s' % user['name'], user)
@ -166,7 +169,8 @@ class Identity(kvs.Base, identity.Driver):
        if 'name' in user:
            existing = self.db.get('user_name-%s' % user['name'])
            if existing and user_id != existing['id']:
-                raise Exception('Duplicate name')
+                msg = 'Duplicate name, %s.' % user['name']
+                raise exception.Conflict(type='user', details=msg)
        # get the old name and delete it too
        old_user = self.db.get('user-%s' % user_id)
        new_user = old_user.copy()
@ -189,9 +193,11 @@ class Identity(kvs.Base, identity.Driver):

    def create_tenant(self, tenant_id, tenant):
        if self.get_tenant(tenant_id):
-            raise Exception('Duplicate id')
+            msg = 'Duplicate ID, %s.' % tenant_id
+            raise exception.Conflict(type='tenant', details=msg)
        if self.get_tenant_by_name(tenant['name']):
-            raise Exception('Duplicate name')
+            msg = 'Duplicate name, %s.' % tenant['name']
+            raise exception.Conflict(type='tenant', details=msg)
        self.db.set('tenant-%s' % tenant_id, tenant)
        self.db.set('tenant_name-%s' % tenant['name'], tenant)
        return tenant
@ -200,7 +206,8 @@ class Identity(kvs.Base, identity.Driver):
        if 'name' in tenant:
            existing = self.db.get('tenant_name-%s' % tenant['name'])
            if existing and tenant_id != existing['id']:
-                raise Exception('Duplicate name')
+                msg = 'Duplicate name, %s.' % tenant['name']
+                raise exception.Conflict(type='tenant', details=msg)
        # get the old name and delete it too
        old_tenant = self.db.get('tenant-%s' % tenant_id)
        new_tenant = old_tenant.copy()
--- a/keystone/identity/backends/ldap/core.py
+++ b/keystone/identity/backends/ldap/core.py
@ -545,7 +545,7 @@ class RoleApi(common_ldap.BaseLdap, ApiShimMixin):
    def add_user(self, role_id, user_id, tenant_id=None):
        user = self.user_api.get(user_id)
        if user is None:
-            raise exception.NotFound('User %s not found' % (user_id,))
+            raise exception.UserNotFound(user_id=user_id)
        role_dn = self._subrole_id_to_dn(role_id, tenant_id)
        conn = self.get_connection()
        user_dn = self.user_api._id_to_dn(user_id)
--- a/keystone/identity/backends/sql.py
+++ b/keystone/identity/backends/sql.py
@ -15,8 +15,10 @@
 # under the License.

 import copy
+import functools

 from keystone import identity
+from keystone import exception
 from keystone.common import sql
 from keystone.common import utils
 from keystone.common.sql import migration
@ -35,6 +37,19 @@ def _ensure_hashed_password(user_ref):
    return user_ref


+def handle_conflicts(type='object'):
+    """Converts IntegrityError into HTTP 409 Conflict."""
+    def decorator(method):
+        @functools.wraps(method)
+        def wrapper(*args, **kwargs):
+            try:
+                return method(*args, **kwargs)
+            except sql.IntegrityError as e:
+                raise exception.Conflict(type=type, details=str(e))
+        return wrapper
+    return decorator
+
+
 class User(sql.ModelBase, sql.DictBase):
    __tablename__ = 'user'
    id = sql.Column(sql.String(64), primary_key=True)
@ -280,6 +295,7 @@ class Identity(sql.Base, identity.Driver):
            self.create_metadata(user_id, tenant_id, metadata_ref)

    # CRUD
+    @handle_conflicts(type='user')
    def create_user(self, user_id, user):
        user = _ensure_hashed_password(user)
        session = self.get_session()
@ -289,6 +305,7 @@ class Identity(sql.Base, identity.Driver):
            session.flush()
        return user_ref.to_dict()

+    @handle_conflicts(type='user')
    def update_user(self, user_id, user):
        session = self.get_session()
        with session.begin():
@ -311,6 +328,7 @@ class Identity(sql.Base, identity.Driver):
            session.delete(user_ref)
            session.flush()

+    @handle_conflicts(type='tenant')
    def create_tenant(self, tenant_id, tenant):
        session = self.get_session()
        with session.begin():
@ -319,6 +337,7 @@ class Identity(sql.Base, identity.Driver):
            session.flush()
        return tenant_ref.to_dict()

+    @handle_conflicts(type='tenant')
    def update_tenant(self, tenant_id, tenant):
        session = self.get_session()
        with session.begin():
@ -340,6 +359,7 @@ class Identity(sql.Base, identity.Driver):
            session.delete(tenant_ref)
            session.flush()

+    @handle_conflicts(type='metadata')
    def create_metadata(self, user_id, tenant_id, metadata):
        session = self.get_session()
        with session.begin():
@ -349,6 +369,7 @@ class Identity(sql.Base, identity.Driver):
            session.flush()
        return metadata

+    @handle_conflicts(type='metadata')
    def update_metadata(self, user_id, tenant_id, metadata):
        session = self.get_session()
        with session.begin():
@ -367,6 +388,7 @@ class Identity(sql.Base, identity.Driver):
        self.db.delete('metadata-%s-%s' % (tenant_id, user_id))
        return None

+    @handle_conflicts(type='role')
    def create_role(self, role_id, role):
        session = self.get_session()
        with session.begin():
@ -374,6 +396,7 @@ class Identity(sql.Base, identity.Driver):
            session.flush()
        return role

+    @handle_conflicts(type='role')
    def update_role(self, role_id, role):
        session = self.get_session()
        with session.begin():
--- a/keystone/identity/core.py
+++ b/keystone/identity/core.py
@ -20,8 +20,6 @@ import uuid
 import urllib
 import urlparse

-import webob.exc
-
 from keystone import config
 from keystone import exception
 from keystone import policy
@ -287,7 +285,7 @@ class TenantController(wsgi.Application):
        self.assert_admin(context)
        tenant = self.identity_api.get_tenant(context, tenant_id)
        if not tenant:
-            return webob.exc.HTTPNotFound()
+            raise exception.TenantNotFound(tenant_id=tenant_id)
        return {'tenant': tenant}

    # CRUD Extension
@ -329,16 +327,17 @@ class TenantController(wsgi.Application):
                    break
            else:
                msg = 'Marker could not be found'
-                raise webob.exc.HTTPBadRequest(explanation=msg)
+                raise exception.ValidationError(message=msg)

        limit = kwargs.get('limit')
        if limit is not None:
            try:
                limit = int(limit)
-                assert limit >= 0
+                if limit < 0:
+                    raise AssertionError()
            except (ValueError, AssertionError):
                msg = 'Invalid limit value'
-                raise webob.exc.HTTPBadRequest(explanation=msg)
+                raise exception.ValidationError(message=msg)

        tenant_refs = tenant_refs[page_idx:limit]

@ -361,7 +360,7 @@ class UserController(wsgi.Application):
        self.assert_admin(context)
        user_ref = self.identity_api.get_user(context, user_id)
        if not user_ref:
-            raise webob.exc.HTTPNotFound()
+            raise exception.UserNotFound(user_id=user_id)
        return {'user': user_ref}

    def get_users(self, context):
@ -425,7 +424,8 @@ class RoleController(wsgi.Application):

        """
        if tenant_id is None:
-            raise Exception('User roles not supported: tenant_id required')
+            raise exception.NotImplemented(message='User roles not supported: '
+                                                   'tenant_id required')
        roles = self.identity_api.get_roles_for_user_and_tenant(
                context, user_id, tenant_id)
        return {'roles': [self.identity_api.get_role(context, x)
@ -436,7 +436,7 @@ class RoleController(wsgi.Application):
        self.assert_admin(context)
        role_ref = self.identity_api.get_role(context, role_id)
        if not role_ref:
-            raise webob.exc.HTTPNotFound()
+            raise exception.RoleNotFound(role_id=role_id)
        return {'role': role_ref}

    def create_role(self, context, role):
@ -466,7 +466,8 @@ class RoleController(wsgi.Application):
        """
        self.assert_admin(context)
        if tenant_id is None:
-            raise Exception('User roles not supported: tenant_id required')
+            raise exception.NotImplemented(message='User roles not supported: '
+                                                   'tenant_id required')

        # This still has the weird legacy semantics that adding a role to
        # a user also adds them to a tenant
@ -485,7 +486,8 @@ class RoleController(wsgi.Application):
        """
        self.assert_admin(context)
        if tenant_id is None:
-            raise Exception('User roles not supported: tenant_id required')
+            raise exception.NotImplemented(message='User roles not supported: '
+                                                   'tenant_id required')

        # This still has the weird legacy semantics that adding a role to
        # a user also adds them to a tenant
--- a/keystone/policy/backends/rules.py
+++ b/keystone/policy/backends/rules.py
@ -97,7 +97,7 @@ def enforce(credentials, action, target):
    try:
        common_policy.enforce(match_list, target, credentials)
    except common_policy.NotAuthorized:
-        raise exception.Forbidden(action=action)
+        raise exception.ForbiddenAction(action=action)


 class Policy(policy.Driver):
--- a/keystone/service.py
+++ b/keystone/service.py
@ -17,8 +17,6 @@
 import uuid

 import routes
-import webob.dec
-import webob.exc

 from keystone import catalog
 from keystone import exception
@ -277,7 +275,7 @@ class TokenController(wsgi.Application):

                # If the user is disabled don't allow them to authenticate
                if not user_ref.get('enabled', True):
-                    raise webob.exc.HTTPForbidden('User has been disabled')
+                    raise exception.Forbidden(message='User has been disabled')
            except AssertionError as e:
                raise exception.Unauthorized(e.message)

--- a/tests/test_exception.py
+++ b/tests/test_exception.py
@ -54,9 +54,9 @@ class ExceptionTestCase(test.TestCase):
        e = exception.Unauthorized()
        self.assertValidJsonRendering(e)

-    def test_forbidden(self):
+    def test_forbidden_action(self):
        action = uuid.uuid4().hex
-        e = exception.Forbidden(action=action)
+        e = exception.ForbiddenAction(action=action)
        self.assertValidJsonRendering(e)
        self.assertIn(action, str(e))