Bug 220106 - Can't login to Skype from Microsoft Outlook account in Safari
Summary: Can't login to Skype from Microsoft Outlook account in Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-22 14:30 PST by Kate Cheney
Modified: 2020-12-23 11:18 PST (History)
7 users (show)

See Also:


Attachments
Patch (20.24 KB, patch)
2020-12-22 15:15 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (20.90 KB, patch)
2020-12-22 17:49 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (21.67 KB, patch)
2020-12-23 10:35 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-12-22 14:30:38 PST
Skype.com requires 3rd party cookies in order to complete the login flow under outlook.live.com.
Comment 1 Kate Cheney 2020-12-22 14:31:12 PST
rdar://problem/72453487
Comment 2 Kate Cheney 2020-12-22 15:15:49 PST
Created attachment 416695 [details]
Patch
Comment 3 Alex Christensen 2020-12-22 16:35:59 PST
Comment on attachment 416695 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:976
> +        return element.hasClass() && (element.classNames().contains("glyph_signIn_circle") || element.classNames().contains("mectrl_headertext") || element.classNames().contains("mectrl_header") || element.classNames().contains("ext-button primary") || element.classNames().contains("ext-primary"));

This would probably be easier to read if everything after "return element.hasClass() && (" had new lines.

> Source/WebCore/page/Quirks.cpp:981
> +        return element.hasClass() && (element.classNames().contains("_3ioEp2RGR5vb0gqRDsaFPa") || element.classNames().contains("_2Am2jvTaBz17UJ8XnfxFOy"));

Are these static?  They look like they might change.

> Source/WebCore/page/Quirks.cpp:1038
> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {

m_document is a WeakPtr.  We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1040
> +            return;

You need to call completionHandler here.

> Source/WebCore/page/Quirks.cpp:1125
> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {

Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.

> Source/WebCore/page/Quirks.cpp:1136
> +                auto domWindow = document->domWindow();

document is a WeakPtr.  We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1143
> +                    if (is<Frame>(*abstractFrame)) {

I think we shouldn't dereference abstractFrame here without checking it.  Just use is<Frame>(abstractFrame)

> Source/WebCore/platform/network/NetworkStorageSession.cpp:434
> +    static NeverDestroyed<HashMap<String, RegistrableDomain>> map = [] {

Since this map is never added to after initializing, it seems like overkill to even have a map here.  Why don't we just have if (urlToMap.host() == "login.live.com") return "microsoft.com"; return urlToMap;  or something like that.  I also think this should live in Quirks rather than NetworkStorageSession because it has nothing to do with storage.
Comment 4 Kate Cheney 2020-12-22 16:56:14 PST
Comment on attachment 416695 [details]
Patch

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

Thanks for the comments!

>> Source/WebCore/page/Quirks.cpp:976
>> +        return element.hasClass() && (element.classNames().contains("glyph_signIn_circle") || element.classNames().contains("mectrl_headertext") || element.classNames().contains("mectrl_header") || element.classNames().contains("ext-button primary") || element.classNames().contains("ext-primary"));
> 
> This would probably be easier to read if everything after "return element.hasClass() && (" had new lines.

Ok, will change.

>> Source/WebCore/page/Quirks.cpp:981
>> +        return element.hasClass() && (element.classNames().contains("_3ioEp2RGR5vb0gqRDsaFPa") || element.classNames().contains("_2Am2jvTaBz17UJ8XnfxFOy"));
> 
> Are these static?  They look like they might change.

They do look rather random, but they are the class names of the Skype button element. As far as I can tell they are not any more likely to change than the other element class names, like sb-signin-button etc.

>> Source/WebCore/page/Quirks.cpp:1038
>> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {
> 
> m_document is a WeakPtr.  We should probably check if it's null.

It is checked in the function that calls this one. Is that enough, or is it better practice to do it here as well?

>> Source/WebCore/page/Quirks.cpp:1040
>> +            return;
> 
> You need to call completionHandler here.

Will fix.

>> Source/WebCore/page/Quirks.cpp:1125
>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
> 
> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.

It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.

>> Source/WebCore/page/Quirks.cpp:1136
>> +                auto domWindow = document->domWindow();
> 
> document is a WeakPtr.  We should probably check if it's null.

You can't see it in the diff but it is checked at the beginning of this function.

>> Source/WebCore/page/Quirks.cpp:1143
>> +                    if (is<Frame>(*abstractFrame)) {
> 
> I think we shouldn't dereference abstractFrame here without checking it.  Just use is<Frame>(abstractFrame)

Ok, will change.

>> Source/WebCore/platform/network/NetworkStorageSession.cpp:434
>> +    static NeverDestroyed<HashMap<String, RegistrableDomain>> map = [] {
> 
> Since this map is never added to after initializing, it seems like overkill to even have a map here.  Why don't we just have if (urlToMap.host() == "login.live.com") return "microsoft.com"; return urlToMap;  or something like that.  I also think this should live in Quirks rather than NetworkStorageSession because it has nothing to do with storage.

You're right. I will adjust accordingly.
Comment 5 Alex Christensen 2020-12-22 17:08:20 PST
Comment on attachment 416695 [details]
Patch

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

>>> Source/WebCore/page/Quirks.cpp:1038
>>> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {
>> 
>> m_document is a WeakPtr.  We should probably check if it's null.
> 
> It is checked in the function that calls this one. Is that enough, or is it better practice to do it here as well?

I don't see any guarantees that the lambda is called synchronously.  If it is, it's not necessary to do another check, but it wouldn't hurt in case someone changes it in the future.

>>> Source/WebCore/page/Quirks.cpp:1136
>>> +                auto domWindow = document->domWindow();
>> 
>> document is a WeakPtr.  We should probably check if it's null.
> 
> You can't see it in the diff but it is checked at the beginning of this function.

Same.  It might be checked in the calling function, but when the inside of the lambda is called the Document may have been destroyed.
Comment 6 Kate Cheney 2020-12-22 17:32:59 PST
Comment on attachment 416695 [details]
Patch

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

>>> Source/WebCore/page/Quirks.cpp:1125
>>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
>> 
>> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.
> 
> It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.

I remember why I did this. WebCore::Element has an implicitly-deleted copy constructor because it's eventual base class 'CanMakeWeakPtr<WebCore::Node>' has a deleted copy constructor. Is there any way to make it safe to pass by reference even with IPC or no?
Comment 7 Kate Cheney 2020-12-22 17:49:17 PST
Created attachment 416699 [details]
Patch
Comment 8 Kate Cheney 2020-12-22 17:49:43 PST
(In reply to katherine_cheney from comment #6)
> Comment on attachment 416695 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416695&action=review
> 
> >>> Source/WebCore/page/Quirks.cpp:1125
> >>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
> >> 
> >> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.
> > 
> > It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.
> 
> I remember why I did this. WebCore::Element has an implicitly-deleted copy
> constructor because it's eventual base class 'CanMakeWeakPtr<WebCore::Node>'
> has a deleted copy constructor. Is there any way to make it safe to pass by
> reference even with IPC or no?

I used a weakPtr. I think this should be safe.
Comment 9 Alex Christensen 2020-12-23 08:48:33 PST
Comment on attachment 416699 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:971
> +static bool isStorageAccessQuirkDomainElementPair(const URL& url, const Element& element)

I'm not sure "Pair" is a good thing to have in this name.

> Source/WebCore/page/Quirks.cpp:1050
> +    auto domainInNeedOfStorageAccess = RegistrableDomain(*domainsInNeedOfStorageAccess.value().begin().get());

We should probably check if begin().get() is null, which could happen if value() returns an empty container.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:390
> +        map.add(RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"),

The HashMap& returned by this function should probably be const.
Comment 10 Kate Cheney 2020-12-23 10:35:45 PST
Created attachment 416713 [details]
Patch for landing
Comment 11 Kate Cheney 2020-12-23 10:37:06 PST
(In reply to Alex Christensen from comment #9)
> Comment on attachment 416699 [details]
> Patch
> 

Thanks for the review Alex!

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416699&action=review
> 
> > Source/WebCore/page/Quirks.cpp:971
> > +static bool isStorageAccessQuirkDomainElementPair(const URL& url, const Element& element)
> 
> I'm not sure "Pair" is a good thing to have in this name.
> 

I changed this to isStorageAccessQuirkDomainAndElement()

> > Source/WebCore/page/Quirks.cpp:1050
> > +    auto domainInNeedOfStorageAccess = RegistrableDomain(*domainsInNeedOfStorageAccess.value().begin().get());
> 
> We should probably check if begin().get() is null, which could happen if
> value() returns an empty container.
> 

I think this call will actually crash on an empty container, so I added an empty check before. 

> > Source/WebCore/platform/network/NetworkStorageSession.cpp:390
> > +        map.add(RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"),
> 
> The HashMap& returned by this function should probably be const.

Fixed.
Comment 12 EWS 2020-12-23 11:18:36 PST
Committed r271074: <https://trac.webkit.org/changeset/271074>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416713 [details].