Bug 219505 – Issue logging in to Microsoft Teams if logged into other Microsoft accounts and navigating directly to teams.microsoft.com

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

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

[Kate Cheney]

Created attachment 415331 [details]
Patch

[Brent Fulgham]

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!)

[Alex Christensen]

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.

[Kate Cheney]

(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.

[Kate Cheney]

(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?

[Alex Christensen]

(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.

[Kate Cheney]

(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!

[Kate Cheney]

Created attachment 415358 [details]
Patch

[Alex Christensen]

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.

[Kate Cheney]

Created attachment 415362 [details]
Patch

[Kate Cheney]

<rdar://problem/71391657>

[EWS]

Committed r270421: <https://trac.webkit.org/changeset/270421>

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