[Bug 27622] Pluggable SASL authentication

Mon May 3 21:29:31 CEST 2010

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

--- Comment #15 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-05-03 12:29:30 PDT ---
(In reply to comment #14)
> Review of attachment 35387 [details]:
> 
> Looking good, various nitpicks as usual though :p
> 
> * first nitpick: your commit message isn't <subject>\n\n<body>, which confuses
> giggle

Amended.

> 
> * make check shows the following errors:
> 
> ./wocky-auth-registry.c:212:      GError *error;
> ./wocky-auth-registry.c:274:  GError *error;
> ./wocky-auth-registry.c:330:  GError *error;
> ^^^ The above files contain uninitialized GError*s - they should be
>     initialized to NULL
> 
> those should indeed be fixed otherwise GError will bite us
> 

88e2fb8

> ::: examples/connect.c
> @@ +88,3 @@
> +  sasl = wocky_sasl_auth_new (server, username, password, conn,
> auth_registry);
> +
> +  g_object_unref (auth_registry);
> 
> any reason to instantiate our own auth registry here? it could just use the
> default i guess?

Yah, I have been pondering the lifecycle of these objects and references to
them. So I added another argument to the *_new functions of both the connector
and the auth registry. If NULL is past they will use the default registry. It's
needed in both because both classes are potential entry points as the tests and
the connect.c example demonstrate.

c4305a4

> 
> ::: tests/wocky-test-sasl-auth.c
> @@ +149,3 @@
>    g_assert (sasl == NULL);
> +
> +  auth_registry = wocky_auth_registry_new ();
> 
> any reason to make our own and not use the default one ?
> 

Fixed in same commit (c4305a4).

> ::: wocky/wocky-auth-registry.c
> @@ +1,1 @@
> +/* wocky-auth-registry.c */
> 
> general: most of the finish methods could be implemented by just using wjts
> crazy macros defined in wocky-utils.h

974805f

> 
> @@ +240,3 @@
> +  GString *response_data;
> +  g_assert (initial_data != NULL);
> +  g_assert (mechanism != NULL);
> 
> nitpick, no newline between code and definitions

Redid that code to follow the same flow as the macros, even though we aren't
using the macro here (974805f).

> 
> @@ +362,3 @@
> +    GError *error)
> +{
> +}
> 
> maybe leave this out of the API untill we actually use it ? (I wonder if this
> should be an async method where the result can indicate we should restart the
> authentication procedure ?)

f13db09

> 
> ::: wocky/wocky-connector.c
> @@ +509,3 @@
> +      case PROP_AUTH_REGISTRY:
> +        g_object_unref (priv->auth_registry);
> +        priv->auth_registry = g_value_dup_object (value);
> 
> This will go wrong when it's set and auth_registry is still NULL.. Also the
> property should probably be CONSTRUCT_ONLY as changing it while authenticating
> seems scary. Is there a reason why one would like to set it after construction
> ?
> 

Yup, it's a CONSTRUCTONLY param now (c4305a4).

> ::: wocky/wocky-sasl-auth.c
> @@ +469,3 @@
> +  response_stanza = wocky_stanza_new ("response", WOCKY_XMPP_NS_SASL_AUTH);
> +  wocky_node_set_content (wocky_stanza_get_top_node (response_stanza),
> +      response);
> 
> this can be done in one go by:
> wocky_stanza_build (WOCKY_STANZA_TYPE_RESPONSE, WOCKY_STANZA_SUB_TYPE_NONE,
> NULL, NULL, '$', response, NULL));
> 
> unclear if it's a net win though

Seems like wocky_stanza_build asserts against the node text being NULL
(wocky-node.c:1219). Don't know why that is there, but it's not a net win
because we can't guarantee that it is not NULL. Not comfortable removing the
g_assert() in wocky_stanza_build() since I don't know how else it's used.

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