Bug 177738 - [SOUP] Default kerberos authentication credentials are used in ephemeral (private) mode
Summary: [SOUP] Default kerberos authentication credentials are used in ephemeral (pri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tomas Popela
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-02 02:19 PDT by Tomas Popela
Modified: 2017-10-02 05:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.85 KB, patch)
2017-10-02 02:25 PDT, Tomas Popela
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2017-10-02 02:19:29 PDT
If the kerberos credentials are set and there is a valid ticket, then it is used even in the ephemeral (private) mode. It's not expected as we should show an authentication dialog instead. Firefox was doing the same until https://bugzilla.mozilla.org/show_bug.cgi?id=1248564 was fixed. Chromium still needs to be fixed.
Comment 1 Tomas Popela 2017-10-02 02:25:32 PDT
Created attachment 322364 [details]
Patch
Comment 2 Michael Catanzaro 2017-10-02 04:47:25 PDT
Comment on attachment 322364 [details]
Patch

It looks good to me, but ask Carlos Garcia to review this, because since it's possible to have an ephemeral web view with a non-ephemeral web context, I'm not certain if ephemeral web views actually use ephemeral sessions IDs. Hopefully they do.
Comment 3 Carlos Garcia Campos 2017-10-02 04:55:57 PDT
Comment on attachment 322364 [details]
Patch

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

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:86
> +    globalSessionMap().add(sessionID, std::make_unique<NetworkStorageSession>(sessionID, std::make_unique<SoupNetworkSession>(nullptr, sessionID)));

I think we could change the order of the construct parameters t avoid this nullptr, since when providing cookies we always provide a session id too.

> Source/WebCore/platform/network/soup/SoupNetworkSession.h:51
> -    explicit SoupNetworkSession(SoupCookieJar* = nullptr);
> +    explicit SoupNetworkSession(SoupCookieJar* = nullptr, PAL::SessionID = PAL::SessionID::emptySessionID());

why is empty session the default and not default session ID? what's the difference? I guess i doesn't mater as long as is ephemeral is false in both cases.
Comment 4 Tomas Popela 2017-10-02 05:46:28 PDT
(In reply to Carlos Garcia Campos from comment #3)
> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:86
> > +    globalSessionMap().add(sessionID, std::make_unique<NetworkStorageSession>(sessionID, std::make_unique<SoupNetworkSession>(nullptr, sessionID)));
> 
> I think we could change the order of the construct parameters t avoid this
> nullptr, since when providing cookies we always provide a session id too.

Yes, that would be definitely better.
 
> > Source/WebCore/platform/network/soup/SoupNetworkSession.h:51
> > -    explicit SoupNetworkSession(SoupCookieJar* = nullptr);
> > +    explicit SoupNetworkSession(SoupCookieJar* = nullptr, PAL::SessionID = PAL::SessionID::emptySessionID());
> 
> why is empty session the default and not default session ID? what's the
> difference? I guess i doesn't mater as long as is ephemeral is false in both
> cases.

Yeah, both values are not ephemeral. But I will change it to the default session ID as that sounds some appropriate (and is used more across the codebase).
Comment 5 Tomas Popela 2017-10-02 05:55:32 PDT
Committed r222706: <http://trac.webkit.org/changeset/222706>