Bug 119303 - [GTK] Support browser credential storage
Summary: [GTK] Support browser credential storage
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Holt
URL:
Keywords:
Depends on: 99352
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-30 20:25 PDT by Ben Boeckel
Modified: 2017-03-11 10:51 PST (History)
11 users (show)

See Also:


Attachments
Patch (16.29 KB, patch)
2013-10-30 09:30 PDT, Brian Holt
cgarcia: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Boeckel 2013-07-30 20:25:13 PDT
With the new "authenticate" signal, it'd be nice to be able to disable WebKit's backing store and allow the browser to handle all credentials. Just ignoring WebKit's store still causes dbus to be launched when libsecret probes for a libsecret client.
Comment 1 Carlos Garcia Campos 2013-08-08 05:52:49 PDT
I think we could add webkit_authentication_request_set_allow_persistent_storage() or something similar. That way the user can connect to authenticate signal, set allow_persistent_storage to FALSE and return FALSE, so that the default dialog is shown, but it won't allow to save credentials persistenly using libsecret.
Comment 2 Martin Robinson 2013-08-08 06:16:50 PDT
(In reply to comment #1)
> I think we could add webkit_authentication_request_set_allow_persistent_storage() or something similar. That way the user can connect to authenticate signal, set allow_persistent_storage to FALSE and return FALSE, so that the default dialog is shown, but it won't allow to save credentials persistenly using libsecret.

Maybe this deserves a global setting?
Comment 3 Carlos Garcia Campos 2013-08-08 07:16:01 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > I think we could add webkit_authentication_request_set_allow_persistent_storage() or something similar. That way the user can connect to authenticate signal, set allow_persistent_storage to FALSE and return FALSE, so that the default dialog is shown, but it won't allow to save credentials persistenly using libsecret.
> 
> Maybe this deserves a global setting?

It would be easier to use indeed, I guess you won't want a different mode per auth request. Global settings as per web view group or web context?
Comment 4 Martin Robinson 2013-08-08 07:20:10 PDT
(In reply to comment #3)

> It would be easier to use indeed, I guess you won't want a different mode per auth request. Global settings as per web view group or web context?

Can requests not have a WebView that owns them? I'd say WebContext, but I'm not certain.
Comment 5 Brian Holt 2013-10-30 07:08:06 PDT
Changed title
Comment 6 Brian Holt 2013-10-30 09:30:36 PDT
Created attachment 215514 [details]
Patch
Comment 7 WebKit Commit Bot 2013-10-30 09:31:56 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Brian Holt 2013-10-30 13:24:08 PDT
(In reply to comment #0)
> With the new "authenticate" signal, it'd be nice to be able to disable WebKit's backing store and allow the browser to handle all credentials. Just ignoring WebKit's store still causes dbus to be launched when libsecret probes for a libsecret client.

Is my patch what you had in mind? Run-time enabling/disabling libsecret persistent storage?  Of course you can always force WebKit not to use libsecret by compiling with --enable-credential-storage=no, but this patch gives you run-time control.
Comment 9 Ben Boeckel 2013-10-30 18:03:15 PDT
(In reply to comment #8)
> Is my patch what you had in mind? Run-time enabling/disabling libsecret persistent storage?

Yep. Thanks!

> Of course you can always force WebKit not to use libsecret by compiling with --enable-credential-storage=no, but this patch gives you run-time control.

I don't know of any binary distro that would build with that option :) .
Comment 10 Carlos Garcia Campos 2013-10-31 01:33:50 PDT
Comment on attachment 215514 [details]
Patch

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

I think the default dialog should honor this setting and not show the option to save the credentials when this is disabled. webkit_authentication_request_can_save_credentials() should also return FALSE when persistent credential storage is enabled, I guess. The problem of this is that this doesn't allow to use the default dialog but with the user implementing the credential storage.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1091
> +void ResourceHandle::setUsePersistentCredentialStorage(bool usePersistentCredentialStorage)
> +{
> +    gUsePersistentCredentialStorage = usePersistentCredentialStorage;
> +}

All other uses of credential storage are GTK specific, so I think this should be in a #if PLATFORM(GTK) block

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:833
> + * webkit_web_context_get_persistent_storage_enabled:

This is a very generic name, persistent storage doesn't say anything about HTTPs authentication nor credentials.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:836
> + * Get whether persistent storage is currently enabled.

You should explain what persistent storage is here, and what happens when this is enabled/disabled. We should also document what the default is.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1407
> +    webkit_settings_set_enable_private_browsing(webkit_web_view_get_settings(test->m_webView), FALSE);
> +    webkit_web_context_set_persistent_storage_enabled(webkit_web_view_get_context(test->m_webView), FALSE);

How can you know if it's because of the private browsing or persisten storage if both are disabled?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1416
> -    webkit_settings_set_enable_private_browsing(webkit_web_view_get_settings(test->m_webView), FALSE);
> +    webkit_web_context_set_persistent_storage_enabled(webkit_web_view_get_context(test->m_webView), TRUE);

I think we want to check both, if persistent storage is enabled, but private browsing disabled, credentials should not be stored permanently.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1419
>      g_assert(!webkit_authentication_request_get_proposed_credential(request));

One of the goals of this API is allow the users to implement credential sotring by themselves. I think we should add a unit tests to check that case it's actually possible, disabling the persistent credential storage in wk, but providing your own credentials in the authenticate signal.

> Source/WebKit2/UIProcess/WebContext.cpp:151
> +    , m_usePersistentCredentialStorage(true)

I think this is GTK specific.

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:147
> +    m_usePersistentCredentialStorage = usePersistentCredentialStorage;
> +    sendToAllProcesses(Messages::WebProcess::SetUsePersistentCredentialStorage(m_usePersistentCredentialStorage));

You should check that the value has actually changed to avoid sending a meesage to the WebProcess for nothing. We can also avoid the message if webkit has been compiled without credential storage support (ENABLE(CREDENTIAL_STORAGE))
Comment 11 Brian Holt 2013-10-31 04:53:12 PDT
Comment on attachment 215514 [details]
Patch

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

Thanks for the review!

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:833
>> + * webkit_web_context_get_persistent_storage_enabled:
> 
> This is a very generic name, persistent storage doesn't say anything about HTTPs authentication nor credentials.

Very good point.  I've been thinking about possible names, what about webkit_web_context_get_persistent_credential_storage_enabled?

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:836
>> + * Get whether persistent storage is currently enabled.
> 
> You should explain what persistent storage is here, and what happens when this is enabled/disabled. We should also document what the default is.

Sure, that makes sense.

>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1419
>>      g_assert(!webkit_authentication_request_get_proposed_credential(request));
> 
> One of the goals of this API is allow the users to implement credential sotring by themselves. I think we should add a unit tests to check that case it's actually possible, disabling the persistent credential storage in wk, but providing your own credentials in the authenticate signal.

All good points, I'll improve the unit test.