Bug 188624

Summary: [WebAuthn] Support NFC authenticators for iOS
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex.gaynor, benjamin, bfulgham, cdumez, chris, cmarcelo, daniel.stjean, dbates, ews, james, jiewen_tan, jschoi, me, paul.l.kehrer, richardoliver, sam, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201813
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
cdumez: review+, cdumez: commit-queue-
Patch for Landing jiewen_tan: review?

Description Jiewen Tan 2018-08-15 15:40:54 PDT
Support NFC authenticators for iOS.
Comment 1 Radar WebKit Bug Importer 2018-08-15 15:41:20 PDT
<rdar://problem/43354214>
Comment 2 Jiewen Tan 2019-08-20 13:13:38 PDT
Created attachment 376796 [details]
Patch
Comment 3 Jiewen Tan 2019-08-20 14:53:46 PDT
Created attachment 376812 [details]
Patch
Comment 4 Jiewen Tan 2019-08-20 15:01:40 PDT
Created attachment 376814 [details]
Patch
Comment 5 Jiewen Tan 2019-08-20 15:43:58 PDT
Created attachment 376823 [details]
Patch
Comment 6 Jiewen Tan 2019-08-20 16:36:13 PDT
Comment on attachment 376823 [details]
Patch

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

Will upload another patch without those two files after EWS runs.

> Source/WTF/wtf/Platform.h:1556
> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0

Oops. Somehow forgot to checkout this file.

> Source/WebKit/Platform/IPC/HandleMessage.h:116
> +//        ASSERT(decoder.isInvalid());

Oops. Somehow forgot to checkout this file.
Comment 7 Jiewen Tan 2019-08-20 16:45:16 PDT
Comment on attachment 376823 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
> +        // FIXME: Somehow elminiate the need for the copy?

elminiate => eliminate

> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:12
> +    function checkResult(credential)

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:38
> +    {

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-get-success-nfc.https.html:12
> +    {

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-get-success-nfc.https.html:29
> +    {

Should probably move this to util and share it between NFC and HID.
Comment 8 Jiewen Tan 2019-08-20 17:42:42 PDT
Comment on attachment 376823 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:72
> +        respondAsync(WTFMove(callback), (Vector<uint8_t>(apduResponse->data())));

Remove the outer parenthesis of Vector<uint8_t>(apduResponse->data()).
Comment 9 Brent Fulgham 2019-08-21 09:09:12 PDT
Comment on attachment 376823 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1556
>> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0
> 
> Oops. Somehow forgot to checkout this file.

LOl. r- to fix! :-)

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:74
> +        addResult = result.add(AuthenticatorTransport::Nfc);

Do we support NFC on Macs?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:44
> +// FIXME(200934)

"// FIXME(200934): Support NFCCTAP_GETRESPONSE"

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:84
> +    RunLoop::main().dispatch([callback = WTFMove(callback), response = WTFMove(response)] () mutable {

Please double-check that ResponseCallback (WTF::Function?) can be safely moved across threads without needing a cross-thread copy. I think if it's a WTF::Function and is constructed/destroyed on the same thread it's fine.

>> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:12
>> +    function checkResult(credential)
> 
> Should probably move this to util and share it between NFC and HID.

Good idea!
Comment 10 Jiewen Tan 2019-08-21 11:21:50 PDT
Comment on attachment 376823 [details]
Patch

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

Thanks Brent for reviewing the patch.

>>> Source/WTF/wtf/Platform.h:1556
>>> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0
>> 
>> Oops. Somehow forgot to checkout this file.
> 
> LOl. r- to fix! :-)

'webkit-patch upload ' tricks me every time for uploading uncommitted changes...

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:74
>> +        addResult = result.add(AuthenticatorTransport::Nfc);
> 
> Do we support NFC on Macs?

The framework is available but it doesn't do anything. I figure out it is better to have the NFHardwareManager to figure it out itself in runtime than a compile time flag .

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:44
>> +// FIXME(200934)
> 
> "// FIXME(200934): Support NFCCTAP_GETRESPONSE"

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:84
>> +    RunLoop::main().dispatch([callback = WTFMove(callback), response = WTFMove(response)] () mutable {
> 
> Please double-check that ResponseCallback (WTF::Function?) can be safely moved across threads without needing a cross-thread copy. I think if it's a WTF::Function and is constructed/destroyed on the same thread it's fine.

I didn't move the captured list objects across threads. I just make the response happen in the next runloop.
Comment 11 Jiewen Tan 2019-08-21 12:21:21 PDT
Created attachment 376904 [details]
Patch
Comment 12 Jiewen Tan 2019-08-21 14:49:03 PDT
Sadly, NearField doesn't present in iOS simulator. I will add a compile time flag for it.

Good news is my mocking tests work on macOS therefore the code is still covered by tests.
Comment 13 Jiewen Tan 2019-08-21 15:43:33 PDT
(In reply to Jiewen Tan from comment #12)
> Sadly, NearField doesn't present in iOS simulator. I will add a compile time
> flag for it.
> 
> Good news is my mocking tests work on macOS therefore the code is still
> covered by tests.

The easiest fix I guess is to skip those tests.
Comment 14 Jiewen Tan 2019-08-21 18:12:52 PDT
Created attachment 376960 [details]
Patch
Comment 15 Alex Christensen 2019-08-22 07:43:19 PDT
Comment on attachment 376960 [details]
Patch

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

> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);

Do we need to declare this deprecated property?

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:76
> +        auto addResult = result.add(AuthenticatorTransport::Nfc);

I'd put this outside the #if guard and call it something like nfcAddResult, or add it before Usb

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {

You could do session = retainPtr(session).
Also move weakThis into the lambda capture.
Just reducing a little ref churn.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.mm:53
> +    RunLoop::main().dispatch([connection = _connection, tags = retainTags] {

ditto about tags

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:52
> +        command.setCla(0x80);
> +        command.setIns(0x10);

Could these be given names and put in FidoConstants.h?
Comment 16 Jiewen Tan 2019-08-22 12:58:15 PDT
Comment on attachment 376960 [details]
Patch

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

Thanks Alex for reviewing the patch.

>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
>> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);
> 
> Do we need to declare this deprecated property?

I guess not. I have to conform the protocol in MockNfcService to do mock testing.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:76
>> +        auto addResult = result.add(AuthenticatorTransport::Nfc);
> 
> I'd put this outside the #if guard and call it something like nfcAddResult, or add it before Usb

Fixed. I add it before USB.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
> 
> You could do session = retainPtr(session).
> Also move weakThis into the lambda capture.
> Just reducing a little ref churn.

Fixed the retainPtr(session).

What do you mean by moving weakThis into the lambda capture which it is already in?

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.mm:53
>> +    RunLoop::main().dispatch([connection = _connection, tags = retainTags] {
> 
> ditto about tags

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:52
>> +        command.setIns(0x10);
> 
> Could these be given names and put in FidoConstants.h?

Definitely, yes.

Fixed.
Comment 17 Jiewen Tan 2019-08-22 12:59:14 PDT
Comment on attachment 376960 [details]
Patch

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

>>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
>>> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);
>> 
>> Do we need to declare this deprecated property?
> 
> I guess not. I have to conform the protocol in MockNfcService to do mock testing.

What I mean is we need to.
Comment 18 Alex Christensen 2019-08-22 13:00:31 PDT
(In reply to Jiewen Tan from comment #16)
> What do you mean by moving weakThis into the lambda capture which it is
> already in?
weakThis = WTFMove(weakThis)
This kind of pattern prevents one ref and one deref of the WeakPtrImpl
Comment 19 Jiewen Tan 2019-08-22 13:10:11 PDT
(In reply to Alex Christensen from comment #18)
> (In reply to Jiewen Tan from comment #16)
> > What do you mean by moving weakThis into the lambda capture which it is
> > already in?
> weakThis = WTFMove(weakThis)
> This kind of pattern prevents one ref and one deref of the WeakPtrImpl

I was not aware that WeakPtr uses a RefPtr to wrap the WeakPtrImpl. Thanks for the tips.
Comment 20 Chris Dumez 2019-08-22 13:44:05 PDT
Comment on attachment 376960 [details]
Patch

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

r=me with changes

> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:85
> +- (void) endSession;

Why the space before endSession? Same comment for several other below.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:682
> +        WebKit::MockWebAuthenticationConfiguration::Nfc nfc;

Why is this local needed? Why not use configuration.nfc directly below?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:687
> +        if (error == "no-tags")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:689
> +        if (error == "wrong-tag-type")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:691
> +        if (error == "no-connections")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:693
> +        if (error == "malicious-payload")

else if

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:79
> +    // FIXME(200932)

Please put in an actual comment, not just a number.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
> +        if (!m_service)

Why is this in the loop and not at the beginning of the method? Can m_service change while iterating?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h:50
> +    std::unique_ptr<CtapNfcDriver> m_driver;

We normally try to avoid protected data members. We put data members private and have protected getters / setters for them if needed.

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
>> 
>> You could do session = retainPtr(session).
>> Also move weakThis into the lambda capture.
>> Just reducing a little ref churn.
> 
> Fixed the retainPtr(session).
> 
> What do you mean by moving weakThis into the lambda capture which it is already in?

weakThis = WTFMove(weakThis)

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.h:31
> +#import <wtf/WeakPtr.h>

Does not appear to be needed in the header.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:98
> +using Mock = MockWebAuthenticationConfiguration::Nfc;

Mock seems too generic a name, especially now that we have unified builds.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:146
> +    return result;

Should this autorelease ?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
> +        // FIXME: Somehow eliminate the need for the copy?

You could add the following getter to ApduResponse:
Vector<uint8_t>& data() { return m_data; }

and then WTFMove(apduResponse->data()) below.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:56
> +        weakThis->continueAfterGetInfo(ptr, WTFMove(response));

what guarantees that ptr is not stale here? Also please find a better name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:62
> +void FidoService::continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& response)

ptr is not a good name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.h:45
> +    void continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& info);

ptr should be omitted.
Comment 21 Jiewen Tan 2019-08-22 14:16:20 PDT
Comment on attachment 376960 [details]
Patch

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

Thanks Chris for r+ this patch.

>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:85
>> +- (void) endSession;
> 
> Why the space before endSession? Same comment for several other below.

Good catch. I copy & paste it from the actual header...

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:682
>> +        WebKit::MockWebAuthenticationConfiguration::Nfc nfc;
> 
> Why is this local needed? Why not use configuration.nfc directly below?

configuration.nfc is an Optional. I guess I cannot directly use it?

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:687
>> +        if (error == "no-tags")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:689
>> +        if (error == "wrong-tag-type")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:691
>> +        if (error == "no-connections")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:693
>> +        if (error == "malicious-payload")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:79
>> +    // FIXME(200932)
> 
> Please put in an actual comment, not just a number.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
>> +        if (!m_service)
> 
> Why is this in the loop and not at the beginning of the method? Can m_service change while iterating?

I should check it ahead.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h:50
>> +    std::unique_ptr<CtapNfcDriver> m_driver;
> 
> We normally try to avoid protected data members. We put data members private and have protected getters / setters for them if needed.

Fixed.

>>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>>>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
>>> 
>>> You could do session = retainPtr(session).
>>> Also move weakThis into the lambda capture.
>>> Just reducing a little ref churn.
>> 
>> Fixed the retainPtr(session).
>> 
>> What do you mean by moving weakThis into the lambda capture which it is already in?
> 
> weakThis = WTFMove(weakThis)

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.h:31
>> +#import <wtf/WeakPtr.h>
> 
> Does not appear to be needed in the header.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:98
>> +using Mock = MockWebAuthenticationConfiguration::Nfc;
> 
> Mock seems too generic a name, especially now that we have unified builds.

Changed to MockNfc.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:146
>> +    return result;
> 
> Should this autorelease ?

Maybe?

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
>> +        // FIXME: Somehow eliminate the need for the copy?
> 
> You could add the following getter to ApduResponse:
> Vector<uint8_t>& data() { return m_data; }
> 
> and then WTFMove(apduResponse->data()) below.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:56
>> +        weakThis->continueAfterGetInfo(ptr, WTFMove(response));
> 
> what guarantees that ptr is not stale here? Also please find a better name.

The ptr is the key to the hashset. Therefore, it doesn't matter whether it is stale or not.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:62
>> +void FidoService::continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& response)
> 
> ptr is not a good name.

id?

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.h:45
>> +    void continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& info);
> 
> ptr should be omitted.

Fixed.
Comment 22 Jiewen Tan 2019-08-22 14:45:09 PDT
Created attachment 377048 [details]
Patch for Landing
Comment 23 Build Bot 2019-08-22 14:48:11 PDT
Attachment 377048 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webauthn/apdu/ApduResponse.h:64:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Jiewen Tan 2019-08-23 11:56:26 PDT
Committed r249059: <https://trac.webkit.org/changeset/249059>
Comment 25 Jiewen Tan 2019-08-23 14:22:16 PDT
A build fix:
Committed r249068: <https://trac.webkit.org/changeset/249068>
Comment 27 Brent Fulgham 2019-08-26 09:48:39 PDT
(In reply to Truitt Savell from comment #26)
> It looks like two of the new tests added in
> https://trac.webkit.org/changeset/249059/webkit are failing 
> 
> http/wpt/webauthn/public-key-credential-create-success-nfc.https.html
> http/wpt/webauthn/public-key-credential-get-success-nfc.https.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> create-success-nfc.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> credential-get-success-nfc.https.html
> 
> Diff:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/http/wpt/webauthn/public-key-credential-create-success-nfc.
> https-diff.txt
> 
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/http/wpt/webauthn/public-key-credential-get-success-nfc.
> https-diff.txt

They might need to be skipped on older OS's.
Comment 28 Jiewen Tan 2019-08-26 11:42:33 PDT
(In reply to Brent Fulgham from comment #27)
> (In reply to Truitt Savell from comment #26)
> > It looks like two of the new tests added in
> > https://trac.webkit.org/changeset/249059/webkit are failing 
> > 
> > http/wpt/webauthn/public-key-credential-create-success-nfc.https.html
> > http/wpt/webauthn/public-key-credential-get-success-nfc.https.html
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> > create-success-nfc.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> > credential-get-success-nfc.https.html
> > 
> > Diff:
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/http/wpt/webauthn/public-key-credential-create-success-nfc.
> > https-diff.txt
> > 
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/http/wpt/webauthn/public-key-credential-get-success-nfc.
> > https-diff.txt
> 
> They might need to be skipped on older OS's.

Yes, here we go:
Committed r249107: <https://trac.webkit.org/changeset/249107>