From 65072e41d32460be60804321fe04ac53d6ca0afa Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 8 Sep 2016 16:57:06 +0100 Subject: [PATCH 1/4] Fix issue where exception thrown when user does not activate email but successfully completes forgotten-password flow (which includes 2f) --- app/main/views/sign_in.py | 1 - app/main/views/two_factor.py | 5 ++++- app/utils.py | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index cdbc31c8f..524a6339e 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -28,7 +28,6 @@ from app.main.forms import LoginForm @main.route('/sign-in', methods=(['GET', 'POST'])) def sign_in(): - if current_user and current_user.is_authenticated: return redirect(url_for('main.choose_service')) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index a728aea19..54a8400f9 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -24,6 +24,7 @@ def two_factor(): form = TwoFactorForm(_check_code) if form.validate_on_submit(): + import pdb; pdb.set_trace() try: user = user_api_client.get_user(user_id) services = service_api_client.get_services({'user_id': str(user_id)}).get('data', []) @@ -32,7 +33,9 @@ def two_factor(): user.set_password(session['user_details']['password']) user.reset_failed_login_count() user_api_client.update_user(user) - login_user(user, remember=True) + + activated_user = user_api_client.activate_user(user) + login_user(activated_user, remember=True) finally: del session['user_details'] diff --git a/app/utils.py b/app/utils.py index 98178fd7f..aee92effd 100644 --- a/app/utils.py +++ b/app/utils.py @@ -54,6 +54,7 @@ def user_has_permissions(*permissions, admin_override=False, any_=False): def redirect_to_sign_in(f): @wraps(f) def wrapped(*args, **kwargs): + import pdb; pdb.set_trace() if 'user_details' not in session: return redirect(url_for('main.sign_in')) else: From 171eec5984cea5a2710ad9e7bc80e9a62d562166 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 8 Sep 2016 16:59:32 +0100 Subject: [PATCH 2/4] Remove pdb breaks --- app/main/views/two_factor.py | 1 - app/utils.py | 1 - 2 files changed, 2 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 54a8400f9..72359ecb8 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -24,7 +24,6 @@ def two_factor(): form = TwoFactorForm(_check_code) if form.validate_on_submit(): - import pdb; pdb.set_trace() try: user = user_api_client.get_user(user_id) services = service_api_client.get_services({'user_id': str(user_id)}).get('data', []) diff --git a/app/utils.py b/app/utils.py index aee92effd..98178fd7f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -54,7 +54,6 @@ def user_has_permissions(*permissions, admin_override=False, any_=False): def redirect_to_sign_in(f): @wraps(f) def wrapped(*args, **kwargs): - import pdb; pdb.set_trace() if 'user_details' not in session: return redirect(url_for('main.sign_in')) else: From 9eab8a726f5ede53c003da2bb79ad69934e1b4b0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 9 Sep 2016 15:22:56 +0100 Subject: [PATCH 3/4] - Add test to check that two-factor auth activates a user as expected - Ensure DB user activation statusupdate only executed when required - Fix test_should_activate_user_after_verify --- app/main/views/two_factor.py | 1 + app/notify_client/user_api_client.py | 7 +++++-- tests/app/main/views/test_two_factor.py | 21 +++++++++++++++++++++ tests/app/main/views/test_verify.py | 7 ++++--- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 72359ecb8..81cb0ef20 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -26,6 +26,7 @@ def two_factor(): if form.validate_on_submit(): try: user = user_api_client.get_user(user_id) + services = service_api_client.get_services({'user_id': str(user_id)}).get('data', []) # Check if coming from new password page if 'password' in session['user_details']: diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 8c43f2eeb..5bd534869 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -124,5 +124,8 @@ class UserApiClient(BaseAPIClient): return True def activate_user(self, user): - user.state = 'active' - return self.update_user(user) + if user.state == 'pending': + user.state = 'active' + return self.update_user(user) + else: + return user diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index cf234483f..2a5421cf0 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -222,3 +222,24 @@ def test_two_factor_should_redirect_to_sign_in_if_user_not_in_session(app_, data={'sms_code': '12345'}) assert response.status_code == 302 assert response.location == url_for('main.sign_in', _external=True) + + +def test_two_factor_should_activate_pending_user(app_, + mocker, + api_user_pending, + mock_check_verify_code, + mock_update_user + ): + mocker.patch('app.user_api_client.get_user', return_value=api_user_pending) + mocker.patch('app.service_api_client.get_services', return_value={'data': []}) + with app_.test_request_context(): + with app_.test_client() as client: + with client.session_transaction() as session: + session['user_details'] = { + 'id': api_user_pending.id, + 'email_address': api_user_pending.email_address + } + client.post(url_for('main.two_factor'), data={'sms_code': '12345'}) + + assert mock_update_user.called + assert api_user_pending.is_active diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 4aaab069b..97f2d4da8 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -39,15 +39,16 @@ def test_should_redirect_to_add_service_when_sms_code_is_correct(app_, def test_should_activate_user_after_verify(app_, - api_user_active, - mock_get_user, + mocker, + api_user_pending, mock_send_verify_code, mock_check_verify_code, mock_update_user): + mocker.patch('app.user_api_client.get_user', return_value=api_user_pending) with app_.test_request_context(): with app_.test_client() as client: with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} client.post(url_for('main.verify'), data={'sms_code': '12345'}) assert mock_update_user.called From defa7ac2c80b91fec447808fb0bd4abd01426640 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 9 Sep 2016 15:24:56 +0100 Subject: [PATCH 4/4] Remove spacing --- app/main/views/two_factor.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 81cb0ef20..022a189f5 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -26,14 +26,12 @@ def two_factor(): if form.validate_on_submit(): try: user = user_api_client.get_user(user_id) - services = service_api_client.get_services({'user_id': str(user_id)}).get('data', []) # Check if coming from new password page if 'password' in session['user_details']: user.set_password(session['user_details']['password']) user.reset_failed_login_count() user_api_client.update_user(user) - activated_user = user_api_client.activate_user(user) login_user(activated_user, remember=True) finally: