From 90e19eacec92a99ecf5e64cf58e4d5faea3cda9f Mon Sep 17 00:00:00 2001 From: Kushal Das Date: Thu, 16 Jul 2020 15:36:42 +0530 Subject: [PATCH] Fixes #5378, adds more checks for invalid username In #5284 we made sure that the username `deleted` is not not allowed to be added in the system via the admin section of the journalist web application. But, one could still edit any existing user and change the name to `deleted`. Or, an admin can add a new user via `manage.py` in command line. In PR adds checks to make sure that addiving a new user via `manage.py` will fail if you try to set the username as `deleted`, it also blocks editing any existing username to `deleted`. The PR also includes unit and functional tests. --- securedrop/models.py | 4 ++ .../functional/journalist_navigation_steps.py | 43 +++++++++++++++++++ .../tests/functional/test_admin_interface.py | 7 +++ securedrop/tests/test_journalist.py | 23 ++++++++++ 4 files changed, 77 insertions(+) diff --git a/securedrop/models.py b/securedrop/models.py index 92b14a8ffd..dcd26bbaf0 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -484,6 +484,10 @@ def check_username_acceptable(cls, username: str) -> None: raise InvalidUsernameException( 'Username "{}" must be at least {} characters long.' .format(username, cls.MIN_USERNAME_LEN)) + if username in cls.INVALID_USERNAMES: + raise InvalidUsernameException( + "This username is invalid because it is reserved " + "for internal use by the software.") @classmethod def check_name_acceptable(cls, name): diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index 710058cdbc..6776818e18 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -529,6 +529,49 @@ def edit_user_page_loaded(): hotp_reset_uid = hotp_reset_button.find_element_by_name("uid") assert hotp_reset_uid.is_displayed() is False + def _admin_editing_invalid_username(self): + # Log the new user out + self._logout() + + self.wait_for(lambda: self.driver.find_element_by_css_selector(".login-form")) + + self._login_user(self.admin, self.admin_pw, self.admin_user["totp"]) + + # Go to the admin interface + self.safe_click_by_id("link-admin-index") + + self.wait_for(lambda: self.driver.find_element_by_css_selector("button#add-user")) + + # Click the "edit user" link for the new user + # self._edit_user(self.new_user['username']) + new_user_edit_links = [ + el + for el in self.driver.find_elements_by_tag_name("a") + if (el.get_attribute("data-username") == self.new_user["username"]) + ] + assert len(new_user_edit_links) == 1 + new_user_edit_links[0].click() + + def can_edit_user(): + h = self.driver.find_elements_by_tag_name("h1")[0] + assert 'Edit user "{}"'.format(self.new_user["username"]) == h.text + + self.wait_for(can_edit_user) + + new_username = "deleted" + + self.safe_send_keys_by_css_selector('input[name="username"]', Keys.CONTROL + "a") + self.safe_send_keys_by_css_selector('input[name="username"]', Keys.DELETE) + self.safe_send_keys_by_css_selector('input[name="username"]', new_username) + self.safe_click_by_css_selector("button[type=submit]") + + def user_edited(): + if not hasattr(self, "accept_languages"): + flash_msg = self.driver.find_element_by_css_selector(".flash") + assert "Invalid username" in flash_msg.text + + self.wait_for(user_edited) + def _admin_can_edit_new_user(self): # Log the new user out self._logout() diff --git a/securedrop/tests/functional/test_admin_interface.py b/securedrop/tests/functional/test_admin_interface.py index 8d3fb7ae7c..b22355a790 100644 --- a/securedrop/tests/functional/test_admin_interface.py +++ b/securedrop/tests/functional/test_admin_interface.py @@ -15,6 +15,13 @@ def test_admin_interface(self): self._new_user_can_log_in() self._admin_can_edit_new_user() + def test_admin_edit_invalid_username(self): + self._admin_logs_in() + self._admin_visits_admin_interface() + self._admin_adds_a_user() + self._new_user_can_log_in() + self._admin_editing_invalid_username() + def test_admin_edits_hotp_secret(self): # Toggle security slider to force prefs change self.set_tbb_securitylevel(ft.TBB_SECURITY_HIGH) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index a7230da205..da6a08c99c 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -820,6 +820,29 @@ def test_admin_edits_user_invalid_username( 'error') +def test_admin_edits_user_invalid_username_deleted( + journalist_app, test_admin, test_journo): + """Test expected error message when admin attempts to change a user's + username to deleted""" + new_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: + app.post( + url_for('admin.edit_user', user_id=test_admin['id']), + data=dict(username=new_username, + first_name='', + last_name='', + is_admin=None)) + + ins.assert_message_flashed( + 'Invalid username: This username is invalid because it ' + 'is reserved for internal use by the software.', + 'error') + + def test_admin_resets_user_hotp_format_non_hexa( journalist_app, test_admin, test_journo):