Bug 220897

<rdar://problem/73541303>

Created attachment 418233 [details] Patch

Why would we want to allow this without a user gesture?

(In reply to Sam Weinig from comment #3) > Why would we want to allow this without a user gesture? The spec doesn't suggest it needs a user gesture. We think for any UI that can be triggered by scripts will be safer with some API rate limiter. Therefore, we ask for user gesture. However, most of the old sites don't expect that. This relaxation is more or less a quirk or compromise for those early adopters.

Created attachment 418369 [details] Patch

(In reply to Jiewen Tan from comment #4) > (In reply to Sam Weinig from comment #3) > > Why would we want to allow this without a user gesture? > > The spec doesn't suggest it needs a user gesture. We think for any UI that > can be triggered by scripts will be safer with some API rate limiter. > Therefore, we ask for user gesture. However, most of the old sites don't > expect that. This relaxation is more or less a quirk or compromise for those > early adopters. The future is far greater than the past. I can't think of why it would ever be ok to to prompt for this without a user gesture, so I would recommend we just always require it and get early adopters to update their sites or add site specific quirks if needed.

Created attachment 418495 [details] Patch

(In reply to Sam Weinig from comment #6) > (In reply to Jiewen Tan from comment #4) > > (In reply to Sam Weinig from comment #3) > > > Why would we want to allow this without a user gesture? > > > > The spec doesn't suggest it needs a user gesture. We think for any UI that > > can be triggered by scripts will be safer with some API rate limiter. > > Therefore, we ask for user gesture. However, most of the old sites don't > > expect that. This relaxation is more or less a quirk or compromise for those > > early adopters. > > The future is far greater than the past. I can't think of why it would ever > be ok to to prompt for this without a user gesture, so I would recommend we > just always require it and get early adopters to update their sites or add > site specific quirks if needed. Some really large early adopters have refused to update their sites or have implied it will take years to update their sites. Since there is a number of those websites, this approach seems a bit better than creating quirks for each of them.

I completely disagree. Creating a bad user experience for our users forever because a few sites refuse to change now does not seem like the right tradeoff here.

(In reply to Sam Weinig from comment #9) > I completely disagree. Creating a bad user experience for our users forever > because a few sites refuse to change now does not seem like the right > tradeoff here. This patch is not just about compromise. It also improves the situation a bit by requiring user gestures for security keys as well. Before we only do that for the platform authenticator.

(In reply to Sam Weinig from comment #9) > I completely disagree. Creating a bad user experience for our users forever > because a few sites refuse to change now does not seem like the right > tradeoff here. Jiewen's change is more about making WebKit behave like Chrome does, which is (sadly) what most web developers have standardized around. I think our description of "a few large sites" is misleading. Literally zero sites work with our current flow, because developers expect to be able to trigger WebAuthn without user gesture as part of the sign-in flow. As an example, Microsoft's login page asks the user if they wish to sign in. Once the user agrees, they navigate to their authentication origin (I can't remember the exact eTLD's right now) and attempt to trigger WebAuthn when they hit that site. When WebKit does this, the call fails and users can't log in. Google properties and Facebook work similarly, and we received feedback through various channels from other web developers that requested the behavior we are trying to implement here. The current plan is also based on discussion with other browser implementors who point out that the specification does not speak to user gesture in this case, and that WebKit is the odd person out. Jiewen and I argued your side of this for a long time before being (grudgingly) convinced that this was the best option for our users. Can you suggest another approach that would allow us to preserve the user gesture requirement while being compatible with all of these large web service providers that are designed to work without a gesture? I would be very happy to come up with a way to do so!

Comment on attachment 418495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:386 > + transports.clear(); So this change requires user gesture for any transport now? I think that's a good thing. > Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > + m_requireUserGesture = true; It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:92 > + m_requireUserGesture = true; Having to remember to set this manually seems wrong. It would be better if we just had a method 'processingUserGesture()' that would return the answer and do the book-keeping for us.

(In reply to Brent Fulgham from comment #11) > (In reply to Sam Weinig from comment #9) > > I completely disagree. Creating a bad user experience for our users forever > > because a few sites refuse to change now does not seem like the right > > tradeoff here. > > Can you suggest another approach that would allow us to preserve the user > gesture requirement while being compatible with all of these large web > service providers that are designed to work without a gesture? I would be > very happy to come up with a way to do so! Sure. Make always requiring a user gesture the default and add temporary quirks for the existing providers and let them know that we plan to remove the quirks at some specific time in the future.

Comment on attachment 418495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review Thanks Brent for reviewing this patch. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:386 >> + transports.clear(); > > So this change requires user gesture for any transport now? I think that's a good thing. Yes, that's right. Since we now offer this free prompt quirk, it won't be too much a compatible problem to require user gestures for all. >> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 >> + m_requireUserGesture = true; > > It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. Sure, will do that. >> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:92 >> + m_requireUserGesture = true; > > Having to remember to set this manually seems wrong. It would be better if we just had a method 'processingUserGesture()' that would return the answer and do the book-keeping for us. Sure.

Comment on attachment 418495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 >>> + m_requireUserGesture = true; >> >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > Sure, will do that. How about: auto processingUserGesture = UserGestureIndicator::processingUserGestureForMedia(); if (!processingUserGesture) { if (<<If This site Needs Quirk>>) processingUserGesture |= m_requireUserGesture; } m_requireUserGesture = true;

(In reply to Brent Fulgham from comment #15) > Comment on attachment 418495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418495&action=review > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > >>> + m_requireUserGesture = true; > >> > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > > > Sure, will do that. > > How about: > > auto processingUserGesture = > UserGestureIndicator::processingUserGestureForMedia(); > if (!processingUserGesture) { > if (<<If This site Needs Quirk>>) > processingUserGesture |= m_requireUserGesture; > } > m_requireUserGesture = true; I may just not remember the naming here, but why "processingUserGestureForMedia"? Specifically the "Media" part seems odd.

(In reply to Sam Weinig from comment #16) > (In reply to Brent Fulgham from comment #15) > > Comment on attachment 418495 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review > > > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > > >>> + m_requireUserGesture = true; > > >> > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > > > > > Sure, will do that. > > > > How about: > > > > auto processingUserGesture = > > UserGestureIndicator::processingUserGestureForMedia(); > > if (!processingUserGesture) { > > if (<<If This site Needs Quirk>>) > > processingUserGesture |= m_requireUserGesture; > > } > > m_requireUserGesture = true; > > I may just not remember the naming here, but why > "processingUserGestureForMedia"? Specifically the "Media" part seems odd. This particular method was originally created to support media (I think maybe related to autoplay behavior). We should probably rename it now that we've found a non-media reason for it to exist.

(In reply to Brent Fulgham from comment #17) > (In reply to Sam Weinig from comment #16) > > (In reply to Brent Fulgham from comment #15) > > > Comment on attachment 418495 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review > > > > > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > > > >>> + m_requireUserGesture = true; > > > >> > > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > > > > > > > Sure, will do that. > > > > > > How about: > > > > > > auto processingUserGesture = > > > UserGestureIndicator::processingUserGestureForMedia(); > > > if (!processingUserGesture) { > > > if (<<If This site Needs Quirk>>) > > > processingUserGesture |= m_requireUserGesture; > > > } > > > m_requireUserGesture = true; > > > > I may just not remember the naming here, but why > > "processingUserGestureForMedia"? Specifically the "Media" part seems odd. > > This particular method was originally created to support media (I think > maybe related to autoplay behavior). We should probably rename it now that > we've found a non-media reason for it to exist. I filed Bug 221232 to consider better naming.

(In reply to Brent Fulgham from comment #17) > (In reply to Sam Weinig from comment #16) > > (In reply to Brent Fulgham from comment #15) > > > Comment on attachment 418495 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review > > > > > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > > > >>> + m_requireUserGesture = true; > > > >> > > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > > > > > > > Sure, will do that. > > > > > > How about: > > > > > > auto processingUserGesture = > > > UserGestureIndicator::processingUserGestureForMedia(); > > > if (!processingUserGesture) { > > > if (<<If This site Needs Quirk>>) > > > processingUserGesture |= m_requireUserGesture; > > > } > > > m_requireUserGesture = true; > > > > I may just not remember the naming here, but why > > "processingUserGestureForMedia"? Specifically the "Media" part seems odd. > > This particular method was originally created to support media (I think > maybe related to autoplay behavior). We should probably rename it now that > we've found a non-media reason for it to exist. How does it differ from processingUserGesture()? If the difference isn't that about media, what it is about?

Created attachment 418948 [details] Patch

Created attachment 418949 [details] Patch

Created attachment 419034 [details] Patch

Created attachment 419050 [details] Patch

(In reply to Sam Weinig from comment #19) > (In reply to Brent Fulgham from comment #17) > > (In reply to Sam Weinig from comment #16) > > > (In reply to Brent Fulgham from comment #15) > > > > Comment on attachment 418495 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review > > > > > > > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71 > > > > >>> + m_requireUserGesture = true; > > > > >> > > > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true. > > > > > > > > > > Sure, will do that. > > > > > > > > How about: > > > > > > > > auto processingUserGesture = > > > > UserGestureIndicator::processingUserGestureForMedia(); > > > > if (!processingUserGesture) { > > > > if (<<If This site Needs Quirk>>) > > > > processingUserGesture |= m_requireUserGesture; > > > > } > > > > m_requireUserGesture = true; > > > > > > I may just not remember the naming here, but why > > > "processingUserGestureForMedia"? Specifically the "Media" part seems odd. > > > > This particular method was originally created to support media (I think > > maybe related to autoplay behavior). We should probably rename it now that > > we've found a non-media reason for it to exist. > > How does it differ from processingUserGesture()? If the difference isn't > that about media, what it is about? The difference is that processingUserGestureForMedia will populate the user gesture through XHR and fetch, which is commonly used right before WebAuthn API to fetch nonce. Not sure the media use case though.

Comment on attachment 419050 [details] Patch r=me. Thanks for switching to the tighter version (with Quirk).

(In reply to Brent Fulgham from comment #25) > Comment on attachment 419050 [details] > Patch > > r=me. Thanks for switching to the tighter version (with Quirk). No problems. Thanks for r+ this patch.

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

That's awesome! Sounds like a great solution. Do we know what version of iOS it'll be on?

(In reply to Tal Peretz from comment #28) > That's awesome! Sounds like a great solution. Do we know what version of iOS > it'll be on? I believe this change shipped in iOS 14.5, and is definitely in the iOS 15 series. Note that this behavior was limited to a set of Quirked sites (i.e., only some sites were given this behavior). Another bug tracks work to bring this behavior to all sites, though I don’t have it handy to paste here. If you search in Bugzilla you should find it.