Bug 190783 – [WebAuthn] Combine AuthenticatorResponse and PublicKeyCredentialData

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

We probably should combine AuthenticatorResponse with PublicKeyCredentialData, and customize AuthenticatorResponse more to include fields like numberOfCredentials from FIDO CTAP authenticators. Thus, AuthenticatorResponse will not serve merely for IDL interfaces.

[Jiewen Tan]

The difficulty is that IPC can't serialize ref counted objects. Therefore, we might still need a separate AuthenticatorResponseData struct that can be passed around different processes.

[Radar WebKit Bug Importer]

<rdar://problem/57781183>

[Jiewen Tan]

Created attachment 385239 [details]
Patch

[Jiewen Tan]

The layout test failure doesn't seem to be related:
http/tests/cache-storage/page-cache-domcache-pending-promise.html.

[Brent Fulgham]

Comment on attachment 385239 [details]
Patch

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

Looks good! r=me

> Source/WebCore/ChangeLog:12
> +        all different type of responses from authenticators anymore. For example, authenticatorGetNextAssertion

all THE different

> Source/WebCore/ChangeLog:13
> +        depends on numberOfCredentials member from authenticatorGetAssertion response to function, but

depends on THE numberOfCredentials ...

> Source/WebCore/ChangeLog:14
> +        numberOfCredentials is not used anywhere else. Therefore, a polymorphism type is needed to

Therefore, a POLYMORPHIC type is needed to ...

> Source/WebCore/ChangeLog:19
> +        IPC. To solve this, AuthenticatorResponseData (PublicKeyCredentialData) is kept to be an intermediate type

is kept AS an intermediate type ...

> Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientOutputs.h:30
> +#include <wtf/text/WTFString.h>

Is WTFString actually needed here? No string appears in the header.

Seems more likely we would need <wtf/Optional.h>

> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:45
> +    RefPtr<ArrayBuffer> userhandle;

I don't think it's wise to rely on case difference to distinguish between 'userhandle' and 'userHandle'.

I suggest 'userHandleData' and 'userHandle'.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:55
> +// FIXME: Find a way to reduce the unnecessary copies.

You should file a bugzilla or we will never remember to do this!

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:55
> +    void setClientDataJSON(Ref<ArrayBuffer>&&);

Why isn't this inlined like 'setExtensions'? Seems like we should be consistent (all in the implementation file, or all in the header).

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019 maybe?

[Jiewen Tan]

Comment on attachment 385239 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebCore/ChangeLog:12
>> +        all different type of responses from authenticators anymore. For example, authenticatorGetNextAssertion
> 
> all THE different

Fixed.

>> Source/WebCore/ChangeLog:13
>> +        depends on numberOfCredentials member from authenticatorGetAssertion response to function, but
> 
> depends on THE numberOfCredentials ...

Fixed.

>> Source/WebCore/ChangeLog:14
>> +        numberOfCredentials is not used anywhere else. Therefore, a polymorphism type is needed to
> 
> Therefore, a POLYMORPHIC type is needed to ...

Fixed.

>> Source/WebCore/ChangeLog:19
>> +        IPC. To solve this, AuthenticatorResponseData (PublicKeyCredentialData) is kept to be an intermediate type
> 
> is kept AS an intermediate type ...

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientOutputs.h:30
>> +#include <wtf/text/WTFString.h>
> 
> Is WTFString actually needed here? No string appears in the header.
> 
> Seems more likely we would need <wtf/Optional.h>

Removed.

>> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:45
>> +    RefPtr<ArrayBuffer> userhandle;
> 
> I don't think it's wise to rely on case difference to distinguish between 'userhandle' and 'userHandle'.
> 
> I suggest 'userHandleData' and 'userHandle'.

Right, I was somehow lazy.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:42
> +struct AuthenticatorResponseData;

Moved to the top.

>> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:55
>> +// FIXME: Find a way to reduce the unnecessary copies.
> 
> You should file a bugzilla or we will never remember to do this!

I just removed the FIXME actually as I don't think it worths optimizing.

>> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:55
>> +    void setClientDataJSON(Ref<ArrayBuffer>&&);
> 
> Why isn't this inlined like 'setExtensions'? Seems like we should be consistent (all in the implementation file, or all in the header).

The considerations here is to limit the access to certain methods given this is a private header. I moved all to the implementations and use WEBCORE_EXPORT for rawId and setExtensions.

>> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:2
>> + * Copyright (C) 2018 Apple Inc. All rights reserved.
> 
> 2019 maybe?

Yes!

[Jiewen Tan]

Created attachment 385441 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 385441 [details]
Patch for landing

Clearing flags on attachment: 385441

Committed r253398: <https://trac.webkit.org/changeset/253398>