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

feat: skip verifying attestation statement #219

Merged

Conversation

MaximeNdutiye
Copy link
Contributor

Trying to implement #213. Looking for feedback on desired implementation. @bdewater @grzuy

@brauliomartinezlm
Copy link
Member

Hi @MaximeNdutiye ! Thank you so much for opening this PR :).
I have some initial feedback. Would of course like to hear from @grzuy and @bdewater as well.

How about exploring an option to keep this a bit more isolated?

I'm thinking we halt execution earlier by conditionally executing this line. This would allow the implementation to leave this a bit more DRY and less spread out. Also, it's a bit more correct IMHO in the sense we should not even be verifying it in it's disabled by configuration, reducing any additional method calling and class instantiation.

In my mind it would end up looking like:

# lib/webauthn/authenticator_attestation_response.rb
def verify(...)
  super

  verify_item(:attestation_statement) if configuration.verify_attestation_statment

  true
end

@MaximeNdutiye
Copy link
Contributor Author

Thanks for the great feedback @brauliomartinezlm !

I did have some concerns about how repetitive the code was, as well as how spread out things were. I've made some changes based on your feedback and things are looking much cleaner 👍

Copy link
Contributor

@grzuy grzuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MaximeNdutiye,
Thank you for opening PR.

I left a couple of comments.

lib/webauthn/configuration.rb Outdated Show resolved Hide resolved
@MaximeNdutiye MaximeNdutiye force-pushed the skip_verify_attestation_statement branch from 5bf9d15 to 54ca09a Compare June 18, 2019 23:06
@grzuy
Copy link
Contributor

grzuy commented Jun 19, 2019

Hi @MaximeNdutiye, thank you for the updates!

Note PR is still "Draft".
Please mark PR as "Ready for review" if you think there's nothing else to add.

@MaximeNdutiye MaximeNdutiye marked this pull request as ready for review June 19, 2019 13:33
Copy link
Contributor

@grzuy grzuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🚀

@grzuy grzuy merged commit 387c330 into cedarcode:master Jun 19, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants