From b1336b0a3921621741ff8ba2adbc44113357e175 Mon Sep 17 00:00:00 2001 From: Dolph Mathews Date: Fri, 23 Mar 2012 10:46:16 -0500 Subject: [PATCH] Validate object refs (return 404 instead of 500) Combined fix for bug 963056: user-crud 404 service-crud 404 ec2-credential-crud 404 user-role-crud 404 endpoint-crud 404 Change-Id: I7762aaaae9817ea7426039e4700e16b59e18cba1 --- keystone/catalog/core.py | 5 +- keystone/contrib/ec2/core.py | 2 + keystone/exception.py | 2 +- keystone/identity/backends/kvs.py | 4 + keystone/identity/backends/sql.py | 4 + keystone/identity/core.py | 26 ++++- tests/test_keystoneclient.py | 175 ++++++++++++++++++++++++++++++ tests/test_keystoneclient_sql.py | 7 ++ 8 files changed, 222 insertions(+), 3 deletions(-) diff --git a/keystone/catalog/core.py b/keystone/catalog/core.py index ef35921fdc..98b5ae9d79 100644 --- a/keystone/catalog/core.py +++ b/keystone/catalog/core.py @@ -133,7 +133,10 @@ def get_service(self, context, service_id): return {'OS-KSADM:service': service_ref} def delete_service(self, context, service_id): - service_ref = self.catalog_api.delete_service(context, service_id) + service_ref = self.catalog_api.get_service(context, service_id) + if not service_ref: + raise exception.ServiceNotFound(service_id=service_id) + self.catalog_api.delete_service(context, service_id) def create_service(self, context, OS_KSADM_service): service_id = uuid.uuid4().hex diff --git a/keystone/contrib/ec2/core.py b/keystone/contrib/ec2/core.py index 80604c592a..4449b6bb85 100644 --- a/keystone/contrib/ec2/core.py +++ b/keystone/contrib/ec2/core.py @@ -239,6 +239,7 @@ def get_credential(self, context, user_id, credential_id): """ if not self._is_admin(context): self._assert_identity(context, user_id) + self._assert_valid_user_id(context, user_id) creds = self._get_credentials(context, credential_id) return {'credential': creds} @@ -256,6 +257,7 @@ def delete_credential(self, context, user_id, credential_id): self._assert_identity(context, user_id) self._assert_owner(context, user_id, credential_id) + self._assert_valid_user_id(context, user_id) self._get_credentials(context, credential_id) return self.ec2_api.delete_credential(context, credential_id) diff --git a/keystone/exception.py b/keystone/exception.py index c76201b081..f1651bf008 100644 --- a/keystone/exception.py +++ b/keystone/exception.py @@ -74,7 +74,7 @@ class NotFound(Error): class EndpointNotFound(NotFound): - """Could not find endopint: %(endpoint_id)s""" + """Could not find endpoint: %(endpoint_id)s""" class RoleNotFound(NotFound): diff --git a/keystone/identity/backends/kvs.py b/keystone/identity/backends/kvs.py index 95286a9f98..55bd8f0814 100644 --- a/keystone/identity/backends/kvs.py +++ b/keystone/identity/backends/kvs.py @@ -144,6 +144,10 @@ def remove_role_from_user_and_tenant(self, user_id, tenant_id, role_id): if not metadata_ref: metadata_ref = {} roles = set(metadata_ref.get('roles', [])) + if role_id not in roles: + msg = 'Cannot remove role that has not been granted, %s' % role_id + raise exception.RoleNotFound(message=msg) + roles.remove(role_id) metadata_ref['roles'] = list(roles) self.update_metadata(user_id, tenant_id, metadata_ref) diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index 00dedcba0f..443ddd3bee 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -286,6 +286,10 @@ def remove_role_from_user_and_tenant(self, user_id, tenant_id, role_id): is_new = True metadata_ref = {} roles = set(metadata_ref.get('roles', [])) + if role_id not in roles: + msg = 'Cannot remove role that has not been granted, %s' % role_id + raise exception.RoleNotFound(message=msg) + roles.remove(role_id) metadata_ref['roles'] = list(roles) if not is_new: diff --git a/keystone/identity/core.py b/keystone/identity/core.py index 07c4878755..ee22526455 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -320,6 +320,9 @@ def delete_tenant(self, context, tenant_id, **kw): def get_tenant_users(self, context, tenant_id, **kw): self.assert_admin(context) + if self.identity_api.get_tenant(context, tenant_id) is None: + raise exception.TenantNotFound(tenant_id=tenant_id) + user_refs = self.identity_api.get_tenant_users(context, tenant_id) return {'users': user_refs} @@ -383,6 +386,9 @@ def create_user(self, context, user): user = self._normalize_dict(user) self.assert_admin(context) tenant_id = user.get('tenantId', None) + if (tenant_id is not None + and self.identity_api.get_tenant(context, tenant_id) is None): + raise exception.TenantNotFound(tenant_id=tenant_id) user_id = uuid.uuid4().hex user_ref = user.copy() user_ref['id'] = user_id @@ -395,11 +401,17 @@ def create_user(self, context, user): def update_user(self, context, user_id, user): # NOTE(termie): this is really more of a patch than a put self.assert_admin(context) + if self.identity_api.get_user(context, user_id) is None: + raise exception.UserNotFound(user_id=user_id) + user_ref = self.identity_api.update_user(context, user_id, user) return {'user': user_ref} def delete_user(self, context, user_id): self.assert_admin(context) + if self.identity_api.get_user(context, user_id) is None: + raise exception.UserNotFound(user_id=user_id) + self.identity_api.delete_user(context, user_id) def set_user_enabled(self, context, user_id, user): @@ -485,6 +497,12 @@ def add_role_to_user(self, context, user_id, role_id, tenant_id=None): if tenant_id is None: raise exception.NotImplemented(message='User roles not supported: ' 'tenant_id required') + if self.identity_api.get_user(context, user_id) is None: + raise exception.UserNotFound(user_id=user_id) + if self.identity_api.get_tenant(context, tenant_id) is None: + raise exception.TenantNotFound(tenant_id=tenant_id) + if self.identity_api.get_role(context, role_id) is None: + raise exception.RoleNotFound(role_id=role_id) # This still has the weird legacy semantics that adding a role to # a user also adds them to a tenant @@ -505,9 +523,15 @@ def remove_role_from_user(self, context, user_id, role_id, tenant_id=None): if tenant_id is None: raise exception.NotImplemented(message='User roles not supported: ' 'tenant_id required') + if self.identity_api.get_user(context, user_id) is None: + raise exception.UserNotFound(user_id=user_id) + if self.identity_api.get_tenant(context, tenant_id) is None: + raise exception.TenantNotFound(tenant_id=tenant_id) + if self.identity_api.get_role(context, role_id) is None: + raise exception.RoleNotFound(role_id=role_id) # This still has the weird legacy semantics that adding a role to - # a user also adds them to a tenant + # a user also adds them to a tenant, so we must follow up on that self.identity_api.remove_role_from_user_and_tenant( context, user_id, tenant_id, role_id) roles = self.identity_api.get_roles_for_user_and_tenant( diff --git a/tests/test_keystoneclient.py b/tests/test_keystoneclient.py index 706dc8a6e5..810d233f10 100644 --- a/tests/test_keystoneclient.py +++ b/tests/test_keystoneclient.py @@ -336,6 +336,61 @@ def test_user_create_update_delete(self): tenant_id='bar') self.assertEquals(user2.name, test_username) + def test_user_create_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.create, + name=uuid.uuid4().hex, + password=uuid.uuid4().hex, + email=uuid.uuid4().hex, + tenant_id=uuid.uuid4().hex) + + def test_user_get_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.get, + user=uuid.uuid4().hex) + + def test_user_list_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.list, + tenant_id=uuid.uuid4().hex) + + def test_user_update_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.update, + user=uuid.uuid4().hex) + + def test_user_update_tenant_404(self): + raise nose.exc.SkipTest('N/A') + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.update, + user=self.user_foo['id'], + tenant_id=uuid.uuid4().hex) + + def test_user_update_password_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.update_password, + user=uuid.uuid4().hex, + password=uuid.uuid4().hex) + + def test_user_delete_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.users.delete, + user=uuid.uuid4().hex) + def test_user_list(self): client = self.get_client(admin=True) users = client.users.list() @@ -426,6 +481,41 @@ def test_ec2_credential_crud(self): creds = client.ec2.list(user_id=self.user_foo['id']) self.assertEquals(creds, []) + def test_ec2_credentials_create_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client() + self.assertRaises(client_exceptions.NotFound, + client.ec2.create, + user_id=uuid.uuid4().hex, + tenant_id=self.tenant_bar['id']) + self.assertRaises(client_exceptions.NotFound, + client.ec2.create, + user_id=self.user_foo['id'], + tenant_id=uuid.uuid4().hex) + + def test_ec2_credentials_delete_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client() + self.assertRaises(client_exceptions.NotFound, + client.ec2.delete, + user_id=uuid.uuid4().hex, + access=uuid.uuid4().hex) + + def test_ec2_credentials_get_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client() + self.assertRaises(client_exceptions.NotFound, + client.ec2.get, + user_id=uuid.uuid4().hex, + access=uuid.uuid4().hex) + + def test_ec2_credentials_list_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client() + self.assertRaises(client_exceptions.NotFound, + client.ec2.list, + user_id=uuid.uuid4().hex) + def test_ec2_credentials_list_user_forbidden(self): from keystoneclient import exceptions as client_exceptions @@ -486,6 +576,39 @@ def test_service_list(self): # TODO(devcamcar): This assert should be more specific. self.assertTrue(len(services) > 0) + def test_service_delete_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.services.delete, + id=uuid.uuid4().hex) + + def test_service_get_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.services.get, + id=uuid.uuid4().hex) + + def test_endpoint_create_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.endpoints.create, + region=uuid.uuid4().hex, + service_id=uuid.uuid4().hex, + publicurl=uuid.uuid4().hex, + adminurl=uuid.uuid4().hex, + internalurl=uuid.uuid4().hex) + + def test_endpoint_delete_404(self): + # the catalog backend is expected to return Not Implemented + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.HTTPNotImplemented, + client.endpoints.delete, + id=uuid.uuid4().hex) + def test_admin_requires_adminness(self): from keystoneclient import exceptions as client_exceptions # FIXME(ja): this should be Unauthorized @@ -557,6 +680,49 @@ def test_tenant_add_and_remove_user(self): user_refs = client.tenants.list_users(tenant=self.tenant_baz['id']) self.assert_(self.user_foo['id'] not in [x.id for x in user_refs]) + def test_user_role_add_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.roles.add_user_role, + tenant=uuid.uuid4().hex, + user=self.user_foo['id'], + role=self.role_useless['id']) + self.assertRaises(client_exceptions.NotFound, + client.roles.add_user_role, + tenant=self.tenant_baz['id'], + user=uuid.uuid4().hex, + role=self.role_useless['id']) + self.assertRaises(client_exceptions.NotFound, + client.roles.add_user_role, + tenant=self.tenant_baz['id'], + user=self.user_foo['id'], + role=uuid.uuid4().hex) + + def test_user_role_remove_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.roles.remove_user_role, + tenant=uuid.uuid4().hex, + user=self.user_foo['id'], + role=self.role_useless['id']) + self.assertRaises(client_exceptions.NotFound, + client.roles.remove_user_role, + tenant=self.tenant_baz['id'], + user=uuid.uuid4().hex, + role=self.role_useless['id']) + self.assertRaises(client_exceptions.NotFound, + client.roles.remove_user_role, + tenant=self.tenant_baz['id'], + user=self.user_foo['id'], + role=uuid.uuid4().hex) + self.assertRaises(client_exceptions.NotFound, + client.roles.remove_user_role, + tenant=self.tenant_baz['id'], + user=self.user_foo['id'], + role=self.role_useless['id']) + def test_tenant_list_marker(self): client = self.get_client() @@ -701,3 +867,12 @@ def test_user_create_update_delete(self): client.users.delete(user.id) self.assertRaises(client_exceptions.NotFound, client.users.get, user.id) + + def test_user_update_404(self): + raise nose.exc.SkipTest('N/A') + + def test_endpoint_create_404(self): + raise nose.exc.SkipTest('N/A') + + def test_endpoint_delete_404(self): + raise nose.exc.SkipTest('N/A') diff --git a/tests/test_keystoneclient_sql.py b/tests/test_keystoneclient_sql.py index 641ee7703e..9a13712a8b 100644 --- a/tests/test_keystoneclient_sql.py +++ b/tests/test_keystoneclient_sql.py @@ -72,3 +72,10 @@ def test_endpoint_crud(self): client.endpoints.delete(id=endpoint.id) self.assertRaises(client_exceptions.NotFound, client.endpoints.delete, id=endpoint.id) + + def test_endpoint_delete_404(self): + from keystoneclient import exceptions as client_exceptions + client = self.get_client(admin=True) + self.assertRaises(client_exceptions.NotFound, + client.endpoints.delete, + id=uuid.uuid4().hex)