[webkit-reviews] review granted: [Bug 183881] [WebAuthN] Implement authenticatorGetAssertion : [Attachment 336250] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 183881: [WebAuthN] Implement authenticatorGetAssertion
https://bugs.webkit.org/show_bug.cgi?id=183881

Attachment 336250: Patch

https://bugs.webkit.org/attachment.cgi?id=336250&action=review




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 336250
  --> https://bugs.webkit.org/attachment.cgi?id=336250
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.