Bug 27645

And here is my review:

== C code ==

Rather than adding a get_authentication_manager() function, add another field
to GabbleConnection like the other ChannelManagers?

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.

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

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


  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 think 'server_sasl_handler'
has three states as far as the auth manager is concerned:

 • Not yet announced;
 • Announced;
 • Closed

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), and then, in gabble_authentication_manager_foreach_channel() only
apply the function to 'server_sasl_handler' if it's in the middle state.
Finally, blow away all references to 'channels'.

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.

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

    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.

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

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

  struct _GabbleServerSaslChannelPrivate
  ...
    gint auth_state;

Could this have a nicer type?

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

      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.

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.

              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!

== Tests ==

Maybe remove the 'test-sasl-' prefix from the tests' filenames? It seems
redundant.

    old_signal, new_signal = q.expect_many(
            EventPattern('dbus-signal', signal='NewChannel'),
            EventPattern('dbus-signal', signal='NewChannels'),
            )

    path, type, handle_type, handle, suppress_handler = old_signal.args

At least one test should test that the signals' arguments are actually what we
expect them to be. Do we have the mechs we expect? Is the identity as expected?
&c &c.

    assert (e.args == [cs.CONN_STATUS_DISCONNECTED,
                       cs.CSR_AUTHENTICATION_FAILED])

If you use assertEquals() from servicetest.py then you get nicer error messages
if this fails. It'd also be nice to test that Gabble doesn't die if you call
Disconnect() while it's waiting for you to authenticate.

    expected_challenge = 'None' # because of byte_arrays=True

Whu........ an empty 'ay' becomes None?! Thanks dbus-python!

  def _test_abort(q, bus, conn, stream, stage):

'stage' is always "mid". What other tests were you planning?

        assert (expected_mechanisms == [m[0] for m in avail_mechs])

I don't really understand what this line is doing to avail_mechs.

== 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".

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.

(In reply to comment #1)
> == Tests ==
> 
> Maybe remove the 'test-sasl-' prefix from the tests' filenames? It seems
> redundant.

Done (f12f045).

> 
>     old_signal, new_signal = q.expect_many(
>             EventPattern('dbus-signal', signal='NewChannel'),
>             EventPattern('dbus-signal', signal='NewChannels'),
>             )
> 
>     path, type, handle_type, handle, suppress_handler = old_signal.args
> 
> At least one test should test that the signals' arguments are actually what we
> expect them to be. Do we have the mechs we expect? Is the identity as expected?
> &c &c.
> 

At this time, only PeerIdentity is immutable, so we only check for that (c62a360).

>     assert (e.args == [cs.CONN_STATUS_DISCONNECTED,
>                        cs.CSR_AUTHENTICATION_FAILED])
> 
> If you use assertEquals() from servicetest.py then you get nicer error messages
> if this fails.

Done (b19a770).

> It'd also be nice to test that Gabble doesn't die if you call
> Disconnect() while it's waiting for you to authenticate.
> 

Added (d713282). I also added a test to make sure that Close() after authentication does not kill the connection, I am not sure if I did it correctly, how do you check that Close() does not call a StatusChanged indefinitely? How well..

>     expected_challenge = 'None' # because of byte_arrays=True
> 
> Whu........ an empty 'ay' becomes None?! Thanks dbus-python!
> 
>   def _test_abort(q, bus, conn, stream, stage):
> 
> 'stage' is always "mid". What other tests were you planning?
> 

Changed it to 'case', see commit above (d713282).

>         assert (expected_mechanisms == [m[0] for m in avail_mechs])
> 
> I don't really understand what this line is doing to avail_mechs.
> 

We are extracting the mechanism name from the mechanism details structure. Made it more readable (b5e9880).

Reassigning to Vivek for review as part of an n-way review trade.

(In reply to comment #2)
> What's with all these fancy bullets and Unicode you flaunt? You should write a
> blog post about keyboard shortcuts for useful chars.

An excellent idea! On my TODO list. :)

Two minor points in review II:

server-sasl-channel.c: typo:
    GabbleSvcChannelInterfaceAuthMechanismChooser *mechanims_chooser,
      mechanims_chooser);

(Trivial I know, but let's not bake in typos)

and from review I:

"in gabble_authentication_manager_foreach_channel() only apply the function to 'server_sasl_handler' if it's in the middle state."

Now you check for ->closed before applying the func, but is it possible the
function still gets applied in stats 0 (Not Announced) here?
(It may be that something prevents this, or that it is harmless, in which 
case fine).

Everything else looks like it's been taken care of.

Just to be clear, the proposed branch is the new one based on the Wocky GSimpleAsyncResult SASL API. It is very similar to the other branch, just with some different wocky calls.

http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=shortlog;h=refs/heads/sasl-async-result

(In reply to comment #5)
> Two minor points in review II:
> 
> server-sasl-channel.c: typo:
>     GabbleSvcChannelInterfaceAuthMechanismChooser *mechanims_chooser,
>       mechanims_chooser);
> 
> (Trivial I know, but let's not bake in typos)
> 

Fixed, thanks.

> and from review I:
> 
> "in gabble_authentication_manager_foreach_channel() only apply the function to
> 'server_sasl_handler' if it's in the middle state."
> 
> Now you check for ->closed before applying the func, but is it possible the
> function still gets applied in stats 0 (Not Announced) here?
> (It may be that something prevents this, or that it is harmless, in which 
> case fine).

The channel constructor assigns TRUE to ->closed, so the check should work also when the channel is in the unannounced stage.

> 
> Everything else looks like it's been taken care of.

In that case, passed.

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!

Oh yeah, and:

      priv->object_path = g_strdup_printf ("%s/SaslChannel_%p",
          conn->object_path, self);

There's only one of these per connection, surely, so this doesn't have to be uniquified by the address of self.

(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

(In reply to comment #10)
> Oh yeah, and:
> 
>       priv->object_path = g_strdup_printf ("%s/SaslChannel_%p",
>           conn->object_path, self);
> 
> There's only one of these per connection, surely, so this doesn't have to be
> uniquified by the address of self.

0148e1d

(In reply to comment #11)
> (In reply to comment #9)
> > 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

Won't this:

+  if (!g_simple_async_result_is_valid (G_ASYNC_RESULT (priv->result),
+          G_OBJECT (self), wocky_auth_registry_start_auth_finish))

still crash if priv->result is NULL?

(In reply to comment #13)
> Won't this:
> 
> +  if (!g_simple_async_result_is_valid (G_ASYNC_RESULT (priv->result),
> +          G_OBJECT (self), wocky_auth_registry_start_auth_finish))
> 
> still crash if priv->result is NULL?

It will raise a g_warning which I figured wouldn't be such a bad idea, but I'll add a NULL check as a lazy evaluation first.

(In reply to comment #14)
> (In reply to comment #13)
> > Won't this:
> > 
> > +  if (!g_simple_async_result_is_valid (G_ASYNC_RESULT (priv->result),
> > +          G_OBJECT (self), wocky_auth_registry_start_auth_finish))
> > 
> > still crash if priv->result is NULL?
> 
> It will raise a g_warning which I figured wouldn't be such a bad idea, but I'll
> add a NULL check as a lazy evaluation first.

4143d47

ship it!

Merged, thanks!