Bug 183881

<rdar://problem/37258628>

Created attachment 336250 [details] Patch

Attachment 336250 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:188: Missing spaces around : [whitespace/init] [4] ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:205: Missing spaces around : [whitespace/init] [4] ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:157: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:430: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 336250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336250&action=review Looks like good progress. r=me, but please fix the minor nits (aside from the OptionSet suggestion). > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:53 > +const uint8_t makeCredentialFlags = 69; // UP, UV and AT are set. Oh, I see -- this is a bitmap: 0d69 == 0b01000101 We often use hex for bitmaps, just to help signal that we aren't dealing with a magic number -- it's a bit pattern. Alternatively, you could use a OptionSet<> and state which bits are on using an enum, which would be much clearer to read. But no need to do so in this patch. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:89 > + } counterUnion; I assume this will eventually by defined in a header somewhere for use elsewhere? I'm not keen on having a bunch of local struct definitions in the code. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:139 > + if (!excludeCredentialIds.isEmpty()) { I think this should be an early return, unless there are future steps that might need to happen even if there are no excludeCredentialIds. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:148 > + CFTypeRef attributesArrayRef = NULL; nullptr? > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:152 > + exceptionCallback({ UnknownError, ASCIILiteral("Unknown internal error.") }); This might need a bit more detail when we start trying to debug failed Keychain access. :-) > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:159 > + if (excludeCredentialIds.contains(String(reinterpret_cast<const char*>(nsCredentialId.bytes), nsCredentialId.length))) { Could we have cases where the application label is not ASCII characters? I guess String is smart enough to recognize utf8 encodings and do the right thing. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:401 > + allowCredentialIds.add(String(reinterpret_cast<const char*>(allowCredential.idVector.data()), allowCredential.idVector.size())); This is the second time we've seen this code. I feel like we need a String specialization that can take an NSData and convert to string. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:416 > + CFTypeRef attributesArrayRef = NULL; nullptr > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:420 > + exceptionCallback({ UnknownError, ASCIILiteral("Unknown internal error.") }); I feel like we could do a better job of error reporting here. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:432 > + if (allowCredentialIds.contains(String(reinterpret_cast<const char*>(nsCredentialId.bytes), nsCredentialId.length))) More NSData -> String! > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:459 > + NSData * nsCredentialId = selectedCredentialAttributes[(id)kSecAttrApplicationLabel]; Extra space before nsCredentialId. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:462 > + NSData * nsUserhandle = selectedCredentialAttributes[(id)kSecAttrApplicationTag]; Ditto whitespace. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:488 > + CFTypeRef privateKeyRef = NULL; nullptr. > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:500 > + CFErrorRef errorRef = NULL; nullptr.

Comment on attachment 336250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336250&action=review Thanks Brent for r+ my patch. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:53 >> +const uint8_t makeCredentialFlags = 69; // UP, UV and AT are set. > > Oh, I see -- this is a bitmap: 0d69 == 0b01000101 > > We often use hex for bitmaps, just to help signal that we aren't dealing with a magic number -- it's a bit pattern. > > Alternatively, you could use a OptionSet<> and state which bits are on using an enum, which would be much clearer to read. But no need to do so in this patch. Got it. Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:89 >> + } counterUnion; > > I assume this will eventually by defined in a header somewhere for use elsewhere? I'm not keen on having a bunch of local struct definitions in the code. Sure, I should move all constants to another header. Updated Bug 183534 to reflect this. Ideally, counter should be encoded as big endian. Since we couldn't remember counter at this moment, it is 0 all the time. It makes little and big endian no differences. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:139 >> + if (!excludeCredentialIds.isEmpty()) { > > I think this should be an early return, unless there are future steps that might need to happen even if there are no excludeCredentialIds. Actually, this branch is an early return. See L160. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:148 >> + CFTypeRef attributesArrayRef = NULL; > > nullptr? In conventional CFTypeRef initialization, it uses NULL following the C style. In WebKit though, occurrence of nullptr outweighs NULL for CFTypeRef initialization. Changed all initialization to use nullptr. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:152 >> + exceptionCallback({ UnknownError, ASCIILiteral("Unknown internal error.") }); > > This might need a bit more detail when we start trying to debug failed Keychain access. :-) It is intentional to not report too detailed error back to Web but keep it in local logs. Maybe I am too cautious? >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:159 >> + if (excludeCredentialIds.contains(String(reinterpret_cast<const char*>(nsCredentialId.bytes), nsCredentialId.length))) { > > Could we have cases where the application label is not ASCII characters? I guess String is smart enough to recognize utf8 encodings and do the right thing. The way I do this is just to use the hash function of String. So I treat the whole application label as an ASCII string. It doesn't matter what encoding application label is encoded. I should detail this in the ChangeLog as well. Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:401 >> + allowCredentialIds.add(String(reinterpret_cast<const char*>(allowCredential.idVector.data()), allowCredential.idVector.size())); > > This is the second time we've seen this code. I feel like we need a String specialization that can take an NSData and convert to string. Well, the algorithm the LocalAuthenticator::getAssertion and LocalAuthenticator::makeCredential find intersection are the same. However, the ways they use the algorithm is such different that it is hard, at least for me, to unify them into a helper function. I unified the way how hashset is produced. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:416 >> + CFTypeRef attributesArrayRef = NULL; > > nullptr Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:420 >> + exceptionCallback({ UnknownError, ASCIILiteral("Unknown internal error.") }); > > I feel like we could do a better job of error reporting here. See above. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:432 >> + if (allowCredentialIds.contains(String(reinterpret_cast<const char*>(nsCredentialId.bytes), nsCredentialId.length))) > > More NSData -> String! This is probably too subtle to optimize. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:459 >> + NSData * nsCredentialId = selectedCredentialAttributes[(id)kSecAttrApplicationLabel]; > > Extra space before nsCredentialId. Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:462 >> + NSData * nsUserhandle = selectedCredentialAttributes[(id)kSecAttrApplicationTag]; > > Ditto whitespace. Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:488 >> + CFTypeRef privateKeyRef = NULL; > > nullptr. Fixed. >> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:500 >> + CFErrorRef errorRef = NULL; > > nullptr. Fixed.

Created attachment 336614 [details] Patch for landing

Created attachment 336615 [details] Patch for landing

Attachment 336615 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:188: Missing spaces around : [whitespace/init] [4] ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:205: Missing spaces around : [whitespace/init] [4] ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:160: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:427: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 336615 [details] Patch for landing Clearing flags on attachment: 336615 Committed r230012: <https://trac.webkit.org/changeset/230012>