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

@@ -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

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)
 

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):

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)

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:

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(

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')

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)