[GTK][WPE] Add API to allow applications to handle the HTTP authentication credential storage

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We depend on libsecret to handle credential persistent storage automatically, but apps might want to use their own storage. I think we could do something like this:

1. Add webkit_authentication_request_set_use_custom_credential_storage (or similar name) to let the request know that saving credentials is allowed but will be handled by user.
2. Add webkit_authentication_request_set_proposed_credential to set the credentials read by application from persistent storage (for apps that want to show the dialog, other prefer to just authenticate without showing the dialog prefilled).
3. When using custom credential storage the user keeps a ref of the request but returns FALSE from authenticate signal to let WebKit handle the dialog (or just TRUE to authenticate with storage credentials)
4. We add a new CredentialPersistence type CredentialPersistenceHandledByUser to let the network process know and avoid storing the credentials using libsecret.
5. On authentication success the request emits WebKitAuthenticationRequest::authenticated signal with the accepted credentials

With this approach the the auhentication request would still have the proposed credential read from persistent storage by libsecret. Apps might decide to always ignore the proposed credentials when using custom storage. Another alternative would be to add a setting to the WebContext to globally disable the persistent storage using libsecret, then we wouldn't need to the webkit_authentication_request_set_use_custom_credential_storage().

[Michael Catanzaro]

Sounds good to me.

> With this approach the the auhentication request would still have the
> proposed credential read from persistent storage by libsecret. Apps might
> decide to always ignore the proposed credentials when using custom storage.

I think if apps will write to custom storage, they probably also want to read from custom storage and ignore any credentials managed by WebKit... right?

> Another alternative would be to add a setting to the WebContext to globally
> disable the persistent storage using libsecret, then we wouldn't need to the
> webkit_authentication_request_set_use_custom_credential_storage().

Hm, either way seems fine to me... keeping everything in WebKitAuthenticationRequest seems nice, to keep related APIs in one place. On the other hand, it probably does make more sense as a global setting rather than a per-request setting. I doubt any app would want to sometimes use WebKit storage for some requests and sometimes use custom storage for other requests.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #1)
> Sounds good to me.
> 
> > With this approach the the auhentication request would still have the
> > proposed credential read from persistent storage by libsecret. Apps might
> > decide to always ignore the proposed credentials when using custom storage.
> 
> I think if apps will write to custom storage, they probably also want to
> read from custom storage and ignore any credentials managed by WebKit...
> right?

Exactly, the thing is that with API to auth request, the decision is made too late and the credentials have already been read by WebKit using libsecret. It's not a problem, because apps handling storage can just ignore the proposed credentials.

> > Another alternative would be to add a setting to the WebContext to globally
> > disable the persistent storage using libsecret, then we wouldn't need to the
> > webkit_authentication_request_set_use_custom_credential_storage().
> 
> Hm, either way seems fine to me... keeping everything in
> WebKitAuthenticationRequest seems nice, to keep related APIs in one place.
> On the other hand, it probably does make more sense as a global setting
> rather than a per-request setting. I doubt any app would want to sometimes
> use WebKit storage for some requests and sometimes use custom storage for
> other requests.

Right, that's a good point.

[Michael Catanzaro]

Both options seem almost equally good, but since we have to pick one, I guess I would slightly favor WebKitWebContext.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Both options seem almost equally good, but since we have to pick one, I
> guess I would slightly favor WebKitWebContext.

I decided to do both in the end:

  - I added a "global" setting to disable persistent credential storage (but to website data manager instead of web context). This way apps can simply disable persistent credential storage even if they are not going to handle the credentials at all.

  - Added webkit_authentication_request_set_proposed_credential() that allows apps to set the proposed credential from their own storage.

  - Added WebKitAuthenticationRequest::authenticated signal. Emitted when the authentication success with the accepted credentials to allow applications to save them.

I have found two problems when starting to use this API in epiphany:

  1- Already remembered password will be lost from the user point of view. I'm not sure it's possible to write a migrator that reads the webkit passwords and stores them in ephy.

  2- We need to get the stored credentials synchronously, since we need to decide whether to return TRUE (when we have credentials and authenticate without showing the dialog) or FALSE to show the dialog. I don't know how to do this.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Michael Catanzaro from comment #3)
> > Both options seem almost equally good, but since we have to pick one, I
> > guess I would slightly favor WebKitWebContext.
> 
> I decided to do both in the end:
> 
>   - I added a "global" setting to disable persistent credential storage (but
> to website data manager instead of web context). This way apps can simply
> disable persistent credential storage even if they are not going to handle
> the credentials at all.
> 
>   - Added webkit_authentication_request_set_proposed_credential() that
> allows apps to set the proposed credential from their own storage.
> 
>   - Added WebKitAuthenticationRequest::authenticated signal. Emitted when
> the authentication success with the accepted credentials to allow
> applications to save them.

Also added webkit_authentication_request_set_can_save_credentials() so that the auth dialog shows the remember checkbox even when persistent storage is disabled in WebKit.

> I have found two problems when starting to use this API in epiphany:
> 
>   1- Already remembered password will be lost from the user point of view.
> I'm not sure it's possible to write a migrator that reads the webkit
> passwords and stores them in ephy.
> 
>   2- We need to get the stored credentials synchronously, since we need to
> decide whether to return TRUE (when we have credentials and authenticate
> without showing the dialog) or FALSE to show the dialog. I don't know how to
> do this.

I managed to make 2 work, by always assuming we have credentials saved and returning TRUE to not show the dialog. If we finally get the credentials we just authenticate, otherwise we force a retry and then we check it's a retry and return FALSE to show the dialog.

[Carlos Garcia Campos]

Created attachment 402549 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

> 1- Already remembered password will be lost from the user point of view. I'm not sure it's possible to write a migrator that reads the webkit passwords and stores them in ephy.

It's impossible, because there's no way to tell which passwords were saved by WebKit. That's the problem with using the default network schema: all the credentials are "leaked". The only way to clean them up is to delete everything using the default schema, but that would delete credentials saved by random apps. Well, you could write a migrator that would migrate *every* secret, but it would necessarily migrate passwords saved by random apps, so I don't think it's a good idea. I would treat all previous HTTP auth passwords as permanently lost.

It would be nice for WebKit to stop using the default schema altogether when built with USE(GTK4).

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5) 
> I managed to make 2 work, by always assuming we have credentials saved and
> returning TRUE to not show the dialog. If we finally get the credentials we
> just authenticate, otherwise we force a retry and then we check it's a retry
> and return FALSE to show the dialog.

I think this is OK, but it should be documented clearly, because it's going to be confusing for everyone if the documentation doesn't indicate how to do it.

[Michael Catanzaro]

Comment on attachment 402549 [details]
Patch

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

Good API design. Good test (very comprehensive). Thanks for handling this.

> Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:187
> +    if (auto* session = networkSession(sessionID))
> +        static_cast<NetworkSessionSoup&>(*session).setPersistentCredentialStorageEnabled(enabled);

Can we ASSERT() here that session is nonnull?

> Source/WebKit/UIProcess/API/glib/WebKitAuthenticationRequest.cpp:198
> + * if private browsing is enabled or persistent credential storage has been

if private browsing is enabled, or if persistent credential storage has been

> Source/WebKit/UIProcess/API/glib/WebKitAuthenticationRequest.cpp:229
> + * This should be used by applications handlig their own credentials

handling

> Source/WebKit/UIProcess/API/glib/WebKitAuthenticationRequest.cpp:277
> + * stored from a previous session. This should only be used by applications
> + * handling their own credential storage, when using the default WebKit credential
> + * storage webkit_authentication_request_get_proposed_credential() already contains
> + * previously stored credentials.

Sounds like: "this should only be used when handling own credential storage and when default credential storage contains previously stored credentials," which is not what you meant.

Fix: "This should only be used by applications handling their own credential storage. (When using the default WebKit credential storage, webkit_authentication_request_get_proposed_credential() already contains previously-stored credentials.)

> Source/WebKit/UIProcess/API/glib/WebKitAuthenticationRequest.cpp:278
> + * Passing a %NULL @credential will clear the proposed credential.

When might you want to clear the proposed credential?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestAuthentication.cpp:165
> -    g_assert_true(test->authenticationCancelledReceived);
> +    g_assert_true(test->m_authenticationCancelledReceived);

How about also checking: g_assert_false(test->m_authenticationSucceededReceived)?

[Carlos Garcia Campos]

Comment on attachment 402549 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitAuthenticationRequest.cpp:278
>> + * Passing a %NULL @credential will clear the proposed credential.
> 
> When might you want to clear the proposed credential?

When using the internal credential storage and you don't want the dialog to be pre-filled, for example.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestAuthentication.cpp:165
>> +    g_assert_true(test->m_authenticationCancelledReceived);
> 
> How about also checking: g_assert_false(test->m_authenticationSucceededReceived)?

We are doing it below, I just forgot to remove this one.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestAuthentication.cpp:174
> +    g_assert_true(test->m_authenticationCancelledReceived);
> +    g_assert_false(test->m_authenticationSucceededReceived);

Here, it's the same check at the same point.

[Carlos Garcia Campos]

Committed r263444: <https://trac.webkit.org/changeset/263444>