Merge lp:~alecu/ubuntu-sso-client/properly-return-errors into lp:ubuntu-sso-client

[Alejandro J. Cura]

Make all the signals return the errors as dictionaries (LP: #616105).
Credential* signals should return the app_name as well (LP: #621377).

Make all the signals return the errors as dictionaries (LP: #616105).
Credential* signals should return the app_name as well (LP: #621377).

[Rodrigo Moya (rodrigo-moya)]

review: Approve

[Natalia Bidart (nataliabidart)]

review: Approve

=== modified file 'ubuntu_sso/gui.py'
--- ubuntu_sso/gui.py	2010-08-19 20:56:52 +0000
+++ ubuntu_sso/gui.py	2010-08-23 19:43:39 +0000
@@ -57,11 +57,11 @@
 SIG_USER_CANCELATION = 'user-cancelation'
 
 SIGNAL_ARGUMENTS = [
-    (SIG_LOGIN_FAILED, ()),
+    (SIG_LOGIN_FAILED, (gobject.TYPE_STRING,)),
     (SIG_LOGIN_SUCCEEDED, (gobject.TYPE_STRING,)),
-    (SIG_REGISTRATION_FAILED, ()),
+    (SIG_REGISTRATION_FAILED, (gobject.TYPE_STRING,)),
     (SIG_REGISTRATION_SUCCEEDED, (gobject.TYPE_STRING,)),
-    (SIG_USER_CANCELATION, ()),
+    (SIG_USER_CANCELATION, (gobject.TYPE_STRING,)),
 ]
 
 for sig, args in SIGNAL_ARGUMENTS:
@@ -586,7 +586,7 @@
             self.window.hide()
 
         if len(args) > 0 and args[0] in self.cancels:
-            self.window.emit(SIG_USER_CANCELATION)
+            self.window.emit(SIG_USER_CANCELATION, self.app_name)
         elif len(self._gtk_signal_log) > 0:
             signal = self._gtk_signal_log[-1][0]
             args = self._gtk_signal_log[-1][1:]
@@ -823,7 +823,7 @@
                        args, kwargs)
         self._set_current_page(self.enter_details_vbox,
                                warning_text=self.UNKNOWN_ERROR)
-        self._gtk_signal_log.append((SIG_REGISTRATION_FAILED,))
+        self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name))
 
     def on_email_validated(self, app_name, *args, **kwargs):
         """User email was successfully verified."""
@@ -838,7 +838,7 @@
                        args, kwargs)
         self._set_current_page(self.verify_email_vbox,
                                warning_text=self.UNKNOWN_ERROR)
-        self._gtk_signal_log.append((SIG_REGISTRATION_FAILED,))
+        self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name))
 
     def on_logged_in(self, app_name, *args, **kwargs):
         """User was successfully logged in."""
@@ -853,7 +853,7 @@
                        args, kwargs)
         self._set_current_page(self.login_vbox,
                                warning_text=self.UNKNOWN_ERROR)
-        self._gtk_signal_log.append((SIG_LOGIN_FAILED,))
+        self._gtk_signal_log.append((SIG_LOGIN_FAILED, self.app_name))
 
     def on_password_reset_token_sent(self, email, *args, **kwargs):
         """Password reset token was successfully sent."""

=== modified file 'ubuntu_sso/main.py'
--- ubuntu_sso/main.py	2010-08-20 18:04:28 +0000
+++ ubuntu_sso/main.py	2010-08-23 19:43:39 +0000
@@ -31,6 +31,7 @@
 import socket
 import time
 import threading
+import traceback
 import urllib
 import urllib2
 import urlparse
@@ -115,7 +116,7 @@
             creds = {
                 'consumer_key': "ubuntuone",
                 'consumer_secret': "hammertime",
-                'name': U1_APP_NAME,
+                'token_name': OLD_KEY_NAME,
                 'token': old_creds["oauth_token"],
                 'token_secret': old_creds["oauth_token_secret"],
             }
@@ -280,6 +281,14 @@
             raise NewPasswordError('Received invalid reply: %s' % result)
 
 
+def except_to_errdict(e):
+    """Turn an exception into a dictionary to return thru DBus."""
+    return {
+        "errtype": type(e).__name__,
+        "message": "\n".join(str(arg) for arg in e.args),
+    }
+
+
 def blocking(f, result_cb, error_cb):
     """Run f in a thread; return or throw an exception thru the callbacks."""
     def _in_thread():
@@ -289,7 +298,7 @@
         except Exception, e:
             msg = "Exception while running DBus blocking code in a thread."""
             logger.exception(msg)
-            error_cb(str(e))
+            error_cb(except_to_errdict(e))
     threading.Thread(target=_in_thread).start()
 
 
@@ -315,7 +324,7 @@
     def CaptchaGenerated(self, result):
         """Signal thrown after the captcha is generated."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def CaptchaGenerationError(self, error):
         """Signal thrown when there's a problem generating the captcha."""
 
@@ -333,7 +342,7 @@
     def UserRegistered(self, result):
         """Signal thrown when the user is registered."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def UserRegistrationError(self, error):
         """Signal thrown when there's a problem registering the user."""
 
@@ -352,7 +361,7 @@
     def LoggedIn(self, result):
         """Signal thrown when the user is logged in."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def LoginError(self, error):
         """Signal thrown when there is a problem in the login."""
 
@@ -372,7 +381,7 @@
     def EmailValidated(self, app_name):
         """Signal thrown after the email is validated."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def EmailValidationError(self, error):
         """Signal thrown when there's a problem validating the email."""
 
@@ -392,7 +401,7 @@
     def PasswordResetTokenSent(self, email):
         """Signal thrown when the token is succesfully sent."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def PasswordResetError(self, error):
         """Signal thrown when there's a problem sending the token."""
 
@@ -411,7 +420,7 @@
     def PasswordChanged(self, email):
         """Signal thrown when the token is succesfully sent."""
 
-    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="s")
+    @dbus.service.signal(DBUS_IFACE_USER_NAME, signature="a{ss}")
     def PasswordChangeError(self, error):
         """Signal thrown when there's a problem sending the token."""
 
@@ -429,16 +438,16 @@
 class SSOCredentials(dbus.service.Object):
     """DBus object that gets credentials, and login/registers if needed."""
 
-    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="")
-    def AuthorizationDenied(self):
+    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="s")
+    def AuthorizationDenied(self, app_name):
         """Signal thrown when the user denies the authorization."""
 
-    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="a{ss}")
-    def CredentialsFound(self, app_name):
+    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="sa{ss}")
+    def CredentialsFound(self, app_name, credentials):
         """Signal thrown when the credentials are found."""
 
-    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="s")
-    def CredentialsError(self, error):
+    @dbus.service.signal(DBUS_IFACE_CRED_NAME, signature="sss")
+    def CredentialsError(self, app_name, error_message, detailed_error):
         """Signal thrown when there is a problem finding the credentials."""
 
     @dbus.service.method(dbus_interface=DBUS_IFACE_CRED_NAME,
@@ -455,20 +464,21 @@
         """Handles the response from the UI dialog."""
         try:
             creds = keyring_get_credentials(app_name)
-            self.CredentialsFound(creds)
-        except Exception, e:
-            logger.exception("problem when getting credentials from keyring")
-            self.CredentialsError(str(e))
+            self.CredentialsFound(app_name, creds)
+        except Exception:
+            msg = "Problem getting the credentials from the keyring."
+            logger.exception(msg)
+            self.CredentialsError(app_name, msg, traceback.format_exc())
 
-    def _login_error_cb(self, dialog):
+    def _login_error_cb(self, dialog, app_name):
         """Handles a problem in the UI."""
-        msg = "misc problem getting the credentials"
-        self.CredentialsError(msg)
+        msg = "Problem getting the credentials from the keyring."
+        self.CredentialsError(app_name, msg, "no more info available")
         logger.error(msg)
 
-    def _login_auth_denied_cb(self, dialog):
+    def _login_auth_denied_cb(self, dialog, app_name):
         """The user decides not to allow the registration or login."""
-        self.AuthorizationDenied()
+        self.AuthorizationDenied(app_name)
 
     def _show_login_or_register_ui(self, app_name, tc_url, help_text,
                                    win_id, login_only=False):
@@ -483,12 +493,13 @@
             gui_app.connect(gui.SIG_REGISTRATION_FAILED, self._login_error_cb)
             gui_app.connect(gui.SIG_USER_CANCELATION,
                             self._login_auth_denied_cb)
-        except Exception, e:
+        except Exception:
             msg = '_show_login_or_register_ui failed when calling ' \
                   'gui.UbuntuSSOClientGUI(%r, %r, %r, %r, %r)'
             logger.exception(msg, app_name, tc_url, help_text,
                              win_id, login_only)
-            self.CredentialsError(str(e))
+            msg = "Problem opening the Ubuntu SSO user interface."
+            self.CredentialsError(app_name, msg, traceback.format_exc())
 
     @dbus.service.method(dbus_interface=DBUS_IFACE_CRED_NAME,
                          in_signature="sssx", out_signature="")
@@ -512,10 +523,11 @@
                                  app_name, terms_and_conditions_url,
                                  help_text, window_id)
             else:
-                self.CredentialsFound(token)
-        except Exception, e:
-            logger.exception("problem while getting the credentials")
-            self.CredentialsError(str(e))
+                self.CredentialsFound(app_name, token)
+        except Exception:
+            msg = "Problem getting the credentials from the keyring."
+            logger.exception(msg)
+            self.CredentialsError(app_name, msg, traceback.format_exc())
 
     def _show_login_only_ui(self, app_name, help_text, win_id):
         """Shows the UI so the user can login."""
@@ -541,10 +553,11 @@
                 gobject.idle_add(self._show_login_only_ui, app_name,
                                  help_text, window_id)
             else:
-                self.CredentialsFound(token)
-        except Exception, e:
-            logger.exception("problem while getting the credentials")
-            self.CredentialsError(str(e))
+                self.CredentialsFound(app_name, token)
+        except Exception:
+            msg = "Problem getting the credentials from the keyring."
+            logger.exception(msg)
+            self.CredentialsError(app_name, msg, traceback.format_exc())
 
 
 class LoginProcessor:

=== modified file 'ubuntu_sso/tests/test_gui.py'
--- ubuntu_sso/tests/test_gui.py	2010-08-19 20:56:52 +0000
+++ ubuntu_sso/tests/test_gui.py	2010-08-23 19:43:39 +0000
@@ -1347,15 +1347,15 @@
             self.ui.connect(sig, self._set_called, sig)
             widget = getattr(self.ui, name)
             widget.clicked()
-            self.assertEqual(self._called[sig],
-                             (self.ui.window, (), {}), msg % name)
+            expected_args = (self.ui.window, (APP_NAME,), {})
+            self.assertEqual(self._called[sig], expected_args, msg % name)
             self._called = {}
 
     def test_on_user_registration_error_proper_signal_is_emitted(self):
         """On UserRegistrationError, 'registration-failed' signal is sent."""
         self.ui.on_user_registration_error()
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (), {}),
+        self.assertEqual((self.ui.window, (APP_NAME,), {}),
                          self._called[gui.SIG_REGISTRATION_FAILED])
 
     def test_on_email_validated_proper_signals_is_emitted(self):
@@ -1369,7 +1369,7 @@
         """On EmailValidationError, 'registration-failed' signal is sent."""
         self.ui.on_email_validation_error()
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (), {}),
+        self.assertEqual((self.ui.window, (APP_NAME,), {}),
                           self._called[gui.SIG_REGISTRATION_FAILED])
 
     def test_on_logged_in_proper_signals_is_emitted(self):
@@ -1384,7 +1384,7 @@
         self.click_connect_with_valid_data()
         self.ui.on_login_error()
         self.ui.on_close_clicked()
-        self.assertEqual((self.ui.window, (), {}),
+        self.assertEqual((self.ui.window, (APP_NAME,), {}),
                           self._called[gui.SIG_LOGIN_FAILED])
 
     def test_registration_successfull_even_if_prior_registration_error(self):

=== modified file 'ubuntu_sso/tests/test_main.py'
--- ubuntu_sso/tests/test_main.py	2010-08-20 17:33:33 +0000
+++ ubuntu_sso/tests/test_main.py	2010-08-23 19:43:39 +0000
@@ -39,7 +39,7 @@
     InvalidEmailError, InvalidPasswordError, SSOLogin, SSOCredentials,
     get_token_name, keyring_get_credentials, keyring_store_credentials, logger,
     LoginProcessor, NewPasswordError, SSOLoginProcessor, RegistrationError,
-    ResetPasswordTokenError, OLD_KEY_NAME, U1_APP_NAME)
+    ResetPasswordTokenError, OLD_KEY_NAME, U1_APP_NAME, except_to_errdict)
 
 
 APP_NAME = 'The Coolest App Ever'
@@ -411,7 +411,7 @@
     def fake_err_blocking(self, f, cb, eb):
         """A fake blocking function that fails."""
         f()
-        eb(BlockingSampleException())
+        eb(except_to_errdict(BlockingSampleException()))
 
     def test_creation(self):
         """Test that the object creation is successful."""
@@ -457,8 +457,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -499,8 +499,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -539,8 +539,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -581,8 +581,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -620,8 +620,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -662,8 +662,8 @@
         self.patch(ubuntu_sso.main, "blocking", self.fake_err_blocking)
         self.mocker.replay()
 
-        def verify(error):
-            self.assertIsInstance(error, BlockingSampleException)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
             d.callback("Ok")
 
         client = SSOLogin(self.mockbusname,
@@ -676,6 +676,7 @@
 
 class BlockingFunctionTestCase(TestCase):
     """Tests for the "blocking" function."""
+    timeout = 5
 
     def test_blocking(self):
         """Test the normal behaviour."""
@@ -700,8 +701,9 @@
         def f():
             raise BlockingSampleException(expected_error_message)
 
-        def verify(error):
-            self.assertEqual(error, expected_error_message)
+        def verify(errdict):
+            self.assertEqual(errdict["errtype"], "BlockingSampleException")
+            self.assertEqual(errdict["message"], expected_error_message)
             d.callback("Ok")
 
         blocking(f, d.errback, verify)
@@ -778,6 +780,8 @@
         self.assertEqual(new_creds["token"], sample_oauth_token)
         self.assertIn("token_secret", new_creds)
         self.assertEqual(new_creds["token_secret"], sample_oauth_secret)
+        self.assertIn("token_name", new_creds)
+        self.assertEqual(new_creds["token_name"], OLD_KEY_NAME)
 
     def test_keyring_get_old_cred_not_found(self):
         """The method returns None if no old nor new credentials found."""
@@ -838,8 +842,8 @@
         expected_creds = TOKEN
         d = Deferred()
 
-        def verify(result):
-            self.assertEqual(result, expected_creds)
+        def verify(app_name, credentials):
+            self.assertEqual(credentials, expected_creds)
             d.callback("ok")
 
         kgt = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
@@ -878,8 +882,8 @@
         expected_error = "Sample Error - not for resale"
         d = Deferred()
 
-        def verify(result, *a):
-            self.assertEqual(result, expected_error)
+        def verify(app_name, error_message, detailed_error):
+            self.assertEqual(app_name, APP_NAME)
             d.callback("ok")
 
         kgt = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
@@ -899,8 +903,8 @@
         expected_creds = TOKEN
         d = Deferred()
 
-        def verify(result):
-            self.assertEqual(result, expected_creds)
+        def verify(app_name, credentials):
+            self.assertEqual(credentials, expected_creds)
             d.callback("ok")
 
         kgt = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
@@ -939,8 +943,8 @@
         expected_error = "Sample Error - not for resale"
         d = Deferred()
 
-        def verify(result, *a):
-            self.assertEqual(result, expected_error)
+        def verify(app_name, error_message, detailed_error):
+            self.assertEqual(app_name, APP_NAME)
             d.callback("ok")
 
         kgt = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
@@ -955,6 +959,91 @@
         client.login_to_get_credentials(*LOGIN_ONLY_ARGS)
         return d
 
+    def test_login_error_cb(self):
+        """The login error callback should throw the signal."""
+        d = Deferred()
+
+        def verify(app_name, error_message, detailed_error):
+            self.assertEqual(app_name, APP_NAME)
+            d.callback("ok")
+
+        self.mocker.replay()
+        client = SSOCredentials(self.mocker.mock())
+        self.patch(client, "CredentialsError", verify)
+
+        client._login_error_cb(None, APP_NAME)
+        return d
+
+    def test_show_login_or_register_ui_error(self):
+        """An error happens when trying to register."""
+        d = Deferred()
+        mockgui = self.mocker.replace("ubuntu_sso.gui.UbuntuSSOClientGUI")
+        mockgui(ARGS)
+        self.mocker.throw(Exception())
+        self.mocker.replay()
+
+        def verify(app_name, msg, full_error):
+            self.assertEqual(app_name, APP_NAME)
+            d.callback("ok")
+
+        client = SSOCredentials(self.mocker.mock())
+        self.patch(client, "CredentialsError", verify)
+
+        client._show_login_or_register_ui(APP_NAME, "http:tc_url", "help", 0)
+        return d
+
+    def test_login_success_cb_works(self):
+        """Check that the right signal is sent."""
+        expected_creds = TOKEN
+        d = Deferred()
+        kgc = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
+        kgc(ARGS)
+        self.mocker.result(expected_creds)
+        self.mocker.replay()
+
+        def verify(app_name, credentials):
+            self.assertEqual(credentials, expected_creds)
+            d.callback("ok")
+
+        client = SSOCredentials(self.mocker.mock())
+        self.patch(client, "CredentialsFound", verify)
+
+        client._login_success_cb(None, APP_NAME)
+        return d
+
+    def test_login_success_cb_error(self):
+        """An error happens when accessing the keyring."""
+        d = Deferred()
+        kgc = self.mocker.replace("ubuntu_sso.main.keyring_get_credentials")
+        kgc(ARGS)
+        self.mocker.throw(Exception())
+        self.mocker.replay()
+
+        def verify(app_name, msg, full_error):
+            self.assertEqual(app_name, APP_NAME)
+            d.callback("ok")
+
+        client = SSOCredentials(self.mocker.mock())
+        self.patch(client, "CredentialsError", verify)
+
+        client._login_success_cb(None, APP_NAME)
+        return d
+
+    def test_auth_denied_cb(self):
+        """When the user decides not to allow the registration or login."""
+        d = Deferred()
+        self.mocker.replay()
+
+        def verify(app_name):
+            self.assertEqual(app_name, APP_NAME)
+            d.callback("ok")
+
+        client = SSOCredentials(self.mocker.mock())
+        self.patch(client, "AuthorizationDenied", verify)
+
+        client._login_auth_denied_cb(None, APP_NAME)
+        return d
+
 
 class CredentialsGUITestCase(TestCase):
     """login_or_register_to_get_credentials opens the proper GUI."""