feat/attrs

[MasterKale]

This PR Is an attempt to swap out Pydantic use for the attrs+cattrs library combo. Based on feedback in Issue #110 I've come around to the idea that this library should incorporate dependencies that won't impose "restrictions" on which other dependencies the actual application consuming this library can use.

For example: if a project wishing to use py_webauthn can't because their credential IDs and public keys are passed around as pymongo's bytes subclasses, and Pydantic won't accept these values because they're not bytes, then maybe the validation logic is a bit too restrictive. If the subclass otherwise handles like a raw bytes value why shouldn't it Just Work™?

The work to switch over to attrs was fairly simple. I figured I'd put it up here for feedback on whether or not I should make this change.

[CLAassistant]

All committers have signed the CLA.

[MasterKale]

I have one failing unit test, where "transports": null is being returned from generate_registration_options() because I'm not specifying a value and it's defaulting to None, which becomes a JSON null. I'm trying to figure out how to get cattrs to unstructure the options while omitting values that aren't set, but so far a solution has eluded me.

[jwag956]

A large change - but you are right - mostly just changed syntax - with most of the new code just being convert/unconvert as would be expected.

In your CI actions - I don't see you running mypy which likely could have broken during this change.

Remember to change your README to swap out 'Pydantic' for 'attrs'.

Thanks for considering and working through this - I believe this will make the library much more usable across various applications.

[jwag956]

Don't forget to change setup.py to remove pydantic and add attrs/cattr.

[jwag956]

Once you merge this into master - I'll try it out with my middleware project - Flask-Security and let you know of any issues.

Thanks again for all the hard work.

[MasterKale]

@jwag956 Can you try installing this branch instead with pip? The following command should let you pull this code locally to test it out before I merge the PR into master.

pip install git+https://github.com/duo-labs/py_webauthn.git@feat/attrs

[jwag956]

Works great - and I could remove my 'explicit convert to bytes' for mongo.

thanks!

[samuelcolvin]

This is a real shame I think (perhaps obviously :smile:).

In particular this change means that webauthn types no longer drop into FastAPI app.

[samuelcolvin]

If we really don't want to use pydantic, which I can kind of understand. Would it not have been better to use dataclasses?

dataclasses would have allowed the duck typing, relaxed "pythonic" types, but also have dropped into fastapi and pydantic models.

:confused:

[jwag956]

As someone who encouraged them to drop pydantic (unless I am missing something - it is quite un-pythonic) - for middleware libraries such as this - that want/need broad exposure; I am curious why it doesn't 'drop' in to FastApi. Attrs is basically dataclasses + validation. In addition - pywebauthn provides python type hints - so works with mypy and IDEs out of the box. Note that pywebauthn doesn't actually define DB data models - which users still have to do - and it was this that caused issues with a variety of databases when using pydantic. So FASTApi users would still be able to, if they want, use pydantic for their data models I would think.

[MasterKale]

@samuelcolvin Can you explain how Pydantic allowed for this library to "drop in" to a FastAPI app? I've not used FastAPI before and am not sure what you mean by this.

[samuelcolvin]

"pythonic" is a very vague concept, PEP-20 "The Zen of python" is probably the closest thing to a definition of what "pythonic" means, but python itself breaks PEP-20 in numerous regards. Let's use specific points or criticisms rather than terms like "pythonic".

---

Can you explain how Pydantic allowed for this library to "drop in" to a FastAPI app? I've not used FastAPI before and am not sure what you mean by this.

FastAPI uses pydantic for all it's data validation (both in requests and responses), so I can setup a class like (this is copied from production code):

class GetMFA(BaseModel):
    otp: bool
    webauthn_options: PublicKeyCredentialRequestOptions = None

    class Config:
        json_encoders = {bytes: bytes_to_base64url}

To return webauthn options to a user and it'll just work out of the box - PublicKeyCredentialRequestOptions (from this library) is/was a pydantic model, and pydantic allows for recursive models, thus validation and coercion of PublicKeyCredentialRequestOptions is supported.

Similarly When validating a response in pydantic I could do

@app.post('/confirm-mfa/{blob}/webauthn/')
async def confirm_webauthn(
    request: Request,
    credential: CustomAuthenticationCredential,
    user_id: int = Depends(mfa_user_id),
):
    ...

(again taken from production code).

And the validation of the post body JSON to an instance of CustomAuthenticationCredential works out of the box.

(CustomAuthenticationCredential is derived from AuthenticationCredential with some conversion added to deal with the base 64 headache.

class CustomAuthenticationCredential(AuthenticationCredential):
    @validator('raw_id', pre=True)
    def convert_raw_id(cls, v: str):
        assert isinstance(v, str), 'raw_id is not a string'
        return url_b64_decode(v)

    @validator('response', pre=True)
    def convert_response(cls, data: dict):
        assert isinstance(data, dict), 'response is not a dictionary'
        return {k: url_b64_decode(v) for k, v in data.items()}

Obviously pydantic doesn't support validation of attrs classes, so both the above break with v1.2.

---

I really don't understand how using attrs is any different from using pydantic? It's a third party library with tools for validation, you may prefer attr over pydantic, but that's a value judgement.

Whatever "pythonic" might or might not mean, I really can't imagine a defence of attr over pydantic that uses "pythonic" as an argument and holds any water. Sorry.

If you really want "dataclasses + validation", why not use pydantic's datacalsses which performs validation and returns a real, vanilla, standard library dataclass.

[jwag956]

By pythonic I meant duck-typing. Perhaps there is a way in pydantic to allow for subclasses. The bug I filed was around a couple well used packages - mongodb and sqlachemy (for certain DBs) - that as you know use lots of python idioms to provide their ORM abstractions. See this issue: https://github.com/duo-labs/py_webauthn/issues/110 for a complete description of the problem. It is quite possible that pydantic has a way to specify this - but it wasn't obvious.

personally - for middleware libraries such as this - I think run-time validation is likely to cause more harm than good - but of course that's not my call for this library - however - as you point out - if runtime validation isn't wanted/needed then built-in data classes would be the way to go.

[MasterKale]

@samuelcolvin Would you mind turning this into an Issue so we can continue this discussion outside of a merged PR? I think you raise some valid questions, and I want to respond, I just don't want this to get lost down here.

[samuelcolvin]

done, #113. Thanks so much for being open to my moaning/ideas, I know how easy it is to be hostile or just ignore criticism like this.

Also, sorry if my message above seemed hostile, I was having a frustrating day upgrading packages, but that's no excuse :pray:.