Skip to content
Browse files

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
  • Loading branch information...
1 parent 3a70a2f commit 009d661a7e06ad72ab39b93433839bf567755ece @dolph dolph committed
View
2 keystone/catalog/backends/sql.py
@@ -125,7 +125,7 @@ def delete_endpoint(self, endpoint_id):
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()
View
11 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 @@ def get_services(self, context):
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 @@ def create_endpoint(self, context, endpoint):
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)
View
11 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 @@ def affirm_unique(self, values):
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()
View
1 keystone/common/sql/core.py
@@ -43,6 +43,7 @@
String = sql.String
ForeignKey = sql.ForeignKey
DateTime = sql.DateTime
+IntegrityError = sql.exc.IntegrityError
# Special Fields
View
3 keystone/common/wsgi.py
@@ -185,6 +185,9 @@ def __call__(self, req):
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'))
View
20 keystone/contrib/ec2/core.py
@@ -36,8 +36,6 @@
import uuid
-import webob.exc
-
from keystone import catalog
from keystone import config
from keystone import exception
@@ -114,12 +112,9 @@ def check_signature(self, creds_ref, credentials):
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 @@ def authenticate(self, context, credentials=None,
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 @@ def _assert_identity(self, context, user_id):
: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 @@ def _assert_identity(self, context, user_id):
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 @@ def _assert_owner(self, context, user_id, credential_id):
: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()
View
50 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 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 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 TokenNotFound(NotFound):
- """Could not find token: %(token_id)s"""
+class UnexpectedError(Error):
+ """An unexpected error prevented the server from fulfilling your request.
+ %(exception)s
-class ServiceNotFound(NotFound):
- """Could not find service: %(service_id)s"""
+ """
+ code = 500
+ title = 'Internal Server Error'
View
19 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 @@ def remove_role_from_user_and_tenant(self, user_id, tenant_id, role_id):
# 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 @@ def update_user(self, user_id, user):
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 @@ def delete_user(self, user_id):
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 @@ def update_tenant(self, tenant_id, tenant):
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()
View
2 keystone/identity/backends/ldap/core.py
@@ -545,7 +545,7 @@ def get_by_name(self, name, filter=None):
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)
View
23 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 @@ def remove_role_from_user_and_tenant(self, user_id, tenant_id, role_id):
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 @@ def create_user(self, user_id, user):
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 @@ def delete_user(self, user_id):
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 @@ def create_tenant(self, tenant_id, tenant):
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 @@ def delete_tenant(self, tenant_id):
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 @@ def create_metadata(self, user_id, tenant_id, metadata):
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 @@ def delete_metadata(self, user_id, tenant_id):
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 @@ def create_role(self, role_id, role):
session.flush()
return role
+ @handle_conflicts(type='role')
def update_role(self, role_id, role):
session = self.get_session()
with session.begin():
View
24 keystone/identity/core.py
@@ -20,8 +20,6 @@
import urllib
import urlparse
-import webob.exc
-
from keystone import config
from keystone import exception
from keystone import policy
@@ -287,7 +285,7 @@ def get_tenant(self, context, tenant_id):
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 @@ def _format_tenant_list(self, tenant_refs, **kwargs):
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 @@ def get_user(self, context, user_id):
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 @@ def get_user_roles(self, context, user_id, tenant_id=None):
"""
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 @@ def get_role(self, context, role_id):
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 @@ def add_role_to_user(self, context, user_id, role_id, tenant_id=None):
"""
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 @@ def remove_role_from_user(self, context, user_id, role_id, tenant_id=None):
"""
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
View
2 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):
View
4 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 @@ def authenticate(self, context, auth=None):
# 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)
View
4 tests/test_exception.py
@@ -54,9 +54,9 @@ def test_unauthorized(self):
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))

0 comments on commit 009d661

Please sign in to comment.
Something went wrong with that request. Please try again.