Here's a bug to track the work on adding pluggable SASL handlers.
I've reviewed Eitan's branch up to 51d581f. I've got a few patches on top of it at http://git.collabora.co.uk/?p=user/wjt/wocky.git;a=shortlog;h=refs/heads/sasl. They're just trivia; I like the shape of the API and the majority of the implementation. == Trivia == 241bd34 messes with the license header in wocky/wocky-sasl-auth.c. c28f475 adds a NULL check before calling g_free(), which is unnecessary: g_free (NULL); is a no-op. Both wocky_sasl_auth_dispose() and wocky_sasl_auth_finalize() free priv->selected_mechanism. Only the latter should do it. (Arguably, the freeing priv->mechanisms should be in _finalize() rather than _dispose() since it's freeing memory not releasing references, but...). wocky_sasl_auth_choose_handler(): the coding style checker ought to have complained that there should be a space after the cast in: + (plain_mech = g_slist_find_custom ( + priv->mechanisms, "PLAIN", (GCompareFunc)strcmp)) != NULL) But this whole loop could be replaced by a call to g_slist_remove_all()... oh no it couldn't because we need to call strcmp. Grr anaemic collections. This is just nit-picking, but in wocky_sasl_auth_authenticate_async(): /* add built-in handlers */ builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_digest_md5_new ( priv->server, priv->username, priv->password)); wocky_sasl_auth_add_handler (sasl, builtin_handler); g_object_unref (builtin_handler); builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_plain_new ( priv->username, priv->password)); wocky_sasl_auth_add_handler (sasl, builtin_handler); g_object_unref (builtin_handler); priv->state = WOCKY_SASL_AUTH_STATE_NEGOTIATING_MECHANISM; mech_node = wocky_xmpp_node_get_child_ns (features->node, "mechanisms", WOCKY_XMPP_NS_SASL_AUTH); priv->mechanisms = wocky_sasl_auth_mechanisms_to_list (mech_node); That's a whole lot of single statement paragraphs! I've got a patch in my branch that makes it (I think) easier to read. == Remarks == Wow, I've never seen g_object_disconnect() before. The signal spec arguments for that function are ... odd. :) Congratulations on removing an actual spaghetti-code goto at the end of sasl_auth_stanza_received() in ecacc9a! :) == sasl_auth_got_failure: == if (g_strcmp0 ("aborted", reason->name) == 0) { g_simple_async_result_propagate_error (priv->result, error); } else { g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE, "Authentication failed: %s", reason == NULL ? "Unknown reason" : reason->name); } Firstly, reason may be NULL by the code above, in which case we'll crash when calling g_strcmp0(). Secondly, why does the name of the node returned by the server affect whether we should take the error from priv->result or build a new one? Finally, the one caller of sasl_auth_got_failure() immediatelly calls auth_failed() which just shoves the error back into priv->result! :) Reading the code more closely, I think what's happening is: when we abort after choosing a mechanism, we wait until the server acks our aborting before we throw an error, and in the meantime stash the reason the handler gave for aborting the exchange on priv->result (since it's a convenient place to keep it kicking around). Then when we get an ack from the server, we pull the reason out again. But this isn't safe: • If we abort, but the server returns a different reason for failure, then we'll call g_simple_async_result_set_error (priv->result, ...) twice. Actually, that turns out to be okay. I'm a bit surprised! • If we don't abort, but the server erroneously claims that we did, then g_simple_async_result_propagate_error() will not in fact propagate an error in sasl_auth_got_failure() and then the g_assert (error != NULL); in sasl_auth_stanza_received() will fail and the world will end. I think it might be clearer to have gchar *abort_message in priv and do something like: if (priv->abort_message != NULL) { g_set_error_literal (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE, priv->abort_message); } else { g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE, "Authentication failed: %s", reason == NULL ? "Unknown reason" : reason->name); } Relatedly, there's a TODO in this function that I've fixed in my branch of your branch! == wocky_sasl_auth_choose_handler: == priv->handler = priv->handlers->data; wocky_sasl_handler_handle_mechanisms_advertised (priv->handlers->data, priv->mechanisms); On a first reading, I thought that this meant that only the first handler would be tried. But of course that's not the case: on_handler_selected_mechanism() tries the next one if the first gives up, etc. Maybe extract part of on_handler_selected_mechanism() into a try_next_handler() function which wocky_sasl_auth_choose_handler() calls, and/or sling in a "kick the first handler to start the chain" comment or something?
Oh, and a bunch of the SASL tests run by `make check` fail for me. I should double-check that my extra patches aren't causing that.
(In reply to comment #2) > Oh, and a bunch of the SASL tests run by `make check` fail for me. I should > double-check that my extra patches aren't causing that. I like all the changes, merged them into my branch. Indeed you introduced some fails, I fixed them with a NULL check (3b3cbff). I'll look at the rest of your comments now, and resolve them.
Sjoerd raised a question in real life: rather than having the authenticators 'return' from methods called on them by emitting signals, why not make the methods be GIO-style async methods?
I finally pushed my changes, after 18 hours of flight and no connectivity. (In reply to comment #1) > I've reviewed Eitan's branch up to 51d581f. I've got a few patches on top of it > at > http://git.collabora.co.uk/?p=user/wjt/wocky.git;a=shortlog;h=refs/heads/sasl. > They're just trivia; I like the shape of the API and the majority of the > implementation. > > == Trivia == > > 241bd34 messes with the license header in wocky/wocky-sasl-auth.c. > Fixed (b81e992) > c28f475 adds a NULL check before calling g_free(), which is unnecessary: g_free > (NULL); is a no-op. > Fixed (744871f) > Both wocky_sasl_auth_dispose() and wocky_sasl_auth_finalize() free > priv->selected_mechanism. Only the latter should do it. (Arguably, the freeing > priv->mechanisms should be in _finalize() rather than _dispose() since it's > freeing memory not releasing references, but...). > Fixed (24f3b6d) > wocky_sasl_auth_choose_handler(): the coding style checker ought to have > complained that there should be a space after the cast in: > > + (plain_mech = g_slist_find_custom ( > + priv->mechanisms, "PLAIN", (GCompareFunc)strcmp)) != NULL) > > But this whole loop could be replaced by a call to g_slist_remove_all()... oh > no it couldn't because we need to call strcmp. Grr anaemic collections. > Fixed (b280fe1) > This is just nit-picking, but in wocky_sasl_auth_authenticate_async(): > > /* add built-in handlers */ > > builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_digest_md5_new ( > priv->server, priv->username, priv->password)); > > wocky_sasl_auth_add_handler (sasl, builtin_handler); > > g_object_unref (builtin_handler); > > builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_plain_new ( > priv->username, priv->password)); > > wocky_sasl_auth_add_handler (sasl, builtin_handler); > > g_object_unref (builtin_handler); > > priv->state = WOCKY_SASL_AUTH_STATE_NEGOTIATING_MECHANISM; > > mech_node = wocky_xmpp_node_get_child_ns (features->node, "mechanisms", > WOCKY_XMPP_NS_SASL_AUTH); > > priv->mechanisms = wocky_sasl_auth_mechanisms_to_list (mech_node); > > That's a whole lot of single statement paragraphs! I've got a patch in my > branch that makes it (I think) easier to read. Thanks for doing that! > > == Remarks == > > Wow, I've never seen g_object_disconnect() before. The signal spec arguments > for that > function are ... odd. :) > > Congratulations on removing an actual spaghetti-code goto at the end of > sasl_auth_stanza_received() in ecacc9a! :) > > == sasl_auth_got_failure: == > > if (g_strcmp0 ("aborted", reason->name) == 0) > { > g_simple_async_result_propagate_error (priv->result, error); > } > else > { > g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE, > "Authentication failed: %s", > reason == NULL ? "Unknown reason" : reason->name); > } > > Firstly, reason may be NULL by the code above, in which case we'll crash when > calling g_strcmp0(). Secondly, why does the name of the node returned by the > server affect whether we should take the error from priv->result or build a new > one? Finally, the one caller of sasl_auth_got_failure() immediatelly calls > auth_failed() which just shoves the error back into priv->result! :) > > Reading the code more closely, I think what's happening is: when we abort after > choosing a mechanism, we wait until the server acks our aborting before we > throw an error, and in the meantime stash the reason the handler gave for > aborting the exchange on priv->result (since it's a convenient place to keep it > kicking around). Then when we get an ack from the server, we pull the reason > out again. > > But this isn't safe: > > • If we abort, but the server returns a different reason for failure, then > we'll call g_simple_async_result_set_error (priv->result, ...) twice. > Actually, that turns out to be okay. I'm a bit surprised! > • If we don't abort, but the server erroneously claims that we did, then > g_simple_async_result_propagate_error() will not in fact propagate an error > in sasl_auth_got_failure() and then the > g_assert (error != NULL); > in sasl_auth_stanza_received() will fail and the world will end. > > I think it might be clearer to have gchar *abort_message in priv and do > something like: > > if (priv->abort_message != NULL) > { > g_set_error_literal (error, WOCKY_SASL_AUTH_ERROR, > WOCKY_SASL_AUTH_ERROR_FAILURE, priv->abort_message); > } > else > { > g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE, > "Authentication failed: %s", > reason == NULL ? "Unknown reason" : reason->name); > } > Good idea. Instead of a string, I used a GError, so a user could specify a code in addition to a message (2b05d82). > Relatedly, there's a TODO in this function that I've fixed in my branch of your > branch! > > == wocky_sasl_auth_choose_handler: == > > priv->handler = priv->handlers->data; > > wocky_sasl_handler_handle_mechanisms_advertised (priv->handlers->data, > priv->mechanisms); > > On a first reading, I thought that this meant that only the first handler would > be tried. But of course that's not the case: on_handler_selected_mechanism() > tries the next one if the first gives up, etc. Maybe extract part of > on_handler_selected_mechanism() into a try_next_handler() function which > wocky_sasl_auth_choose_handler() calls, and/or sling in a "kick the first > handler to start the chain" comment or something? I think I made it more readable (695ce71).
(In reply to comment #4) > Sjoerd raised a question in real life: rather than having the authenticators > 'return' from methods called on them by emitting signals, why not make the > methods be GIO-style async methods? I did that at one point, and then I abandoned it. I can't recall why... It's not a bad idea at all. Maybe I should spend a few hours on that soon.
(In reply to comment #6) > (In reply to comment #4) > > Sjoerd raised a question in real life: rather than having the authenticators > > 'return' from methods called on them by emitting signals, why not make the > > methods be GIO-style async methods? > > I did that at one point, and then I abandoned it. I can't recall why... It's > not a bad idea at all. Maybe I should spend a few hours on that soon. Ok, there are branches named "sasl-async-result" both for wocky and telepathy-gabble. If they look decent, we could go with those instead.
(In reply to comment #5) > > c28f475 adds a NULL check before calling g_free(), which is unnecessary: g_free > > (NULL); is a no-op. > > > > Fixed (744871f) I added a further fix along these lines (6ee9e51). The rest looks good; I'll take a look at the async result version now...
A few words on the async result version of the branch: on_handler_responded shouldn't be assuming the result it's given is a SimpleAsyncResult. Instead, wocky_sasl_handler_handle_challenge and handle_success should have an _async suffix, and have corresponding _finish() functions (rather than the handler calling wocky_sasl_handler_task_finished() in either case) which returns the response. I fear that, if the WockySaslAuth dies before the call to wocky_sasl_handler_handle_challenge() finishes, we'll die when the handler calls the callback.
(In reply to comment #9) > A few words on the async result version of the branch: > > on_handler_responded shouldn't be assuming the result it's given is a > SimpleAsyncResult. Instead, wocky_sasl_handler_handle_challenge and > handle_success should have an _async suffix, and have corresponding _finish() > functions (rather than the handler calling wocky_sasl_handler_task_finished() > in either case) which returns the response. > > I fear that, if the WockySaslAuth dies before the call to > wocky_sasl_handler_handle_challenge() finishes, we'll die when the handler > calls the callback. Obviously I didn't understand how to use GSimpleAsyncResult correctly. After reading this comment a few times, and after some experimentation, I think I got something a bit more sane, it's pushed.
The design looks hackish to me in some places unfortunately :( * having to dig out the sasl-auth-obj from the connector seems quite bad, just add a function on the connector to add one. * needing to dig out the jid of the connector, then splitting it yourself and passing it onto your handler doesn't seem great either. Better to the SaslAuth pass that information to the SaslHandler (given that most if not all need it). * You removed the initial_response_func for no apparent reason, i'm not sure why. It makes things harder as we can't have some nice fallback logic (which is the reason i added that method in the first place).. As a result WockySaslPlain doesn't handle the case where the server sends it a challenge instead of success anymore (yes it will never happen, but shows the issue nicely)... Also it means that you have a success argument that can only ever be useful for mechanisms only doing an initial response and they all need to implement it in the same silly way ? * Given that we're moving to interactive authentication, you should tell the SaslHandler whether the result it sends will go over the wire in plain-text or not and/or if wocky thinks plain text is acceptable. * You removed the ->plain boolean from the Handler struct and instead remove the PLAIN mechanism from what the server claims its mechanims are.. There are more non-secure SASL mechanisms though, which should be considered plain-text. Having the filtering be in SaslAuth seems wrong. In the old case SaslHandlers would just tell when they were plain-text (the naming might have been confusing.) * There is no way to differentiate between a handler not being able to handle any of the mechanisms or it marking all the mechanisms as inadequate. I'm assuming it would be useful for the primary handler to be able to completely abort the authentication * This setup doesn't work at all for old-style jabber authentication. We should probably split out the old-style jabber stuff into something akin to SaslAuth and have it do things for you. We can fake the old-style stuff as being SASL to the helpers so we can use the same interface. * When we're splitting out anyway, we should probably split out the handler picking process into it's own object (which could even look like a SaslHandler to the other objects just for extra fun and profit. This would also mean your gabble itneractive handler could use such an object (or maybe derive from it for even more fun) to implement just having a password given to it from the user and then needing to do the actual sassling itself.. Actually i've just thought of an even more cunning scheme, but it's time to go home first :) (ping me on xmpp/irc later?) code-wise i've got several nitpicks that i won't go into now, but the main issue i saw is that you're doing _async_result_complete from code not called from the mainloop, which violates how GAsync stuff is supposed to work ;)
So i pondered a bit on how this should work, taking into account that we want things to just work when using old-style jabber auth and also make it possible to plug something in which may just get your password asynchronously, but then use wocky's built-in handlers to do the actual sasling. So what we actually need is not a a pluggable handler per se, but it's a pluggable handler selector/information gatherer :).. In the gabble case the actual handler can be either be a remote process over dbus or one of the local handlers if all the authentication handler gave us was the password to use.. So my altered wocky design would be something like this: WockyConnector -> Wocky{Sasl,Jabber}Auth -> WockyAuthRegistry -*-> WockySaslHandler * WockyConnector is the current connector we all love and/or hate :) * WockySaslAuth is also the same as currently, but with some functionality cut out * WockyJabberAuth does the same job as WockySaslAuth but for old-school authentication, this functionality needs to be cut out of the Connector * WockyAuthRegistry is a bad name for a new magical objec that does the complicated work * WockySaslHandler are the current sasl handlers as they are in master, they won't need a change. Now the task of WockyAuthRegistry is to find the right handler if possible and proxy the challenges back and forward. When creating the WockyConnector object you can optionally set your own Registry, otherwise the default will be used. The register should be passed to Wocky{Sasl,Jabber}Auth at construct time. The async API of the Registry would be vaguely similar to what you changed the WockySaslHandlers to atm. Although a tiny bit more streamlined (in my opinion and feel free to change the names): _start_authentication (<list of availble mechanisms, allow-plain-text, is-secure-channel, user_name, maybe password, servername) -> (mechanism to use, initial response if any) or error. * Registery picks the authentication mechanism based on the given parameters in whatever way, the builtin one would use the same algorithm to pick one and then returns a value. While the Gabble one would ask over dbus what the mechanism should be and gets an intial response. _challenge (<challenge>) -> (maybe <response>) or error * Nothing exiting the auth mechanism provides a challenge, the registry passes it on to what ever the bit handling it is and gives back the result. _success () -> maybe error * Server claimed the authentication is finished, we let the handler check if it agrees. _failure () -> nothing * Server claimed authentication has failed for whatever reason. The default registry shouldn't need an implementation for this, but in the gabble case we probably want to relay this to the tp handler. In case we're doing old-style jabber auth, we can define "X-WOCKY-JABBER-AUTH" or "X-WOCKY-JABBER-DIGEST". and pass that to the registry instead. Which means we don't have to have seperate registry implementation for old and new style auth (win!) In case we want to support restarting we could add an extra async method _restart? () -> bool, which if returning TRUE would start the whole process from the beginning again. The default registry would always just return FALSE here. Now if you're extra cunning you make it such that if the Tp authentication handler just gives you back a password and doesn't actually want to do sasl, you just set the password propery on yourself/your baseclass and then from then onwards proxy the _start/_challenge/_success onwards to the base class so you use exactly the same code as if you were given a password (you might even automagically want to do this if authorization is started with a password..) Sorry for not getting into a design discussion with you earlier as you already did a big chunk of work, but i think this design is both much simpler, more elegant and address more of the use-cases we actually want to address..
Created attachment 35387 [details] [review] First implementation of the sasl registry taken from eitans sasl-once-again branch
Review of attachment 35387 [details] [review]: 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
(In reply to comment #14) > Review of attachment 35387 [details] [review]: > > 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.
Added the Jabber auth stuff to the new world order, the branch is at: http://git.collabora.co.uk/?p=user/eitan/wocky.git;a=shortlog;h=refs/heads/jabber-auth-refactor
(In reply to comment #16) > Added the Jabber auth stuff to the new world order, the branch is at: > > http://git.collabora.co.uk/?p=user/eitan/wocky.git;a=shortlog;h=refs/heads/jabber-auth-refactor wocky-jabber-auth-digest: auth digest isn't a plain method strictly One might argue it's not a very secure one as the server controls one part of your hashing and it's only a single round so easy enough to brute-force.. Maybe we should leave it as non-plain.. wocky-jabber-auth.c: You use "X-WOCKY-JABBER-{PASSWORD,DIGEST} in some places, having a #define would be nice (typo's are too easy to make) iff --git a/wocky/wocky-node.c b/wocky/wocky-node.c index 9658704..9086443 100644 @@ -1216,7 +1216,6 @@ wocky_node_add_build_va (WockyNode *node, va_list ap) { gchar *txt = va_arg (ap, gchar *); - g_assert (txt != NULL); wocky_node_set_content (stack->data, txt); } break; that should really be in a seperate commit as it's a quite crucial change The copyright header for wocky-jabber-auth seems wrong? I'm pretty sure you copied some code from wocky-connector into it ? wocky-jabber-auth stream error parsing loop should probably do wocky_node_iter instead of a for loop of the children
Did some rebasing.. (In reply to comment #17) > (In reply to comment #16) > > Added the Jabber auth stuff to the new world order, the branch is at: > > > > http://git.collabora.co.uk/?p=user/eitan/wocky.git;a=shortlog;h=refs/heads/jabber-auth-refactor > > wocky-jabber-auth-digest: auth digest isn't a plain method strictly > One might argue it's not a very secure one as the server controls one part of > your hashing and it's only a single round so easy enough to brute-force.. Maybe > we should leave it as non-plain.. Oops, b09e8de. > > wocky-jabber-auth.c: > You use "X-WOCKY-JABBER-{PASSWORD,DIGEST} in some places, having a #define > would be > nice (typo's are too easy to make) > c9feb1f > > iff --git a/wocky/wocky-node.c b/wocky/wocky-node.c > index 9658704..9086443 100644 > @@ -1216,7 +1216,6 @@ wocky_node_add_build_va (WockyNode *node, va_list ap) > { > gchar *txt = va_arg (ap, gchar *); > > - g_assert (txt != NULL); > wocky_node_set_content (stack->data, txt); > } > break; > > that should really be in a seperate commit as it's a quite crucial change > a1e4e66 > > The copyright header for wocky-jabber-auth seems wrong? I'm pretty sure you > copied some code from wocky-connector into it ? > ammended in to main patch, d3a5493 > wocky-jabber-auth stream error parsing loop should probably do wocky_node_iter > instead of a for loop of the children this is existing code from wocky-sasl-auth.c, i cleaned up both: 0339ffd
(In reply to comment #18) > Did some rebasing.. > > (In reply to comment #17) > > (In reply to comment #16) > > > Added the Jabber auth stuff to the new world order, the branch is at: > > > > > > http://git.collabora.co.uk/?p=user/eitan/wocky.git;a=shortlog;h=refs/heads/jabber-auth-refactor > > > > wocky-jabber-auth-digest: auth digest isn't a plain method strictly > > One might argue it's not a very secure one as the server controls one part of > > your hashing and it's only a single round so easy enough to brute-force.. Maybe > > we should leave it as non-plain.. > > Oops, b09e8de. cool > > > > wocky-jabber-auth.c: > > You use "X-WOCKY-JABBER-{PASSWORD,DIGEST} in some places, having a #define > > would be > > nice (typo's are too easy to make) > > > > c9feb1f looks good > > iff --git a/wocky/wocky-node.c b/wocky/wocky-node.c > > index 9658704..9086443 100644 > > @@ -1216,7 +1216,6 @@ wocky_node_add_build_va (WockyNode *node, va_list ap) > > { > > gchar *txt = va_arg (ap, gchar *); > > > > - g_assert (txt != NULL); > > wocky_node_set_content (stack->data, txt); > > } > > break; > > > > that should really be in a seperate commit as it's a quite crucial change > > > > a1e4e66 thanks > > The copyright header for wocky-jabber-auth seems wrong? I'm pretty sure you > > copied some code from wocky-connector into it ? > > > > ammended in to main patch, d3a5493 > > > wocky-jabber-auth stream error parsing loop should probably do wocky_node_iter > > instead of a for loop of the children > > this is existing code from wocky-sasl-auth.c, i cleaned up both: 0339ffd Cool, if i'm not mistaken it's exactly the same code in both? Throw it in a utilty function maybe ? (maybe in wocky-xmpp-error.c ? to compliment the more modern wocky_xmpp_stream_error_from_node)
(In reply to comment #19) > > > wocky-jabber-auth stream error parsing loop should probably do wocky_node_iter > > > instead of a for loop of the children > > > > this is existing code from wocky-sasl-auth.c, i cleaned up both: 0339ffd > > Cool, if i'm not mistaken it's exactly the same code in both? Throw it in a > utilty function maybe ? (maybe in wocky-xmpp-error.c ? to compliment the more > modern wocky_xmpp_stream_error_from_node) There already is a wocky_xmpp_stream_error_from_node function. Looks like we saved 22 likes of code! dfbce16
New day, new branch! This one factors out the core functionality of the registry into private virtual methods that on can override to offer different asynchronous registry/handler interaction as we will need in Gabble.
Alright, This branch deviated from it's name, but it is basically a set of changes that is required for pluggable SASL as done in Gabble. Any chance of review soonish?
(In reply to comment #22) > Alright, > > This branch deviated from it's name, but it is basically a set of changes that > is required for pluggable SASL as done in Gabble. Any chance of review soonish? Sure :) b1283b8e6bd5703f378b65f216e4ddde9e076a5e This patch actually does 3 things (add virtual functions, makes handler and their list public and adds success functions). Would have been nice to split these up in seperate patches for easier review. Anyway, i'm unsure why you made the handler(s) public? rationale there would be good.. For the virtualizing stuff, it's customary to let the actual implemention allocate the GAsyncResult as it can choose not to use a GSimpleAsyncResult. Also it should be possible to also override the finish method to not force the Result to safe the data in a certain way. I'm ok with having the default one do something sane, but in that case it will be an API guarantee and should be properly documented For the success function, should that be async ? If so the finish function seems missing and also there is no definition of a finish method or a definition of _success_async in the header 61d7de141597d74bd003e959b3d23fb33983b848 Might be nice to document that wocky_auth_registry_start_data_new takes ownership of the initial_response so you don't have to do one extra copy. c0bcb52118db08eb76752b2a48df6a786f19e3a4 && cfeb9702d8695fa99469b01ad36081150e83870f look fine :) 91690551e627ede09c673ce6405e4f5284ad1b71 looks ok, might be nice to document which error domains and also how to distinguish between fatal errors and temporary errors (as in did the server say failed or did it close the stream)
(In reply to comment #23) > (In reply to comment #22) > > Alright, > > > > This branch deviated from it's name, but it is basically a set of changes that > > is required for pluggable SASL as done in Gabble. Any chance of review soonish? > > Sure :) > > b1283b8e6bd5703f378b65f216e4ddde9e076a5e > > This patch actually does 3 things (add virtual functions, makes handler and > their list public and adds success functions). Would have been nice to split > these up in seperate patches for easier review. > > Anyway, i'm unsure why you made the handler(s) public? rationale there would be > good.. > We discussed this. I was getting ahead of myself, reverted it. > For the virtualizing stuff, it's customary to let the actual implemention > allocate the GAsyncResult as it can choose not to use a GSimpleAsyncResult. > Also it should be possible to also override the finish method to not force the > Result to safe the data in a certain way. I'm ok with having the default one do > something sane, but in that case it will be an API guarantee and should be > properly documented Made the functions public, and added public virtual _finish functions (5765294 & 2361184). Added docs for the default behavior (f15f039). > > For the success function, should that be async ? If so the finish function > seems missing and also there is no definition of a finish method or a > definition of _success_async in the header I didn't add success functions in this commit, it was part of the original API that is already in master, and everything is in the header file, etc. > > 61d7de141597d74bd003e959b3d23fb33983b848 > > Might be nice to document that wocky_auth_registry_start_data_new takes > ownership of the initial_response so you don't have to do one extra copy. > I looked this over a few times, we don't take ownership, but copy. Are you saying we _should_ do this and save one copy op? Seems like it would be confusing and inconsistent, especially since the mechanism arg is copied. > c0bcb52118db08eb76752b2a48df6a786f19e3a4 && > cfeb9702d8695fa99469b01ad36081150e83870f > > look fine :) > > 91690551e627ede09c673ce6405e4f5284ad1b71 > > looks ok, might be nice to document which error domains and also how to > distinguish between fatal errors and temporary errors (as in did the server say > failed or did it close the stream) I moved it to auth_failed from _authenticate_finish. As you said there is no guarantee _finish is called. The domain is always WOCKY_AUTH_ERROR as of now. Not sure myself how to distinguish between fatal and temporary errors at this point since the connection is always aborted afaik. Am I wrong? Might be a future TODO to have multiple auth attempts, but from what I managed to test in current XMPP servers, I didn't see any that supported multiple attempts, but I may be wrong about that too.
(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > Alright, > > > > > > This branch deviated from it's name, but it is basically a set of changes that > > > is required for pluggable SASL as done in Gabble. Any chance of review soonish? > > > > Sure :) > > > > b1283b8e6bd5703f378b65f216e4ddde9e076a5e > > > > This patch actually does 3 things (add virtual functions, makes handler and > > their list public and adds success functions). Would have been nice to split > > these up in seperate patches for easier review. > > > > Anyway, i'm unsure why you made the handler(s) public? rationale there would be > > good.. > > > > We discussed this. I was getting ahead of myself, reverted it. Cool, you can probably squash the patch for that back into the earlier patch and it'll be as if it never happened :p > > For the virtualizing stuff, it's customary to let the actual implemention > > allocate the GAsyncResult as it can choose not to use a GSimpleAsyncResult. > > Also it should be possible to also override the finish method to not force the > > Result to safe the data in a certain way. I'm ok with having the default one do > > something sane, but in that case it will be an API guarantee and should be > > properly documented > > Made the functions public, and added public virtual _finish functions > (5765294 & 2361184). I'm unsure what you mean with public here, but the code looks good :) > Added docs for the default behavior (f15f039). + * Recives a challenge and asynchronously provides a reply. By default the typo :) Does documenting it like this make it end up in gtk-doc btw? That's not a merge blocker for me, but would be good to make sure > > For the success function, should that be async ? If so the finish function > > seems missing and also there is no definition of a finish method or a > > definition of _success_async in the header > > I didn't add success functions in this commit, it was part of the original API > that is already in master, and everything is in the header file, etc. > > > > > 61d7de141597d74bd003e959b3d23fb33983b848 > > > > Might be nice to document that wocky_auth_registry_start_data_new takes > > ownership of the initial_response so you don't have to do one extra copy. > > I looked this over a few times, we don't take ownership, but copy. Are you > saying we _should_ do this and save one copy op? Seems like it would be > confusing and inconsistent, especially since the mechanism arg is copied. Yeah, we might want to take ownership as that's probably what most code wants. Just an idea though. In future version of glib GString will hopefully be reference counted. > > c0bcb52118db08eb76752b2a48df6a786f19e3a4 && > > cfeb9702d8695fa99469b01ad36081150e83870f > > > > look fine :) > > > > 91690551e627ede09c673ce6405e4f5284ad1b71 > > > > looks ok, might be nice to document which error domains and also how to > > distinguish between fatal errors and temporary errors (as in did the server say > > failed or did it close the stream) > > I moved it to auth_failed from _authenticate_finish. As you said there is no > guarantee _finish is called. The domain is always WOCKY_AUTH_ERROR as of now. > Not sure myself how to distinguish between fatal and temporary errors at this > point since the connection is always aborted afaik. Am I wrong? Might be a > future TODO to have multiple auth attempts, but from what I managed to test in > current XMPP servers, I didn't see any that supported multiple attempts, but I > may be wrong about that too. Supporting just one try is fine for now. The XMPP spec says that implementation should allow a reasonable number of retries but it's entirely possible that no XMPP server actually allows more then one try.. Considering all errors as being fatal works for me :)
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > (In reply to comment #22) > > > > Alright, > > > > > > > > This branch deviated from it's name, but it is basically a set of changes that > > > > is required for pluggable SASL as done in Gabble. Any chance of review soonish? > > > > > > Sure :) > > > > > > b1283b8e6bd5703f378b65f216e4ddde9e076a5e > > > > > > This patch actually does 3 things (add virtual functions, makes handler and > > > their list public and adds success functions). Would have been nice to split > > > these up in seperate patches for easier review. > > > > > > Anyway, i'm unsure why you made the handler(s) public? rationale there would be > > > good.. > > > > > > > We discussed this. I was getting ahead of myself, reverted it. > > Cool, you can probably squash the patch for that back into the earlier patch > and it'll be as if it never happened :p > Squashed. > > > For the virtualizing stuff, it's customary to let the actual implemention > > > allocate the GAsyncResult as it can choose not to use a GSimpleAsyncResult. > > > Also it should be possible to also override the finish method to not force the > > > Result to safe the data in a certain way. I'm ok with having the default one do > > > something sane, but in that case it will be an API guarantee and should be > > > properly documented > > > > Made the functions public, and added public virtual _finish functions > > (5765294 & 2361184). > > I'm unsure what you mean with public here, but the code looks good :) > > > Added docs for the default behavior (f15f039). > > + * Recives a challenge and asynchronously provides a reply. By default the > > typo :) Does documenting it like this make it end up in gtk-doc btw? That's not > a merge blocker for me, but would be good to make sure > Amended typo fix and made all the changes appear in gtk-doc. 33446fe987968c368e15eeb4726f597909933b1a > > > For the success function, should that be async ? If so the finish function > > > seems missing and also there is no definition of a finish method or a > > > definition of _success_async in the header > > > > I didn't add success functions in this commit, it was part of the original API > > that is already in master, and everything is in the header file, etc. > > > > > > > > 61d7de141597d74bd003e959b3d23fb33983b848 > > > > > > Might be nice to document that wocky_auth_registry_start_data_new takes > > > ownership of the initial_response so you don't have to do one extra copy. > > > > I looked this over a few times, we don't take ownership, but copy. Are you > > saying we _should_ do this and save one copy op? Seems like it would be > > confusing and inconsistent, especially since the mechanism arg is copied. > > Yeah, we might want to take ownership as that's probably what most code wants. > Just an idea though. In future version of glib GString will hopefully be > reference > counted. > If reference counting is coming, and since this is a public API, I'll just keep it as it is since ref counting will let us keep a simple API and not make an extra copy, this happens maybe 3 times a session, not a real massive perf thing. > > > c0bcb52118db08eb76752b2a48df6a786f19e3a4 && > > > cfeb9702d8695fa99469b01ad36081150e83870f > > > > > > look fine :) > > > > > > 91690551e627ede09c673ce6405e4f5284ad1b71 > > > > > > looks ok, might be nice to document which error domains and also how to > > > distinguish between fatal errors and temporary errors (as in did the server say > > > failed or did it close the stream) > > > > I moved it to auth_failed from _authenticate_finish. As you said there is no > > guarantee _finish is called. The domain is always WOCKY_AUTH_ERROR as of now. > > Not sure myself how to distinguish between fatal and temporary errors at this > > point since the connection is always aborted afaik. Am I wrong? Might be a > > future TODO to have multiple auth attempts, but from what I managed to test in > > current XMPP servers, I didn't see any that supported multiple attempts, but I > > may be wrong about that too. > > Supporting just one try is fine for now. The XMPP spec says that implementation > should allow a reasonable number of retries but it's entirely possible that no > XMPP server actually allows more then one try.. Considering all errors as being > fatal works for me :) Great!
Three words: MERGE MERGE MERGE :)
Eitan merged this
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.