Merge branch 'fd.o-35619-gtalk-muc-conflicts'

Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=35619>
Reviewed-by: Jonny Lamb <jonny.lamb@collabora.co.uk>

3 files changed, 145 insertions, 68 deletions

diff --git a/src/disco.c b/src/disco.c
index c743be6a3..ba29a0fb7 100644
--- a/src/disco.c
+++ b/src/disco.c
@@ -318,7 +318,7 @@ timeout_request (gpointer data)
 {
   GabbleDiscoRequest *request = (GabbleDiscoRequest *) data;
   GabbleDisco *disco;
-  GError *err /* doesn't need initializing */;
+  GError *err = NULL;
   g_return_val_if_fail (data != NULL, FALSE);
 
   err = g_error_new (GABBLE_DISCO_ERROR, GABBLE_DISCO_ERROR_TIMEOUT,
@@ -354,7 +354,7 @@ timeout_request (gpointer data)
 static void
 cancel_request (GabbleDiscoRequest *request)
 {
-  GError *err /* doesn't need initializing */;
+  GError *err = NULL;
 
   g_assert (request != NULL);
 
diff --git a/src/muc-channel.c b/src/muc-channel.c
index 1bf418ec3..5d7330cb9 100644
--- a/src/muc-channel.c
+++ b/src/muc-channel.c
@@ -802,19 +802,14 @@ create_room_identity (GabbleMucChannel *chan)
       GUINT_TO_POINTER (GABBLE_JID_ROOM_MEMBER), NULL);
 }
 
-static gboolean
+static void
 send_join_request (GabbleMucChannel *gmuc,
-                   const gchar *password,
-                   GError **error)
+                   const gchar *password)
 {
   GabbleMucChannelPrivate *priv = gmuc->priv;
 
   g_object_set (priv->wmuc, "password", password, NULL);
   wocky_muc_join (priv->wmuc, NULL);
-
-  /* this used to be a meaningful success/failure return, but the two-stage  *
-   * async op involved means we don't have any way of detecting failure here */
-  return TRUE;
 }
 
 static gboolean
@@ -1463,6 +1458,7 @@ _gabble_muc_channel_is_ready (GabbleMucChannel *chan)
 
 static gboolean
 handle_nick_conflict (GabbleMucChannel *chan,
+                      WockyStanza *stanza,
                       GError **tp_error)
 {
   GabbleMucChannelPrivate *priv = chan->priv;
@@ -1472,6 +1468,26 @@ handle_nick_conflict (GabbleMucChannel *chan,
       tp_base_channel_get_connection (base), TP_HANDLE_TYPE_CONTACT);
   TpHandle self_handle;
   TpIntSet *add_rp, *remove_rp;
+  const gchar *from = wocky_stanza_get_from (stanza);
+
+  /* If this is a nick conflict message with a resource in the JID, and the
+   * resource doesn't match the one we're currently trying to join as, then
+   * ignore it. This works around a bug in Google Talk's MUC server, which
+   * sends the conflict message twice. It's valid for there to be no resource
+   * in the from='' field. If Google didn't include the resource, we couldn't
+   * work around the bug; but they happen to do so, so yay.
+   * <https://bugs.freedesktop.org/show_bug.cgi?id=35619>
+   *
+   * FIXME: WockyMuc should provide a _join_async() method and do all this for
+   * us.
+   */
+  g_assert (from != NULL);
+
+  if (index (from, '/') != NULL && tp_strdiff (from, priv->self_jid->str))
+    {
+      DEBUG ("ignoring spurious conflict message for %s", from);
+      return TRUE;
+    }
 
   if (priv->nick_retry_count >= MAX_NICK_RETRIES)
     {
@@ -1502,7 +1518,8 @@ handle_nick_conflict (GabbleMucChannel *chan,
   tp_handle_unref (contact_repo, self_handle);
 
   priv->nick_retry_count++;
-  return send_join_request (chan, priv->password, tp_error);
+  send_join_request (chan, priv->password);
+  return TRUE;
 }
 
 static LmHandlerResult
@@ -1850,7 +1867,7 @@ handle_error (GObject *source,
     }
   else
     {
-      GError *tp_error /* doesn't need initializing */;
+      GError *tp_error = NULL;
 
       switch (errnum)
         {
@@ -1871,7 +1888,7 @@ handle_error (GObject *source,
             break;
 
           case WOCKY_XMPP_ERROR_CONFLICT:
-            if (handle_nick_conflict (gmuc, &tp_error))
+            if (handle_nick_conflict (gmuc, stanza, &tp_error))
               return;
             break;
 
@@ -2911,7 +2928,6 @@ gabble_muc_channel_provide_password (TpSvcChannelInterfacePassword *iface,
                                      DBusGMethodInvocation *context)
 {
   GabbleMucChannel *self = GABBLE_MUC_CHANNEL (iface);
-  GError *error = NULL;
   GabbleMucChannelPrivate *priv;
 
   g_assert (GABBLE_IS_MUC_CHANNEL (self));
@@ -2921,23 +2937,15 @@ gabble_muc_channel_provide_password (TpSvcChannelInterfacePassword *iface,
   if ((priv->password_flags & TP_CHANNEL_PASSWORD_FLAG_PROVIDE) == 0 ||
       priv->password_ctx != NULL)
     {
-      error = g_error_new (TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
-                           "password cannot be provided in the current state");
-      dbus_g_method_return_error (context, error);
-      g_error_free (error);
-
-      return;
+      GError error = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
+          "password cannot be provided in the current state" };
+      dbus_g_method_return_error (context, &error);
     }
-
-  if (!send_join_request (self, password, &error))
+  else
     {
-      dbus_g_method_return_error (context, error);
-      g_error_free (error);
-
-      return;
+      send_join_request (self, password);
+      priv->password_ctx = context;
     }
-
-  priv->password_ctx = context;
 }
 
 
@@ -3030,7 +3038,6 @@ gabble_muc_channel_add_member (GObject *obj,
       TpBaseConnection *conn = tp_base_channel_get_connection (base);
       TpIntSet *set_remove_members, *set_remote_pending;
       GArray *arr_members;
-      gboolean result;
 
       /* are we already a member or in remote pending? */
       if (tp_handle_set_is_member (mixin->members, handle) ||
@@ -3069,16 +3076,12 @@ gabble_muc_channel_add_member (GObject *obj,
       tp_intset_destroy (set_remote_pending);
 
       /* seek to enter the room */
-      result = send_join_request (self, NULL, error);
-
-      g_object_set (obj, "state",
-                    (result) ? MUC_STATE_INITIATED : MUC_STATE_ENDED,
-                    NULL);
+      send_join_request (self, NULL);
+      g_object_set (obj, "state", MUC_STATE_INITIATED, NULL);
 
       /* deny adding */
       tp_group_mixin_change_flags (obj, 0, TP_CHANNEL_GROUP_FLAG_CAN_ADD);
-
-      return result;
+      return TRUE;
     }
 
   /* check that we're indeed a member when attempting to invite others */
diff --git a/tests/twisted/muc/name-conflict.py b/tests/twisted/muc/name-conflict.py
index 21e73ee00..512c16e0b 100644
--- a/tests/twisted/muc/name-conflict.py
+++ b/tests/twisted/muc/name-conflict.py
@@ -1,3 +1,4 @@
+# vim: fileencoding=utf-8 :
 """
 Test gabble trying alternative nicknames when the nick you wanted is already in
 use in a MUC you try to join.
@@ -5,16 +6,12 @@ use in a MUC you try to join.
 
 import dbus
 
-from twisted.words.xish import domish
-
 from gabbletest import (
-    exec_test, make_muc_presence, request_muc_handle, sync_stream
+    exec_test, make_muc_presence, sync_stream, elem,
     )
-from servicetest import call_async, unwrap, sync_dbus, assertEquals
-from constants import (
-    HT_CONTACT, HT_ROOM,
-    CONN_IFACE_REQUESTS, CHANNEL_TYPE_TEXT, CHANNEL_IFACE_GROUP,
-    CHANNEL_TYPE, TARGET_HANDLE_TYPE, TARGET_HANDLE,
+from servicetest import (
+    call_async, unwrap, sync_dbus, assertEquals, assertSameSets, wrap_channel,
+    EventPattern,
     )
 import constants as cs
 import ns
@@ -23,6 +20,9 @@ def test(q, bus, conn, stream):
     test_join(q, bus, conn, stream, 'chat@conf.localhost', False)
     test_join(q, bus, conn, stream, 'chien@conf.localhost', True)
 
+    test_gtalk_weirdness(q, bus, conn, stream,
+        'private-chat-massive-uuid@groupchat.google.com')
+
 def test_join(q, bus, conn, stream, room_jid, transient_conflict):
     """
     Tells Gabble to join a MUC, but make the first nick it tries conflict with
@@ -31,34 +31,26 @@ def test_join(q, bus, conn, stream, room_jid, transient_conflict):
     user turns out not actually to be in the room (they left while we were
     retrying).
     """
-
-    self_handle = conn.GetSelfHandle()
-
-    requests = dbus.Interface(conn, CONN_IFACE_REQUESTS)
-
-    room_handle = request_muc_handle(q, conn, stream, room_jid)
     # Implementation detail: Gabble uses the first part of your jid (if you
     # don't have an alias) as your room nickname, and appends an underscore a
     # few times before giving up.
     member, member_ = [room_jid + '/' + x for x in ['test', 'test_']]
 
-    call_async(q, requests, 'CreateChannel',
-        dbus.Dictionary({ CHANNEL_TYPE: CHANNEL_TYPE_TEXT,
-                          TARGET_HANDLE_TYPE: HT_ROOM,
-                          TARGET_HANDLE: room_handle,
+    call_async(q, conn.Requests, 'CreateChannel',
+        dbus.Dictionary({ cs.CHANNEL_TYPE: cs.CHANNEL_TYPE_TEXT,
+                          cs.TARGET_HANDLE_TYPE: cs.HT_ROOM,
+                          cs.TARGET_ID: room_jid,
                         }, signature='sv'))
 
     # Gabble first tries to join as test
     q.expect('stream-presence', to=member)
 
     # MUC says no: there's already someone called test in room_jid
-    presence = domish.Element((None, 'presence'))
-    presence['from'] = member
-    presence['type'] = 'error'
-    x = presence.addElement((ns.MUC, 'x'))
-    error = presence.addElement((None, 'error'))
-    error['type'] = 'cancel'
-    error.addElement((ns.STANZA, 'conflict'))
+    presence = elem('presence', from_=member, type='error')(
+        elem(ns.MUC, 'x'),
+        elem('error', type='cancel')(
+          elem(ns.STANZA, 'conflict'),
+        ))
     stream.send(presence)
 
     # Gabble tries again as test_
@@ -83,11 +75,10 @@ def test_join(q, bus, conn, stream, room_jid, transient_conflict):
     # Only now should we have finished joining the room.
     event = q.expect('dbus-return', method='CreateChannel')
     path, props = event.value
-    text_chan = bus.get_object(conn.bus_name, path)
-    group_props = unwrap(text_chan.GetAll(CHANNEL_IFACE_GROUP,
-        dbus_interface=dbus.PROPERTIES_IFACE))
+    text_chan = wrap_channel(bus.get_object(conn.bus_name, path), 'Text')
+    group_props = unwrap(text_chan.Properties.GetAll(cs.CHANNEL_IFACE_GROUP))
 
-    t, t_ = conn.RequestHandles(HT_CONTACT, [member, member_])
+    t, t_ = conn.RequestHandles(cs.HT_CONTACT, [member, member_])
 
     # Check that Gabble think our nickname in the room is test_, not test
     muc_self_handle = group_props['SelfHandle']
@@ -101,7 +92,7 @@ def test_join(q, bus, conn, stream, room_jid, transient_conflict):
         assert members == [t_], (members, t_, t)
     else:
         # Check there are exactly two members (test and test_)
-        assert sorted(members) == sorted([t, t_]), (members, [t, t_])
+        assertSameSets([t, t_], members)
 
     # In either case, there should be no pending members.
     assert len(group_props['LocalPendingMembers']) == 0, group_props
@@ -110,7 +101,7 @@ def test_join(q, bus, conn, stream, room_jid, transient_conflict):
     # Check that test_'s handle owner is us, and that test (if it's there) has
     # no owner.
     handle_owners = group_props['HandleOwners']
-    assertEquals (self_handle,  handle_owners[t_])
+    assertEquals (conn.GetSelfHandle(), handle_owners[t_])
     if not transient_conflict:
         assertEquals (0, handle_owners[t])
 
@@ -119,8 +110,91 @@ def test_join(q, bus, conn, stream, room_jid, transient_conflict):
     text_chan.Close()
 
     event = q.expect('stream-presence', to=member_)
-    elem = event.stanza
-    assert elem['type'] == 'unavailable'
+    assertEquals('unavailable', event.stanza['type'])
+
+def test_gtalk_weirdness(q, bus, conn, stream, room_jid):
+    """
+    There's a strange bug in the Google Talk MUC server where it sends the
+    <conflict/> stanza twice. This has been reported to their server team; but
+    in any case it triggered a crazy bug in Gabble, so here's a regression test.
+    """
+
+    # Implementation detail: Gabble uses the first part of your jid (if you
+    # don't have an alias) as your room nickname, and appends an underscore a
+    # few times before giving up.
+    jids = ['%s/test%s' % (room_jid, x) for x in ['', '_', '__']]
+    member, member_, member__ = jids
+
+    # Gabble should never get as far as trying to join as 'test__' since
+    # joining as 'test_' will succeed.
+    q.forbid_events([ EventPattern('stream-presence', to=member__) ])
+
+    call_async(q, conn.Requests, 'CreateChannel',
+        dbus.Dictionary({ cs.CHANNEL_TYPE: cs.CHANNEL_TYPE_TEXT,
+                          cs.TARGET_HANDLE_TYPE: cs.HT_ROOM,
+                          cs.TARGET_ID: room_jid,
+                        }, signature='sv'))
+
+    # Gabble first tries to join as test
+    q.expect('stream-presence', to=member)
+
+    # Google Talk says no from 'test', twice.
+    presence = elem('presence', from_=member, type='error')(
+        elem(ns.MUC, 'x'),
+        elem('error', type='cancel')(
+          elem(ns.STANZA, 'conflict'),
+        ))
+    stream.send(presence)
+    stream.send(presence)
+
+    # Gabble should try to join again as test_
+    q.expect('stream-presence', to=member_)
+
+    # Since 'test_' is not in use in the MUC, joining should succeed. According
+    # to XEP-0045 §7.1.3 <http://xmpp.org/extensions/xep-0045.html#enter-pres>:
+    #  The service MUST first send the complete list of the existing occupants
+    #  to the new occupant and only then send the new occupant's own presence
+    #  to the new occupant
+    # but groupchat.google.com cheerfully violates this.
+    stream.send(make_muc_presence('none', 'participant', room_jid, 'test_'))
+
+    # Here's some other random person, who owns the MUC.
+    stream.send(make_muc_presence('owner', 'moderator', room_jid, 'foobar_gmail.com'))
+    # And here's our hypothetical other self.
+    stream.send(make_muc_presence('none', 'participant', room_jid, 'test'))
+
+    # The Gabble bug makes this time out: because Gabble thinks it's joining as
+    # test__ it ignores the presence for test_, since it's not flagged with
+    # code='210' to say “this is you”. (This is acceptable behaviour by the
+    # server: it only needs to include code='210' if it's assigned the client a
+    # name other than the one it asked for.
+    #
+    # The forbidden stream-presence event above doesn't blow up here because
+    # servicetest doesn't process events on the 'stream-*' queue at all when
+    # we're not waiting for one. But during disconnection in the test clean-up,
+    # the forbidden event is encountered and correctly flagged up.
+    event = q.expect('dbus-return', method='CreateChannel')
+    path, _ = event.value
+    text_chan = wrap_channel(bus.get_object(conn.bus_name, path), 'Text')
+
+    # As far as Gabble's concerned, the two other participants joined
+    # immediately after we did.  We can't request handles for them before we
+    # try to join the MUC, because until we do so, Gabble doesn't know that
+    # room_jid is a MUC, and so considers these three JIDs to be different
+    # resources of the same contact. There is no race between this method
+    # returning and MembersChangedDetailed firing, because libdbus reorders
+    # messages when you make blocking calls.
+    handle, handle_, handle__, foobar_handle = conn.RequestHandles(
+        cs.HT_CONTACT, jids + ['%s/foobar_gmail.com' % room_jid])
+
+    q.expect('dbus-signal', signal='MembersChangedDetailed',
+        predicate=lambda e: e.args[0:4] == [[foobar_handle], [], [], []])
+    q.expect('dbus-signal', signal='MembersChangedDetailed',
+        predicate=lambda e: e.args[0:4] == [[handle], [], [], []])
+
+    group_props = text_chan.Properties.GetAll(cs.CHANNEL_IFACE_GROUP)
+    assertEquals(handle_, group_props['SelfHandle'])
+    assertSameSets([handle, handle_, foobar_handle], group_props['Members'])
 
 if __name__ == '__main__':
     exec_test(test)