[webkit-reviews] review denied: [Bug 218471] Can't login to Playstation.com : [Attachment 413066] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has denied  review:
Bug 218471: Can't login to Playstation.com
https://bugs.webkit.org/show_bug.cgi?id=218471

Attachment 413066: Patch

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




--- Comment #5 from John Wilander <wilander at apple.com> ---
Comment on attachment 413066
  --> https://bugs.webkit.org/attachment.cgi?id=413066
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(HashSe
t<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?