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

Help RP's understand actionable exceptions from create() and get() #2047

Merged
merged 29 commits into from
Aug 7, 2024

Conversation

MasterKale
Copy link
Contributor

@MasterKale MasterKale commented Mar 20, 2024

This PR attempts to pull together any exceptions raised by create() and get() to help RP's understand what exceptions may be encountered when using WebAuthn. The intention here is to help RP's understand which errors might be surfaced to the user, and which should not.

Addresses #1859.


Preview | Diff

@MasterKale MasterKale changed the title [WIP] Help RP's understand actionable exceptions from create() and get() Help RP's understand actionable exceptions from create() and get() May 1, 2024
@MasterKale MasterKale marked this pull request as ready for review May 1, 2024 22:53
@MasterKale
Copy link
Contributor Author

I've finally cobbled together reasons for all of the exceptions during both registration and authentication.

...Except I cop out a bit with NotAllowedError because it has many more possible reasons it gets raised, and in practice clients have overloaded this exception with causes not documented in the spec. I thought it prudent to present this error as one that RPs should prepare to handle as a general, "the user canceled the ceremony, or something went wrong" exception and handle it as such. This is as opposed to encouraging each RP to try and interpret all possible reasons the issue was raised. I'm open to feedback on this approach.

@yackermann
Copy link
Contributor

100% support this @MasterKale. Recently working on client lib, NotAllowedError is kinda useless. *)

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.

Substance wise this looks good to me 🙂 but this does highlight that we're not at all consistent about actually forwarding some of these "error code equivalent to..." errors from the authenticator through the client layer. But that should be a different issue.

Some editorial issues to take care of, though:

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
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
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@MasterKale
Copy link
Contributor Author

@emlun I incorporated all but one of your suggested changes; I left a comment on the one about TypeError. Can you please take another look when you have a chance?

And @timcappalli and @sbweeden can you also please take a look when you can?

@sbweeden
Copy link
Contributor

sbweeden commented Jun 5, 2024

Whilst I cannot vouch for the completeness or accuracy of the documented errors, I certainly support the idea that they be listed, along with their reasons when known. I do feel however that this effort is more about documenting encumbent behaviour than defining a set of useful-to-the-RP error conditions that the clients should implement. As a simple example, it would be good for an RP to be able to differentiate a user canceling an operation vs timeout. At the next WG call I'd like to at least raise whether this effort should consider the approach of trying to define what RPs want, rather than just what browsers currently do.

@MasterKale
Copy link
Contributor Author

@sbweeden This PR will pave a path towards introducing more nuanced errors like the ones I detailed here in response to a similar statement you made a month ago:

#2062 (comment)

The hope is that we establish a sensible way of centrally capturing errors, and keep it updated as we consider introducing more nuanced errors for the benefit of RPs.

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.

These are just nits (I don't feel too strongly either way).

Looks good otherwise.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@MasterKale
Copy link
Contributor Author

WAWG Meeting @ 6/26:

  • @nsatragno suggested I mark these sections non-normative so the processing steps remain the authority on what errors clients should raise
  • I'm going to make @timcappalli's suggested section title changes, it's a bit confusing as-is given parent header titles

Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@MasterKale MasterKale mentioned this pull request Jul 11, 2024
3 tasks
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
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs 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
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
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@nadalin nadalin requested a review from pascoej July 31, 2024 18:53
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.

Thanks @MasterKale!

@MasterKale
Copy link
Contributor Author

Heya @pascoej and @marcoscaceres is there a chance you can take a look at this and comment/approve in time for tomorrow's meeting?

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 seems useful.

@MasterKale MasterKale merged commit 056ed8b into main Aug 7, 2024
2 checks passed
@MasterKale MasterKale deleted the 1859-differentiate-errors branch August 7, 2024 19:50
github-actions bot added a commit that referenced this pull request Aug 7, 2024
SHA: 056ed8b
Reason: push, by MasterKale

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

10 participants