Bug 188624

<rdar://problem/43354214>

Created attachment 376796 [details] Patch

Created attachment 376812 [details] Patch

Created attachment 376814 [details] Patch

Created attachment 376823 [details] Patch

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

Created attachment 376904 [details] Patch

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.

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

Created attachment 376960 [details] Patch

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

(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

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

Created attachment 377048 [details] Patch for Landing

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.

Committed r249059: <https://trac.webkit.org/changeset/249059>

A build fix: Committed r249068: <https://trac.webkit.org/changeset/249068>

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

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

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