Refactor(oauth): resolve RuntimeWarning #2152
Conversation
github-actions
bot
added
back-end
angela-tran
added
chore
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
This looks like it fixed the issue! 👍 I ran the app locally and saw that the RuntimeWarning no longer shows up.
I was wondering about the difference between
oauth_client = register_provider(oauth, verifier.auth_provider) and
oauth_client = oauth.create_client(verifier.auth_provider.client_name)
and when to use one over the other 🤔. It looks like both of them return the same object and calling register_provider() multiple times is safe, so they seem to be equivalent (as long as a provider has been registered). I guess this observation may relate to Kegan's comment.
|
|
||
| for provider in providers: | ||
| logger.debug(f"Registering OAuth client: {provider.client_name}") | ||
| client = oauth_registry.register( |
There was a problem hiding this comment.
At first I was wondering about this line since the Authlib documentation does not mention a return type for .register(), but after running the app in debug mode and looking at the Authlib code, it turns out that .register() does return a client (<authlib.integrations.django_client.apps.DjangoOAuth2App object), which is the same client shown in oauth._clients after calling register() on oauth.
There was a problem hiding this comment.
I was wondering about the difference between
oauth_client = register_provider(oauth, verifier.auth_provider)andoauth_client = oauth.create_client(verifier.auth_provider.client_name)and when to use one over the other 🤔. It looks like both of them return the same object and calling
register_provider()multiple times is safe, so they seem to be equivalent (as long as a provider has been registered). I guess this observation may relate to Kegan's comment.
Yeah, so you'd want to use register_provider(oauth, verifier.auth_provider) instead of oauth.create_client(verifier.auth_provider.client_name) when it's possible that the AuthProvider's config hasn't been registered with Authlib yet.
I used register_provider in the login view but not in authorize because we know authorize will always be preceded by login and therefore the client will already be registered. But as @thekaveman is suggesting, it wouldn't hurt to do that in authorize too, so I'm good with making that change.
Ideally I would've liked to somehow check if the client_name has already been registered, call register for it if it hasn't, and then call create_client. But Authlib doesn't provide a way for us to check the registry, so all we can do is call register every time.
There was a problem hiding this comment.
But Authlib doesn't provide a way for us to check the registry, so all we can do is call register every time.
What happens when we try to get a client_name that hasn't been registered yet? Exception, or does it just return None? Either way, we could handle that case and use it to register. Wrap both cases up in a helper function and call it a day 😎
There was a problem hiding this comment.
I think an exception could mean a misconfiguration, so I'll leave that case out. But I went ahead and made the helper function that registers the client if we got None back - 4d2e1ec. Hopefully this code reads as more intuitive.
this way we avoid reading from the database during any AppConfig.ready implementations, which Django warns us about because they are executed as a part of management commands as well.
the register method seems to be idempotent, so it's fine to call it like this. the approach of registering during view import made importing the module complicated (e.g. when importing from the module for our tests)
currently the only way to get to the authorize view is from the login view, so the client should already be registered. But it doesn't hurt to call it here either.
angela-tran
force-pushed
the
refactor/auth-provider-registration
branch
from
47a1ae6
to
770f8e1
Compare
taking this approach in hopes of it being a more readable solution.
|
Preview url: https://benefits-2152--cal-itp-previews.netlify.app |
|
|
||
| Adapted from https://stackoverflow.com/a/64174413. | ||
| """ | ||
| logger.info("Registering OAuth clients") | ||
| logger.debug(f"Registering OAuth client: {provider.client_name}") |
Closes #2003
The problem with calling
register_providersinready()is that it reads from the database (to get all theAuthProviders), butreadyis called during management commands, so it is not entirely safe to interact with the database at that time.This PR refactors the function to take one provider model object and moves it from
ready()to the view where the OAuthclientobject is needed. Theregistermethod onBaseOAuthlooks safe to call multiple times and returns a client object.When reviewing
You should see that the
RuntimeWarningno longer occurs when starting up the app (or running any management command - e.g.makemigrations,makemessages).