From 4e30e04e2cd2dc0d7ad24a539fbc88b7704e718e Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Sun, 18 Oct 2020 12:54:50 +0200 Subject: [PATCH 1/2] auth_oidc: dont stop redirect on state lost (#47825) Next url is usually to a local view requiring login, so on state lost we can just continue to original next_url and it will start a new login. --- src/authentic2_auth_oidc/views.py | 15 +++++++----- tests/test_auth_oidc.py | 40 ++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/authentic2_auth_oidc/views.py b/src/authentic2_auth_oidc/views.py index 49bad4e0..25aff145 100644 --- a/src/authentic2_auth_oidc/views.py +++ b/src/authentic2_auth_oidc/views.py @@ -41,6 +41,9 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs): provider = get_provider(pk) scopes = set(provider.scopes.split()) | set(['openid']) state = str(uuid.uuid4()) + next_url = next_url or request.GET.get(REDIRECT_FIELD_NAME, '') + if next_url and not good_next_url(request, next_url): + next_url = None nonce = request.GET.get('nonce') or str(uuid.uuid4()) display = set() prompt = set() @@ -49,7 +52,7 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs): 'scope': ' '.join(scopes), 'response_type': 'code', 'redirect_uri': request.build_absolute_uri(reverse('oidc-login-callback')), - 'state': state, + 'state': state if not next_url else state + ' ' + next_url, 'nonce': nonce, } if provider.claims_parameter_supported: @@ -73,8 +76,7 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs): saved_state = request.session.setdefault('auth_oidc', {}).setdefault(state, {}) saved_state['request'] = params saved_state['issuer'] = provider.issuer - next_url = next_url or request.GET.get(REDIRECT_FIELD_NAME, '') - if good_next_url(request, next_url): + if next_url: saved_state['next_url'] = next_url request.session.modified = True # necessary if auth_oidc already exists logger.debug('auth_oidc: sent request to authorization endpoint %r', params) @@ -102,12 +104,13 @@ class LoginCallback(View): def get(self, request, *args, **kwargs): logger = logging.getLogger(__name__) code = request.GET.get('code') - state = request.GET.get('state') + raw_state = request.GET.get('state', '') + state = raw_state.split(' ', 1)[0] oidc_state = self.oidc_state = request.session.get('auth_oidc', {}).get(state) if not state or not oidc_state or 'request' not in oidc_state: - messages.warning(request, _('Login with OpenIDConnect failed, state lost.')) logger.warning('auth_oidc: state lost') - return redirect(request, settings.LOGIN_REDIRECT_URL) + state_next_url = raw_state.split(' ', 1)[1] if ' ' in raw_state else None + return redirect(request, state_next_url or settings.LOGIN_REDIRECT_URL) oidc_request = oidc_state.get('request') assert isinstance(oidc_request, dict), 'state is not properly initialized' nonce = oidc_request.get('nonce') diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 3c83e9eb..dec31d8d 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -496,13 +496,14 @@ def test_sso(app, caplog, code, oidc_provider, oidc_provider_jwkset, hooks): assert location.netloc == endpoint.netloc assert location.path == endpoint.path query = check_simple_qs(urlparse.parse_qs(location.query)) - assert query['state'] in app.session['auth_oidc'] + state = query['state'].split()[0] + assert state in app.session['auth_oidc'] assert query['response_type'] == 'code' assert query['client_id'] == str(oidc_provider.client_id) assert query['scope'] == 'openid' assert query['redirect_uri'] == 'http://testserver' + reverse('oidc-login-callback') # get the nonce - nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] + nonce = app.session['auth_oidc'][state]['request']['nonce'] if oidc_provider.claims_parameter_supported: claims = json.loads(query['claims']) @@ -627,7 +628,8 @@ def test_strategy_find_uuid(app, caplog, code, oidc_provider, oidc_provider_jwks response = response.click(oidc_provider.name) location = urlparse.urlparse(response.location) query = check_simple_qs(urlparse.parse_qs(location.query)) - nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] + state = query['state'].split()[0] + nonce = app.session['auth_oidc'][state]['request']['nonce'] # sub=john.doe, MUST not work with utils.check_log(caplog, 'cannot create user'): @@ -669,7 +671,8 @@ def test_strategy_create(app, caplog, code, oidc_provider, oidc_provider_jwkset) response = response.click(oidc_provider.name) location = urlparse.urlparse(response.location) query = check_simple_qs(urlparse.parse_qs(location.query)) - nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] + state = query['state'].split()[0] + nonce = app.session['auth_oidc'][state]['request']['nonce'] # sub=john.doe with utils.check_log(caplog, 'auth_oidc: created user'): @@ -743,7 +746,8 @@ def test_invalid_kid(app, caplog, code, oidc_provider_rsa, response = response.click(oidc_provider_rsa.name) location = urlparse.urlparse(response.location) query = check_simple_qs(urlparse.parse_qs(location.query)) - nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] + state = query['state'].split()[0] + nonce = app.session['auth_oidc'][state]['request']['nonce'] # test invalid kid with utils.check_log(caplog, message='not in key set', levelname='WARNING'): @@ -808,7 +812,8 @@ def test_templated_claim_mapping(app, caplog, code, oidc_provider, oidc_provider response = response.click(oidc_provider.name) location = urlparse.urlparse(response.location) query = check_simple_qs(urlparse.parse_qs(location.query)) - nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] + state = query['state'].split()[0] + nonce = app.session['auth_oidc'][state]['request']['nonce'] with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): response = app.get(login_callback_url(oidc_provider), params={'code': code, 'state': query['state']}).maybe_follow() @@ -822,3 +827,26 @@ def test_templated_claim_mapping(app, caplog, code, oidc_provider, oidc_provider assert user.last_name == 'DOE' # typo in template string, no rendering assert user.first_name == '{{ given_name' + + +def test_lost_state(app, caplog, code, oidc_provider, oidc_provider_jwkset, hooks): + response = app.get('/login/?next=/whatever/') + assert oidc_provider.name in response.text + response = response.click(oidc_provider.name) + qs = urlparse.parse_qs(urlparse.urlparse(response.location).query) + state = qs['state'][0] + assert ' /whatever/' in state + nonce = app.session['auth_oidc'][state.split()[0]]['request']['nonce'] + + # reset the session to forget the state + app.session.flush() + + caplog.clear() + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): + response = app.get(login_callback_url(oidc_provider), params={'code': code, 'state': state}) + # not logged + assert 'auth_oidc: state lost' == caplog.records[-1].message + # event is recorded + assert '_auth_user_id' not in app.session + # we are automatically redirected to our destination + assert response.location == '/whatever/' -- 2.28.0