search

cedarcode / webauthn-ruby

WebAuthn ruby server library ― Make your Ruby/Rails web server become a conformant WebAuthn Relying Party
https://rubygems.org/gems/webauthn
MIT License
644 stars 51 forks source link

Add spec for `WebAuthn::PublicKeyCredential#verify` #414

Closed soartec-lab closed 5 months ago

soartec-lab commented 8 months ago

There was no test code for WebAuthn::PublicKeyCredential#verify, so I added it.

I noticed this when I updated lib/webauthn/public_key_credential.rb at https://github.com/cedarcode/webauthn-ruby/pull/413, but the scopes were different, so I split the PR.

santiagorodriguez96 commented 6 months ago

Hey @soartec-lab!

We do have tests for the PublicKeyCredential but on its subclasses: PublicKeyCredentialWithAttestation (https://github.com/cedarcode/webauthn-ruby/blob/23f3be4/spec/webauthn/authenticator_attestation_response_spec.rb) and PublicKeyCredentialWithAssertion (https://github.com/cedarcode/webauthn-ruby/blob/6a5d7e9/spec/webauthn/public_key_credential_with_assertion_spec.rb). It could be said however, that the tests for PublicKeyCredentialWithAttestation, do not include tests like the one you are adding here. I think we can create a PR for that!

soartec-lab commented 6 months ago

Hi @santiagorodriguez96 ,

Thank you for feedback.

Does that mean that the test should be provided in the subclass and there is no need to test for PublicKeyCredential? Since PublicKeyCredential is a public class, I thought it would be necessary to test it. What do you think?

santiagorodriguez96 commented 6 months ago

Hi @santiagorodriguez96 ,

Thank you for feedback.

Does that mean that the test should be provided in the subclass and there is no need to test for PublicKeyCredential? Since PublicKeyCredential is a public class, I thought it would be necessary to test it. What do you think?

It's not that is not tested, is just that we are testing it indirectly. I think I'd lean towards having the tests for the PublicKeyCredential as shared examples and calling them from its subclasses. After all we never use the PublicKeyCredential class directly.

I also think that there are methods in PublicKeyCredential that we are not testing, so we should add those tests to the spec suite of its sub classes.

soartec-lab commented 6 months ago

@santiagorodriguez96

Ok, i got it. you meam, since PublicKeyCredential is not used directly, there is no need to add any tests, and if i add tests, i add tests to the subclasses right? If my understanding is correct, i will close this PR. thank you for tell me.

santiagorodriguez96 commented 6 months ago

@santiagorodriguez96

Ok, i got it. you meam, since PublicKeyCredential is not used directly, there is no need to add any tests, and if i add tests, i add tests to the subclasses right?

Yeah, pretty much! Although if we want to add tests for methods defined in PublicKeyCredential we could use shared examples for testing the methods in its subclasses 🙂