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

remove unimplemented extensions (was: Update index.bs) #1399

Merged
merged 8 commits into from Apr 15, 2020
Merged

remove unimplemented extensions (was: Update index.bs) #1399

merged 8 commits into from Apr 15, 2020

Conversation

ve7jtb
Copy link
Contributor

@ve7jtb ve7jtb commented Apr 1, 2020

This removes unimplemented extensions
Fixes #1386


Preview | Diff

Add lightning transport

Fixes #1261
Fix missing comma
This removes unimplimented extensions
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense to me. It will be much easier to get approval for the Level 2 spec without having to seek exceptions to the normal W3C criteria that spec features in a Recommendation must be implemented.

Note also that the deleted extensions aren't going away. They'll still be in the Level 1 spec and in the IANA registry. So if anyone ever wants to implement them, they're still there.

@equalsJeffH equalsJeffH changed the title Update index.bs remove unimplemented extensions (was: Update index.bs) Apr 1, 2020
index.bs Outdated
- WebAuthn Extension Identifier: uvm
- Description: This [=registration extension=] and [=authentication extension=] enables use of a user verification method.
The user verification method extension returns to the [=[WRP]=] which user verification methods (factors) were
used for the WebAuthn operation.
- Specification Document: Section [[#sctn-uvm-extension]] of this specification
<br/><br/>
- WebAuthn Extension Identifier: credProps
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this one should be deleted. It's the counterpart to rk=preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Based on discussions, this might be correct if this isn't listed by IANA.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added credProps back to the IANA registration.

@nadalin nadalin added this to the L2-WD-03 milestone Apr 1, 2020
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.

Overall LGTM, tho there's one minor issue:

  • there a Note: that now has a dangling link to user verification index. note begins with:

Note: Distinguishing natural persons depends in significant part...

We ought to figure out if we retain that Note or not and if so, what it ought to now say.

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

there's another dangling reference: [[#sctn-location-extension]] -- hence the build fails -- hopefully no more but shud check after fixing this one....

to build spec and get list of error msgs, you can do this on your *nix machine or cygwin terminal:

curl https://api.csswg.org/bikeshed/ -F file=@index.bs -F force=1 -F output=err

@equalsJeffH
Copy link
Contributor

there's another dangling reference: [[#sctn-location-extension]] -- hence the build fails
I have checked and with the [[#sctn-location-extension]] cross-reference removed the spec builds fine.

I suggest we remove the entire subsection entitled "13.4.1. Browser Permissions Framework and Extensions" because the only extant (in the L1 spec) extension requiring this is the "Location extension", and we are removing it from L2.

@ve7jtb
Copy link
Contributor Author

ve7jtb commented Apr 9, 2020

OK I will remove that section.

It will still be in L1 if anyone needs it.

That leaves an empty Security considerations for clients.

Should that empty section go as well?

The only extension using it is now gone.
@equalsJeffH equalsJeffH self-requested a review April 9, 2020 15:18
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 @ve7jtb !

Firehed added a commit to Firehed/webauthn-php that referenced this pull request Nov 13, 2023
While it's already possible to explicitly require the `userVerified`
flag during the registration and authentication ceremonies, it's
possible that a server may want to provide differing behavior when a
user was verified even if the ceremony expressed a preference of
`preferred` or even `discouraged`.

The common case here is to treat UV=1 as a two-factor auth, and UV=0 as
a single (possession) factor, and adjust permissions or a sign-in flow
accordingly. It's not strictly reliably accurate 100% of the time, and
this alone is insufficient to know _which_ factors were used, but it's a
close-enough assumption for the majority of use-cases. In any case, this
library imposes no requirements that servers even look at this, but it
provides the option.

Note that in theory the [`uvm`
extension](https://www.w3.org/TR/webauthn-2/#sctn-uvm-extension) should
provide similar information, MDN (as of writing) [doesn't
acknowledge](https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API/WebAuthn_extensions)
this value; I've had no success getting it to behave in any browser I've
tested. w3c/webauthn#1386 /
w3c/webauthn#1399 suggest it should continue to
exist, but in practice that seems not to be the case.
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.

Remove unimplemented extensions
6 participants