[webkit-reviews] review denied: [Bug 183527] [WebAuthN] Implement authenticatorMakeCredential : [Attachment 335911] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 335911: Patch

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




--- Comment #43 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 335911
  --> https://bugs.webkit.org/attachment.cgi?id=335911
Patch

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

I think this looks pretty good, but there are a few things that need to be
tweaked before it's ready. r- to fix the two crashes I mentioned, and to
consider whether CrossThreadTask/CrossThreadCopy resolves your issues.

> Source/WebCore/ChangeLog:21
> +	   the WebAuthN API, and thus it is moved to WebCore to perform unit
tesing flavor API tests. This is not enoguh as it

... "This is not enoUGh as it" (you transposed two characters).

> Source/WebCore/ChangeLog:22
> +	   can't test message exchange between the UI and Web processes. We
will address this issue later.

By later here, do you mean in a follow-up patch? I think you should just say
that: "We will address this in a subsequent patch."

> Source/WebCore/ChangeLog:23
> +	   3. More on testing. The attestation process is abstracted into a
protected method such that the testing enviroment can

I would use a colon here: "More on testing: The attestation ..."

> Source/WebCore/ChangeLog:24
> +	   override it with self attestation as network access is restricted in
WebKit testing enviroment. Also, swizzlers of

is restricted in THE WebKit testing environment.

> Source/WebCore/ChangeLog:26
> +	   4. More on testing. The actual Apple attestation can only happen in
real device and with network access, Therefore

Ditto on colon use. "More on testing: The actual Apple ...".

'Therefore' should not be capitalized in the middle of a sentence.

> Source/WebCore/ChangeLog:30
> +		   This method is the one does all the magic.

I'd move this text to the same line as "makeCredential():"

> Source/WebCore/ChangeLog:38
> +	   6. Even though files are of .mm format, they are written in a way
that mix NS, CF and C++ types. Here is the rule:

in a way that MIXES NS, CF, ...

> Source/WebCore/ChangeLog:47
> +	   (WebCore::getIdFromAttestationObject): Deleted.

Can you say why it's okay to delete this ? Do we now use DeviceIdentity
framework to support the same task?

> Source/WebCore/Modules/webauthn/COSEConstants.h:29
> +

I suggest a comment:

// See RFC 8152 - CBOR Object Signing and Encryption
<https://tools.ietf.org/html/rfc8152>

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:74
> +    // It only copies other's rp and user, which are used in another thread.

I don't understand this comment. It seems like you are warning people not to
use it, perhaps because it is incomplete or lacking features you need for the
future.

If so, I would put a FIXME() comment here indicating that it will be revised in
a subsequent patch.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:48
> +const uint8_t authenticatorDataFlags = 69;

I don't understand what this is, or why it has the value 69, but I assume it's
correct.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:134
> +    auto* localHash = new Vector<uint8_t>(hash);

Can you use CrossThreadTask for this? Or at least use CrossThreadCopy to get
the data across?

Is this outer function operating on Main? I suggest you ASSERT(isMainThread())
outside the closure for 'evalutePolicy'.

Also: This is Objective C++, so you should do the asterisks backwards:

auto *localCallback
auto *localException...
etc.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:136
> +    [context evaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics
localizedReason:reason reply:^(BOOL success, NSError *error) {

... and ASSERT(!isMainThread()) in here.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:163
> +		   LOG_ERROR("Couldn't attestate: %@", error);

"Couldn't attest: %@"

>
Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:5
6
> +	   exceptionReply(messageId, { WebCore::NotAllowedError,
ASCIILiteral("No avaliable authenticators.") });

Shouldn't you early return here? You go on and dereference m_authenticator
later.

>
Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:7
8
> +    isUserVerifyingPlatformAuthenticatorAvailableReply(messageId,
m_authenticator->isAvailable());

Shouldn't you have an else here? If m_authenticator was just false, you are
going to crash here!