Bug 101840 – [GTK] Move CredentialBackingStore usage from GtkAuthenticationDialog to ResourceHandleSoup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

This will allow persistent credential storage to work no matter if the GtkAuthenticationDialog is used or not.

[Martin Robinson]

Part of this work includes smarter interaction with the per-session credential storage and redirect handling as well.

[Martin Robinson]

Created attachment 173459 [details]
Patch

[Dan Winship]

Comment on attachment 173459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173459&action=review

i don't understand everything that's going on here, but a few comments...

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:307
> +    if (message->status_code != 401 && message->status_code < 500 && !d->m_credentialDataToSaveInPersistentStore.credential.isEmpty()) {

The 401 here suggests that this code isn't dealing with proxy authentication. Although the old code also had some explicit 401 references...

I guess if the assumption is that proxy user/pass have to be specified in the proxy configuration, and can't be provided at runtime, then this is ok.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:347
> +    // We always put the credentials into the URL. In the CFNetwork-port HTTP family credentials are applied in

Hm... I didn't realize you were going to have to *always* put the credentials into the URL...

as mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=675306, I'm planning to kill off the authenticate signal for SoupRequest-based messages, and just return an error instead, and have the caller to re-send after authenticating. Once we do that, SoupAuthManager would become useless to you, so you could just disable it and then behave like CFNetwork does.

Although then NTLM wouldn't work... I'll have to think about that.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:396
> +        // TODO: We are losing any username and password specified in the redirect URL, as this is the 
> +        // same behavior as the CFNet port. We should investigate if this is really what we want.

definitely sounds like the right thing

[Martin Robinson]

(In reply to comment #3)

> i don't understand everything that's going on here, but a few comments...

Thanks for looking this over.

> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:307
> > +    if (message->status_code != 401 && message->status_code < 500 && !d->m_credentialDataToSaveInPersistentStore.credential.isEmpty()) {
> 
> The 401 here suggests that this code isn't dealing with proxy authentication. Although the old code also had some explicit 401 references...

> I guess if the assumption is that proxy user/pass have to be specified in the proxy configuration, and can't be provided at runtime, then this is ok.

It would be nice to have full-fledged support for proxy authentication here as well -- though perhaps those credentials should be stored in the same way in the keychain. I essentially moved this from the old GtkAuthenticationDialog code. Does proxy authentication currently route through the "authenticate" signal as well? Perhaps we should just should check for success error codes.
 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:347
> > +    // We always put the credentials into the URL. In the CFNetwork-port HTTP family credentials are applied in
> 
> Hm... I didn't realize you were going to have to *always* put the credentials into the URL...

Can you think of any alternatives? I think this is only for requests that have credentials specified explicitly.

> as mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=675306, I'm planning to kill off the authenticate signal for SoupRequest-based messages, and just return an error instead, and have the caller to re-send after authenticating. Once we do that, SoupAuthManager would become useless to you, so you could just disable it and then behave like CFNetwork does.
> 
> Although then NTLM wouldn't work... I'll have to think about that.

If you keep me up to date with the design, I'm happy to change things around to match it. It's quite possible that we can support both the "authenticate" signal and the new approach for backwards compatibility.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:396
> > +        // TODO: We are losing any username and password specified in the redirect URL, as this is the 
> > +        // same behavior as the CFNet port. We should investigate if this is really what we want.
> 
> definitely sounds like the right thing

Great.

[Gustavo Noronha (kov)]

Comment on attachment 173459 [details]
Patch

Makes sense to me.

[WebKit Review Bot]

Comment on attachment 173459 [details]
Patch

Clearing flags on attachment: 173459

Committed r134955: <http://trac.webkit.org/changeset/134955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.