Comments ont he branch:

+      if (tests[i].result.mech == "DIGEST-MD5")

I doubt that works in C :)

4ba6a316ac5ecfa6b2f408bc7aba6500e2e38a66:
+  if (0 == strcmp (priv->mech, "PLAIN"))
  we have wocky_strdiff, similar to tp_strdiff (this wasn't used in another part ofthe code as it came from gibber from before there was a _strdiff there)

Also i don't like the hardcoding of the array indexes in the code just below that.. 

-  for (i = 0 ; handlers[priv->mech][i].name != NULL; i++)
+  for (i = 0 ; handler[i].name != NULL; i++ )

but handler isn't an array? This only seems to work because handler is already set to the right index in the array, in the if {} else {} above... But just removing that block and reverting this one would still make the code do the right thing..

Probably would be nice to #define the standard mech strings somewhere, i'm worried about making silly typos when hardcoding strings.

b65faa10d8de11feb00c4bf2196f76c7f7e2bf90:
Cool refactoring work. I'm always slightly in favour of having function return FALSE when they fail instead of relying on error being non-null. But that's just a nitpick

87be0be5449ab592db1688c2034bab6f8431de14:
I'd prefer it to keep the state explicit, maybe have the success handlers change the state direclty and have stanza_received call _succeeded when the state equals success..  Seems more clear to me. Although otoh as this is always for xmpp auth, maybe adding a comment in the code to say that if we successfully handled the success stanza the authentication succeeded would be enough.


1634e77b712036d82f3d0080b466cc93dd2b5523:
  _dispose in sasl auth doesn't set priv->handler to NULL after freeing it

561db1c2b0d1981b38aba6a65a9a543eb5e48ee1
my poor mind has a hard time comprehending 
 + if ((response == NULL) == (error == NULL))

why not just do:
  if (response != NULL && error != NULL)
    {
       handler fucked up
     }
  else if (response == NULL && error == NULL)
    {
        g_warning (weirdness?);

    } 

71b215e0b41f78efef67f35160fb8e256ac8b2ad:
In dispose of wocky_sasl_digest_md5 non of the members are ever set to NULL

78885334038f201e9a4dbaf1bd04d004fd35b32a:
If you'd give handlers a bool indicating whether they are plain or not, then you could generalize the handler selection slightly more so you don't need to special case the DIGEST-MD5 and PLAIN handlers mechanisms.

For the future i wonder if we should have some way to prioritize handlers (as in, prefer 
CRAM-MD5 over DIGEST-MD5 like  RFC 3920bis suggests and prefer sso tokens over everything else?)


Note that some of the comments in this bug are for code that was removed by later branch, but my brain died, so i didn't bother filtering those.. sorry about that

(In reply to comment #1)
> Comments ont he branch:
> 
> +      if (tests[i].result.mech == "DIGEST-MD5")
> 
> I doubt that works in C :)

This code is gone.

> 4ba6a316ac5ecfa6b2f408bc7aba6500e2e38a66:
> +  if (0 == strcmp (priv->mech, "PLAIN"))
>   we have wocky_strdiff, similar to tp_strdiff (this wasn't used in another
> part ofthe code as it came from gibber from before there was a _strdiff there)

Right, I'll use wocky_strdiff. (I think I would prefer a wocky_streq, but I realise that it was inherited from Gabble.)
 
> Also i don't like the hardcoding of the array indexes in the code just below
> that.. 
> 
> -  for (i = 0 ; handlers[priv->mech][i].name != NULL; i++)
> +  for (i = 0 ; handler[i].name != NULL; i++ )
> 
> but handler isn't an array? This only seems to work because handler is already
> set to the right index in the array, in the if {} else {} above... But just
> removing that block and reverting this one would still make the code do the
> right thing..

The point was that priv->mech was no longer an enum, so could no longer index the handlers array. But this code was all temporary refactoring too and is now gone.

> Probably would be nice to #define the standard mech strings somewhere, i'm
> worried about making silly typos when hardcoding strings.

You mean #define WOCKY_SASL_MECHANISM_DIGEST_MD5 "DIGEST-MD5"?

I think any typos should be caught by the tests.

> b65faa10d8de11feb00c4bf2196f76c7f7e2bf90:
> Cool refactoring work. I'm always slightly in favour of having function return
> FALSE when they fail instead of relying on error being non-null. But that's
> just a nitpick
> 
> 87be0be5449ab592db1688c2034bab6f8431de14:
> I'd prefer it to keep the state explicit, maybe have the success handlers
> change the state direclty and have stanza_received call _succeeded when the
> state equals success..  Seems more clear to me. Although otoh as this is always
> for xmpp auth, maybe adding a comment in the code to say that if we
> successfully handled the success stanza the authentication succeeded would be
> enough.

Yeah, I guess I should document the WockySaslHandler interface. The idea is that handlers keep internal state and return GErrors if the wrong thing happens in the wrong state.

I could make the built-in PLAIN and DIGEST-MD5 handlers have a state enum member for success and return errors if extra stuff happens after success, if you think it's worth it.

> 1634e77b712036d82f3d0080b466cc93dd2b5523:
>   _dispose in sasl auth doesn't set priv->handler to NULL after freeing it
> 
> 561db1c2b0d1981b38aba6a65a9a543eb5e48ee1
> my poor mind has a hard time comprehending 
>  + if ((response == NULL) == (error == NULL))
> 
> why not just do:
>   if (response != NULL && error != NULL)
>     {
>        handler fucked up
>      }
>   else if (response == NULL && error == NULL)
>     {
>         g_warning (weirdness?);
> 
>     } 

Good idea.

> 71b215e0b41f78efef67f35160fb8e256ac8b2ad:
> In dispose of wocky_sasl_digest_md5 non of the members are ever set to NULL
> 
> 78885334038f201e9a4dbaf1bd04d004fd35b32a:
> If you'd give handlers a bool indicating whether they are plain or not, then
> you could generalize the handler selection slightly more so you don't need to
> special case the DIGEST-MD5 and PLAIN handlers mechanisms.

Oh, right, I was going to handle this by just special casing the PLAIN mechanism but forgot. (I.e. if (!wocky_strcmp (handler_mech, "PLAIN") && !allow_plain) continue;) Perhaps having a plain flag is more flexible though. Preference?
 
> For the future i wonder if we should have some way to prioritize handlers (as
> in, prefer 
> CRAM-MD5 over DIGEST-MD5 like  RFC 3920bis suggests and prefer sso tokens over
> everything else?)

The code prioritises mechanisms by the order that handlers for them is added in.
(This is why the loop in _select_handler iterates over handlers and then mechanisms and not the other way around.) I'm assuming the API user has control over which order the handlers are added.

(In reply to comment #2)
> > 4ba6a316ac5ecfa6b2f408bc7aba6500e2e38a66:
> > +  if (0 == strcmp (priv->mech, "PLAIN"))
> >   we have wocky_strdiff, similar to tp_strdiff (this wasn't used in another
> > part ofthe code as it came from gibber from before there was a _strdiff there)
> 
> Right, I'll use wocky_strdiff. (I think I would prefer a wocky_streq, but I
> realise that it was inherited from Gabble.)

wouldn't be against introducing that, but everyone is used to the other one. your call :)

> > Also i don't like the hardcoding of the array indexes in the code just below
> > that.. 
> > 
> > -  for (i = 0 ; handlers[priv->mech][i].name != NULL; i++)
> > +  for (i = 0 ; handler[i].name != NULL; i++ )
> > 
> > but handler isn't an array? This only seems to work because handler is already
> > set to the right index in the array, in the if {} else {} above... But just
> > removing that block and reverting this one would still make the code do the
> > right thing..
> 
> The point was that priv->mech was no longer an enum, so could no longer index
> the handlers array. But this code was all temporary refactoring too and is now
> gone.

Yup, just ignore this

> > Probably would be nice to #define the standard mech strings somewhere, i'm
> > worried about making silly typos when hardcoding strings.
> 
> You mean #define WOCKY_SASL_MECHANISM_DIGEST_MD5 "DIGEST-MD5"?
> 
> I think any typos should be caught by the tests.

Yeah after completing the refactoring it only occurs in the tests and once in the handler, so that's fine. Halfway through the branch there were loads of places strdiffing for it, which why i made the comment

> > 87be0be5449ab592db1688c2034bab6f8431de14:
> > I'd prefer it to keep the state explicit, maybe have the success handlers
> > change the state direclty and have stanza_received call _succeeded when the
> > state equals success..  Seems more clear to me. Although otoh as this is always
> > for xmpp auth, maybe adding a comment in the code to say that if we
> > successfully handled the success stanza the authentication succeeded would be
> > enough.
> 
> Yeah, I guess I should document the WockySaslHandler interface. The idea is
> that handlers keep internal state and return GErrors if the wrong thing happens
> in the wrong state.

That particular code seems fine now. At this commit it was half using the internal state and half using the type of stanza it got to make decisions. Now you've refactored it, the function always uses the stanzas to call into the right function of the handler, which seems fine to me. So this is another comment i can take back :)
 
> I could make the built-in PLAIN and DIGEST-MD5 handlers have a state enum
> member for success and return errors if extra stuff happens after success, if
> you think it's worth it.

see above :)
 
> > 78885334038f201e9a4dbaf1bd04d004fd35b32a:
> > If you'd give handlers a bool indicating whether they are plain or not, then
> > you could generalize the handler selection slightly more so you don't need to
> > special case the DIGEST-MD5 and PLAIN handlers mechanisms.
> 
> Oh, right, I was going to handle this by just special casing the PLAIN
> mechanism but forgot. (I.e. if (!wocky_strcmp (handler_mech, "PLAIN") &&
> !allow_plain) continue;) Perhaps having a plain flag is more flexible though.
> Preference?

A plain flag seems cleaner (and could be reused if another strange plainlike mechanisms comes along (although that's unlikely))

> > For the future i wonder if we should have some way to prioritize handlers (as
> > in, prefer 
> > CRAM-MD5 over DIGEST-MD5 like  RFC 3920bis suggests and prefer sso tokens over
> > everything else?)
> 
> The code prioritises mechanisms by the order that handlers for them is added
> in.
> (This is why the loop in _select_handler iterates over handlers and then
> mechanisms and not the other way around.) I'm assuming the API user has control
> over which order the handlers are added.

Fair enough, please make sure you document this. I guess the auth itself will always add like PLAIN, DIGEST-MD5, CRAM-MD5 (as soon as we add this one) as the last options in the list automagically?

(In reply to comment #3)
> (In reply to comment #2)
> > > 4ba6a316ac5ecfa6b2f408bc7aba6500e2e38a66:
> > > +  if (0 == strcmp (priv->mech, "PLAIN"))
> > >   we have wocky_strdiff, similar to tp_strdiff (this wasn't used in another
> > > part ofthe code as it came from gibber from before there was a _strdiff there)
> > 
> > Right, I'll use wocky_strdiff. (I think I would prefer a wocky_streq, but I
> > realise that it was inherited from Gabble.)
> 
> wouldn't be against introducing that, but everyone is used to the other one.
> your call :)

I'll do that later in a separate branch.

> > > 87be0be5449ab592db1688c2034bab6f8431de14:
> > > I'd prefer it to keep the state explicit, maybe have the success handlers
> > > change the state direclty and have stanza_received call _succeeded when the
> > > state equals success..  Seems more clear to me. Although otoh as this is always
> > > for xmpp auth, maybe adding a comment in the code to say that if we
> > > successfully handled the success stanza the authentication succeeded would be
> > > enough.
> > 
> > Yeah, I guess I should document the WockySaslHandler interface. The idea is
> > that handlers keep internal state and return GErrors if the wrong thing happens
> > in the wrong state.
> 
> That particular code seems fine now. At this commit it was half using the
> internal state and half using the type of stanza it got to make decisions. Now
> you've refactored it, the function always uses the stanzas to call into the
> right function of the handler, which seems fine to me. So this is another
> comment i can take back :)

:)

I added some documentation for WockySaslHandler anyway.

> > > 78885334038f201e9a4dbaf1bd04d004fd35b32a:
> > > If you'd give handlers a bool indicating whether they are plain or not, then
> > > you could generalize the handler selection slightly more so you don't need to
> > > special case the DIGEST-MD5 and PLAIN handlers mechanisms.
> > 
> > Oh, right, I was going to handle this by just special casing the PLAIN
> > mechanism but forgot. (I.e. if (!wocky_strcmp (handler_mech, "PLAIN") &&
> > !allow_plain) continue;) Perhaps having a plain flag is more flexible though.
> > Preference?
> 
> A plain flag seems cleaner (and could be reused if another strange plainlike
> mechanisms comes along (although that's unlikely))

Ok, did this.

> > > For the future i wonder if we should have some way to prioritize handlers (as
> > > in, prefer 
> > > CRAM-MD5 over DIGEST-MD5 like  RFC 3920bis suggests and prefer sso tokens over
> > > everything else?)
> > 
> > The code prioritises mechanisms by the order that handlers for them is added
> > in.
> > (This is why the loop in _select_handler iterates over handlers and then
> > mechanisms and not the other way around.) I'm assuming the API user has control
> > over which order the handlers are added.
> 
> Fair enough, please make sure you document this. I guess the auth itself will
> always add like PLAIN, DIGEST-MD5, CRAM-MD5 (as soon as we add this one) as the
> last options in the list automagically?

It doesn't add them to the list per se, but it does fall back to DIGEST-MD5 and then PLAIN if none of the external handlers are usable. 

Pushed the branch with 6 extra patches.

This one was merged :)