From ef3ea29fcbc2592b3825fe6ecf36ab0c6377dbd6 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Tue, 12 Jul 2022 14:30:41 +0200 Subject: [PATCH] idp_oidc: use invalid_grant error in token endpoint (#66544) --- src/authentic2_idp_oidc/views.py | 13 +++-- tests/idp_oidc/conftest.py | 28 +++++++---- tests/idp_oidc/test_misc.py | 84 +++++++++++++++++++++++++++----- 3 files changed, 100 insertions(+), 25 deletions(-) diff --git a/src/authentic2_idp_oidc/views.py b/src/authentic2_idp_oidc/views.py index 4a8fa1a9..056bb72e 100644 --- a/src/authentic2_idp_oidc/views.py +++ b/src/authentic2_idp_oidc/views.py @@ -74,6 +74,7 @@ class OIDCException(Exception): content['error_description'] = self.error_description if self.client: + content['client_id'] = self.client.client_id msg = 'idp_oidc: error "%s" in %s endpoint "%s" for client %s' if self.extra_info: msg += ' (%s)' % self.extra_info @@ -181,6 +182,10 @@ class InvalidClient(OIDCException): error_code = 'invalid_client' +class InvalidGrant(OIDCException): + error_code = 'invalid_grant' + + class WrongClientSecret(InvalidClient): error_description = _('Wrong client secret') @@ -730,12 +735,14 @@ def tokens_from_authz_code(request): try: oidc_code = models.OIDCCode.objects.select_related().get(uuid=code) except models.OIDCCode.DoesNotExist: - raise InvalidRequest(_('Parameter "code" is invalid'), client=client) + raise InvalidGrant(_('Code is unknown.'), client=client) + if oidc_code.client != client: + raise InvalidGrant(_('Code was issued to a different client.'), client=client) if not oidc_code.is_valid(): - raise InvalidRequest(_('Parameter "code" has expired or user is disconnected'), client=client) + raise InvalidGrant(_('Code has expired, user is disconnected or session was lost.'), client=client) redirect_uri = request.POST.get('redirect_uri') if oidc_code.redirect_uri != redirect_uri: - raise InvalidRequest(_('Parameter "redirect_uri" does not match the code.'), client=client) + raise InvalidGrant(_('Redirect_uri does not match the code.'), client=client) if client.access_token_duration is None: expires_in = datetime.timedelta(seconds=oidc_code.session.get_expiry_age()) expired = None diff --git a/tests/idp_oidc/conftest.py b/tests/idp_oidc/conftest.py index 0d2aa40b..5e8d89dc 100644 --- a/tests/idp_oidc/conftest.py +++ b/tests/idp_oidc/conftest.py @@ -82,20 +82,23 @@ def profile_settings(settings, jwkset): return settings -def make_client(app, superuser, params=None): - Attribute.objects.create( +def make_client(app, params=None): + Attribute.objects.get_or_create( name='cityscape_image', - label='cityscape', - kind='profile_image', - asked_on_registration=True, - required=False, - user_visible=True, - user_editable=True, + defaults=dict( + label='cityscape', + kind='profile_image', + asked_on_registration=True, + required=False, + user_visible=True, + user_editable=True, + ), ) client = OIDCClient( name='oidcclient', slug='oidcclient', + client_id='1234', ou=get_default_ou(), unauthorized_url='https://example.com/southpark/', redirect_uris='https://example.com/callbac%C3%A9', @@ -111,9 +114,14 @@ def make_client(app, superuser, params=None): return client +@pytest.fixture(name='make_client') +def make_client_fixture(): + return make_client + + @pytest.fixture def client(app, superuser): - return make_client(app, superuser, {}) + return make_client(app) @pytest.fixture @@ -125,7 +133,7 @@ def simple_oidc_client(db): @pytest.fixture def oidc_client(request, superuser, app, simple_user, oidc_settings): - return make_client(app, superuser, getattr(request, 'param', None) or {}) + return make_client(app, getattr(request, 'param', None) or {}) @pytest.fixture diff --git a/tests/idp_oidc/test_misc.py b/tests/idp_oidc/test_misc.py index 8e36b1a8..4e566b0a 100644 --- a/tests/idp_oidc/test_misc.py +++ b/tests/idp_oidc/test_misc.py @@ -470,7 +470,7 @@ def assert_authorization_response(response, fragment=False, **kwargs): @pytest.mark.parametrize('oidc_client', OIDC_CLIENT_PARAMS, indirect=True) -def test_invalid_request(oidc_client, caplog, oidc_settings, simple_user, app): +def test_invalid_request(oidc_client, caplog, oidc_settings, simple_user, app, make_client): redirect_uri = oidc_client.redirect_uris.split()[0] if oidc_client.authorization_flow == oidc_client.FLOW_AUTHORIZATION_CODE: fragment = False @@ -865,18 +865,17 @@ def test_invalid_request(oidc_client, caplog, oidc_settings, simple_user, app): # check expired codes if oidc_client.authorization_flow == oidc_client.FLOW_AUTHORIZATION_CODE: assert OIDCCode.objects.count() == 1 - code = OIDCCode.objects.get() - assert code.is_valid() - # make code expire - code.expired = now() - datetime.timedelta(seconds=120) - assert not code.is_valid() - code.save() + oidc_code = OIDCCode.objects.get() + assert oidc_code.is_valid() + location = urllib.parse.urlparse(response['Location']) query = urllib.parse.parse_qs(location.query) assert set(query.keys()) == {'code'} - assert query['code'] == [code.uuid] + assert query['code'] == [oidc_code.uuid] code = query['code'][0] token_url = make_url('oidc-token') + + # missing code parameter params = { 'grant_type': 'authorization_code', 'redirect_uri': oidc_client.redirect_uris.split()[0], @@ -888,12 +887,74 @@ def test_invalid_request(oidc_client, caplog, oidc_settings, simple_user, app): assert response.json['error_description'] == 'Missing parameter "code"' params['code'] = code + + # wrong redirect_uri + response = app.post( + token_url, + params=dict(params, redirect_uri='https://example.com/'), + headers=client_authentication_headers(oidc_client), + status=400, + ) + assert 'error' in response.json + assert response.json['error'] == 'invalid_grant', response.json + assert response.json['error_description'] == 'Redirect_uri does not match the code.' + assert response.json['client_id'] == '1234' + + # unknown code + response = app.post( + token_url, + params=dict(params, code='xyz'), + headers=client_authentication_headers(oidc_client), + status=400, + ) + assert 'error' in response.json + assert response.json['error'] == 'invalid_grant' + assert response.json['error_description'] == 'Code is unknown.' + assert response.json['client_id'] == '1234' + + # code from another client + other_client = make_client(app, params={'slug': 'other', 'name': 'other', 'client_id': 'abcd'}) + other_oidc_code = OIDCCode.objects.create( + client=other_client, + user=oidc_code.user, + profile=None, + scopes='', + state='1234', + nonce='1234', + expired=now() + datetime.timedelta(hours=1), + redirect_uri=oidc_code.redirect_uri, + auth_time=now(), + session_key=oidc_code.session_key, + ) + response = app.post( + token_url, + params=dict(params, code=other_oidc_code.uuid), + headers=client_authentication_headers(oidc_client), + status=400, + ) + assert 'error' in response.json + assert response.json['error'] == 'invalid_grant' + assert response.json['error_description'] == 'Code was issued to a different client.', response.json + assert response.json['client_id'] == '1234' + other_oidc_code.delete() + other_client.delete() + + # make code expire + oidc_code.expired = now() - datetime.timedelta(seconds=120) + assert not oidc_code.is_valid() + oidc_code.save() + + # expired code response = app.post( token_url, params=params, headers=client_authentication_headers(oidc_client), status=400 ) assert 'error' in response.json - assert response.json['error'] == 'invalid_request' - assert response.json['error_description'] == 'Parameter "code" has expired or user is disconnected' + assert response.json['error'] == 'invalid_grant' + assert ( + response.json['error_description'] + == 'Code has expired, user is disconnected or session was lost.' + ) + assert response.json['client_id'] == '1234' # invalid logout logout_url = make_url( @@ -926,8 +987,7 @@ def test_invalid_request(oidc_client, caplog, oidc_settings, simple_user, app): status=400, ) assert 'error' in response.json - assert response.json['error'] == 'invalid_request' - assert response.json['error_description'] == 'Parameter "code" has expired or user is disconnected' + assert response.json['error'] == 'invalid_grant' def test_client_secret_post_authentication(oidc_settings, app, simple_oidc_client, simple_user): -- 2.35.1