From 4915e6d12b3df0892da1692e29edf126864ace97 Mon Sep 17 00:00:00 2001 From: Tulio Casagrande Date: Thu, 6 Apr 2017 12:57:30 -0300 Subject: [PATCH 01/12] [#935] Simplify tests for account recovery --- .../test_account_recovery_resource.py | 55 +++++-------------- 1 file changed, 15 insertions(+), 40 deletions(-) diff --git a/service/test/unit/resources/test_account_recovery_resource.py b/service/test/unit/resources/test_account_recovery_resource.py index 4e26fc5bf..1aaa709a0 100644 --- a/service/test/unit/resources/test_account_recovery_resource.py +++ b/service/test/unit/resources/test_account_recovery_resource.py @@ -17,9 +17,8 @@ from mock import MagicMock from twisted.trial import unittest from twisted.web.test.requesthelper import DummyRequest -from twisted.internet import defer -from pixelated.resources.account_recovery_resource import AccountRecoveryResource, InvalidPasswordError +from pixelated.resources.account_recovery_resource import AccountRecoveryResource from test.unit.resources import DummySite @@ -44,7 +43,8 @@ def assert_200_when_user_logged_in(_): def test_post_returns_successfully(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' - self.resource._handle_post = MagicMock(return_value=defer.succeed(None)) + request.content = MagicMock() + request.content.getvalue.return_value = '{"password": "12345678", "confirmPassword": "12345678"}' d = self.web.get(request) @@ -54,10 +54,11 @@ def assert_successful_response(_): d.addCallback(assert_successful_response) return d - def test_post_returns_failure(self): + def test_post_returns_failure_by_password_length(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' - self.resource._handle_post = MagicMock(return_value=defer.fail(InvalidPasswordError)) + request.content = MagicMock() + request.content.getvalue.return_value = '{"password": "1234", "confirmPassword": "1234"}' d = self.web.get(request) @@ -67,42 +68,16 @@ def assert_error_response(_): d.addCallback(assert_error_response) return d - def test_handle_post_successfully(self): - request = MagicMock() - self.resource._get_post_form = MagicMock() - self.resource._validate_password = MagicMock(return_value=True) + def test_post_returns_failure_by_password_confirmation(self): + request = DummyRequest(['/account-recovery']) + request.method = 'POST' + request.content = MagicMock() + request.content.getvalue.return_value = '{"password": "12345678", "confirmPassword": "1234"}' - d = self.resource._handle_post(request) + d = self.web.get(request) - def assert_successful(success): - self.assertEqual(success, 'Done!') + def assert_error_response(_): + self.assertEqual(500, request.responseCode) - d.addCallback(assert_successful) + d.addCallback(assert_error_response) return d - - @defer.inlineCallbacks - def test_handle_post_failed(self): - request = MagicMock() - self.resource._get_post_form = MagicMock() - self.resource._validate_password = MagicMock(return_value=False) - - with self.assertRaises(InvalidPasswordError): - yield self.resource._handle_post(request) - - def test_get_post_form(self): - request = MagicMock() - request.content.getvalue.return_value = '{"userCode": "abc", "password": "123", "confirmPassword": "456"}' - form = self.resource._get_post_form(request) - - self.assertEqual(form.get('userCode'), 'abc') - self.assertEqual(form.get('password'), '123') - self.assertEqual(form.get('confirmPassword'), '456') - - def test_validate_password_successfully(self): - self.assertTrue(self.resource._validate_password('12345678', '12345678')) - - def test_validate_password_failed_by_confirmation(self): - self.assertFalse(self.resource._validate_password('12345678', '1234')) - - def test_validate_password_failed_by_length(self): - self.assertFalse(self.resource._validate_password('1234', '1234')) From 8d5349ab6187a5f978ae80b9d16565ddbb5339ef Mon Sep 17 00:00:00 2001 From: Tulio Casagrande Date: Thu, 6 Apr 2017 18:09:47 -0300 Subject: [PATCH 02/12] [#935] Send username and usercode to back-end --- .../resources/account_recovery_resource.py | 16 ++++++++++-- .../test_account_recovery_resource.py | 24 +++++++++++++---- web-ui/package.json | 1 + .../new_password_form/new_password_form.js | 2 ++ .../new_password_form.spec.js | 8 ++++-- web-ui/src/account_recovery/page.js | 6 ++++- web-ui/src/account_recovery/page.spec.js | 15 ++++++++++- web-ui/src/common/util.js | 7 +++++ web-ui/src/common/util.spec.js | 26 +++++++++++++++---- 9 files changed, 89 insertions(+), 16 deletions(-) diff --git a/service/pixelated/resources/account_recovery_resource.py b/service/pixelated/resources/account_recovery_resource.py index 209a7693d..9e0da3eae 100644 --- a/service/pixelated/resources/account_recovery_resource.py +++ b/service/pixelated/resources/account_recovery_resource.py @@ -18,7 +18,7 @@ import json from twisted.python.filepath import FilePath -from twisted.web.http import OK, INTERNAL_SERVER_ERROR +from twisted.web.http import OK, INTERNAL_SERVER_ERROR, BAD_REQUEST from twisted.web.template import Element, XMLFile, renderElement from twisted.web.server import NOT_DONE_YET from twisted.internet import defer @@ -34,6 +34,10 @@ class InvalidPasswordError(Exception): pass +class EmptyFieldsError(Exception): + pass + + class AccountRecoveryPage(Element): loader = XMLFile(FilePath(os.path.join(get_public_static_folder(), 'account_recovery.html'))) @@ -63,7 +67,10 @@ def success_response(response): def error_response(failure): log.warn(failure) - request.setResponseCode(INTERNAL_SERVER_ERROR) + if failure.type is InvalidPasswordError or failure.type is EmptyFieldsError: + request.setResponseCode(BAD_REQUEST) + else: + request.setResponseCode(INTERNAL_SERVER_ERROR) request.finish() d = self._handle_post(request) @@ -84,4 +91,9 @@ def _handle_post(self, request): if not self._validate_password(password, confirm_password): return defer.fail(InvalidPasswordError('The user entered an invalid password or confirmation')) + username = form.get('username') + user_code = form.get('userCode') + if not username or not user_code: + return defer.fail(EmptyFieldsError('The user entered an empty username or empty usercode')) + return defer.succeed('Done!') diff --git a/service/test/unit/resources/test_account_recovery_resource.py b/service/test/unit/resources/test_account_recovery_resource.py index 1aaa709a0..bd79758b5 100644 --- a/service/test/unit/resources/test_account_recovery_resource.py +++ b/service/test/unit/resources/test_account_recovery_resource.py @@ -44,7 +44,7 @@ def test_post_returns_successfully(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' request.content = MagicMock() - request.content.getvalue.return_value = '{"password": "12345678", "confirmPassword": "12345678"}' + request.content.getvalue.return_value = '{"username": "alice", "userCode": "abc123", "password": "12345678", "confirmPassword": "12345678"}' d = self.web.get(request) @@ -54,16 +54,30 @@ def assert_successful_response(_): d.addCallback(assert_successful_response) return d + def test_post_returns_failure_by_empty_usercode(self): + request = DummyRequest(['/account-recovery']) + request.method = 'POST' + request.content = MagicMock() + request.content.getvalue.return_value = '{"username": "alice", "userCode": "", "password": "1234", "confirmPassword": "1234"}' + + d = self.web.get(request) + + def assert_error_response(_): + self.assertEqual(400, request.responseCode) + + d.addCallback(assert_error_response) + return d + def test_post_returns_failure_by_password_length(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' request.content = MagicMock() - request.content.getvalue.return_value = '{"password": "1234", "confirmPassword": "1234"}' + request.content.getvalue.return_value = '{"username": "alice", "userCode": "abc123", "password": "1234", "confirmPassword": "1234"}' d = self.web.get(request) def assert_error_response(_): - self.assertEqual(500, request.responseCode) + self.assertEqual(400, request.responseCode) d.addCallback(assert_error_response) return d @@ -72,12 +86,12 @@ def test_post_returns_failure_by_password_confirmation(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' request.content = MagicMock() - request.content.getvalue.return_value = '{"password": "12345678", "confirmPassword": "1234"}' + request.content.getvalue.return_value = '{"username": "alice", "userCode": "abc123", "password": "12345678", "confirmPassword": "1234"}' d = self.web.get(request) def assert_error_response(_): - self.assertEqual(500, request.responseCode) + self.assertEqual(400, request.responseCode) d.addCallback(assert_error_response) return d diff --git a/web-ui/package.json b/web-ui/package.json index 2ef337a2a..af9d3cb2d 100644 --- a/web-ui/package.json +++ b/web-ui/package.json @@ -65,6 +65,7 @@ "node-sass": "4.5.0", "nyc": "10.1.2", "postcss-loader": "1.2.2", + "query-string": "^4.3.2", "quoted-printable": "1.0.1", "react": "15.4.2", "react-a11y": "0.3.3", diff --git a/web-ui/src/account_recovery/new_password_form/new_password_form.js b/web-ui/src/account_recovery/new_password_form/new_password_form.js index 5e1e72c9c..54f30b093 100644 --- a/web-ui/src/account_recovery/new_password_form/new_password_form.js +++ b/web-ui/src/account_recovery/new_password_form/new_password_form.js @@ -43,6 +43,7 @@ export class NewPasswordForm extends React.Component { submitHandler = (event) => { event.preventDefault(); submitForm(event, '/account-recovery', { + username: this.props.username, userCode: this.props.userCode, password: this.state.password, confirmPassword: this.state.confirmPassword @@ -105,6 +106,7 @@ NewPasswordForm.propTypes = { t: React.PropTypes.func.isRequired, next: React.PropTypes.func.isRequired, previous: React.PropTypes.func.isRequired, + username: React.PropTypes.string.isRequired, userCode: React.PropTypes.string.isRequired }; diff --git a/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js b/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js index c29487a7e..75bbd5188 100644 --- a/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js +++ b/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js @@ -14,7 +14,7 @@ describe('NewPasswordForm', () => { mockTranslations = key => key; mockPrevious = expect.createSpy(); newPasswordForm = shallow( - + ); }); @@ -45,7 +45,7 @@ describe('NewPasswordForm', () => { beforeEach((done) => { mockNext = expect.createSpy().andCall(() => done()); newPasswordForm = shallow( - + ); fetchMock.post('/account-recovery', 200); newPasswordForm.find('InputField[name="new-password"]').simulate('change', { target: { value: '123' } }); @@ -57,6 +57,10 @@ describe('NewPasswordForm', () => { expect(fetchMock.called('/account-recovery')).toBe(true, 'POST was not called'); }); + it('sends username as content', () => { + expect(fetchMock.lastOptions('/account-recovery').body).toContain('"username":"alice"'); + }); + it('sends user code as content', () => { expect(fetchMock.lastOptions('/account-recovery').body).toContain('"userCode":"def234"'); }); diff --git a/web-ui/src/account_recovery/page.js b/web-ui/src/account_recovery/page.js index 94927a165..53b9532dc 100644 --- a/web-ui/src/account_recovery/page.js +++ b/web-ui/src/account_recovery/page.js @@ -24,6 +24,7 @@ import UserRecoveryCodeForm from 'src/account_recovery/user_recovery_code_form/u import NewPasswordForm from 'src/account_recovery/new_password_form/new_password_form'; import BackupAccountStep from 'src/account_recovery/backup_account_step/backup_account_step'; import Footer from 'src/common/footer/footer'; +import Util from 'src/common/util'; import 'font-awesome/scss/font-awesome.scss'; import './page.scss'; @@ -33,9 +34,11 @@ export class Page extends React.Component { constructor(props) { super(props); - this.state = { step: 0, userCode: '' }; + this.state = { step: 0, userCode: '', username: this.setUsername() }; } + setUsername = () => (Util.getQueryParameter('username') || ''); + nextStep = (event) => { if (event) { event.preventDefault(); @@ -64,6 +67,7 @@ export class Page extends React.Component { previous={this.previousStep} userCode={this.state.userCode} next={this.nextStep} + username={this.state.username} />), 3: }) diff --git a/web-ui/src/account_recovery/page.spec.js b/web-ui/src/account_recovery/page.spec.js index 8e4ccc33d..14364bb2c 100644 --- a/web-ui/src/account_recovery/page.spec.js +++ b/web-ui/src/account_recovery/page.spec.js @@ -14,13 +14,26 @@ import BackupAccountStepWrapper from './backup_account_step/backup_account_step' describe('Account Recovery Page', () => { let page; let pageInstance; + const mockTranslations = key => key; beforeEach(() => { - const mockTranslations = key => key; + global.window = { location: { search: '?username=alice' } }; page = shallow(); pageInstance = page.instance(); }); + it('gets username from url', () => { + expect(pageInstance.state.username).toEqual('alice'); + }); + + it('gets username from url as empty string', () => { + global.window = { location: { search: '' } }; + page = shallow(); + pageInstance = page.instance(); + + expect(pageInstance.state.username).toEqual(''); + }); + it('renders account recovery page title', () => { expect(page.props().title).toEqual('account-recovery.page-title'); }); diff --git a/web-ui/src/common/util.js b/web-ui/src/common/util.js index c70a84447..227167fd3 100644 --- a/web-ui/src/common/util.js +++ b/web-ui/src/common/util.js @@ -1,3 +1,4 @@ +import queryString from 'query-string'; import browser from 'helpers/browser'; export const hasQueryParameter = (param) => { @@ -5,6 +6,11 @@ export const hasQueryParameter = (param) => { return !(decodedUri.split('&').indexOf(param) < 0); }; +export const getQueryParameter = (param) => { + const parsed = queryString.parse(window.location.search); + return parsed[param]; +}; + export const submitForm = (event, url, body = {}) => { event.preventDefault(); @@ -23,5 +29,6 @@ export const submitForm = (event, url, body = {}) => { export default { hasQueryParameter, + getQueryParameter, submitForm }; diff --git a/web-ui/src/common/util.spec.js b/web-ui/src/common/util.spec.js index a79859a02..60fde1ff2 100644 --- a/web-ui/src/common/util.spec.js +++ b/web-ui/src/common/util.spec.js @@ -6,11 +6,9 @@ import Util from 'src/common/util'; describe('Utils', () => { describe('.hasQueryParameter', () => { - global.window = { - location: { - search: '?auth-error&lng=pt-BR' - } - }; + before(() => { + global.window = { location: { search: '?auth-error&lng=pt-BR' } }; + }); it('checks if param included in query parameters', () => { expect(Util.hasQueryParameter('auth-error')).toBe(true); @@ -21,6 +19,24 @@ describe('Utils', () => { }); }); + describe('.getQueryParameter', () => { + before(() => { + global.window = { location: { search: '?auth-error&lng=pt-BR' } }; + }); + + it('gets value of param included in query parameters', () => { + expect(Util.getQueryParameter('lng')).toBe('pt-BR'); + }); + + it('returns empty if param does not have value', () => { + expect(Util.getQueryParameter('auth-error')).toBe(null); + }); + + it('returns undefined if param is not included in query parameters', () => { + expect(Util.getQueryParameter('username')).toBe(undefined); + }); + }); + describe('submitForm', () => { const event = {}; From b3365bbc0c174f782af24a79073d00a1a21874eb Mon Sep 17 00:00:00 2001 From: Tulio Casagrande Date: Fri, 7 Apr 2017 14:39:32 -0300 Subject: [PATCH 03/12] [#935] Extract credentials from the authentication flow --- service/pixelated/authentication.py | 12 ++++++------ .../test/support/integration/multi_user_client.py | 15 ++++++++------- service/test/unit/test_authentication.py | 5 +++-- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/service/pixelated/authentication.py b/service/pixelated/authentication.py index b5edbec0b..f2150ab39 100644 --- a/service/pixelated/authentication.py +++ b/service/pixelated/authentication.py @@ -35,24 +35,24 @@ def __init__(self, leap_provider): @inlineCallbacks def authenticate(self, username, password): username = self.clean_username(username) - auth = yield self._srp_auth(username, password) + credentials = Credentials(username, password) + auth = yield self._srp_auth(credentials) returnValue(auth) @inlineCallbacks - def _srp_auth(self, username, password): + def _srp_auth(self, credentials): try: - auth = yield self._bonafide_auth(username, password) + auth = yield self._bonafide_auth(credentials) except SRPAuthError: raise UnauthorizedLogin("User typed wrong password/username combination.") returnValue(auth) @inlineCallbacks - def _bonafide_auth(self, user, password): + def _bonafide_auth(self, credentials): srp_provider = Api(self._leap_provider.api_uri) - credentials = Credentials(user, password) self.bonafide_session = Session(credentials, srp_provider, self._leap_provider.local_ca_crt) yield self.bonafide_session.authenticate() - returnValue(Authentication(user, + returnValue(Authentication(credentials.username, self.bonafide_session.token, self.bonafide_session.uuid, 'session_id', diff --git a/service/test/support/integration/multi_user_client.py b/service/test/support/integration/multi_user_client.py index 82acb2109..ccd10d915 100644 --- a/service/test/support/integration/multi_user_client.py +++ b/service/test/support/integration/multi_user_client.py @@ -16,7 +16,7 @@ from leap.bitmask.bonafide._srp import SRPAuthError from mock import patch from mockito import mock, when, any as ANY -from pixelated.authentication import Authenticator, Authentication +from pixelated.authentication import Authenticator, Authentication, Credentials from twisted.internet import defer from pixelated.application import UserAgentMode, set_up_protected_resources @@ -50,12 +50,12 @@ def start_client(self, mode=UserAgentMode(is_single_user=True)): self.credentials_checker = StubSRPChecker(leap_provider) self.resource = set_up_protected_resources(root_resource, leap_provider, self.service_factory) - def _mock_bonafide_auth(self, username, password): - if username == 'username' and password == 'password': - self.credentials_checker.add_user(username, password) - when(Authenticator)._bonafide_auth(username, password).thenReturn(self.user_auth) + def _mock_bonafide_auth(self, credentials): + if credentials.username == 'username' and credentials.password == 'password': + self.credentials_checker.add_user(credentials.username, credentials.password) + when(Authenticator)._bonafide_auth(credentials).thenReturn(self.user_auth) else: - when(Authenticator)._bonafide_auth(username, password).thenRaise(SRPAuthError) + when(Authenticator)._bonafide_auth(credentials).thenRaise(SRPAuthError) def login(self, username='username', password='password'): session = Authentication(username, 'some_user_token', 'some_user_uuid', 'session_id', {'is_admin': False}) @@ -69,7 +69,8 @@ def login(self, username='username', password='password'): self.services = self._test_account.services self.user_auth = session - self._mock_bonafide_auth(username, password) + credentials = Credentials(username, password) + self._mock_bonafide_auth(credentials) when(LeapSessionFactory).create(username, password, session).thenReturn(leap_session) with patch('mockito.invocation.AnswerSelector', AnswerSelector): diff --git a/service/test/unit/test_authentication.py b/service/test/unit/test_authentication.py index 0d261685a..bbae5c026 100644 --- a/service/test/unit/test_authentication.py +++ b/service/test/unit/test_authentication.py @@ -22,7 +22,7 @@ from mock import patch, Mock -from pixelated.authentication import Authenticator +from pixelated.authentication import Authenticator, Credentials from pixelated.bitmask_libraries.provider import LeapProvider from pixelated.authentication import Authentication @@ -58,10 +58,11 @@ def test_bonafide_srp_exceptions_should_raise_unauthorized_login(self): def test_domain_name_is_stripped_before_making_bonafide_srp_auth(self): username_without_domain = 'username' username_with_domain = '%s@%s' % (username_without_domain, self._domain) + credentials = Credentials(username_without_domain, 'password') auth = Authenticator(self._leap_provider) with patch.object(Authenticator, '_bonafide_auth') as mock_leap_authenticate: yield auth.authenticate(username_with_domain, 'password') - mock_leap_authenticate.assert_called_once_with(username_without_domain, 'password') + mock_leap_authenticate.assert_called_once_with(credentials) @inlineCallbacks def test_successful_bonafide_auth_should_return_the_user_authentication_object(self): From 9e0d0f17f0d354e247cb1bf9e1b10d372d478c1a Mon Sep 17 00:00:00 2001 From: Sriram Viswanathan Date: Mon, 10 Apr 2017 15:14:46 -0300 Subject: [PATCH 04/12] [#935] Implements account recovery authenticator which extends Authenticator with @tayanefernandes --- .../account_recovery_authenticator.py | 42 +++++++++++++++ service/pixelated/authentication.py | 5 +- .../resources/account_recovery_resource.py | 31 +++++++---- service/pixelated/resources/login_resource.py | 3 +- service/pixelated/resources/root_resource.py | 2 +- service/requirements.txt | 2 +- .../test_account_recovery_resource.py | 30 +++++++++-- .../unit/resources/test_login_resource.py | 5 ++ .../test_account_recovery_authenticator.py | 54 +++++++++++++++++++ 9 files changed, 155 insertions(+), 19 deletions(-) create mode 100644 service/pixelated/account_recovery_authenticator.py create mode 100644 service/test/unit/test_account_recovery_authenticator.py diff --git a/service/pixelated/account_recovery_authenticator.py b/service/pixelated/account_recovery_authenticator.py new file mode 100644 index 000000000..d0da8749e --- /dev/null +++ b/service/pixelated/account_recovery_authenticator.py @@ -0,0 +1,42 @@ +# +# Copyright (c) 2014 ThoughtWorks, Inc. +# +# Pixelated is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Pixelated is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with Pixelated. If not, see . + +from leap.bitmask.bonafide.provider import Api +from leap.bitmask.bonafide.session import Session + +from twisted.cred.error import UnauthorizedLogin +from twisted.internet.defer import inlineCallbacks, returnValue + +from authentication import Authenticator, Authentication + + +class AccountRecoveryAuthenticator(Authenticator): + def __init__(self, leap_provider): + super(AccountRecoveryAuthenticator, self).__init__(leap_provider) + + def _auth_error(self): + raise UnauthorizedLogin("User typed wrong recovery-code/username combination.") + + @inlineCallbacks + def _bonafide_auth(self, credentials): + srp_provider = Api(self._leap_provider.api_uri) + self.bonafide_session = Session(credentials, srp_provider, self._leap_provider.local_ca_crt) + yield self.bonafide_session.authenticate_with_recovery_code() + returnValue(Authentication(credentials.username, + self.bonafide_session.token, + self.bonafide_session.uuid, + 'session_id', + {'is_admin': False})) diff --git a/service/pixelated/authentication.py b/service/pixelated/authentication.py index f2150ab39..1d89c9d18 100644 --- a/service/pixelated/authentication.py +++ b/service/pixelated/authentication.py @@ -44,7 +44,7 @@ def _srp_auth(self, credentials): try: auth = yield self._bonafide_auth(credentials) except SRPAuthError: - raise UnauthorizedLogin("User typed wrong password/username combination.") + self._auth_error() returnValue(auth) @inlineCallbacks @@ -58,6 +58,9 @@ def _bonafide_auth(self, credentials): 'session_id', {'is_admin': False})) + def _auth_error(self): + raise UnauthorizedLogin("User typed wrong password/username combination.") + def clean_username(self, username): if '@' not in username: return username diff --git a/service/pixelated/resources/account_recovery_resource.py b/service/pixelated/resources/account_recovery_resource.py index 9e0da3eae..60b39bd1d 100644 --- a/service/pixelated/resources/account_recovery_resource.py +++ b/service/pixelated/resources/account_recovery_resource.py @@ -18,14 +18,16 @@ import json from twisted.python.filepath import FilePath -from twisted.web.http import OK, INTERNAL_SERVER_ERROR, BAD_REQUEST +from twisted.web.http import OK, INTERNAL_SERVER_ERROR, BAD_REQUEST, UNAUTHORIZED from twisted.web.template import Element, XMLFile, renderElement from twisted.web.server import NOT_DONE_YET from twisted.internet import defer from twisted.logger import Logger +from twisted.cred.error import UnauthorizedLogin from pixelated.resources import BaseResource from pixelated.resources import get_public_static_folder +from pixelated.account_recovery_authenticator import AccountRecoveryAuthenticator log = Logger() @@ -49,8 +51,9 @@ class AccountRecoveryResource(BaseResource): BASE_URL = 'account-recovery' isLeaf = True - def __init__(self, services_factory): + def __init__(self, services_factory, provider): BaseResource.__init__(self, services_factory) + self._authenticator = AccountRecoveryAuthenticator(provider) def render_GET(self, request): request.setResponseCode(OK) @@ -69,6 +72,8 @@ def error_response(failure): log.warn(failure) if failure.type is InvalidPasswordError or failure.type is EmptyFieldsError: request.setResponseCode(BAD_REQUEST) + elif failure.type is UnauthorizedLogin: + request.setResponseCode(UNAUTHORIZED) else: request.setResponseCode(INTERNAL_SERVER_ERROR) request.finish() @@ -80,20 +85,24 @@ def error_response(failure): def _get_post_form(self, request): return json.loads(request.content.getvalue()) + def _validate_empty_fields(self, username, user_code): + if not username or not user_code: + raise EmptyFieldsError('The user entered an empty username or empty usercode') + def _validate_password(self, password, confirm_password): - return password == confirm_password and len(password) >= 8 and len(password) <= 9999 + if password != confirm_password or len(password) < 8 or len(password) > 9999: + raise InvalidPasswordError('The user entered an invalid password or confirmation') + @defer.inlineCallbacks def _handle_post(self, request): form = self._get_post_form(request) + username = form.get('username') + user_code = form.get('userCode') password = form.get('password') confirm_password = form.get('confirmPassword') - if not self._validate_password(password, confirm_password): - return defer.fail(InvalidPasswordError('The user entered an invalid password or confirmation')) - - username = form.get('username') - user_code = form.get('userCode') - if not username or not user_code: - return defer.fail(EmptyFieldsError('The user entered an empty username or empty usercode')) + self._validate_empty_fields(username, user_code) + self._validate_password(password, confirm_password) - return defer.succeed('Done!') + user_auth = yield self._authenticator.authenticate(username, user_code) + defer.returnValue(user_auth) diff --git a/service/pixelated/resources/login_resource.py b/service/pixelated/resources/login_resource.py index 45942ea6f..8e117ae93 100644 --- a/service/pixelated/resources/login_resource.py +++ b/service/pixelated/resources/login_resource.py @@ -17,7 +17,6 @@ import os from xml.sax import SAXParseException -from pixelated.authentication import Authenticator from pixelated.config.leap import BootstrapUserServices from pixelated.resources import BaseResource, UnAuthorizedResource, IPixelatedSession from pixelated.resources.account_recovery_resource import AccountRecoveryResource @@ -103,7 +102,7 @@ def getChild(self, path, request): if path == 'status': return LoginStatusResource(self._services_factory) if path == AccountRecoveryResource.BASE_URL: - return AccountRecoveryResource(self._services_factory) + return AccountRecoveryResource(self._services_factory, self._provider) if not self.is_logged_in(request): return UnAuthorizedResource() return NoResource() diff --git a/service/pixelated/resources/root_resource.py b/service/pixelated/resources/root_resource.py index 896bc24b7..50902b8dc 100644 --- a/service/pixelated/resources/root_resource.py +++ b/service/pixelated/resources/root_resource.py @@ -92,7 +92,7 @@ def _is_xsrf_valid(self, request): def initialize(self, provider=None, disclaimer_banner=None, authenticator=None): self._child_resources.add('assets', File(self._protected_static_folder)) - self._child_resources.add(AccountRecoveryResource.BASE_URL, AccountRecoveryResource(self._services_factory)) + self._child_resources.add(AccountRecoveryResource.BASE_URL, AccountRecoveryResource(self._services_factory, provider)) self._child_resources.add('backup-account', BackupAccountResource(self._services_factory, authenticator, provider)) self._child_resources.add('sandbox', SandboxResource(self._protected_static_folder)) self._child_resources.add('keys', KeysResource(self._services_factory)) diff --git a/service/requirements.txt b/service/requirements.txt index 01a2df5ba..1adb98faa 100644 --- a/service/requirements.txt +++ b/service/requirements.txt @@ -7,7 +7,7 @@ srp==1.0.6 whoosh==2.6.0 Twisted==16.1.1 -e 'git+https://0xacab.org/leap/leap_pycommon.git@master#egg=leap.common' --e 'git+https://0xacab.org/pixelated/bitmask-dev.git@development#egg=leap.bitmask' +-e 'git+https://0xacab.org/pixelated/bitmask-dev.git@auth-recovery-code#egg=leap.bitmask' -e 'git+https://0xacab.org/pixelated/soledad.git@development#egg=leap.soledad.common&subdirectory=common/' -e 'git+https://0xacab.org/pixelated/soledad.git@development#egg=leap.soledad.client&subdirectory=client/' -e . diff --git a/service/test/unit/resources/test_account_recovery_resource.py b/service/test/unit/resources/test_account_recovery_resource.py index bd79758b5..190bdff6a 100644 --- a/service/test/unit/resources/test_account_recovery_resource.py +++ b/service/test/unit/resources/test_account_recovery_resource.py @@ -14,9 +14,12 @@ # You should have received a copy of the GNU Affero General Public License # along with Pixelated. If not, see . -from mock import MagicMock +from mock import MagicMock, patch + +from twisted.internet import defer from twisted.trial import unittest from twisted.web.test.requesthelper import DummyRequest +from twisted.cred.error import UnauthorizedLogin from pixelated.resources.account_recovery_resource import AccountRecoveryResource from test.unit.resources import DummySite @@ -25,7 +28,8 @@ class TestAccountRecoveryResource(unittest.TestCase): def setUp(self): self.services_factory = MagicMock() - self.resource = AccountRecoveryResource(self.services_factory) + self.provider = MagicMock() + self.resource = AccountRecoveryResource(self.services_factory, self.provider) self.web = DummySite(self.resource) def test_get(self): @@ -40,20 +44,40 @@ def assert_200_when_user_logged_in(_): d.addCallback(assert_200_when_user_logged_in) return d - def test_post_returns_successfully(self): + @patch('pixelated.resources.account_recovery_resource.AccountRecoveryAuthenticator.authenticate') + def test_post_returns_successfully(self, mock_authenticate): request = DummyRequest(['/account-recovery']) request.method = 'POST' request.content = MagicMock() request.content.getvalue.return_value = '{"username": "alice", "userCode": "abc123", "password": "12345678", "confirmPassword": "12345678"}' + mock_authenticate.return_value = defer.succeed('') d = self.web.get(request) def assert_successful_response(_): self.assertEqual(200, request.responseCode) + mock_authenticate.assert_called_with('alice', 'abc123') d.addCallback(assert_successful_response) return d + @patch('pixelated.resources.account_recovery_resource.AccountRecoveryAuthenticator.authenticate') + def test_post_returns_unauthorized(self, mock_authenticate): + request = DummyRequest(['/account-recovery']) + request.method = 'POST' + request.content = MagicMock() + request.content.getvalue.return_value = '{"username": "alice", "userCode": "abc123", "password": "12345678", "confirmPassword": "12345678"}' + mock_authenticate.return_value = defer.fail(UnauthorizedLogin()) + + d = self.web.get(request) + + def assert_error_response(_): + self.assertEqual(401, request.responseCode) + mock_authenticate.assert_called_with('alice', 'abc123') + + d.addErrback(assert_error_response) + return d + def test_post_returns_failure_by_empty_usercode(self): request = DummyRequest(['/account-recovery']) request.method = 'POST' diff --git a/service/test/unit/resources/test_login_resource.py b/service/test/unit/resources/test_login_resource.py index eaaba1d4c..a100c45c1 100644 --- a/service/test/unit/resources/test_login_resource.py +++ b/service/test/unit/resources/test_login_resource.py @@ -77,6 +77,11 @@ def assert_successful(_): d.addCallback(assert_successful) return d + def test_get_child_for_account_recovery_path(self): + request = DummyRequest(['account-recovery']) + result = self.resource.getChild('account-recovery', request) + self.assertEqual(result._authenticator._leap_provider, self.portal) + @patch('pixelated.resources.session.PixelatedSession.is_logged_in') def test_there_are_no_grand_children_resources_when_logged_in(self, mock_is_logged_in): request = DummyRequest(['/login/grand_children']) diff --git a/service/test/unit/test_account_recovery_authenticator.py b/service/test/unit/test_account_recovery_authenticator.py new file mode 100644 index 000000000..197b9a081 --- /dev/null +++ b/service/test/unit/test_account_recovery_authenticator.py @@ -0,0 +1,54 @@ +# +# Copyright (c) 2015 ThoughtWorks, Inc. +# +# Pixelated is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Pixelated is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with Pixelated. If not, see . + +from twisted.cred.error import UnauthorizedLogin +from twisted.trial import unittest +from twisted.internet.defer import inlineCallbacks + +from leap.bitmask.bonafide._srp import SRPAuthError + +from mock import patch, MagicMock + +from pixelated.account_recovery_authenticator import AccountRecoveryAuthenticator +from pixelated.bitmask_libraries.provider import LeapProvider + +PROVIDER_JSON = { + "api_uri": "https://api.domain.org:4430", + "api_version": "1", + "ca_cert_fingerprint": "SHA256: some_stub_sha", + "ca_cert_uri": "https://domain.org/ca.crt", + "domain": "domain.org", +} + + +class AccountRecoveryAuthenticatorTest(unittest.TestCase): + def setUp(self): + self._domain = 'domain.org' + with patch.object(LeapProvider, 'fetch_provider_json', return_value=PROVIDER_JSON): + self._leap_provider = LeapProvider(self._domain) + + @inlineCallbacks + def test_bonafide_srp_exceptions_should_raise_unauthorized_login(self): + account_recovery_authenticator = AccountRecoveryAuthenticator(self._leap_provider) + mock_bonafide_session = MagicMock() + mock_bonafide_session.authenticate = MagicMock(side_effect=SRPAuthError()) + with patch('pixelated.authentication.Session', return_value=mock_bonafide_session): + with self.assertRaises(UnauthorizedLogin): + try: + yield account_recovery_authenticator.authenticate('username', 'recovery_code') + except UnauthorizedLogin as e: + self.assertEqual("User typed wrong recovery-code/username combination.", e.message) + raise From c689e8d02dfce9efce7ff465d801fa366557cd62 Mon Sep 17 00:00:00 2001 From: Sriram Viswanathan Date: Tue, 18 Apr 2017 18:24:26 -0300 Subject: [PATCH 05/12] [#935] Authentication using account recovery code with @tuliocasagrande --- .../account_recovery_authenticator.py | 19 ++----------------- service/pixelated/authentication.py | 5 +++-- .../test_account_recovery_authenticator.py | 8 ++++++++ ...uthentication.py => test_authenticator.py} | 19 +++++++++++++------ 4 files changed, 26 insertions(+), 25 deletions(-) rename service/test/unit/{test_authentication.py => test_authenticator.py} (89%) diff --git a/service/pixelated/account_recovery_authenticator.py b/service/pixelated/account_recovery_authenticator.py index d0da8749e..f68e6bfdb 100644 --- a/service/pixelated/account_recovery_authenticator.py +++ b/service/pixelated/account_recovery_authenticator.py @@ -14,29 +14,14 @@ # You should have received a copy of the GNU Affero General Public License # along with Pixelated. If not, see . -from leap.bitmask.bonafide.provider import Api -from leap.bitmask.bonafide.session import Session - from twisted.cred.error import UnauthorizedLogin -from twisted.internet.defer import inlineCallbacks, returnValue -from authentication import Authenticator, Authentication +from authentication import Authenticator class AccountRecoveryAuthenticator(Authenticator): def __init__(self, leap_provider): - super(AccountRecoveryAuthenticator, self).__init__(leap_provider) + super(AccountRecoveryAuthenticator, self).__init__(leap_provider, recovery=True) def _auth_error(self): raise UnauthorizedLogin("User typed wrong recovery-code/username combination.") - - @inlineCallbacks - def _bonafide_auth(self, credentials): - srp_provider = Api(self._leap_provider.api_uri) - self.bonafide_session = Session(credentials, srp_provider, self._leap_provider.local_ca_crt) - yield self.bonafide_session.authenticate_with_recovery_code() - returnValue(Authentication(credentials.username, - self.bonafide_session.token, - self.bonafide_session.uuid, - 'session_id', - {'is_admin': False})) diff --git a/service/pixelated/authentication.py b/service/pixelated/authentication.py index 1d89c9d18..23bc1d6e9 100644 --- a/service/pixelated/authentication.py +++ b/service/pixelated/authentication.py @@ -27,10 +27,11 @@ class Authenticator(object): - def __init__(self, leap_provider): + def __init__(self, leap_provider, recovery=False): self._leap_provider = leap_provider self.domain = leap_provider.server_name self.bonafide_session = None + self.recovery = recovery @inlineCallbacks def authenticate(self, username, password): @@ -51,7 +52,7 @@ def _srp_auth(self, credentials): def _bonafide_auth(self, credentials): srp_provider = Api(self._leap_provider.api_uri) self.bonafide_session = Session(credentials, srp_provider, self._leap_provider.local_ca_crt) - yield self.bonafide_session.authenticate() + yield self.bonafide_session.authenticate(recovery=self.recovery) returnValue(Authentication(credentials.username, self.bonafide_session.token, self.bonafide_session.uuid, diff --git a/service/test/unit/test_account_recovery_authenticator.py b/service/test/unit/test_account_recovery_authenticator.py index 197b9a081..433c1afe0 100644 --- a/service/test/unit/test_account_recovery_authenticator.py +++ b/service/test/unit/test_account_recovery_authenticator.py @@ -52,3 +52,11 @@ def test_bonafide_srp_exceptions_should_raise_unauthorized_login(self): except UnauthorizedLogin as e: self.assertEqual("User typed wrong recovery-code/username combination.", e.message) raise + + def test_bonafide_auth_called_with_recovery_as_true(self): + auth = AccountRecoveryAuthenticator(self._leap_provider) + mock_bonafide_session = MagicMock() + + with patch('pixelated.authentication.Session', return_value=mock_bonafide_session): + auth.authenticate('username', 'password') + mock_bonafide_session.authenticate.assert_called_with(recovery=True) diff --git a/service/test/unit/test_authentication.py b/service/test/unit/test_authenticator.py similarity index 89% rename from service/test/unit/test_authentication.py rename to service/test/unit/test_authenticator.py index bbae5c026..e5ffd1990 100644 --- a/service/test/unit/test_authentication.py +++ b/service/test/unit/test_authenticator.py @@ -68,17 +68,24 @@ def test_domain_name_is_stripped_before_making_bonafide_srp_auth(self): def test_successful_bonafide_auth_should_return_the_user_authentication_object(self): auth = Authenticator(self._leap_provider) mock_bonafide_session = Mock() - mock_srp_auth = Mock() - mock_srp_auth.token = 'some_token' - mock_srp_auth.uuid = 'some_uuid' - mock_bonafide_session.authenticate = Mock(return_value=mock_srp_auth) - with patch('pixelated.authentication.Session', return_value=mock_srp_auth): + mock_bonafide_session.token = 'some_token' + mock_bonafide_session.uuid = 'some_uuid' + + with patch('pixelated.authentication.Session', return_value=mock_bonafide_session): resulting_auth = yield auth.authenticate('username@domain.org', 'password') self.assertIsInstance(resulting_auth, Authentication) self.assertEquals('username', resulting_auth.username) self.assertEquals('some_token', resulting_auth.token) self.assertEquals('some_uuid', resulting_auth.uuid) - self.assertEquals(mock_srp_auth, auth.bonafide_session) + self.assertEquals(mock_bonafide_session, auth.bonafide_session) + + def test_bonafide_auth_called_with_recovery_as_false(self): + auth = Authenticator(self._leap_provider) + mock_bonafide_session = Mock() + + with patch('pixelated.authentication.Session', return_value=mock_bonafide_session): + auth.authenticate('username', 'password') + mock_bonafide_session.authenticate.assert_called_with(recovery=False) def test_username_without_domain_is_not_changed(self): username_without_domain = 'username' From 14db9036fe43eb730ea0cf1c2a099685268ca1e8 Mon Sep 17 00:00:00 2001 From: Sriram Viswanathan Date: Wed, 19 Apr 2017 11:04:28 -0300 Subject: [PATCH 06/12] [#935] Use development branch for bitmask dependency with @tuliocasagrande --- service/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/requirements.txt b/service/requirements.txt index 1adb98faa..01a2df5ba 100644 --- a/service/requirements.txt +++ b/service/requirements.txt @@ -7,7 +7,7 @@ srp==1.0.6 whoosh==2.6.0 Twisted==16.1.1 -e 'git+https://0xacab.org/leap/leap_pycommon.git@master#egg=leap.common' --e 'git+https://0xacab.org/pixelated/bitmask-dev.git@auth-recovery-code#egg=leap.bitmask' +-e 'git+https://0xacab.org/pixelated/bitmask-dev.git@development#egg=leap.bitmask' -e 'git+https://0xacab.org/pixelated/soledad.git@development#egg=leap.soledad.common&subdirectory=common/' -e 'git+https://0xacab.org/pixelated/soledad.git@development#egg=leap.soledad.client&subdirectory=client/' -e . From 8bee0c0f936a7ccdab20d891668bddb9eb0b0afd Mon Sep 17 00:00:00 2001 From: Sriram Viswanathan Date: Wed, 19 Apr 2017 15:15:39 -0300 Subject: [PATCH 07/12] [#935] Fixes error message in case of auth error with @tuliocasagrande --- service/pixelated/account_recovery_authenticator.py | 2 +- service/pixelated/authentication.py | 2 +- service/test/unit/test_account_recovery_authenticator.py | 2 +- service/test/unit/test_authenticator.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/service/pixelated/account_recovery_authenticator.py b/service/pixelated/account_recovery_authenticator.py index f68e6bfdb..242ffcf3b 100644 --- a/service/pixelated/account_recovery_authenticator.py +++ b/service/pixelated/account_recovery_authenticator.py @@ -24,4 +24,4 @@ def __init__(self, leap_provider): super(AccountRecoveryAuthenticator, self).__init__(leap_provider, recovery=True) def _auth_error(self): - raise UnauthorizedLogin("User typed wrong recovery-code/username combination.") + raise UnauthorizedLogin("User typed wrong username/recovery-code combination.") diff --git a/service/pixelated/authentication.py b/service/pixelated/authentication.py index 23bc1d6e9..853d11e79 100644 --- a/service/pixelated/authentication.py +++ b/service/pixelated/authentication.py @@ -60,7 +60,7 @@ def _bonafide_auth(self, credentials): {'is_admin': False})) def _auth_error(self): - raise UnauthorizedLogin("User typed wrong password/username combination.") + raise UnauthorizedLogin("User typed wrong username/password combination.") def clean_username(self, username): if '@' not in username: diff --git a/service/test/unit/test_account_recovery_authenticator.py b/service/test/unit/test_account_recovery_authenticator.py index 433c1afe0..15755b9f6 100644 --- a/service/test/unit/test_account_recovery_authenticator.py +++ b/service/test/unit/test_account_recovery_authenticator.py @@ -50,7 +50,7 @@ def test_bonafide_srp_exceptions_should_raise_unauthorized_login(self): try: yield account_recovery_authenticator.authenticate('username', 'recovery_code') except UnauthorizedLogin as e: - self.assertEqual("User typed wrong recovery-code/username combination.", e.message) + self.assertEqual("User typed wrong username/recovery-code combination.", e.message) raise def test_bonafide_auth_called_with_recovery_as_true(self): diff --git a/service/test/unit/test_authenticator.py b/service/test/unit/test_authenticator.py index e5ffd1990..853494b56 100644 --- a/service/test/unit/test_authenticator.py +++ b/service/test/unit/test_authenticator.py @@ -51,7 +51,7 @@ def test_bonafide_srp_exceptions_should_raise_unauthorized_login(self): try: yield auth.authenticate('username', 'password') except UnauthorizedLogin as e: - self.assertEqual("User typed wrong password/username combination.", e.message) + self.assertEqual("User typed wrong username/password combination.", e.message) raise @inlineCallbacks From 66d772aa6a2c9be9e892bc03dc746a2396ca216a Mon Sep 17 00:00:00 2001 From: Sriram Viswanathan Date: Wed, 19 Apr 2017 16:45:07 -0300 Subject: [PATCH 08/12] [#935] Shows snackbar error response on new password submit with @tuliocasagrande --- web-ui/app/locales/en_US/translation.json | 1 + web-ui/app/locales/pt_BR/translation.json | 1 + .../new_password_form/new_password_form.js | 11 +- .../new_password_form.spec.js | 107 ++++++++++++++---- web-ui/src/account_recovery/page.js | 24 +++- web-ui/src/account_recovery/page.spec.js | 32 ++++++ 6 files changed, 144 insertions(+), 32 deletions(-) diff --git a/web-ui/app/locales/en_US/translation.json b/web-ui/app/locales/en_US/translation.json index 6ca72283c..7bbbad3b6 100644 --- a/web-ui/app/locales/en_US/translation.json +++ b/web-ui/app/locales/en_US/translation.json @@ -68,6 +68,7 @@ "general": "Problems talking to server", "parse": "Got invalid response from server", "auth": "Invalid email or password", + "recovery-auth": "Invalid email or recovery code", "login": { "title": "Oh, something went wrong :(", "message": "Try to login again in a few minutes. If the problem persists, contact your account administrator." diff --git a/web-ui/app/locales/pt_BR/translation.json b/web-ui/app/locales/pt_BR/translation.json index 1baf9b076..3bad56538 100644 --- a/web-ui/app/locales/pt_BR/translation.json +++ b/web-ui/app/locales/pt_BR/translation.json @@ -68,6 +68,7 @@ "general": "Problemas ao se comunicar com o servidor", "parse": "Obteve uma resposta inválida do servidor", "auth": "E-mail ou senha inválidos", + "recovery-auth": "E-mail ou código de recuperação inválidos", "login": { "title": "Ops, algo deu errado :(", "message": "Tente entrar novamente em alguns minutos. Se o problema persistir, contate o administrador da sua conta." diff --git a/web-ui/src/account_recovery/new_password_form/new_password_form.js b/web-ui/src/account_recovery/new_password_form/new_password_form.js index 54f30b093..379f728cb 100644 --- a/web-ui/src/account_recovery/new_password_form/new_password_form.js +++ b/web-ui/src/account_recovery/new_password_form/new_password_form.js @@ -47,8 +47,12 @@ export class NewPasswordForm extends React.Component { userCode: this.props.userCode, password: this.state.password, confirmPassword: this.state.confirmPassword - }).then(() => this.props.next()); - } + }).then((response) => { + if (response.ok) this.props.next(); + else if (response.status === 401) this.props.onError('error.recovery-auth'); + else this.props.onError('error.general'); + }); + }; handleChangePassword = (event) => { this.setState({ password: event.target.value }); @@ -107,7 +111,8 @@ NewPasswordForm.propTypes = { next: React.PropTypes.func.isRequired, previous: React.PropTypes.func.isRequired, username: React.PropTypes.string.isRequired, - userCode: React.PropTypes.string.isRequired + userCode: React.PropTypes.string.isRequired, + onError: React.PropTypes.func.isRequired }; export default translate('', { wait: true })(NewPasswordForm); diff --git a/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js b/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js index 75bbd5188..6589b002e 100644 --- a/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js +++ b/web-ui/src/account_recovery/new_password_form/new_password_form.spec.js @@ -8,13 +8,23 @@ describe('NewPasswordForm', () => { let newPasswordForm; let mockPrevious; let mockNext; + let mockOnError; let mockTranslations; beforeEach(() => { mockTranslations = key => key; mockPrevious = expect.createSpy(); + mockNext = expect.createSpy(); + mockOnError = expect.createSpy(); newPasswordForm = shallow( - + ); }); @@ -42,35 +52,88 @@ describe('NewPasswordForm', () => { }); describe('Submit', () => { - beforeEach((done) => { - mockNext = expect.createSpy().andCall(() => done()); - newPasswordForm = shallow( - - ); - fetchMock.post('/account-recovery', 200); + const submitForm = () => { newPasswordForm.find('InputField[name="new-password"]').simulate('change', { target: { value: '123' } }); newPasswordForm.find('InputField[name="confirm-password"]').simulate('change', { target: { value: '456' } }); newPasswordForm.find('form').simulate('submit', { preventDefault: expect.createSpy() }); - }); + }; - it('posts to account recovery', () => { - expect(fetchMock.called('/account-recovery')).toBe(true, 'POST was not called'); - }); + const createNewPasswordForm = () => { + newPasswordForm = shallow( + + ); + }; + + context('on success', () => { + beforeEach((done) => { + mockNext = expect.createSpy().andCall(() => done()); + createNewPasswordForm(); + fetchMock.post('/account-recovery', 200); + submitForm(); + }); - it('sends username as content', () => { - expect(fetchMock.lastOptions('/account-recovery').body).toContain('"username":"alice"'); - }); + it('posts to account recovery', () => { + expect(fetchMock.called('/account-recovery')).toBe(true, 'POST was not called'); + }); + + it('sends username as content', () => { + expect(fetchMock.lastOptions('/account-recovery').body).toContain('"username":"alice"'); + }); + + it('sends user code as content', () => { + expect(fetchMock.lastOptions('/account-recovery').body).toContain('"userCode":"def234"'); + }); + + it('sends password as content', () => { + expect(fetchMock.lastOptions('/account-recovery').body).toContain('"password":"123"'); + }); - it('sends user code as content', () => { - expect(fetchMock.lastOptions('/account-recovery').body).toContain('"userCode":"def234"'); + it('sends confirm password as content', () => { + expect(fetchMock.lastOptions('/account-recovery').body).toContain('"confirmPassword":"456"'); + }); + + it('calls next handler on success', () => { + expect(mockNext).toHaveBeenCalled(); + }); + + afterEach(fetchMock.restore); }); - it('sends password as content', () => { - expect(fetchMock.lastOptions('/account-recovery').body).toContain('"password":"123"'); + context('on unauthorized error', () => { + beforeEach((done) => { + mockOnError.andCall(() => done()); + createNewPasswordForm(); + fetchMock.post('/account-recovery', 401); + submitForm(); + }); + + it('shows error message on 401', () => { + expect(mockOnError).toHaveBeenCalledWith('error.recovery-auth'); + }); + + afterEach(fetchMock.restore); }); - it('sends confirm password as content', () => { - expect(fetchMock.lastOptions('/account-recovery').body).toContain('"confirmPassword":"456"'); + context('on server error', () => { + beforeEach((done) => { + mockOnError.andCall(() => done()); + createNewPasswordForm(); + fetchMock.post('/account-recovery', 500); + submitForm(); + }); + + it('shows error message on 500', () => { + expect(mockOnError).toHaveBeenCalledWith('error.general'); + }); + + afterEach(fetchMock.restore); }); }); @@ -157,9 +220,5 @@ describe('NewPasswordForm', () => { expect(newPasswordForm.find('SubmitButton').props().disabled).toBe(true); }); }); - - it('calls next handler on success', () => { - expect(mockNext).toHaveBeenCalled(); - }); }); }); diff --git a/web-ui/src/account_recovery/page.js b/web-ui/src/account_recovery/page.js index 53b9532dc..fd69480e6 100644 --- a/web-ui/src/account_recovery/page.js +++ b/web-ui/src/account_recovery/page.js @@ -25,6 +25,7 @@ import NewPasswordForm from 'src/account_recovery/new_password_form/new_password import BackupAccountStep from 'src/account_recovery/backup_account_step/backup_account_step'; import Footer from 'src/common/footer/footer'; import Util from 'src/common/util'; +import SnackbarNotification from 'src/common/snackbar_notification/snackbar_notification'; import 'font-awesome/scss/font-awesome.scss'; import './page.scss'; @@ -34,7 +35,7 @@ export class Page extends React.Component { constructor(props) { super(props); - this.state = { step: 0, userCode: '', username: this.setUsername() }; + this.state = { step: 0, userCode: '', username: this.setUsername(), errorMessage: '' }; } setUsername = () => (Util.getQueryParameter('username') || ''); @@ -44,15 +45,19 @@ export class Page extends React.Component { event.preventDefault(); } this.setState({ step: this.state.step + 1 }); - } + }; previousStep = () => { this.setState({ step: this.state.step - 1 }); - } + }; saveUserCode = (event) => { this.setState({ userCode: event.target.value }); - } + }; + + errorHandler = (errorMessage) => { + this.setState({ errorMessage }); + }; steps = () => ({ 0: , @@ -68,12 +73,20 @@ export class Page extends React.Component { userCode={this.state.userCode} next={this.nextStep} username={this.state.username} + onError={this.errorHandler} />), 3: - }) + }); mainContent = () => this.steps()[this.state.step]; + showSnackbarOnError = (t) => { + if (this.state.errorMessage) { + return ; + } + return undefined; // To satisfy eslint error - consistent-return + }; + render() { const t = this.props.t; return ( @@ -85,6 +98,7 @@ export class Page extends React.Component { {this.mainContent()} + {this.showSnackbarOnError(t)}