Better token creation support #1
Conversation
This moves all loading of users to `get_user()` and expands it to taking kwargs. Developers can override the values that used for finding the user by overriding `get_user_kwargs`. Note: if a `user_id` is provided to `get_user`, it is translated to `pk` and will overwrite any `pk` provided.
This allows developers to control what's put into the token payload without having to write their own resolvers.
|
All of the tests pass with the new code paths, but I still consider this a WIP. There are probably some areas in the backend that could be refactored further to allow users to override smaller pieces of the backend without having to duplicate big chunks. Would love to hear your thoughts on this before I put too much more effort into it though. |
| return import_string( | ||
| getattr( | ||
| settings, | ||
| "JWT_BACKEND", |
There was a problem hiding this comment.
Kind of thinking this might should be DJANGO_ARIADNE_JWT_BACKEND to avoid any possible confusion.
There was a problem hiding this comment.
Agreed. I guess we should prefix JWT_EXPIRATION_DELTA and JWT_ALGORITHM as well to keep it consistent.
There was a problem hiding this comment.
For consistency, I guess let's leave it since everything is using JWT_ as the prefix. Seems like a candidate for supporting both with a DeprecationWarning if the old version is detected.
| def load_backend(): | ||
| return import_string( | ||
| getattr( | ||
| settings, | ||
| "JWT_BACKEND", | ||
| "django_ariadne_jwt.backends.JSONWebTokenBackend", | ||
| ) | ||
| )() |
There was a problem hiding this comment.
How does this code interact with the value from settings.AUTHENTICATION_BACKENDS? Wouldn't it allow a configuration issue where two different backends could be specified?
There was a problem hiding this comment.
This doesn't interact with the settings.AUTHENTICATION_BACKENDS code in any way other than having the one object that's pulling double duty and that I'm using the same pattern for loading backends. I could potentially see some issues if someone's got a backend that either:
- Does a lot of heavy lifting in the
__init__ - Requires that only one instance of the backend class was instantiated
Both of those feel like anti-patterns, so I'm not sure it'd worth coding around that.
All that said, I do wonder if it's confusing to folks to see the same backend configuration being used in multiple different areas. I could definitely see making two different parts, but things like loading users and such are shared in both concerns. That was my original thought for combining them.
There was a problem hiding this comment.
Assuming there is no case for using two different JSONWebTokenBackend classes in settings.AUTHENTICATION_BACKENDS, maybe we could make load_backend detect the JSONWebTokenBackend class or subclass referenced by the settings in order to avoid the duplicity.
This PR moves the the interaction with tokens into the
JSONWebTokenBackendobject and removes theutilsmodule.I needed the ability to control what goes in to the payload for a token. I contemplated creating a new object that would house all of the token interaction, but decided against that since so many of the methods are needed alongside the full authentication backend.