Bug 220897 - [WebAuthn] Allow one user gesture free prompt for each navigation
Summary: [WebAuthn] Allow one user gesture free prompt for each navigation
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: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2021-01-23 17:33 PST by Jiewen Tan
Modified: 2021-02-03 15:46 PST (History)
4 users (show)

See Also:


Attachments
Patch (75.65 KB, patch)
2021-01-23 17:40 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (79.66 KB, patch)
2021-01-25 21:27 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (84.26 KB, patch)
2021-01-26 18:23 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (87.80 KB, patch)
2021-02-01 18:18 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (87.93 KB, patch)
2021-02-01 18:21 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (92.55 KB, patch)
2021-02-02 11:32 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (92.90 KB, patch)
2021-02-02 12:39 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2021-01-23 17:33:28 PST
Allow one user gesture free prompt for every navigation.
Comment 1 Radar WebKit Bug Importer 2021-01-23 17:33:59 PST
<rdar://problem/73541303>
Comment 2 Jiewen Tan 2021-01-23 17:40:17 PST
Created attachment 418233 [details]
Patch
Comment 3 Sam Weinig 2021-01-23 18:01:48 PST
Why would we want to allow this without a user gesture?
Comment 4 Jiewen Tan 2021-01-25 21:25:52 PST
(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.
Comment 5 Jiewen Tan 2021-01-25 21:27:06 PST
Created attachment 418369 [details]
Patch
Comment 6 Sam Weinig 2021-01-26 14:07:43 PST
(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.
Comment 7 Jiewen Tan 2021-01-26 18:23:07 PST
Created attachment 418495 [details]
Patch
Comment 8 Jiewen Tan 2021-01-26 18:25:19 PST
(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.
Comment 9 Sam Weinig 2021-01-27 10:46:32 PST
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.
Comment 10 Jiewen Tan 2021-01-27 20:04:33 PST
(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.
Comment 11 Brent Fulgham 2021-01-28 17:26:31 PST
(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 12 Brent Fulgham 2021-01-29 09:36:00 PST
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.
Comment 13 Sam Weinig 2021-01-29 11:06:57 PST
(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 14 Jiewen Tan 2021-01-29 12:05:55 PST
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 15 Brent Fulgham 2021-01-29 13:43:08 PST
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;
Comment 16 Sam Weinig 2021-01-30 09:37:24 PST
(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.
Comment 17 Brent Fulgham 2021-02-01 15:39:40 PST
(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.
Comment 18 Brent Fulgham 2021-02-01 15:43:51 PST
(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.
Comment 19 Sam Weinig 2021-02-01 15:44:29 PST
(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?
Comment 20 Jiewen Tan 2021-02-01 18:18:32 PST
Created attachment 418948 [details]
Patch
Comment 21 Jiewen Tan 2021-02-01 18:21:54 PST
Created attachment 418949 [details]
Patch
Comment 22 Jiewen Tan 2021-02-02 11:32:11 PST
Created attachment 419034 [details]
Patch
Comment 23 Jiewen Tan 2021-02-02 12:39:53 PST
Created attachment 419050 [details]
Patch
Comment 24 Jiewen Tan 2021-02-02 13:01:24 PST
(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 25 Brent Fulgham 2021-02-03 15:23:22 PST
Comment on attachment 419050 [details]
Patch

r=me. Thanks for switching to the tighter version (with Quirk).
Comment 26 Jiewen Tan 2021-02-03 15:25:15 PST
(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.
Comment 27 EWS 2021-02-03 15:46:50 PST
Committed r272345: <https://trac.webkit.org/changeset/272345>

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