Feature/blazor wasm

[Regenhardt]

closes #371

I tried building the same demo again basically. The docker deployment commit is completely optional and I can remove it if you prefer that.

[codecov-commenter]

Codecov Report

Merging #379 (0727dd4) into master (5e72564) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master     #379   +/-   ##
=======================================
  Coverage   76.80%   76.80%           
=======================================
  Files          89       89           
  Lines        2531     2531           
  Branches      427      427           
=======================================
  Hits         1944     1944           
  Misses        466      466           
  Partials      121      121           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Regenhardt]

Forgot the resident key option in custom, it's there now.

[Regenhardt]

So for some reason it tries publishing the BlazorWasmDemo project now instead of the Demo project, any idea why? I didn't change anything in the pipeline (could I even?).

[Regenhardt]

Sorry forgot to set my E-Mail, nothing else changed with this force push I just updated the author.

[Regenhardt]

Any idea how the pipeline can publish the new project instead of Demo/Demo.csproj as stated in the pipeline config? Seems like a bug to me, but I haven't actually worked with azure pipelines, just Gitlab where I write the actual bash script myself. Should I open an issue with Microsoft about that pipeline task?

[Regenhardt]

So I might have an idea what to do, although I'm not sure if it's the correct way or if I'm even able to change the pipeline config or test it here, but I'm gonna try and hope for some feedback either way :)

[Regenhardt]

Oh it actually worked, it was missing sourcelink so for some reason it included a beta version of it, which broke the build.
Additionally to the working pipeline, there is now also a pipeline job building the new project, so I guess that's nice too.

[Regenhardt]

So I'm unsure about the process here, I'm the only one in this thread either so I don't think anyone else gets pinged when I write something?

@abergs Review please, Blazor lib & Demo are finished.

[iamcarbon]

@Regenhardt Any chance you can host this on onrender.com for testing -- going to see if I can carve out some time next week to review. This seems like a high quality and useful implementation for the library (and helps validate that we got our AOT / trimming annotations right).

I'm also hoping we can get the passkeys PR merged in soon, which might result in some conflicts. We should probably get that in first -- and make sure everything there works on WASM too.

[Regenhardt]

I'd love to, but for the first time ever I can't seem to get the Dockerfile right. Gonna dig a bit into what's going on here. Error message is the same before and after switching from TargetFramework to TargetFrameworks:

Mar 6 10:28:38 AM   > [build 11/14] RUN dotnet restore "BlazorWasmDemo/Server/BlazorWasmDemo.Server.csproj":
Mar 6 10:28:38 AM  dotnet/roslyn#19 0.901   Determining projects to restore...
Mar 6 10:28:38 AM  dotnet/roslyn#19 2.035 /usr/share/dotnet/sdk/6.0.406/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(92,5): error NETSDK1013: The TargetFramework value '' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. [/src/BlazorWasmDemo/Server/BlazorWasmDemo.Server.csproj]

[Regenhardt]

I have no more ideas what to do here so I guess I'll have to wait for answer here for now: dotnet/sdk#31011

[Regenhardt]

Ok turns out I didn't have the Directory.Build.props in the docker image, making dotnet indeed restore projects without target Frameworks.
Now to figure out how to work around sourcelink expecting to be in a git repository and not in a docker image.

[Regenhardt]

@iamcarbon know of any other site to deploy this without an explicit need for a Dockerfile? Sourcelink needs some weird hacks to be compiled in a Dockerfile, this might take a few days for me to get right.

[Regenhardt]

Ok I got it working locally and cleaned the mess I made with the commits while doing it, time to try it on render.
Reminder for me if it doesn't work: NuGet/Home#10275 (comment)

[Regenhardt]

Ok it seems a failed deploy was something temporary on render's side, it works now:

https://fido2-blazorwasm.onrender.com

[Regenhardt]

Will wait for #393 to be merged, then rebase to make sure this is up to date.

[abergs]

@Regenhardt Thanks for updating. Just in case you didn't see it, we still have some conflicts.

[Regenhardt]

Yeah this was the first pass to clean up the commit history and make it more easily reviewable. I just rebased onto master and am now integrating the new features, shouldn't be too long now.

[Regenhardt]

Few questions that confuse me from a users perspective, that I probably should have asked in the review of the other branch but was too occupied (these are probably all not necessary to answer if you don't have the time, I can make it work anyway):

What's the difference between StoredCredential.Type and StoredCredential.CredType? They seem redundant.

What's the difference between AttestationVerificationSuccess.Id and AssertionVerificationResult.CredentialId, which AttestationVerificationSuccess inherits? Seem redundant again.

Why two properties StoredCredential.SignCount and SignatureCounter? Backward compatibility?

RequestNewCredential takes an AttestationConveyancePreference, but the extensions passed to it include AuthenticationExtensionsDevicePublicKeyInputs which also includes the AttestationConveyancePreference but as a string? Do I have that right? Seems redundant and it just being a string makes me think I should put something else there that is not the enum.

The extensions include a byte[][] AuthenticatorSelection, how do I use that? I have the AuthenticatorSelection I pass to RequestNewCredential anyway, but that's a complex object and not a byte[][]?

Since we're making breaking changes anyway: Could we rename the PublicKeyCredentialType type to CredentialType? Sure this library explicitly does public key credentials, but technically public-key is just one of the possible types, while the others are not public key credentials but other types.

[Regenhardt]

Also I just realised AssertionVerificationResult.CredentialId isn't set anymore, what's up with that?

[Regenhardt]

Alright everything up-to-date and way cleaner than before. My questions can also be answered at a later date, those are just things that can be confusing, not important (probably) for the actual feature here.

[aseigler]

Thank you so much for all this cleanup work! It's been bugging me for quite a while, I just hadn't been able to find the contiguous time that it takes to focus and get everything back right.

The reason behind all the renaming is to align the names directly to match the spec as much as possible. If you look at some of the earliest code, the names didn't match the verbiage in the spec at all. We got a little better about that as the library matured, but the spec also matured, some things were added/removed/renamed and it's hard to keep track of while also not breaking anything (maintaining backward compatibility).

I'll try to get to the other more specific questions later today.

[Regenhardt]

Also fixed the deploy, it can be tested on render: https://fido2-blazorwasm.onrender.com/

[Regenhardt]

It's been a few weeks, are you ok?

[aseigler]

Few questions that confuse me from a users perspective, that I probably should have asked in the review of the other branch but was too occupied (these are probably all not necessary to answer if you don't have the time, I can make it work anyway):

What's the difference between StoredCredential.Type and StoredCredential.CredType? They seem redundant.

StoredCredential.Type is a PublicKeyCredentialType, which currently can only be public-key. The StoredCredential.CredType is meant to store the attestation fmt string for...reasons I don't remember. Probably a bad name, this is a good time to rename it.

What's the difference between AttestationVerificationSuccess.Id and AssertionVerificationResult.CredentialId, which AttestationVerificationSuccess inherits? Seem redundant again.

I think those are redundant. A few times I tried to move everything to use the AssertionVerificationResult.CredentialId and remove AttestationVerificationSuccess.Id but I kept getting wrapped around the axles. This is a good time to fix that as well, if we can figure out how to do it cleanly.

Why two properties StoredCredential.SignCount and SignatureCounter? Backward compatibility?

Those seem redundant, probably backward compatibility as you said. We should keep the one that matches the naming in the spec, and have the other one point to that one instead of having two properties.

RequestNewCredential takes an AttestationConveyancePreference, but the extensions passed to it include AuthenticationExtensionsDevicePublicKeyInputs which also includes the AttestationConveyancePreference but as a string? Do I have that right? Seems redundant and it just being a string makes me think I should put something else there that is not the enum.

This is new territory with devicePublicKey, I don't see AttestationConveyancePreference in AuthenticationExtensionsDevicePublicKeyInputs, all I see is attestation and attestationFormats, no?

The extensions include a byte[][] AuthenticatorSelection, how do I use that? I have the AuthenticatorSelection I pass to RequestNewCredential anyway, but that's a complex object and not a byte[][]?

I think the authnSel extension got removed a few years ago, we should probably remove it.

Since we're making breaking changes anyway: Could we rename the PublicKeyCredentialType type to CredentialType? Sure this library explicitly does public key credentials, but technically public-key is just one of the possible types, while the others are not public key credentials but other types.

I don't feel super strongly about this, but I am striving to move all of the object names to match the spec wherever feasible. The PublicKeyCredentialType is directly modeled from https://www.w3.org/TR/webauthn/#enum-credentialType. I would be fine with CredentialType, I think it fits.

[Regenhardt]

Great, thanks for clearing those up! Will gladly implement some of those things after this is merged, as I would prefer not to have to rebase this branch again.

[aseigler]

Great, thanks for clearing those up! Will gladly implement some of those things after this is merged, as I would prefer not to have to rebase this branch again.

I'm sorry for taking so long to sit down and respond to these. Definitely can relate to not wanting to rebase again...

[Regenhardt]

So I just found out again where the question about DevicePubKey.Attestation being another AttestationConveyancePreferance: I copied it from the existing demo along with everything else:

I figured whatever you're doing there works so no reason to take a week and learn about these intricacies just to forget 80% of it after another week 😅.

[abergs]

While this is mostly new files that don't carry much risk, I still intend to take some time and review and run on my machine.

If @aseigler or @iamcarbon wants to do this before I get a chance, you're welcome and I will trust your review.

[abergs]

Couldn't get this to run and register a credential on my machine on the "go passwordless" page.

---> System.Text.Json.JsonException: The JSON value could not be converted to System.Nullable`1[System.Boolean]. Path: $.extensions.credProps | LineNumber: 0 | BytePositionInLine: 153.
blazor.webassembly.js:1 ---> System.InvalidOperationException: Cannot get the value of a token type 'StartObject' as a boolean.