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

Adding Revision history section #2224

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

simoneonofri
Copy link
Contributor

@simoneonofri simoneonofri commented Jan 8, 2025

adding revision history section

Closes: #2223


Preview | Diff

@timcappalli timcappalli self-requested a review January 8, 2025 20:09
@sbweeden sbweeden self-requested a review January 8, 2025 20:09
index.bs Outdated Show resolved Hide resolved
index.bs Outdated

This section contains the substantive changes that have been made to this specification over time.

## Changes since Web Authentication Level 2 [[webauthn-2-20210408]] {#changes-since-l2}
Copy link
Member

Choose a reason for hiding this comment

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

Missing ## here causes the title to render as 18.1. Changes since Web Authentication Level 2 [webauthn-2-20210408] {#changes-since-l2}.

Suggested change
## Changes since Web Authentication Level 2 [[webauthn-2-20210408]] {#changes-since-l2}
## Changes since Web Authentication Level 2 [[webauthn-2-20210408]] ## {#changes-since-l2}

Copy link
Member

Choose a reason for hiding this comment

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

Also, even with this change the title is rendered as 18.1. Changes since Web Authentication Level 2 [webauthn-2-20210408]. The [webauthn-2-20210408] looks quite awkward to me, especially in the table of contents. Should we move the link into the section body or maybe fold the hyperlink into the "Web Authentication Level 2" text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emlun thank you. Ok to move on the link section (I was reusing the piece of code from another part of the spec)

Copy link
Member

Choose a reason for hiding this comment

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

Ok! Hm, it occurred to me now that the title "changes since level 2" may become awkward once we release L4. Would "Changes in Web Authentication Level 3" be a suitable title? Is there a process document describing what this section should look like?

index.bs Outdated
- Conditional mediation for get: [[#sctn-getAssertion]]
- Conditional mediation for create: [[#sctn-createCredential]]
- Added topOrigin: [[#dictionary-client-data]]
- Create operations in an iframe: [[#sctn-createCredential]]
Copy link
Member

Choose a reason for hiding this comment

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

I reckon this should also reference section §5.10. Using Web Authentication within iframe elements; see also #2229

Copy link
Member

Choose a reason for hiding this comment

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

This would be covered by meta-PR #2230.

Copy link
Contributor

Choose a reason for hiding this comment

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

Section 5.10. Using Web Authentication within iframe elements existed as-is in Level 2, so it's not a change. Therefore I don't think it should be added to this list.

Copy link
Member

Choose a reason for hiding this comment

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

As is right now, yes, but as noted in issue #2229 this section needs to be updated to reflect that create() is now also allowed in cross-origin iframes (and requires the respective feature policy).

Copy link
Member

Choose a reason for hiding this comment

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

PR #2231 would make the aforementioned update.

@selfissued
Copy link
Contributor

We should add the tokenBinding field in CollectedClientData was changed to [RESERVED].

Proposed changes from review of PR 2224
Comment on lines +9884 to +9885
## Changes since Web Authentication Level 2 [[webauthn-2-20210408]] ## {#changes-since-l2}

Copy link
Member

Choose a reason for hiding this comment

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

Proposing change as discussed in #2224 (comment):

Suggested change
## Changes since Web Authentication Level 2 [[webauthn-2-20210408]] ## {#changes-since-l2}
## Changes in Web Authentication Level 3 ## {#changes-l3}
The following changes were made between Web Authentication Level 2 [[webauthn-2-20210408]] and Level 3.

Add more to L3 revision history
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.

Ok with me to merge and iterate going forward.

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.

WD Requirement: adding Revision History section
6 participants