GNOME Bugzilla – Bug 703186
fallback from automatic NTLM auth to 'authenticate' signal not working.
Last modified: 2013-09-13 14:06:20 UTC
Created attachment 247900 [details] [review] Set state to SOUP_NTLM_SSO_FAILED when SSO fails, not SOUP_NTLM_FAILED If my ntlm_auth helper has an out-of-date password, my connections should get an 'authenticate' signal and I should be able to provide a correct password. This doesn't work. Probably fallout of commit b24b8324b152, or maybe it never worked (the regression test evidently doesn't seem to check this fallback). The connection state becomes SOUP_NTLM_FAILED when the automatic NTLM auth fails, so I'm *asked* for a password, but it never actually gets used. I can fix soup_auth_ntlm_update_connection() to set the state to SOUP_NTLM_SSO_FAILED when this happens, instead. And now things seem to work... almost. We're still invoking the *first* "authenticate" signal with 'retrying=TRUE', which makes the application things that its idea of the password is incorrect and ask the user for a new one. Every time. See bug #703181. I'm also entirely unclear about how the PASSWORD_ACCEPTED case is supposed to work. When you get a 401 after you've *successfully* authenticated, you actually want the user to provide a different *username* if they want to get anything more helpful from the server, right? I'm not quite sure why SOUP_NTLM_FAILED is correct there either; surely it'll never do anything sensible? So maybe that case wants some change too.
This fixes the 'retrying' issue for me: @@ -433,7 +437,7 @@ soup_auth_ntlm_is_connection_ready (SoupConnectionAuth *auth, if (priv->password_state == SOUP_NTLM_PASSWORD_PROVIDED) return TRUE; - return conn->state != SOUP_NTLM_FAILED && conn->state != SOUP_NTLM_SSO_FAILED; + return conn->state != SOUP_NTLM_FAILED; } static void In soup_auth_manager.c::auth_got_headers() we call soup_auth_is_ready() to determine a value for prior_auth_failed, which translates to the 'retrying' argument to the authenticate signal. So we want soup_auth_ntlm_is_connection_ready() to return *FALSE* in the case where the state is SOUP_NTLM_SSO_FAILED. It will then issue an NTLM Type1 packet of its own, to be followed with normal NTLM authentication as if SSO hadn't happened at all. This now works without having to touch Evolution's auth handling code at all (although the initial part of bug 703181 still exists). Testing is somewhat limited, since I have made our Exchange servers hate me. But it *was* working.
(In reply to comment #1) > So we want soup_auth_ntlm_is_connection_ready() to return *FALSE* in the case > where the state is SOUP_NTLM_SSO_FAILED. Er, that should be *TRUE*, not false. It *is* still ready, because it can still continue. In fact, its state is now exactly as it would have been on initial startup if SSO hadn't been available. To reinforce that observation, we could even ditch the SOUP_NTLM_SSO_FAILED state and set conn->state to SOUP_NTLM_NEW in the few places which set it thus. Those places would also have to clear priv->sso_available.
Created attachment 247936 [details] [review] Complete fix. [PATCH] Bug 703186 - fallback from automatic NTLM auth not working There were two issues. Firstly, when the automatic NTLM authentication failed we were setting conn->state to SOUP_NTLM_FAILED. So although we would emit the 'authenticate' signal and obtain a password, we'd never actually *try* it. We have the SOUP_NTLM_SSO_FAILED state for this, which makes it work. Secondly, the resulting 'authenticate' signal we being emitted with the 'retrying' property set, even when it was the first time we actually asked for a password. This was confusing Evolution (bug 703181 comments 1-3). Fix the latter problem by making soup_auth_ntlm_is_connection_ready() return TRUE when the state is SOUP_NTLM_SSO_FAILED. That makes auth_got_headers() do the right thing, and Evolution is happy again. With this fixed, Evolution will now automatically try to connect to an Exchange server using automatic NTLM authentication, and will fall back to asking for a password only if it actually needs to. ---
Comment on attachment 247936 [details] [review] Complete fix. I think this is right, but I'd like a test case added to ntlm-test for it so it doesn't get broken again in the future.
I'll see if I can update the test too. I note the comments say that the NTLM server-side part for testing isn't very sophisticated. But that's OK; we can make the Type3 response in this test case *egregiously* wrong, to make sure it gets rejected. And then check that a normal non-SSO authentication works. Note that when we get Kerberos support working too, we're going to need to make sure the fallback Kerberos -> autoNTLM -> NTLM works properly too. In a Windows environment, a *lot* of Kerberos->NTLM fallback goes on. Windows does it automatically, so when DNS or other stuff is screwed up and Kerberos fails, nobody notices. You notice it very quickly on Linux if you *only* have Kerberos.
(In reply to comment #5) > I'll see if I can update the test too. I note the comments say that the NTLM > server-side part for testing isn't very sophisticated. But that's OK; we can > make the Type3 response in this test case *egregiously* wrong, to make sure it > gets rejected. All you need to do is call soup_auth_authenticate() with the wrong username or password. (And make sure server_callback() recognizes that as "not authenticated".)
Fixed in master (2b934573) and gnome-3-8 (df924548) after accepting the following warning: <danw> dwmw2_gone: you can push that fix if you want, but i preemptively mock you for when it breaks again in the future because you didn't add a regression test