passwordless-lib / fido2-net-lib

FIDO2 .NET library for FIDO2 / WebAuthn Attestation and Assertion using .NET
https://fido2-net-lib.passwordless.dev/
MIT License
1.12k stars 158 forks source link

Confusing API: MakeAssertionAsync #518

Open glatzert opened 2 months ago

glatzert commented 2 months ago

The function https://github.com/passwordless-lib/fido2-net-lib/blob/9ad038b43aa0c37d993cdfd662c03e8d5a08419c/Src/Fido2/IFido2.cs#L16 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 https://github.com/passwordless-lib/fido2-net-lib/blob/9ad038b43aa0c37d993cdfd662c03e8d5a08419c/BlazorWasmDemo/Server/Controllers/UserController.cs#L284, 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:

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

glatzert commented 3 weeks ago

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 commented 5 days ago

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.