Merge lp:~nataliabidart/ubuntu-sso-client/better-errors into lp:ubuntu-sso-client

[Natalia Bidart]

Errors from SSO servers are being shown now to users, matching error-specific to fields (LP: #616101).
Also, be robust when SSO server answer with a string where it's supposed to be a list (LP: #623447).

Errors from SSO servers are being shown now to users, matching error-specific to fields.

[Natalia Bidart (nataliabidart)]

To test this, you should try both registration and login using email address like '@' (that will make the GUI accept the address as valid but SSO will reject it).

You can also try registering with an already registered email address, etc.

Also, you can try logging with non existing accounts, and resetting passwords for non-existing accounts, and resetting password for a valid account but using a bad token.

[lp:~nataliabidart/ubuntu-sso-client/better-errors]

603. By Natalia Bidart on 2010-08-26

Merged trunk in.

[Vincenzo Di Somma (vds)]

review: Approve

[John Lenton (chipaca)]

review: Approve

=== modified file 'ubuntu_sso/gui.py'
--- ubuntu_sso/gui.py	2010-08-26 18:44:24 +0000
+++ ubuntu_sso/gui.py	2010-08-26 22:54:41 +0000
@@ -547,7 +547,6 @@
 
     def _build_success_page(self):
         """Build the success page."""
-        #self.success_vbox.help_text = ''
         self.success_label.set_markup('<span size="x-large">%s</span>' %
                                       self.SUCCESS)
         return self.success_vbox
@@ -846,6 +845,17 @@
 
     # backend callbacks
 
+    def _build_general_error_message(self, errordict):
+        """Concatenate __all__ and message from the errordict."""
+        result = None
+        msg1 = errordict.get('__all__')
+        msg2 = errordict.get('message')
+        if msg1 is not None and msg2 is not None:
+            result = '\n'.join((msg1, msg2))
+        else:
+            result = msg1 if msg1 is not None else msg2
+        return result
+
     @log_call
     def on_captcha_generated(self, app_name, captcha_id, *args, **kwargs):
         """Captcha image has been generated and is available to be shown."""
@@ -868,10 +878,21 @@
     @log_call
     def on_user_registration_error(self, app_name, error, *args, **kwargs):
         """Captcha image generation failed."""
-        self._set_current_page(self.enter_details_vbox,
-                               warning_text=self.UNKNOWN_ERROR)
+        msg = error.get('email')
+        if msg is not None:
+            self.email1_entry.set_warning(msg)
+            self.email2_entry.set_warning(msg)
+
+        msg = error.get('password')
+        if msg is not None:
+            self.password1_entry.set_warning(msg)
+            self.password2_entry.set_warning(msg)
+
+        msg = self._build_general_error_message(error)
+        self._set_current_page(self.enter_details_vbox, warning_text=msg)
+
         self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name,
-                                     error))
+                                    msg))
 
     @log_call
     def on_email_validated(self, app_name, email, *args, **kwargs):
@@ -883,10 +904,15 @@
     @log_call
     def on_email_validation_error(self, app_name, error, *args, **kwargs):
         """User email validation failed."""
-        self._set_current_page(self.verify_email_vbox,
-                               warning_text=self.UNKNOWN_ERROR)
+        msg = error.get('email_token')
+        if msg is not None:
+            self.email_token_entry.set_warning(msg)
+
+        msg = self._build_general_error_message(error)
+        self._set_current_page(self.verify_email_vbox, warning_text=msg)
+
         self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name,
-                                     error))
+                                     msg))
 
     @log_call
     def on_logged_in(self, app_name, email, *args, **kwargs):
@@ -898,10 +924,10 @@
     @log_call
     def on_login_error(self, app_name, error, *args, **kwargs):
         """User was not successfully logged in."""
-        self._set_current_page(self.login_vbox,
-                               warning_text=self.UNKNOWN_ERROR)
+        msg = self._build_general_error_message(error)
+        self._set_current_page(self.login_vbox, warning_text=msg)
         self._gtk_signal_log.append((SIG_LOGIN_FAILED, self.app_name,
-                                     error))
+                                     msg))
 
     @log_call
     def on_password_reset_token_sent(self, app_name, email, *args, **kwargs):
@@ -913,8 +939,8 @@
     @log_call
     def on_password_reset_error(self, app_name, error, *args, **kwargs):
         """Password reset failed."""
-        self._set_current_page(self.login_vbox,
-                               warning_text=self.UNKNOWN_ERROR)
+        msg = self._build_general_error_message(error)
+        self._set_current_page(self.login_vbox, warning_text=msg)
 
     @log_call
     def on_password_changed(self, app_name, email, *args, **kwargs):
@@ -925,5 +951,6 @@
     @log_call
     def on_password_change_error(self, app_name, error, *args, **kwargs):
         """Password reset failed."""
+        msg = self._build_general_error_message(error)
         self._set_current_page(self.request_password_token_vbox,
-                               warning_text=self.UNKNOWN_ERROR)
+                               warning_text=msg)

=== modified file 'ubuntu_sso/main.py'
--- ubuntu_sso/main.py	2010-08-26 21:06:36 +0000
+++ ubuntu_sso/main.py	2010-08-26 22:54:41 +0000
@@ -139,8 +139,12 @@
     def _format_webservice_errors(self, errdict):
         """Turn each list of strings in the errdict into a LF separated str."""
         result = {}
-        for k in errdict:
-            result[k] = "\n".join(errdict[k])
+        for k, v in errdict.iteritems():
+            # workaround until bug #624955 is solved
+            if isinstance(v, basestring):
+                result[k] = v
+            else:
+                result[k] = "\n".join(v)
         return result
 
     def generate_captcha(self, filename):
@@ -239,7 +243,7 @@
             result = service(email=email)
         except HTTPError, e:
             logger.exception('request_password_reset_token failed with:')
-            raise ResetPasswordTokenError(e.content)
+            raise ResetPasswordTokenError(e.content.split('\n')[0])
 
         if result['status'] == 'ok':
             return email
@@ -261,7 +265,7 @@
                              new_password=new_password)
         except HTTPError, e:
             logger.exception('set_new_password failed with:')
-            raise NewPasswordError(e.content)
+            raise NewPasswordError(e.content.split('\n')[0])
 
         if result['status'] == 'ok':
             return email
@@ -273,7 +277,6 @@
     """Turn an exception into a dictionary to return thru DBus."""
     result = {
         "errtype": e.__class__.__name__,
-        "errargs": repr(e.args),
     }
     if len(e.args) == 0:
         result["message"] = e.__class__.__doc__

=== modified file 'ubuntu_sso/tests/test_gui.py'
--- ubuntu_sso/tests/test_gui.py	2010-08-26 18:44:24 +0000
+++ ubuntu_sso/tests/test_gui.py	2010-08-26 22:54:41 +0000
@@ -44,7 +44,6 @@
 CAPTCHA_SOLUTION = 'william Byrd'
 EMAIL = '[e-mail]'
 EMAIL_TOKEN = 'B2Pgtf'
-ERROR = 'Something bad happened!'
 NAME = 'Juanito Pérez'
 PASSWORD = 'h3lloWorld'
 RESET_PASSWORD_TOKEN = '8G5Wtq'
@@ -344,6 +343,7 @@
                       'tc_browser', 'login', 'request_password_token',
                       'set_new_password')
         self.ui = self.gui_class(**self.kwargs)
+        self.ERROR = {'message': self.ui.UNKNOWN_ERROR}
 
     def tearDown(self):
         """Clean up."""
@@ -866,17 +866,39 @@
         """Init."""
         super(UserRegistrationErrorTestCase, self).setUp()
         self.click_join_with_valid_data()
-        self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
 
     def test_previous_page_is_shown(self):
         """On UserRegistrationError the previous page is shown."""
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_pages_visibility(enter_details=True)
 
     def test_warning_label_is_shown(self):
         """On UserRegistrationError the warning label is shown."""
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_correct_label_warning(self.ui.warning_label,
                                           self.ui.UNKNOWN_ERROR)
 
+    def test_specific_errors_from_backend_are_shown(self):
+        """Specific errors from backend are used."""
+        error = {'errtype': 'RegistrationError',
+                 'message': 'We\'re so doomed.',
+                 'email': 'Enter a valid e-mail address.',
+                 'password': 'I don\'t like your password.',
+                 '__all__': 'Wrong captcha solution.'}
+
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=error)
+
+        expected = '\n'.join((error['__all__'], error['message']))
+        self.assert_correct_label_warning(self.ui.warning_label, expected)
+        self.assert_correct_entry_warning(self.ui.email1_entry,
+                                          error['email'])
+        self.assert_correct_entry_warning(self.ui.email2_entry,
+                                          error['email'])
+        self.assert_correct_entry_warning(self.ui.password1_entry,
+                                          error['password'])
+        self.assert_correct_entry_warning(self.ui.password2_entry,
+                                          error['password'])
+
 
 class VerifyEmailTestCase(UbuntuSSOClientTestCase):
     """Test suite for the user registration (verify email page)."""
@@ -926,19 +948,33 @@
         self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
         self.assert_pages_visibility(success=True)
 
-    def test_on_email_validated_clears_the_help_text(self):
-        """On email validated the help text is removed."""
+    def test_on_email_validated_does_not_clear_the_help_text(self):
+        """On email validated the help text is not removed."""
         self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
-        self.assertEqual('', self.ui.help_label.get_text())
-    test_on_email_validated_clears_the_help_text.skip = 'Maybe this is wrong.'
+        self.assertEqual(self.ui.verify_email_vbox.help_text,
+                         self.ui.help_label.get_label())
 
     def test_on_email_validation_error_verify_email_is_shown(self):
         """On email validation error, the verify_email page is shown."""
-        self.ui.on_email_validation_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_email_validation_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_pages_visibility(verify_email=True)
         self.assert_correct_label_warning(self.ui.warning_label,
                                           self.ui.UNKNOWN_ERROR)
 
+    def test_specific_errors_from_backend_are_shown(self):
+        """Specific errors from backend are used."""
+        error = {'errtype': 'EmailValidationError',
+                 'message': 'We\'re so doomed.',
+                 'email_token': 'Enter a valid e-mail address.',
+                 '__all__': 'We all are gonna die.'}
+
+        self.ui.on_email_validation_error(app_name=APP_NAME, error=error)
+
+        expected = '\n'.join((error['__all__'], error['message']))
+        self.assert_correct_label_warning(self.ui.warning_label, expected)
+        self.assert_correct_entry_warning(self.ui.email_token_entry,
+                                          error['email_token'])
+
     def test_success_label_is_correct(self):
         """The success message is correct."""
         self.assertEqual(self.ui.SUCCESS, self.ui.success_label.get_text())
@@ -1152,20 +1188,31 @@
     def test_on_login_error_morphs_to_login_page(self):
         """On user login error, the previous page is shown."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_pages_visibility(login=True)
 
     def test_on_login_error_a_warning_is_shown(self):
         """On user login error, a warning is shown with proper wording."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_correct_label_warning(self.ui.warning_label,
                                           self.ui.UNKNOWN_ERROR)
 
+    def test_specific_errors_from_backend_are_shown(self):
+        """Specific errors from backend are used."""
+        error = {'errtype': 'AuthenticationError',
+                 'message': 'We\'re so doomed.',
+                 '__all__': 'We all are gonna die.'}
+
+        self.ui.on_login_error(app_name=APP_NAME, error=error)
+
+        expected = '\n'.join((error['__all__'], error['message']))
+        self.assert_correct_label_warning(self.ui.warning_label, expected)
+
     def test_back_to_registration_hides_warning(self):
         """After user login error, warning is hidden when clicking 'Back'."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.login_back_button.clicked()
         self.assertFalse(self.ui.warning_label.get_property('visible'))
 
@@ -1317,11 +1364,22 @@
 
     def test_on_password_reset_error_shows_login_page(self):
         """When reset token wasn't successfuly sent the login page is shown."""
-        self.ui.on_password_reset_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_password_reset_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_correct_label_warning(self.ui.warning_label,
                                           self.ui.UNKNOWN_ERROR)
         self.assert_pages_visibility(login=True)
 
+    def test_specific_errors_from_backend_are_shown(self):
+        """Specific errors from backend are used."""
+        error = {'errtype': 'ResetPasswordTokenError',
+                 'message': 'We\'re so doomed.',
+                 '__all__': 'We all are gonna die.'}
+
+        self.ui.on_password_reset_error(app_name=APP_NAME, error=error)
+
+        expected = '\n'.join((error['__all__'], error['message']))
+        self.assert_correct_label_warning(self.ui.warning_label, expected)
+
 
 class SetNewPasswordTestCase(UbuntuSSOClientTestCase):
     """Test suite for setting a new password functionality."""
@@ -1374,11 +1432,22 @@
 
     def test_on_password_change_error_shows_login_page(self):
         """When password wasn't changed the reset password page is shown."""
-        self.ui.on_password_change_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_password_change_error(app_name=APP_NAME, error=self.ERROR)
         self.assert_correct_label_warning(self.ui.warning_label,
                                           self.ui.UNKNOWN_ERROR)
         self.assert_pages_visibility(request_password_token=True)
 
+    def test_specific_errors_from_backend_are_shown(self):
+        """Specific errors from backend are used."""
+        error = {'errtype': 'NewPasswordError',
+                 'message': 'We\'re so doomed.',
+                 '__all__': 'We all are gonna die.'}
+
+        self.ui.on_password_change_error(app_name=APP_NAME, error=error)
+
+        expected = '\n'.join((error['__all__'], error['message']))
+        self.assert_correct_label_warning(self.ui.warning_label, expected)
+
 
 class DbusTestCase(UbuntuSSOClientTestCase):
     """Test suite for the dbus calls."""
@@ -1458,9 +1527,10 @@
 
     def test_on_user_registration_error_proper_signal_is_emitted(self):
         """On UserRegistrationError, 'registration-failed' signal is sent."""
-        self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
+        expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
+        self.assertEqual(expected,
                          self._called[gui.SIG_REGISTRATION_FAILED])
 
     def test_on_email_validated_proper_signals_is_emitted(self):
@@ -1472,10 +1542,11 @@
 
     def test_on_email_validation_error_proper_signals_is_emitted(self):
         """On EmailValidationError, 'registration-failed' signal is sent."""
-        self.ui.on_email_validation_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_email_validation_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
-                          self._called[gui.SIG_REGISTRATION_FAILED])
+        expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
+        self.assertEqual(expected,
+                         self._called[gui.SIG_REGISTRATION_FAILED])
 
     def test_on_logged_in_proper_signals_is_emitted(self):
         """On LoggedIn, 'login-succeeded' signal is sent."""
@@ -1487,15 +1558,16 @@
     def test_on_login_error_proper_signals_is_emitted(self):
         """On LoginError, 'login-failed' signal is sent."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
-                          self._called[gui.SIG_LOGIN_FAILED])
+        expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
+        self.assertEqual(expected,
+                         self._called[gui.SIG_LOGIN_FAILED])
 
     def test_registration_successfull_even_if_prior_registration_error(self):
         """Only one signal is sent with the final outcome."""
         self.click_join_with_valid_data()
-        self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
         self.click_join_with_valid_data()
         self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
         self.ui.on_close_clicked()
@@ -1506,7 +1578,7 @@
     def test_login_successfull_even_if_prior_login_error(self):
         """Only one signal is sent with the final outcome."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.click_connect_with_valid_data()
         self.ui.on_logged_in(app_name=APP_NAME, email=EMAIL)
         self.ui.on_close_clicked()
@@ -1517,7 +1589,7 @@
     def test_user_cancelation_even_if_prior_registration_error(self):
         """Only one signal is sent with the final outcome."""
         self.click_join_with_valid_data()
-        self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.join_cancel_button.clicked()
 
         self.assertEqual(len(self._called), 1)
@@ -1526,7 +1598,7 @@
     def test_user_cancelation_even_if_prior_login_error(self):
         """Only one signal is sent with the final outcome."""
         self.click_connect_with_valid_data()
-        self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
+        self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
         self.ui.login_cancel_button.clicked()
 
         self.assertEqual(len(self._called), 1)

=== modified file 'ubuntu_sso/tests/test_main.py'
--- ubuntu_sso/tests/test_main.py	2010-08-26 21:04:47 +0000
+++ ubuntu_sso/tests/test_main.py	2010-08-26 22:54:41 +0000
@@ -54,6 +54,7 @@
                               "Can't reset password for this account"
 RESET_TOKEN_INVALID_CONTENT = "AuthToken matching query does not exist."
 EMAIL = '[e-mail]'
+EMAIL_ALREADY_REGISTERED = '[e-mail]'
 EMAIL_TOKEN = 'B2Pgtf'
 HELP = 'help text'
 PASSWORD = 'be4tiFul'
@@ -102,7 +103,10 @@
 
     def register(self, email, password, captcha_id, captcha_solution):
         """Fake registration. Return a fix result."""
-        if captcha_id is None and captcha_solution is None:
+        if email == EMAIL_ALREADY_REGISTERED:
+            return {'status': 'error',
+                    'errors': {'email': 'Email already registered'}}
+        elif captcha_id is None and captcha_solution is None:
             return STATUS_UNKNOWN
         elif captcha_id != CAPTCHA_ID or captcha_solution != CAPTCHA_SOLUTION:
             return STATUS_ERROR
@@ -146,7 +150,10 @@
         """Fake the email validation. Return a fix result."""
         if email_token is None:
             return STATUS_EMAIL_UNKNOWN
-        if email_token != EMAIL_TOKEN:
+        elif email_token == EMAIL_ALREADY_REGISTERED:
+            return {'status': 'error',
+                    'errors': {'email': 'Email already registered'}}
+        elif email_token != EMAIL_TOKEN:
             return STATUS_EMAIL_ERROR
         else:
             return STATUS_EMAIL_OK
@@ -234,6 +241,16 @@
             self.assertIn(k, STATUS_ERROR['errors'])
             self.assertEqual(v, "\n".join(STATUS_ERROR['errors'][k]))
 
+    def test_register_user_if_status_error_with_string_message(self):
+        """Proper error is raised if register fails."""
+        self.register_kwargs['email'] = EMAIL_ALREADY_REGISTERED
+        failure = self.assertRaises(RegistrationError,
+                                    self.processor.register_user,
+                                    **self.register_kwargs)
+        for k, v in failure.args[0].items():
+            self.assertIn(k, {'email': 'Email already registered'})
+            self.assertEqual(v, 'Email already registered')
+
     def test_register_user_if_status_unknown(self):
         """Proper error is raised if register returns an unknown status."""
         self.register_kwargs['captcha_id'] = None
@@ -274,6 +291,16 @@
             self.assertIn(k, STATUS_EMAIL_ERROR['errors'])
             self.assertEqual(v, "\n".join(STATUS_EMAIL_ERROR['errors'][k]))
 
+    def test_validate_email_if_status_error_with_string_message(self):
+        """Proper error is raised if register fails."""
+        self.login_kwargs['email_token'] = EMAIL_ALREADY_REGISTERED
+        failure = self.assertRaises(EmailTokenError,
+                                    self.processor.validate_email,
+                                    **self.login_kwargs)
+        for k, v in failure.args[0].items():
+            self.assertIn(k, {'email': 'Email already registered'})
+            self.assertEqual(v, 'Email already registered')
+
     def test_validate_email_if_status_unknown(self):
         """Proper error is raised if email validation returns unknown."""
         self.login_kwargs['email_token'] = None
@@ -797,7 +824,6 @@
         e = TestExceptToErrdictException(*sample_args)
         result = except_to_errdict(e)
         self.assertEqual(result["errtype"], e.__class__.__name__)
-        self.assertEqual(result["errargs"], repr(sample_args))
 
 
 class KeyringCredentialsTestCase(MockerTestCase):