Bug 28173 – [GTK] Remove keyring optional features

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

[Xan Lopez]

Per bug #26242, providing optional keyring support in webkitgtk+ itself can be quite problematic for distributors and application developers, and would force us to ship a runtime introspection system to see the features the library you are using has enabled.

For that reason I propose to move the keyring support to libsoup instead, which already has a system like that, and where applications will be able to trivially choose to enable it or not. The only downside of this is that we'll need to figure out a way to store auth data from forms in keyring from applications/libsoup, since webkit won't be able to depend on it directly. I think that will be doable though, since other ports are already doing it.

Attached is a first patch to use the extra auth support available in the 'keyring' branch in libsoup git. If this is approved we'll commit both this patch and that branch to master at the same time.

[Xan Lopez]

Created attachment 34546 [details]
removekeyring.patch

Remove optional keyring support from WebKit, move it to libsoup.

[Xan Lopez]

Created attachment 34547 [details]
removekeyringv2.patch

Only save the auth data if the authentication succeeded. This was fixed recently, do not regress :)

[Jan Alonzo]

Comment on attachment 34547 [details]
removekeyringv2.patch

> -#if USE(GNOMEKEYRING)
>      GtkWidget* checkButton;
> -#endif

Is there a reason why you removed the conditional but not the block inside it? There are a few instances of this in your patch and would be nice to know why they're not removed.
>      char *username;
>      char *password;
>  } WebKitAuthData;
> @@ -103,54 +98,38 @@ static void free_authData(WebKitAuthData* authData)
>      g_slice_free(WebKitAuthData, authData);
>  }
>  
>  static void save_password_callback(SoupMessage* msg, WebKitAuthData* authData)
>  {
>      /* Check only for Success status codes (2xx) */
> -    if (msg->status_code >= 200 && msg->status_code < 300) {
> -        SoupURI* uri = soup_message_get_uri(authData->msg);
> -        gnome_keyring_set_network_password(NULL,
> -                                           authData->username,
> -                                           soup_auth_get_realm(authData->auth),
> -                                           uri->host,
> -                                           NULL,
> -                                           uri->scheme,
> -                                           soup_auth_get_scheme_name(authData->auth),
> -                                           uri->port,
> -                                           authData->password,
> -                                           (GnomeKeyringOperationGetIntCallback)set_password_callback,
> -                                           NULL,
> -                                           NULL);
> -    }
> +    if (msg->status_code >= 200 && msg->status_code < 300)
> +        soup_auth_save_password(authData->auth, authData->username, authData->password);
> +

Do we need to check the status code here still?


>  static void response_callback(GtkDialog* dialog, gint response_id, WebKitAuthData* authData)
>  {
> +    gboolean freeAuthData = TRUE;
> +
>      switch(response_id) {
>      case GTK_RESPONSE_OK:
>          authData->username = g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->loginEntry)));
>          authData->password = g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->passwordEntry)));
> +
>          soup_auth_authenticate(authData->auth, authData->username, authData->password);
>  
> -#if USE(GNOMEKEYRING)
> -        if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton)))
> +        if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton))) {
>              g_signal_connect(authData->msg, "got-headers", G_CALLBACK(save_password_callback), authData);
> -#endif
> +            freeAuthData = FALSE;
> +        }
> +
>      default:
>          break;
>      }

I don't think we need the switch here. I think we can just refactor this into a conditional so we can ditch "freeAuthData".

> @@ -197,10 +176,8 @@ static void show_auth_dialog(WebKitAuthData* authData, const char* login, const
>      GtkWidget* messageLabel;
>      char* message;
>      SoupURI* uri;
> -#if USE(GNOMEKEYRING)
>      GtkWidget* rememberBox;
>      GtkWidget* checkButton;
> -#endif

We seem to be introducing features that were not in non-gnomekeyring builds (I think this is the source of my confusion). What are these for btw?

>  
>      /* From GTK+ gtkmountoperation.c, modified and simplified. LGPL 2 license */
>  
> @@ -279,7 +256,6 @@ static void show_auth_dialog(WebKitAuthData* authData, const char* login, const
>  
>      gtk_entry_set_visibility(GTK_ENTRY(authData->passwordEntry), FALSE);
>  
> -#if USE(GNOMEKEYRING)
>      rememberBox = gtk_vbox_new (FALSE, 6);
>      gtk_box_pack_start (GTK_BOX (vbox), rememberBox,
>                          FALSE, FALSE, 0);
> @@ -290,36 +266,18 @@ static void show_auth_dialog(WebKitAuthData* authData, const char* login, const
>      gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))), TRUE);
>      gtk_box_pack_start (GTK_BOX (rememberBox), checkButton, FALSE, FALSE, 0);
>      authData->checkButton = checkButton;
> -#endif

Ditto with introducing features in default non-gnomekeyring builds.

>  static void session_authenticate(SoupSession* session, SoupMessage* msg, SoupAuth* auth, gboolean retrying, gpointer user_data)
>  {
>      SoupURI* uri;
>      WebKitAuthData* authData;
>      SoupSessionFeature* manager = (SoupSessionFeature*)user_data;
> +    GSList *users;

The asterisk should be in GSList's side.

> +    login = password = NULL;
> +
> +    users = soup_auth_get_saved_users(auth);

This returns one entry only as we don't seem to have a loop below?

> +    if (users) {
> +        login = users->data;
> +        password = soup_auth_get_saved_password(auth, login);
> +        soup_auth_free_saved_users(auth, users);
> +    }

No big issue with the patch. Just some concerns about introducing new features in default non-gnomekeyring builds, etc.. I'm going to say r- for now as patch needs revision.

[Xan Lopez]

(In reply to comment #3)
> > +    if (msg->status_code >= 200 && msg->status_code < 300)
> > +        soup_auth_save_password(authData->auth, authData->username, authData->password);
> > +
> 
> Do we need to check the status code here still?

I think so. Why wouldn't we don't need to?

> 
> 
> >  static void response_callback(GtkDialog* dialog, gint response_id, WebKitAuthData* authData)
> >  {
> > +    gboolean freeAuthData = TRUE;
> > +
> >      switch(response_id) {
> >      case GTK_RESPONSE_OK:
> >          authData->username = g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->loginEntry)));
> >          authData->password = g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->passwordEntry)));
> > +
> >          soup_auth_authenticate(authData->auth, authData->username, authData->password);
> >  
> > -#if USE(GNOMEKEYRING)
> > -        if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton)))
> > +        if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton))) {
> >              g_signal_connect(authData->msg, "got-headers", G_CALLBACK(save_password_callback), authData);
> > -#endif
> > +            freeAuthData = FALSE;
> > +        }
> > +
> >      default:
> >          break;
> >      }
> 
> I don't think we need the switch here. I think we can just refactor this into a
> conditional so we can ditch "freeAuthData".

Well, I can free the authdata unconditionally if the answer is not OK, I guess it's an improvement.

> >  static void session_authenticate(SoupSession* session, SoupMessage* msg, SoupAuth* auth, gboolean retrying, gpointer user_data)
> >  {
> >      SoupURI* uri;
> >      WebKitAuthData* authData;
> >      SoupSessionFeature* manager = (SoupSessionFeature*)user_data;
> > +    GSList *users;
> 
> The asterisk should be in GSList's side.

Right.

> 
> > +    login = password = NULL;
> > +
> > +    users = soup_auth_get_saved_users(auth);
> 
> This returns one entry only as we don't seem to have a loop below?

We only support one login/password per domain, that hasn't changed.

> 
> > +    if (users) {
> > +        login = users->data;
> > +        password = soup_auth_get_saved_password(auth, login);
> > +        soup_auth_free_saved_users(auth, users);
> > +    }
> 
> No big issue with the patch. Just some concerns about introducing new features
> in default non-gnomekeyring builds, etc.. I'm going to say r- for now as patch
> needs revision.

We need to add support in libsoup to figure out if it actually has support to store authentication data permanently, so that we can decide at runtime if we want to show the checkboxes. The best we can do now is to always show them, but they won't do anything if libsoup can't save the data. I'll fix this in a follow-up patch.

[Xan Lopez]

Created attachment 34643 [details]
removekeyringv3.patch

Updated patch.

- Fixes style issues.
- Updates the check of the HTTP status code per danw's suggestion.
- Only shows the 'save passwords' option if the session has a password manager feature.
- I couldn't figure out a way of refactor response_callback to not have a boolean freeAuthData without either duplicating code or adding a goto... suggestions welcome.

[Jan Alonzo]

Comment on attachment 34643 [details]
removekeyringv3.patch

> +static gboolean session_can_save_passwords(SoupSession* session)
> +{
> +    return soup_session_get_feature(session, SOUP_TYPE_PASSWORD_MANAGER) != NULL;
> +}
> +

Is the function necessary? It's only being used once.

>  static void show_auth_dialog(WebKitAuthData* authData, const char* login, const char* password)
>  {
>      GtkWidget* toplevel;
> @@ -197,10 +179,8 @@ static void show_auth_dialog(WebKitAuthData* authData, const char* login, const
>      GtkWidget* messageLabel;
>      char* message;
>      SoupURI* uri;
> -#if USE(GNOMEKEYRING)
>      GtkWidget* rememberBox;
>      GtkWidget* checkButton;
> -#endif
>  
> -#if USE(GNOMEKEYRING)
> -    rememberBox = gtk_vbox_new (FALSE, 6);
> -    gtk_box_pack_start (GTK_BOX (vbox), rememberBox,
> -                        FALSE, FALSE, 0);
> +    rememberBox = gtk_vbox_new(FALSE, 6);
> +    gtk_box_pack_start(GTK_BOX(vbox), rememberBox,
> +                       FALSE, FALSE, 0);

Uhmm can't we pack the rememberBox along with the checkButton only if we can save passwords?

>  
> -    checkButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));
> -    if (login && password)
> -        gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton), TRUE);
> -    gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))), TRUE);
> -    gtk_box_pack_start (GTK_BOX (rememberBox), checkButton, FALSE, FALSE, 0);
> -    authData->checkButton = checkButton;
> -#endif
> +    if (session_can_save_passwords(authData->session)) {
> +        checkButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));
> +        if (login && password)
> +            gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton), TRUE);
> +        gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))), TRUE);
> +        gtk_box_pack_start(GTK_BOX(rememberBox), checkButton, FALSE, FALSE, 0);
> +        authData->checkButton = checkButton;
> +    }

Patch is fine otherwise. r=me but please don't commit this yet. We need to get (1) libsoup updated, and (2) bot support for latest libsoup. 

Thanks!

[Xan Lopez]

(In reply to comment #6)
> (From update of attachment 34643 [details])
> > +static gboolean session_can_save_passwords(SoupSession* session)
> > +{
> > +    return soup_session_get_feature(session, SOUP_TYPE_PASSWORD_MANAGER) != NULL;
> > +}
> > +
> 
> Is the function necessary? It's only being used once.

I think it makes the code more readable, putting the expression directly is a bit obscure.

> 
> >  static void show_auth_dialog(WebKitAuthData* authData, const char* login, const char* password)
> >  {
> >      GtkWidget* toplevel;
> > @@ -197,10 +179,8 @@ static void show_auth_dialog(WebKitAuthData* authData, const char* login, const
> >      GtkWidget* messageLabel;
> >      char* message;
> >      SoupURI* uri;
> > -#if USE(GNOMEKEYRING)
> >      GtkWidget* rememberBox;
> >      GtkWidget* checkButton;
> > -#endif
> >  
> > -#if USE(GNOMEKEYRING)
> > -    rememberBox = gtk_vbox_new (FALSE, 6);
> > -    gtk_box_pack_start (GTK_BOX (vbox), rememberBox,
> > -                        FALSE, FALSE, 0);
> > +    rememberBox = gtk_vbox_new(FALSE, 6);
> > +    gtk_box_pack_start(GTK_BOX(vbox), rememberBox,
> > +                       FALSE, FALSE, 0);
> 
> Uhmm can't we pack the rememberBox along with the checkButton only if we can
> save passwords?

Good point, I'll do that.

> 
> >  
> > -    checkButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));
> > -    if (login && password)
> > -        gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton), TRUE);
> > -    gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))), TRUE);
> > -    gtk_box_pack_start (GTK_BOX (rememberBox), checkButton, FALSE, FALSE, 0);
> > -    authData->checkButton = checkButton;
> > -#endif
> > +    if (session_can_save_passwords(authData->session)) {
> > +        checkButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));
> > +        if (login && password)
> > +            gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton), TRUE);
> > +        gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))), TRUE);
> > +        gtk_box_pack_start(GTK_BOX(rememberBox), checkButton, FALSE, FALSE, 0);
> > +        authData->checkButton = checkButton;
> > +    }
> 
> Patch is fine otherwise. r=me but please don't commit this yet. We need to get
> (1) libsoup updated, and (2) bot support for latest libsoup. 

Nod.

> 
> Thanks!

[Xan Lopez]

Landed as r47129, closing.