[Bug 27645] Implement SASL authentication channels.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-04-15 12:32:15 PDT ---
You are very good at exposing my sloppy.

(In reply to comment #1)
> And here is my review:
> 
> == C code ==
> 
> Rather than adding a get_authentication_manager() function, add another field
> to GabbleConnection like the other ChannelManagers?
> 

Done (51046b2). It's confusing with the public and private struct. Went with
the private one because it's not an override.

> In connector_error_disconnect:
> 
>   else if (error->domain == WOCKY_SASL_AUTH_ERROR)
>     {
>       reason = TP_CONNECTION_STATUS_REASON_AUTHENTICATION_FAILED;
>     }
> 
> I think this is already fixed in master.

Done (01bc61a).

> 
> I wouldn't be opposed to calling GabbleAuthenticationManager GabbleAuthManager
> to save on typing.
> 

Done (29b7ab9).

> Coding style: the braces and body in the block added to
> _gabble_connection_connect() should be two spaces further in.
> 
> 

Done (9422985).

>   struct _GabbleAuthenticationManagerPrivate
>   {
>     GabbleConnection *conn;
> 
>     /* Used to represent a set of channels.
>      * Keys are GabbleCertificateChannel *, values are an arbitrary non-NULL
> pointer.
>      */
>     GHashTable *channels;
> 
>     GabbleServerSaslChannel *server_sasl_handler;
> 
> 'channels' is never used. The only channel this manager cares about is
> 'server_sasl_handler', and it's not used in
> gabble_authentication_manager_foreach_channel(). 

I was keeping that around for a future when the manager actually manages
channels, but it won't be hard to add later. Done (7e92ce8).

> I think 'server_sasl_handler'
> has three states as far as the auth manager is concerned:
> 
>  • Not yet announced;
>  • Announced;
>  • Closed

What's with all these fancy bullets and Unicode you flaunt? You should write a
blog post about keyboard shortcuts for useful chars.

> 
> Currently GabbleAuthenticationManager tracks the first state transition, but
> not the second. I think it wants to also listen for the "closed" signal
> (calling tp_channel_manager_emit_channel_closed_for_object() when it's
> received), 

I added a callback, although currently I don't think it's ever called. When
closing a server sasl channel, the connection is subsequently closed and the
auth manager disposed, (5af8aef).

> and then, in gabble_authentication_manager_foreach_channel() only
> apply the function to 'server_sasl_handler' if it's in the middle state.

Done (2514445).

> Finally, blow away all references to 'channels'.

Done (7e92ce8).

> 
> I think you want to leave foreach_channel_class() unimplemented. It's just used
> to implement the RequestableChannelClasses property, but these channels aren't
> requestable, so that'll be a lie.
> 

Done (7e92ce8).

> Why does gabble_server_sasl_channel_register() accept a path, and completely
> ignore it? Given that we only have one of these per connection, maybe the
> channel's constructed() could set self->base.object_path itself, and then the
> AuthManager could just call gabble_base_channel_register().
> 

The constructor is premature since the connection does not have an object path
yet. But I removed the silly argument (faf8837).

>     G_IMPLEMENT_INTERFACE (GABBLE_TYPE_CAPS_CHANNEL_MANAGER,
>       caps_channel_manager_iface_init));
> 
> I think you can use NULL as the second parameter given that
> caps_channel_manager_iface_init() is a no-op.
> 

Done (8f06724).

> #define GABBLE_SERVER_SASL_CHANNEL_GET_PRIVATE(obj) ((obj)->priv)
> 
> What's wrong with just using self->priv directly? (I know some of the other
> classes in Gabble have roughly this macro; I think they're wrong too. :))
> 

It's hard to keep track of what is trendy for referencing priv :)
Done (717059e).

> #define SERVER_SASL_CHANNEL_TYPE_CHALLENGE_LIST \
>   gabble_server_sasl_channel_challenge_list_get_type ()
> 
> #define SERVER_SASL_CHANNEL_TYPE_CHALLENGE \
>   gabble_server_sasl_channel_challenge_get_type ()
> 
> Doesn't the code generator generate GTypes for this? I could be wrong, maybe it
> doesn't, but I thought it did.

No, I expected it to be generated too. I tried defining a 'ay' simple-type in
the spec, but that didn't help either. I would love to get rid of that.

> 
>   struct _GabbleServerSaslChannelPrivate
>   ...
>     gint auth_state;
> 
> Could this have a nicer type?

Done, that's a relic from a hackish -1 state (902f95e).

> 
>       case PROP_CHANNEL_PROPERTIES:
>         g_value_take_boxed (value,
>             tp_dbus_properties_mixin_make_properties_hash (object,
>                 TP_IFACE_CHANNEL, "TargetHandle",
>                 TP_IFACE_CHANNEL, "TargetHandleType",
>                 TP_IFACE_CHANNEL, "ChannelType",
>                 TP_IFACE_CHANNEL, "TargetID",
>                 TP_IFACE_CHANNEL, "InitiatorHandle",
>                 TP_IFACE_CHANNEL, "InitiatorID",
>                 TP_IFACE_CHANNEL, "Requested",
>                 TP_IFACE_CHANNEL, "Interfaces",
>                 NULL));
> 
> Isn't at least PeerIdentity an immutable property? 

Done (35cf3c9).

> All the immutable properties
> should be included here. In fact, I think that since the channel isn't
> announced until the mechanisms are known, the mechanisms are immutable in the
> Telepathy sense too.
> 

Not on the spec level. More sophisticated mechanism negotiations could have the
advertised mechanisms change in the lifetime of the channel. Could the same
property be immutable in one channel and not in another?

>       g_value_init (&tmp, GABBLE_STRUCT_TYPE_MECHANISM_DETAILS);
> 
>       g_value_take_boxed (&tmp, dbus_g_type_specialized_construct (
>               GABBLE_STRUCT_TYPE_MECHANISM_DETAILS));
> 
>       dbus_g_type_struct_set (&tmp,
>           0, tp_mech_name,
>           1, g_hash_table_new (g_str_hash, g_str_equal),
>           G_MAXUINT);
> 
> I think Gabble depends on a new enough tp-glib to use tp_value_array_build().
> Also, I think this leaks the hash table.
> 

New to that (1ec30f0).

> gabble_server_sasl_channel_select_mechanisms(): I'm unconvinced that this code
> is made clearer by converting the two GPtrArrays to GSLists before taking the
> first element of one and searching for it in the other.

I am easily convinced (94c03df).

> 
>               chosen_mechanism + 5);  // offset to remove 'SASL-' prefix.
> 
> Some people get really upset if you use C99 comments (because they really like
> grunge or something, I don't know) but more to the point, if you replace 5 with
> strlen("SASL-") then the code is self-documenting!
> 

Yay self documenting code! (94c03df).

> == Tests ==
> 

I'll get to this next.

> == A spec question, before I forget ==
> 
> How do I, as a channel handler, write a filter that only matches ServerAuth
> channels, given that the channel type is the same as (e.g.) a certificate
> channel?
> 
> I think the answer is probably, by specifying the Interfaces property in the
> filter. But currently you're not allowed 'as' properties in your filters:
> http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.Observer.html#org.freedesktop.Telepathy.Client.Observer.ObserverChannelFilter
> 
> I think the answer is, let's allow them, and define the matching to be "is a
> subset of".

Gee, I never thought of this. I guess I figured Handlers could drop channels in
a programmatic fashion. We could have an immutable property with blessed
strings like "SERVER-SASL", "SERVER-TLS", but I think it defeats the point of
our abstraction.

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