[Bug 27622] Pluggable SASL authentication

[bugzilla-daemon at freedesktop.org]

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

--- Comment #25 from Sjoerd Simons <sjoerd at luon.net> 2010-05-18 04:18:05 PDT ---
(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 :)

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