From 9fda5d18473d43ed8bc716bb8e5ba10517c0aea3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 23 Feb 2017 16:43:09 +0000 Subject: [PATCH 1/2] remove remember_me cookie and related code --- app/__init__.py | 5 ++- app/config.py | 5 --- app/event_handlers.py | 5 --- app/main/views/sign_in.py | 14 --------- app/notify_client/models.py | 3 +- app/templates/views/cookies.html | 14 +-------- scripts/run_tests.sh | 2 +- tests/__init__.py | 4 --- tests/app/main/views/test_sign_in.py | 42 ------------------------- tests/app/main/views/test_two_factor.py | 17 ---------- tests/app/test_event_handlers.py | 18 +---------- 11 files changed, 6 insertions(+), 123 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index d4193822a..fbd736688 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -471,8 +471,7 @@ def register_errorhandlers(application): def setup_event_handlers(): - from flask.ext.login import user_logged_in, user_login_confirmed - from app.event_handlers import on_user_logged_in, on_user_login_confirmed + from flask.ext.login import user_logged_in + from app.event_handlers import on_user_logged_in user_logged_in.connect(on_user_logged_in) - user_login_confirmed.connect(on_user_login_confirmed) diff --git a/app/config.py b/app/config.py index 7bad978ef..95ae63c08 100644 --- a/app/config.py +++ b/app/config.py @@ -39,10 +39,6 @@ class Config(object): NOTIFY_LOG_LEVEL = 'DEBUG' NOTIFY_LOG_PATH = '/var/log/notify/application.log' PERMANENT_SESSION_LIFETIME = 20 * 60 * 60 # 20 hours - REMEMBER_COOKIE_DURATION = timedelta(days=1) - REMEMBER_COOKIE_HTTPONLY = True - REMEMBER_COOKIE_NAME = 'notify_admin_remember_me' - REMEMBER_COOKIE_SECURE = True SEND_FILE_MAX_AGE_DEFAULT = 365 * 24 * 60 * 60 # 1 year SESSION_COOKIE_HTTPONLY = True SESSION_COOKIE_NAME = 'notify_admin_session' @@ -81,7 +77,6 @@ class Config(object): class Development(Config): DEBUG = True - REMEMBER_COOKIE_SECURE = False SESSION_COOKIE_SECURE = False WTF_CSRF_ENABLED = False SESSION_PROTECTION = None diff --git a/app/event_handlers.py b/app/event_handlers.py index 90c9e78fe..8fc3a815d 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -6,11 +6,6 @@ def on_user_logged_in(sender, user): _send_event(sender, event_type='sucessful_login', user=user) -# should change the event type? This is a remember me login. -def on_user_login_confirmed(sender): - _send_event(sender, event_type='sucessful_login_remember_me', user=current_user) - - def _send_event(sender, **kwargs): from flask import request try: diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index bbc901166..73108b4b4 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -11,7 +11,6 @@ from flask import ( from flask_login import ( current_user, - login_fresh, confirm_login ) @@ -49,19 +48,6 @@ def sign_in(): else: invite_api_client.accept_invite(invited_user['service'], invited_user['id']) 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 = service_api_client.get_active_services({'user_id': str(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.is_active: user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index e9fc8209b..0ca6d8991 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -1,4 +1,4 @@ -from flask_login import UserMixin, AnonymousUserMixin, login_fresh +from flask_login import UserMixin, AnonymousUserMixin from flask import session @@ -30,7 +30,6 @@ class User(UserMixin): @property def is_authenticated(self): return ( - login_fresh() and not self.logged_in_elsewhere() and super(User, self).is_authenticated ) diff --git a/app/templates/views/cookies.html b/app/templates/views/cookies.html index de20bbfc4..2c8d2f6da 100644 --- a/app/templates/views/cookies.html +++ b/app/templates/views/cookies.html @@ -40,21 +40,9 @@ Used to keep you logged in - 1 hour + 20 hours - - - notify_admin_remember_me - - - Used to let you sign in again on the same day, without requiring 2-factor authentication - - - 24 hours - - - diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index 1e478e15b..01e109020 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -34,5 +34,5 @@ npm test display_result $? 2 "Front end code style check" ## Code coverage -py.test -n2 --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml +py.test -n2 --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml --strict display_result $? 3 "Code coverage" diff --git a/tests/__init__.py b/tests/__init__.py index 816c1e702..bd12a591c 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -13,7 +13,6 @@ class TestClient(FlaskClient): # Skipping authentication here and just log them in with self.session_transaction() as session: session['user_id'] = user.id - session['_fresh'] = True if mocker: mocker.patch('app.user_api_client.get_user', return_value=user) mocker.patch('app.events_api_client.create_event') @@ -23,9 +22,6 @@ class TestClient(FlaskClient): mocker.patch('app.service_api_client.get_service', return_value={'data': service}) login_user(user, remember=True) - def login_fresh(self): - return True - def logout(self, user): self.get(url_for("main.logout")) diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 4bed768be..a8d2f1d93 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -144,45 +144,3 @@ def test_should_attempt_redirect_when_user_is_pending( 'password': 'val1dPassw0rd!'}) assert response.location == url_for('main.resend_email_verification', _external=True) assert response.status_code == 302 - - -def test_not_fresh_session_login_redirects_to_dashboard( - logged_in_client, - api_user_active, - mock_login, - mock_get_user_by_email, - mock_verify_password, - mock_get_services_with_one_service, -): - with logged_in_client.session_transaction() as session: - assert session['_fresh'] - session['_fresh'] = False - # This should skip the two factor - response = logged_in_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) - - -def test_not_fresh_session_login_redirects_to_choose_service( - logged_in_client, - api_user_active, - mock_login, - mock_get_user_by_email, - mock_verify_password, - mock_get_services, -): - with logged_in_client.session_transaction() as session: - assert session['_fresh'] - session['_fresh'] = False - # This should skip the two factor - response = logged_in_client.post( - url_for('main.sign_in'), data={ - 'email_address': api_user_active.email_address, - 'password': 'val1dPassw0rd!'}) - assert response.status_code == 302 - assert response.location == url_for('main.choose_service', _external=True) diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 6f700cda7..77b23d569 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -141,23 +141,6 @@ def test_should_login_user_when_multiple_valid_codes_exist( assert response.status_code == 302 -def test_remember_me_set( - client, - api_user_active, - mock_get_user, - mock_get_user_by_email, - mock_check_verify_code, - mock_get_services_with_one_service, -): - 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 - - def test_two_factor_should_set_password_when_new_password_exists_in_session( client, api_user_active, diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 30a4859f6..7f1ae1fb7 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -1,9 +1,6 @@ from unittest.mock import ANY -from app.event_handlers import ( - on_user_logged_in, - on_user_login_confirmed -) +from app.event_handlers import on_user_logged_in def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): @@ -14,16 +11,3 @@ def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): {'browser_fingerprint': {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, 'ip_address': ANY, 'user_id': str(api_user_active.id)}) - - -def test_on_user_login_confirmed_in_calls_events_api(app_, api_user_active, mock_events): - - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) # user must have been logged in already. - - on_user_login_confirmed(app_) - mock_events.assert_called_with('sucessful_login_remember_me', - {'browser_fingerprint': - {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, - 'ip_address': ANY, 'user_id': str(api_user_active.id)}) From 85efe0d117c29daae7a1f2e9eaf5751d964905e7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 2 Mar 2017 16:55:10 +0000 Subject: [PATCH 2/2] remove flask's builtin remember me functionality we don't need it cos we do it all ourselves --- app/main/views/two_factor.py | 2 +- tests/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 05896ee8f..c91836742 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -36,7 +36,7 @@ def two_factor(): form.sms_code.errors.append('Code not found') return render_template('views/two-factor.html', form=form) activated_user = user_api_client.activate_user(user) - login_user(activated_user, remember=True) + login_user(activated_user) finally: del session['user_details'] diff --git a/tests/__init__.py b/tests/__init__.py index bd12a591c..a7bb18a58 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -20,7 +20,7 @@ class TestClient(FlaskClient): with self.session_transaction() as session: session['service_id'] = service['id'] mocker.patch('app.service_api_client.get_service', return_value={'data': service}) - login_user(user, remember=True) + login_user(user) def logout(self, user): self.get(url_for("main.logout"))