From c959678c49e6a5277de66ff94d8b99d48c21d226 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 23 Feb 2016 15:45:19 +0000 Subject: [PATCH] Remember me functionality added and tested. Merge extra. Fixed comment. --- app/main/forms.py | 4 +++- app/main/views/sign_in.py | 17 ++++++++++++-- app/main/views/sign_out.py | 3 +-- app/main/views/two_factor.py | 7 ++++-- app/notify_client/user_api_client.py | 8 ++++++- app/templates/components/checkbox.html | 30 +++++++++++++++++++++++++ app/templates/views/two-factor.html | 2 ++ config.py | 11 +++++++-- tests/app/main/views/test_sign_in.py | 23 +++++++++++++++++++ tests/app/main/views/test_sign_out.py | 11 ++++----- tests/app/main/views/test_two_factor.py | 17 ++++++++++++++ 11 files changed, 118 insertions(+), 15 deletions(-) create mode 100644 app/templates/components/checkbox.html diff --git a/app/main/forms.py b/app/main/forms.py index ea2e1cd69..a2885fe8f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -7,7 +7,8 @@ from wtforms import ( ValidationError, TextAreaField, FileField, - RadioField + RadioField, + BooleanField ) from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import DataRequired, Email, Length, Regexp @@ -110,6 +111,7 @@ class TwoFactorForm(Form): super(TwoFactorForm, self).__init__(*args, **kwargs) sms_code = sms_code() + remember_me = BooleanField("Remember me") def validate_sms_code(self, field): is_valid, reason = self.validate_code_func(field.data) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index fc27a366e..c2931b1cd 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -7,10 +7,10 @@ from flask import ( flash ) -from flask.ext.login import current_user +from flask.ext.login import (current_user, login_fresh, confirm_login) from app.main import main -from app.main.dao import users_dao +from app.main.dao import (users_dao, services_dao) from app.main.forms import LoginForm @@ -18,11 +18,24 @@ from app.main.forms import LoginForm def sign_in(): if current_user and current_user.is_authenticated(): return redirect(url_for('main.choose_service')) + form = LoginForm() if form.validate_on_submit(): user = users_dao.get_user_by_email(form.email_address.data) user = _get_and_verify_user(user, form.password.data) if user: + # Remember me login + if not login_fresh() and \ + not current_user.is_anonymous() and \ + current_user.id == user.id and \ + user.is_active(): + confirm_login() + services = services_dao.get_services(user.id).get('data', []) + if (len(services) == 1): + return redirect(url_for('main.service_dashboard', service_id=services[0]['id'])) + else: + return redirect(url_for('main.choose_service')) + session['user_details'] = {"email": user.email_address, "id": user.id} if user.state == 'pending': return redirect(url_for('.verify')) diff --git a/app/main/views/sign_out.py b/app/main/views/sign_out.py index de0ca58e1..cbe2738e3 100644 --- a/app/main/views/sign_out.py +++ b/app/main/views/sign_out.py @@ -6,8 +6,7 @@ from app.main import main @main.route('/sign-out', methods=(['GET'])) -@login_required def sign_out(): session.clear() logout_user() - return redirect(url_for('main.index')) + return redirect(url_for('main.sign_in')) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 5a7af0112..325225ae2 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -12,7 +12,10 @@ from app.main.forms import TwoFactorForm @main.route('/two-factor', methods=['GET', 'POST']) def two_factor(): # TODO handle user_email not in session - user_id = session['user_details']['id'] + try: + user_id = session['user_details']['id'] + except KeyError: + return redirect('main.sign_in') def _check_code(code): return users_dao.check_verify_code(user_id, code, "sms") @@ -27,7 +30,7 @@ def two_factor(): if 'password' in session['user_details']: user.set_password(session['user_details']['password']) users_dao.update_user(user) - login_user(user) + login_user(user, remember=form.remember_me.data if form.remember_me.data else False) finally: del session['user_details'] if (len(services) == 1): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 693a321f7..937a97834 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,7 +1,7 @@ from notifications_python_client.notifications import BaseAPIClient from notifications_python_client.errors import HTTPError -from flask.ext.login import UserMixin +from flask.ext.login import (UserMixin, login_fresh) class UserApiClient(BaseAPIClient): @@ -100,6 +100,12 @@ class User(UserMixin): def is_active(self): return self.state == 'active' + def is_authenticated(self): + # To handle remember me token renewal + if not login_fresh(): + return False + return super(User, self).is_authenticated() + @property def id(self): return self._id diff --git a/app/templates/components/checkbox.html b/app/templates/components/checkbox.html new file mode 100644 index 000000000..e816f8aeb --- /dev/null +++ b/app/templates/components/checkbox.html @@ -0,0 +1,30 @@ +{% macro checkbox( + field, + hint=False, + help_link=None, + help_link_text=None, + width='2-3', + suffix=None +) %} + +{% endmacro %} \ No newline at end of file diff --git a/app/templates/views/two-factor.html b/app/templates/views/two-factor.html index bbc121bc9..19126706e 100644 --- a/app/templates/views/two-factor.html +++ b/app/templates/views/two-factor.html @@ -1,5 +1,6 @@ {% extends "withoutnav_template.html" %} {% from "components/textbox.html" import textbox %} +{% from "components/checkbox.html" import checkbox %} {% from "components/page-footer.html" import page_footer %} {% block page_title %} @@ -22,6 +23,7 @@ help_link=url_for('.verification_code_not_received'), help_link_text='I haven’t received a text message' ) }} + {{ checkbox(form.remember_me) }} {{ page_footer( "Continue" ) }} diff --git a/config.py b/config.py index 999949b7b..af641a7b7 100644 --- a/config.py +++ b/config.py @@ -1,4 +1,5 @@ import os +from datetime import timedelta class Config(object): @@ -21,9 +22,14 @@ class Config(object): SESSION_COOKIE_NAME = 'notify_admin_session' SESSION_COOKIE_PATH = '/admin' SESSION_COOKIE_HTTPONLY = True - SESSION_COOKIE_SECURE = False + SESSION_COOKIE_SECURE = True PERMANENT_SESSION_LIFETIME = 3600 # seconds SESSION_REFRESH_EACH_REQUEST = True + REMEMBER_COOKIE_NAME = 'notify_admin_remember_me' + REMEMBER_COOKIE_PATH = '/admin' + REMEMBER_COOKIE_DURATION = timedelta(days=1) + REMEMBER_COOKIE_HTTPONLY = True + REMEMBER_COOKIE_SECURE = True API_HOST_NAME = os.getenv('API_HOST_NAME') NOTIFY_API_SECRET = os.getenv('NOTIFY_API_SECRET', "dev-secret") @@ -54,6 +60,8 @@ class Development(Config): ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' WTF_CSRF_ENABLED = False + REMEMBER_COOKIE_SECURE = False + SESSION_COOKIE_SECURE = False class Test(Development): @@ -64,7 +72,6 @@ class Test(Development): class Preview(Config): DEBUG = False HTTP_PROTOCOL = 'https' - SESSION_COOKIE_SECURE = True HEADER_COLOUR = '#F47738' # $orange diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 1d72b6a00..ef8569338 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -90,3 +90,26 @@ def test_should_return_redirect_when_user_is_pending(app_, 'password': 'val1dPassw0rd!'}) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) + + +def test_not_fresh_session_login(app_, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_verify_password, + mock_get_services_with_one_service): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + with client.session_transaction() as session: + assert session['_fresh'] + session['_fresh'] = False + # This should skip the two factor + response = client.post( + url_for('main.sign_in'), data={ + 'email_address': api_user_active.email_address, + 'password': 'val1dPassw0rd!'}) + assert response.status_code == 302 + service_dct = mock_get_services_with_one_service(api_user_active.id)['data'][0] + assert response.location == url_for( + 'main.service_dashboard', service_id=service_dct['id'], _external=True) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 1be46f423..f4d7019a8 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -7,7 +7,7 @@ def test_render_sign_out_redirects_to_sign_in(app_): url_for('main.sign_out')) assert response.status_code == 302 assert response.location == url_for( - 'main.sign_in', _external=True, next=url_for('main.sign_out')) + 'main.sign_in', _external=True) def test_sign_out_user(app_, @@ -22,9 +22,9 @@ def test_sign_out_user(app_, email = 'valid@example.gov.uk' password = 'val1dPassw0rd!' with app_.test_client() as client: - with client.session_transaction() as session: - print('session: {}'.format(session)) client.login(api_user_active) + with client.session_transaction() as session: + assert session.get('user_id') is not None # Check we are logged in response = client.get( url_for('main.service_dashboard', service_id="123")) @@ -32,5 +32,6 @@ def test_sign_out_user(app_, response = client.get(url_for('main.sign_out')) assert response.status_code == 302 assert response.location == url_for( - 'main.index', _external=True) - assert session.get('ItsdangerousSession') is None + 'main.sign_in', _external=True) + with client.session_transaction() as session: + assert session.get('user_id') is None diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 964842482..a89d8b03c 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -92,3 +92,20 @@ def test_should_login_user_when_multiple_valid_codes_exist(app_, response = client.post(url_for('main.two_factor'), data={'sms_code': '23456'}) assert response.status_code == 302 + + +def test_remember_me_set(app_, + api_user_active, + mock_get_user, + mock_get_user_by_email, + mock_check_verify_code, + mock_get_services_with_one_service): + with app_.test_request_context(): + with app_.test_client() as client: + with client.session_transaction() as session: + session['user_details'] = { + 'id': api_user_active.id, + 'email': api_user_active.email_address} + response = client.post(url_for('main.two_factor'), + data={'sms_code': '23456', 'remember_me': True}) + assert response.status_code == 302