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

Confusing API: MakeAssertionAsync #518

Open
glatzert opened this issue Apr 15, 2024 · 2 comments
Open

Confusing API: MakeAssertionAsync #518

glatzert opened this issue Apr 15, 2024 · 2 comments

Comments

@glatzert
Copy link

The function

Task<VerifyAssertionResult> MakeAssertionAsync(
IFido2.MakeAssertionAsync() returns an instance of VerifyAssertionResult. This leads to questions and some confusion about how to use it:

  1. The return type contains an status and an errorMessage, that is used in the samples like here , but will never be anything else then { status = "ok" } and errorMessage = null despite the sample indicating otherwise. MakeAssertionAsync() will always throw on error, which is okay, but should be clearly communicated.
  2. the VerifyAssertionResult.Status property is of type string and is neither populated by an enum nor by an constant, so if that property is relevant, the caller needs to read the code to understand the possible values.

I propose:

  • removing the inheritance of VerifyAssertionResult to Fido2ResponseBase since that brings in the two problematic fields.
  • adding a bool to VerifyAssertionResult indicating successful verification and thus remove all throws and replace it with return VerifiyAssertionResult.Error(string reason) setting that bool to false (or an enum indicating success and failure to make it clearly distinguishable).

If you'd accept the Idea, I'd implement it and provide a PR.

@glatzert
Copy link
Author

glatzert commented Jun 12, 2024

I followed the Fido2ResponseBase.Status and currently there's only one line of demo code, where the Property takes another value than "Ok" - I think the property is most misleading as such.

Same goes for the errorMessage - it's always empty.
Fido2ResponseBase seems not to be used in any meaningful way and should as such be removed.

@abergs
Copy link
Collaborator

abergs commented Jun 27, 2024

If I recall, the reason for this weirdness was because the conformance tests required the models to look like that. Of course, we improve on this.

I agree - will look into cleaning this up as part of v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants