RESOLVED FIXED 200932
[WebAuthn] Warn users when multiple NFC tags present
https://bugs.webkit.org/show_bug.cgi?id=200932
Summary [WebAuthn] Warn users when multiple NFC tags present
Jiewen Tan
Reported 2019-08-20 12:07:07 PDT
Allow user to select a different NFC tag if multiple present.
Attachments
Patch (44.71 KB, patch)
2019-10-21 22:50 PDT, Jiewen Tan
no flags
Patch (44.73 KB, patch)
2019-10-22 02:06 PDT, Jiewen Tan
bfulgham: review+
Patch for relanding (44.44 KB, patch)
2019-10-25 17:05 PDT, Jiewen Tan
no flags
Jiewen Tan
Comment 1 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.
Radar WebKit Bug Importer
Comment 2 2019-08-30 11:49:34 PDT
Jiewen Tan
Comment 3 2019-10-18 17:24:30 PDT
*** Bug 201017 has been marked as a duplicate of this bug. ***
Jiewen Tan
Comment 4 2019-10-21 22:50:40 PDT
Jiewen Tan
Comment 5 2019-10-22 02:06:18 PDT
Brent Fulgham
Comment 6 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?)
Jiewen Tan
Comment 7 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.
Jiewen Tan
Comment 8 2019-10-23 14:58:25 PDT
Matt Lewis
Comment 9 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
Brent Fulgham
Comment 10 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.
Jiewen Tan
Comment 11 2019-10-25 17:05:38 PDT
Created attachment 381983 [details] Patch for relanding
Jiewen Tan
Comment 12 2019-10-26 06:49:22 PDT
Comment on attachment 381983 [details] Patch for relanding The iOS EWS seems stuck. cq+ it regardless.
Jiewen Tan
Comment 13 2019-10-27 15:53:42 PDT
Note You need to log in before you can comment on or make changes to this bug.