Refactor tests, add documentation and fix bugs #19
Conversation
marius-se
changed the title
Sources/WebAuthn/Ceremonies/Authentication/VerifiedAuthentication.swift
Outdated
Show resolved
Hide resolved
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
| } | ||
|
|
||
| // MARK: - Credential parameters | ||
|
|
||
| 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.
| requireUserVerification: Bool = false, | ||
| confirmCredentialIDNotRegisteredYet: (String) async throws -> Bool = { _ in true } | ||
| ) async throws -> Credential { | ||
| try await webAuthnManager.finishRegistration( | ||
| 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: