Skip to content

Commit

Permalink
Fixes #5378, adds more checks for invalid username
Browse files Browse the repository at this point in the history
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.

(cherry picked from commit 90e19ea)
  • Loading branch information
kushaldas authored and rmol committed Jul 16, 2020
1 parent 867a143 commit bde4ea6
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 0 deletions.
4 changes: 4 additions & 0 deletions securedrop/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
43 changes: 43 additions & 0 deletions securedrop/tests/functional/journalist_navigation_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 7 additions & 0 deletions securedrop/tests/functional/test_admin_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 23 additions & 0 deletions securedrop/tests/test_journalist.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Expand Down

0 comments on commit bde4ea6

Please sign in to comment.