Bug 200932 – [WebAuthn] Warn users when multiple NFC tags present

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

[Jiewen Tan]

Allow user to select a different NFC tag if multiple present.

[Jiewen Tan]

A solution here is when multiple tags are detected, we could advise users to pick one to use in the UI.

[Radar WebKit Bug Importer]

<rdar://problem/54890736>

[Jiewen Tan]

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

[Jiewen Tan]

Created attachment 381511 [details]
Patch

[Jiewen Tan]

Created attachment 381533 [details]
Patch

[Brent Fulgham]

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

[Jiewen Tan]

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.

[Jiewen Tan]

Committed r251500: <https://trac.webkit.org/changeset/251500>

[Matt Lewis]

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

[Brent Fulgham]

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

[Jiewen Tan]

Created attachment 381983 [details]
Patch for relanding

[Jiewen Tan]

Comment on attachment 381983 [details]
Patch for relanding

The iOS EWS seems stuck. cq+ it regardless.

[Jiewen Tan]

Committed r251645: <https://trac.webkit.org/changeset/251645>