Bug 218471 – Can't log in to Playstation.com

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Kate Cheney]

Playstation.com seems to need third party cookie access for their login flow. We should add a quirk to call the Storage Access API on their behalf to improve this site experience.

[Kate Cheney]

<rdar://problem/70261333>

[Kate Cheney]

Created attachment 412995 [details]
Patch

[Kate Cheney]

Created attachment 413066 [details]
Patch

[Kate Cheney]

Created attachment 413071 [details]
Patch

[John Wilander]

Comment on attachment 413066 [details]
Patch

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

Have you made sure that we won't get buggy behavior if Sony decides to actually call the Storage Access API themselves with these particular domains?

I think you should add a way to allow localhost and 127.0.0.1 to be treated in this way during tests and then write a layout test to make sure the multi domain storage access doesn't regress.

> Source/WebCore/ChangeLog:10
> +        domains simultaneously. Also add a site specific quirk for

We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.

> Source/WebCore/ChangeLog:11
> +        playstation.com to call the Storage Access API on their behalf.

Not to be that kind of person but it would be nice to get partial credit for this patch. :)

> Source/WebCore/ChangeLog:21
> +        domains together.

We should provide a link to the proposed FPS standard.

> Source/WebKit/ChangeLog:10
> +        for multiple third parties at once. Also adds a site specific quirk for

We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.

> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:46
> +bool FirstPartySets::needsUserInteraction(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain)

This is too vague of a function name. I think you should reverse it and make it more specific. Something like canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction().

> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:50
> +        return !it->value.contains(subFrameDomain);

This function body should use loginDomainsForFirstParty() to make it clear that this is for login purposes, not a general storage access quirk. That also avoids the code duplication.

> Source/WebCore/Modules/firstpartysets/FirstPartySets.h:35
> +class FirstPartySets {

You need a comment here with a link to the proposed FPS standard and that this is just a draft implementation of functionality that WebKit may use FPS for. Otherwise ppl may think this is an official FPS implementation which it isn't.

> Source/WebCore/dom/DocumentStorageAccess.h:144
> +    Optional<HashSet<RegistrableDomain>> subFrameDomains;

The term subFrame no longer makes sense here since multiple subframes cannot call the API at once.

I would prefer a separate code path for requesting storage access for multiple domains at once and make sure the official API only uses the one restricted to actual subframes and a single requesting domain at once.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1347
> +            if (WebCore::FirstPartySets::needsUserInteraction(subFrameDomain, topFrameDomain))

I'd prefer if this function name was reversed as mentioned above. Right now it seems like the user interaction requirement is the exception whereas in reality it's the default and only quirks get the other treatment.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1355
> +    });

Can you explain why this change was needed?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:421
> +    if (subFrameDomains.size() == 1 && subFrameDomains.begin()->string() == topFrameDomain.string())

I'd prefer if we don't have to do all these isOne checks for the normal behavior of the Storage Access API. But maybe there'd be too much code duplication otherwise. Can we at least keep the old signature and make it call the new signature with just a HashSet wrapper around the one domain?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:470
>  {

Ditto on the signature.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:539
> +void WebResourceLoadStatisticsStore::updateEphemeralStorageAccessInWebProcess(HashSet<RegistrableDomain>& subFrameDomains, const RegistrableDomain& topFrameDomain, CompletionHandler<void()>&& completionHandler)

Have we made sure we will only ever update the right web process with this state? We must not leak state across tabs in ephemeral mode.

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:143
> +- (void)_webView:(WKWebView *)webView requestStorageAccessPanelForDomains:(NSArray<NSString *> *)requestingDomains underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(10.14), ios(12.0));

I think it would be much better to keep the old SPI and add a new one for the multi domain one. Otherwise you may need to deprecate the old one.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1280
> +        processPool->setDomainsWithStorageAccess(HashMap<RegistrableDomain, HashSet<RegistrableDomain>> { domains }, [callbackAggregator] { });

We don't call this for ephemeral sessions, right? Do we handle new web processes too?

[Kate Cheney]

Comment on attachment 413066 [details]
Patch

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

Thanks for the review, John!

>> Source/WebCore/ChangeLog:10
>> +        domains simultaneously. Also add a site specific quirk for
> 
> We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.

Will fix.

>> Source/WebCore/ChangeLog:11
>> +        playstation.com to call the Storage Access API on their behalf.
> 
> Not to be that kind of person but it would be nice to get partial credit for this patch. :)

Eeep, I had every intention of crediting you, and with the craziness of this patch it slipped my mind :( Will fix in the next upload.

>> Source/WebCore/ChangeLog:21
>> +        domains together.
> 
> We should provide a link to the proposed FPS standard.

Will fix.

>> Source/WebKit/ChangeLog:10
>> +        for multiple third parties at once. Also adds a site specific quirk for
> 
> We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.

Will fix.

>> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:46
>> +bool FirstPartySets::needsUserInteraction(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain)
> 
> This is too vague of a function name. I think you should reverse it and make it more specific. Something like canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction().

Sounds good.

>> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:50
>> +        return !it->value.contains(subFrameDomain);
> 
> This function body should use loginDomainsForFirstParty() to make it clear that this is for login purposes, not a general storage access quirk. That also avoids the code duplication.

OK will change.

>> Source/WebCore/Modules/firstpartysets/FirstPartySets.h:35
>> +class FirstPartySets {
> 
> You need a comment here with a link to the proposed FPS standard and that this is just a draft implementation of functionality that WebKit may use FPS for. Otherwise ppl may think this is an official FPS implementation which it isn't.

Sounds good, will add.

>> Source/WebCore/dom/DocumentStorageAccess.h:144
>> +    Optional<HashSet<RegistrableDomain>> subFrameDomains;
> 
> The term subFrame no longer makes sense here since multiple subframes cannot call the API at once.
> 
> I would prefer a separate code path for requesting storage access for multiple domains at once and make sure the official API only uses the one restricted to actual subframes and a single requesting domain at once.

OK, I will try to pull this logic out into a separate path. I mainly was trying to avoid duplication as much as I could, but it got messy.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1347
>> +            if (WebCore::FirstPartySets::needsUserInteraction(subFrameDomain, topFrameDomain))
> 
> I'd prefer if this function name was reversed as mentioned above. Right now it seems like the user interaction requirement is the exception whereas in reality it's the default and only quirks get the other treatment.

Sounds good.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1355
>> +    });
> 
> Can you explain why this change was needed?

See the WebKit Changelog -- we need to make sure that the domains get forwarded to the web process during the Storage Access API flow because that is how the quirk determines whether to redirect to the sign-in page.

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:421
>> +    if (subFrameDomains.size() == 1 && subFrameDomains.begin()->string() == topFrameDomain.string())
> 
> I'd prefer if we don't have to do all these isOne checks for the normal behavior of the Storage Access API. But maybe there'd be too much code duplication otherwise. Can we at least keep the old signature and make it call the new signature with just a HashSet wrapper around the one domain?

I will refactor and try to keep as much of the old signature as possible.

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:470
>>  {
> 
> Ditto on the signature.

See above, I will try and refactor.

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:539
>> +void WebResourceLoadStatisticsStore::updateEphemeralStorageAccessInWebProcess(HashSet<RegistrableDomain>& subFrameDomains, const RegistrableDomain& topFrameDomain, CompletionHandler<void()>&& completionHandler)
> 
> Have we made sure we will only ever update the right web process with this state? We must not leak state across tabs in ephemeral mode.

No, I need to test this.

>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:143
>> +- (void)_webView:(WKWebView *)webView requestStorageAccessPanelForDomains:(NSArray<NSString *> *)requestingDomains underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(10.14), ios(12.0));
> 
> I think it would be much better to keep the old SPI and add a new one for the multi domain one. Otherwise you may need to deprecate the old one.

Yes, good call. Will fix.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1280
>> +        processPool->setDomainsWithStorageAccess(HashMap<RegistrableDomain, HashSet<RegistrableDomain>> { domains }, [callbackAggregator] { });
> 
> We don't call this for ephemeral sessions, right? Do we handle new web processes too?

This does get called for ephemeral sessions, so I need to fix that. I am not sure what you mean by new web processes, can you explain?

[John Wilander]

(In reply to katherine_cheney from comment #6)
> Comment on attachment 413066 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413066&action=review
> 
> Thanks for the review, John!
> 
> >> Source/WebCore/ChangeLog:10
> >> +        domains simultaneously. Also add a site specific quirk for
> > 
> > We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.
> 
> Will fix.
> 
> >> Source/WebCore/ChangeLog:11
> >> +        playstation.com to call the Storage Access API on their behalf.
> > 
> > Not to be that kind of person but it would be nice to get partial credit for this patch. :)
> 
> Eeep, I had every intention of crediting you, and with the craziness of this
> patch it slipped my mind :( Will fix in the next upload.

No worries!

> >> Source/WebCore/ChangeLog:21
> >> +        domains together.
> > 
> > We should provide a link to the proposed FPS standard.
> 
> Will fix.
> 
> >> Source/WebKit/ChangeLog:10
> >> +        for multiple third parties at once. Also adds a site specific quirk for
> > 
> > We need to be more specific here and say that this is *only* for within a first party set and *only* for the purposes of authentication. Otherwise we may get requests for multiple *real* third-parties to be able to request storage access at once.
> 
> Will fix.
> 
> >> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:46
> >> +bool FirstPartySets::needsUserInteraction(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain)
> > 
> > This is too vague of a function name. I think you should reverse it and make it more specific. Something like canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction().
> 
> Sounds good.
> 
> >> Source/WebCore/Modules/firstpartysets/FirstPartySets.cpp:50
> >> +        return !it->value.contains(subFrameDomain);
> > 
> > This function body should use loginDomainsForFirstParty() to make it clear that this is for login purposes, not a general storage access quirk. That also avoids the code duplication.
> 
> OK will change.
> 
> >> Source/WebCore/Modules/firstpartysets/FirstPartySets.h:35
> >> +class FirstPartySets {
> > 
> > You need a comment here with a link to the proposed FPS standard and that this is just a draft implementation of functionality that WebKit may use FPS for. Otherwise ppl may think this is an official FPS implementation which it isn't.
> 
> Sounds good, will add.
> 
> >> Source/WebCore/dom/DocumentStorageAccess.h:144
> >> +    Optional<HashSet<RegistrableDomain>> subFrameDomains;
> > 
> > The term subFrame no longer makes sense here since multiple subframes cannot call the API at once.
> > 
> > I would prefer a separate code path for requesting storage access for multiple domains at once and make sure the official API only uses the one restricted to actual subframes and a single requesting domain at once.
> 
> OK, I will try to pull this logic out into a separate path. I mainly was
> trying to avoid duplication as much as I could, but it got messy.
> 
> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1347
> >> +            if (WebCore::FirstPartySets::needsUserInteraction(subFrameDomain, topFrameDomain))
> > 
> > I'd prefer if this function name was reversed as mentioned above. Right now it seems like the user interaction requirement is the exception whereas in reality it's the default and only quirks get the other treatment.
> 
> Sounds good.
> 
> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1355
> >> +    });
> > 
> > Can you explain why this change was needed?
> 
> See the WebKit Changelog -- we need to make sure that the domains get
> forwarded to the web process during the Storage Access API flow because that
> is how the quirk determines whether to redirect to the sign-in page.
> 
> >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:421
> >> +    if (subFrameDomains.size() == 1 && subFrameDomains.begin()->string() == topFrameDomain.string())
> > 
> > I'd prefer if we don't have to do all these isOne checks for the normal behavior of the Storage Access API. But maybe there'd be too much code duplication otherwise. Can we at least keep the old signature and make it call the new signature with just a HashSet wrapper around the one domain?
> 
> I will refactor and try to keep as much of the old signature as possible.
> 
> >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:470
> >>  {
> > 
> > Ditto on the signature.
> 
> See above, I will try and refactor.
> 
> >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:539
> >> +void WebResourceLoadStatisticsStore::updateEphemeralStorageAccessInWebProcess(HashSet<RegistrableDomain>& subFrameDomains, const RegistrableDomain& topFrameDomain, CompletionHandler<void()>&& completionHandler)
> > 
> > Have we made sure we will only ever update the right web process with this state? We must not leak state across tabs in ephemeral mode.
> 
> No, I need to test this.
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:143
> >> +- (void)_webView:(WKWebView *)webView requestStorageAccessPanelForDomains:(NSArray<NSString *> *)requestingDomains underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(10.14), ios(12.0));
> > 
> > I think it would be much better to keep the old SPI and add a new one for the multi domain one. Otherwise you may need to deprecate the old one.
> 
> Yes, good call. Will fix.
> 
> >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1280
> >> +        processPool->setDomainsWithStorageAccess(HashMap<RegistrableDomain, HashSet<RegistrableDomain>> { domains }, [callbackAggregator] { });
> > 
> > We don't call this for ephemeral sessions, right? Do we handle new web processes too?
> 
> This does get called for ephemeral sessions, so I need to fix that. I am not
> sure what you mean by new web processes, can you explain?

You'll see the same dual treatment in how I forward hasHadUserInteraction for specific quirk domains. These are the two cases:
1. The state change just happened and we need to populate all relevant processes with the new state information.
2. The state change has already happened and the user opens a new tab (=>new web process) or clicks a link that navigates them cross-site (=>new web process). The new process immediately needs to be told about the shared state.

[Kate Cheney]

(In reply to John Wilander from comment #5)
> Comment on attachment 413066 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413066&action=review
> 
> Have you made sure that we won't get buggy behavior if Sony decides to
> actually call the Storage Access API themselves with these particular
> domains?

I have not ensured this. How do you suggest testing that?

> 
> I think you should add a way to allow localhost and 127.0.0.1 to be treated
> in this way during tests and then write a layout test to make sure the multi
> domain storage access doesn't regress.
> 
> Ok, I'll work on this!

[Kate Cheney]

> > >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1280
> > >> +        processPool->setDomainsWithStorageAccess(HashMap<RegistrableDomain, HashSet<RegistrableDomain>> { domains }, [callbackAggregator] { });
> > > 
> > > We don't call this for ephemeral sessions, right? Do we handle new web processes too?
> > 
> > This does get called for ephemeral sessions, so I need to fix that. I am not
> > sure what you mean by new web processes, can you explain?
> 
> You'll see the same dual treatment in how I forward hasHadUserInteraction
> for specific quirk domains. These are the two cases:
> 1. The state change just happened and we need to populate all relevant
> processes with the new state information.
> 2. The state change has already happened and the user opens a new tab (=>new
> web process) or clicks a link that navigates them cross-site (=>new web
> process). The new process immediately needs to be told about the shared
> state.

Ah, I missed this, and instead forwarded from the network process every time cookie updating happened instead of the new-tab check. I'll change this in the next patch.

[John Wilander]

(In reply to katherine_cheney from comment #9)
> > > >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1280
> > > >> +        processPool->setDomainsWithStorageAccess(HashMap<RegistrableDomain, HashSet<RegistrableDomain>> { domains }, [callbackAggregator] { });
> > > > 
> > > > We don't call this for ephemeral sessions, right? Do we handle new web processes too?
> > > 
> > > This does get called for ephemeral sessions, so I need to fix that. I am not
> > > sure what you mean by new web processes, can you explain?
> > 
> > You'll see the same dual treatment in how I forward hasHadUserInteraction
> > for specific quirk domains. These are the two cases:
> > 1. The state change just happened and we need to populate all relevant
> > processes with the new state information.
> > 2. The state change has already happened and the user opens a new tab (=>new
> > web process) or clicks a link that navigates them cross-site (=>new web
> > process). The new process immediately needs to be told about the shared
> > state.
> 
> Ah, I missed this, and instead forwarded from the network process every time
> cookie updating happened instead of the new-tab check. I'll change this in
> the next patch.

It's probably not a big thing for you since cookies will be available and so the site will not show the login button.

But without the right info in the web process you may have unnecessary calls to the network process for cookie access via document.cookie. Check that code so that you properly tell web processes that certain domains have cookie access. Chris wrote that code and it has shouldAskITP in the places where it doesn't know and has to call the network process.

The regular Storage Access API already makes sure but it's tied to the page so it doesn't have to sync the way you do. This may turn out to be a larger change than you expected. Check Chris's code to start with.

[John Wilander]

(In reply to katherine_cheney from comment #8)
> (In reply to John Wilander from comment #5)
> > Comment on attachment 413066 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=413066&action=review
> > 
> > Have you made sure that we won't get buggy behavior if Sony decides to
> > actually call the Storage Access API themselves with these particular
> > domains?
> 
> I have not ensured this. How do you suggest testing that?

I think keeping the regular Storage Access API and the new quirk separate as much as possible will help you not get them tangled up. You can see that I've tried to name functions that are used for quirks so that they are separate. Ideally, we should be able to get rid of the quirks without touching real web API code.

You can probably test it by injecting a sony.com iframe into a playstation.com web page, then inject an event handler to call the Storage Access API in that iframe, and finally click/tap it. These injections would be done through Web Inspector. Monkey patch the web page more or less.

> > I think you should add a way to allow localhost and 127.0.0.1 to be treated
> > in this way during tests and then write a layout test to make sure the multi
> > domain storage access doesn't regress.
> > 
> > Ok, I'll work on this!

Great!

[Kate Cheney]

Created attachment 413242 [details]
Patch

[Kate Cheney]

Created attachment 413243 [details]
Patch

[Kate Cheney]

I’m still working on a test case for multiple domains (had to take a break, it was getting complex), but I made all other suggested changes. This patch creates an entirely new code path to avoid regressions in real storage access code, at the expense of some code duplication. It also properly handles ephemeral sessions by keeping storage access data in the web process so it won’t leak between tabs. Putting it up for EWS and more review while working on the test case.

[Kate Cheney]

Created attachment 413307 [details]
Patch

[Kate Cheney]

Created attachment 413435 [details]
Patch

[Kate Cheney]

Created attachment 413445 [details]
Patch

[David Kilzer (:ddkilzer)]

(In reply to katherine_cheney from comment #17)
> Created attachment 413445 [details]
> Patch

All of the ports are "missing" the FirstPartySets.h header, so find another header in that same folder (or project), then `grep -r OtherHeader.h Source/ProjectName` for where the other header is listed in a CMake file, then add FirstPartySets.h.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #18)
> (In reply to katherine_cheney from comment #17)
> > Created attachment 413445 [details]
> > Patch
> 
> All of the ports are "missing" the FirstPartySets.h header, so find another
> header in that same folder (or project), then `grep -r OtherHeader.h
> Source/ProjectName` for where the other header is listed in a CMake file,
> then add FirstPartySets.h.

Probably to Source/WebCore/Headers.cmake.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #19)
> (In reply to David Kilzer (:ddkilzer) from comment #18)
> > (In reply to katherine_cheney from comment #17)
> > > Created attachment 413445 [details]
> > > Patch
> > 
> > All of the ports are "missing" the FirstPartySets.h header, so find another
> > header in that same folder (or project), then `grep -r OtherHeader.h
> > Source/ProjectName` for where the other header is listed in a CMake file,
> > then add FirstPartySets.h.
> 
> Probably to Source/WebCore/Headers.cmake.

Hmm...this patch already does that.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #20)
> (In reply to David Kilzer (:ddkilzer) from comment #19)
> > (In reply to David Kilzer (:ddkilzer) from comment #18)
> > > (In reply to katherine_cheney from comment #17)
> > > > Created attachment 413445 [details]
> > > > Patch
> > > 
> > > All of the ports are "missing" the FirstPartySets.h header, so find another
> > > header in that same folder (or project), then `grep -r OtherHeader.h
> > > Source/ProjectName` for where the other header is listed in a CMake file,
> > > then add FirstPartySets.h.
> > 
> > Probably to Source/WebCore/Headers.cmake.
> 
> Hmm...this patch already does that.

Try adding the path here:

$ git diff Source/WebCore/CMakeLists.txt
diff --git a/Source/WebCore/CMakeLists.txt b/Source/WebCore/CMakeLists.txt
index 435db6a7f18e..112cf355e0b2 100644
--- a/Source/WebCore/CMakeLists.txt
+++ b/Source/WebCore/CMakeLists.txt
@@ -33,6 +33,7 @@ set(WebCore_PRIVATE_INCLUDE_DIRECTORIES
     "${WEBCORE_DIR}/Modules/encryptedmedia"
     "${WEBCORE_DIR}/Modules/encryptedmedia/legacy"
     "${WEBCORE_DIR}/Modules/entriesapi"
+    "${WEBCORE_DIR}/Modules/firstpartysets"
     "${WEBCORE_DIR}/Modules/fetch"
     "${WEBCORE_DIR}/Modules/geolocation"
     "${WEBCORE_DIR}/Modules/highlight"

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #21)
> (In reply to David Kilzer (:ddkilzer) from comment #20)
> > (In reply to David Kilzer (:ddkilzer) from comment #19)
> > > (In reply to David Kilzer (:ddkilzer) from comment #18)
> > > > (In reply to katherine_cheney from comment #17)
> > > > > Created attachment 413445 [details]
> > > > > Patch
> > > > 
> > > > All of the ports are "missing" the FirstPartySets.h header, so find another
> > > > header in that same folder (or project), then `grep -r OtherHeader.h
> > > > Source/ProjectName` for where the other header is listed in a CMake file,
> > > > then add FirstPartySets.h.
> > > 
> > > Probably to Source/WebCore/Headers.cmake.
> > 
> > Hmm...this patch already does that.
> 
> Try adding the path here:
> 
> $ git diff Source/WebCore/CMakeLists.txt
> diff --git a/Source/WebCore/CMakeLists.txt b/Source/WebCore/CMakeLists.txt
> index 435db6a7f18e..112cf355e0b2 100644
> --- a/Source/WebCore/CMakeLists.txt
> +++ b/Source/WebCore/CMakeLists.txt
> @@ -33,6 +33,7 @@ set(WebCore_PRIVATE_INCLUDE_DIRECTORIES
>      "${WEBCORE_DIR}/Modules/encryptedmedia"
>      "${WEBCORE_DIR}/Modules/encryptedmedia/legacy"
>      "${WEBCORE_DIR}/Modules/entriesapi"
>      "${WEBCORE_DIR}/Modules/highlight"

Except in the correct alphabetical order!  :D

>      "${WEBCORE_DIR}/Modules/fetch"
> +    "${WEBCORE_DIR}/Modules/firstpartysets"
>      "${WEBCORE_DIR}/Modules/geolocation"

[Kate Cheney]

Created attachment 413458 [details]
Patch

[Kate Cheney]

Created attachment 413467 [details]
Patch

[Kate Cheney]

Created attachment 413481 [details]
Patch

[Kate Cheney]

Created attachment 413484 [details]
Patch

[Kate Cheney]

Created attachment 413495 [details]
Patch

[Sam Weinig]

(In reply to katherine_cheney from comment #0)
> Playstation.com seems to need third party cookie access for their login
> flow. We should add a quirk to call the Storage Access API on their behalf
> to improve this site experience.

On the face of it, from the description of this bug, it's not clear to me why this is something we would want to fix. We historically have told people that it third party cookies are not something they can use when using WebKit. Can you explain a bit about what makes this case special?

[Kate Cheney]

(In reply to Sam Weinig from comment #28)
> (In reply to katherine_cheney from comment #0)
> > Playstation.com seems to need third party cookie access for their login
> > flow. We should add a quirk to call the Storage Access API on their behalf
> > to improve this site experience.
> 
> On the face of it, from the description of this bug, it's not clear to me
> why this is something we would want to fix. We historically have told people
> that it third party cookies are not something they can use when using
> WebKit. Can you explain a bit about what makes this case special?

Thanks for raising this question, I am happy to explain more! The short of it is, Safari users are asking us for a solution to this, and when evangelism doesn't work, we always consider quirks as long as we can uphold our privacy guarantees. Regarding privacy, some things to note in this patch:

- This quirk is only for domains within the same org.
- This quirk is only for login purposes.
- Still all third-party cookies are blocked by default.
- First Party Sets is an ongoing talk in public standards groups, specifically the Privacy Community Group (https://github.com/privacycg/first-party-sets), and this is a proof-of-concept as to how we may use that in the future.

If we can make this site work while remaining aligned with our privacy goals, I think we should.

[Sam Weinig]

(In reply to katherine_cheney from comment #29)
> (In reply to Sam Weinig from comment #28)
> > (In reply to katherine_cheney from comment #0)
> > > Playstation.com seems to need third party cookie access for their login
> > > flow. We should add a quirk to call the Storage Access API on their behalf
> > > to improve this site experience.
> > 
> > On the face of it, from the description of this bug, it's not clear to me
> > why this is something we would want to fix. We historically have told people
> > that it third party cookies are not something they can use when using
> > WebKit. Can you explain a bit about what makes this case special?
> 
> Thanks for raising this question, I am happy to explain more! The short of
> it is, Safari users are asking us for a solution to this, and when
> evangelism doesn't work, we always consider quirks as long as we can uphold
> our privacy guarantees. Regarding privacy, some things to note in this patch:
> 
> - This quirk is only for domains within the same org.
> - This quirk is only for login purposes.
> - Still all third-party cookies are blocked by default.
> - First Party Sets is an ongoing talk in public standards groups,
> specifically the Privacy Community Group
> (https://github.com/privacycg/first-party-sets), and this is a
> proof-of-concept as to how we may use that in the future.
> 
> If we can make this site work while remaining aligned with our privacy
> goals, I think we should.

Thanks for the details. A few additional questions.

- Do we have existing precident for relaxing the third-party cookie policy for a site? I'm just generally curious as that isn't one of the areas I would have expected us to introduce quirks for.
- This is an awfully large change to introduce a quirk for one site. Is there a more targeted change we could be making that fixes this without relying on a whole new set of infrastructure, especially if it is just for one site.

If the fix does really require us to implement first-party-sets, I think we should split out the implementation into it's own change so that can be reviewed separately from the introduction of the quirk.

[Kate Cheney]

(In reply to Sam Weinig from comment #30)
> If the fix does really require us to implement first-party-sets, I think we
> should split out the implementation into it's own change so that can be
> reviewed separately from the introduction of the quirk.

That is a good idea, I forked that part out here: https://bugs.webkit.org/show_bug.cgi?id=218721. It will also help shrink the size of this patch.

[Kate Cheney]

Created attachment 413712 [details]
Patch

[Kate Cheney]

Created attachment 413714 [details]
Patch

[Brent Fulgham]

After talking with our good friends at Sony, we don't think this will be needed so we can close this bug.