From b02ec2ee48f724807c86008aa508cbf2589534a8 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Thu, 9 Aug 2018 15:37:19 +0200 Subject: [PATCH] keep authentication context (fixes #21908) - simplify and reorganize login templates, - URL are not built inside templates anymore, - we have now 3 different templates: - login.html for the login page - registration.html for the registration page - linking.html for the account page - using feature from #25623, authentication_method is kept by the registration view. - the service slug is correctly threaded between every views. --- src/authentic2_auth_fc/auth_frontends.py | 23 ++- .../authentic2_auth_fc/connecting.html | 23 --- .../templates/authentic2_auth_fc/login.html | 47 ++++- .../authentic2_auth_fc/registration.html | 24 +++ src/authentic2_auth_fc/views.py | 37 ++-- tests/conftest.py | 29 +++ tests/test_auth_fc.py | 176 +++++++++++++++++- 7 files changed, 311 insertions(+), 48 deletions(-) delete mode 100644 src/authentic2_auth_fc/templates/authentic2_auth_fc/connecting.html create mode 100644 src/authentic2_auth_fc/templates/authentic2_auth_fc/registration.html diff --git a/src/authentic2_auth_fc/auth_frontends.py b/src/authentic2_auth_fc/auth_frontends.py index 5a4797c..e9d3bf0 100644 --- a/src/authentic2_auth_fc/auth_frontends.py +++ b/src/authentic2_auth_fc/auth_frontends.py @@ -2,7 +2,7 @@ from django.utils.translation import gettext_noop from django.template.loader import render_to_string from django.shortcuts import render -from authentic2 import app_settings as a2_app_settings +from authentic2 import app_settings as a2_app_settings, utils as a2_utils from . import app_settings @@ -17,11 +17,23 @@ class FcFrontend(object): def id(self): return 'fc' + @property + def popup(self): + return False + def login(self, request, *args, **kwargs): if 'nofc' in request.GET: return context = kwargs.pop('context', {}).copy() - context['about_url'] = app_settings.about_url + params = {} + if self.popup: + params['popup'] = '' + context.update({ + 'popup': self.popup, + 'login_url': a2_utils.make_url('fc-login-or-link', keep_params=True, params=params, request=request), + 'registration_url': a2_utils.make_url('fc-registration', keep_params=True, request=request), + 'about_url': app_settings.about_url, + }) if 'fc_user_info' in request.session: context['fc_user_info'] = request.session['fc_user_info'] return render(request, 'authentic2_auth_fc/login.html', context) @@ -34,7 +46,7 @@ class FcFrontend(object): context = kwargs.pop('context', {}).copy() context.update({ - 'popup': True, + 'popup': self.popup, 'unlink': unlink, 'about_url': app_settings.about_url }) @@ -46,7 +58,8 @@ class FcFrontend(object): context = kwargs.get('context', {}).copy() context.update({ + 'login_url': a2_utils.make_url('fc-login-or-link', keep_params=True, params={'registration': ''}, request=request), + 'popup': self.popup, 'about_url': app_settings.about_url, - 'registration': True, }) - return render(request, 'authentic2_auth_fc/login.html', context) + return render(request, 'authentic2_auth_fc/registration.html', context) diff --git a/src/authentic2_auth_fc/templates/authentic2_auth_fc/connecting.html b/src/authentic2_auth_fc/templates/authentic2_auth_fc/connecting.html deleted file mode 100644 index 75e38b9..0000000 --- a/src/authentic2_auth_fc/templates/authentic2_auth_fc/connecting.html +++ /dev/null @@ -1,23 +0,0 @@ -{% load staticfiles %} -{% load i18n %} - -{% if 'nofc' not in request.GET %} - -
- - {% block fc-explanation %} -

{% trans "What is FranceConnect?" %}

-

{% blocktrans %} - FranceConnect is the solution proposed by the French state to streamline - logging in online services. You can use to connect to your account. - {% endblocktrans %}

- {% endblock %} -
-{% endif %} diff --git a/src/authentic2_auth_fc/templates/authentic2_auth_fc/login.html b/src/authentic2_auth_fc/templates/authentic2_auth_fc/login.html index 3398ce5..7d9e782 100644 --- a/src/authentic2_auth_fc/templates/authentic2_auth_fc/login.html +++ b/src/authentic2_auth_fc/templates/authentic2_auth_fc/login.html @@ -1 +1,46 @@ -{% include "authentic2_auth_fc/connecting.html" %} +{% load staticfiles %} +{% load i18n %} + + +
+ +{% block fc-explanation %} +

+ {% trans "What is FranceConnect?" %} +

+

{% blocktrans %} + FranceConnect is the solution proposed by the French state to streamline + logging in online services. You can use to connect to your account. + {% endblocktrans %}

+{% endblock %} +
diff --git a/src/authentic2_auth_fc/templates/authentic2_auth_fc/registration.html b/src/authentic2_auth_fc/templates/authentic2_auth_fc/registration.html new file mode 100644 index 0000000..ae55a77 --- /dev/null +++ b/src/authentic2_auth_fc/templates/authentic2_auth_fc/registration.html @@ -0,0 +1,24 @@ +{% load staticfiles %} +{% load i18n %} + + +
+ +{% block fc-explanation %} +

+ {% trans "What is FranceConnect?" %} +

+

{% blocktrans %} + FranceConnect is the solution proposed by the French state to streamline + logging in online services. You can use to connect to your account. + {% endblocktrans %}

+{% endblock %} +
diff --git a/src/authentic2_auth_fc/views.py b/src/authentic2_auth_fc/views.py index 63aa9fa..6538db1 100644 --- a/src/authentic2_auth_fc/views.py +++ b/src/authentic2_auth_fc/views.py @@ -143,7 +143,7 @@ class FcOAuthSessionViewMixin(LoggerMixin): def get_in_popup(self): return self.in_popup - def redirect_to(self, request, *args, **kwargs): + def redirect_to(self, request): if request.method == 'POST': redirect_to = request.POST.get(self.redirect_field_name, request.GET.get(self.redirect_field_name, '')) @@ -162,7 +162,7 @@ class FcOAuthSessionViewMixin(LoggerMixin): {'redirect_to': next_url}) def simple_redirect(self, request, next_url, *args, **kwargs): - return HttpResponseRedirect(next_url) + return a2_utils.redirect(request, next_url, *args, **kwargs) def redirect(self, request, *args, **kwargs): next_url = kwargs.pop('next_url', None) @@ -175,12 +175,10 @@ class FcOAuthSessionViewMixin(LoggerMixin): return self.simple_redirect(request, next_url, *args, **kwargs) def redirect_and_come_back(self, request, next_url, *args, **kwargs): - old_next_url = self.redirect_to(request, *args, **kwargs) - here = '{0}?{1}'.format( - request.path, urlencode({REDIRECT_FIELD_NAME: old_next_url})) - there = '{0}{2}{1}'.format( - next_url, urlencode({REDIRECT_FIELD_NAME: here}), - '&' if '?' in next_url else '?') + old_next_url = self.redirect_to(request) + here = a2_utils.make_url(request.path, params={REDIRECT_FIELD_NAME: old_next_url}) + here = a2_utils.make_url(here, **kwargs) + there = a2_utils.make_url(next_url, params={REDIRECT_FIELD_NAME: here}) return self.redirect(request, next_url=there, *args, **kwargs) def get_scopes(self): @@ -421,16 +419,24 @@ class LoginOrLinkView(PopupViewMixin, FcOAuthSessionViewMixin, View): self.logger.info('logged in using fc sub %s', self.sub) return self.redirect(request) else: + params = {} + if self.service_slug: + params[constants.SERVICE_FIELD_NAME] = self.service_slug if registration: - return self.redirect_and_come_back(request, reverse('fc-registration')) + return self.redirect_and_come_back(request, + a2_utils.make_url('fc-registration', + params=params), + params=params) else: messages.info(request, _('If you already have an account, please log in, else ' 'create your account.')) - if app_settings.show_button_quick_account_creation: - return self.redirect_and_come_back(request, settings.LOGIN_URL) - else: - return self.redirect_and_come_back(request, - '{0}?nofc=1'.format(settings.LOGIN_URL)) + + login_params = params.copy() + if not app_settings.show_button_quick_account_creation: + login_params['nofc'] = 1 + + login_url = a2_utils.make_url(settings.LOGIN_URL, params=login_params) + return self.redirect_and_come_back(request, login_url, params=params) class RegistrationView(LoggerMixin, View): @@ -460,6 +466,9 @@ class RegistrationView(LoggerMixin, View): signing.dumps(data))) data['valid_email'] = False data['franceconnect'] = True + data['authentication_method'] = 'france-connect' + if constants.SERVICE_FIELD_NAME in request.GET: + data[constants.SERVICE_FIELD_NAME] = request.GET[constants.SERVICE_FIELD_NAME] activation_url = a2_utils.build_activation_url(request, next_url=redirect_to, **data) diff --git a/tests/conftest.py b/tests/conftest.py index aca8c81..feccfef 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -84,3 +84,32 @@ def admin(db): def user_cartman(db, ou_southpark): return create_user(username='ecartman', first_name='eric', last_name='cartman', email='ecartman@southpark.org', ou=ou_southpark, federation=CARTMAN_FC_INFO) + + +class AllHook(object): + def __init__(self): + self.calls = {} + from authentic2 import hooks + hooks.get_hooks.cache.clear() + + def __call__(self, hook_name, *args, **kwargs): + calls = self.calls.setdefault(hook_name, []) + calls.append({'args': args, 'kwargs': kwargs}) + + def __getattr__(self, name): + return self.calls.get(name, []) + + def clear(self): + self.calls = {} + + +@pytest.fixture +def hooks(settings): + if hasattr(settings, 'A2_HOOKS'): + hooks = settings.A2_HOOKS + else: + hooks = settings.A2_HOOKS = {} + hook = hooks['__all__'] = AllHook() + yield hook + hook.clear() + del settings.A2_HOOKS['__all__'] diff --git a/tests/test_auth_fc.py b/tests/test_auth_fc.py index 0fb2106..412b455 100644 --- a/tests/test_auth_fc.py +++ b/tests/test_auth_fc.py @@ -64,9 +64,9 @@ def check_authorization_url(url): @pytest.mark.parametrize('exp', [timestamp_from_datetime(now() + datetime.timedelta(seconds=1000)), timestamp_from_datetime(now() - datetime.timedelta(seconds=1000))]) -def test_login(app, fc_settings, caplog, exp): - callback = reverse('fc-login-or-link') - response = app.get(callback, status=302) +def test_login_simple(app, fc_settings, caplog, hooks, exp): + response = app.get('/login/?service=portail&next=/idp/') + response = response.click(href='callback') location = response['Location'] state = check_authorization_url(location) @@ -101,17 +101,21 @@ def test_login(app, fc_settings, caplog, exp): 'given_name': u'Ÿuñe', }) + callback = reverse('fc-login-or-link') with httmock.HTTMock(access_token_response, user_info_response): - response = app.get(callback + '?code=zzz&state=%s' % state, status=302) + response = app.get(callback + '?service=portail&next=/idp/&code=zzz&state=%s' % state, status=302) assert User.objects.count() == 0 fc_settings.A2_FC_CREATE = True with httmock.HTTMock(access_token_response, user_info_response): - response = app.get(callback + '?code=zzz&state=%s' % state, status=302) + response = app.get(callback + '?service=portail&next=/idp/&code=zzz&state=%s' % state, status=302) if exp < timestamp_from_datetime(now()): assert User.objects.count() == 0 else: assert User.objects.count() == 1 if User.objects.count(): + assert response['Location'] == 'http://testserver/idp/' + assert hooks.event[1]['kwargs']['name'] == 'login' + assert hooks.event[1]['kwargs']['service'] == 'portail' # we must be connected assert app.session['_auth_user_id'] assert models.FcAccount.objects.count() == 1 @@ -273,3 +277,165 @@ def test_password_reset(app, mailoutbox): models.FcAccount.objects.create(user=user, sub='xxx', token='aaa') response = app.get(url) assert 'new_password1' in response.form.fields + + +def test_registration1(app, fc_settings, caplog, hooks): + exp = timestamp_from_datetime(now() + datetime.timedelta(seconds=1000)) + response = app.get('/login/?service=portail&next=/idp/') + response = response.click(href="callback") + # 1. Try a login + # 2. Verify we come back to login page + # 3. Check presence of registration link + # 4. Follow it + location = response['Location'] + state = check_authorization_url(location) + + @httmock.urlmatch(path=r'.*/token$') + def access_token_response(url, request): + parsed = {x: y[0] for x, y in urlparse.parse_qs(request.body).items()} + assert set(parsed.keys()) == set(['code', 'client_id', 'client_secret', 'redirect_uri', + 'grant_type']) + assert parsed['code'] == 'zzz' + assert parsed['client_id'] == 'xxx' + assert parsed['client_secret'] == 'yyy' + assert parsed['grant_type'] == 'authorization_code' + assert callback in parsed['redirect_uri'] + id_token = { + 'sub': '1234', + 'aud': 'xxx', + 'nonce': state, + 'exp': exp, + 'iss': 'https://fcp.integ01.dev-franceconnect.fr/', + 'email': 'john.doe@example.com', + } + return json.dumps({ + 'access_token': 'uuu', + 'id_token': hmac_jwt(id_token, 'yyy') + }) + + @httmock.urlmatch(path=r'.*userinfo$') + def user_info_response(url, request): + assert request.headers['Authorization'] == 'Bearer uuu' + return json.dumps({ + 'sub': '1234', + 'family_name': u'Frédérique', + 'given_name': u'Ÿuñe', + 'email': 'john.doe@example.com', + }) + + callback = urlparse.parse_qs(urlparse.urlparse(location).query)['redirect_uri'][0] + with httmock.HTTMock(access_token_response, user_info_response): + response = app.get(callback + '&code=zzz&state=%s' % state, status=302) + assert User.objects.count() == 0 + assert response['Location'].startswith('http://testserver/login/') + response = response.follow() + response = response.click('Create your account with FranceConnect') + location = response['Location'] + location.startswith('http://testserver/accounts/activate/') + response = response.follow() + assert hooks.calls['event'][0]['kwargs']['service'] == 'portail' + # we must be connected + assert app.session['_auth_user_id'] + assert response['Location'].startswith(callback) + response = response.follow() + location = response['Location'] + state = check_authorization_url(location) + with httmock.HTTMock(access_token_response, user_info_response): + response = app.get(callback + '&code=zzz&state=%s' % state, status=302) + assert models.FcAccount.objects.count() == 1 + response = app.get('/accounts/') + response = response.click('Delete link') + response.form.set('new_password1', 'ikKL1234') + response.form.set('new_password2', 'ikKL1234') + response = response.form.submit(name='unlink') + assert 'The link with the FranceConnect account has been deleted' in response.content + assert models.FcAccount.objects.count() == 0 + continue_url = response.pyquery('a#a2-continue').attr['href'] + state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] + assert app.session['fc_states'][state]['next'] == '/accounts/' + response = app.get(reverse('fc-logout') + '?state=' + state) + assert response['Location'] == 'http://testserver/accounts/' + + +def test_registration2(app, fc_settings, caplog, hooks): + exp = timestamp_from_datetime(now() + datetime.timedelta(seconds=1000)) + response = app.get('/login/?service=portail&next=/idp/') + response = response.click("Register") + response = response.click(href='callback') + # 1. Try a login + # 2. Verify we come back to login page + # 3. Check presence of registration link + # 4. Follow it + location = response['Location'] + state = check_authorization_url(location) + + @httmock.urlmatch(path=r'.*/token$') + def access_token_response(url, request): + parsed = {x: y[0] for x, y in urlparse.parse_qs(request.body).items()} + assert set(parsed.keys()) == set(['code', 'client_id', 'client_secret', 'redirect_uri', + 'grant_type']) + assert parsed['code'] == 'zzz' + assert parsed['client_id'] == 'xxx' + assert parsed['client_secret'] == 'yyy' + assert parsed['grant_type'] == 'authorization_code' + assert callback in parsed['redirect_uri'] + id_token = { + 'sub': '1234', + 'aud': 'xxx', + 'nonce': state, + 'exp': exp, + 'iss': 'https://fcp.integ01.dev-franceconnect.fr/', + 'email': 'john.doe@example.com', + } + return json.dumps({ + 'access_token': 'uuu', + 'id_token': hmac_jwt(id_token, 'yyy') + }) + + @httmock.urlmatch(path=r'.*userinfo$') + def user_info_response(url, request): + assert request.headers['Authorization'] == 'Bearer uuu' + return json.dumps({ + 'sub': '1234', + 'family_name': u'Frédérique', + 'given_name': u'Ÿuñe', + 'email': 'john.doe@example.com', + }) + + callback = urlparse.parse_qs(urlparse.urlparse(location).query)['redirect_uri'][0] + with httmock.HTTMock(access_token_response, user_info_response): + response = app.get(callback + '&code=zzz&state=%s' % state, status=302) + assert User.objects.count() == 0 + assert response['Location'].startswith('http://testserver/accounts/fc/register/') + response = response.follow() + location = response['Location'] + location.startswith('http://testserver/accounts/activate/') + response = response.follow() + assert hooks.calls['event'][0]['kwargs']['service'] == 'portail' + assert hooks.calls['event'][1]['kwargs']['service'] == 'portail' + # we must be connected + assert app.session['_auth_user_id'] + # remove the registration parameter + callback = callback.replace('®istration=', '') + callback = callback.replace('?registration=', '?') + assert response['Location'].startswith(callback) + response = response.follow() + location = response['Location'] + state = check_authorization_url(location) + with httmock.HTTMock(access_token_response, user_info_response): + response = app.get(callback + '&code=zzz&state=%s' % state, status=302) + assert models.FcAccount.objects.count() == 1 + response = app.get('/accounts/') + response = response.click('Delete link') + response.form.set('new_password1', 'ikKL1234') + response.form.set('new_password2', 'ikKL1234') + response = response.form.submit(name='unlink') + assert 'The link with the FranceConnect account has been deleted' in response.content + assert models.FcAccount.objects.count() == 0 + continue_url = response.pyquery('a#a2-continue').attr['href'] + state = urlparse.parse_qs(urlparse.urlparse(continue_url).query)['state'][0] + assert app.session['fc_states'][state]['next'] == '/accounts/' + response = app.get(reverse('fc-logout') + '?state=' + state) + assert response['Location'] == 'http://testserver/accounts/' + import pdb + pdb.set_trace() -- 2.18.0