[Bug 27645] Implement SASL authentication channels.
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Wed Apr 14 21:40:15 CEST 2010
Will Thompson <will.thompson at collabora.co.uk> changed:
What |Removed |Added
AssignedTo|telepathy-bugs at lists.freede |eitan.isaacson at collabora.co
--- 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?
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.
/* Used to represent a set of channels.
* Keys are GabbleCertificateChannel *, values are an arbitrary non-NULL
'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;
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().
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 \
#define SERVER_SASL_CHANNEL_TYPE_CHALLENGE \
Doesn't the code generator generate GTypes for this? I could be wrong, maybe it
doesn't, but I thought it did.
Could this have a nicer type?
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 (
1, g_hash_table_new (g_str_hash, g_str_equal),
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
old_signal, new_signal = q.expect_many(
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?
assert (e.args == [cs.CONN_STATUS_DISCONNECTED,
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 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
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:
I think the answer is, let's allow them, and define the matching to be "is a
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.
More information about the telepathy-bugs