From c4d230aba8843bc9d9dfa7d80c01eb50bcffe544 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 13 Nov 2018 16:58:36 -0800 Subject: [PATCH 1/2] improve login validation input --- securedrop_client/gui/widgets.py | 19 +++++++++- tests/gui/test_widgets.py | 63 ++++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 3c44c4ef3..ffb2df2f2 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -346,12 +346,27 @@ def validate(self): password = self.password_field.text() tfa_token = self.tfa_field.text().replace(' ', '') if username and password and tfa_token: + # Validate username + min_journalist_username = 3 # Journalist.MIN_USERNAME_LEN on server + if len(username) < min_journalist_username: + self.setDisabled(False) + self.error(_('Your username should be at least 3 characters. ')) + return + + # Validate password + min_password_len = 14 # Journalist.MIN_PASSWORD_LEN on server + max_password_len = 128 # Journalist.MAX_PASSWORD_LEN on server + if len(password) < min_password_len or len(password) > max_password_len: + self.setDisabled(False) + self.error(_('Your password should be between 14 and 128 characters. ')) + return + + # Validate 2FA token try: int(tfa_token) except ValueError: self.setDisabled(False) - self.error(_('Please use only numerals for the ' - 'two factor number.')) + self.error(_('Please use only numerals for the two factor number.')) return self.controller.login(username, password, tfa_token) else: diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index be65553ea..063702692 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -350,7 +350,7 @@ def test_LoginDialog_validate_input_non_numeric_2fa(): ld = LoginDialog(None) ld.setup(mock_controller) ld.username_field.text = mock.MagicMock(return_value='foo') - ld.password_field.text = mock.MagicMock(return_value='bar') + ld.password_field.text = mock.MagicMock(return_value='nicelongpassword') ld.tfa_field.text = mock.MagicMock(return_value='baz') ld.setDisabled = mock.MagicMock() ld.error = mock.MagicMock() @@ -360,6 +360,63 @@ def test_LoginDialog_validate_input_non_numeric_2fa(): assert mock_controller.login.call_count == 0 +def test_LoginDialog_validate_too_short_username(): + """ + If the username is too small, we show an informative error message. + """ + mock_controller = mock.MagicMock() + ld = LoginDialog(None) + ld.setup(mock_controller) + ld.username_field.text = mock.MagicMock(return_value='he') + ld.password_field.text = mock.MagicMock(return_value='nicelongpassword') + ld.tfa_field.text = mock.MagicMock(return_value='123456') + ld.setDisabled = mock.MagicMock() + ld.error = mock.MagicMock() + ld.validate() + assert ld.setDisabled.call_count == 2 + assert ld.error.call_count == 1 + assert mock_controller.login.call_count == 0 + + +def test_LoginDialog_validate_too_short_password(): + """ + If the password is too small, we show an informative error message. + """ + mock_controller = mock.MagicMock() + ld = LoginDialog(None) + ld.setup(mock_controller) + ld.username_field.text = mock.MagicMock(return_value='foo') + ld.password_field.text = mock.MagicMock(return_value='bar') + ld.tfa_field.text = mock.MagicMock(return_value='123456') + ld.setDisabled = mock.MagicMock() + ld.error = mock.MagicMock() + ld.validate() + assert ld.setDisabled.call_count == 2 + assert ld.error.call_count == 1 + assert mock_controller.login.call_count == 0 + + +def test_LoginDialog_validate_too_long_password(): + """ + If the password is too long, we show an informative error message. + """ + mock_controller = mock.MagicMock() + ld = LoginDialog(None) + ld.setup(mock_controller) + + max_password_len = 128 + too_long_password = 'a' * (max_password_len + 1) + ld.username_field.text = mock.MagicMock(return_value='foo') + ld.password_field.text = mock.MagicMock(return_value=too_long_password) + ld.tfa_field.text = mock.MagicMock(return_value='123456') + ld.setDisabled = mock.MagicMock() + ld.error = mock.MagicMock() + ld.validate() + assert ld.setDisabled.call_count == 2 + assert ld.error.call_count == 1 + assert mock_controller.login.call_count == 0 + + def test_LoginDialog_validate_input_ok(): """ Valid input from the user causes a call to the controller's login method. @@ -368,14 +425,14 @@ def test_LoginDialog_validate_input_ok(): ld = LoginDialog(None) ld.setup(mock_controller) ld.username_field.text = mock.MagicMock(return_value='foo') - ld.password_field.text = mock.MagicMock(return_value='bar') + ld.password_field.text = mock.MagicMock(return_value='nicelongpassword') ld.tfa_field.text = mock.MagicMock(return_value='123456') ld.setDisabled = mock.MagicMock() ld.error = mock.MagicMock() ld.validate() assert ld.setDisabled.call_count == 1 assert ld.error.call_count == 0 - mock_controller.login.assert_called_once_with('foo', 'bar', '123456') + mock_controller.login.assert_called_once_with('foo', 'nicelongpassword', '123456') def test_SpeechBubble_init(): From 8d52feac446fc6050e162bc2aefe552052ccf93e Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Wed, 14 Nov 2018 10:55:27 -0800 Subject: [PATCH 2/2] Move constant declarations to top of LoginDialog class --- securedrop_client/gui/widgets.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index ffb2df2f2..4ce2cf8a7 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -268,6 +268,10 @@ class LoginDialog(QDialog): A dialog to display the login form. """ + MIN_PASSWORD_LEN = 14 # Journalist.MIN_PASSWORD_LEN on server + MAX_PASSWORD_LEN = 128 # Journalist.MAX_PASSWORD_LEN on server + MIN_JOURNALIST_USERNAME = 3 # Journalist.MIN_USERNAME_LEN on server + def __init__(self, parent): super().__init__(parent) @@ -347,16 +351,13 @@ def validate(self): tfa_token = self.tfa_field.text().replace(' ', '') if username and password and tfa_token: # Validate username - min_journalist_username = 3 # Journalist.MIN_USERNAME_LEN on server - if len(username) < min_journalist_username: + if len(username) < self.MIN_JOURNALIST_USERNAME: self.setDisabled(False) self.error(_('Your username should be at least 3 characters. ')) return # Validate password - min_password_len = 14 # Journalist.MIN_PASSWORD_LEN on server - max_password_len = 128 # Journalist.MAX_PASSWORD_LEN on server - if len(password) < min_password_len or len(password) > max_password_len: + if len(password) < self.MIN_PASSWORD_LEN or len(password) > self.MAX_PASSWORD_LEN: self.setDisabled(False) self.error(_('Your password should be between 14 and 128 characters. ')) return