Bug 104910 - [GTK] When in private mode WebKitGTK+ should not save HTTP authentication credentials to the persistent storage
Summary: [GTK] When in private mode WebKitGTK+ should not save HTTP authentication cre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-13 06:39 PST by Martin Robinson
Modified: 2012-12-14 08:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.92 KB, patch)
2012-12-14 03:35 PST, Alberto Garcia
no flags Details | Formatted Diff | Diff
Patch v2 (12.40 KB, patch)
2012-12-14 07:46 PST, Alberto Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-12-13 06:39:42 PST
Instead of using persistent credential storage when private mode is active, WebKitGTK+ should just use session storage.
Comment 1 Alberto Garcia 2012-12-13 08:33:41 PST
I'll take a look
Comment 2 Alberto Garcia 2012-12-14 03:35:08 PST
Created attachment 179463 [details]
Patch
Comment 3 WebKit Review Bot 2012-12-14 03:37:35 PST
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 4 Martin Robinson 2012-12-14 04:02:44 PST
Comment on attachment 179463 [details]
Patch

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

Great patch! If you were a committer I'd r+ and have you land with a few small changes. I have just a few small aesthetic suggestions to make the code more WebKitty.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:260
> +        CredentialPersistence persistence;
> +
> +        if (rememberPassword && dialog->m_browsingMode == NormalMode)
> +            persistence = CredentialPersistencePermanent;
> +        else
> +            persistence = CredentialPersistenceForSession;

This could be one line.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.h:40
> +    enum BrowsingMode {
> +        NormalMode, // The user is asked whether to store credential information
> +        PrivateMode // Credential information is only kept in the session
> +    };

I think that it would make more sense for the authentication dialog to have no knowledge of the concept of browsing mode. Instead the naming of these could be something like:

enum PersistentStorageAllowed {
    AllowPersistentStorage,
    DisallowPersistentStorage,
}

WebKit comments that are complete sentences should end with a period, but I think it's also okay to omit the comments here since the naming is clear enough.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1622
> +    GtkAuthenticationDialog::BrowsingMode browsingMode;
> +
> +    if (webkit_settings_get_enable_private_browsing(webView->priv->settings.get()))
> +        browsingMode = GtkAuthenticationDialog::PrivateMode;
> +    else
> +        browsingMode = GtkAuthenticationDialog::NormalMode;
> +

This could probably just be one line:

PersistentStorageAllowed persistentStorage = webkit_settings_get_enable_private_browsing(webView->priv->settings.get()) ? GtkAuthenticationDialog::AllowPersistentStorage ? GtkAuthenticationDialog::DisallowPersistentStorage;
Comment 5 Alberto Garcia 2012-12-14 04:26:42 PST
(In reply to comment #4)
> > +        CredentialPersistence persistence;
> > +
> > +        if (rememberPassword && dialog->m_browsingMode == NormalMode)
> > +            persistence = CredentialPersistencePermanent;
> > +        else
> > +            persistence = CredentialPersistenceForSession;
>
> This could be one line.

I know, I did it on purpose because the alternative is a 150+ column
line, and I think it's much more readable like this.

But I can change it if you think it's better.

> I think that it would make more sense for the authentication dialog
> > to have no knowledge of the concept of browsing mode. Instead the
> > naming of these could be something like:
>
> enum PersistentStorageAllowed {
>     AllowPersistentStorage,
>     DisallowPersistentStorage,
> }

That sounds good to me.

> WebKit comments that are complete sentences should end with a period

I can also fix that.
Comment 6 Martin Robinson 2012-12-14 05:06:07 PST
(In reply to comment #5)

> I know, I did it on purpose because the alternative is a 150+ column
> line, and I think it's much more readable like this.
> 
> But I can change it if you think it's better.

You can always break the line too. Lines in WebKit typically go to 120 columns and beyond. I usually break mine around 120 columns. Up to you in the end.
Comment 7 Alberto Garcia 2012-12-14 07:46:53 PST
Created attachment 179481 [details]
Patch v2
Comment 8 WebKit Review Bot 2012-12-14 08:58:57 PST
Comment on attachment 179481 [details]
Patch v2

Clearing flags on attachment: 179481

Committed r137748: <http://trac.webkit.org/changeset/137748>
Comment 9 WebKit Review Bot 2012-12-14 08:59:00 PST
All reviewed patches have been landed.  Closing bug.