Bug 218778

<rdar://problem/36331568>

Created attachment 413756 [details] Patch

Comment on attachment 413756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413756&action=review > Source/WebCore/page/Quirks.cpp:1022 > + // Microsoft Teams login case. Can you add a FIXME comment to remove this quirk, and open a Bugzilla to remove the Quirk once Microsoft completes their migration work? > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1268 > + Mystery whitespace change!

(In reply to Brent Fulgham from comment #3) > Comment on attachment 413756 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413756&action=review > > > Source/WebCore/page/Quirks.cpp:1022 > > + // Microsoft Teams login case. > > Can you add a FIXME comment to remove this quirk, and open a Bugzilla to > remove the Quirk once Microsoft completes their migration work? > I added one in NetworkStorageSession, but I think you're right it makes more sense here so I will move it. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1268 > > + > > Mystery whitespace change! I noticed a space was missing between functions so I slipped it in! :)

Created attachment 413759 [details] Patch

Comment on attachment 413759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413759&action=review Please test manually that a microsoftonline.com iframe under microsoft.com has access to document.cookie after the user grants storage access. The JavaScript side of cookie access has special plumbing. > Source/WebCore/ChangeLog:3 > + Can't login to Microsoft Teams I think a more description title is "Call the Storage Access API on behalf of microsoftonline.com to allow Microsoft Teams login." > Source/WebCore/ChangeLog:14 > + Create a quirk to call the Storage Access API on behalf of microsoft. Microsoft with capital M. :) > Source/WebKit/ChangeLog:14 > + Create a quirk to call the Storage Access API on behalf of microsoft. Ditto. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2108 > +HashMap<RegistrableDomain, RegistrableDomain> ResourceLoadStatisticsDatabaseStore::domainsWithStorageAccess() const Shouldn't this function filter based on domains on the quirks list?

(In reply to John Wilander from comment #6) > Comment on attachment 413759 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413759&action=review > > Please test manually that a microsoftonline.com iframe under microsoft.com > has access to document.cookie after the user grants storage access. The > JavaScript side of cookie access has special plumbing. > > > Source/WebCore/ChangeLog:3 > > + Can't login to Microsoft Teams > > I think a more description title is "Call the Storage Access API on behalf > of microsoftonline.com to allow Microsoft Teams login." *descriptive > > Source/WebCore/ChangeLog:14 > > + Create a quirk to call the Storage Access API on behalf of microsoft. > > Microsoft with capital M. :) > > > Source/WebKit/ChangeLog:14 > > + Create a quirk to call the Storage Access API on behalf of microsoft. > > Ditto. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2108 > > +HashMap<RegistrableDomain, RegistrableDomain> ResourceLoadStatisticsDatabaseStore::domainsWithStorageAccess() const > > Shouldn't this function filter based on domains on the quirks list?

(In reply to John Wilander from comment #6) > Comment on attachment 413759 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413759&action=review > > Please test manually that a microsoftonline.com iframe under microsoft.com > has access to document.cookie after the user grants storage access. The > JavaScript side of cookie access has special plumbing. > Will do. > > Source/WebCore/ChangeLog:3 > > + Can't login to Microsoft Teams > > I think a more description title is "Call the Storage Access API on behalf > of microsoftonline.com to allow Microsoft Teams login." > I thought bugzilla titles were supposed to highlight the bug, not the solution. That is why I titled it this way. Perhaps this is a special case? > > Source/WebCore/ChangeLog:14 > > + Create a quirk to call the Storage Access API on behalf of microsoft. > > Microsoft with capital M. :) > Good catch, will change! > > Source/WebKit/ChangeLog:14 > > + Create a quirk to call the Storage Access API on behalf of microsoft. > > Ditto. > Ditto, will fix. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2108 > > +HashMap<RegistrableDomain, RegistrableDomain> ResourceLoadStatisticsDatabaseStore::domainsWithStorageAccess() const > > Shouldn't this function filter based on domains on the quirks list? The filtering happens in WebResourceLoadStatisticsStore::callUpdatePrevalentDomainsToBlockCookiesFor() before the domains are sent to the web process or stored in NetworkStorageSession (line 1263-4). I did it here because that is where I saw the filtering for the existing user interaction quirks.

Comment on attachment 413759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413759&action=review > Source/WebCore/page/Quirks.cpp:1023 > + // FIXME: Remove this quirk once the microsoft.com login flow works (https://bugs.webkit.org/show_bug.cgi?id=218779). We write this like so (and a suggested revision of the comment) // FIXME(218779): Remove this quirk once microsoft.com completes their login flow redesign. > Source/WebCore/page/Quirks.cpp:1026 > + if (classNames.contains("glyph_signIn_circle") || classNames.contains("mectrl_headertext") || classNames.contains("mectrl_header")) { This might be easier for others to understand with a helper function (or lambda), just like the 'isKinjaLoginAvatarElement' implementation. static bool isMicrosoftLoginElement(const Element& element) { if (!element.hasClass()) return false; auto& classNames = element.classNames(); return classNames.contains("glyph_signIn_circle") || classNames.contains("mectrl_headertext") || classNames.contains("mectrl_header") } Then this line would just be: if (domain == microsoftDomain && isMicrosoftLoginElement(element)) { ... do stuff > Source/WebCore/platform/network/NetworkStorageSession.cpp:393 > + if (loginDomain) if (auto login domain = loginDomain... > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1327 > + if (!NetworkStorageSession::canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction(subFrameDomain, topFrameDomain)) If the only purpose of this test is to ASSERT, I propose placing it inside "#if ASSERT_ENABLED" so we don't perform these functions if no assert can be generated. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1352 > + if (!NetworkStorageSession::canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction(subFrameDomain, topFrameDomain)) Ditto. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1264 > + if (loginDomain && topDomain == loginDomain) You repeat this pattern a lot. I wonder if a helper function would make things clearer: if (WebCore::NetworkStorageSession::loginDomainMatchesFirstParty(topDomain)) ... do stuff And the implementation of "loginDomainMatchesFirstParty" would just be: WebCore { bool NetworkStorageSession::loginDomainMatchesFirstParty(const RegistrableDomain& firstParty) { auto loginDomain = WebCore::NetworkStorageSession::loginDomainForFirstParty(firstParty); return loginDomain && firstParty == loginDomain; }

Created attachment 413864 [details] Patch

Thanks Brent! I made those changes. John, I also tested document.cookie and found the logic you were talking about in WebCookieJar::shouldBlockCookies. I fixed that in this patch and confirmed it is working. Lastly, as I was testing I realized I did not handle the case in which a user goes straight to login.microsoftonline.com or login.live.com without going through the Microsoft sign in page first. I am not sure how frequent this case is, but I should probably adjust the quirk to handle this case too.

Comment on attachment 413864 [details] Patch Thank you for making the changes -- I think this looks good now.

Created attachment 413893 [details] Patch

(In reply to katherine_cheney from comment #13) > Created attachment 413893 [details] > Patch This adjusts the quirk to also call the prompt if the user takes a different login path for Microsoft Teams via login.live.com.

Comment on attachment 413893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413893&action=review r=me with comments. > Source/WebCore/dom/DocumentStorageAccess.cpp:269 > + auto topDomain = NetworkStorageSession::mapToTopDomain(RegistrableDomain::uncheckedCreateFromHost(m_document.topDocument().securityOrigin().host())); Why not use the RegistrableDomain(const URL& url) constructor? We should use the unchecked* calls as little as possible. > Source/WebCore/page/Quirks.cpp:960 > + static NeverDestroyed<RegistrableDomain> microsoftLiveDomain = RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"_s); I would call these microsoftDotCom and liveDotCom since both are Microsoft domains and your function is called isMicrosoftDomain(). > Source/WebCore/page/Quirks.h:30 > +#include <wtf/Optional.h> There doesn't seem to be any new usage of these two in this header file. Could these includes be moved to the implementation file? > Source/WebKit/Shared/WebProcessDataStoreParameters.h:80 > + encoder << domainsWithStorageAccessQuirk; It's not just that these have a quirk, right? They also have cross-page access. See below for comments on naming. I think it's kind of leaking in abstraction here. These lower layers don't really have to know naming-wise that this is a quirk. Here you can probably name things for what they are. The code higher up that actually reasons about quirks, accessing the hardcoded domains, can call things quirky. This is mostly infrastructure to support the quirk in my view. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:203 > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); See below on naming. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:51 > + SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> domains) -> () Async Ditto. > Source/WebKit/UIProcess/WebProcessPool.h:464 > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); It looks strange adding this. One wonders why this wasn't needed before. Isn't it the case here that you need this because these domains have *persistent* or at least *cross-page* storage access? That should go into the function name such as setDomainsWithCrossPageStorageAccess(). I would also like some syntactic sugar with "using" here so that you can make it clear in the header which of the domains is the first party and which is the third party. > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:66 > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&) final; Same thing here with function naming. I believe this is needed for the extended scope of the access because the same WebProcess will be used for all matching webpages. > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:87 > + HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> m_domainsWithStorageAccess; Ditto with naming. > Source/WebKit/WebProcess/WebProcess.cpp:2011 > +void WebProcess::setDomainsWithStorageAccess(HashMap<RegistrableDomain, RegistrableDomain>&& domains, CompletionHandler<void()>&& completionHandler) Ditto. > Source/WebKit/WebProcess/WebProcess.h:499 > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); Ditto. > Source/WebKit/WebProcess/WebProcess.messages.in:166 > + SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> domains) -> () Async Ditto.

(In reply to John Wilander from comment #15) > Comment on attachment 413893 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413893&action=review > > r=me with comments. > Thanks! > > Source/WebCore/dom/DocumentStorageAccess.cpp:269 > > + auto topDomain = NetworkStorageSession::mapToTopDomain(RegistrableDomain::uncheckedCreateFromHost(m_document.topDocument().securityOrigin().host())); > > Why not use the RegistrableDomain(const URL& url) constructor? We should use > the unchecked* calls as little as possible. > I noticed it was done this way in the non-quirk call to requestStorageAccess(), and assumed it was intentional. I don't see a reason to not use the url constructor, though, so I will change it. > > Source/WebCore/page/Quirks.cpp:960 > > + static NeverDestroyed<RegistrableDomain> microsoftLiveDomain = RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"_s); > > I would call these microsoftDotCom and liveDotCom since both are Microsoft > domains and your function is called isMicrosoftDomain(). > Will change. > > Source/WebCore/page/Quirks.h:30 > > +#include <wtf/Optional.h> > > There doesn't seem to be any new usage of these two in this header file. > Could these includes be moved to the implementation file? > Yep, these were accidentally left in from previous drafts. They can be removed completely. > > Source/WebKit/Shared/WebProcessDataStoreParameters.h:80 > > + encoder << domainsWithStorageAccessQuirk; > > It's not just that these have a quirk, right? They also have cross-page > access. See below for comments on naming. I think it's kind of leaking in > abstraction here. These lower layers don't really have to know naming-wise > that this is a quirk. Here you can probably name things for what they are. > The code higher up that actually reasons about quirks, accessing the > hardcoded domains, can call things quirky. This is mostly infrastructure to > support the quirk in my view. Good point, I will update the names based on your comments. > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:203 > > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); > > See below on naming. > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:51 > > + SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> domains) -> () Async > > Ditto. > > > Source/WebKit/UIProcess/WebProcessPool.h:464 > > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); > > It looks strange adding this. One wonders why this wasn't needed before. > Isn't it the case here that you need this because these domains have > *persistent* or at least *cross-page* storage access? That should go into > the function name such as setDomainsWithCrossPageStorageAccess(). I would > also like some syntactic sugar with "using" here so that you can make it > clear in the header which of the domains is the first party and which is the > third party. > Very good point, this can get confusing. I'll change the naming and utilize some more descriptive type aliases. > > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:66 > > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&) final; > > Same thing here with function naming. I believe this is needed for the > extended scope of the access because the same WebProcess will be used for > all matching webpages. > > > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:87 > > + HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> m_domainsWithStorageAccess; > > Ditto with naming. > > > Source/WebKit/WebProcess/WebProcess.cpp:2011 > > +void WebProcess::setDomainsWithStorageAccess(HashMap<RegistrableDomain, RegistrableDomain>&& domains, CompletionHandler<void()>&& completionHandler) > > Ditto. > > > Source/WebKit/WebProcess/WebProcess.h:499 > > + void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&); > > Ditto. > > > Source/WebKit/WebProcess/WebProcess.messages.in:166 > > + SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain> domains) -> () Async > > Ditto. I'll fix these.

Created attachment 414099 [details] Patch for landing

Committed r269807: <https://trac.webkit.org/changeset/269807> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414099 [details].