From 786893d9202383b97c879597cb51a9e9e9acf136 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 24 Mar 2021 15:35:16 +0000 Subject: [PATCH 1/2] Reduce max concurrent 2 factor codes I was doing some analysis and saw that in the last 24 hours the most codes that anyone had was in a 15 minute window was 3. So I think we can safely reduce this to 5 to get a bit more security with enough headroom to not have any negative impact to the user. --- app/config.py | 2 +- tests/app/user/test_rest_verify.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/config.py b/app/config.py index 3834a73bf..a19405862 100644 --- a/app/config.py +++ b/app/config.py @@ -147,7 +147,7 @@ class Config(object): API_PAGE_SIZE = 250 TEST_MESSAGE_FILENAME = 'Test message' ONE_OFF_MESSAGE_FILENAME = 'Report' - MAX_VERIFY_CODE_COUNT = 10 + MAX_VERIFY_CODE_COUNT = 5 # be careful increasing this size without being sure that we won't see slowness in pysftp MAX_LETTER_PDF_ZIP_FILESIZE = 40 * 1024 * 1024 # 40mb diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 8268d0c36..8b4eb8a56 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -247,7 +247,7 @@ def test_send_sms_code_returns_404_for_bad_input_data(client): def test_send_sms_code_returns_204_when_too_many_codes_already_created(client, sample_user): - for _ in range(10): + for _ in range(5): verify_code = VerifyCode( code_type='sms', _code=12345, @@ -257,14 +257,14 @@ def test_send_sms_code_returns_204_when_too_many_codes_already_created(client, s ) db.session.add(verify_code) db.session.commit() - assert VerifyCode.query.count() == 10 + assert VerifyCode.query.count() == 5 auth_header = create_admin_authorization_header() resp = client.post( url_for('user.send_user_2fa_code', code_type='sms', user_id=sample_user.id), data=json.dumps({}), headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - assert VerifyCode.query.count() == 10 + assert VerifyCode.query.count() == 5 def test_send_new_user_email_verification(client, From 544bfbf56931d43bddf3a152fc04e84d4c816ef1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 4 Oct 2021 10:18:58 +0100 Subject: [PATCH 2/2] Add separate config item for failed login count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s confusing that changing `MAX_VERIFY_CODE_COUNT` also limits the number of failed login attempts that a user of text messages 2FA can make. This makes the parameters independent, and adds a test to make sure any future changes which affect the limit of failed login attempts are covered. --- app/config.py | 1 + app/user/rest.py | 2 +- tests/app/user/test_rest_verify.py | 25 +++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index a19405862..ae5de0b6e 100644 --- a/app/config.py +++ b/app/config.py @@ -148,6 +148,7 @@ class Config(object): TEST_MESSAGE_FILENAME = 'Test message' ONE_OFF_MESSAGE_FILENAME = 'Report' MAX_VERIFY_CODE_COUNT = 5 + MAX_FAILED_LOGIN_COUNT = 10 # be careful increasing this size without being sure that we won't see slowness in pysftp MAX_LETTER_PDF_ZIP_FILESIZE = 40 * 1024 * 1024 # 40mb diff --git a/app/user/rest.py b/app/user/rest.py index ddb28265d..1f25affee 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -205,7 +205,7 @@ def verify_user_code(user_id): user_to_verify = get_user_by_id(user_id=user_id) code = get_user_code(user_to_verify, data['code'], data['code_type']) - if user_to_verify.failed_login_count >= current_app.config.get('MAX_VERIFY_CODE_COUNT'): + if user_to_verify.failed_login_count >= current_app.config.get('MAX_FAILED_LOGIN_COUNT'): raise InvalidRequest("Code not found", status_code=404) if not code: # only relevant from sms diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 8b4eb8a56..1078416a6 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -71,6 +71,31 @@ def test_user_verify_code_bad_code_and_increments_failed_login_count(client, assert User.query.get(sample_sms_code.user.id).failed_login_count == 1 +@pytest.mark.parametrize('failed_login_count, expected_status', ( + (9, 204), + (10, 404), +)) +def test_user_verify_code_rejects_good_code_if_too_many_failed_logins( + client, + sample_sms_code, + failed_login_count, + expected_status, +): + sample_sms_code.user.failed_login_count = failed_login_count + resp = client.post( + url_for('user.verify_user_code', user_id=sample_sms_code.user.id), + data=json.dumps({ + 'code_type': sample_sms_code.code_type, + 'code': sample_sms_code.txt_code, + }), + headers=[ + ('Content-Type', 'application/json'), + create_admin_authorization_header(), + ], + ) + assert resp.status_code == expected_status + + @freeze_time('2020-04-01 12:00') @pytest.mark.parametrize('code_type', [EMAIL_TYPE, SMS_TYPE]) def test_user_verify_code_expired_code_and_increments_failed_login_count(code_type, admin_request, sample_user):