Merge lp:~nataliabidart/ubuntuone-client/not-autoconnect-if-no-tokens into lp:ubuntuone-client

[Natalia Bidart]

Syncdaemon should get credentials using find_credentials, not calling register which may raise a GUI our of nowhere (LP: #701588).

[dobey (dobey)]

So does this stop autoconnecting when there are no tokens, or does it just change it so that if you don't have a token, you won't be able to register when it connects. This seems like the wrong fix to the bug to me.

review: Needs Information

[Natalia Bidart (nataliabidart)]

This will make syncdaemon to continue autoconnecting if the token is already in the keyring.
If not, it will do nothing, since the point of entry to the service should be some other (usually) UI app.

[dobey (dobey)]

That's what I thought. It doesn't solve the bug, but works around it. I think the correct fix wold be to make ubuntuone-launch not start ubuntuone-syncdaemon if there is no token yet (it used to do that), and perhaps to make syncdaemon not try to connect automatically if there is no token, but if a connect() is explicitly requested, with no token, I think the register() call is still the right one to make. No?

[Roberto Alsina (ralsina)]

+1 as discussed on IRC

review: Approve

[Natalia Bidart (nataliabidart)]

The bug to track down why syncdaemon is started if the user doesn't have a U1 account is located at:

bug #702902

[Lucio Torre (lucio.torre)]

+1

review: Approve

[Natalia Bidart (nataliabidart)]

The attempt to merge lp:~nataliabidart/ubuntuone-client/not-autoconnect-if-no-tokens into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
(B testing autoconf2.50... not found.
  testing autoconf... found 2.67
checking for automake >= 1.10...
(B testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
(B testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
(B testing intltoolize... found 0.41.1
checking for pkg-config >= 0.14.0...
(B testing pkg-config... found 0.25
checking for gtk-doc >= 1.0...
(B testing gtkdocize... found 1.15
Checking for required M4 macros...
(BChecking for forbidden M4 macros...
(BProcessing ./configure.ac
(BRunning libtoolize...
(Blibtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
(BRunning gtkdocize...
(BRunning aclocal-1.11...
(BRunning autoconf...
(BRunning autoheader...
(BRunning automake-1.11...
(BRunning ./configure --enable-gtk-doc --enable-debug --with-protocol=~/canonical/ubuntuone-storage-protocol/trunk/ ...
(Bchecking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864...

[lp:~nataliabidart/ubuntuone-client/not-autoconnect-if-no-tokens]

806. By Natalia Bidart on 2011-01-14

Merged trunk in.

807. By Natalia Bidart on 2011-01-14

Making tests pass.

=== modified file 'contrib/testing/testcase.py'
--- contrib/testing/testcase.py	2010-12-16 14:35:23 +0000
+++ contrib/testing/testcase.py	2011-01-14 20:00:42 +0000
@@ -96,7 +96,7 @@
     def shutdown(self, with_restart=False):
         """...that only knows how to go away"""
 
-    def _request_token(self):
+    def _request_token(self, *args, **kwargs):
         """Return a token which is a fixed set of credentials."""
         return FAKED_CREDENTIALS
 

=== modified file 'tests/platform/linux/test_dbus.py'
--- tests/platform/linux/test_dbus.py	2011-01-07 17:24:11 +0000
+++ tests/platform/linux/test_dbus.py	2011-01-14 20:00:42 +0000
@@ -34,7 +34,6 @@
     BaseTwistedTestCase, FakeMain, FAKED_CREDENTIALS,
     MementoHandler)
 
-from ubuntuone.clientdefs import APP_NAME
 from ubuntuone.storageprotocol.sharersp import NotifyShareHolder
 from ubuntuone.syncdaemon import states, config
 from ubuntuone.platform.linux.dbus_interface import (
@@ -97,13 +96,14 @@
         """
         self.next_login_cb = (callback, args)
 
+
 class FakeDBusInterface(object):
     """A fake DBusInterface..."""
 
     def shutdown(self, with_restart=False):
         """...that only knows how to go away"""
 
-    def _request_token(self):
+    def _request_token(self, *args, **kwargs):
         """Return a token which is a fixed set of credentials."""
         return FAKED_CREDENTIALS
 
@@ -136,7 +136,7 @@
         self.dbus_iface = DBusInterface(self.bus, self.main,
                                         system_bus=self.bus)
         # token is a fixed set of credentials
-        self.dbus_iface._request_token = lambda: FAKED_CREDENTIALS
+        self.dbus_iface._request_token = lambda *a, **kw: FAKED_CREDENTIALS
 
         self.main.external = ExternalInterface(self.main, False, False,
                                                dbus_iface=self.dbus_iface)
@@ -317,6 +317,10 @@
         """Fake register."""
         self._called['register'] = (args, kwargs)
 
+    def find_credentials(self, *args, **kwargs):
+        """Fake find_credentials."""
+        self._called['find_credentials'] = (args, kwargs)
+
 
 class FakedBusName(object):
     """Fake a dbus name."""
@@ -2493,9 +2497,15 @@
 
 
 class DBusOAuthTestCase(BaseTwistedTestCase):
-    """Tests the interaction between dbus_interface and ubuntu-sso-client."""
+    """Tests the interaction between dbus_interface and credentials.
+
+    Check conditions when autconnecting is False.
+
+    """
 
     timeout = 2
+    method = 'register'
+    autoconnecting = False
 
     def setUp(self):
         """Init."""
@@ -2539,8 +2549,8 @@
             self.assertEqual(self.dbus_iface._signal_handler, cb)
             self.dbus_iface._deferred.callback(None)
 
-        self.patch(FakedSSOBackend, 'register', f)
-        yield self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
 
     @defer.inlineCallbacks
     def test_signals_are_removed_after_connection(self):
@@ -2550,8 +2560,8 @@
             """Just succeed."""
             self.dbus_iface._deferred.callback(None)
 
-        self.patch(FakedSSOBackend, 'register', f)
-        yield self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         self.assertNotIn((DBUS_CREDENTIALS_IFACE, None), self.bus.callbacks)
 
     @defer.inlineCallbacks
@@ -2560,11 +2570,11 @@
 
         def f(*a, **kw):
             """Receive credentials."""
-            self.dbus_iface._signal_handler(APP_NAME, FAKED_CREDENTIALS,
+            self.dbus_iface._signal_handler(FAKED_CREDENTIALS,
                                             member='CredentialsFound')
 
-        self.patch(FakedSSOBackend, 'register', f)
-        yield self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         self.assertEqual(self.events, [('SYS_USER_CONNECT',
                                        {'access_token': FAKED_CREDENTIALS})])
 
@@ -2574,12 +2584,12 @@
 
         def f(*a, **kw):
             """Receive error signal."""
-            self.dbus_iface._signal_handler(APP_NAME, 'Error description',
-                                            'Detailed error',
+            self.dbus_iface._signal_handler({'error_type': 'Error description',
+                                             'error_detail': 'Detailed error'},
                                             member='CredentialsError')
 
-        self.patch(FakedSSOBackend, 'register', f)
-        d = self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        d = self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         d.addErrback(lambda failure: self.assertEqual(NoAccessToken,
                                                       failure.type))
         yield d
@@ -2590,11 +2600,24 @@
 
         def f(*a, **kw):
             """Receive error signal."""
-            self.dbus_iface._signal_handler(APP_NAME,
-                                            member='AuthorizationDenied')
-
-        self.patch(FakedSSOBackend, 'register', f)
-        d = self.dbus_iface.connect()
+            self.dbus_iface._signal_handler(member='AuthorizationDenied')
+
+        self.patch(FakedSSOBackend, self.method, f)
+        d = self.dbus_iface.connect(autoconnecting=self.autoconnecting)
+        d.addErrback(lambda failure: self.assertEqual(NoAccessToken,
+                                                      failure.type))
+        yield d
+
+    @defer.inlineCallbacks
+    def test_connect_raises_NoAccessToken_if_no_creds(self):
+        """If no credentials, NoAccessToken if no credentials."""
+
+        def f(*a, **kw):
+            """Receive error signal."""
+            self.dbus_iface._signal_handler(member='CredentialsNotFound')
+
+        self.patch(FakedSSOBackend, self.method, f)
+        d = self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         d.addErrback(lambda failure: self.assertEqual(NoAccessToken,
                                                       failure.type))
         yield d
@@ -2608,8 +2631,8 @@
             """Just fail."""
             raise expected
 
-        self.patch(FakedSSOBackend, 'register', f)
-        d = self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        d = self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         d.addErrback(lambda failure: self.assertEqual(expected, failure.value))
         yield d
 
@@ -2622,14 +2645,15 @@
             """Just fail."""
             raise expected
 
-        self.patch(FakedSSOBackend, 'register', f)
-        d = self.dbus_iface.connect()
+        self.patch(FakedSSOBackend, self.method, f)
+        d = self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         d.addErrback(lambda failure: self.assertEqual(expected, failure.value))
         yield d
         self.assertTrue(len(self.memento.records) > 0)
-        msg = self.memento.records[0].message
+        record = self.memento.records[1]
+        msg = record.message
         self.assertIn('connect failed while getting the token', msg)
-        self.assertIn(expected, self.memento.records[0].exc_info)
+        self.assertIn(expected, record.exc_info)
 
     def test_oauth_credentials_are_none_at_startup(self):
         """If the oauth_credentials are not passed as param, they are None."""
@@ -2644,7 +2668,7 @@
                     'token_secret': 'faked_token_secret'}
         self.dbus_iface.oauth_credentials = (expected['token'],
                                              expected['token_secret'])
-        yield self.dbus_iface.connect()
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         self.assertEqual(self.events, [('SYS_USER_CONNECT',
                                        {'access_token': expected})])
 
@@ -2659,7 +2683,7 @@
                                              expected['consumer_secret'],
                                              expected['token'],
                                              expected['token_secret'])
-        yield self.dbus_iface.connect()
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         self.assertEqual(self.events, [('SYS_USER_CONNECT',
                                        {'access_token': expected})])
 
@@ -2669,67 +2693,31 @@
         self.dbus_iface.oauth_credentials = ('consumer_key',
                                              'consumer_secret',
                                              'token_secret')
-        yield self.dbus_iface.connect()
+        yield self.dbus_iface.connect(autoconnecting=self.autoconnecting)
         self.assertEqual(self.events, [])
         msgs = (str(self.dbus_iface.oauth_credentials), 'useless')
         self.assertTrue(self.memento.check_warning(*msgs))
 
-    def test_only_log_if_app_name_is_not_correct_when_credentials_found(self):
-        """If the app_name is not ours, just log an INFO message."""
-
-        def f(*a, **kw):
-            """Receive wrong app_name."""
-            self.dbus_iface._signal_handler(APP_NAME * 2,
-                                            member='CredentialsFound')
-
-        self.patch(FakedSSOBackend, 'register', f)
-        self.dbus_iface._request_token()
-
-        self.assertFalse(self.dbus_iface._deferred.called)
-        self.assertTrue(self.memento.check_info('CredentialsFound',
-                                                APP_NAME * 2))
-
-    def test_only_log_if_app_name_is_not_correct_when_credentials_error(self):
-        """If the app_name is not ours, just log an INFO message."""
-
-        def f(*a, **kw):
-            """Receive a wrong app_name."""
-            self.dbus_iface._signal_handler(APP_NAME * 2,
-                                            member='CredentialsError')
-
-        self.patch(FakedSSOBackend, 'register', f)
-        self.dbus_iface._request_token()
-
-        self.assertFalse(self.dbus_iface._deferred.called)
-        self.assertTrue(self.memento.check_info('CredentialsError',
-                                                APP_NAME * 2))
-
-    def test_only_log_if_app_name_is_not_correct_when_authorization_denied(self):
-        """If the app_name is not ours, just log an INFO message."""
-
-        def f(*a, **kw):
-            """Receive a wrong app_name."""
-            self.dbus_iface._signal_handler(APP_NAME * 2,
-                                            member='AuthorizationDenied')
-
-        self.patch(FakedSSOBackend, 'register', f)
-        self.dbus_iface._request_token()
-
-        self.assertFalse(self.dbus_iface._deferred.called)
-        self.assertTrue(self.memento.check_info('AuthorizationDenied',
-                                                APP_NAME * 2))
-
     def test_signal_handler_remains_generic(self):
         """The signal handler function should be generic."""
         self.dbus_iface._signal_handler()
         # no failure
-        self.assertTrue(self.memento.check_debug('member: None',
-                                                 'app_name: None'))
+        self.assertTrue(self.memento.check_debug('member: None'))
 
         self.dbus_iface._signal_handler(no_member_kwarg='Test')
         # no failure
-        self.assertTrue(self.memento.check_debug('member: None',
-                                                 'app_name: None'))
+        self.assertTrue(self.memento.check_debug('member: None'))
+
+
+class DBusOAuthTestCaseRegister(DBusOAuthTestCase):
+    """Tests the interaction between dbus_interface and credentials.
+
+    Check conditions when autconnecting is True.
+
+    """
+
+    method = 'find_credentials'
+    autoconnecting = True
 
 
 class FolderTests(DBusTwistedTestCase):

=== modified file 'tests/syncdaemon/test_main.py'
--- tests/syncdaemon/test_main.py	2010-12-17 18:19:41 +0000
+++ tests/syncdaemon/test_main.py	2011-01-14 20:00:42 +0000
@@ -101,7 +101,7 @@
                 """Pass the test when we get this event."""
                 reactor.callLater(0, d0.callback, None)
 
-        main = self.build_main(handshake_timeout=.5)
+        main = self.build_main(handshake_timeout=0)
 
         def fake_connect(*a):
             """Only connect when States told so."""
@@ -183,12 +183,13 @@
 
         self.connect_called = False
         self.patch(main_mod.ubuntuone.platform.ExternalInterface, 'connect',
-                   lambda *a: setattr(self, 'connect_called', True))
+                   lambda *a, **kw: setattr(self, 'connect_called', (a, kw)))
 
         main = self.build_main()
         self.addCleanup(lambda: main.shutdown())
 
-        self.assertTrue(self.connect_called)
+        self.assertEqual(self.connect_called, ((main.external,),
+                                               {'autoconnecting': True}))
 
     def test_dont_connect_if_autoconnect_is_disabled(self):
         """If autoconnect option is disabled, do not connect the syncdaemon."""
@@ -199,7 +200,7 @@
 
         self.connect_called = False
         self.patch(main_mod.ubuntuone.platform.ExternalInterface, 'connect',
-                   lambda *a: setattr(self, 'connect_called', True))
+                   lambda *a, **kw: setattr(self, 'connect_called', True))
 
         main = self.build_main()
         self.addCleanup(lambda: main.shutdown())

=== modified file 'ubuntuone/platform/linux/__init__.py'
--- ubuntuone/platform/linux/__init__.py	2010-12-20 12:28:01 +0000
+++ ubuntuone/platform/linux/__init__.py	2011-01-14 20:00:42 +0000
@@ -83,5 +83,5 @@
     def shutdown(self, with_restart):
         self.dbus_iface.shutdown(with_restart)
 
-    def connect(self):
-        self.dbus_iface.connect()
+    def connect(self, *args, **kwargs):
+        self.dbus_iface.connect(*args, **kwargs)

=== modified file 'ubuntuone/platform/linux/dbus_interface.py'
--- ubuntuone/platform/linux/dbus_interface.py	2011-01-06 13:54:21 +0000
+++ ubuntuone/platform/linux/dbus_interface.py	2011-01-14 20:00:42 +0000
@@ -27,10 +27,8 @@
 
 from twisted.internet import defer
 from twisted.python.failure import Failure
-from ubuntu_sso import (DBUS_BUS_NAME, DBUS_CREDENTIALS_PATH,
+from ubuntuone.credentials import (DBUS_BUS_NAME, DBUS_CREDENTIALS_PATH,
     DBUS_CREDENTIALS_IFACE)
-from ubuntu_sso.credentials import (HELP_TEXT_KEY, PING_URL_KEY, TC_URL_KEY,
-    WINDOW_ID_KEY)
 from ubuntuone.syncdaemon.interfaces import IMarker
 from ubuntuone.syncdaemon import config
 
@@ -1847,12 +1845,16 @@
             self.event_queue.push(event)
 
     @defer.inlineCallbacks
-    def connect(self):
+    def connect(self, autoconnecting=False):
         """Push the SYS_USER_CONNECT event with the token.
 
-        The token is requested via ubuntu-sso-client.
+        The token is requested via com.ubuntuone.credentials service. If
+        'autoconnecting' is True, no UI window will be raised to promt the user
+        for login/registration, only already existent credentials will be used.
 
         """
+        logger.info('connect was requested. Are we autoconnecting? %r.',
+                    autoconnecting)
         if self.oauth_credentials is not None:
             logger.debug('connect: oauth credentials were given by parameter.')
             ckey = csecret = key = secret = None
@@ -1868,7 +1870,7 @@
             token = {'consumer_key': ckey, 'consumer_secret': csecret,
                      'token': key, 'token_secret': secret}
         else:
-            token = yield self._request_token()
+            token = yield self._request_token(autoconnecting=autoconnecting)
 
         logger.info('connect: credential request was successful, '
                     'pushing SYS_USER_CONNECT.')
@@ -1877,17 +1879,11 @@
     def _signal_handler(self, *args, **kwargs):
         """Generic signal handler."""
         member = kwargs.get('member', None)
-        app_name = args[0] if len(args) > 0 else None
         d = self._deferred
-        logger.debug('Handling DBus signal for member: %r, app_name: %r.',
-                     member, app_name)
-
-        if app_name != APP_NAME:
-            logger.info('Received %s but app_name %s does not match %s, ' \
-                        'exiting.', member, app_name, APP_NAME)
-            return
-
-        if member in ('CredentialsError', 'AuthorizationDenied'):
+        logger.debug('Handling DBus signal for member: %r.', member)
+
+        if member in ('CredentialsError', 'AuthorizationDenied',
+                      'CredentialsNotFound'):
             logger.warning('%r: %r %r', member, args, kwargs)
             if not args:
                 d.errback(Failure(NoAccessToken(member)))
@@ -1895,14 +1891,14 @@
                 d.errback(Failure(NoAccessToken("%s: %s %s" %
                                                 (member, args, kwargs))))
         elif member == 'CredentialsFound' and not d.called:
-            credentials = args[1]
+            credentials = args[0]
             logger.info('%r: callbacking with credentials.', member)
             d.callback(credentials)
         else:
             logger.debug('_signal_handler: member %r not used or deferred '
-                         'already called? %r.', member, d.called)
+                         'already called? %r.', member, d)
 
-    def _request_token(self):
+    def _request_token(self, autoconnecting):
         """Request to SSO auth service to fetch the token."""
         self._deferred = d = defer.Deferred()
 
@@ -1921,12 +1917,13 @@
             client = self.bus.get_object(DBUS_BUS_NAME, DBUS_CREDENTIALS_PATH,
                                          follow_name_owner_changes=True)
             iface = dbus.Interface(client, DBUS_CREDENTIALS_IFACE)
-            params = {HELP_TEXT_KEY: DESCRIPTION, TC_URL_KEY: TC_URL,
-                      PING_URL_KEY: PING_URL, WINDOW_ID_KEY: '0'}
-            iface.register(APP_NAME, params,
-                           # ignore the reply, we get the result via signals
-                           reply_handler=lambda: None,
-                           error_handler=error_handler)
+            # ignore the reply, we get the result via signals
+            if autoconnecting:
+                iface.find_credentials(reply_handler=lambda: None,
+                                       error_handler=error_handler)
+            else:
+                iface.register(reply_handler=lambda: None,
+                               error_handler=error_handler)
         except DBusException, e:
             error_handler(e)
         except:

=== modified file 'ubuntuone/syncdaemon/main.py'
--- ubuntuone/syncdaemon/main.py	2011-01-07 18:51:45 +0000
+++ ubuntuone/syncdaemon/main.py	2011-01-14 20:00:42 +0000
@@ -74,7 +74,8 @@
     def __init__(self, root_dir, shares_dir, data_dir, partials_dir,
                  host='fs-1.one.ubuntu.com', port=443, dns_srv=None, ssl=True,
                  disable_ssl_verify=False, glib_loop=False,
-                 mark_interval=120, broadcast_events=False, handshake_timeout=30,
+                 mark_interval=120, broadcast_events=False,
+                 handshake_timeout=30,
                  shares_symlink_name='Shared With Me',
                  read_limit=None, write_limit=None, throttling_enabled=False,
                  ignore_files=None, oauth_credentials=None):
@@ -125,7 +126,7 @@
             self, glib_loop, broadcast_events)
         self.external.oauth_credentials = oauth_credentials
         if user_config.get_autoconnect():
-            self.external.connect()
+            self.external.connect(autoconnecting=True)
 
         self.action_q.content_queue.set_change_notification_cb(
             self.external.change_notification)