Add schema check for authorize request token

This patch add the schema check for authorize
request token API. It'll avoiding some 500 error
caused by invalid input format and it will raise
400 error correctly.

This patch also add role name support for
authorize request token

Closes-bug: #1736875
Change-Id: I9d113692702e7aaa0127ffa9405a17908c0c6ff7

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)

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
+    }
+}

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

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

releasenotes/notes/bug-1736875-c790f568c5f4d671.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - >
+    [`bug 1736875 <https://bugs.launchpad.net/keystone/+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.