Bug 200932 - [WebAuthn] Warn users when multiple NFC tags present
Summary: [WebAuthn] Warn users when multiple NFC tags present
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
: 201017 (view as bug list)
Depends on: 202559
Blocks: 181943
  Show dependency treegraph
 
Reported: 2019-08-20 12:07 PDT by Jiewen Tan
Modified: 2019-10-27 15:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (44.71 KB, patch)
2019-10-21 22:50 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (44.73 KB, patch)
2019-10-22 02:06 PDT, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for relanding (44.44 KB, patch)
2019-10-25 17:05 PDT, 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 2019-08-20 12:07:07 PDT
Allow user to select a different NFC tag if multiple present.
Comment 1 Jiewen Tan 2019-08-21 18:25:12 PDT
A solution here is when multiple tags are detected, we could advise users to pick one to use in the UI.
Comment 2 Radar WebKit Bug Importer 2019-08-30 11:49:34 PDT
<rdar://problem/54890736>
Comment 3 Jiewen Tan 2019-10-18 17:24:30 PDT
*** Bug 201017 has been marked as a duplicate of this bug. ***
Comment 4 Jiewen Tan 2019-10-21 22:50:40 PDT
Created attachment 381511 [details]
Patch
Comment 5 Jiewen Tan 2019-10-22 02:06:18 PDT
Created attachment 381533 [details]
Patch
Comment 6 Brent Fulgham 2019-10-23 14:27:50 PDT
Comment on attachment 381533 [details]
Patch

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

Looks good! Nice to see more WPT's passing. r=me.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h:60
> +    // These operation are guaranteed to execute asynchronously.

These operations are ...

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:133
> +    NSError *error = nil;

You don't have to initialize to 'nil' in ObjC. It happens automatically.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:259
> +            [tags addObject:adoptNS([[WKMockNFTag alloc] initWithType:NFTagTypeGeneric4A tagID:adoptNS([[NSData alloc] initWithBytes:tagID2 length:sizeof(tagID2)]).get()]).get()];

Could something in here be 'mutable' so we can leave this as a 'const' method? (Assuming the mutation is not something that actually changes the logical constness of the object?)
Comment 7 Jiewen Tan 2019-10-23 14:38:36 PDT
Comment on attachment 381533 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h:60
>> +    // These operation are guaranteed to execute asynchronously.
> 
> These operations are ...

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:133
>> +    NSError *error = nil;
> 
> You don't have to initialize to 'nil' in ObjC. It happens automatically.

I tried to grep different patterns on the project, and here is the result:
"NSError *error;": 10
"NSError *error = nil;": 46

So I think maybe "NSError *error = nil;" is more inline with our coding practice.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:259
>> +            [tags addObject:adoptNS([[WKMockNFTag alloc] initWithType:NFTagTypeGeneric4A tagID:adoptNS([[NSData alloc] initWithBytes:tagID2 length:sizeof(tagID2)]).get()]).get()];
> 
> Could something in here be 'mutable' so we can leave this as a 'const' method? (Assuming the mutation is not something that actually changes the logical constness of the object?)

Good catch. This is a mistake. I was doing something else that requires dropping the const qualifier, but then later I removed those code.
Comment 8 Jiewen Tan 2019-10-23 14:58:25 PDT
Committed r251500: <https://trac.webkit.org/changeset/251500>
Comment 9 Matt Lewis 2019-10-25 14:32:01 PDT
This was rolled out because it interfered with rolling out of

https://bugs.webkit.org/show_bug.cgi?id=202561

The rollout https://trac.webkit.org/changeset/251602/webkit
Comment 10 Brent Fulgham 2019-10-25 15:07:05 PDT
(In reply to Matt Lewis from comment #9)
> This was rolled out because it interfered with rolling out of
> 
> https://bugs.webkit.org/show_bug.cgi?id=202561
> 
> The rollout https://trac.webkit.org/changeset/251602/webkit

I don't think rolling out this important bug fix because you had trouble rolling out a separate patch is appropriate.
Comment 11 Jiewen Tan 2019-10-25 17:05:38 PDT
Created attachment 381983 [details]
Patch for relanding
Comment 12 Jiewen Tan 2019-10-26 06:49:22 PDT
Comment on attachment 381983 [details]
Patch for relanding

The iOS EWS seems stuck. cq+ it regardless.
Comment 13 Jiewen Tan 2019-10-27 15:53:42 PDT
Committed r251645: <https://trac.webkit.org/changeset/251645>