[Bug 27622] Pluggable SASL authentication

[bugzilla-daemon at freedesktop.org]

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

--- Comment #26 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-05-18 11:35:00 PDT ---
(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!

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