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

Consolidate Id/CredentialId? #510

Open
Regenhardt opened this issue Mar 6, 2024 · 0 comments
Open

Consolidate Id/CredentialId? #510

Regenhardt opened this issue Mar 6, 2024 · 0 comments

Comments

@Regenhardt
Copy link
Contributor

Separate discussion about point 2. from #426 as requested.

Redundancy

On Attestation, the ID of the newly created credential (RegisteredPublicKeyCredential.Id) is saved to the database redundantly, once in StoredCredential.Id and once wrapped in a PublicKeyCredentialDescriptor in StoredCredential.Descriptor:

grafik

StoredCredential.Descriptor is used in multiple places to work with the credential's ID.
StoredCredential.Id on the other hand is never read, just written to in the demos; I propose to just delete this property.

grafik

Naming

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 (redundant, see above)
PublicKeyCredentialDescriptor (used to identify a credential throughout the lib; usually named like it is the credential) has Id
VerifyAssertionResult (lib-output when cred was verified / after assertion) has CredentialId

This one is not as clear and maybe totally correct like it is.
In things called credential, the ID is called Id, so when talking about or using it, it's usually something like "Credential ID"/credential.Id.
In things not called credential, i.e. adjacent data to an actual credential, it's called Credentialid, so talking about it would be something like "the result's credential ID" or "the attested data's credential ID".

Presuming of course the one talking about it kind of knows what they're talking about. For me, integrating the lib in the demo app was confusing, as I hadn't touched WebAuthn/FIDO2 before that.

So renaming them all to CredentialId could make it easier, but it would also deviate from the spec for PublicKeyCredentialDescriptor.
On the other hand, it would probably be way less confusing anyway if the public types have XML docs for the types and properties per #501.

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

1 participant