[Bug 27645] Implement SASL authentication channels.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #11 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-05-25 12:37:36 PDT ---
(In reply to comment #9)
> 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.

3e56f8e

> 
> +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.
> 

2204b2b

> +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.)
> 

09663b5

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

It already inherits from WockyAuthRegistry.

> change_current_state:
> 
> +      gabble_svc_channel_interface_sasl_authentication_emit_state_changed (
> +          self,current_status, current_dbus_error, current_message);
> 
> Missing a space ^
> 

06722e2

> 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. :) )

87667b8

> 
> 
> +      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!
> 

Actually in a later changeset I kept the GPtrArray for the duration. Seemed a
bit nicer. (fd0cf7c).

> 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?

Yeah :P

This makes changes across the board, it wasn't actually implemented in Wocky,
so I opened a bug for it (bug 28247).
I put it in place in e5cbfe0 & e5c68b9.

> 
>    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!
> 

708053a

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