Merge branch 'fd.o-35120-crash-when-banned-from-muc'

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

8 files changed, 125 insertions, 46 deletions

diff --git a/src/muc-channel.c b/src/muc-channel.c
index f6ae535b3..1bf418ec3 100644
--- a/src/muc-channel.c
+++ b/src/muc-channel.c
@@ -1399,73 +1399,54 @@ close_channel (GabbleMucChannel *chan, const gchar *reason,
   GabbleMucChannelPrivate *priv = chan->priv;
   GabbleConnection *conn = GABBLE_CONNECTION (
       tp_base_channel_get_connection (base));
-
   TpIntSet *set;
   GArray *handles;
-  GError error = { TP_ERRORS,
-      TP_ERROR_CANCELLED,
-      "Muc channel closed below us"
-  };
+  GError error = { TP_ERRORS, TP_ERROR_CANCELLED,
+      "Muc channel closed below us" };
 
   if (tp_base_channel_is_destroyed (base) || priv->closing)
     return;
 
   DEBUG ("Closing");
+  /* Ensure we stay alive even while telling everyone else to abandon us. */
+  g_object_ref (chan);
 
   gabble_muc_channel_close_tube (chan);
-
   muc_call_channel_finish_requests (chan, NULL, &error);
-
   g_cancellable_cancel (priv->requests_cancellable);
 
   while (priv->calls != NULL)
     tp_base_channel_close (TP_BASE_CHANNEL (priv->calls->data));
 
-  /* Remove us from member list */
-  set = tp_intset_new ();
-  tp_intset_add (set, TP_GROUP_MIXIN (chan)->self_handle);
-
-  tp_group_mixin_change_members ((GObject *) chan,
-                                     (reason != NULL) ? reason : "",
-                                     NULL, set, NULL, NULL, actor,
-                                     reason_code);
-
+  set = tp_intset_new_containing (TP_GROUP_MIXIN (chan)->self_handle);
+  tp_group_mixin_change_members ((GObject *) chan, reason,
+      NULL, set, NULL, NULL, actor, reason_code);
   tp_intset_destroy (set);
 
-  /* Inform the MUC if requested. Don't inform the muc if we're in the
-   * auth state because not all jabberds will echo the MUC presence
-   * when in this state. One example of these jabber servers is M-Link
-   * which is currently running on jabber.org. */
+  /* If we're currently in the MUC, tell it we're leaving and wait for a reply;
+   * handle_parted() will call tp_base_channel_destroyed() and all the Closed
+   * signals will be emitted. (Since there's no waiting-for-password state on
+   * the protocol level, MUC_STATE_AUTH doesn't count as ‘in the MUC’.) See
+   * fd.o#19930 for more details.
+   */
   if (inform_muc && priv->state >= MUC_STATE_INITIATED
       && priv->state != MUC_STATE_AUTH)
     {
-      /* If we want to inform the MUC of our leaving, and we have
-       * actually joined the MUC, then we should wait for our presence
-       * stanza to be given back to us by the conference server before
-       * calling tp_base_channel_destroyed. handle_parted will deal
-       * with calling _destroyed. This is fine because the channel
-       * isn't closed until Closed/ChannelClosed is emitted,
-       * regardless of when the CM returns from Close(). See
-       * fd.o#19930 for more details. */
       send_leave_message (chan, reason);
       priv->closing = TRUE;
     }
   else
     {
-      /* See the comment just above, except we're not sending the
-       * leave message, so let the channel destroy immediately. */
       tp_base_channel_destroyed (base);
     }
 
   handles = tp_handle_set_to_array (chan->group.members);
-
   gabble_presence_cache_update_many (conn->presence_cache, handles,
     NULL, GABBLE_PRESENCE_UNKNOWN, NULL, 0);
-
   g_array_free (handles, TRUE);
 
-  /* Update state and emit Closed signal */
   g_object_set (chan, "state", MUC_STATE_ENDED, NULL);
+  g_object_unref (chan);
 }
 
 gboolean
diff --git a/tests/twisted/Makefile.am b/tests/twisted/Makefile.am
index 85dd955b2..cf41d832f 100644
--- a/tests/twisted/Makefile.am
+++ b/tests/twisted/Makefile.am
@@ -20,8 +20,10 @@ TWISTED_TESTS = \
 	client-types.py \
 	cm/protocol.py \
 	gateways.py \
+	muc/banned.py \
 	muc/conference.py \
 	muc/chat-states.py \
+	muc/kicked.py \
 	muc/name-conflict.py \
 	muc/presence-before-closing.py \
 	muc/renamed.py \
diff --git a/tests/twisted/constants.py b/tests/twisted/constants.py
index db26f0358..dd82ea675 100644
--- a/tests/twisted/constants.py
+++ b/tests/twisted/constants.py
@@ -162,6 +162,8 @@ CERT_UNTRUSTED = ERROR + '.Cert.Untrusted'
 SERVICE_BUSY = ERROR + '.ServiceBusy'
 SERVICE_CONFUSED = ERROR + '.ServiceConfused'
 
+BANNED = ERROR + '.Channel.Banned'
+
 UNKNOWN_METHOD = 'org.freedesktop.DBus.Error.UnknownMethod'
 
 TUBE_PARAMETERS = CHANNEL_IFACE_TUBE + '.Parameters'
diff --git a/tests/twisted/gabbletest.py b/tests/twisted/gabbletest.py
index ba3c22e61..0c6562ed2 100644
--- a/tests/twisted/gabbletest.py
+++ b/tests/twisted/gabbletest.py
@@ -650,6 +650,9 @@ def exec_test_deferred(fun, params, protocol=None, timeout=None,
                 conn.Disconnect()
         except dbus.DBusException, e:
             pass
+        except Exception, e:
+            traceback.print_exc()
+            error = e
 
         try:
             conn.Disconnect()
diff --git a/tests/twisted/muc/banned.py b/tests/twisted/muc/banned.py
new file mode 100644
index 000000000..1d6d8e5e1
--- /dev/null
+++ b/tests/twisted/muc/banned.py
@@ -0,0 +1,31 @@
+"""
+Tests the server refusing to let us join a MUC. This is a regression test for
+<https://bugs.freedesktop.org/show_bug.cgi?id=35120> where Gabble would crash
+in this situation.
+"""
+
+from gabbletest import exec_test, elem
+from mucutil import try_to_join_muc
+import constants as cs
+import ns
+
+MUC = 'deerhoof@evil.lit'
+
+def test(q, bus, conn, stream):
+    try_to_join_muc(q, bus, conn, stream, MUC)
+
+    stream.send(
+        elem('presence', from_=MUC, type='error')(
+          elem(ns.MUC, 'x'),
+          elem('error', code='403', type='auth')(
+            elem(ns.STANZA, 'forbidden'),
+            elem(ns.STANZA, 'text')(
+              u'Access denied by service policy',
+            )
+          )
+        ))
+
+    q.expect('dbus-error', method='CreateChannel', name=cs.BANNED)
+
+if __name__ == '__main__':
+    exec_test(test)
diff --git a/tests/twisted/muc/kicked.py b/tests/twisted/muc/kicked.py
new file mode 100644
index 000000000..e32648853
--- /dev/null
+++ b/tests/twisted/muc/kicked.py
@@ -0,0 +1,51 @@
+"""
+Tests the user being kicked from a MUC. Another symptom of the underlying bug
+behind <https://bugs.freedesktop.org/show_bug.cgi?id=35120> was that this would
+crash.
+"""
+
+from servicetest import assertEquals, assertContains
+from gabbletest import exec_test, elem
+from mucutil import join_muc
+import constants as cs
+import ns
+
+MUC = 'deerhoof@evil.lit'
+
+def test(q, bus, conn, stream):
+    # The user happily joins a MUC
+    _, chan, _, _ = join_muc(q, bus, conn, stream, MUC)
+    muc_self_handle = chan.Group.GetSelfHandle()
+    muc_self_jid, = conn.InspectHandles(cs.HT_CONTACT, [muc_self_handle])
+
+    # But then Bob kicks us.
+    bob_jid = '%s/bob' % MUC
+    bob_handle, = conn.RequestHandles(cs.HT_CONTACT, [bob_jid])
+    stream.send(
+        elem('presence', from_=muc_self_jid, type='unavailable')(
+          elem(ns.MUC_USER, 'x')(
+            elem('item', affiliation='none', role='none')(
+              elem('actor', jid=bob_jid),
+              elem('reason')(
+                u'bye'
+              )
+            ),
+            elem('status', code='307'),
+          )
+        ))
+
+    mcd_event = q.expect('dbus-signal', signal='MembersChangedDetailed')
+    added, removed, local_pending, remote_pending, details = mcd_event.args
+    assertEquals([], added)
+    assertEquals([muc_self_handle], removed)
+    assertEquals([], local_pending)
+    assertEquals([], remote_pending)
+    assertContains('actor', details)
+    assertEquals(bob_handle, details['actor'])
+    assertEquals(cs.GC_REASON_KICKED, details['change-reason'])
+    assertEquals('bye', details['message'])
+
+    q.expect('dbus-signal', signal='ChannelClosed')
+
+if __name__ == '__main__':
+    exec_test(test)
diff --git a/tests/twisted/mucutil.py b/tests/twisted/mucutil.py
index 5d492932c..c99e1a4e8 100644
--- a/tests/twisted/mucutil.py
+++ b/tests/twisted/mucutil.py
@@ -25,13 +25,9 @@ def echo_muc_presence (q, stream, stanza, affiliation, role):
 
     stream.send (stanza)
 
-def join_muc(q, bus, conn, stream, muc, request=None,
-        also_capture=[], role='participant'):
-    """
-    Joins 'muc', returning the muc's handle, a proxy object for the channel,
-    its path and its immutable properties just after the CreateChannel event
-    has fired. The room contains one other member.
-    """
+def try_to_join_muc(q, bus, conn, stream, muc, request=None):
+    """Ask Gabble to join a MUC, and expect it to send <presence/>"""
+
     if request is None:
         request = {
             cs.CHANNEL_TYPE: cs.CHANNEL_TYPE_TEXT,
@@ -39,8 +35,6 @@ def join_muc(q, bus, conn, stream, muc, request=None,
             cs.TARGET_ID: muc,
         }
 
-    muc_handle = request_muc_handle(q, conn, stream, muc)
-
     requests = dbus.Interface(conn, cs.CONN_IFACE_REQUESTS)
     call_async(q, requests, 'CreateChannel',
         dbus.Dictionary(request, signature='sv'))
@@ -54,6 +48,16 @@ def join_muc(q, bus, conn, stream, muc, request=None,
         join_event.stanza)
     assertLength(1, x_muc_nodes)
 
+def join_muc(q, bus, conn, stream, muc, request=None,
+        also_capture=[], role='participant'):
+    """
+    Joins 'muc', returning the muc's handle, a proxy object for the channel,
+    its path and its immutable properties just after the CreateChannel event
+    has fired. The room contains one other member.
+    """
+    muc_handle = request_muc_handle(q, conn, stream, muc)
+    try_to_join_muc(q, bus, conn, stream, muc, request=request)
+
     # Send presence for other member of room.
     stream.send(make_muc_presence('owner', 'moderator', muc, 'bob'))
 
diff --git a/tests/twisted/servicetest.py b/tests/twisted/servicetest.py
index a3a6d8e71..1771ddaf4 100644
--- a/tests/twisted/servicetest.py
+++ b/tests/twisted/servicetest.py
@@ -98,6 +98,14 @@ class EventPattern:
 class TimeoutError(Exception):
     pass
 
+class ForbiddenEventOccurred(Exception):
+    def __init__(self, event):
+        Exception.__init__(self)
+        self.event = event
+
+    def __str__(self):
+        return '\n' + '\n'.join(format_event(self.event))
+
 class BaseEventQueue:
     """Abstract event queue base class.
 
@@ -146,10 +154,7 @@ class BaseEventQueue:
     def _check_forbidden(self, event):
         for e in self.forbidden_events:
             if e.match(event):
-                print "forbidden event occurred:"
-                for x in format_event(event):
-                    print x
-                assert False
+                raise ForbiddenEventOccurred(event)
 
     def expect(self, type, **kw):
         """