From fa21608801f8d2ef710d4c28abbb558883afeaf7 Mon Sep 17 00:00:00 2001
From: Tulio Casagrande <tcasagra@thoughtworks.com>
Date: Mon, 20 Feb 2017 14:37:37 -0300
Subject: [#907] Translate auth error message on login

with @anikarni
---
 service/pixelated/resources/login_resource.py      | 17 ++++------
 service/test/integration/test_multi_user_login.py  |  9 +++--
 service/test/unit/resources/test_login_resource.py | 15 +++++----
 web-ui/app/locales/en_US/translation.json          |  3 +-
 web-ui/app/locales/pt_BR/translation.json          |  3 +-
 web-ui/src/login/app.js                            | 24 +++++++++----
 web-ui/src/login/app.scss                          | 39 ++++++++++++++++++++++
 web-ui/src/login/login.css                         | 33 ++----------------
 web-ui/src/login/login.html                        |  6 +---
 web-ui/src/login/login.js                          |  3 +-
 web-ui/src/util.js                                 |  7 ++++
 web-ui/test/unit/login/app.spec.js                 | 11 +++++-
 web-ui/test/unit/util.spec.js                      | 20 +++++++++++
 13 files changed, 121 insertions(+), 69 deletions(-)
 create mode 100644 web-ui/src/util.js
 create mode 100644 web-ui/test/unit/util.spec.js

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,17 +70,10 @@ 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);
+    });
+  });
+});
-- 
cgit v1.2.3