diff --git a/keystone/oauth1/controllers.py b/keystone/oauth1/controllers.py index bc363697b1..47bda86a1a 100644 --- a/keystone/oauth1/controllers.py +++ b/keystone/oauth1/controllers.py @@ -370,6 +370,16 @@ class OAuthControllerV3(controller.V3Controller): return response + def _normalize_role_list(self, authorize_roles): + roles = set() + for role in authorize_roles: + if role.get('id'): + roles.add(role['id']) + else: + roles.add(PROVIDERS.role_api.get_unique_role_by_name( + role['name'])['id']) + return roles + @controller.protected() def authorize_request_token(self, request, request_token_id, roles): """An authenticated user is going to authorize a request token. @@ -379,6 +389,7 @@ class OAuthControllerV3(controller.V3Controller): there is not another easy way to make sure the user knows which roles are being requested before authorizing. """ + validation.lazy_validate(schema.request_token_authorize, roles) if request.context.is_delegated_auth: raise exception.Forbidden( _('Cannot authorize a request token' @@ -394,10 +405,7 @@ class OAuthControllerV3(controller.V3Controller): if now > expires: raise exception.Unauthorized(_('Request token is expired')) - # put the roles in a set for easy comparison - authed_roles = set() - for role in roles: - authed_roles.add(role['id']) + authed_roles = self._normalize_role_list(roles) # verify the authorizing user has the roles user_token = authorization.get_token_ref(request.context_dict) diff --git a/keystone/oauth1/schema.py b/keystone/oauth1/schema.py index 51c11afeeb..1b80809fd1 100644 --- a/keystone/oauth1/schema.py +++ b/keystone/oauth1/schema.py @@ -32,3 +32,17 @@ consumer_update = { 'minProperties': 1, 'additionalProperties': True } + +request_token_authorize = { + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': { + 'id': parameter_types.id_string, + 'name': parameter_types.name, + }, + 'minProperties': 1, + 'maxProperties': 1, + 'additionalProperties': False + } +} diff --git a/keystone/tests/unit/test_v3_oauth1.py b/keystone/tests/unit/test_v3_oauth1.py index d9d85c3a86..cd8b7255a3 100644 --- a/keystone/tests/unit/test_v3_oauth1.py +++ b/keystone/tests/unit/test_v3_oauth1.py @@ -35,7 +35,6 @@ from keystone.tests.unit.common import test_notifications from keystone.tests.unit import ksfixtures from keystone.tests.unit.ksfixtures import temporaryfile from keystone.tests.unit import test_v3 -from keystone.tests.unit import utils as test_utils CONF = keystone.conf.CONF @@ -720,8 +719,6 @@ class MaliciousOAuth1Tests(OAuth1Tests): body = {'roles': [{'id': self.role_id}]} self.put(url, body=body, expected_status=http_client.NOT_FOUND) - @test_utils.wip('Waiting on validation to be added from fixing bug ' - '1736875') def test_bad_request_body_when_authorize(self): consumer = self._create_single_consumer() consumer_id = consumer['id'] @@ -872,7 +869,7 @@ class MaliciousOAuth1Tests(OAuth1Tests): self.assertIn('Provided consumer key', resp_data.get('error', {}).get('message')) - def test_bad_authorizing_roles(self): + def test_bad_authorizing_roles_id(self): consumer = self._create_single_consumer() consumer_id = consumer['id'] consumer_secret = consumer['secret'] @@ -892,6 +889,24 @@ class MaliciousOAuth1Tests(OAuth1Tests): self.admin_request(path=url, method='PUT', body=body, expected_status=http_client.NOT_FOUND) + def test_bad_authorizing_roles_name(self): + consumer = self._create_single_consumer() + consumer_id = consumer['id'] + consumer_secret = consumer['secret'] + consumer = {'key': consumer_id, 'secret': consumer_secret} + + url, headers = self._create_request_token(consumer, self.project_id) + content = self.post( + url, headers=headers, + response_content_type='application/x-www-form-urlencoded') + credentials = _urllib_parse_qs_text_keys(content.result) + request_key = credentials['oauth_token'][0] + + url = self._authorize_request_token(request_key) + body = {'roles': [{'name': 'fake_name'}]} + self.admin_request(path=url, method='PUT', + body=body, expected_status=http_client.NOT_FOUND) + def test_no_authorizing_user_id(self): consumer = self._create_single_consumer() consumer_id = consumer['id'] diff --git a/keystone/tests/unit/test_validation.py b/keystone/tests/unit/test_validation.py index b325012b73..63df039789 100644 --- a/keystone/tests/unit/test_validation.py +++ b/keystone/tests/unit/test_validation.py @@ -2319,8 +2319,11 @@ class OAuth1ValidationTestCase(unit.BaseTestCase): create = oauth1_schema.consumer_create update = oauth1_schema.consumer_update + authorize = oauth1_schema.request_token_authorize self.create_consumer_validator = validators.SchemaValidator(create) self.update_consumer_validator = validators.SchemaValidator(update) + self.authorize_request_token_validator = validators.SchemaValidator( + authorize) def test_validate_consumer_request_succeeds(self): """Test that we validate a consumer request successfully.""" @@ -2363,6 +2366,53 @@ class OAuth1ValidationTestCase(unit.BaseTestCase): self.create_consumer_validator.validate(request_to_validate) self.update_consumer_validator.validate(request_to_validate) + def test_validate_authorize_request_token(self): + request_to_validate = [ + { + "id": "711aa6371a6343a9a43e8a310fbe4a6f" + }, + { + "name": "test_role" + } + ] + + self.authorize_request_token_validator.validate(request_to_validate) + + def test_validate_authorize_request_token_with_additional_properties(self): + request_to_validate = [ + { + "id": "711aa6371a6343a9a43e8a310fbe4a6f", + "fake_key": "fake_value" + } + ] + + self.assertRaises(exception.SchemaValidationError, + self.authorize_request_token_validator.validate, + request_to_validate) + + def test_validate_authorize_request_token_with_id_and_name(self): + request_to_validate = [ + { + "id": "711aa6371a6343a9a43e8a310fbe4a6f", + "name": "admin" + } + ] + + self.assertRaises(exception.SchemaValidationError, + self.authorize_request_token_validator.validate, + request_to_validate) + + def test_validate_authorize_request_token_with_non_id_or_name(self): + request_to_validate = [ + { + "fake_key": "fake_value" + } + ] + + self.assertRaises(exception.SchemaValidationError, + self.authorize_request_token_validator.validate, + request_to_validate) + class PasswordValidationTestCase(unit.TestCase): def setUp(self): diff --git a/releasenotes/notes/bug-1736875-c790f568c5f4d671.yaml b/releasenotes/notes/bug-1736875-c790f568c5f4d671.yaml new file mode 100644 index 0000000000..3deec73c01 --- /dev/null +++ b/releasenotes/notes/bug-1736875-c790f568c5f4d671.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - > + [`bug 1736875 `_] + Add schema check to return a 400 status code instead of a 500 when + authorize a request token with non-id attributes in the `roles` parameter. +other: + - > + Keystone now supports authorizing a request token by providing a role name. + A `role` in the `roles` parameter can include either a role name or role + id, but not both.