Merge lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs into lp:ubuntuone-client

[Rodrigo Moya]

Make ubuntuone-preferences use ubuntu-sso

Make ubuntuone-preferences use ubuntu-sso

[Natalia Bidart (nataliabidart)]

For the constants use those in ubuntuone/clientdefs.py.

Please fix the docstrings "Helper that handles the REST response." (and the similar ones).

This needs to use APP_NAME instead of 'ubuntuone': iface.clear_token('ubuntuone'.
This also: + if app_name == "Ubuntu One":

Remove + print "Got credentials"
Remove _() for log messages.

review: Needs Fixing

[lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs]

648. By Rodrigo Moya on 2010-08-24

Some fixes

[dobey (dobey)]

Your change introduces a regression where multiple dialogs can be opened in the same process. There should only ever be one single dialog for this process.

review: Needs Fixing

[lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs]

649. By Rodrigo Moya on 2010-08-24

Create the dialog only once

[dobey (dobey)]

It looks like test_login_check in the tests is skipped at the moment, but it should be updated to comply with the changes here too. I'd also avoid using globals for the oauth_token and oauth_consumer variables, and store them as attributes in the dialog (and pass them on to the other classes and methods that might need them).

I'm still confused about the changes for the dialog creation though. Why did you change it to not create the dialog when starting up? Shouldn't the dialog be created and shown, and its window id passed over to ubuntu_sso for setting as the parent?

The dialog creation was how it was, in order to show the dialog to the user as soon as possible, so that they know things are happening. Is there a good reason to not do that now?

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

The docstrings were not fixed (see lines 92-94 and 104-106)

review: Needs Fixing

[Rodrigo Moya (rodrigo-moya)]

> The docstrings were not fixed (see lines 92-94 and 104-106)

I think they're fixed now, I guess you wanted them on just 1 line?

[lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs]

650. By Rodrigo Moya on 2010-08-24

Fixed docstrings

[Rodrigo Moya (rodrigo-moya)]

> It looks like test_login_check in the tests is skipped at the moment, but it
> should be updated to comply with the changes here too. I'd also avoid using
> globals for the oauth_token and oauth_consumer variables, and store them as
> attributes in the dialog (and pass them on to the other classes and methods
> that might need them).
>
test_login_check still depends on the real DBus service to be run, not sure how we can make it work without depending on the tests in ubuntu-sso-client. If there's a way, I'm ok with doing it, but not in this branch, which is needed for the new release to be done today or tomorrow at the latest.

> I'm still confused about the changes for the dialog creation though. Why did
> you change it to not create the dialog when starting up? Shouldn't the dialog
> be created and shown, and its window id passed over to ubuntu_sso for setting
> as the parent?
>
well, what was the point of having a dialog with all the widgets showing temporary data ('Unknown', etc)? So, now, we create the dialog associated to the LoginHandler, and show it when we get the credentials or when calling _present

> The dialog creation was how it was, in order to show the dialog to the user as
> soon as possible, so that they know things are happening. Is there a good
> reason to not do that now?
>
if the credentials are in the keyring, the dialog will show up inmediately, if not, they will see the SSO login/register screen

[Natalia Bidart (nataliabidart)]

Looks great now!

review: Approve

[lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs]

651. By Rodrigo Moya on 2010-08-24

Move instantiation of dialog back to its original place

[dobey (dobey)]

review: Approve

=== modified file 'bin/ubuntuone-preferences'
--- bin/ubuntuone-preferences	2010-06-16 18:44:02 +0000
+++ bin/ubuntuone-preferences	2010-08-24 21:19:41 +0000
@@ -3,9 +3,10 @@
 
 # ubuntuone-client-applet - Tray icon applet for managing Ubuntu One
 #
-# Author: Rodney Dawes <[e-mail]>
+# Authors: Rodney Dawes <[e-mail]>
+#          Rodrigo Moya <[e-mail]>
 #
-# Copyright 2009 Canonical Ltd.
+# Copyright 2009-2010 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -29,12 +30,11 @@
 import sys
 import time
 import gettext
-import gnomekeyring
 import subprocess
 from threading import Thread
 from oauth import oauth
 
-from ubuntu_sso import DBUS_IFACE_AUTH_NAME, DBUS_PATH_AUTH
+from ubuntu_sso import DBUS_IFACE_AUTH_NAME, DBUS_IFACE_CRED_NAME, DBUS_CRED_PATH
 from ubuntuone import clientdefs
 from ubuntuone.syncdaemon.tools import SyncDaemonTool
 from ubuntuone.api.restclient import RestClient
@@ -62,7 +62,7 @@
 try:
     from desktopcouch.replication_services import ubuntuone as dcouch
 except ImportError:
-    logger.error(_("DesktopCouch replication API not found"))
+    logger.error("DesktopCouch replication API not found")
 
 DBusGMainLoop(set_as_default=True)
 
@@ -92,7 +92,8 @@
 BW_CHCK_ARGS = ['dpkg', '-l', BW_PKG_NAME]
 
 # This is a global so we can avoid creating multiple instances in some cases
-prefs_dialog = None
+oauth_token = None
+oauth_consumer = None
 
 def dbus_async(*args, **kwargs):
     """Simple handler to make dbus do stuff async."""
@@ -103,48 +104,31 @@
     """Make a login request to the login handling daemon."""
     try:
         client = bus.get_object(DBUS_IFACE_AUTH_NAME,
-                                DBUS_PATH_AUTH,
+                                DBUS_CRED_PATH,
                                 follow_name_owner_changes=True)
-        iface = dbus.Interface(client, DBUS_IFACE_AUTH_NAME)
-        iface.login('https://ubuntuone.com', 'ubuntuone',
-                    reply_handler=dbus_async,
-                    error_handler=error_handler)
+        iface = dbus.Interface(client, DBUS_IFACE_CRED_NAME)
+        iface.login_or_register_to_get_credentials(
+            clientdefs.APP_NAME,
+            clientdefs.TC_URL,
+            clientdefs.DESCRIPTION,
+            0,
+            reply_handler=dbus_async,
+            error_handler=error_handler)
     except DBusException, e:
         error_handler(e)
 
-def get_access_token(keyring):
-    """Get the access token from the keyring."""
-    items = []
-    try:
-        items = keyring.find_items_sync(
-            keyring.ITEM_GENERIC_SECRET,
-            {'ubuntuone-realm': "https://ubuntuone.com",
-             'oauth-consumer-key': 'ubuntuone'})
-        secret = items[0].secret
-        return oauth.OAuthToken.from_string(secret)
-    except (gnomekeyring.NoMatchError, gnomekeyring.DeniedError):
-        return None
-
-def do_rest_request(rest_client, url, method, token, callback):
-    """
-    Helper that handles the REST response.
-    """
-    consumer = oauth.OAuthConsumer('ubuntuone', 'hammertime')
-    result = rest_client.call(url, method, consumer, token)
+def do_rest_request(rest_client, url, method, callback):
+    """Helper that handles the REST request.""" 
+    result = rest_client.call(url, method, oauth_consumer, oauth_token)
 
     gtk.gdk.threads_enter()
     callback(result)
     gtk.gdk.threads_leave()
 
-def make_rest_request(url=None, method='GET',
-                      callback=None, keyring=None):
-    """
-    Helper that makes an oauth-wrapped REST request.
-    """
-    token = get_access_token(keyring)
-
+def make_rest_request(url=None, method='GET', callback=None):
+    """Helper that makes an oauth-wrapped REST request."""
     rest_client = RestClient(url)
-    Thread(target=do_rest_request, args=(rest_client, url, method, token, callback)).start()
+    Thread(target=do_rest_request, args=(rest_client, url, method, callback)).start()
 
 class DevicesWidget(gtk.Table):
     """
@@ -152,20 +136,14 @@
     """
     def __init__(self,
                  bus,
-                 keyring=gnomekeyring,
-                 realm='https://ubuntuone.com',
-                 consumer_key='ubuntuone',
                  url='https://one.ubuntu.com/api/1.0/devices/'):
         super(DevicesWidget, self).__init__(rows=2, columns=3)
         self.bus = bus
-        self.keyring = keyring
         self.sdtool = SyncDaemonTool(bus)
         self.set_border_width(6)
         self.set_row_spacings(6)
         self.set_col_spacings(6)
         self.devices = None
-        self.realm = realm
-        self.consumer_key = consumer_key
         self.base_url = url
         self.conn = None
         self.consumer = None
@@ -301,14 +279,7 @@
 
         Hook up parse_devices to run on the result (when it gets here).
         """
-        try:
-            get_access_token(self.keyring)
-        except gnomekeyring.NoMatchError:
-            self.error("No token in the keyring")
-            self.devices = None
-        else:
-            make_rest_request(url=self.base_url, keyring=self.keyring,
-                              callback=self.parse_devices)
+        make_rest_request(url=self.base_url, callback=self.parse_devices)
 
     def parse_devices(self, result):
         """
@@ -356,8 +327,6 @@
         """
         self.clear_devices_view()
 
-        token = get_access_token(self.keyring)
-
         fsync_enabled = self.sdtool.is_files_sync_enabled()
 
         if not self.devices:
@@ -365,7 +334,7 @@
                 # a stopgap device so you can at least try to connect
                 self.devices = [{'kind': 'Computer',
                                  'description': _("<LOCAL MACHINE>"),
-                                 'token': token.key if token else '',
+                                 'token': oauth_token.key if oauth_token else '',
                                  'FAKE': 'YES'}]
         else:
             self.resize(len(self.devices)+1, 3)
@@ -387,7 +356,7 @@
                 butn.connect('clicked', self.remove,
                              row['kind'], row.get('token'))
                 self.attach(butn, 2, 3, i, i+1, xoptions=0, yoptions=0)
-            if ((token and row.get('token') == token.key or 'FAKE' in row)
+            if ((oauth_token and row.get('token') == oauth_token.key or 'FAKE' in row)
                 and fsync_enabled):
                 self.bw_chk = ck_btn = gtk.CheckButton(
                     _("_Limit Bandwidth Usage"))
@@ -463,20 +432,18 @@
         """
         make_rest_request(url=('%sremove/%s/%s' % (self.base_url,
                                                    kind.lower(), token)),
-                          keyring=self.keyring,
                           callback=self.parse_devices)
-        local = get_access_token(self.keyring)
         def local_removal_cb(*args, **kwargs):
             """Try to get a new token if we remove the local one."""
             do_login_request(self.bus, self.error)
 
-        if token == local.key:
+        if token == oauth_token.key:
             try:
-                client = self.bus.get_object(DBUS_IFACE_AUTH_NAME,
-                                             DBUS_PATH_AUTH,
+                client = self.bus.get_object(DBUS_IFACE_CRED_NAME,
+                                             DBUS_CRED_PATH,
                                              follow_name_owner_changes=True)
-                iface = dbus.Interface(client, DBUS_IFACE_AUTH_NAME)
-                iface.clear_token('https://ubuntuone.com', 'ubuntuone',
+                iface = dbus.Interface(client, DBUS_IFACE_CRED_NAME)
+                iface.clear_token('ubuntuone',
                                   reply_handler=local_removal_cb,
                                   error_handler=self.error)
             except DBusException, e:
@@ -486,7 +453,7 @@
 class UbuntuOneDialog(gtk.Dialog):
     """Preferences dialog."""
 
-    def __init__(self, config=None, keyring=gnomekeyring, *args, **kw):
+    def __init__(self, config=None, *args, **kw):
         """Initializes our config dialog."""
         super(UbuntuOneDialog, self).__init__(*args, **kw)
         self.set_title(_("Ubuntu One Preferences"))
@@ -506,7 +473,6 @@
         self.ums_id = None
 
         self.__bus = dbus.SessionBus()
-        self.keyring = keyring
 
         # Timeout ID to avoid spamming DBus from spinbutton changes
         self.__update_id = 0
@@ -663,7 +629,6 @@
     def request_quota_info(self):
         """Request new quota info from server, and update display."""
         make_rest_request(url='https://one.ubuntu.com/api/quota/',
-                          keyring=self.keyring,
                           callback=self.got_quota_info)
 
     def got_account_info(self, user):
@@ -675,7 +640,6 @@
     def request_account_info(self):
         """Request account info from server, and update display."""
         make_rest_request(url='https://one.ubuntu.com/api/account/',
-                          keyring=self.keyring,
                           callback=self.got_account_info)
 
     def __bw_inst_cb(self, button):
@@ -950,7 +914,7 @@
         self.notebook.append_page(sw)
         self.notebook.set_tab_label_text(sw, _("Devices"))
         sw.show()
-        self.devices = DevicesWidget(self.__bus, self.keyring)
+        self.devices = DevicesWidget(self.__bus)
         sw.add_with_viewport(self.devices)
         self.devices.list_devices()
         self.devices.show_all()
@@ -1036,7 +1000,6 @@
     def __init__(self, dialog, *args, **kw):
         self.bus = dbus.SessionBus()
 
-        # The actual UI
         self.dialog = dialog
 
         # DBus object magic
@@ -1059,20 +1022,28 @@
         if self.dialog.get_property('visible'):
             self.dialog.present_with_time(int(time.time()))
 
-    def got_newcredentials(self, realm, consumer_key):
+    def got_newcredentials(self, app_name, credentials):
         """Show our dialog, since we can do stuff now."""
-        self.present()
-
-    def got_oautherror(self, message=None):
-        """Got an error during oauth."""
+        global oauth_consumer
+        global oauth_token
+
+        if app_name == clientdefs.APP_NAME:
+            oauth_consumer = oauth.OAuthConsumer(credentials['consumer_key'],
+                                                 credentials['consumer_secret'])
+            oauth_token = oauth.OAuthToken(credentials['token'],
+                                           credentials['token_secret'])
+            self.present()
+
+    def got_credentialserror(self, message=None):
+        """Got an error during authentication."""
         if message:
             logger.error(message)
         else:
-            logger.error(_("OAuthError with no message."))
+            logger.error("CredentialsError with no message.")
 
     def got_authdenied(self):
         """User denied access."""
-        logger.error(_("Authorization was denied."))
+        logger.error("Authorization was denied.")
 
     def got_dbus_error(self, error):
         """Got a DBusError."""
@@ -1082,16 +1053,16 @@
         """Register the dbus signal handlers."""
         self.bus.add_signal_receiver(
             handler_function=self.got_newcredentials,
-            signal_name='NewCredentials',
-            dbus_interface=DBUS_IFACE_AUTH_NAME)
+            signal_name='CredentialsFound',
+            dbus_interface=DBUS_IFACE_CRED_NAME)
         self.bus.add_signal_receiver(
-            handler_function=self.got_oautherror,
-            signal_name='OAuthError',
-            dbus_interface=DBUS_IFACE_AUTH_NAME)
+            handler_function=self.got_credentialserror,
+            signal_name='CredentialsError',
+            dbus_interface=DBUS_IFACE_CRED_NAME)
         self.bus.add_signal_receiver(
             handler_function=self.got_authdenied,
             signal_name='AuthorizationDenied',
-            dbus_interface=DBUS_IFACE_AUTH_NAME)
+            dbus_interface=DBUS_IFACE_CRED_NAME)
 
 
 if __name__ == "__main__":
@@ -1120,6 +1091,7 @@
     try:
         # The prefs dialog
         gtk.gdk.threads_enter()
+
         prefs_dialog = UbuntuOneDialog()
         prefs_dialog.show()
 

=== modified file 'tests/test_preferences.py'
--- tests/test_preferences.py	2010-08-16 14:00:13 +0000
+++ tests/test_preferences.py	2010-08-24 21:19:41 +0000
@@ -20,7 +20,6 @@
 import new
 import os
 
-import gnomekeyring
 import ubuntu_sso
 
 from contrib.mocker import MockerTestCase
@@ -71,31 +70,6 @@
         self._old_path = ubuntu_sso.DBUS_PATH_AUTH
         ubuntu_sso.DBUS_PATH_AUTH = '/oauthdesktop'
 
-        # For testing keyring queries
-        self.keyring = self.mocker.mock()
-        self.item = self.mocker.mock(gnomekeyring.Found)
-
-        self.item_id = 999
-
-        self.item.item_id
-        self.mocker.result(self.item_id)
-        self.mocker.count(0, None)
-
-        self.item.secret
-        self.mocker.result('oauth_token=access_key'
-                           '&oauth_token_secret=access_secret')
-        self.mocker.count(0, None)
-
-        self.keyring.find_items_sync(
-            None,
-            {'ubuntuone-realm': 'https://ubuntuone.com',
-             'oauth-consumer-key': 'ubuntuone'})
-        self.mocker.count(0, None)
-        self.mocker.result([self.item])
-        self.keyring.ITEM_GENERIC_SECRET
-        self.mocker.count(0, None)
-        self.mocker.result(None)
-
         self.u1prefs.make_rest_request = self.make_rest_request
         self.u1prefs.SyncDaemonTool = SyncDaemonTool
 
@@ -114,7 +88,7 @@
         yield DBusTwistedTestCase.tearDown(self)
 
     def make_rest_request(self, url=None, method='GET',
-                          callback=None, keyring=None):
+                          callback=None):
         """Override the real request call to mock some stuff."""
         if callback:
             callback(self.content)
@@ -124,7 +98,7 @@
     def test_bw_throttling(self):
         """Test that toggling bw throttling works correctly."""
         self.mocker.replay()
-        widget = self.u1prefs.DevicesWidget(None, keyring=self.keyring)
+        widget = self.u1prefs.DevicesWidget(None)
         self.assertFalse(widget.got_limits)
         widget.update_bw_settings = self.mocker.mock()
         try:
@@ -160,7 +134,7 @@
 
     def test_list_devices_fills_devices_list_with_fake_result_when_empty(self):
         self.mocker.replay()
-        widget = self.u1prefs.DevicesWidget(None, keyring=self.keyring)
+        widget = self.u1prefs.DevicesWidget(None)
         widget.update_bw_settings = self.mocker.mock()
         try:
             widget.devices = []
@@ -174,7 +148,7 @@
 
     def test_list_devices_shows_devices_list(self):
         self.mocker.replay()
-        widget = self.u1prefs.DevicesWidget(None, keyring=self.keyring)
+        widget = self.u1prefs.DevicesWidget(None)
         widget.update_bw_settings = self.mocker.mock()
         try:
             widget.devices = []
@@ -205,7 +179,7 @@
 
     def test_list_devices_shows_real_devices_list(self):
         self.mocker.replay()
-        widget = self.u1prefs.DevicesWidget(None, keyring=self.keyring)
+        widget = self.u1prefs.DevicesWidget(None)
         widget.update_bw_settings = self.mocker.mock()
         try:
             widget.devices = [{'kind': 'Computer',
@@ -242,7 +216,7 @@
     def test_quota_display(self):
         """Test that quota display works correctly."""
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         self.assertEqual(dialog.usage_graph.get_fraction(), 0.0)
         dialog.update_quota_display(1024, 2048)
@@ -254,7 +228,7 @@
         """Test that we can request the quota info properly."""
         self.content = {"total":2048, "used":1024}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         self.assertEqual(dialog.usage_graph.get_fraction(), 0.0)
         dialog.request_quota_info()
@@ -266,7 +240,7 @@
         """Test that the quota notice is not visible if usage is low."""
         self.content = {"total":2048, "used":1024}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_quota_info()
         # the label should just be the blank '\n'
@@ -283,7 +257,7 @@
         """
         self.content = {"total": 100, "used": 95}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_quota_info()
         # don't check the exact text, as it will probably
@@ -304,7 +278,7 @@
         """
         self.content = {"total": 50<<30, "used": 49<<30}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_quota_info()
         # don't check the exact text, as it will probably
@@ -326,7 +300,7 @@
         """
         self.content = {"total": 100, "used": 100}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_quota_info()
         # don't check the exact text, as it will probably
@@ -344,7 +318,7 @@
         """Test that the quota notice is visible if usage is 100%."""
         self.content = {"total": 50<<30, "used": 50<<30}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_quota_info()
         # don't check the exact text, as it will probably
@@ -361,7 +335,7 @@
         self.content = {"username": "ubuntuone", "nickname": "Ubuntu One",
                         "email": "[e-mail]"}
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.request_account_info()
         self.content = {"total":2048, "used":1024}
@@ -387,7 +361,7 @@
         self.expect(toggle_db_sync('bookmarks', True))
         self.expect(toggle_db_sync('bookmarks', False))
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.toggle_db_sync = toggle_db_sync
         dialog.bookmarks_check.set_active(True)
@@ -406,7 +380,7 @@
         self.expect(toggle_db_sync('contacts', True))
         self.expect(toggle_db_sync('contacts', False))
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.toggle_db_sync = toggle_db_sync
         dialog.abook_check.set_active(True)
@@ -422,7 +396,7 @@
     def test_toggle_files(self):
         """Test toggling the files service on/off."""
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         dialog.files_check.set_active(True)
         self.assertTrue(dialog.files_check.get_active())
@@ -436,7 +410,7 @@
     def test_toggle_files_and_music(self):
         """Test toggling the files and music services on/off."""
         self.mocker.replay()
-        dialog = self.u1prefs.UbuntuOneDialog(keyring=self.keyring)
+        dialog = self.u1prefs.UbuntuOneDialog()
         self.assertTrue(dialog is not None)
         def files_toggled(checkbutton):
             enabled = checkbutton.get_active()