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.

Sign up for GitHub

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

Jump to bottom

exclude/allowCredentials state different defaults in definition vs description #1346

Closed
emlun opened this issue Nov 18, 2019 · 7 comments
Closed

exclude/allowCredentials state different defaults in definition vs description #1346

emlun opened this issue Nov 18, 2019 · 7 comments
Assignees
@emlun
Labels
subtype:algorithms/WebIDL type:editorial
Milestone
L2-WD-03 Feature ...

Comments

@emlun
Copy link
Member

emlun commented Nov 18, 2019

The definitions state:

    sequence<PublicKeyCredentialDescriptor>      excludeCredentials = [];
    sequence<PublicKeyCredentialDescriptor>      allowCredentials = [];

But the descriptions state:

excludeCredentials, of type sequence<PublicKeyCredentialDescriptor>, defaulting to None
allowCredentials, of type sequence<PublicKeyCredentialDescriptor>, defaulting to None

Which is correct?
@nadalin nadalin added this to the L2-WD-03 milestone Nov 27, 2019
@equalsJeffH
Copy link
Contributor

I think the definitions are correct, IIRC. bikeshed perhaps is misunderstanding what to state in the descriptions. this may have been caused by a webidl-driven auto-spec edit, have to look into that....

@emlun
Copy link
Member Author

emlun commented Jan 13, 2020

Looks like this goes back to the very beginnings of the spec, it's not a recent change: https://www.w3.org/TR/2017/WD-webauthn-20170505/#dom-publickeycredentialrequestoptions-allowlist

The earliest spec version with the [] default is https://www.w3.org/TR/2017/WD-webauthn-20170216/#dom-assertionoptions-allowlist, but that doesn't use the same definition header.

@emlun
Copy link
Member Author

emlun commented Jan 15, 2020

Digging deeper, this seems like a bug in Bikeshed: speced/bikeshed#1592

@emlun emlun added the stat:Blocked Prerequisites are not yet satisfied label Jan 15, 2020
@emlun emlun removed the stat:Blocked Prerequisites are not yet satisfied label Jan 27, 2020
@emlun
Copy link
Member Author

emlun commented Jan 27, 2020

This is now fixed upstream, so any builds with a fresh Bikeshed installation should be good. I'll keep an eye on the editor's draft and close this once the fix shows up there.

@equalsJeffH
Copy link
Contributor

thx @emlun, do we need to manually update the travis machinery to use the latest bikeshed?

@emlun
Copy link
Member Author

emlun commented Jan 29, 2020

I don't think so, the Travis build should be using the latest Bikeshed version as far as I can tell. Unless the install step is cached, but it looks like it's not.

webauthn/.travis.yml

Line 19 in 6349d24

- git clone --depth=100 --branch=master https://github.com/tabatkins/bikeshed.git ./bikeshed

@emlun
Copy link
Member Author

emlun commented Jan 30, 2020

This is now fixed in the working draft.

@emlun emlun closed this as completed Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emlun emlun

Labels
subtype:algorithms/WebIDL type:editorial
Projects
None yet
Milestone
L2-WD-03 Feature Complete
Development

No branches or pull requests
3 participants
@equalsJeffH @emlun @nadalin