diff options
author | Tulio Casagrande <tcasagra@thoughtworks.com> | 2017-02-20 14:37:37 -0300 |
---|---|---|
committer | Tulio Casagrande <tcasagra@thoughtworks.com> | 2017-02-21 13:32:05 -0300 |
commit | fa21608801f8d2ef710d4c28abbb558883afeaf7 (patch) | |
tree | 009db4bb064b077a231a1abe4de212bceb20928b | |
parent | bfd85dff6b086abae1c16014e318c89cba929b66 (diff) |
[#907] Translate auth error message on login
with @anikarni
-rw-r--r-- | service/pixelated/resources/login_resource.py | 17 | ||||
-rw-r--r-- | service/test/integration/test_multi_user_login.py | 9 | ||||
-rw-r--r-- | service/test/unit/resources/test_login_resource.py | 15 | ||||
-rw-r--r-- | web-ui/app/locales/en_US/translation.json | 3 | ||||
-rw-r--r-- | web-ui/app/locales/pt_BR/translation.json | 3 | ||||
-rw-r--r-- | web-ui/src/login/app.js | 24 | ||||
-rw-r--r-- | web-ui/src/login/app.scss | 39 | ||||
-rw-r--r-- | web-ui/src/login/login.css | 33 | ||||
-rw-r--r-- | web-ui/src/login/login.html | 6 | ||||
-rw-r--r-- | web-ui/src/login/login.js | 3 | ||||
-rw-r--r-- | web-ui/src/util.js | 7 | ||||
-rw-r--r-- | web-ui/test/unit/login/app.spec.js | 11 | ||||
-rw-r--r-- | web-ui/test/unit/util.spec.js | 20 |
13 files changed, 121 insertions, 69 deletions
diff --git a/service/pixelated/resources/login_resource.py b/service/pixelated/resources/login_resource.py index bb05489e..2b00680b 100644 --- a/service/pixelated/resources/login_resource.py +++ b/service/pixelated/resources/login_resource.py @@ -70,18 +70,11 @@ class DisclaimerElement(Element): class LoginWebSite(Element): loader = XMLFile(FilePath(os.path.join(get_public_static_folder(), 'login.html'))) - def __init__(self, error_msg=None, disclaimer_banner_file=None): + def __init__(self, disclaimer_banner_file=None): super(LoginWebSite, self).__init__() - self._error_msg = error_msg self.disclaimer_banner_file = disclaimer_banner_file @renderer - def error_msg(self, request, tag): - if self._error_msg is not None: - return tag(self._error_msg) - return tag('') - - @renderer def disclaimer(self, request, tag): return DisclaimerElement(self.disclaimer_banner_file).render(request) @@ -116,8 +109,8 @@ class LoginResource(BaseResource): request.setResponseCode(OK) return self._render_template(request) - def _render_template(self, request, error_msg=None): - site = LoginWebSite(error_msg=error_msg, disclaimer_banner_file=self._disclaimer_banner) + def _render_template(self, request): + site = LoginWebSite(disclaimer_banner_file=self._disclaimer_banner) return renderElement(request, site) def render_POST(self, request): @@ -137,7 +130,9 @@ class LoginResource(BaseResource): log.error('Authentication error for %s' % request.args['username'][0]) log.error('%s' % error) request.setResponseCode(UNAUTHORIZED) - return self._render_template(request, 'Invalid username or password') + content = util.redirectTo("/login?auth", request) + request.write(content) + request.finish() d = self._handle_login(request) d.addCallbacks(render_response, render_error) diff --git a/service/test/integration/test_multi_user_login.py b/service/test/integration/test_multi_user_login.py index fe456583..b04a4e9e 100644 --- a/service/test/integration/test_multi_user_login.py +++ b/service/test/integration/test_multi_user_login.py @@ -13,7 +13,6 @@ # # You should have received a copy of the GNU Affero General Public License # along with Pixelated. If not, see <http://www.gnu.org/licenses/>. -from mock import patch from twisted.internet import defer @@ -47,8 +46,8 @@ class MultiUserLoginTest(MultiUserSoledadTestBase): self.assertEquals(val, response[key]) @defer.inlineCallbacks - def test_wrong_credentials_cannot_access_resources(self): + def test_wrong_credentials_is_redirected_to_login(self): response, login_request = self.app_test_client.login('username', 'wrong_password') - response_str = yield response - self.assertEqual(401, login_request.responseCode) - self.assertIn('Invalid username or password', login_request.written) + yield response + self.assertEqual(302, login_request.responseCode) + self.assertIn('/login?auth-error', login_request.uri) diff --git a/service/test/unit/resources/test_login_resource.py b/service/test/unit/resources/test_login_resource.py index bd0f9122..733583a3 100644 --- a/service/test/unit/resources/test_login_resource.py +++ b/service/test/unit/resources/test_login_resource.py @@ -203,22 +203,23 @@ class TestLoginPOST(unittest.TestCase): return d @patch('pixelated.config.leap.BootstrapUserServices.setup') + @patch('twisted.web.util.redirectTo') @patch('pixelated.authentication.Authenticator.authenticate') - def test_should_return_form_back_with_error_message_when_login_fails(self, mock_authenticate, - mock_user_bootstrap_setup): + def test_should_redirect_to_login_with_error_flag_when_login_fails(self, mock_authenticate, + mock_redirect, + mock_user_bootstrap_setup): mock_authenticate.side_effect = UnauthorizedLogin() + mock_redirect.return_value = "mocked redirection" d = self.web.get(self.request) - def assert_error_response_and_user_services_not_setup(_): + def assert_redirected_to_login(_): mock_authenticate.assert_called_once_with(self.username, self.password) - self.assertEqual(401, self.request.responseCode) - written_response = ''.join(self.request.written) - self.assertIn('Invalid username or password', written_response) + mock_redirect.assert_called_once_with('/login?auth-error', self.request) self.assertFalse(mock_user_bootstrap_setup.called) self.assertFalse(self.resource.get_session(self.request).is_logged_in()) - d.addCallback(assert_error_response_and_user_services_not_setup) + d.addCallback(assert_redirected_to_login) return d @patch('pixelated.config.leap.BootstrapUserServices.setup') diff --git a/web-ui/app/locales/en_US/translation.json b/web-ui/app/locales/en_US/translation.json index ffbce321..7aad4fe8 100644 --- a/web-ui/app/locales/en_US/translation.json +++ b/web-ui/app/locales/en_US/translation.json @@ -64,7 +64,8 @@ "error": { "timeout": "A timeout occurred", "general": "Problems talking to server", - "parse": "Got invalid response from server" + "parse": "Got invalid response from server", + "auth": "Invalid email or password" }, "tags": { "inbox": "Inbox", diff --git a/web-ui/app/locales/pt_BR/translation.json b/web-ui/app/locales/pt_BR/translation.json index 45d4a210..b38b7535 100644 --- a/web-ui/app/locales/pt_BR/translation.json +++ b/web-ui/app/locales/pt_BR/translation.json @@ -64,7 +64,8 @@ "error": { "timeout": "A operação excedeu o limite de tempo", "general": "Problemas ao se comunicar com o servidor", - "parse": "Obteve uma resposta inválida do servidor" + "parse": "Obteve uma resposta inválida do servidor", + "auth": "E-mail ou senha inválidos" }, "tags": { "inbox": "Caixa de Entrada", diff --git a/web-ui/src/login/app.js b/web-ui/src/login/app.js index ee5a7652..07099c60 100644 --- a/web-ui/src/login/app.js +++ b/web-ui/src/login/app.js @@ -22,16 +22,26 @@ import SubmitButton from 'src/common/submit_button/submit_button'; import './app.scss'; -export const App = ({ t }) => ( - <form className='standard' id='login_form' action='/login' method='post'> - <InputField name='username' label={t('login.email')} /> - <InputField type='password' name='password' label={t('login.password')} /> - <SubmitButton buttonText={t('login.submit')} /> - </form> +const errorMessage = (t, authError) => { + if (authError) return <p className='error'>{t('error.auth')}</p>; + return <div />; +}; + +export const App = ({ t, authError }) => ( + <div className='login'> + <img className='logo' src='/public/images/logo-orange.svg' alt='Pixelated logo' /> + {errorMessage(t, authError)} + <form className='standard' id='login_form' action='/login' method='post'> + <InputField name='username' label={t('login.email')} /> + <InputField type='password' name='password' label={t('login.password')} /> + <SubmitButton buttonText={t('login.submit')} /> + </form> + </div> ); App.propTypes = { - t: React.PropTypes.func.isRequired + t: React.PropTypes.func.isRequired, + authError: React.PropTypes.bool }; export default translate('', { wait: true })(App); diff --git a/web-ui/src/login/app.scss b/web-ui/src/login/app.scss index 76390cb7..f971750f 100644 --- a/web-ui/src/login/app.scss +++ b/web-ui/src/login/app.scss @@ -15,16 +15,55 @@ * along with Pixelated. If not, see <http://www.gnu.org/licenses/>. */ + +.error { + color: #D72A25; + margin: 10px 0 0 0; +} + +.login { + display: block; + width: 90%; + margin: auto; + max-width: 400px; + padding: 2em 0; + margin-top: 3%; + margin-bottom: 3%; + background-color: #FFF; + display: flex; + flex-direction: column; + align-items: center; +} + #login_form { padding: 20px 0; + width: 70%; .input-field-group { width: 100%; } + + .submit-button { + width: 100%; + } +} + +.logo { + width: 70%; } @media only screen and (min-width : 500px) { #login_form .input-field-group { margin-top: 1em; } + + .login { + width: 60%; + } +} + +@media only screen and (min-width : 960px) { + .login { + width: 40%; + } } diff --git a/web-ui/src/login/login.css b/web-ui/src/login/login.css index bbb37443..d1206a39 100644 --- a/web-ui/src/login/login.css +++ b/web-ui/src/login/login.css @@ -25,29 +25,6 @@ body { background-repeat: repeat; } -.error { - color: #D72A25; - margin: 10px 0 0 0; -} - -.login { - display: block; - width: 90%; - margin: auto; - max-width: 400px; - padding: 2em 0; - margin-top: 3%; - margin-bottom: 3%; - background-color: #FFF; - display: flex; - flex-direction: column; - align-items: center; -} - -#root { - width: 70%; -} - .disclaimer { display: block; width: 90%; @@ -68,22 +45,18 @@ body { margin-top: 1em; } -.logo { - width: 70%; -} - @media only screen and (min-width : 500px) { body { - font-size: 1.3em; + font-size: 1.2em; } - .login, .disclaimer { + .disclaimer { width: 60%; } } @media only screen and (min-width : 960px) { - .login, .disclaimer { + .disclaimer { width: 40%; } } diff --git a/web-ui/src/login/login.html b/web-ui/src/login/login.html index 40593096..3cebf6f4 100644 --- a/web-ui/src/login/login.html +++ b/web-ui/src/login/login.html @@ -12,11 +12,7 @@ </head> <body> <div class="content"> - <div class="login"> - <img class="logo" src="/public/images/logo-orange.svg" alt="Pixelated logo"/> - <p t:render="error_msg" class="error"></p> - <div id="root"/> - </div> + <div id="root"/> <div class="disclaimer"> <div class="disclaimer-content"> <div t:render="disclaimer"></div> diff --git a/web-ui/src/login/login.js b/web-ui/src/login/login.js index 9a8c9042..ed25b9a2 100644 --- a/web-ui/src/login/login.js +++ b/web-ui/src/login/login.js @@ -22,12 +22,13 @@ import { I18nextProvider } from 'react-i18next'; import AppWrapper from './app'; import i18n from '../i18n'; +import { hasQueryParameter } from '../util'; if (process.env.NODE_ENV === 'development') a11y(React); render( <I18nextProvider i18n={i18n}> - <AppWrapper /> + <AppWrapper authError={hasQueryParameter('auth')} /> </I18nextProvider>, document.getElementById('root') ); diff --git a/web-ui/src/util.js b/web-ui/src/util.js new file mode 100644 index 00000000..1b244458 --- /dev/null +++ b/web-ui/src/util.js @@ -0,0 +1,7 @@ +export const hasQueryParameter = param => ( + decodeURIComponent(window.location.search.substring(1)).includes(param) +); + +export default { + hasQueryParameter +}; diff --git a/web-ui/test/unit/login/app.spec.js b/web-ui/test/unit/login/app.spec.js index 3b6461cf..347e2b19 100644 --- a/web-ui/test/unit/login/app.spec.js +++ b/web-ui/test/unit/login/app.spec.js @@ -5,13 +5,22 @@ import { App } from 'src/login/app'; describe('App', () => { let app; + const mockTranslations = key => key; beforeEach(() => { - const mockTranslations = key => key; app = shallow(<App t={mockTranslations} />); }); it('renders login form', () => { expect(app.find('form').props().action).toEqual('/login'); }); + + it('renders auth error message', () => { + app = shallow(<App t={mockTranslations} authError />); + expect(app.find('.error').length).toEqual(1); + }); + + it('does not render auth error message', () => { + expect(app.find('.error').length).toEqual(0); + }); }); diff --git a/web-ui/test/unit/util.spec.js b/web-ui/test/unit/util.spec.js new file mode 100644 index 00000000..84decf6f --- /dev/null +++ b/web-ui/test/unit/util.spec.js @@ -0,0 +1,20 @@ +import expect from 'expect'; +import Util from 'src/util'; + +describe('Utils', () => { + describe('.hasQueryParameter', () => { + global.window = { + location: { + search: '?auth&lng=pt-BR' + } + }; + + it('checks if param included in query parameters', () => { + expect(Util.hasQueryParameter('auth')).toBe(true); + }); + + it('checks if param not included in query parameters', () => { + expect(Util.hasQueryParameter('error')).toBe(false); + }); + }); +}); |