Refactor tests, add documentation and fix bugs #19
Conversation
marius-se
changed the title
0xTim
left a comment
0xTim
left a comment
There was a problem hiding this comment.
Tests look way better! Added some comments and queries. Most changes are around documentation of public types
Sources/WebAuthn/Ceremonies/Authentication/PublicKeyCredentialRequestOptions.swift
Show resolved
Hide resolved
| // case .packed: | ||
| // try await PackedAttestation.verify( | ||
| // attStmt: attestationStatement, | ||
| // authenticatorData: rawAuthenticatorData, | ||
| // clientDataHash: Data(clientDataHash), | ||
| // credentialPublicKey: credentialPublicKey, | ||
| // pemRootCertificates: pemRootCertificates | ||
| // ) | ||
| // case .tpm: | ||
| // try TPMAttestation.verify( | ||
| // attStmt: attestationStatement, | ||
| // authenticatorData: rawAuthenticatorData, | ||
| // attestedCredentialData: attestedCredentialData, | ||
| // clientDataHash: Data(clientDataHash), | ||
| // credentialPublicKey: credentialPublicKey, | ||
| // pemRootCertificates: pemRootCertificates | ||
| // ) |
There was a problem hiding this comment.
We'll need this once attestation verification is ready. If uncommented it should compile, but the verification flow is not done yet.
Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift
Show resolved
Hide resolved
| public struct PublicKeyCredentialParameters: Equatable, Codable { | ||
| public let type: String | ||
| public let algorithm: COSEAlgorithmIdentifier | ||
| public let alg: COSEAlgorithmIdentifier |
There was a problem hiding this comment.
Any reason to rename this? alg might be unclear (we can use CodingKeys if it's a JSON thing
There was a problem hiding this comment.
Regular users of this library shouldn't care about this. They'll just encode this and send it to the frontend/ client without inspecting what's in there.
Internally following the naming conventions of the WebAuthn specs makes things a lot easier during development. But I'm happy to change it back since I know the codebase, I'm just worrying about newbies.
| challenge: challenge, | ||
| credentialCreationData: RegistrationCredential( | ||
| id: id, | ||
| id: id.asString(), |
There was a problem hiding this comment.
Is it worth extracting this to it's own type instead of just a String?
There was a problem hiding this comment.
We're handling a lot of base64/base64url data and having some base64 "type safety" is super helpful.
This PR adds a lot of tests and test infrastructure.
Besides that I renamed some properties to follow the WebAuthn spec more closely, I renamed the
Userprotocol toWebAuthnUser, which closes #18, and I updated the README a little bit.Finally a lot of work went into adding attestation verification for packed and TPM attestation formats, but there are still too many pieces missing to release it. To fully support attestation verification we'll have to have support for: