Re: [Evolution-hackers] Camel Authentication Changes

[David Woodhouse <dwmw2 infradead org>]

On Thu, 2011-10-20 at 12:50 -0400, Matthew Barnes wrote:
> 
> I think we could achieve this with multiple calls to
> camel_session_authenticate_sync() with a different mechanism name each
> time ("kerberos", "ntlm", "login"), and making sure the session logic
> knows when to loop and when to bail out if an auth attempt fails.
> 
> It already kinda does that by looking up the CamelServiceAuthType struct
> associated with the mechanism name (if one was given) and checking its
> 'need_password' flag.  If FALSE, it does ONE authentication attempt and
> bails on failure (instead of looping).  After which you could possibly
> call camel_session_authenticate_sync() again with a fallback mechanism
> name.
> 
> Problem is when a mechanism name is NOT given, as is the case for most
> HTTP-based backends, or when 'need_password' is TRUE, it assumes you'll
> *will* need a password to authenticate and prompts if one is not found
> in the keyring (which I understand now to be a bad assumption).  That's
> the part that needs tweaking, for now. 

Yeah, the provider could just call camel_session_authenticate_sync() as
and when it actually *needs* a password (which looks fairly much to be
what imapx *is* doing, so I'm not really sure why it's demanding my
password unnecessarily).

The issue is the complexity. What happens is that the provider calls out
to camel_session_authenticate_sync(), which then calls back to the
provider to give it the information it wanted... but it provides it in a
*synchronous* method and waits for a result, so we can't just hand off
the password with a GAsyncResult to the code which called
camel_session_authenticate_sync() in the first place; we have to do
nasty things with threads and avoid deadlock while we make our
provider's ->authenticate_sync() actually wait for the completion of the
function which *called* it.

Forget EWS for now; let's just look at it in imapx. This bit in
imapx_create_new_connection(), for example:

	/* XXX As part of the connect operation the CamelIMAPXServer will
	 *     have to call camel_session_authenticate_sync(), but it has
	 *     no way to pass itself through in that call so the service
	 *     knows which CamelIMAPXServer is trying to authenticate.
	 *
	 *     IMAPX is the only provider that does multiple connections
	 *     like this, so I didn't want to pollute the CamelSession and
	 *     CamelService authentication APIs with an extra argument.
	 *     Instead we do this little hack so the service knows which
	 *     CamelIMAPXServer to act on in its authenticate_sync() method.
	 *
	 *     Because we're holding the CAMEL_SERVICE_REC_CONNECT_LOCK
	 *     (and our own CON_LOCK for that matter) we should not have
	 *     multiple IMAPX connections trying to authenticate at once,
	 *     so this should be thread-safe.
	 */
	imapx_store->authenticating_server = g_object_ref (conn);
	success = camel_imapx_server_connect (conn, cancellable, error);
	g_object_unref (imapx_store->authenticating_server);

Then in imapx_reconnect() you call camel_session_authenticate_sync()
which ends up calling *back* to imapx's imapx_authenticate_sync()
method... which uses that nastily-stashed 'server' object... which was
RIGHT THERE in the imapx_reconnect() function. We could have just
returned the password to imapx_reconnect() and all would have been well.

I'm unlikely to get much done before I disappear to Prague on Saturday,
and I'm on baby duty tomorrow because she's unwell and can't go to
nursery, but I'll try to put together a patch which sorts this out.
Maybe during next week.

-- 
dwmw2