From 937c9f2adc1eeb1a06628fd834c540c5d7aa6030 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 31 Jan 2020 15:50:55 +0000 Subject: [PATCH] Ensure that the session is logged out server side, not just client side. Anytime a user clicks "sign out" we should be signing them out server side as well. This can be accomplished by setting the Users.current_session_id = null. I found that the method User.logged_in_elsewhere doesn't need to check if the current_session_id is None. The current_session_ids in the cookie and db (redis or postgres) then the user should be forced to log in again. --- app/main/views/sign_out.py | 3 ++- app/models/user.py | 7 +++++-- app/notify_client/user_api_client.py | 3 ++- tests/app/main/views/test_sign_in.py | 3 +-- tests/app/main/views/test_sign_out.py | 4 ++-- 5 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/main/views/sign_out.py b/app/main/views/sign_out.py index 39c50b5ff..d2ee33b9a 100644 --- a/app/main/views/sign_out.py +++ b/app/main/views/sign_out.py @@ -1,11 +1,12 @@ from flask import redirect, session, url_for -from flask_login import logout_user +from flask_login import current_user, logout_user from app.main import main @main.route('/sign-out', methods=(['GET'])) def sign_out(): + current_user.sign_out() session.clear() logout_user() return redirect(url_for('main.index')) diff --git a/app/models/user.py b/app/models/user.py index a1876d765..c66ff44e3 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -127,8 +127,7 @@ class User(JSONModel, UserMixin): ) def logged_in_elsewhere(self): - # if the current user (ie: db object) has no session, they've never logged in before - return self.current_session_id is not None and session.get('current_session_id') != self.current_session_id + return session.get('current_session_id') != self.current_session_id def activate(self): if self.state == 'pending': @@ -154,6 +153,10 @@ class User(JSONModel, UserMixin): return True + def sign_out(self): + # Update the db so the server also knows the user is logged out. + return self.update(current_session_id=None) + @property def sms_auth(self): return self.auth_type == 'sms_auth' diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index ababbe5b5..67cd572e7 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -10,7 +10,8 @@ ALLOWED_ATTRIBUTES = { 'email_address', 'mobile_number', 'auth_type', - 'updated_by' + 'updated_by', + 'current_session_id' } diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index de396ca8c..fa2c98b18 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -60,8 +60,7 @@ def test_doesnt_redirect_to_sign_in_if_no_session_info( @pytest.mark.parametrize('db_sess_id, cookie_sess_id', [ - pytest.param(None, None, marks=pytest.mark.xfail), # OK - not used notify since browser signout was implemented - + (None, None), (uuid.UUID(int=1), None), # BAD - has used other browsers before but this is a brand new browser with no cookie (uuid.UUID(int=1), uuid.UUID(int=2)), # BAD - this person has just signed in on a different browser ]) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 7f0506c12..d48affb7e 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -4,9 +4,9 @@ from tests.conftest import SERVICE_ONE_ID def test_render_sign_out_redirects_to_sign_in( - client + logged_in_client ): - response = client.get( + response = logged_in_client.get( url_for('main.sign_out')) assert response.status_code == 302 assert response.location == url_for(