Clean up and break things

[Regenhardt]

As previously mentioned, with breaking changes coming anyway, we might as well clean up some stuff. So here's things I understood from over here we could change, I'll try to make different points and work within those so I don't get confused too much.

1. StoredCredential.CredType

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.

This directly maps to AttestationVerificationSuccess.CredType. If I understand this correctly, it maps to the spec's attestationFormat(see https://w3c.github.io/webauthn/#sctn-generating-an-attestation-object), so how about I rename CredType to AttestationFormat plus documentation like "Format of the attestation statement, e.g. fido-u2f or android-key" with that link?

---

Update 2024-01-12

Done in #427

2. Id/CredentialId

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.

• These seem to be used internally only:
  • .Id is created on attestation, then stored with the new credential, both as .Id and .Descriptor.Id -> redundant again?
  • .CredentialId is created on successful assertion, used to up the counter, literally used a index to find the stored .Descriptor.Id, TestController also stores it in the .Descriptor.Id, I'm confident it's the same as .Id.

Was speaks against shoving everything into .CredentialId? Tbh. I'm having a hard time finding this one in the spec, it's possible there was something along the lines of public byte[] Id => this.CredentialId; in there basically but in natural language.
If you think this would ok spec-wise, I'll try to make everything use CredentialId.

---

Update 2024-01-12

AttestedCredentialData (lib-input in Attestation/Assertion authenticator responses) has CredentialId
RegisteredPublicKeyCredential (lib-output when cred was created / after attestation) has Id
StoredCredential (dev storage) has Id
VerifyAssertionResult (lib-output when cred was verified / after assertion) has CredentialId

All these are assigned to each other, so I guess these should be consolidated.

---

Update 2024-03-06

Moved to #510.

3. SignCount

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.

The field is called signCount in the spec so I guess that's the one we want. An easy answer for once.

---

Update 2024-01-12

Done in #427

4. attType / AttestationConveyancePreference

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?

RequestNewCredential takes a mandatory parameter of AttestationConveyancePreference (with a default value of none), so if there is a preference conveyed by the client, it's definitely there. This property is just added to the CredentialCreateOptions.
It also takes extensions, which optionally include AuthenticationExtensionsDevicePublicKeyInputs, which have a property called Attestation spec'd like this:

The Relying Party MAY use this OPTIONAL member to specify a preference regarding attestation conveyance. Its value SHOULD be a member of AttestationConveyancePreference.

The extensions are also currently only used to add them to the CredentialCreateOptions.

We could be restrictive and just remove the redundancy, or we could keep the redundancy because it fits the spec and technically one of them is optional so maybe different use cases. In both cases, documentation would help to understand what's going on.
Also we could make the AuthenticationExtensionsDevicePublicKeyInputs.Attestation be of type AttestationConveyancePreference too, which would be restrictive on the SHOULD part of the spec again (it may technically be something else, in which case the client is supposed to ignore it).

---

Update 2024-03-06

Moved to #511

5. AuthenticatorSelection/authnSel

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.

I can't find this extension (or the keyword) in the spec, but I did find that the spec's documentation for the non-extension parameter AuthenticatorSelection kinda fits the documentation for authnSel extension in the lib, so I guess we do indeed delete this one?

---

Update 2024-01-12

Done in #435

6. PublicKeyCredentialType

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.

After reading the spec and seeing this enum is indeed called PublicKeyCredentialType there, I have mixed feelings here. I disagree with the spec, as I think public-key is a type of credential, same as password and federated, as explained in Mozilla's MDN:

Valid values are password, federated and public-key.

They don't mention the enum at all.
I do however agree with keeping things close to the spec.

So I really don't know what to do about this one, as my brain is fighting itself here.

---

Update 2024-03-06

Done as nothing to do.

[jonashendrickx]

428 I've temporarily mapped it in this PR.

[abergs]

Hey! Thanks for summarising these thoughts.

1, 2 and 3 has all been more-or-less hashed out in #428 and #427, correct? There may be more work with Id/CredentialId coming, but I have not landed on any decision I fell comfortable with yet.

4 & 5 are things I'd need to look into, but seems something could be done there, at least 5.

6 - We're going to align with spec, because it makes things easier to cross-reference and check what is what over long time periods :)

[abergs]

I'd welcome a PR for 5 and we can discuss it more focused there.

[iamcarbon]

@joegoldman2 It looks like we made good progress on this. Can we close this issue, and move anything left into a new issue?

[Regenhardt]

I updated the original post to try and keep this organized.

2 (Id/CredentialId) could need some more work,
4 (attType/AttestationConveyencePreference) I feel like we talked about somewhere else but I can't find it, and
6 (PublicKeyCredentialType) is basically nothing to do

correct?

[Regenhardt]

Also will these things be part of the 4.0.0 release? I think a tag or project (or whatever Github provides) would be helpful to have a list of things to prioritize for the next release.

[abergs]

GH have milestones, and yes I think this would be good to get in before closing releasing the 4.0 stable.

I would appreciate separate issues for 2 and 4. Could you create them @Regenhardt? Closing this in anticipation of the new issues.