From f8e79ab50775bcf5964c7547297577d0a3b82519 Mon Sep 17 00:00:00 2001 From: wangxiyuan Date: Fri, 24 Nov 2017 12:40:20 +0800 Subject: [PATCH] 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 | 5 ----- keystone/tests/unit/test_validation.py | 10 +++++++--- keystone/trust/controllers.py | 16 ++++++++-------- keystone/trust/schema.py | 17 ++++++++++++++--- .../notes/bug-1734244-1b4ea83baa72566d.yaml | 6 ++++++ 5 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/bug-1734244-1b4ea83baa72566d.yaml diff --git a/keystone/tests/unit/test_v3_trust.py b/keystone/tests/unit/test_v3_trust.py index f5d1ebe333..0a363654ad 100644 --- a/keystone/tests/unit/test_v3_trust.py +++ b/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) diff --git a/keystone/tests/unit/test_validation.py b/keystone/tests/unit/test_validation.py index 56c980cea9..28d2e853a3 100644 --- a/keystone/tests/unit/test_validation.py +++ b/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!'} diff --git a/keystone/trust/controllers.py b/keystone/trust/controllers.py index ecd5cbb8e0..dac848c1f2 100644 --- a/keystone/trust/controllers.py +++ b/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): diff --git a/keystone/trust/schema.py b/keystone/trust/schema.py index 673b786b26..b8b941c058 100644 --- a/keystone/trust/schema.py +++ b/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 = { diff --git a/releasenotes/notes/bug-1734244-1b4ea83baa72566d.yaml b/releasenotes/notes/bug-1734244-1b4ea83baa72566d.yaml new file mode 100644 index 0000000000..12d8136bdb --- /dev/null +++ b/releasenotes/notes/bug-1734244-1b4ea83baa72566d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + [`bug 1734244 `_] + Return a 400 status code instead of a 500 when creating a trust with + extra attributes in the roles parameter.