[Bug 27645] Implement SASL authentication channels.

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27645

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |http://git.collabora.co.uk/
                   |?p=user/eitan/telepathy-gab |?p=user/eitan/telepathy-gab
                   |ble.git;a=shortlog;h=refs/h |ble.git;a=shortlog;h=refs/h
                   |eads/sasl-async-result      |eads/sasl-once-again
  Status Whiteboard|review+                     |

--- Comment #9 from Will Thompson <will.thompson at collabora.co.uk> 2010-05-25 10:49:44 PDT ---
Round three. Fight!

+  /* Used to represent a set of channels.
+   * Keys are GabbleCertificateChannel *, values are an arbitrary non-NULL
pointer.
+   */
+  GabbleServerSaslChannel *server_sasl_channel;

This comment is outdated.

+static const gchar * const server_sasl_channel_fixed_properties[] = {
+    TP_IFACE_CHANNEL ".ChannelType",
+    NULL
+};
+
+static const gchar * const server_sasl_channel_allowed_properties[] = {
+    NULL
+};
+

These are (rightly) unused.

+static GObject *
+gabble_auth_manager_constructor (GType type,
+    guint n_props,
+    GObjectConstructParam *props)

Might be nicer to put the code into _constructed() rather than _constructor()
to avoid the extra cruft the former introduces? (This is just a matter of
personal taste; your call.)

Also, maybe it'd be cool to use GabbleBaseChannel?

change_current_state:

+      gabble_svc_channel_interface_sasl_authentication_emit_state_changed (
+          self,current_status, current_dbus_error, current_message);

Missing a space ^

gabble_server_sasl_channel_start_mechanism() assumes priv->result != NULL
without an assertion; _respond() makes the same assumption, and does assert. I
think this makes Gabble remotely crashable, if someone calls these D-Bus
methods at the wrong time? Better to raise an error (NotAvailable or similar?)
or abort the whole thing if a client messes up, rather than crashing. (Or if it
doesn't crash, I can't see why not, so a test and more obvious robustness would
be good. :) )


+      for (i = (GSList *) mechanisms; i != NULL; i = i->next)
+        g_ptr_array_add (arr, g_strdup ((gchar *)i->data));
+
+      g_ptr_array_add (arr, NULL);
+
+      priv->available_mechanisms = (gchar **) g_ptr_array_free (arr, FALSE);

I feel like I've seen and written this code quite a few times. Maybe we should
have a utility function!

gabble_server_sasl_channel_start_auth_async_func() appears to ignore
allow_plain is_secure_channel, etc. Maybe at least the latter should be exposed
on the Telepathy channel?

   if (!gabble_decode_jid (account, &username, &server, &resource) ||
-      username == NULL)
+      server == NULL)
     {
       g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
           "unable to get username and server from account");

All valid JIDs contain a server part, so the second part of the conditional is
unnecessary. And the error message could use updating!

Otherwise looks good!

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.