From b4e9f444bfef11a72176f7197999f18776fbc4db Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Mon, 10 Jun 2024 16:19:08 +0000 Subject: [PATCH 1/5] refactor(oauth): move client registration to happen during view import 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. --- benefits/oauth/apps.py | 12 ------------ benefits/oauth/views.py | 5 ++++- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/benefits/oauth/apps.py b/benefits/oauth/apps.py index 8c9acf873..68d7be30f 100644 --- a/benefits/oauth/apps.py +++ b/benefits/oauth/apps.py @@ -9,15 +9,3 @@ class OAuthAppConfig(AppConfig): name = "benefits.oauth" label = "oauth" verbose_name = "Benefits OAuth" - - def ready(self): - # delay import until the ready() function is called, signaling that - # Django has loaded all the apps and models - from .client import oauth, register_providers - - # wrap registration in try/catch - # even though we are in a ready() function, sometimes it's called early? - try: - register_providers(oauth) - except Exception: - pass diff --git a/benefits/oauth/views.py b/benefits/oauth/views.py index 3c0c5c0f4..ad3b27d8b 100644 --- a/benefits/oauth/views.py +++ b/benefits/oauth/views.py @@ -6,7 +6,7 @@ from benefits.core import session from . import analytics, redirects -from .client import oauth +from .client import oauth, register_providers from .middleware import VerifierUsesAuthVerificationSessionRequired @@ -20,6 +20,9 @@ ROUTE_POST_LOGOUT = "oauth:post_logout" +register_providers(oauth) + + @decorator_from_middleware(VerifierUsesAuthVerificationSessionRequired) def login(request): """View implementing OIDC authorize_redirect.""" From c9e74946284eef1dd751bc7b988daf65a317290d Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Mon, 10 Jun 2024 17:40:03 +0000 Subject: [PATCH 2/5] refactor(oauth): change to approach of registering client in login view 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) --- benefits/oauth/client.py | 28 +++++++++-------------- benefits/oauth/views.py | 8 +++---- tests/pytest/oauth/test_app.py | 23 ------------------- tests/pytest/oauth/test_client.py | 38 +++++++++++-------------------- 4 files changed, 27 insertions(+), 70 deletions(-) delete mode 100644 tests/pytest/oauth/test_app.py diff --git a/benefits/oauth/client.py b/benefits/oauth/client.py index 3be706194..67e3d759c 100644 --- a/benefits/oauth/client.py +++ b/benefits/oauth/client.py @@ -6,9 +6,6 @@ from authlib.integrations.django_client import OAuth -from benefits.core.models import AuthProvider - - logger = logging.getLogger(__name__) oauth = OAuth() @@ -42,23 +39,20 @@ def _authorize_params(scheme): return params -def register_providers(oauth_registry): +def register_provider(oauth_registry, provider): """ - Register OAuth clients into the given registry, using configuration from AuthProvider models. + Register OAuth clients into the given registry, using configuration from AuthProvider model. Adapted from https://stackoverflow.com/a/64174413. """ - logger.info("Registering OAuth clients") - - providers = AuthProvider.objects.all() + logger.debug(f"Registering OAuth client: {provider.client_name}") - for provider in providers: - logger.debug(f"Registering OAuth client: {provider.client_name}") + client = oauth_registry.register( + provider.client_name, + client_id=provider.client_id, + server_metadata_url=_server_metadata_url(provider.authority), + client_kwargs=_client_kwargs(provider.scope), + authorize_params=_authorize_params(provider.scheme), + ) - oauth_registry.register( - provider.client_name, - client_id=provider.client_id, - server_metadata_url=_server_metadata_url(provider.authority), - client_kwargs=_client_kwargs(provider.scope), - authorize_params=_authorize_params(provider.scheme), - ) + return client diff --git a/benefits/oauth/views.py b/benefits/oauth/views.py index ad3b27d8b..ec310b56f 100644 --- a/benefits/oauth/views.py +++ b/benefits/oauth/views.py @@ -6,7 +6,7 @@ from benefits.core import session from . import analytics, redirects -from .client import oauth, register_providers +from .client import oauth, register_provider from .middleware import VerifierUsesAuthVerificationSessionRequired @@ -20,14 +20,12 @@ ROUTE_POST_LOGOUT = "oauth:post_logout" -register_providers(oauth) - - @decorator_from_middleware(VerifierUsesAuthVerificationSessionRequired) def login(request): """View implementing OIDC authorize_redirect.""" verifier = session.verifier(request) - oauth_client = oauth.create_client(verifier.auth_provider.client_name) + + oauth_client = register_provider(oauth, verifier.auth_provider) if not oauth_client: raise Exception(f"oauth_client not registered: {verifier.auth_provider.client_name}") diff --git a/tests/pytest/oauth/test_app.py b/tests/pytest/oauth/test_app.py deleted file mode 100644 index 93b1aaeb6..000000000 --- a/tests/pytest/oauth/test_app.py +++ /dev/null @@ -1,23 +0,0 @@ -import benefits -from benefits.oauth.apps import OAuthAppConfig - - -def test_ready_registers_clients(mocker): - mock_registry = mocker.patch("benefits.oauth.client.oauth") - mock_register_providers = mocker.patch("benefits.oauth.client.register_providers") - - app = OAuthAppConfig("oauth", benefits) - app.ready() - - mock_register_providers.assert_called_once_with(mock_registry) - - -def test_ready_register_exception(mocker): - mocker.patch("benefits.oauth.client.oauth") - mocker.patch("benefits.oauth.client.register_providers", side_effect=Exception) - - app = OAuthAppConfig("oauth", benefits) - app.ready() - - # we expect no Exception to be raised - assert app diff --git a/tests/pytest/oauth/test_client.py b/tests/pytest/oauth/test_client.py index 4c6286cd6..4cc25dd51 100644 --- a/tests/pytest/oauth/test_client.py +++ b/tests/pytest/oauth/test_client.py @@ -1,7 +1,7 @@ import pytest from benefits.core.models import AuthProvider -from benefits.oauth.client import _client_kwargs, _server_metadata_url, _authorize_params, register_providers +from benefits.oauth.client import _client_kwargs, _server_metadata_url, _authorize_params, register_provider def test_client_kwargs(): @@ -39,33 +39,21 @@ def test_authorize_params_no_scheme(): @pytest.mark.django_db -def test_register_providers(mocker, mocked_oauth_registry): - mock_providers = [] - - for i in range(3): - p = mocker.Mock(spec=AuthProvider) - p.client_name = f"client_name_{i}" - p.client_id = f"client_id_{i}" - mock_providers.append(p) - - mocked_client_provider = mocker.patch("benefits.oauth.client.AuthProvider") - mocked_client_provider.objects.all.return_value = mock_providers +def test_register_provider(mocker, mocked_oauth_registry): + mocked_client_provider = mocker.Mock(spec=AuthProvider) + mocked_client_provider.client_name = "client_name_1" + mocked_client_provider.client_id = "client_id_1" mocker.patch("benefits.oauth.client._client_kwargs", return_value={"client": "kwargs"}) mocker.patch("benefits.oauth.client._server_metadata_url", return_value="https://metadata.url") mocker.patch("benefits.oauth.client._authorize_params", return_value={"scheme": "test_scheme"}) - register_providers(mocked_oauth_registry) - - mocked_client_provider.objects.all.assert_called_once() - - for provider in mock_providers: - i = mock_providers.index(provider) + register_provider(mocked_oauth_registry, mocked_client_provider) - mocked_oauth_registry.register.assert_any_call( - f"client_name_{i}", - client_id=f"client_id_{i}", - server_metadata_url="https://metadata.url", - client_kwargs={"client": "kwargs"}, - authorize_params={"scheme": "test_scheme"}, - ) + mocked_oauth_registry.register.assert_any_call( + "client_name_1", + client_id="client_id_1", + server_metadata_url="https://metadata.url", + client_kwargs={"client": "kwargs"}, + authorize_params={"scheme": "test_scheme"}, + ) From 770f8e185a192f802f1bec47b31c5b6e48a2b763 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 12 Jun 2024 21:17:59 +0000 Subject: [PATCH 3/5] feat: ensure client is registered for authorize view 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. --- benefits/oauth/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benefits/oauth/views.py b/benefits/oauth/views.py index ec310b56f..e99c94a85 100644 --- a/benefits/oauth/views.py +++ b/benefits/oauth/views.py @@ -44,7 +44,7 @@ def login(request): def authorize(request): """View implementing OIDC token authorization.""" verifier = session.verifier(request) - oauth_client = oauth.create_client(verifier.auth_provider.client_name) + oauth_client = register_provider(oauth, verifier.auth_provider) if not oauth_client: raise Exception(f"oauth_client not registered: {verifier.auth_provider.client_name}") From 4d2e1ec854acb45f25b9b770492becdd636fc7c4 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 12 Jun 2024 21:59:14 +0000 Subject: [PATCH 4/5] refactor(oauth): attempt to create client and register if needed taking this approach in hopes of it being a more readable solution. --- benefits/oauth/client.py | 14 ++++++++++- benefits/oauth/views.py | 6 ++--- tests/pytest/oauth/test_client.py | 40 +++++++++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/benefits/oauth/client.py b/benefits/oauth/client.py index 67e3d759c..923b29ab6 100644 --- a/benefits/oauth/client.py +++ b/benefits/oauth/client.py @@ -39,7 +39,7 @@ def _authorize_params(scheme): return params -def register_provider(oauth_registry, provider): +def _register_provider(oauth_registry, provider): """ Register OAuth clients into the given registry, using configuration from AuthProvider model. @@ -56,3 +56,15 @@ def register_provider(oauth_registry, provider): ) return client + + +def create_client(oauth_registry, provider): + """ + Returns an OAuth client, registering it if needed. + """ + client = oauth_registry.create_client(provider.client_name) + + if client is None: + client = _register_provider(oauth_registry, provider) + + return client diff --git a/benefits/oauth/views.py b/benefits/oauth/views.py index e99c94a85..9ce8297bf 100644 --- a/benefits/oauth/views.py +++ b/benefits/oauth/views.py @@ -6,7 +6,7 @@ from benefits.core import session from . import analytics, redirects -from .client import oauth, register_provider +from .client import oauth, create_client from .middleware import VerifierUsesAuthVerificationSessionRequired @@ -25,7 +25,7 @@ def login(request): """View implementing OIDC authorize_redirect.""" verifier = session.verifier(request) - oauth_client = register_provider(oauth, verifier.auth_provider) + oauth_client = create_client(oauth, verifier.auth_provider) if not oauth_client: raise Exception(f"oauth_client not registered: {verifier.auth_provider.client_name}") @@ -44,7 +44,7 @@ def login(request): def authorize(request): """View implementing OIDC token authorization.""" verifier = session.verifier(request) - oauth_client = register_provider(oauth, verifier.auth_provider) + oauth_client = create_client(oauth, verifier.auth_provider) if not oauth_client: raise Exception(f"oauth_client not registered: {verifier.auth_provider.client_name}") diff --git a/tests/pytest/oauth/test_client.py b/tests/pytest/oauth/test_client.py index 4cc25dd51..3bfe63df8 100644 --- a/tests/pytest/oauth/test_client.py +++ b/tests/pytest/oauth/test_client.py @@ -1,7 +1,7 @@ import pytest from benefits.core.models import AuthProvider -from benefits.oauth.client import _client_kwargs, _server_metadata_url, _authorize_params, register_provider +from benefits.oauth.client import _client_kwargs, _server_metadata_url, _authorize_params, _register_provider, create_client def test_client_kwargs(): @@ -48,7 +48,7 @@ def test_register_provider(mocker, mocked_oauth_registry): mocker.patch("benefits.oauth.client._server_metadata_url", return_value="https://metadata.url") mocker.patch("benefits.oauth.client._authorize_params", return_value={"scheme": "test_scheme"}) - register_provider(mocked_oauth_registry, mocked_client_provider) + _register_provider(mocked_oauth_registry, mocked_client_provider) mocked_oauth_registry.register.assert_any_call( "client_name_1", @@ -57,3 +57,39 @@ def test_register_provider(mocker, mocked_oauth_registry): client_kwargs={"client": "kwargs"}, authorize_params={"scheme": "test_scheme"}, ) + + +@pytest.mark.django_db +def test_create_client_already_registered(mocker, mocked_oauth_registry): + mocked_client_provider = mocker.Mock(spec=AuthProvider) + mocked_client_provider.client_name = "client_name_1" + mocked_client_provider.client_id = "client_id_1" + + create_client(mocked_oauth_registry, mocked_client_provider) + + mocked_oauth_registry.create_client.assert_any_call("client_name_1") + mocked_oauth_registry.register.assert_not_called() + + +@pytest.mark.django_db +def test_create_client_already_not_registered_yet(mocker, mocked_oauth_registry): + mocked_client_provider = mocker.Mock(spec=AuthProvider) + mocked_client_provider.client_name = "client_name_1" + mocked_client_provider.client_id = "client_id_1" + + mocker.patch("benefits.oauth.client._client_kwargs", return_value={"client": "kwargs"}) + mocker.patch("benefits.oauth.client._server_metadata_url", return_value="https://metadata.url") + mocker.patch("benefits.oauth.client._authorize_params", return_value={"scheme": "test_scheme"}) + + mocked_oauth_registry.create_client.return_value = None + + create_client(mocked_oauth_registry, mocked_client_provider) + + mocked_oauth_registry.create_client.assert_any_call("client_name_1") + mocked_oauth_registry.register.assert_any_call( + "client_name_1", + client_id="client_id_1", + server_metadata_url="https://metadata.url", + client_kwargs={"client": "kwargs"}, + authorize_params={"scheme": "test_scheme"}, + ) From 06186fa04ff990978f6b6fe10049aecea576a834 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 12 Jun 2024 22:12:06 +0000 Subject: [PATCH 5/5] docs(oauth): update details around client registration --- docs/configuration/oauth.md | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/configuration/oauth.md b/docs/configuration/oauth.md index 6966a5d10..b8f33ba66 100644 --- a/docs/configuration/oauth.md +++ b/docs/configuration/oauth.md @@ -31,14 +31,9 @@ The [data migration file](./data.md) contains sample values for an `AuthProvider The [`benefits.oauth.client`][oauth-client] module defines helpers for registering OAuth clients, and creating instances for use in e.g. views. -- `register_providers(oauth_registry)` uses data from `AuthProvider` instances to register clients into the given registry - `oauth` is an `authlib.integrations.django_client.OAuth` instance -Providers are registered into this instance once in the [`OAuthAppConfig.ready()`][oauth-app-ready] function at application -startup. +Consumers call `benefits.oauth.client.create_client(oauth, provider)` with the name of a client to obtain an Authlib client +instance. If that client name has not been registered yet, `_register_provider(oauth_registry, provider)` uses data from the given `AuthProvider` instance to register the client into this instance and returns the client object. -Consumers call `oauth.create_client(client_name)` with the name of a previously registered client to obtain an Authlib client -instance. - -[oauth-app-ready]: https://github.com/cal-itp/benefits/blob/dev/benefits/oauth/__init__.py [oauth-client]: https://github.com/cal-itp/benefits/blob/dev/benefits/oauth/client.py