Bug 219505

Summary: Issue logging in to Microsoft Teams if logged into other Microsoft accounts and navigating directly to teams.microsoft.com
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, ews-watchlist, japhet, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kate Cheney 2020-12-03 12:54:10 PST 
In https://bugs.webkit.org/show_bug.cgi?id=218778, we fixed multiple login paths for Microsoft Teams, which relies on third party cookies from login.microsoftonline.com. There is one edge case which this fix did not cover:

1. The user was logged into other Microsoft sites prior to https://bugs.webkit.org/show_bug.cgi?id=218778
2. They go straight to teams.microsoft.com (not through the login page on www.microsoft.com/en-us/microsoft-365/microsoft-teams/)
3. Since a login has occurred on another Microsoft site, it seems that the login step is skipped
4. Teams enters a login loop because there is no storage access
Comment 1 Kate Cheney 2020-12-03 12:57:21 PST 
Created attachment 415331 [details]
Patch
Comment 2 Brent Fulgham 2020-12-03 13:59:34 PST 
Comment on attachment 415331 [details]
Patch

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

I think this looks correct. r=me

> Source/WebCore/ChangeLog:21
> +        reboot loop will occur because the site has login credentials from a previous

Reboot? Or just endless redirect (I suspect the latter!)
Comment 3 Alex Christensen 2020-12-03 14:02:05 PST 
Comment on attachment 415331 [details]
Patch

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

> Source/WebCore/ChangeLog:28
> +        (WebCore::FrameLoader::receivedFirstData):

DocumentLoader::responseReceived might be a better place for this.

> Source/WebCore/page/Quirks.cpp:982
> +bool Quirks::isMicrosoftTeamsRedirectCase(const URL& url)

"Case" -> "URL"?

> Source/WebCore/page/Quirks.cpp:984
> +    return url.string().contains("teams.microsoft.com") && url.query().toString().contains("Retried+3+times+without+success");

Could we check the host instead of the whole url?  I'd rather not match https://example.com/teams.microsoft.com?Retried+3+times+without+success

> Source/WebCore/page/Quirks.cpp:987
> +URL Quirks::microsoftTeamsRedirectURL()

I'm not sure this URL should be in Quirks.
Comment 4 Kate Cheney 2020-12-03 14:19:37 PST 
(In reply to Brent Fulgham from comment #2)
> Comment on attachment 415331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415331&action=review
> 
> I think this looks correct. r=me

Thanks for looking. I will hold off based on Alex's r-.

> 
> > Source/WebCore/ChangeLog:21
> > +        reboot loop will occur because the site has login credentials from a previous
> 
> Reboot? Or just endless redirect (I suspect the latter!)

Oops! Will fix in the next upload.
Comment 5 Kate Cheney 2020-12-03 14:22:10 PST 
(In reply to Alex Christensen from comment #3)
> Comment on attachment 415331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415331&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        (WebCore::FrameLoader::receivedFirstData):
> 
> DocumentLoader::responseReceived might be a better place for this.
> 

Ok, I'll move it there instead.

> > Source/WebCore/page/Quirks.cpp:982
> > +bool Quirks::isMicrosoftTeamsRedirectCase(const URL& url)
> 
> "Case" -> "URL"?
> 

Will change.

> > Source/WebCore/page/Quirks.cpp:984
> > +    return url.string().contains("teams.microsoft.com") && url.query().toString().contains("Retried+3+times+without+success");
> 
> Could we check the host instead of the whole url?  I'd rather not match
> https://example.com/teams.microsoft.com?Retried+3+times+without+success
> 

It is a combination of the host being teams.microsoft.com and the query containing the "Retried+3+times+without+success" message that means the login has failed because no storage access. If the user has previously granted storage access or has not logged in yet then we do not want to redirect, so we cannot redirect based on the host alone.

> > Source/WebCore/page/Quirks.cpp:987
> > +URL Quirks::microsoftTeamsRedirectURL()
> 
> I'm not sure this URL should be in Quirks.

Do you think in DocumentLoader::responseReceived instead?
Comment 6 Alex Christensen 2020-12-03 14:30:30 PST 
(In reply to katherine_cheney from comment #5)
> It is a combination of the host being teams.microsoft.com and the query
> containing the "Retried+3+times+without+success" message that means the
> login has failed because no storage access. If the user has previously
> granted storage access or has not logged in yet then we do not want to
> redirect, so we cannot redirect based on the host alone.
Right, but I think you want to replace url.string()...&&... with url.host()...&&...
> Do you think in DocumentLoader::responseReceived instead?
Yes, or wherever it ends up being used.
Comment 7 Kate Cheney 2020-12-03 14:32:11 PST 
(In reply to Alex Christensen from comment #6)
> (In reply to katherine_cheney from comment #5)
> > It is a combination of the host being teams.microsoft.com and the query
> > containing the "Retried+3+times+without+success" message that means the
> > login has failed because no storage access. If the user has previously
> > granted storage access or has not logged in yet then we do not want to
> > redirect, so we cannot redirect based on the host alone.
> Right, but I think you want to replace url.string()...&&... with
> url.host()...&&...

Oh! I see. Yes I will do that.

> > Do you think in DocumentLoader::responseReceived instead?
> Yes, or wherever it ends up being used.

Ok, will fix.


Thanks for looking!
Comment 8 Kate Cheney 2020-12-03 15:32:41 PST 
Created attachment 415358 [details]
Patch
Comment 9 Alex Christensen 2020-12-03 15:35:14 PST 
Comment on attachment 415358 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:769
> +static URL microsoftTeamsRedirectURL()

This should be inside #if ENABLE(RESOURCE_LOAD_STATISTICS) or just inline where it's used.
Comment 10 Kate Cheney 2020-12-03 15:37:19 PST 
Created attachment 415362 [details]
Patch
Comment 11 Kate Cheney 2020-12-03 17:17:00 PST 
<rdar://problem/71391657>
Comment 12 EWS 2020-12-03 17:32:49 PST 
Committed r270421: <https://trac.webkit.org/changeset/270421>

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