[GTK] Support browser credential storage

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Ben Boeckel]

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.

[Carlos Garcia Campos]

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.

[Martin Robinson]

(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?

[Carlos Garcia Campos]

(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?

[Martin Robinson]

(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.

[Brian Holt]

Changed title

[Brian Holt]

Created attachment 215514 [details]
Patch

[WebKit Commit Bot]

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

[Brian Holt]

(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.

[Ben Boeckel]

(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 :) .

[Carlos Garcia Campos]

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))

[Brian Holt]

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.

[Michael Catanzaro]

The adventure continues in bug #213177

[Carlos Garcia Campos]

I had forgotten this, let's continue in bug #213177, yes, I'll submit a patch soon.