Login throttling should be per-user

[redshiftzero]

Status

Ready for review

Description of Changes

Fixes #3563.

Changes proposed in this pull request:

• Adds a regression test for bug in Login throttling should be per-user #3563
• Resolve Login throttling should be per-user #3563 bug by making the login throttling per-user

Testing

• Reproduce original bug following STR in Login throttling should be per-user #3563
• Checkout the regression test without the fix (i.e. check out e128b04) and verify it fails
• Apply the fix in 369b472, the regression test should now pass and logging in as the second user in the STR should now result in success

Deployment

To be deployed in securedrop-app-code deb package tomorrow

Checklist

If you made changes to the server application code:

• Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

[redshiftzero]

(once merged this should get backported into 0.8.0)

[codecov-io]

Codecov Report

Merging #3564 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3564   +/-   ##
========================================
  Coverage    85.12%   85.12%           
========================================
  Files           37       37           
  Lines         2367     2367           
  Branches       260      260           
========================================
  Hits          2015     2015           
  Misses         289      289           
  Partials        63       63

Impacted Files	Coverage Δ
models.py	88.35% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af5df30...369b472. Read the comment docs.

[emkll]

@redshiftzero thanks for adding a regression test and providing a test plan, changes look good.

One thing I've discovered while doing manual testing of this PR: logins for invalid usernames are not throttled. This would allow an attacker to potentially enumerate journalist usernames. Given that it requires an ATHS token, I think the risk is quite small.

[redshiftzero]

Very good point, marking as wip for a bit

[redshiftzero]

Actually, upon investigation, this is actually the case on develop also - this is due to the fact we check if the username is valid prior to checking the number of prior login attempts.

[redshiftzero]

And since in the journalist_login_attempt table we have journalist_id as a foreign key to the journalist table, we can't actually store or track login attempts for non-journalists users... so this is something that we should file as a followup and address in 0.9.0

[redshiftzero]

(here's a regression test for that other bug for later)

diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py
index 13426872..098ddb8b 100644
--- a/securedrop/tests/test_journalist.py
+++ b/securedrop/tests/test_journalist.py
@@ -231,6 +231,38 @@ def test_login_throttle_is_not_global(journalist_app, test_journo, test_admin):
         models.LOGIN_HARDENING = False
 
 
+def test_login_throttle_works_for_invalid_usernames(journalist_app,
+                                                    test_journo, test_admin):
+
+    # Overwrite the default value used during testing
+    # Note that this may break other tests if doing parallel testing
+    models.LOGIN_HARDENING = True
+    try:
+        with journalist_app.test_client() as app:
+            for _ in range(Journalist._MAX_LOGIN_ATTEMPTS_PER_PERIOD):
+                resp = app.post(
+                    url_for('main.login'),
+                    data=dict(username='invalid',
+                              password='invalid',
+                              token='invalid'))
+                assert resp.status_code == 200
+                text = resp.data.decode('utf-8')
+                assert "Login failed" in text
+
+            resp = app.post(
+                url_for('main.login'),
+                data=dict(username='invalid',
+                          password='invalid',
+                          token='invalid'))
+            assert resp.status_code == 200
+            text = resp.data.decode('utf-8')
+            assert ("Please wait at least {} seconds".format(
+                Journalist._LOGIN_ATTEMPT_PERIOD) in text)
+
+    finally:
+        models.LOGIN_HARDENING = False
+
+
 def test_login_invalid_credentials(journalist_app, test_journo):
     with journalist_app.test_client() as app:
         resp = app.post(url_for('main.login'),