feat: generalize credential public key return value #139
Conversation
| key.public_key = public_key | ||
| key = | ||
| if WebAuthn::AttestationStatement::FidoU2f::PublicKey.uncompressed_point?(public_key_bytes) | ||
| # For backwards compatibility with stored public keys with format returned by v1.10.0 or lower |
| @@ -48,6 +49,24 @@ | |||
| end | |||
| end | |||
|
|
|||
| # Backwards compatibility with v1.10.0 or lower | |||
There was a problem hiding this comment.
The problem you found and solve in this PR is a very complex one to catch before hand like you did.
Thank you so much @grzuy ! Nice job 🍻
| raw_data = raw_attested_credential_data( | ||
| public_key: fake_cose_credential_key(y_coordinate: SecureRandom.random_bytes(31)) | ||
| ) | ||
| it "returns false if public key doesn't contain the alg parameter" do |
There was a problem hiding this comment.
Does it add any value to add a test that check that credential.public_key is retrieving an intact version of the public_key, just as it was formed by the authenticator?
| key.public_key = public_key | ||
| key = | ||
| if WebAuthn::AttestationStatement::FidoU2f::PublicKey.uncompressed_point?(public_key_bytes) | ||
| # For backwards compatibility with stored public keys with format returned by v1.10.0 or lower |
There was a problem hiding this comment.
Thinking about a future us reading this code, it would be great if we could add more context/documentation on why did we end up supporting the signature validation of the processed version of the COSE PK, potentially used/stored by users of the gem previous to this PR release.
| def valid_credential_public_key? | ||
| cose_key = COSE::Key.deserialize(public_key) | ||
|
|
||
| cose_key.algorithm |
There was a problem hiding this comment.
Almost cosmetic here, but given this method ends in a ? I would expect to return a boolean.
What do you think about !!cose_key.algorithm?
There was a problem hiding this comment.
Right...
If we prefer that we should maybe have rubocop checking that..
There was a problem hiding this comment.
Never built a solid opinion on that particular pattern FWIW.
There was a problem hiding this comment.
Not a strong opinion neither. Specially not on how to implemented. I do like when something ends with a ? returns a boolean.
Your call 👍
Fixes #137