search

duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

Add default value for all arguments in the WebAuthnUser class #38

Closed mdedonno1337 closed 5 years ago

mdedonno1337 commented 5 years ago

I was asking myself why all arguments in the WebAuthnUser class are mendatory, but not always used. I had this code working on production:

webauthn_user = webauthn.WebAuthnUser( 
    None, None, None, None,
    key[ 'credential_id' ], None, None, config.RP_ID
)
webauthn_assertion_options = webauthn.WebAuthnAssertionOptions( webauthn_user, challenge )

This is not a definitive PR, but a snippet to discuss (of course backwards-compatible ; need some checks in the library)

What do you think ?

Edit: Same comment with this code:

webauthn_user = webauthn.WebAuthnUser( 
    None, session[ 'username' ], None, None,
    user[ 'credential_id' ], user[ 'pub_key' ], user[ 'sign_count' ], config.RP_ID
)

webauthn_assertion_response = webauthn.WebAuthnAssertionResponse( 
    webauthn_user,
    assertion_response,
    challenge,
    config.ORIGIN,
    uv_required = False
)
futureimperfect commented 5 years ago

Good observation, @mdedonno1337! I think this looks mostly good. What do you think about leaving the arguments in the constructor the same, but just raising if any of the required parameters are missing? I don't think there should be any major API breaking changes with this fix, but I could be missing something. Thanks again for your contributions!

mdedonno1337 commented 5 years ago

Not sur that all values have to be checked while creating the WebAuthnUser object, since not necessary mendatory depending upon the usage of the object. The a16563f commit deal with this part.

mdedonno1337 commented 5 years ago

Thanks for the updates! When you get a chance, do you mind addressing the changes I requested?

Done (I think). Consider a squash-merge instead of a normal-merge if you consider it more useful.

futureimperfect commented 5 years ago

Sorry for all the back-and-forth, @mdedonno1337! If you remove the changes to the method signature for the WebAuthnUser constructor we should be good to go. Thanks again!

mdedonno1337 commented 5 years ago

Useless PR finally I think (see the comment https://github.com/duo-labs/py_webauthn/pull/38#discussion_r287563733 ). Let me know when a refactoring process will be mendatory to add variables to the constructor, I will do it with pleasure.

futureimperfect commented 5 years ago

Hey @mdedonno1337,

I still think this PR has value, even without the changes to the constructor. It will raise if necessary inputs are not supplied to the WebAuthnUser class. If you'd rather wait until I revisit making changes to the constructor, that's totally understandable. Thanks again for your contribution!

mdedonno1337 commented 5 years ago

It's a pleasure to contribute to a usefull project ! A bit difficult for my first real contribution to opensource via PR because of the back-and-forth, but I'm very happy to contribute.

Can you squash-merge, or I have to rebase ? The git-log will be a mess if not squashed ...

futureimperfect commented 5 years ago

Thanks so much for your contribution, @mdedonno1337! Your PR is squashed and merged now. :)