[Bug 27622] Pluggable SASL authentication

[bugzilla-daemon at freedesktop.org]

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

--- Comment #24 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-05-17 12:20:08 PDT ---
(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.

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