Summary: | [WebAuthN] Import U2F command/response converters from Chromium | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||
Component: | WebCore Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, ews-watchlist, jiewen_tan, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 181943 | ||||||||||
Attachments: |
|
Description
Jiewen Tan
2019-01-04 12:05:03 PST
Created attachment 358358 [details]
Patch
Attachment 358358 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/webauthn/WebAuthenticationConstants.h:41: P_256 is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 358399 [details]
Patch
Attachment 358399 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/webauthn/WebAuthenticationConstants.h:41: P_256 is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358399&action=review R=me > Source/WebCore/ChangeLog:20 > + Besides importing stuffs from Chroimum, it also gathers a bunch of constants and helper functions into WebAuthenticationConstants.h Besides importing stuff from Chromium ... > Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:90 > + if (parameters.alg == COSE::ES256) This really seems like something a std::find would work for. But it’s fine as-is. > Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:114 > + return constructU2fSignCommand(produceRpIdHash(request.rp.id), clientDataHash, keyHandle.idVector, true /* checkOnly */); The need for this comment indicates this should be an enumeration! But since this is imported code, land as-is and then post a patch to clean up the interface. > Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:71 > + x.append(u2fData.data() + pos, ES256FieldElementLength); Can the vector be constructed directly with this data? Or at least sized properly, rather than having a separate allocation event, then a resize as we append. This is true for all such cases. > Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:34 > +#include "GPURenderPipeline.h" Is this needed as part of this patch? Was it causing a build error for you? Comment on attachment 358399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358399&action=review Thanks Brent for r+ this patch. >> Source/WebCore/ChangeLog:20 >> + Besides importing stuffs from Chroimum, it also gathers a bunch of constants and helper functions into WebAuthenticationConstants.h > > Besides importing stuff from Chromium ... Fixed. >> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:90 >> + if (parameters.alg == COSE::ES256) > > This really seems like something a std::find would work for. But it’s fine as-is. You are right! Fixed. >> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:114 >> + return constructU2fSignCommand(produceRpIdHash(request.rp.id), clientDataHash, keyHandle.idVector, true /* checkOnly */); > > The need for this comment indicates this should be an enumeration! But since this is imported code, land as-is and then post a patch to clean up the interface. Sure. >> Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:71 >> + x.append(u2fData.data() + pos, ES256FieldElementLength); > > Can the vector be constructed directly with this data? Or at least sized properly, rather than having a separate allocation event, then a resize as we append. > > This is true for all such cases. It looks like that the default cntr of Vector does nothing. Therefore we will have only one allocation event when we append. >> Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:34 >> +#include "GPURenderPipeline.h" > > Is this needed as part of this patch? Was it causing a build error for you? Some kind of unified source build magic... Created attachment 358470 [details]
Patch for landing
Comment on attachment 358470 [details] Patch for landing Clearing flags on attachment: 358470 Committed r239665: <https://trac.webkit.org/changeset/239665> |