Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle case of deleted journalists #5284

Merged
merged 14 commits into from
Jul 8, 2020
Merged
4 changes: 4 additions & 0 deletions docs/admin.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
journalists which are deleted from the system.

FreeOTP
^^^^^^^

Expand Down
8 changes: 7 additions & 1 deletion securedrop/journalist_app/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,16 @@ def name_length_validation(form, field):
.format(max_chars=Journalist.MAX_NAME_LEN)))


def check_invalid_usernames(form, field):
if field.data in Journalist.INVALID_USERNAMES:
raise ValidationError(gettext(
"This username is invalid because it is reserved for internal use by the software."))


class NewUserForm(FlaskForm):
username = StringField('username', validators=[
InputRequired(message=gettext('This field is required.')),
minimum_length_validation
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()])
Expand Down
7 changes: 7 additions & 0 deletions securedrop/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -641,12 +642,18 @@ def login(cls,
username: str,
password: str,
token: str) -> 'Journalist':

try:
user = Journalist.query.filter_by(username=username).one()
except NoResultFound:
raise InvalidUsernameException(
"invalid username '{}'".format(username))

if user.username in Journalist.INVALID_USERNAMES and \
rmol marked this conversation as resolved.
Show resolved Hide resolved
user.uuid in Journalist.INVALID_USERNAMES:
raise InvalidUsernameException(
"Invalid username")

if LOGIN_HARDENING:
cls.throttle_login(user)

Expand Down
22 changes: 22 additions & 0 deletions securedrop/tests/functional/journalist_navigation_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,28 @@ 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]")

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

def _admin_adds_a_user(self, is_admin=False, new_username=""):
self.safe_click_by_id("add-user")

Expand Down
6 changes: 6 additions & 0 deletions securedrop/tests/functional/test_admin_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
62 changes: 61 additions & 1 deletion securedrop/tests/test_journalist.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1076,6 +1076,66 @@ 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'])

resp = app.post(url_for('admin.add_user'),
data=dict(username=username,
first_name='',
last_name='',
password=VALID_PASSWORD,
is_admin=None))

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):
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_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'],
Expand Down