[webkit-gtk] Authentication dialog API
Brian Holt
brian.holt at samsung.com
Fri Jul 26 03:41:51 PDT 2013
> -----Original Message-----
> From: webkit-gtk-bounces at lists.webkit.org [mailto:webkit-gtk-
> bounces at lists.webkit.org] On Behalf Of Carlos Garcia Campos
> Sent: 26 July 2013 07:10
> To: webkit-gtk at lists.webkit.org
> Subject: Re: [webkit-gtk] Authentication dialog API
>
> El jue, 25-07-2013 a las 18:08 +0200, Carlos Garcia Campos escribió:
> > El jue, 25-07-2013 a las 16:17 +0100, Brian Holt escribió:
> > > Hi,
> > >
> > > There still remains an open question about the ownership of the
> WebKitCredential object that could do with some discussion, but before
> we consider the arguments, here is some pseudocode of how I would
> envisage the API being used for a custom dialog:
> > >
> > > gboolean runAuthenticateCallback(WebKitWebView*,
> > > WebKitAuthenticationRequest* request, gpointer) <- the signal
> handler for authenticate {
> > > WebKitCredential* credential =
> webkit_authentication_request_get_credential(request);
> > > // use the proposed credential to populate the dialog }
> > >
> > > void okButtonClicked(GtkWidget* widget, gpointer) {
> > > // perhaps check whether the credentials are the same as the
> proposed credentials
> > > WebKitAuthenticationRequest* request = priv->request;
> > >
> > > // if not the same as stored
> >
> > I think you always have to create a new credential.
>
> hmm, actually I'm wrong here, there's a use case were we might want to
> use the proposed crtedential directly. The default dialog is always
> shown even when we have saved credentials, but other browsers don't
> show the dialog in such case, so the user might want to do the same.
> This API could also be used to override that default behaviour and
> still use the default dialog with something like this:
>
> gboolean authCallback()
> {
> WebKitCredential *credential =
> webkit_authentication_request_get_credential (request);
> if (credential && webkit_credential_has_password (credential)) {
> webkit_authentication_request_authenticate (request,
> credential);
> webkit_credential_free (credential);
> return TRUE;
> }
>
> // Show the default dialog.
> return FALSE;
> }
>
> I know I'm leaking the credential when showing the default dialog, this
> is just an example. As you see we need a method to know whether the
> credential has a password (it will always have a user if return NULL
> for empty credentials)
Great point. The WebCore::Credential class has a hasPassword() function so I'll add that to the API.
> > > WebKitCredential* credential =
> > > webkit_credential_new(button.username, button.password,
> > > persistence);
> > >
> > > // do the authentication
> > > webkit_authentication_request_authenticate(request, credential) }
> > >
> > > The question to discuss now is how to best design the API regarding
> the ownership of the WebKitCredential. I see 2 main options:
> > >
> > > 1) The WebKitAuthenticationRequest caches the WebKitCredential and
> > > returns a const* in webkit_authentication_request_get_credential()
> > > (transfer none). The credential parameter in
> > > webkit_authentication_request_authenticate() is (transfer full).
> >
> > For a method returning an object we can cache it and return transfer
> > none, but for a parameter I prefer if it's always transfer none and
> > the caller, who created the credential, frees it.
> >
> > > Memory is freed in (a) the request's destructor, (b) in
> > > authenticate() if a previous credential is present
> >
> > I don't think the proposal credential object and the one provided by
> > the user is the same thing. and authenticate is not supposed to be
> > called twice, so the cached credential would only be freed in the
> destructor.
> >
> > > and (c) in the caller *if* the caller creates a WebKitCredential
> > > but doesn't pass it to authenticate().
> >
> > This is one of the reason why I think the caller should take the
> > ownership of the credential provided to authenticate.
> >
> > > This case is easy from the point of view of the client, you can
> > > create the credential, use it to authenticate and forget about it.
> > >
> > > 2) Do not cache the WebKitCredential. The WebKitCredential is
> > > created and returned each time
> > > webkit_authentication_request_get_credential()
> > > (transfer full) is called and
> > > webkit_authentication_request_authenticate() (transfer none) does
> > > not free the memory. It remains the responsibility of the client
> to
> > > always free using webkit_credential_free().
> > >
> > > What do you think?
> >
> > Even if 1) looks more convenient for the user, given that this
> methods
> > are expected to be called once and the crendential object is mostly
> > read only and single use only too (that's why I proposed to not make
> > it refcounted), I think option 2) makes things consistent:
> >
> > gboolean authenticateCallback()
> > {
> > WebKitCredential *credential =
> webkit_authentication_request_get_credential (request);
> > if (credential) {
> > // Fill the user/pass using the credential.
> > }
> > webkit_credential_free (credential); }
> >
> > void okButtonClicked ()
> > {
> > WebKitCredential *credential = webkit_credential_new (user, pass,
> persistence);
> > webkit_authentication_request_authenticate (request, credential);
> > webkit_credential_free (credential); }
>
> I think that we can avoid confusions by renaming
> webkit_authentication_request_get_credential() as
> webkit_authentication_request_get_proposed_credential() to make it
> clear that after webkit_authentication_request_authenticate() you can't
> use get_credential to get the given credential in authenticate.
>
> What do you think?
Very happy to change to _get_proposed_credential(). It makes good sense.
>
> >
> > > Regards
> > > Brian
> > >
> > > -----Original Message-----
> > > From: webkit-gtk-bounces at lists.webkit.org
> > > [mailto:webkit-gtk-bounces at lists.webkit.org] On Behalf Of Brian
> Holt
> > > Sent: 22 July 2013 17:08
> > > To: webkit-gtk at lists.webkit.org
> > > Subject: Re: [webkit-gtk] Authentication dialog API
> > >
> > > Hi all,
> > >
> > > After some feedback and comments here is a new proposal for the
> API:
> > >
> > > The request object that will be emitted along with the
> > > "authenticate" signal is a WebKitAuthenticationRequest with this
> > > public interface
> > >
> > > WEBKIT_API GType
> > > webkit_authentication_request_get_type (void);
> > >
> > > WEBKIT_API gboolean
> > > webkit_authentication_request_can_save_credential
> > > (WebKitAuthenticationRequest *request);
> > >
> > > WEBKIT_API WebKitCredential*
> > > webkit_authentication_request_get_credential
> (WebKitAuthenticationRequest *request);
> > >
> > > WEBKIT_API void
> > > webkit_authentication_request_authenticate
> (WebKitAuthenticationRequest *request,
> > >
> > > WebKitCredential *credential);
> > >
> > > WEBKIT_API void
> > > webkit_authentication_request_cancel
> (WebKitAuthenticationRequest *request);
> > >
> > > webkit_authentication_request_can_save_credential() will return
> FALSE if private browsing is enabled or if there is no libsecret
> support.
> > >
> > >
> > > A new class WebKitCredential is provided to encapsulate the
> credential, which simplifies the API will also allow us to add support
> for client certificate authentication using the same API in the future.
> The user can pass back the existing credential or can create a new one
> prior to authentication. The public API for this object would look as
> follows:
> > >
> > > WEBKIT_API GType
> > > webkit_credential_get_type (void);
> > >
> > > WEBKIT_API WebKitCredential *
> > > webkit_credential_ref (WebKitCredential
> *credential);
> > >
> > > WEBKIT_API void
> > > webkit_credential_unref (WebKitCredential
> *credential);
> > >
> > > WEBKIT_API WebKitCredential *
> > > webkit_credential_new (const gchar *username,
> const gchar *password, WebKitCredentialPersistence persistence);
> > >
> > > WEBKIT_API const gchar *
> > > webkit_credential_get_username (WebKitCredential
> *credential);
> > >
> > > WEBKIT_API const gchar *
> > > webkit_credential_get_password (WebKitCredential
> *credential);
> > >
> > > WEBKIT_API WebKitCredentialPersistence
> > > webkit_credential_get_persistence (WebKitCredential
> *credential);
> > >
> > >
> > > Does this seem like a reasonable approach?
> > >
> > > Regards
> > > Brian
> > >
> > >
> > > -----Original Message-----
> > > From: webkit-gtk-bounces at lists.webkit.org
> > > [mailto:webkit-gtk-bounces at lists.webkit.org] On Behalf Of Brian
> Holt
> > > Sent: 19 July 2013 18:03
> > > To: 'Carlos Garcia Campos'; webkit-gtk at lists.webkit.org
> > > Subject: Re: [webkit-gtk] Authentication dialog API
> > >
> > > Hi Carlos,
> > >
> > > Thanks for your comments and suggestions. I've just uploaded a
> second patch, renaming the signal to "authenticate" and the request
> class WebKitWebViewAuthenticationRequest now has the API you
> suggested.
> > >
> > > Any other feedback and comments welcome.
> > >
> > > Regards
> > > Brian
> > >
> > > -----Original Message-----
> > > From: webkit-gtk-bounces at lists.webkit.org
> > > [mailto:webkit-gtk-bounces at lists.webkit.org] On Behalf Of Carlos
> > > Garcia Campos
> > > Sent: 19 July 2013 15:49
> > > To: webkit-gtk at lists.webkit.org
> > > Subject: Re: [webkit-gtk] Authentication dialog API
> > >
> > > El vie, 19-07-2013 a las 14:21 +0000, Brian Holt escribió:
> > > > Hi WebKitGtk+,
> > > >
> > > > I'm creating the API for the Authentication Dialog (see
> > > > https://trac.webkit.org/wiki/WebKitGTK/WebKit2Roadmap) tracked
> > > > under https://bugs.webkit.org/show_bug.cgi?id=99352. The basic
> > > > idea is that some application authors might want to bring up
> their
> > > > own authentication dialog for http authentication, and so this
> > > > work is to enable them to do so by connecting to a new signal and
> > > > interacting with a new API.
> > >
> > > Not necessarily a dialog, API users could use any auth mechanism,
> as long as they provide a user/pass. So I would avoid the word dialog,
> WebKitWebView::authenticate and WebKitWebViewAuthenticationRequest
> sound good to me.
> > >
> > > > A prototype is working, now I just need to check that I've got
> the
> > > > names and architecture right.
> > > >
> > > > When WebLoaderClient::didReceiveAuthenticationChallengeInFrame()
> > > > is called I propose to create a new
> > > > WebkitAuthenticationDialogRequest object. This class will be the
> > > > public interface for the authentication request and I propose
> that it have the following public API:
> > > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_set_username
> (WebKitAuthenticationDialogRequest *request,
> > > >
> > > > const gchar *username);
> > > >
> > > > WEBKIT_API const gchar *
> > > > webkit_authentication_dialog_request_get_username
> (WebKitAuthenticationDialogRequest *request);
> > > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_set_password
> (WebKitAuthenticationDialogRequest *request,
> > > >
> > > > const gchar* password);
> > > >
> > > > WEBKIT_API const gchar *
> > > > webkit_authentication_dialog_request_get_password
> (WebKitAuthenticationDialogRequest *request);
> > >
> > > Instead of having get and set for the user/pass I think the request
> should be created with the user/pass of the challenge in case it was
> stored in the keyring, and new user/pass should be passed in the
> authenticate method, together with the storage option.
> > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_set_remember_password
> (WebKitAuthenticationDialogRequest *request,
> > > >
> > > > gboolean remember);
> > > >
> > > > WEBKIT_API gboolean
> > > > webkit_authentication_dialog_request_get_remember_password
> (WebKitAuthenticationDialogRequest *request);
> > >
> > > Instead of a boolean this should probably be an enum that would
> allow us to extend it in the future to support other persistence modes
> like permanent, session and none, for example.
> > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_set_private_browsing
> (WebKitAuthenticationDialogRequest *request,
> > > >
> > > > gboolean privateBrowsingEnabled);
> > > >
> > > > WEBKIT_API gboolean
> > > > webkit_authentication_dialog_request_get_private_browsing
> (WebKitAuthenticationDialogRequest *request);
> > >
> > > We have a setting for this, so I think it should be handled
> internally, probably ignoring the saving mode when private browsing is
> enabled.
> > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_authenticate
> (WebKitAuthenticationDialogRequest *request);
> > > >
> > > > WEBKIT_API void
> > > > webkit_authentication_dialog_request_cancel
> (WebKitAuthenticationDialogRequest *request);
> > > >
> > > > To support the existing webkitAuthenticationDialog custom widget
> > > > without much change I propose to also support a private pair of
> > > > functions to access the AuthenticationChallengeProxy*
> > > >
> > > > void
> > > > webkit_authentication_dialog_request_set_authentication_challenge
> > > > (WebKitAuthenticationDialogRequest *request,
> > > >
> > > > WebKit::AuthenticationChallengeProxy * authenticationChallenge);
> > > >
> > > > WebKit::AuthenticationChallengeProxy*
> > > > webkit_authentication_dialog_request_get_authentication_challenge
> > > > (WebKitAuthenticationDialogRequest *request);
> > >
> > > Remember private functions should use the WebKit coding style.
> > >
> > > > From WebLoaderClient::didReceiveAuthenticationChallengeInFrame(),
> > > > the
> > > > WebkitAuthenticationDialogRequest* is passed into the existing
> > > > webkitWebViewHandleAuthenticationChallenge(...) (parameters now
> > > > changed) which emits a new signal.
> > > >
> > > > I propose the signal name be "run-authentication-dialog", since
> in
> > > > principle it is quite similar to "run-file-chooser" in that it
> > > > brings up a dialog for the user to interact with.
> > > >
> > > > The default handler for the signal does what was previously done,
> i.e.
> > > > call webkitWebViewRunAuthenticationDialog() which creates the
> > > > custom webkitAuthenticationDialog.
> > > >
> > > > A WIP patch has been uploaded to
> > > > https://bugs.webkit.org/show_bug.cgi?id=99352 if you want more
> detail.
> > >
> > > Thanks, I'll review it.
> > >
> > > > What do you think?
> > > >
> > > > Regards
> > > > Brian
> > > >
> > > > Brian Holt
> > > > Senior Software Engineer
> > > >
> > > > Samsung Electronics (UK) Limited
> > > > Registered number: 03086621
> > > > Registered address: Samsung House, 1000 Hillswood Drive,
> Chertsey,
> > > > Surrey KT16 0PS, England
> > > >
> > > > South Street Email:
> brian.holt at samsung.com<mailto:brian.holt at samsung.com>
> > > > Staines Fax: +44 (0)1784 428620
> > > > MIDDLESEX Office: +44 (0)1784 428600 (ext 380)
> > > > TW18 4QE
> > > > England
> > >
> > >
> > > --
> > > Carlos Garcia Campos
> > >
> http://pgp.rediris.es:11371/pks/lookup?op=get&search=0xF3D322D0EC458
> > > 2C3
> > >
> > >
> > > _______________________________________________
> > > webkit-gtk mailing list
> > > webkit-gtk at lists.webkit.org
> > > https://lists.webkit.org/mailman/listinfo/webkit-gtk
> > >
> > >
> > > _______________________________________________
> > > webkit-gtk mailing list
> > > webkit-gtk at lists.webkit.org
> > > https://lists.webkit.org/mailman/listinfo/webkit-gtk
> > >
> > >
> > > _______________________________________________
> > > webkit-gtk mailing list
> > > webkit-gtk at lists.webkit.org
> > > https://lists.webkit.org/mailman/listinfo/webkit-gtk
> >
> > _______________________________________________
> > webkit-gtk mailing list
> > webkit-gtk at lists.webkit.org
> > https://lists.webkit.org/mailman/listinfo/webkit-gtk
>
> --
> Carlos Garcia Campos
> http://pgp.rediris.es:11371/pks/lookup?op=get&search=0xF3D322D0EC4582C3
More information about the webkit-gtk
mailing list