Merge pull request #834 from alphagov/reset-2fa

ensure we reset failed_login_count when appropriate

app/dao/users_dao.py

@@ -113,6 +113,7 @@ def reset_failed_login_count(user):
 
 
 def update_user_password(user, password):
+    # reset failed login count - they've just reset their password so should be fine
     user.password = password
     user.password_changed_at = datetime.utcnow()
     db.session.add(user)

app/user/rest.py

@@ -59,12 +59,14 @@ def update_user(user_id):
     user_to_update = get_user_by_id(user_id=user_id)
     req_json = request.get_json()
     update_dct, errors = user_schema_load_json.load(req_json)
+    # TODO don't let password be updated in this PUT method (currently used by the forgot password flow)
     pwd = req_json.get('password', None)
-    # TODO password validation, it is already done on the admin app
+    if pwd is not None:
-    # but would be good to have the same validation here.
+        if not pwd:
-    if pwd is not None and not pwd:
             errors.update({'password': ['Invalid data for field']})
             raise InvalidRequest(errors, status_code=400)
+        else:
+            reset_failed_login_count(user_to_update)
     save_model_user(user_to_update, update_dict=update_dct, pwd=pwd)
     return jsonify(data=user_schema.dump(user_to_update).data), 200
 
@@ -130,6 +132,7 @@ def verify_user_code(user_id):
         increment_failed_login_count(user_to_verify)
         raise InvalidRequest("Code has expired", status_code=400)
     use_user_code(code.id)
+    reset_failed_login_count(user_to_verify)
     return jsonify({}), 204
 
 
@@ -323,6 +326,7 @@ def update_password(user_id):
     if errors:
         raise InvalidRequest(errors, status_code=400)
 
+    reset_failed_login_count(user)
     update_user_password(user, pwd)
     return jsonify(data=user_schema.dump(user).data), 200
 

tests/app/user/test_rest.py

@@ -10,12 +10,10 @@ from app.dao.permissions_dao import default_service_permissions
 from tests import create_authorization_header
 
 
-def test_get_user_list(notify_api, notify_db, notify_db_session, sample_service):
+def test_get_user_list(client, sample_service):
     """
     Tests GET endpoint '/' to retrieve entire user list.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     header = create_authorization_header()
     response = client.get(url_for('user.get_user'),
                           headers=[header])
@@ -34,12 +32,10 @@ def test_get_user_list(notify_api, notify_db, notify_db_session, sample_service)
     assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)])
 
 
-def test_get_user(notify_api, notify_db, notify_db_session, sample_service):
+def test_get_user(client, sample_service):
     """
     Tests GET endpoint '/<user_id>' to retrieve a single service.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     sample_user = sample_service.users[0]
     header = create_authorization_header()
     resp = client.get(url_for('user.get_user',
@@ -59,12 +55,10 @@ def test_get_user(notify_api, notify_db, notify_db_session, sample_service):
     assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)])
 
 
-def test_post_user(notify_api, notify_db, notify_db_session):
+def test_post_user(client, notify_db, notify_db_session):
     """
     Tests POST endpoint '/' to create a user.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 0
     data = {
         "name": "Test User",
@@ -89,12 +83,10 @@ def test_post_user(notify_api, notify_db, notify_db_session):
     assert json_resp['data']['id'] == str(user.id)
 
 
-def test_post_user_missing_attribute_email(notify_api, notify_db, notify_db_session):
+def test_post_user_missing_attribute_email(client, notify_db, notify_db_session):
     """
     Tests POST endpoint '/' missing attribute email.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 0
     data = {
         "name": "Test User",
@@ -117,12 +109,10 @@ def test_post_user_missing_attribute_email(notify_api, notify_db, notify_db_sess
     assert {'email_address': ['Missing data for required field.']} == json_resp['message']
 
 
-def test_create_user_missing_attribute_password(notify_api, notify_db, notify_db_session):
+def test_create_user_missing_attribute_password(client, notify_db, notify_db_session):
     """
     Tests POST endpoint '/' missing attribute password.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 0
     data = {
         "name": "Test User",
@@ -145,14 +135,13 @@ def test_create_user_missing_attribute_password(notify_api, notify_db, notify_db
     assert {'password': ['Missing data for required field.']} == json_resp['message']
 
 
-def test_put_user(notify_api, notify_db, notify_db_session, sample_service):
+def test_put_user(client, sample_service):
     """
     Tests PUT endpoint '/' to update a user.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 1
     sample_user = sample_service.users[0]
+    sample_user.failed_login_count = 1
     new_email = 'new@digital.cabinet-office.gov.uk'
     data = {
         'name': sample_user.name,
@@ -178,6 +167,8 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service):
     assert new_email == fetched['email_address']
     assert sample_user.state == fetched['state']
     assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)])
+    # password wasn't updated, so failed_login_count stays the same
+    assert sample_user.failed_login_count == 1
 
 
 @pytest.mark.parametrize('user_attribute, user_value', [
@@ -203,15 +194,10 @@ def test_post_user_attribute(client, sample_user, user_attribute, user_value):
     assert json_resp['data'][user_attribute] == user_value
 
 
-def test_put_user_update_password(notify_api,
+def test_put_user_update_password(client, sample_service):
-                                  notify_db,
-                                  notify_db_session,
-                                  sample_service):
     """
     Tests PUT endpoint '/' to update a user including their password.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 1
     sample_user = sample_service.users[0]
     new_password = '1234567890'
@@ -241,12 +227,10 @@ def test_put_user_update_password(notify_api,
     assert resp.status_code == 204
 
 
-def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_user, fake_uuid):
+def test_put_user_not_exists(client, sample_user, fake_uuid):
     """
     Tests PUT endpoint '/' to update a user doesn't exist.
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     assert User.query.count() == 1
     new_email = 'new@digital.cabinet-office.gov.uk'
     data = {'email_address': new_email}
@@ -267,10 +251,7 @@ def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_us
     assert user.email_address != new_email
 
 
-def test_get_user_by_email(notify_api, notify_db, notify_db_session, sample_service):
+def test_get_user_by_email(client, sample_service):
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     sample_user = sample_service.users[0]
     header = create_authorization_header()
     url = url_for('user.get_by_email', email=sample_user.email_address)
@@ -289,13 +270,7 @@ def test_get_user_by_email(notify_api, notify_db, notify_db_session, sample_serv
     assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)])
 
 
-def test_get_user_by_email_not_found_returns_404(notify_api,
+def test_get_user_by_email_not_found_returns_404(client, sample_user):
-                                                 notify_db,
-                                                 notify_db_session,
-                                                 sample_user):
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     header = create_authorization_header()
     url = url_for('user.get_by_email', email='no_user@digital.gov.uk')
     resp = client.get(url, headers=[header])
@@ -305,13 +280,7 @@ def test_get_user_by_email_not_found_returns_404(notify_api,
     assert json_resp['message'] == 'No result found'
 
 
-def test_get_user_by_email_bad_url_returns_404(notify_api,
+def test_get_user_by_email_bad_url_returns_404(client, sample_user):
-                                               notify_db,
-                                               notify_db_session,
-                                               sample_user):
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     header = create_authorization_header()
     url = '/user/email'
     resp = client.get(url, headers=[header])
@@ -321,12 +290,7 @@ def test_get_user_by_email_bad_url_returns_404(notify_api,
     assert json_resp['message'] == 'Invalid request. Email query string param required'
 
 
-def test_get_user_with_permissions(notify_api,
+def test_get_user_with_permissions(client, sample_service_permission):
-                                   notify_db,
-                                   notify_db_session,
-                                   sample_service_permission):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     header = create_authorization_header()
     response = client.get(url_for('user.get_user', user_id=str(sample_service_permission.user.id)),
                           headers=[header])
@@ -335,13 +299,7 @@ def test_get_user_with_permissions(notify_api,
     assert sample_service_permission.permission in permissions[str(sample_service_permission.service.id)]
 
 
-def test_set_user_permissions(notify_api,
+def test_set_user_permissions(client, sample_user, sample_service):
-                              notify_db,
-                              notify_db_session,
-                              sample_user,
-                              sample_service):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     data = json.dumps([{'permission': MANAGE_SETTINGS}])
     header = create_authorization_header()
     headers = [('Content-Type', 'application/json'), header]
@@ -360,13 +318,7 @@ def test_set_user_permissions(notify_api,
     assert permission.permission == MANAGE_SETTINGS
 
 
-def test_set_user_permissions_multiple(notify_api,
+def test_set_user_permissions_multiple(client, sample_user, sample_service):
-                                       notify_db,
-                                       notify_db_session,
-                                       sample_user,
-                                       sample_service):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     data = json.dumps([{'permission': MANAGE_SETTINGS}, {'permission': MANAGE_TEMPLATES}])
     header = create_authorization_header()
     headers = [('Content-Type', 'application/json'), header]
@@ -389,13 +341,7 @@ def test_set_user_permissions_multiple(notify_api,
     assert permission.permission == MANAGE_TEMPLATES
 
 
-def test_set_user_permissions_remove_old(notify_api,
+def test_set_user_permissions_remove_old(client, sample_user, sample_service):
-                                         notify_db,
-                                         notify_db_session,
-                                         sample_user,
-                                         sample_service):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     data = json.dumps([{'permission': MANAGE_SETTINGS}])
     header = create_authorization_header()
     headers = [('Content-Type', 'application/json'), header]
@@ -446,8 +392,7 @@ def test_send_user_reset_password_should_return_400_when_email_is_missing(client
     assert mocked.call_count == 0
 
 
-def test_send_user_reset_password_should_return_400_when_user_doesnot_exist(client,
+def test_send_user_reset_password_should_return_400_when_user_doesnot_exist(client, mocker):
-                                                                            mocker):
     mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
     bad_email_address = 'bad@email.gov.uk'
     data = json.dumps({'email': bad_email_address})
@@ -535,7 +480,6 @@ def test_send_user_confirm_new_email_returns_400_when_email_missing(client, samp
 
 
 def test_update_user_password_saves_correctly(client, sample_service):
-    assert User.query.count() == 1
     sample_user = sample_service.users[0]
     new_password = '1234567890'
     data = {
@@ -548,7 +492,7 @@ def test_update_user_password_saves_correctly(client, sample_service):
         data=json.dumps(data),
         headers=headers)
     assert resp.status_code == 200
-    assert User.query.count() == 1
+
     json_resp = json.loads(resp.get_data(as_text=True))
     assert json_resp['data']['password_changed_at'] is not None
     data = {'password': new_password}
@@ -559,3 +503,36 @@ def test_update_user_password_saves_correctly(client, sample_service):
         data=json.dumps(data),
         headers=headers)
     assert resp.status_code == 204
+
+
+def test_update_user_password_resets_failed_login_count(client, sample_service):
+    user = sample_service.users[0]
+    user.failed_login_count = 1
+
+    resp = client.post(
+        url_for('user.update_password', user_id=user.id),
+        data=json.dumps({'_password': 'foo'}),
+        headers=[('Content-Type', 'application/json'), create_authorization_header()]
+    )
+
+    assert resp.status_code == 200
+    assert user.failed_login_count == 0
+
+
+def test_update_user_resets_failed_login_count_if_updating_password(client, sample_service):
+    user = sample_service.users[0]
+    user.failed_login_count = 1
+
+    resp = client.put(
+        url_for('user.update_user', user_id=user.id),
+        data=json.dumps({
+            'name': user.name,
+            'email_address': user.email_address,
+            'mobile_number': user.mobile_number,
+            'password': 'foo'
+        }),
+        headers=[('Content-Type', 'application/json'), create_authorization_header()]
+    )
+
+    assert resp.status_code == 200
+    assert user.failed_login_count == 0

tests/app/user/test_rest_verify.py

@@ -1,27 +1,24 @@
 import json
 import uuid
-
-import pytest
-
 from datetime import (
     datetime,
     timedelta
 )
 
+import pytest
 from flask import url_for, current_app
+from freezegun import freeze_time
+
 from app.dao.services_dao import dao_update_service, dao_fetch_service_by_id
 from app.models import (
     VerifyCode,
     User,
     Notification
 )
-
 from app import db
+import app.celery.tasks
 
 from tests import create_authorization_header
-from freezegun import freeze_time
-
-import app.celery.tasks
 
 
 def test_user_verify_code(client,
@@ -163,7 +160,7 @@ def test_user_verify_password_missing_password(client,
 
 @pytest.mark.parametrize('research_mode', [True, False])
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_send_user_sms_code(notify_api,
+def test_send_user_sms_code(client,
                             sample_user,
                             sms_code_template,
                             mocker,
@@ -171,9 +168,6 @@ def test_send_user_sms_code(notify_api,
     """
     Tests POST endpoint /user/<user_id>/sms-code
     """
-
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     if research_mode:
         notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID'])
         notify_service.research_mode = True
@@ -206,15 +200,13 @@ def test_send_user_sms_code(notify_api,
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_send_user_code_for_sms_with_optional_to_field(notify_api,
+def test_send_user_code_for_sms_with_optional_to_field(client,
                                                        sample_user,
                                                        sms_code_template,
                                                        mocker):
     """
     Tests POST endpoint /user/<user_id>/sms-code with optional to field
     """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
     to_number = '+441119876757'
     mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111')
     mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
@@ -282,12 +274,11 @@ def test_send_user_email_verification(client,
     mocked.assert_called_once_with(([str(notification.id)]), queue="notify")
 
 
-def test_send_email_verification_returns_404_for_bad_input_data(client, notify_db, notify_db_session, mocker):
+def test_send_email_verification_returns_404_for_bad_input_data(client, notify_db_session, mocker):
     """
     Tests POST endpoint /user/<user_id>/sms-code return 404 for bad input data
     """
     mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-    import uuid
     uuid_ = uuid.uuid4()
     auth_header = create_authorization_header()
     resp = client.post(
@@ -297,3 +288,17 @@ def test_send_email_verification_returns_404_for_bad_input_data(client, notify_d
     assert resp.status_code == 404
     assert json.loads(resp.get_data(as_text=True))['message'] == 'No result found'
     assert mocked.call_count == 0
+
+
+def test_user_verify_user_code_valid_code_resets_failed_login_count(client, sample_sms_code):
+    sample_sms_code.user.failed_login_count = 1
+    data = json.dumps({
+        'code_type': sample_sms_code.code_type,
+        'code': sample_sms_code.txt_code})
+    resp = client.post(
+        url_for('user.verify_user_code', user_id=sample_sms_code.user.id),
+        data=data,
+        headers=[('Content-Type', 'application/json'), create_authorization_header()])
+    assert resp.status_code == 204
+    assert sample_sms_code.user.failed_login_count == 0
+    assert sample_sms_code.code_used