Include context about whether the _WKAuthenticatorAssertionResponse came from the platform authenticator or a security key.
<rdar://problem/76554090>
Created attachment 428007 [details] Patch
Comment on attachment 428007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428007&action=review > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:40 > + static Ref<AuthenticatorAssertionResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& authenticatorData, Ref<ArrayBuffer>&& signature, RefPtr<ArrayBuffer>&& userHandle, Optional<AuthenticationExtensionsClientOutputs>&&, AuthenticatorResponse::Attachment); This is only used in the Web API. No need to add attachment to it. > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:36 > + static Ref<AuthenticatorAttestationResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& attestationObject, AuthenticatorResponse::Attachment); Ditto. > Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:194 > + if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), AuthenticatorResponse::Attachment::CrossPlatform)) { You cannot assume this. > Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:265 > + if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), AuthenticatorResponse::Attachment::CrossPlatform)) { Ditto. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:37 > +RefPtr<AuthenticatorResponse> AuthenticatorResponse::tryCreate(AuthenticatorResponseData&& data, Attachment attachment) It's only used in the Web API. No need for this. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:88 > +AuthenticatorResponse::Attachment AuthenticatorResponse::attachment() const You can do it in the header. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:46 > + enum class Attachment { Instead of making a new enum, use WebCore::AuthenticatorAttachment. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:71 > + Attachment m_attachment; This is only for the SPI not the Web API. Let's make it Optional such that it's not necessary to serialize it back to the web process. > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:139 > + return AuthenticatorAttestationResponse::create(credentialId, *attestationObject, AuthenticatorResponse::Attachment::CrossPlatform); Add a new parameter to this function and do it in the Authenticator layer. It's not guaranteed that only external authenticators will use this function. > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:176 > + response = AuthenticatorAssertionResponse::create(credentialId, authData, signature, userHandle, AuthenticatorResponse::Attachment::CrossPlatform); Ditto. > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:192 > + response = AuthenticatorAssertionResponse::create(credentialId, authData, signature, { }, AuthenticatorResponse::Attachment::CrossPlatform); Ditto. > Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:170 > + return AuthenticatorAttestationResponse::create(credentialId, attestationObject, AuthenticatorResponse::Attachment::CrossPlatform); Ditto. > Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:190 > + return AuthenticatorAssertionResponse::create(keyHandle, authData, signature, { }, AuthenticatorResponse::Attachment::CrossPlatform); Ditto.
Created attachment 429816 [details] Updated patch to replace the current one
Created attachment 429952 [details] Garrett's Patch (Rebaselined)
Comment on attachment 429952 [details] Garrett's Patch (Rebaselined) View in context: https://bugs.webkit.org/attachment.cgi?id=429952&action=review > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:40 > + static Ref<AuthenticatorAssertionResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& authenticatorData, Ref<ArrayBuffer>&& signature, RefPtr<ArrayBuffer>&& userHandle, Optional<AuthenticationExtensionsClientOutputs>&&, AuthenticatorAttachment); Jiewen pointed out this is only used by the WebAPI -- is there any reason to keep the unused AuthenticatorAttachment argument? > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:36 > + static Ref<AuthenticatorAttestationResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& attestationObject, AuthenticatorAttachment); Ditto. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:90 > + return m_attachment; Jiewen suggested you move this to the header: Any reason not to? > Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67 > + AuthenticatorAttachment m_attachment; You didn't address Jiewen's comment that this isn't needed in the Web layer, so serializing it back/forth to the WebContent process seems wasteful. Can we make it Optional<>?
(In reply to Brent Fulgham from comment #6) > You didn't address Jiewen's comment that this isn't needed in the Web layer, > so serializing it back/forth to the WebContent process seems wasteful. Can > we make it Optional<>? I'm new in this codebase, so this is probably just my lack of understanding, but what's being "wasted" here? Isn't it the same amount of space to encode an empty Optional<> as it is to encode an int? If we go to Optional<> route, we effectively end up with a partially initialized object which has more complex usage because you have to add code to handle not having a value, as opposed to guaranteeing it's always there. > Jiewen suggested you move this to the header: Any reason not to? I tried moving it to the header and ended up getting errors about an exported symbol being defined in multiple compilation units. My assumption was that you can't mix `WEBCORE_EXPORT` with a method defined in a header, but if I missed something I'm happy to move it :)
Created attachment 430030 [details] Patch (Rebase Attempt #2)
(In reply to Garrett Davidson from comment #7) > (In reply to Brent Fulgham from comment #6) > > > You didn't address Jiewen's comment that this isn't needed in the Web layer, > > so serializing it back/forth to the WebContent process seems wasteful. Can > > we make it Optional<>? > > I'm new in this codebase, so this is probably just my lack of understanding, > but what's being "wasted" here? Isn't it the same amount of space to encode > an empty Optional<> as it is to encode an int? If we go to Optional<> route, > we effectively end up with a partially initialized object which has more > complex usage because you have to add code to handle not having a value, as > opposed to guaranteeing it's always there. Fair enough. We encode a nullptr for this case, which would be the same size in IPC as an integer value. > > Jiewen suggested you move this to the header: Any reason not to? > > I tried moving it to the header and ended up getting errors about an > exported symbol being defined in multiple compilation units. My assumption > was that you can't mix `WEBCORE_EXPORT` with a method defined in a header, > but if I missed something I'm happy to move it :) Ah, yes. That's true. Inline and WEBCORE_EXPORT don't mix.
Comment on attachment 430030 [details] Patch (Rebase Attempt #2) View in context: https://bugs.webkit.org/attachment.cgi?id=430030&action=review > Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:30 > +#include "AuthenticatorAttachment.h" It would be better if we could forward-declare this, and include in the implementation file.
(In reply to Brent Fulgham from comment #10) > Comment on attachment 430030 [details] > Patch (Rebase Attempt #2) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430030&action=review > > > Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:30 > > +#include "AuthenticatorAttachment.h" > > It would be better if we could forward-declare this, and include in the > implementation file. The only thing in AuthenticatorAttachment.h is the AuthenticatorAttachment enum. Can that be forward declared?
Created attachment 430050 [details] Rebased patch with fixed tests
Comment on attachment 430050 [details] Rebased patch with fixed tests View in context: https://bugs.webkit.org/attachment.cgi?id=430050&action=review > Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67 > + AuthenticatorAttachment m_attachment; Since we're storing this, this would probably be a good time to reduce the size. Replace "enum class AuthenticatorAttachment" with "enum class AuthenticatorAttachment : bool", and you can also remove the EnumTraits in AuthenticatorAttachment.h > Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:75 > + handler({ }, (AuthenticatorAttachment)0, exception); This is actually AuthenticatorAttachment::Platform. Is that what you want? Maybe you want Optional<AuthenticatorAttachment> > Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:87 > + handler({ }, (AuthenticatorAttachment)0, exception); ditto > Tools/ChangeLog:1 > +2021-05-28 Garrett Davidson <davidson.garrettm@gmail.com> Is this the email address you want to use? > Tools/ChangeLog:11 > + Update the CTAP tests to specify the new attachment parameter. All of these tests > + assume a cross platform authenticator. Is it possible to add a test that uses AuthenticatorAttachment::Platform?
Comment on attachment 430050 [details] Rebased patch with fixed tests View in context: https://bugs.webkit.org/attachment.cgi?id=430050&action=review >> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67 >> + AuthenticatorAttachment m_attachment; > > Since we're storing this, this would probably be a good time to reduce the size. Replace "enum class AuthenticatorAttachment" with "enum class AuthenticatorAttachment : bool", and you can also remove the EnumTraits in AuthenticatorAttachment.h Please note that ": bool" does not depend on "enum" vs. "enum class". Could do that whether or not we change this to an enum class.
Created attachment 430291 [details] Garrett's Patch with forward declaration.
Comment on attachment 430291 [details] Garrett's Patch with forward declaration. View in context: https://bugs.webkit.org/attachment.cgi?id=430291&action=review Thanks for fixing the API tests. r=me, but waiting for EWS to complete. > Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:87 > + handler({ }, (AuthenticatorAttachment)0, exception); This should be a C++ style cast. Or should we just be hard-coding it as AuthenticatorAttachment::Platform?
Created attachment 430295 [details] Garrett's Patch with fwd declaration (and the cast)
Committed r278358 (238390@main): <https://commits.webkit.org/238390@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430295 [details].