[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
----------------------------------------------------------------------------
         AssignedTo|telepathy-bugs at lists.freede |eitan.isaacson at collabora.co
                   |sktop.org                   |.uk

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2010-04-14 12:40:14 PDT ---
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".

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