Contacts: don't wrongly mark SimplePresence prepared

Previously, if a broken CM included the SimplePresence/presence
attribute when TpContact didn't ask for it,
CONTACT_FEATURE_FLAG_PRESENCE would still be set, even if we weren't
listening to PresencesChanged. So this is broken.

This fix makes TpContact ignore unsolicited presence information in
GetContactAttributes replies. The other option was to bind to the signal
in that event, which seems … surprising.

7 files changed, 244 insertions, 8 deletions

diff --git a/telepathy-glib/contact.c b/telepathy-glib/contact.c
index 9c49e0d1d..b3f362a53 100644
--- a/telepathy-glib/contact.c
+++ b/telepathy-glib/contact.c
@@ -2244,9 +2244,10 @@ contact_maybe_set_simple_presence (TpContact *contact,
   const gchar *status;
   const gchar *message;
 
-  if (contact == NULL || presence == NULL)
+  if (contact == NULL)
     return;
 
+  g_return_if_fail (presence != NULL);
   contact->priv->has_features |= CONTACT_FEATURE_FLAG_PRESENCE;
 
   tp_value_array_unpack (presence, 3, &type, &status, &message);
@@ -3604,10 +3605,21 @@ tp_contact_set_attributes (TpContact *contact,
       contact_set_avatar_token (contact, s, TRUE);
     }
 
-  boxed = tp_asv_get_boxed (asv,
-      TP_TOKEN_CONNECTION_INTERFACE_SIMPLE_PRESENCE_PRESENCE,
-      TP_STRUCT_TYPE_SIMPLE_PRESENCE);
-  contact_maybe_set_simple_presence (contact, boxed);
+  if (wanted & CONTACT_FEATURE_FLAG_PRESENCE)
+    {
+      boxed = tp_asv_get_boxed (asv,
+          TP_TOKEN_CONNECTION_INTERFACE_SIMPLE_PRESENCE_PRESENCE,
+          TP_STRUCT_TYPE_SIMPLE_PRESENCE);
+
+      if (boxed == NULL)
+        WARNING ("%s supposedly implements Contacts and SimplePresence, "
+            "but omitted the mandatory "
+            TP_TOKEN_CONNECTION_INTERFACE_SIMPLE_PRESENCE_PRESENCE
+            " attribute",
+            tp_proxy_get_object_path (connection));
+      else
+        contact_maybe_set_simple_presence (contact, boxed);
+    }
 
   /* Location */
   if (wanted & CONTACT_FEATURE_FLAG_LOCATION)
diff --git a/tests/dbus/contacts.c b/tests/dbus/contacts.c
index f6c4f4d52..42d5049f1 100644
--- a/tests/dbus/contacts.c
+++ b/tests/dbus/contacts.c
@@ -25,6 +25,7 @@
 #include <telepathy-glib/debug.h>
 
 #include "tests/lib/contacts-conn.h"
+#include "tests/lib/broken-client-types-conn.h"
 #include "tests/lib/debug.h"
 #include "tests/lib/myassert.h"
 #include "tests/lib/util.h"
@@ -2411,6 +2412,105 @@ test_no_location (Fixture *f,
 }
 
 static void
+setup_broken_client_types_conn (Fixture *f,
+    gconstpointer unused G_GNUC_UNUSED)
+{
+  tp_tests_create_and_connect_conn (
+      TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION,
+      "me@test.com", &f->base_connection, &f->client_conn);
+
+  f->service_conn = TP_TESTS_CONTACTS_CONNECTION (f->base_connection);
+  g_object_ref (f->service_conn);
+
+  f->service_repo = tp_base_connection_get_handles (f->base_connection,
+      TP_HANDLE_TYPE_CONTACT);
+  f->result.loop = g_main_loop_new (NULL, FALSE);
+}
+
+static void
+test_superfluous_attributes (Fixture *f,
+    gconstpointer unused G_GNUC_UNUSED)
+{
+  TpHandle handle;
+  TpContact *contact;
+  const gchar * const *client_types;
+  TpContactFeature client_types_feature = TP_CONTACT_FEATURE_CLIENT_TYPES;
+  TpContactFeature presence_feature = TP_CONTACT_FEATURE_PRESENCE;
+
+  g_assert (TP_TESTS_IS_BROKEN_CLIENT_TYPES_CONNECTION (f->service_conn));
+
+  handle = tp_handle_ensure (f->service_repo, "helge", NULL, NULL);
+  g_assert_cmpuint (handle, !=, 0);
+
+  /* We ask for ClientTypes; the CM is broken and adds SimplePresence
+   * information to the reply... it also omits the /client-types attribute from
+   * the reply, which, since the spec says “Omitted from the result if the
+   * contact's client types are not known.” leaves us in the exciting position
+   * of having to decide between marking the feature as prepared anyway or
+   * saying it failed, and also deciding whether get_client_types returns [] or
+   * NULL...
+   */
+  tp_connection_get_contacts_by_handle (f->client_conn,
+      1, &handle,
+      1, &client_types_feature,
+      by_handle_cb,
+      &f->result, finish, NULL);
+  g_main_loop_run (f->result.loop);
+  g_assert_cmpuint (f->result.contacts->len, ==, 1);
+  g_assert_cmpuint (f->result.invalid->len, ==, 0);
+  g_assert_no_error (f->result.error);
+
+  g_assert (g_ptr_array_index (f->result.contacts, 0) != NULL);
+  contact = g_object_ref (g_ptr_array_index (f->result.contacts, 0));
+  g_assert_cmpuint (tp_contact_get_handle (contact), ==, handle);
+
+  /* She doesn't have any client types. There are two reasonable ways to
+   * represent this.
+   */
+  client_types = tp_contact_get_client_types (contact);
+  if (client_types != NULL)
+    g_assert_cmpstr (client_types[0], ==, NULL);
+
+  /* She also shouldn't have any presence information, despite it being
+   * inexplicably included in the GetContactAttributes reply. Specifically:
+   * because we have not connected to PresencesChanged, it's not safe to just
+   * randomly stash this information and mark the feature as prepared.
+   *
+   * (If we wanted to be really smart we could do something like: if the
+   * information's there for some reason, and we happen already to be bound to
+   * PresencesChanged due to preparing that feature on another contact … then
+   * accept the mysterious information. But that seems fragile and prone to
+   * people relying on sketchy behaviour.)
+   */
+  g_assert_cmpstr (tp_contact_get_presence_message (contact), ==, "");
+  g_assert_cmpstr (tp_contact_get_presence_status (contact), ==, "");
+  g_assert_cmpuint (tp_contact_get_presence_type (contact), ==,
+      TP_CONNECTION_PRESENCE_TYPE_UNSET);
+
+  reset_result (&f->result);
+
+  /* So now if we try to prepare TP_CONTACT_FEATURE_PRESENCE, we should need to
+   * make some D-Bus calls: it shouldn't have been marked prepared by the
+   * previous call. Successfully upgrading to this feature is tested
+   * elsewhere, so we'll test that upgrading fails if the connection's
+   * mysteriously died.
+   */
+  make_the_connection_disappear (f);
+  tp_connection_get_contacts_by_handle (f->client_conn,
+      1, &handle,
+      1, &presence_feature,
+      by_handle_cb,
+      &f->result, finish, NULL);
+  g_main_loop_run (f->result.loop);
+  /* Not gonna make any particular assertions about what the error is. */
+  g_assert_error (f->result.error, DBUS_GERROR, DBUS_GERROR_UNKNOWN_METHOD);
+
+  put_the_connection_back (f);
+  reset_result (&f->result);
+  g_object_unref (contact);
+}
+
+static void
 setup (Fixture *f,
     gconstpointer unused G_GNUC_UNUSED)
 {
@@ -2511,5 +2611,9 @@ main (int argc,
 
   ADD (no_location);
 
+  g_test_add ("/contacts/superfluous-attributes", Fixture, NULL,
+      setup_broken_client_types_conn, test_superfluous_attributes,
+      teardown);
+
   return g_test_run ();
 }
diff --git a/tests/lib/Makefile.am b/tests/lib/Makefile.am
index 2adf66431..cbc34bce1 100644
--- a/tests/lib/Makefile.am
+++ b/tests/lib/Makefile.am
@@ -18,6 +18,8 @@ libtp_glib_tests_la_SOURCES = \
     bug-19101-conn.h \
     bug16307-conn.c \
     bug16307-conn.h \
+    broken-client-types-conn.c \
+    broken-client-types-conn.h \
     contacts-conn.c \
     contacts-conn.h \
     contact-list-manager.c \
diff --git a/tests/lib/broken-client-types-conn.c b/tests/lib/broken-client-types-conn.c
new file mode 100644
index 000000000..e4d73f310
--- /dev/null
+++ b/tests/lib/broken-client-types-conn.c
@@ -0,0 +1,61 @@
+/*
+ * broken-client-types-conn.c - a connection with a broken client
+ *   types implementation which inexplicably returns presence information!
+ *
+ * Copyright © 2011 Collabora Ltd. <http://www.collabora.co.uk/>
+ *
+ * Copying and distribution of this file, with or without modification,
+ * are permitted in any medium without royalty provided the copyright
+ * notice and this notice are preserved.
+ */
+#include "broken-client-types-conn.h"
+
+#include <telepathy-glib/interfaces.h>
+
+G_DEFINE_TYPE_WITH_CODE (TpTestsBrokenClientTypesConnection,
+    tp_tests_broken_client_types_connection,
+    TP_TESTS_TYPE_CONTACTS_CONNECTION,
+    G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_CONNECTION_INTERFACE_CLIENT_TYPES,
+        NULL);
+    );
+
+static void
+tp_tests_broken_client_types_connection_init (
+    TpTestsBrokenClientTypesConnection *self)
+{
+}
+
+static void
+broken_fill_client_types (
+    GObject *object,
+    const GArray *contacts,
+    GHashTable *attributes)
+{
+  guint i;
+
+  for (i = 0; i < contacts->len; i++)
+    {
+      TpHandle handle = g_array_index (contacts, guint, i);
+      /* Muahaha. Actually we add SimplePresence information. */
+      GValueArray *presence = tp_value_array_build (3,
+          G_TYPE_UINT, TP_CONNECTION_PRESENCE_TYPE_AVAILABLE,
+          G_TYPE_STRING, "available",
+          G_TYPE_STRING, "hi mum!",
+          G_TYPE_INVALID);
+
+      tp_contacts_mixin_set_contact_attribute (attributes,
+          handle,
+          TP_TOKEN_CONNECTION_INTERFACE_SIMPLE_PRESENCE_PRESENCE,
+          tp_g_value_slice_new_take_boxed (G_TYPE_VALUE_ARRAY, presence));
+    }
+}
+
+static void
+tp_tests_broken_client_types_connection_class_init (
+    TpTestsBrokenClientTypesConnectionClass *klass)
+{
+  TpTestsContactsConnectionClass *cc_class =
+      TP_TESTS_CONTACTS_CONNECTION_CLASS (klass);
+
+  cc_class->fill_client_types = broken_fill_client_types;
+}
diff --git a/tests/lib/broken-client-types-conn.h b/tests/lib/broken-client-types-conn.h
new file mode 100644
index 000000000..ad36d7d54
--- /dev/null
+++ b/tests/lib/broken-client-types-conn.h
@@ -0,0 +1,50 @@
+/*
+ * broken-client-types-conn.h - header for a connection with a broken client
+ *   types implementation which inexplicably returns presence information!
+ *
+ * Copyright © 2011 Collabora Ltd. <http://www.collabora.co.uk/>
+ *
+ * Copying and distribution of this file, with or without modification,
+ * are permitted in any medium without royalty provided the copyright
+ * notice and this notice are preserved.
+ */
+
+#ifndef TP_TESTS_BROKEN_CLIENT_TYPES_CONN_H
+#define TP_TESTS_BROKEN_CLIENT_TYPES_CONN_H
+
+#include "contacts-conn.h"
+
+typedef struct _TpTestsBrokenClientTypesConnection TpTestsBrokenClientTypesConnection;
+typedef struct _TpTestsBrokenClientTypesConnectionClass TpTestsBrokenClientTypesConnectionClass;
+typedef struct _TpTestsBrokenClientTypesConnectionPrivate TpTestsBrokenClientTypesConnectionPrivate;
+
+struct _TpTestsBrokenClientTypesConnectionClass {
+    TpTestsContactsConnectionClass parent_class;
+};
+
+struct _TpTestsBrokenClientTypesConnection {
+    TpTestsContactsConnection parent;
+
+    TpTestsBrokenClientTypesConnectionPrivate *priv;
+};
+
+GType tp_tests_broken_client_types_connection_get_type (void);
+
+/* HI MUM */
+#define TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION \
+  (tp_tests_broken_client_types_connection_get_type ())
+#define TP_TESTS_BROKEN_CLIENT_TYPES_CONNECTION(obj) \
+  (G_TYPE_CHECK_INSTANCE_CAST((obj), TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION, \
+                              TpTestsBrokenClientTypesConnection))
+#define TP_TESTS_BROKEN_CLIENT_TYPES_CONNECTION_CLASS(klass) \
+  (G_TYPE_CHECK_CLASS_CAST((klass), TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION, \
+                           TpTestsBrokenClientTypesConnectionClass))
+#define TP_TESTS_IS_BROKEN_CLIENT_TYPES_CONNECTION(obj) \
+  (G_TYPE_CHECK_INSTANCE_TYPE((obj), TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION))
+#define TP_TESTS_IS_BROKEN_CLIENT_TYPES_CONNECTION_CLASS(klass) \
+  (G_TYPE_CHECK_CLASS_TYPE((klass), TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION))
+#define TP_TESTS_BROKEN_CLIENT_TYPES_CONNECTION_GET_CLASS(obj) \
+  (G_TYPE_INSTANCE_GET_CLASS ((obj), TP_TESTS_TYPE_BROKEN_CLIENT_TYPES_CONNECTION, \
+                              TpTestsBrokenClientTypesConnectionClass))
+
+#endif // TP_TESTS_BROKEN_CLIENT_TYPES_CONN_H
diff --git a/tests/lib/contacts-conn.c b/tests/lib/contacts-conn.c
index a993beba0..82eb23212 100644
--- a/tests/lib/contacts-conn.c
+++ b/tests/lib/contacts-conn.c
@@ -348,9 +348,14 @@ client_types_fill_contact_attributes (
     const GArray *contacts,
     GHashTable *attributes)
 {
-  /* Do nothing: a no-op implementation is valid, relatively speaking. The spec
-   * sez the /client-types attribute should be “omitted from the result if the
-   * contact's client types are not known.”
+  TpTestsContactsConnectionClass *klass =
+      TP_TESTS_CONTACTS_CONNECTION_GET_CLASS (object);
+
+  if (klass->fill_client_types != NULL)
+    klass->fill_client_types (object, contacts, attributes);
+  /* …else do nothing: a no-op implementation is valid, relatively speaking.
+   * The spec sez the /client-types attribute should be “omitted from the
+   * result if the contact's client types are not known.”
    */
 }
 
diff --git a/tests/lib/contacts-conn.h b/tests/lib/contacts-conn.h
index 832c2ba31..c085c8c80 100644
--- a/tests/lib/contacts-conn.h
+++ b/tests/lib/contacts-conn.h
@@ -32,6 +32,8 @@ struct _TpTestsContactsConnectionClass {
     TpPresenceMixinClass presence_mixin;
     TpContactsMixinClass contacts_mixin;
     TpDBusPropertiesMixinClass properties_class;
+
+    TpContactsMixinFillContactAttributesFunc fill_client_types;
 };
 
 struct _TpTestsContactsConnection {