From 6199a9a17e07008924969c3a4b52492d73d621e7 Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Fri, 7 Jul 2017 21:58:05 +0300 Subject: [PATCH 1/4] Fix scope handling of token endpoint The token endpoint handled the scope parameter incorrectly for all of the three handled grant types: 1. For "authorization_code" grant type the scope parameter in the token request should not be respected but the scope should be taken from the authorization code. It was not totally ignored, but rather the scope parameter of the token request was used for the generated ID token. This had two consequences: * Spec conforming implementations of authorization code flow didn't get correct ID tokens, since they usually don't pass scope parameter with the token request. * It's possible to get a broader scope for the ID token than what is authorized by the user in the original authorization code request. 2. For "refresh_token" grant type the scope parameter in the token request should only allow narrowing down the scope. It wasn't narrowed, but rather the original auth code scope was used for the access token and the passed in scope parameter was used for the ID token (again allowing unauthorized scopes in the ID token). 3. For "password" grant type the scope parameter in the token request should be respected. The problem with this was that it wasn't properly splitted when passed to ID token creation. Fixes #186 --- oidc_provider/lib/endpoints/token.py | 20 +++- oidc_provider/tests/app/utils.py | 2 + oidc_provider/tests/test_token_endpoint.py | 115 ++++++++++++++++++--- 3 files changed, 116 insertions(+), 21 deletions(-) diff --git a/oidc_provider/lib/endpoints/token.py b/oidc_provider/lib/endpoints/token.py index ae1eb98..88734b7 100644 --- a/oidc_provider/lib/endpoints/token.py +++ b/oidc_provider/lib/endpoints/token.py @@ -162,6 +162,8 @@ class TokenEndpoint(object): return self.create_access_token_response_dic() def create_access_token_response_dic(self): + # See https://tools.ietf.org/html/rfc6749#section-4.3 + token = create_token( self.user, self.client, @@ -173,7 +175,7 @@ class TokenEndpoint(object): nonce='self.code.nonce', at_hash=token.at_hash, request=self.request, - scope=self.params['scope'], + scope=token.scope, ) token.id_token = id_token_dic @@ -188,6 +190,8 @@ class TokenEndpoint(object): } def create_code_response_dic(self): + # See https://tools.ietf.org/html/rfc6749#section-4.1 + token = create_token( user=self.code.user, client=self.code.client, @@ -200,7 +204,7 @@ class TokenEndpoint(object): nonce=self.code.nonce, at_hash=token.at_hash, request=self.request, - scope=self.params['scope'], + scope=token.scope, ) else: id_token_dic = {} @@ -223,10 +227,18 @@ class TokenEndpoint(object): return dic def create_refresh_response_dic(self): + # See https://tools.ietf.org/html/rfc6749#section-6 + + scope_param = self.params['scope'] + scope = (scope_param.split(' ') if scope_param else self.token.scope) + unauthorized_scopes = set(scope) - set(self.token.scope) + if unauthorized_scopes: + raise TokenError('invalid_scope') + token = create_token( user=self.token.user, client=self.token.client, - scope=self.token.scope) + scope=scope) # If the Token has an id_token it's an Authentication request. if self.token.id_token: @@ -236,7 +248,7 @@ class TokenEndpoint(object): nonce=None, at_hash=token.at_hash, request=self.request, - scope=self.params['scope'], + scope=token.scope, ) else: id_token_dic = {} diff --git a/oidc_provider/tests/app/utils.py b/oidc_provider/tests/app/utils.py index 8f8f72e..4f824ec 100644 --- a/oidc_provider/tests/app/utils.py +++ b/oidc_provider/tests/app/utils.py @@ -29,6 +29,8 @@ def create_fake_user(): user = User() user.username = 'johndoe' user.email = 'johndoe@example.com' + user.first_name = 'John' + user.last_name = 'Doe' user.set_password('1234') user.save() diff --git a/oidc_provider/tests/test_token_endpoint.py b/oidc_provider/tests/test_token_endpoint.py index 46e96e4..0760555 100644 --- a/oidc_provider/tests/test_token_endpoint.py +++ b/oidc_provider/tests/test_token_endpoint.py @@ -50,15 +50,18 @@ class TokenTestCase(TestCase): self.user = create_fake_user() self.client = create_fake_client(response_type='code') - def _password_grant_post_data(self): - return { + def _password_grant_post_data(self, scope=None): + result = { 'username': 'johndoe', 'password': '1234', 'grant_type': 'password', 'scope': 'openid email', } + if scope is not None: + result['scope'] = ' '.join(scope) + return result - def _auth_code_post_data(self, code): + def _auth_code_post_data(self, code, scope=None): """ All the data that will be POSTed to the Token Endpoint. """ @@ -70,10 +73,12 @@ class TokenTestCase(TestCase): 'code': code, 'state': uuid.uuid4().hex, } + if scope is not None: + post_data['scope'] = ' '.join(scope) return post_data - def _refresh_token_post_data(self, refresh_token): + def _refresh_token_post_data(self, refresh_token, scope=None): """ All the data that will be POSTed to the Token Endpoint. """ @@ -83,6 +88,8 @@ class TokenTestCase(TestCase): 'grant_type': 'refresh_token', 'refresh_token': refresh_token, } + if scope is not None: + post_data['scope'] = ' '.join(scope) return post_data @@ -103,14 +110,14 @@ class TokenTestCase(TestCase): return response - def _create_code(self): + def _create_code(self, scope=None): """ Generate a valid grant code. """ code = create_code( user=self.user, client=self.client, - scope=['openid', 'email'], + scope=(scope if scope else ['openid', 'email']), nonce=FAKE_NONCE, is_authentication=True) code.save() @@ -228,30 +235,41 @@ class TokenTestCase(TestCase): self.assertEqual(400, response.status_code) self.assertEqual('invalid_client', response_dict['error']) - @patch('oidc_provider.lib.utils.token.uuid') + def test_password_grant_full_response(self): + self.check_password_grant(scope=['openid', 'email']) + + def test_password_grant_scope(self): + self.check_password_grant(scope=['openid', 'profile']) + @override_settings(OIDC_TOKEN_EXPIRE=120, OIDC_GRANT_TYPE_PASSWORD_ENABLE=True) - def test_password_grant_full_response(self, mock_uuid): - test_hex = 'fake_token' - mock_uuid4 = Mock(spec=uuid.uuid4) - mock_uuid4.hex = test_hex - mock_uuid.uuid4.return_value = mock_uuid4 - + def check_password_grant(self, scope): response = self._post_request( - post_data=self._password_grant_post_data(), + post_data=self._password_grant_post_data(scope), extras=self._password_grant_auth_header() ) response_dict = json.loads(response.content.decode('utf-8')) id_token = JWS().verify_compact(response_dict['id_token'].encode('utf-8'), self._get_keys()) - self.assertEqual(response_dict['access_token'], 'fake_token') - self.assertEqual(response_dict['refresh_token'], 'fake_token') + token = Token.objects.get(user=self.user) + self.assertEqual(response_dict['access_token'], token.access_token) + self.assertEqual(response_dict['refresh_token'], token.refresh_token) self.assertEqual(response_dict['expires_in'], 120) self.assertEqual(response_dict['token_type'], 'bearer') self.assertEqual(id_token['sub'], str(self.user.id)) self.assertEqual(id_token['aud'], self.client.client_id) + # Check the scope is honored by checking the claims in the userinfo + userinfo_response = self._get_userinfo(response_dict['access_token']) + userinfo = json.loads(userinfo_response.content.decode('utf-8')) + + for (scope_param, claim) in [('email', 'email'), ('profile', 'name')]: + if scope_param in scope: + self.assertIn(claim, userinfo) + else: + self.assertNotIn(claim, userinfo) + @override_settings(OIDC_TOKEN_EXPIRE=720) def test_authorization_code(self): """ @@ -277,16 +295,64 @@ class TokenTestCase(TestCase): self.assertEqual(id_token['sub'], str(self.user.id)) self.assertEqual(id_token['aud'], self.client.client_id) + @override_settings(OIDC_TOKEN_EXPIRE=720) + def test_scope_is_ignored_for_auth_code(self): + """ + Scope is ignored for token respones to auth code grant type. + """ + SIGKEYS = self._get_keys() + for code_scope in [['openid'], ['openid', 'email']]: + code = self._create_code(code_scope) + + post_data = self._auth_code_post_data( + code=code.code, scope=['openid', 'profile']) + + response = self._post_request(post_data) + response_dic = json.loads(response.content.decode('utf-8')) + + self.assertEqual(response.status_code, 200) + + id_token = JWS().verify_compact(response_dic['id_token'].encode('utf-8'), SIGKEYS) + + if 'email' in code_scope: + self.assertIn('email', id_token) + else: + self.assertNotIn('email', id_token) + def test_refresh_token(self): """ A request to the Token Endpoint can also use a Refresh Token by using the grant_type value refresh_token, as described in Section 6 of OAuth 2.0 [RFC6749]. """ + self.do_refresh_token_check() + + def test_refresh_token_invalid_scope(self): + """ + Extending scope in refresh token is not allowed. + + Try to get a refresh token with "profile" in the scope even + though the original authorized scope in the authorization code + request is only ['openid', 'email']. + """ + self.do_refresh_token_check(scope=['openid', 'profile']) + + def test_refresh_token_narrowed_scope(self): + """ + Narrowing scope in refresh token is allowed. + + Try to get a refresh token with just "openid" in the scope even + though the original authorized scope in the authorization code + request is ['openid', 'email']. + """ + self.do_refresh_token_check(scope=['openid']) + + def do_refresh_token_check(self, scope=None): SIGKEYS = self._get_keys() # Retrieve refresh token code = self._create_code() + self.assertEqual(code.scope, ['openid', 'email']) post_data = self._auth_code_post_data(code=code.code) start_time = time.time() with patch('oidc_provider.lib.utils.token.time.time') as time_func: @@ -297,14 +363,29 @@ class TokenTestCase(TestCase): id_token1 = JWS().verify_compact(response_dic1['id_token'].encode('utf-8'), SIGKEYS) # Use refresh token to obtain new token - post_data = self._refresh_token_post_data(response_dic1['refresh_token']) + post_data = self._refresh_token_post_data( + response_dic1['refresh_token'], scope) with patch('oidc_provider.lib.utils.token.time.time') as time_func: time_func.return_value = start_time + 600 response = self._post_request(post_data) response_dic2 = json.loads(response.content.decode('utf-8')) + + if scope and set(scope) - set(code.scope): # too broad scope + self.assertEqual(response.status_code, 400) # Bad Request + self.assertIn('error', response_dic2) + self.assertEqual(response_dic2['error'], 'invalid_scope') + return # No more checks + id_token2 = JWS().verify_compact(response_dic2['id_token'].encode('utf-8'), SIGKEYS) + if scope and 'email' not in scope: # narrowed scope The auth + # The auth code request had email in scope, so it should be + # in the first id token + self.assertIn('email', id_token1) + # but the refresh request had no email in scope + self.assertNotIn('email', id_token2, 'email was not requested') + self.assertNotEqual(response_dic1['id_token'], response_dic2['id_token']) self.assertNotEqual(response_dic1['access_token'], response_dic2['access_token']) self.assertNotEqual(response_dic1['refresh_token'], response_dic2['refresh_token']) From eb682f23ff9528a26dc5d424d166e068ef67796e Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Fri, 7 Jul 2017 22:55:18 +0300 Subject: [PATCH 2/4] Pass scope to OIDC_IDTOKEN_PROCESSING_HOOK The ID token processing hook might want to add claims to the ID token conditionally based on the scope parameter. Therefore it would be very useful to provide the scope parameter to the processing hook. --- oidc_provider/lib/utils/common.py | 5 ++++- oidc_provider/lib/utils/token.py | 13 +++++++------ oidc_provider/tests/app/utils.py | 12 ++++++++++-- oidc_provider/tests/test_token_endpoint.py | 20 ++++++++++++++++++++ 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/oidc_provider/lib/utils/common.py b/oidc_provider/lib/utils/common.py index c4778bd..9ef225d 100644 --- a/oidc_provider/lib/utils/common.py +++ b/oidc_provider/lib/utils/common.py @@ -114,7 +114,7 @@ def default_after_end_session_hook(request, id_token=None, post_logout_redirect_ return None -def default_idtoken_processing_hook(id_token, user): +def default_idtoken_processing_hook(id_token, user, scope=None): """ Hook to perform some additional actions ti `id_token` dictionary just before serialization. @@ -124,6 +124,9 @@ def default_idtoken_processing_hook(id_token, user): :param user: user for whom id_token is generated :type user: User + :param scope: scope for the token + :type scope: list[str]|None + :return: custom modified dictionary of values for `id_token` :rtype dict """ diff --git a/oidc_provider/lib/utils/token.py b/oidc_provider/lib/utils/token.py index 3f0d2e9..91bf459 100644 --- a/oidc_provider/lib/utils/token.py +++ b/oidc_provider/lib/utils/token.py @@ -53,13 +53,14 @@ def create_id_token(user, aud, nonce='', at_hash='', request=None, scope=[]): if ('email' in scope) and getattr(user, 'email', None): dic['email'] = user.email - processing_hook = settings.get('OIDC_IDTOKEN_PROCESSING_HOOK') + processing_hooks = settings.get('OIDC_IDTOKEN_PROCESSING_HOOK') - if isinstance(processing_hook, (list, tuple)): - for hook in processing_hook: - dic = settings.import_from_str(hook)(dic, user=user) - else: - dic = settings.import_from_str(processing_hook)(dic, user=user) + if not isinstance(processing_hooks, (list, tuple)): + processing_hooks = [processing_hooks] + + for hook_string in processing_hooks: + hook = settings.import_from_str(hook_string) + dic = hook(dic, user=user, scope=scope) return dic diff --git a/oidc_provider/tests/app/utils.py b/oidc_provider/tests/app/utils.py index 4f824ec..31a9aca 100644 --- a/oidc_provider/tests/app/utils.py +++ b/oidc_provider/tests/app/utils.py @@ -107,7 +107,7 @@ def fake_sub_generator(user): return user.email -def fake_idtoken_processing_hook(id_token, user): +def fake_idtoken_processing_hook(id_token, user, scope=None): """ Fake function for inserting some keys into token. Testing OIDC_IDTOKEN_PROCESSING_HOOK. """ @@ -116,10 +116,18 @@ def fake_idtoken_processing_hook(id_token, user): return id_token -def fake_idtoken_processing_hook2(id_token, user): +def fake_idtoken_processing_hook2(id_token, user, scope=None): """ Fake function for inserting some keys into token. Testing OIDC_IDTOKEN_PROCESSING_HOOK - tuple or list as param """ id_token['test_idtoken_processing_hook2'] = FAKE_RANDOM_STRING id_token['test_idtoken_processing_hook_user_email2'] = user.email return id_token + + +def fake_idtoken_processing_hook3(id_token, user, scope=None): + """ + Fake function for checking scope is passed to processing hook. + """ + id_token['scope_passed_to_processing_hook'] = scope + return id_token diff --git a/oidc_provider/tests/test_token_endpoint.py b/oidc_provider/tests/test_token_endpoint.py index 0760555..3fedd83 100644 --- a/oidc_provider/tests/test_token_endpoint.py +++ b/oidc_provider/tests/test_token_endpoint.py @@ -734,6 +734,26 @@ class TokenTestCase(TestCase): self.assertEqual(id_token.get('test_idtoken_processing_hook2'), FAKE_RANDOM_STRING) self.assertEqual(id_token.get('test_idtoken_processing_hook_user_email2'), self.user.email) + @override_settings( + OIDC_IDTOKEN_PROCESSING_HOOK=( + 'oidc_provider.tests.app.utils.fake_idtoken_processing_hook3')) + def test_additional_idtoken_processing_hook_scope_param(self): + """ + Test scope parameter is passed to OIDC_IDTOKEN_PROCESSING_HOOK. + """ + code = self._create_code(['openid', 'email', 'profile', 'dummy']) + + post_data = self._auth_code_post_data(code=code.code) + + response = self._post_request(post_data) + + response_dic = json.loads(response.content.decode('utf-8')) + id_token = JWT().unpack(response_dic['id_token'].encode('utf-8')).payload() + + self.assertEqual( + id_token.get('scope_passed_to_processing_hook'), + ['openid', 'email', 'profile', 'dummy']) + def test_pkce_parameters(self): """ Test Proof Key for Code Exchange by OAuth Public Clients. From b0a82aa4ab6a6d45b0b5128d7f1bd855bd565860 Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Thu, 24 May 2018 00:24:52 +0300 Subject: [PATCH 3/4] Pass token and request to OIDC_ID_TOKEN_PROCESSING_HOOK The ID token processing hook might need the token or request too, so make them available. --- oidc_provider/lib/utils/common.py | 13 ++++++-- oidc_provider/lib/utils/token.py | 2 +- oidc_provider/tests/app/utils.py | 17 ++++++++-- .../tests/cases/test_token_endpoint.py | 31 ++++++++++++++++--- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/oidc_provider/lib/utils/common.py b/oidc_provider/lib/utils/common.py index c1d913f..0ecc95f 100644 --- a/oidc_provider/lib/utils/common.py +++ b/oidc_provider/lib/utils/common.py @@ -107,9 +107,10 @@ def default_after_end_session_hook( return None -def default_idtoken_processing_hook(id_token, user, scope=None): +def default_idtoken_processing_hook( + id_token, user, scope, token, request, **kwargs): """ - Hook to perform some additional actions ti `id_token` dictionary just before serialization. + Hook for modifying `id_token` just before serialization. :param id_token: dictionary contains values that going to be serialized into `id_token` :type id_token: dict @@ -120,8 +121,14 @@ def default_idtoken_processing_hook(id_token, user, scope=None): :param scope: scope for the token :type scope: list[str]|None + :param token: the Token object created for the authentication request + :type token: oidc_provider.models.Token + + :param request: the request initiating this ID token processing + :type request: django.http.HttpRequest + :return: custom modified dictionary of values for `id_token` - :rtype dict + :rtype: dict """ return id_token diff --git a/oidc_provider/lib/utils/token.py b/oidc_provider/lib/utils/token.py index 264c268..089ce45 100644 --- a/oidc_provider/lib/utils/token.py +++ b/oidc_provider/lib/utils/token.py @@ -64,7 +64,7 @@ def create_id_token(token, user, aud, nonce='', at_hash='', request=None, scope= dic = run_processing_hook( dic, 'OIDC_IDTOKEN_PROCESSING_HOOK', - user=user, scope=scope) + user=user, scope=scope, token=token, request=request) return dic diff --git a/oidc_provider/tests/app/utils.py b/oidc_provider/tests/app/utils.py index 6ab07f2..63ddc8d 100644 --- a/oidc_provider/tests/app/utils.py +++ b/oidc_provider/tests/app/utils.py @@ -113,7 +113,7 @@ def fake_sub_generator(user): return user.email -def fake_idtoken_processing_hook(id_token, user, scope=None): +def fake_idtoken_processing_hook(id_token, user, **kwargs): """ Fake function for inserting some keys into token. Testing OIDC_IDTOKEN_PROCESSING_HOOK. """ @@ -122,7 +122,7 @@ def fake_idtoken_processing_hook(id_token, user, scope=None): return id_token -def fake_idtoken_processing_hook2(id_token, user, scope=None): +def fake_idtoken_processing_hook2(id_token, user, **kwargs): """ Fake function for inserting some keys into token. Testing OIDC_IDTOKEN_PROCESSING_HOOK - tuple or list as param @@ -132,7 +132,7 @@ def fake_idtoken_processing_hook2(id_token, user, scope=None): return id_token -def fake_idtoken_processing_hook3(id_token, user, scope=None): +def fake_idtoken_processing_hook3(id_token, user, scope=None, **kwargs): """ Fake function for checking scope is passed to processing hook. """ @@ -140,6 +140,17 @@ def fake_idtoken_processing_hook3(id_token, user, scope=None): return id_token +def fake_idtoken_processing_hook4(id_token, user, **kwargs): + """ + Fake function for checking kwargs passed to processing hook. + """ + id_token['kwargs_passed_to_processing_hook'] = { + key: repr(value) + for (key, value) in kwargs.items() + } + return id_token + + def fake_introspection_processing_hook(response_dict, client, id_token): response_dict['test_introspection_processing_hook'] = FAKE_RANDOM_STRING return response_dict diff --git a/oidc_provider/tests/cases/test_token_endpoint.py b/oidc_provider/tests/cases/test_token_endpoint.py index d0e3703..fcedd35 100644 --- a/oidc_provider/tests/cases/test_token_endpoint.py +++ b/oidc_provider/tests/cases/test_token_endpoint.py @@ -735,7 +735,31 @@ class TokenTestCase(TestCase): """ Test scope parameter is passed to OIDC_IDTOKEN_PROCESSING_HOOK. """ - code = self._create_code(['openid', 'email', 'profile', 'dummy']) + id_token = self._request_id_token_with_scope( + ['openid', 'email', 'profile', 'dummy']) + self.assertEqual( + id_token.get('scope_passed_to_processing_hook'), + ['openid', 'email', 'profile', 'dummy']) + + @override_settings( + OIDC_IDTOKEN_PROCESSING_HOOK=( + 'oidc_provider.tests.app.utils.fake_idtoken_processing_hook4')) + def test_additional_idtoken_processing_hook_kwargs(self): + """ + Test correct kwargs are passed to OIDC_IDTOKEN_PROCESSING_HOOK. + """ + id_token = self._request_id_token_with_scope(['openid', 'profile']) + kwargs_passed = id_token.get('kwargs_passed_to_processing_hook') + assert kwargs_passed + self.assertEqual(kwargs_passed.get('scope'), + repr([u'openid', u'profile'])) + self.assertEqual(kwargs_passed.get('token'), + '') + self.assertEqual(kwargs_passed.get('request'), + "") + + def _request_id_token_with_scope(self, scope): + code = self._create_code(scope) post_data = self._auth_code_post_data(code=code.code) @@ -743,10 +767,7 @@ class TokenTestCase(TestCase): response_dic = json.loads(response.content.decode('utf-8')) id_token = JWT().unpack(response_dic['id_token'].encode('utf-8')).payload() - - self.assertEqual( - id_token.get('scope_passed_to_processing_hook'), - ['openid', 'email', 'profile', 'dummy']) + return id_token def test_pkce_parameters(self): """ From 7eb31574ee540687cd41ae1befcc7f6a6764f423 Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Thu, 24 May 2018 01:09:09 +0300 Subject: [PATCH 4/4] Document the new ID token processing hook parameters --- docs/sections/settings.rst | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/sections/settings.rst b/docs/sections/settings.rst index f6f3131..fbed9fc 100644 --- a/docs/sections/settings.rst +++ b/docs/sections/settings.rst @@ -81,12 +81,28 @@ Here you can add extra dictionary values specific for your app into id_token. The ``list`` or ``tuple`` is useful when you want to set multiple hooks, i.e. one for permissions and second for some special field. -The function receives a ``id_token`` dictionary and ``user`` instance -and returns it with additional fields. +The hook function receives following arguments: + + * ``id_token``: the ID token dictionary which contains at least the + basic claims (``iss``, ``sub``, ``aud``, ``exp``, ``iat``, + ``auth_time``), but may also contain other claims. If several + processing hooks are configured, then the claims of the previous hook + are also present in the passed dictionary. + * ``user``: User object of the authenticating user, + * ``scope``: the authorized scopes as list of strings or None, + * ``token``: the Token object created for the authentication request, and + * ``request``: Django request object of the authentication request. + +The hook function should return the modified ID token as dictionary. + +.. note:: + It is a good idea to add ``**kwargs`` to the hook function argument + list so that the hook function will work even if new arguments are + added to the hook function call signature. Default is:: - def default_idtoken_processing_hook(id_token, user): + def default_idtoken_processing_hook(id_token, user, scope, token, request, **kwargs): return id_token