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

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

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

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?

@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.

- 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
Projects
None yet
Development

No branches or pull requests

3 participants