Don't unblock contacts if they're removed from stored.

We want to ensure that clients which don't understand the 'deny' list
can remove contacts from other lists without accidentally unblocking
them. The only problematic case is 'stored': currently, the XMPP-level
response to removing someone from 'stored' is sending a roster update
removing them from the roster entirely. Since their blocked-ness is
stored on the roster, ...

So instead, when we would like to remove a blocked contact from the
roster, we instead cancel inbound and/or outbound subscriptions. This
has the effect of the server changing their subscription to 'none', at
which point they disappear from 'stored' (because on Google Talk we
hide subscription='none', ask≠'subscribe' contacts from stored).

There are two code paths for editing the roster, depending on whether we
have an edit in flight or not. The latter case is reasonably
straightforward. For the former case, we can be applying multiple edits
at once, so this patch moves the modification of the Google Roster state
to before the modification of the subscription state, to ensure that
queued modifications to the contact's blocked-ness have been processed
before we consider whether to handle removal specially.

Fixes fd.o#20597 (Blocked contacts get removed if they are removed from
stored)

2 files changed, 271 insertions, 18 deletions

diff --git a/src/roster.c b/src/roster.c
index bf7e6d707..f042a703b 100644
--- a/src/roster.c
+++ b/src/roster.c
@@ -2043,6 +2043,34 @@ static LmHandlerResult roster_edited_cb (GabbleConnection *conn,
                                          GObject *roster_obj,
                                          gpointer user_data);
 
+/*
+ * Cancel any subscriptions on @item by sending unsubscribe and/or
+ * unsubscribed, as appropriate.
+ */
+static gboolean
+roster_item_cancel_subscriptions (
+    GabbleRoster *roster,
+    TpHandle contact,
+    GabbleRosterItem *item,
+    GError **error)
+{
+  gboolean ret = TRUE;
+
+  if (item->subscription & GABBLE_ROSTER_SUBSCRIPTION_FROM)
+    {
+      DEBUG ("sending unsubscribed");
+      ret = gabble_roster_handle_unsubscribed (roster, contact, NULL, error);
+    }
+
+  if (ret && (item->subscription & GABBLE_ROSTER_SUBSCRIPTION_TO))
+    {
+      DEBUG ("sending unsubscribe");
+      ret = gabble_roster_handle_unsubscribe (roster, contact, NULL, error);
+    }
+
+  return ret;
+}
+
 /* Apply the unsent edits to the given roster item.
  *
  * \param roster The roster
@@ -2054,7 +2082,7 @@ roster_item_apply_edits (GabbleRoster *roster,
                          TpHandle contact,
                          GabbleRosterItem *item)
 {
-  gboolean altered = FALSE, ret;
+  gboolean altered = FALSE, ret = TRUE;
   GabbleRosterItem edited_item;
   TpIntSet *intset;
   GabbleRosterPrivate *priv = roster->priv;
@@ -2080,13 +2108,42 @@ roster_item_apply_edits (GabbleRoster *roster,
     }
 #endif
 
+  if (edits->new_google_type != GOOGLE_ITEM_TYPE_INVALID
+      && edits->new_google_type != item->google_type)
+    {
+      DEBUG ("Changing Google type from %d to %d", item->google_type,
+             edits->new_google_type);
+      altered = TRUE;
+      edited_item.google_type = edits->new_google_type;
+    }
+
   if (edits->new_subscription != GABBLE_ROSTER_SUBSCRIPTION_INVALID
       && edits->new_subscription != item->subscription)
     {
-      DEBUG ("Changing subscription from %d to %d",
-             item->subscription, edits->new_subscription);
-      altered = TRUE;
-      edited_item.subscription = edits->new_subscription;
+      /* Here we check the google_type of the *edited* item (as patched in the
+       * block above) to deal correctly with a batch of edits containing both
+       * (un)block and remove.
+       */
+      if (edits->new_subscription == GABBLE_ROSTER_SUBSCRIPTION_REMOVE &&
+          edited_item.google_type == GOOGLE_ITEM_TYPE_BLOCKED)
+        {
+          /* If they're blocked, we can't just remove them from the roster,
+           * because that would unblock them! So instead, we cancel both
+           * subscription directions.
+           */
+          DEBUG ("contact is blocked; not removing");
+          ret = roster_item_cancel_subscriptions (roster, contact, item, NULL);
+          /* deliberately not setting altered: we haven't altered the roster
+           * directly.
+           */
+        }
+      else
+        {
+          DEBUG ("Changing subscription from %d to %d",
+              item->subscription, edits->new_subscription);
+          altered = TRUE;
+          edited_item.subscription = edits->new_subscription;
+        }
     }
 
   if (edits->new_name != NULL && tp_strdiff (item->name, edits->new_name))
@@ -2096,15 +2153,6 @@ roster_item_apply_edits (GabbleRoster *roster,
       edited_item.name = edits->new_name;
     }
 
-  if (edits->new_google_type != GOOGLE_ITEM_TYPE_INVALID
-      && edits->new_google_type != item->google_type)
-    {
-      DEBUG ("Changing Google type from %d to %d", item->google_type,
-             edits->new_google_type);
-      altered = TRUE;
-      edited_item.google_type = edits->new_google_type;
-    }
-
   if (edits->add_to_groups || edits->remove_from_groups)
     {
 #ifdef ENABLE_DEBUG
@@ -2187,7 +2235,7 @@ roster_item_apply_edits (GabbleRoster *roster,
       &edited_item);
   ret = _gabble_connection_send_with_reply (priv->conn,
       message, roster_edited_cb, G_OBJECT (roster),
-      GUINT_TO_POINTER(contact), NULL);
+      GUINT_TO_POINTER(contact), NULL) && ret;
   if (ret)
     {
       /* assume everything will be OK */
@@ -2453,6 +2501,15 @@ gabble_roster_handle_remove (GabbleRoster *roster,
       item->unsent_edits->new_subscription = GABBLE_ROSTER_SUBSCRIPTION_REMOVE;
       return TRUE;
     }
+  else if (item->google_type == GOOGLE_ITEM_TYPE_BLOCKED)
+    {
+      /* If they're blocked, we can't just remove them from the roster,
+       * because that would unblock them! So instead, we cancel both
+       * subscription directions.
+       */
+      DEBUG ("contact#%u is blocked; not removing", handle);
+      return roster_item_cancel_subscriptions (roster, handle, item, error);
+    }
   else
     {
       DEBUG ("immediate edit to contact#%u - change subscription to REMOVE",
diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py
index 9964cfcfd..8ebf2abfd 100644
--- a/tests/twisted/roster/test-google-roster.py
+++ b/tests/twisted/roster/test-google-roster.py
@@ -1,3 +1,4 @@
+# vim: set fileencoding=utf-8 :
 """
 Test workarounds for gtalk
 """
@@ -9,7 +10,7 @@ from gabbletest import (
     expect_list_channel
     )
 from servicetest import (
-    sync_dbus, EventPattern, wrap_channel,
+    call_async, sync_dbus, EventPattern,
     assertLength, assertEquals, assertContains,
     )
 import constants as cs
@@ -18,11 +19,11 @@ import ns
 from twisted.words.protocols.jabber.client import IQ
 from twisted.words.xish import domish
 
-def make_set_roster_iq(stream, user, contact, state, ask):
+def make_set_roster_iq(stream, user, contact, state, ask, attrs={}):
     iq = IQ(stream, 'set')
     query = iq.addElement((ns.ROSTER, 'query'))
     add_gr_attributes(query)
-    add_roster_item(query, contact, state, ask)
+    add_roster_item(query, contact, state, ask, attrs=attrs)
     return iq
 
 def add_gr_attributes(query):
@@ -48,6 +49,12 @@ def is_stored(event):
 def is_subscribe(event):
     return event.path.endswith('/subscribe')
 
+def is_publish(event):
+    return event.path.endswith('/publish')
+
+def is_deny(event):
+    return event.path.endswith('/deny')
+
 def test_inital_roster(q, bus, conn, stream):
     """
     This part of the test checks that Gabble correctly alters on which lists
@@ -234,12 +241,201 @@ def test_flickering(q, bus, conn, stream, subscribe):
     sync_dbus(bus, q, conn)
     q.unforbid_events(change_events)
 
+# This event is forbidden in all of the deny tests!
+remove_events = [
+    EventPattern('stream-iq', query_ns=ns.ROSTER,
+        predicate=(lambda event:
+            event.query.firstChildElement()['subscription'] == 'remove'),
+        )
+    ]
+
+def test_deny_simple(q, bus, conn, stream, stored, deny):
+    """
+    If we remove a blocked contact from 'stored', they shouldn't actually be
+    removed from the roster: rather, we should cancel both subscription
+    directions, at which point they will vanish from 'stored', while
+    remaining on 'deny'.
+    """
+
+    contact = 'blocked-but-subscribed@boards.ca'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0]
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    stored.Group.RemoveMembers([handle], "")
+
+    q.forbid_events(remove_events)
+
+    q.expect_many(
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribe'),
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribed'),
+        )
+
+    # Our server sends roster pushes in response to our unsubscribe and
+    # unsubscribed commands.
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "from", False, attrs={'gr:t': 'B'}))
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False, attrs={'gr:t': 'B'}))
+
+    # As a result they should drop off all three non-deny lists, but not fall
+    # off deny:
+    q.expect_many(
+        *[ EventPattern('dbus-signal', signal='MembersChanged',
+                        args=['', [], [handle], [], [], 0, cs.GC_REASON_NONE],
+                        predicate=p)
+           for p in [is_stored, is_subscribe, is_publish]
+         ])
+
+    assertContains(handle,
+        deny.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.unforbid_events(remove_events)
+
+def test_deny_overlap_one(q, bus, conn, stream, subscribe, stored, deny):
+    """
+    Here's a tricker case: blocking a contact, and then removing them before
+    the server's responded to the block request.
+    """
+
+    # As we saw in test_flickering(), we have a subscription to Bob,
+    # everything's peachy.
+    contact = 'bob@foo.com'
+    handle = conn.RequestHandles(cs.HT_CONTACT, ['bob@foo.com'])[0]
+
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        subscribe.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.forbid_events(remove_events)
+
+    # But then we have a falling out. In a blind rage, I block Bob:
+    call_async(q, deny.Group, 'AddMembers', [handle], "")
+    event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    item = event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('B', item[(ns.GOOGLE_ROSTER, 't')])
+
+    # Then — *before the server has replied* — I remove him from stored.
+    call_async(q, stored.Group, 'RemoveMembers', [handle], "")
+
+    # subscription='remove' is still forbidden from above. So we sync to ensure
+    # that Gabble's received RemoveMembers, and if it's going to send us a
+    # remove (or premature <presence type='unsubscribe'/>) we catch it.
+    sync_dbus(bus, q, conn)
+    sync_stream(q, stream)
+
+    # So now we send a roster push and reply for the block request.
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+        'to', False, attrs={ 'gr:t': 'B' }))
+    acknowledge_iq(stream, event.stanza)
+
+    # At which point, Bob should appear on 'deny', and Gabble should send an
+    # unsubscribe, but *not* an unsubscribe*d* because Bob wasn't subscribed to
+    # us!
+    unsubscribed_events = [
+        EventPattern('stream-presence', presence_type='unsubscribed')
+        ]
+    q.forbid_events(unsubscribed_events)
+
+    q.expect_many(
+        EventPattern('dbus-signal', signal='MembersChanged', predicate=is_deny,
+            args=["", [handle], [], [], [], 0, 0]),
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribe'),
+        )
+
+    # And our server sends us a roster push in response to unsubscribe:
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False, attrs={'gr:t': 'B'}))
+
+    # As a result, Gabble makes Bob fall off subscribe and stored.
+    q.expect_many(
+        EventPattern('dbus-signal', signal='MembersChanged',
+            predicate=is_subscribe,
+            args=["", [], [handle], [], [], 0, 0]),
+        EventPattern('dbus-signal', signal='MembersChanged',
+            predicate=is_stored,
+            args=["", [], [handle], [], [], 0, 0]),
+        )
+
+    # And he should definitely still be on deny. That rascal.
+    assertContains(handle,
+        deny.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.unforbid_events(unsubscribed_events)
+    q.unforbid_events(remove_events)
+
+def test_deny_overlap_two(q, bus, conn, stream,
+                          subscribe, publish, stored, deny):
+    """
+    Here's another tricky case: editing a contact (setting an alias, say), and
+    then while that edit's in flight, blocking and remove the contact.
+    """
+
+    # This contact was on our roster when we started.
+    contact = 'lp-bug-298293@gmail.com'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0]
+
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        subscribe.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        publish.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    # Once again, at no point in this test should anyone be removed outright.
+    q.forbid_events(remove_events)
+
+    # First up, we edit the contact's alias, triggering a roster update from
+    # the client.
+    conn.Aliasing.SetAliases({handle: 'oh! the huge manatee!'})
+    event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    item = event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('oh! the huge manatee!', item['name'])
+
+    # Before the server responds, we block and remove the contact. The edits
+    # should be queued...
+    patterns = [
+        EventPattern('stream-iq', query_ns=ns.ROSTER),
+        EventPattern('stream-presence', presence_type='unsubscribed'),
+        EventPattern('stream-presence', presence_type='unsubscribe'),
+        ]
+    q.forbid_events(patterns)
+
+    deny.Group.AddMembers([handle], '')
+    stored.Group.RemoveMembers([handle], '')
+
+    # Make sure if the edits are sent prematurely, we've got them.
+    sync_stream(q, stream)
+    q.unforbid_events(patterns)
+
+    # Okay, now we respond to the alias update. At this point we expect an
+    # update to gr:t=B, leaving subscription=both intact, and subscription
+    # cancellations.
+    acknowledge_iq(stream, event.stanza)
+    roster_event, _, _ = q.expect_many(*patterns)
+
+    item = roster_event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('B', item[(ns.GOOGLE_ROSTER, 't')])
+
+    # And we're done. Clean up.
+    q.unforbid_events(remove_events)
+
 def test(q, bus, conn, stream):
     conn.Connect()
 
     publish, subscribe, stored, deny = test_inital_roster(q, bus, conn, stream)
 
     test_flickering(q, bus, conn, stream, subscribe)
+    test_deny_simple(q, bus, conn, stream, stored, deny)
+    test_deny_overlap_one(q, bus, conn, stream, subscribe, stored, deny)
+    test_deny_overlap_two(q, bus, conn, stream,
+        subscribe, publish, stored, deny)
 
 if __name__ == '__main__':
     exec_test(test, protocol=GoogleXmlStream)