[Bug 27622] Pluggable SASL authentication

Mon May 3 17:10:19 CEST 2010

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

--- Comment #14 from Sjoerd Simons <sjoerd at luon.net> 2010-05-03 08:10:19 PDT ---
Review of attachment 35387:
 --> (https://bugs.freedesktop.org/review?bug=27622&attachment=35387)

Looking good, various nitpicks as usual though :p

* first nitpick: your commit message isn't <subject>\n\n<body>, which confuses
giggle

* 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

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

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

::: 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

@@ +240,3 @@
+  GString *response_data;
+  g_assert (initial_data != NULL);
+  g_assert (mechanism != NULL);

nitpick, no newline between code and definitions

@@ +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 ?)

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

::: 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

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