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

Fix #1268 Type transport strings as DOMStrings. #1275

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

agl
Copy link
Contributor

@agl agl commented Aug 13, 2019

Currently transports are represented as an enum. However, WebIDL has
strict enums. (I.e. an RP which sent an unrecognised transport would
make the whole structure unparsable.) This means that every time we add
a transport, we break all existing browsers.

In order to address this, this change retypes transports as plain
DOMStrings. The AuthenticatorTransport enum still exists, but now only
as documentation and registry — not as a factor in type-checking.

fixes #1268


Preview | Diff

Currently transports are represented as an enum. However, WebIDL has
strict enums. (I.e. an RP which sent an unrecognised transport would
make the whole structure unparsable.) This means that every time we add
a transport, we break all existing browsers.

In order to address this, this change retypes transports as plain
DOMStrings. The AuthenticatorTransport enum still exists, but now only
as documentation and registry — not as a factor in type-checking.
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

LGTM! (except for an editorial detail)

@equalsJeffH equalsJeffH added this to the L2-WD-02 milestone Aug 14, 2019
@jcjones jcjones changed the title Type transport strings as DOMStrings. Fix #1268 Type transport strings as DOMStrings. Aug 14, 2019
@nadalin nadalin requested review from akshayku and jcjones August 14, 2019 19:33
@jcjones
Copy link
Contributor

jcjones commented Aug 14, 2019

This is a fix for #1268.

Copy link
Contributor

@jcjones jcjones left a comment

Choose a reason for hiding this comment

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

Looks right to me.

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

LGTM, thx. Suggested modest edits below.

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

LGTM!

@equalsJeffH
Copy link
Contributor

on 21-08-2019 call, this is ready to land modulo re-review & lgtm from @emlun

@akshayku akshayku merged commit b75aef3 into w3c:master Aug 28, 2019
WebAuthnBot pushed a commit that referenced this pull request Aug 28, 2019
emlun added a commit to Yubico/java-webauthn-server that referenced this pull request Nov 1, 2019
Changes:

- `RelyingParty` now makes an immutable copy of the `origins` argument,
  instead of storing a reference to a possibly mutable value.
- The enum `AuthenticatorTransport` has been replaced by a value class
  containing methods and value constants equivalent to the previous
  enum.
- The return type of `PublicKeyCredentialDescriptor.getTransports()` is
  now a `SortedSet` instead of `Set`. The builder still accepts a plain
  `Set`.
- Registration ceremony now verifies that the returned credential public
  key matches one of the algorithms specified in
  `RelyingParty.preferredPubkeyParams` and can be successfully parsed.

New features:

- Origin matching can now be relaxed via two new `RelyingParty` options:
  - `allowOriginPort` (default `false`): Allow any port number in the
    origin
  - `allowOriginSubdomain` (default `false`): Allow any subdomain of any
    origin listed in `RelyingParty.origins`
  - See JavaDoc for details and examples.
- The new `AuthenticatorTransport` can now contain any string value as
  the transport identifier, as required in the editor's draft of the L2
  spec. See: w3c/webauthn#1275
- Added support for RS1 credentials. Registration of RS1 credentials is
  not enabled by default, but can be enabled by setting
  `RelyingParty.preferredPubKeyCredParams` to a list containing
  `PublicKeyCredentialParameters.RS1`.
  - New constant `PublicKeyCredentialParameters.RS1`
  - New constant `COSEAlgorithmIdentifier.RS1`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change AuthenticatorTransport to be other than an enum
6 participants