Bug 203937 – [WebAuthn] Return NotAllowedError immediately for UI cancellations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

return NotAllowedError immediately for UI cancellations.

[Jiewen Tan]

<rdar://problem/56962420>

[Jiewen Tan]

Created attachment 383003 [details]
Patch

[Brent Fulgham]

Comment on attachment 383003 [details]
Patch

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

r=me, but I would make the patch as small as possible to make it easier to integrate quickly.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
> +    clearState();

To keep the patch small, I would just keep using resetState here.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-217
> -    resetState();

Then you don't need this change.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-357
> -void AuthenticatorManager::resetState()

... or this change.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:-95
> -    void resetState();

Or this change.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:562
> +    EXPECT_TRUE(webAuthenticationPanelFailed);

Nice!

[Jiewen Tan]

Comment on attachment 383003 [details]
Patch

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

Thanks Brent for r+ this change.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
>> +    clearState();
> 
> To keep the patch small, I would just keep using resetState here.

I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.

[Brent Fulgham]

Comment on attachment 383003 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
>>> +    clearState();
>> 
>> To keep the patch small, I would just keep using resetState here.
> 
> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.

I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is.

[Jiewen Tan]

Comment on attachment 383003 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
>>>> +    clearState();
>>> 
>>> To keep the patch small, I would just keep using resetState here.
>> 
>> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.
> 
> I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is.

The reason to have the 'resetState' method is to avoid the m_pendingCompletionHandler check in 'clearState', and is specifically crafted for this method. Now this method will invoke m_pendingCompletionHandler as well, and therefore there is no need to have the 'resetState' method anymore. That's why I remove it.

[Brent Fulgham]

(In reply to Jiewen Tan from comment #6)
> Comment on attachment 383003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383003&action=review
> 
> >>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
> >>>> +    clearState();
> >>> 
> >>> To keep the patch small, I would just keep using resetState here.
> >> 
> >> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.
> > 
> > I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is.
> 
> The reason to have the 'resetState' method is to avoid the
> m_pendingCompletionHandler check in 'clearState', and is specifically
> crafted for this method. Now this method will invoke
> m_pendingCompletionHandler as well, and therefore there is no need to have
> the 'resetState' method anymore. That's why I remove it.

Fine. Let's just get it landed so I can try to get it in.

[Jiewen Tan]

(In reply to Brent Fulgham from comment #7)
> (In reply to Jiewen Tan from comment #6)
> > Comment on attachment 383003 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=383003&action=review
> > 
> > >>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202
> > >>>> +    clearState();
> > >>> 
> > >>> To keep the patch small, I would just keep using resetState here.
> > >> 
> > >> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.
> > > 
> > > I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is.
> > 
> > The reason to have the 'resetState' method is to avoid the
> > m_pendingCompletionHandler check in 'clearState', and is specifically
> > crafted for this method. Now this method will invoke
> > m_pendingCompletionHandler as well, and therefore there is no need to have
> > the 'resetState' method anymore. That's why I remove it.
> 
> Fine. Let's just get it landed so I can try to get it in.

Thanks, Brent.

[Jiewen Tan]

Committed r252248: <https://trac.webkit.org/changeset/252248>

[Jiewen Tan]

*** Bug 204186 has been marked as a duplicate of this bug. ***