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

Address description of uses, and requirements for supplying userHandle #1914

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Address description of uses, and requirements for supplying userHandle #1914

merged 5 commits into from
Aug 16, 2023

Conversation

sbweeden
Copy link
Contributor

@sbweeden sbweeden commented Jun 29, 2023 •
edited by pr-preview bot

Explicitly require userHandle to be supplied during assertion ceremonies with empty allowCredentials list and enhance description and uses of userHandle.

Preview | Diff

@sbweeden
Explicitly require userHandle to be supplied during assertion ceremon…
0553e31 
…ies with empty allowCredentials list and enhance description and uses of userHandle
@sbweeden sbweeden self-assigned this Jun 29, 2023
@sbweeden
Copy link
Contributor Author

Address both #1892 and #1909

This was linked to issues Jun 29, 2023
Require non-null userHandle when allowCredentials is empty? #1892
Closed
Revisit description of userHandle #1909
Closed
emlun
emlun approved these changes Jul 4, 2023
View reviewed changes
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.

Looks good to me, save for a few minor comments (mostly trailing whitespace). Thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
sbweeden and others added 4 commits July 5, 2023 11:36
@sbweeden @emlun
Accept suggested from @emlun
ce3e33e 
Co-authored-by: Emil Lundberg <emil@emlun.se>
@sbweeden @emlun
Accept suggested from @emlun
39c1354 
remove whitespace

Co-authored-by: Emil Lundberg <emil@emlun.se>
@sbweeden @emlun
Acc
836dfb5 
remove whitespace

Co-authored-by: Emil Lundberg <emil@emlun.se>
@sbweeden @emlun
Accept suggested from @emlun
a165fab 
Remove whitespace

Co-authored-by: Emil Lundberg <emil@emlun.se>
@sbweeden
Copy link
Contributor Author

sbweeden commented Jul 5, 2023

Looks good to me, save for a few minor comments (mostly trailing whitespace). Thanks!

Accepted all suggestions - thanks for the review.

@emlun
Copy link
Member

emlun commented Jul 5, 2023

Note: this adds a new normative requirement for clients to ignore empty-allowCredentials responses without user handle. I therefore tagged this with the "technical" label.

MasterKale
MasterKale reviewed Jul 5, 2023
View reviewed changes
index.bs Show resolved Hide resolved
timcappalli
timcappalli approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

lgtm

@nadalin nadalin requested a review from akshayku July 12, 2023 19:29
@nicksteele nicksteele merged commit bd68fbf into w3c:main Aug 16, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Aug 16, 2023
@nicksteele @github-actions
Merge pull request #1914 from sbweeden/sbweeden_1892_and_1909
3cc3a8a 
SHA: bd68fbf
Reason: push, by nicksteele

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@sbweeden
Copy link
Contributor Author

sbweeden commented Aug 16, 2023 •
edited

This was discussed again on call of 16 August 2023, and general consensus was that browser vendors seem unlikely to change current implementations to enforce that assertions from an authenticator be rejected if userHandle is not supplied during ceremonies without an allowCredentials list. That said, it is desirable that the spec be internally consistent. Even without browser implementation changes, RPs should still reject these assertions since section 7 already required the RP to verify the userHandle in such cases. This PR makes the spec consistent and the decision was made to merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kieun Kieun Kieun left review comments

@agl agl agl approved these changes

@emlun emlun emlun approved these changes

@pascoej pascoej pascoej approved these changes

@MasterKale MasterKale MasterKale approved these changes

@timcappalli timcappalli timcappalli approved these changes

@akshayku akshayku Awaiting requested review from akshayku

Assignees

@sbweeden sbweeden

Labels
type:technical
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revisit description of userHandle Require non-null userHandle when allowCredentials is empty?
8 participants
@sbweeden @emlun @agl @pascoej @MasterKale @timcappalli @Kieun @nicksteele