Fix 500 error when create trust with invalid role key

When create trust with invalid role key(neither 'id' nor
'name'), Keystone should raise 400 BadRequest, instead of
500 Internal Error.

This patch removed the redundant loops in
_normalize_role_list as well.

Change-Id: I62bd201c1dda7b573e2ee8b97322c1f25275892c
Closes-bug: #1734244

keystone/tests/unit/test_v3_trust.py

@@ -19,7 +19,6 @@ import keystone.conf
 from keystone import exception
 from keystone.tests import unit
 from keystone.tests.unit import test_v3
-from keystone.tests.unit import utils as test_utils
 
 CONF = keystone.conf.CONF
 
@@ -297,8 +296,6 @@ class TestTrustOperations(test_v3.RestfulTestCase):
         self.post('/OS-TRUST/trusts', body={'trust': ref},
                   expected_status=http_client.NOT_FOUND)
 
-    @test_utils.wip('Waiting on validation to be added from fixing bug'
-                    '1734244')
     def test_create_trust_with_extra_attributes_fails(self):
         ref = unit.new_trust_ref(trustor_user_id=self.user_id,
                                  trustee_user_id=self.trustee_user_id,
@@ -306,8 +303,6 @@ class TestTrustOperations(test_v3.RestfulTestCase):
                                  role_ids=[self.role_id])
         ref['roles'].append({'fake_key': 'fake_value'})
 
-        # Should return 400 Bad Request because `fake_key` is an extra
-        # attribute that keystone doesn't associate with trusts.
         self.post('/OS-TRUST/trusts', body={'trust': ref},
                   expected_status=http_client.BAD_REQUEST)
 

keystone/tests/unit/test_validation.py

@@ -1483,7 +1483,9 @@ class EndpointGroupValidationTestCase(unit.BaseTestCase):
 class TrustValidationTestCase(unit.BaseTestCase):
     """Test for V3 Trust API validation."""
 
-    _valid_roles = ['member', uuid.uuid4().hex, str(uuid.uuid4())]
+    _valid_roles = [{'name': 'member'},
+                    {'id': uuid.uuid4().hex},
+                    {'id': str(uuid.uuid4())}]
     _invalid_roles = [False, True, 123, None]
 
     def setUp(self):
@@ -1505,7 +1507,8 @@ class TrustValidationTestCase(unit.BaseTestCase):
                                'trustee_user_id': uuid.uuid4().hex,
                                'impersonation': False,
                                'project_id': uuid.uuid4().hex,
-                               'roles': [uuid.uuid4().hex, uuid.uuid4().hex],
+                               'roles': [{'id': uuid.uuid4().hex},
+                                         {'id': uuid.uuid4().hex}],
                                'expires_at': 'some timestamp',
                                'remaining_uses': 2}
         self.create_trust_validator.validate(request_to_validate)
@@ -1540,7 +1543,8 @@ class TrustValidationTestCase(unit.BaseTestCase):
                                'trustee_user_id': uuid.uuid4().hex,
                                'impersonation': False,
                                'project_id': uuid.uuid4().hex,
-                               'roles': [uuid.uuid4().hex, uuid.uuid4().hex],
+                               'roles': [{'id': uuid.uuid4().hex},
+                                         {'id': uuid.uuid4().hex}],
                                'expires_at': 'some timestamp',
                                'remaining_uses': 2,
                                'extra': 'something extra!'}

keystone/trust/controllers.py

@@ -79,23 +79,23 @@ class TrustV3(controller.V3Controller):
             'previous': None}
 
     def _normalize_role_list(self, trust_roles):
-        roles = [{'id': role['id']} for role in trust_roles if 'id' in role]
-        names = [role['name'] for role in trust_roles if 'id' not in role]
-        if len(names):
-            # Long way
-            for name in names:
+        roles = []
+        for role in trust_roles:
+            if role.get('id'):
+                roles.append({'id': role['id']})
+            else:
                 hints = driver_hints.Hints()
-                hints.add_filter("name", name, case_sensitive=True)
+                hints.add_filter("name", role['name'], case_sensitive=True)
                 found_roles = self.role_api.list_roles(hints)
                 if not found_roles:
                     raise exception.RoleNotFound(
-                        _("Role %s is not defined") % name
+                        _("Role %s is not defined") % role['name']
                     )
                 elif len(found_roles) == 1:
                     roles.append({'id': found_roles[0]['id']})
                 else:
                     raise exception.AmbiguityError(resource='role',
-                                                   name=name)
+                                                   name=role['name'])
         return roles
 
     def _find_redelegated_trust(self, request):

keystone/trust/schema.py

@@ -13,6 +13,19 @@
 from keystone.common import validation
 from keystone.common.validation import parameter_types
 
+_role_properties = {
+    'type': 'array',
+    'items': {
+        'type': 'object',
+        'properties': {
+            'id': parameter_types.id_string,
+            'name': parameter_types.id_string
+        },
+        'minProperties': 1,
+        'maxProperties': 1,
+        'additionalProperties': False
+    }
+}
 
 _trust_properties = {
     # NOTE(lbragstad): These are set as external_id_string because they have
@@ -36,9 +49,7 @@ _trust_properties = {
         'type': ['integer', 'null'],
         'minimum': 0
     },
-    # TODO(lbragstad): Need to find a better way to do this. We should be
-    # checking that a role is a list of IDs and/or names.
-    'roles': validation.add_array_type(parameter_types.id_string)
+    'roles': _role_properties
 }
 
 trust_create = {

releasenotes/notes/bug-1734244-1b4ea83baa72566d.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    [`bug 1734244 <https://bugs.launchpad.net/keystone/+bug/1734244>`_]
+    Return a 400 status code instead of a 500 when creating a trust with
+    extra attributes in the roles parameter.