user-role-crud 404 (bug 963056)

user-role-add
user-role-remove

Change-Id: I1b3cd019d0d110b01ed175822cdd6c9ddb486412

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

@@ -497,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
@@ -517,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

@@ -670,6 +670,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()