From 6761ce6b484309fec80db094decc48365ae31f9c Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Sat, 30 May 2020 21:56:05 +0530 Subject: [PATCH 01/14] Handle case of deleted journalists --- securedrop/journalist_app/forms.py | 8 +++++++- securedrop/models.py | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 21e0dbdd7a..2d3acd56cb 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -37,10 +37,16 @@ def name_length_validation(form, field): .format(max_chars=Journalist.MAX_NAME_LEN))) +def check_username_deleted(form, field): + if field.data == 'deleted': + raise ValidationError(gettext( + "Invalid username '{}'".format(field.data))) + + class NewUserForm(FlaskForm): username = StringField('username', validators=[ InputRequired(message=gettext('This field is required.')), - minimum_length_validation + minimum_length_validation, check_username_deleted ]) first_name = StringField('first_name', validators=[name_length_validation, Optional()]) last_name = StringField('last_name', validators=[name_length_validation, Optional()]) diff --git a/securedrop/models.py b/securedrop/models.py index 8315e94f2d..150f9e1a8c 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -647,6 +647,10 @@ def login(cls, raise InvalidUsernameException( "invalid username '{}'".format(username)) + if user.username == 'deleted' and user.uuid == 'deleted': + raise InvalidUsernameException( + "Invalid username '{}'".format(username)) + if LOGIN_HARDENING: cls.throttle_login(user) From 763076adb4066d39403e28732385a85de51aad95 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 3 Jun 2020 01:20:34 +0530 Subject: [PATCH 02/14] Check usernames from a list --- securedrop/journalist_app/forms.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 2d3acd56cb..072064d742 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -37,8 +37,9 @@ def name_length_validation(form, field): .format(max_chars=Journalist.MAX_NAME_LEN))) -def check_username_deleted(form, field): - if field.data == 'deleted': +def check_invalid_usernames(form, field): + invalid_usernames = ['deleted'] + if field.data in invalid_usernames: raise ValidationError(gettext( "Invalid username '{}'".format(field.data))) @@ -46,7 +47,7 @@ def check_username_deleted(form, field): class NewUserForm(FlaskForm): username = StringField('username', validators=[ InputRequired(message=gettext('This field is required.')), - minimum_length_validation, check_username_deleted + minimum_length_validation, check_invalid_usernames ]) first_name = StringField('first_name', validators=[name_length_validation, Optional()]) last_name = StringField('last_name', validators=[name_length_validation, Optional()]) From a6e53f8c369f346ebd717cf1b5f9e3833f31c6fc Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 3 Jun 2020 01:52:42 +0530 Subject: [PATCH 03/14] Add tests to check if admin add user fail with invalid username --- securedrop/tests/test_journalist.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index b45791acdc..636c10fea9 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1076,6 +1076,23 @@ def test_admin_add_user(journalist_app, test_admin): uid=new_user.id)) +def test_admin_add_user_with_invalid_username(journalist_app, test_admin): + username = 'deleted' + + with journalist_app.test_client() as app: + _login_user(app, test_admin['username'], test_admin['password'], test_admin['otp_secret']) + + with InstrumentedApp(journalist_app) as ins: + resp = app.post(url_for('admin.add_user'), + data=dict(username=username, + first_name='', + last_name='', + password=VALID_PASSWORD, + is_admin=None)) + + assert "Invalid username '{}'".format(username) in resp.data.decode('utf-8') + + def test_admin_add_user_without_username(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], From e14201fcc53e0814a0ab005b01a991d4688f6508 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 3 Jun 2020 02:03:37 +0530 Subject: [PATCH 04/14] Add documentation for not allowing deleted username --- docs/admin.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/admin.rst b/docs/admin.rst index 7661db7482..47dd88f5c8 100644 --- a/docs/admin.rst +++ b/docs/admin.rst @@ -132,6 +132,10 @@ password manager. Then, you will select whether you would like them to also be an admin (this allows them to add or delete other journalist accounts), and whether they will be using FreeOTP or a YubiKey for two-factor authentication. +.. note:: + We don't allow the username **deleted** as we use it to mark the + journalist which are deleted from the system. + FreeOTP ^^^^^^^ From 498185916f0bde52c2fbfa4704323a4c53273915 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 3 Jun 2020 02:27:32 +0530 Subject: [PATCH 05/14] Unused variable in tests --- securedrop/tests/test_journalist.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 636c10fea9..fb6fcb1f0f 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1082,13 +1082,12 @@ def test_admin_add_user_with_invalid_username(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], test_admin['otp_secret']) - with InstrumentedApp(journalist_app) as ins: - resp = app.post(url_for('admin.add_user'), - data=dict(username=username, - first_name='', - last_name='', - password=VALID_PASSWORD, - is_admin=None)) + resp = app.post(url_for('admin.add_user'), + data=dict(username=username, + first_name='', + last_name='', + password=VALID_PASSWORD, + is_admin=None)) assert "Invalid username '{}'".format(username) in resp.data.decode('utf-8') From 916f8db77a1995100283e5376bb811ec78fa45b6 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Sat, 6 Jun 2020 18:09:07 +0530 Subject: [PATCH 06/14] Check invalid username from lists in login --- securedrop/models.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/securedrop/models.py b/securedrop/models.py index 150f9e1a8c..d644e0af1f 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -641,13 +641,16 @@ def login(cls, username: str, password: str, token: str) -> 'Journalist': + + invalid_usernames = ['deleted'] + try: user = Journalist.query.filter_by(username=username).one() except NoResultFound: raise InvalidUsernameException( "invalid username '{}'".format(username)) - if user.username == 'deleted' and user.uuid == 'deleted': + if user.username in invalid_usernames and user.uuid in invalid_usernames: raise InvalidUsernameException( "Invalid username '{}'".format(username)) From 5a833c67fc38ca2582a5349c78582254f6cb9c48 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Sat, 6 Jun 2020 18:56:30 +0530 Subject: [PATCH 07/14] Add tests for deleted user cannot login --- securedrop/tests/test_journalist.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index fb6fcb1f0f..8826570345 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1092,6 +1092,30 @@ def test_admin_add_user_with_invalid_username(journalist_app, test_admin): assert "Invalid username '{}'".format(username) in resp.data.decode('utf-8') +def test_deleted_user_cannot_login(journalist_app): + username = 'deleted' + uuid = 'deleted' + + # Create a user with username and uuid as deleted + with journalist_app.app_context(): + user, password = utils.db_helper.init_journalist(is_admin=False) + otp_secret = user.otp_secret + user.username = username + user.uuid = uuid + db.session.add(user) + db.session.commit() + + # Verify that deleted user is not able to login + with journalist_app.test_client() as app: + resp = app.post(url_for('main.login'), + data=dict(username=username, + password=password, + token=otp_secret)) + assert resp.status_code == 200 + text = resp.data.decode('utf-8') + assert "Login failed" in text + + def test_admin_add_user_without_username(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], From 12808d577f8f0636cae7cd29c56d87b7535b3e3c Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 10 Jun 2020 23:53:51 +0530 Subject: [PATCH 08/14] Check deleted username exception handling using tests --- securedrop/tests/test_journalist.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 8826570345..37aae592f3 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -26,7 +26,7 @@ from db import db from models import (InvalidPasswordLength, InstanceConfig, Journalist, Reply, Source, - Submission) + InvalidUsernameException, Submission) from .utils.instrument import InstrumentedApp # Smugly seed the RNG for deterministic testing @@ -1116,6 +1116,25 @@ def test_deleted_user_cannot_login(journalist_app): assert "Login failed" in text +def test_deleted_user_cannot_login_exception(journalist_app): + username = 'deleted' + uuid = 'deleted' + + # Create a user with username and uuid as deleted + with journalist_app.app_context(): + user, password = utils.db_helper.init_journalist(is_admin=False) + otp_secret = user.otp_secret + user.username = username + user.uuid = uuid + db.session.add(user) + db.session.commit() + + with pytest.raises(InvalidUsernameException): + Journalist.login(username, + password, + TOTP(otp_secret).now()) + + def test_admin_add_user_without_username(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], From dae2ab84e12cf88f4e1e77a280522812eae89629 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Thu, 18 Jun 2020 00:10:08 +0530 Subject: [PATCH 09/14] Add functional test for admin panel --- .../functional/journalist_navigation_steps.py | 19 +++++++++++++++++++ .../tests/functional/test_admin_interface.py | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index 3f066e647d..f04f1e6192 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -322,6 +322,25 @@ def _add_user(self, username, first_name="", last_name="", is_admin=False, hotp= self.wait_for(lambda: self.driver.find_element_by_id("check-token")) + def _admin_adds_a_user_with_invalid_username(self): + self.safe_click_by_id("add-user") + + self.wait_for(lambda: self.driver.find_element_by_id("username")) + + if not hasattr(self, "accept_languages"): + # The add user page has a form with an "ADD USER" button + btns = self.driver.find_elements_by_tag_name("button") + assert "ADD USER" in [el.text for el in btns] + + invalid_username = 'deleted' + + self.safe_send_keys_by_css_selector('input[name="username"]', invalid_username) + + self.safe_click_by_css_selector("button[type=submit]") + + error_msg = self.driver.find_element_by_css_selector(".form-validation-error") + assert "Invalid username '{}'".format(invalid_username) in error_msg.text + def _admin_adds_a_user(self, is_admin=False, new_username=""): self.safe_click_by_id("add-user") diff --git a/securedrop/tests/functional/test_admin_interface.py b/securedrop/tests/functional/test_admin_interface.py index 027aeed705..8d3fb7ae7c 100644 --- a/securedrop/tests/functional/test_admin_interface.py +++ b/securedrop/tests/functional/test_admin_interface.py @@ -56,6 +56,12 @@ def test_ossec_alert_button(self): self._admin_visits_system_config_page() self._admin_can_send_test_alert() + def test_admin_adds_user_with_invalid_username(self): + self._admin_logs_in() + self._admin_visits_admin_interface() + # Add an user with invalid username + self._admin_adds_a_user_with_invalid_username() + def test_admin_adds_admin_user(self): self._admin_logs_in() self._admin_visits_admin_interface() From 95a0b43ec9419ea3de03eb7221078cb302b0a838 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Sat, 20 Jun 2020 22:02:18 +0530 Subject: [PATCH 10/14] Add wait_for in selenium test --- securedrop/tests/functional/journalist_navigation_steps.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index f04f1e6192..484a6247fc 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -338,6 +338,8 @@ def _admin_adds_a_user_with_invalid_username(self): self.safe_click_by_css_selector("button[type=submit]") + self.wait_for(lambda: self.driver.find_element_by_css_selector(".form-validation-error")) + error_msg = self.driver.find_element_by_css_selector(".form-validation-error") assert "Invalid username '{}'".format(invalid_username) in error_msg.text From ae6ed94feeeeb9e3112983fa2293561438b5ae2d Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Fri, 26 Jun 2020 00:35:39 +0530 Subject: [PATCH 11/14] Requested changes --- docs/admin.rst | 2 +- securedrop/journalist_app/forms.py | 5 ++--- securedrop/models.py | 8 ++++---- .../tests/functional/journalist_navigation_steps.py | 2 +- securedrop/tests/test_journalist.py | 2 +- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/docs/admin.rst b/docs/admin.rst index 47dd88f5c8..fe3c166e8a 100644 --- a/docs/admin.rst +++ b/docs/admin.rst @@ -134,7 +134,7 @@ whether they will be using FreeOTP or a YubiKey for two-factor authentication. .. note:: We don't allow the username **deleted** as we use it to mark the - journalist which are deleted from the system. + journalists which are deleted from the system. FreeOTP ^^^^^^^ diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 072064d742..3c048a9ff3 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -38,10 +38,9 @@ def name_length_validation(form, field): def check_invalid_usernames(form, field): - invalid_usernames = ['deleted'] - if field.data in invalid_usernames: + if field.data in Journalist.INVALID_USERNAMES: raise ValidationError(gettext( - "Invalid username '{}'".format(field.data))) + "Invalid username")) class NewUserForm(FlaskForm): diff --git a/securedrop/models.py b/securedrop/models.py index d644e0af1f..92b14a8ffd 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -414,6 +414,7 @@ class Journalist(db.Model): MIN_USERNAME_LEN = 3 MIN_NAME_LEN = 0 MAX_NAME_LEN = 100 + INVALID_USERNAMES = ['deleted'] def __init__(self, username: str, @@ -642,17 +643,16 @@ def login(cls, password: str, token: str) -> 'Journalist': - invalid_usernames = ['deleted'] - try: user = Journalist.query.filter_by(username=username).one() except NoResultFound: raise InvalidUsernameException( "invalid username '{}'".format(username)) - if user.username in invalid_usernames and user.uuid in invalid_usernames: + if user.username in Journalist.INVALID_USERNAMES and \ + user.uuid in Journalist.INVALID_USERNAMES: raise InvalidUsernameException( - "Invalid username '{}'".format(username)) + "Invalid username") if LOGIN_HARDENING: cls.throttle_login(user) diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index 484a6247fc..aaad7540e9 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -341,7 +341,7 @@ def _admin_adds_a_user_with_invalid_username(self): self.wait_for(lambda: self.driver.find_element_by_css_selector(".form-validation-error")) error_msg = self.driver.find_element_by_css_selector(".form-validation-error") - assert "Invalid username '{}'".format(invalid_username) in error_msg.text + assert "Invalid username" in error_msg.text def _admin_adds_a_user(self, is_admin=False, new_username=""): self.safe_click_by_id("add-user") diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 37aae592f3..bba4fee4b9 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1089,7 +1089,7 @@ def test_admin_add_user_with_invalid_username(journalist_app, test_admin): password=VALID_PASSWORD, is_admin=None)) - assert "Invalid username '{}'".format(username) in resp.data.decode('utf-8') + assert "Invalid username" in resp.data.decode('utf-8') def test_deleted_user_cannot_login(journalist_app): From 6fafa9fa185fe35f2f81900908b1b839349d4c00 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 8 Jul 2020 15:51:30 +0530 Subject: [PATCH 12/14] Change error text to be more informative --- securedrop/journalist_app/forms.py | 2 +- securedrop/tests/functional/journalist_navigation_steps.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 3c048a9ff3..05cee4bfa5 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -40,7 +40,7 @@ def name_length_validation(form, field): def check_invalid_usernames(form, field): if field.data in Journalist.INVALID_USERNAMES: raise ValidationError(gettext( - "Invalid username")) + "This username is invalid because it is reserved for internal use by the software.")) class NewUserForm(FlaskForm): diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index aaad7540e9..a237bfc104 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -341,7 +341,7 @@ def _admin_adds_a_user_with_invalid_username(self): self.wait_for(lambda: self.driver.find_element_by_css_selector(".form-validation-error")) error_msg = self.driver.find_element_by_css_selector(".form-validation-error") - assert "Invalid username" in error_msg.text + assert "This username is invalid because it is reserved for internal use by the software." in error_msg.text def _admin_adds_a_user(self, is_admin=False, new_username=""): self.safe_click_by_id("add-user") From ce9fccaeff41e7ea2d113f7283dfddc7334e30f7 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 8 Jul 2020 16:00:54 +0530 Subject: [PATCH 13/14] Correct linting errors --- securedrop/tests/functional/journalist_navigation_steps.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index a237bfc104..710058cdbc 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -341,7 +341,8 @@ def _admin_adds_a_user_with_invalid_username(self): self.wait_for(lambda: self.driver.find_element_by_css_selector(".form-validation-error")) error_msg = self.driver.find_element_by_css_selector(".form-validation-error") - assert "This username is invalid because it is reserved for internal use by the software." in error_msg.text + assert "This username is invalid because it is reserved for internal use " \ + "by the software." in error_msg.text def _admin_adds_a_user(self, is_admin=False, new_username=""): self.safe_click_by_id("add-user") From 5a0b46ffb4c148cae78f98338aca798895b082f6 Mon Sep 17 00:00:00 2001 From: Prateek Jain Date: Wed, 8 Jul 2020 16:55:34 +0530 Subject: [PATCH 14/14] Correct failing tests --- securedrop/tests/test_journalist.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index bba4fee4b9..9b3032fac6 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1089,7 +1089,8 @@ def test_admin_add_user_with_invalid_username(journalist_app, test_admin): password=VALID_PASSWORD, is_admin=None)) - assert "Invalid username" in resp.data.decode('utf-8') + assert "This username is invalid because it is reserved for internal use by the software." \ + in resp.data.decode('utf-8') def test_deleted_user_cannot_login(journalist_app):