Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strongly type client extension inputs and outputs #765

Conversation

selfissued
Copy link
Contributor

@selfissued selfissued commented Jan 26, 2018

Client extension inputs and outputs are now strongly typed. Several individual extensions were also cleaned up so that their JavaScript and CBOR representations directly correspond to one another. The example extension was so irregular and ad-hoc as to actually be a terrible example. It was therefore deleted. The existing extensions now should serve as good examples.

Fixes #346
Fixes #626
Partially fixes #713
Partially fixes #738


Preview | Diff

@agl
Copy link
Contributor

agl commented Jan 30, 2018

The type AuthenticationExtensionsAuthenticatorOutputs seems to be unused. Otherwise LGTM.

@selfissued
Copy link
Contributor Author

The AuthenticatorExtensionsAuthenticationOutputs type is there to be parallel to the other three types. But you're right looking at it @agl - it's not used. Rather, these outputs appear in the extensions field of https://w3c.github.io/webauthn/#authenticator-data. I'll remove it.

@selfissued selfissued requested a review from agl January 31, 2018 06:08
@selfissued
Copy link
Contributor Author

@agl can you please approve this PR now that the unused typedef has been removed? Thanks.

index.bs Outdated
:: Returns a JSON array of 3-element arrays of numbers that encodes the factors in the authenticator extension output.
<xmp class="idl">
typedef sequence<unsigned long> uvmEntry;
typedef sequence<uvmEntry> uvmEntries;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should these typedefs' names also have initial uppercase like the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the careful read @emlun .

@kpaulh
Copy link
Contributor

kpaulh commented Jan 31, 2018

I concur with agl, this looks good to me as well.

Copy link
Contributor

@akshayku akshayku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants