From 5948f59349b1787a6a942b5ffb1fd68e0ccd55c4 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 26 Aug 2026 15:03:38 +0100 Subject: [PATCH 1/4] Fix OAuth2 userinfo request crash when endpoint is present but None get_user_profile() tested key presence ('OAUTH2_USERINFO_ENDPOINT' not in ...) rather than truthiness, so a config copied from the shipped config.py template - which ships OAUTH2_USERINFO_ENDPOINT: None - would pass the check and call client.get(None), raising requests.exceptions.MissingSchema instead of skipping the call. Closes #10349 --- web/pgadmin/authenticate/oauth2.py | 4 +- .../tests/test_oauth2_userinfo_endpoint.py | 59 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py diff --git a/web/pgadmin/authenticate/oauth2.py b/web/pgadmin/authenticate/oauth2.py index 4e04b5b23be..806744619b7 100644 --- a/web/pgadmin/authenticate/oauth2.py +++ b/web/pgadmin/authenticate/oauth2.py @@ -744,8 +744,8 @@ def get_user_profile(self): # For non-OIDC providers or when ID token is insufficient, # call the userinfo endpoint - if 'OAUTH2_USERINFO_ENDPOINT' not in self.oauth2_config[ - self.oauth2_current_client]: + if not self.oauth2_config[ + self.oauth2_current_client].get('OAUTH2_USERINFO_ENDPOINT'): if self._is_oidc_provider(): # OIDC provider should have provided claims in ID token current_app.logger.warning( diff --git a/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py new file mode 100644 index 00000000000..65df0beaa68 --- /dev/null +++ b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py @@ -0,0 +1,59 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Verify that an OAUTH2_USERINFO_ENDPOINT of None (the shipped config +template default) is treated the same as an absent key, rather than being +passed to the HTTP client and crashing (issue #10349). +""" + +import sys +from unittest.mock import MagicMock, patch + +from pgadmin.utils.route import BaseTestGenerator + + +class OAuth2UserinfoEndpointNoneTestCase(BaseTestGenerator): + """Exercises get_user_profile() directly - no server connection needed.""" + + def setUp(self): + pass + + def runTest(self): + # Resolved at call time, rather than imported at module load time: + # test_auth_gating (run earlier in this same package) deliberately + # forces pgadmin.authenticate.oauth2 to be re-imported, which would + # leave a module-level import here bound to a stale module object + # whose globals patch('...session', ...) below wouldn't reach. + oauth2_module = sys.modules['pgadmin.authenticate.oauth2'] + OAuth2Authentication = oauth2_module.OAuth2Authentication + + auth = OAuth2Authentication.__new__(OAuth2Authentication) + auth.oauth2_current_client = 'test_provider' + auth.oauth2_config = { + 'test_provider': { + 'OAUTH2_NAME': 'test_provider', + # Shipped config.py template default - key present, not set. + 'OAUTH2_USERINFO_ENDPOINT': None, + } + } + mock_client = MagicMock() + auth.oauth2_clients = {'test_provider': mock_client} + + with self.app.app_context(), \ + patch.object(auth, '_authorize_access_token', + return_value={'access_token': 'tok'}), \ + patch.object(auth, '_is_oidc_provider', + return_value=False), \ + patch('pgadmin.authenticate.oauth2.session', {}): + profile = auth.get_user_profile() + + self.assertEqual(profile, {}) + # The bug: client.get(None) raised requests.exceptions.MissingSchema + # instead of skipping the call. + mock_client.get.assert_not_called() From ada67c80db552e8654bde36de86adb76f1df4705 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Fri, 4 Sep 2026 10:19:30 +0100 Subject: [PATCH 2/4] Address review feedback on the OAuth2 userinfo None fix The new test looked up pgadmin.authenticate.oauth2 in sys.modules, which only works when something else has already imported it; in desktop mode the auth source registry never does, so the module raised KeyError when run on its own. Use importlib.import_module() instead, which returns the already-loaded module when there is one and so keeps the re-import concern the comment describes whilst working from a cold start. Whilst here, apply the same "is the value set?" rather than "is the key present?" treatment to OAUTH2_LOGOUT_URL and OAUTH2_ADDITIONAL_CLAIMS, both of which config.py also ships as None; behaviour is unchanged for a configured value, since the logout redirect was already guarded on the URL being truthy and a None claims config is treated as "no check to do". The userinfo endpoint is now read once via .get(), for consistency with the rest of the method, and the test's closing comment no longer claims a MissingSchema that a MagicMock never raised. --- web/pgadmin/authenticate/oauth2.py | 29 +++++++++---------- .../tests/test_oauth2_userinfo_endpoint.py | 21 +++++++++----- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/web/pgadmin/authenticate/oauth2.py b/web/pgadmin/authenticate/oauth2.py index 806744619b7..9addcccafff 100644 --- a/web/pgadmin/authenticate/oauth2.py +++ b/web/pgadmin/authenticate/oauth2.py @@ -600,13 +600,10 @@ def login(self, form): current_app.logger.error(error_msg) return False, error_msg - additional_claims = None - if 'OAUTH2_ADDITIONAL_CLAIMS' in self.oauth2_config[ - self.oauth2_current_client]: - - additional_claims = self.oauth2_config[ - self.oauth2_current_client - ]['OAUTH2_ADDITIONAL_CLAIMS'] + # config.py ships the key with a value of None, so test whether it + # is set rather than whether it is present. + additional_claims = self.oauth2_config.get( + self.oauth2_current_client, {}).get('OAUTH2_ADDITIONAL_CLAIMS') # For OIDC providers, check ID token claims first, then userinfo # For non-OIDC providers, check userinfo only @@ -689,10 +686,12 @@ def get_user_profile(self): session['pass_enc_key'] = session['oauth2_token']['access_token'] - if 'OAUTH2_LOGOUT_URL' in self.oauth2_config[ - self.oauth2_current_client]: - session['oauth2_logout_url'] = self.oauth2_config[ - self.oauth2_current_client]['OAUTH2_LOGOUT_URL'] + # As above, the shipped config.py sets this to None, so only stash + # it in the session when it actually holds a URL. + logout_url = self.oauth2_config.get( + self.oauth2_current_client, {}).get('OAUTH2_LOGOUT_URL') + if logout_url: + session['oauth2_logout_url'] = logout_url # For OIDC providers, parse the ID token JWT to extract claims. # We can skip the userinfo endpoint call if the ID token has @@ -744,8 +743,9 @@ def get_user_profile(self): # For non-OIDC providers or when ID token is insufficient, # call the userinfo endpoint - if not self.oauth2_config[ - self.oauth2_current_client].get('OAUTH2_USERINFO_ENDPOINT'): + userinfo_endpoint = self.oauth2_config.get( + self.oauth2_current_client, {}).get('OAUTH2_USERINFO_ENDPOINT') + if not userinfo_endpoint: if self._is_oidc_provider(): # OIDC provider should have provided claims in ID token current_app.logger.warning( @@ -758,8 +758,7 @@ def get_user_profile(self): return {} resp = self.oauth2_clients[self.oauth2_current_client].get( - self.oauth2_config[ - self.oauth2_current_client]['OAUTH2_USERINFO_ENDPOINT'], + userinfo_endpoint, token=session['oauth2_token'] ) resp.raise_for_status() diff --git a/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py index 65df0beaa68..f1f6000b6a1 100644 --- a/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py +++ b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py @@ -12,7 +12,7 @@ passed to the HTTP client and crashing (issue #10349). """ -import sys +import importlib from unittest.mock import MagicMock, patch from pgadmin.utils.route import BaseTestGenerator @@ -26,11 +26,14 @@ def setUp(self): def runTest(self): # Resolved at call time, rather than imported at module load time: - # test_auth_gating (run earlier in this same package) deliberately - # forces pgadmin.authenticate.oauth2 to be re-imported, which would - # leave a module-level import here bound to a stale module object - # whose globals patch('...session', ...) below wouldn't reach. - oauth2_module = sys.modules['pgadmin.authenticate.oauth2'] + # pgadmin.authenticate.oauth2 can be re-imported after this module + # is loaded, which would leave a module-level import here bound to a + # stale module object whose globals patch('...session', ...) below + # wouldn't reach. import_module returns the module that is already + # in sys.modules when there is one, and imports it otherwise, so + # this also works when nothing else has loaded it (in desktop mode + # the auth source registry doesn't). + oauth2_module = importlib.import_module('pgadmin.authenticate.oauth2') OAuth2Authentication = oauth2_module.OAuth2Authentication auth = OAuth2Authentication.__new__(OAuth2Authentication) @@ -53,7 +56,9 @@ def runTest(self): patch('pgadmin.authenticate.oauth2.session', {}): profile = auth.get_user_profile() + # Pre-fix, the None endpoint was handed straight to client.get(), so + # nothing raised: the MagicMock just returned a mock profile. It is + # these two assertions, rather than an exception, that prove the + # guard skips the call. self.assertEqual(profile, {}) - # The bug: client.get(None) raised requests.exceptions.MissingSchema - # instead of skipping the call. mock_client.get.assert_not_called() From 48020eb3935b986af6bff828667b5c892f169c61 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 11:49:39 +0100 Subject: [PATCH 3/4] Clear a stale OAuth2 logout URL when the provider has none With OAUTH2_LOGOUT_URL now only stored when it is set, a login with a provider that has no logout URL left behind whatever an earlier login had stored, where previously the None value overwrote it; a failed callback also left the URL it had already stored. Either way a later logout could redirect to the wrong provider. Drop the session key in both cases, and extend the test to cover the first. --- web/pgadmin/authenticate/oauth2.py | 9 ++++++++- .../authenticate/tests/test_oauth2_userinfo_endpoint.py | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/web/pgadmin/authenticate/oauth2.py b/web/pgadmin/authenticate/oauth2.py index 9addcccafff..f22af09fd0b 100644 --- a/web/pgadmin/authenticate/oauth2.py +++ b/web/pgadmin/authenticate/oauth2.py @@ -135,6 +135,9 @@ def oauth_authorize(): session.pop('oauth2_current_client', None) return redirect(get_safe_post_login_redirect()) session.pop('oauth2_current_client', None) + # get_user_profile() may already have stored the provider's logout + # URL before the login failed; don't leave it for a later logout. + session.pop('oauth2_logout_url', None) logout_user() flash(msg, MessageType.ERROR) return redirect(get_safe_post_login_redirect()) @@ -687,11 +690,15 @@ def get_user_profile(self): session['pass_enc_key'] = session['oauth2_token']['access_token'] # As above, the shipped config.py sets this to None, so only stash - # it in the session when it actually holds a URL. + # it in the session when it actually holds a URL, and otherwise drop + # any left over from an earlier login so that logout cannot redirect + # to a different provider's URL. logout_url = self.oauth2_config.get( self.oauth2_current_client, {}).get('OAUTH2_LOGOUT_URL') if logout_url: session['oauth2_logout_url'] = logout_url + else: + session.pop('oauth2_logout_url', None) # For OIDC providers, parse the ID token JWT to extract claims. # We can skip the userinfo endpoint call if the ID token has diff --git a/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py index f1f6000b6a1..4857ad20fbd 100644 --- a/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py +++ b/web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py @@ -48,12 +48,15 @@ def runTest(self): mock_client = MagicMock() auth.oauth2_clients = {'test_provider': mock_client} + # Left over from an earlier login with a provider that had one. + fake_session = {'oauth2_logout_url': 'https://idp.example.com/out'} + with self.app.app_context(), \ patch.object(auth, '_authorize_access_token', return_value={'access_token': 'tok'}), \ patch.object(auth, '_is_oidc_provider', return_value=False), \ - patch('pgadmin.authenticate.oauth2.session', {}): + patch('pgadmin.authenticate.oauth2.session', fake_session): profile = auth.get_user_profile() # Pre-fix, the None endpoint was handed straight to client.get(), so @@ -62,3 +65,6 @@ def runTest(self): # guard skips the call. self.assertEqual(profile, {}) mock_client.get.assert_not_called() + # OAUTH2_LOGOUT_URL is absent here, so the stale URL must not + # survive to be used by the next logout. + self.assertNotIn('oauth2_logout_url', fake_session) From 9a9df4ccad1e45cb4b78168ef2eaed5617ebaf04 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 12:55:05 +0100 Subject: [PATCH 4/4] Test that a failed OAuth2 callback clears the stored logout URL Add a scenario to the OAuth2 mocking tests that seeds the session with a provider and a logout URL, fails the login, and checks that the URL is gone afterwards. Like the rest of that test case it only runs with SERVER_MODE = True. --- .../browser/tests/test_oauth2_with_mocking.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/web/pgadmin/browser/tests/test_oauth2_with_mocking.py b/web/pgadmin/browser/tests/test_oauth2_with_mocking.py index dec0baa3260..37af0402db5 100644 --- a/web/pgadmin/browser/tests/test_oauth2_with_mocking.py +++ b/web/pgadmin/browser/tests/test_oauth2_with_mocking.py @@ -139,6 +139,12 @@ class Oauth2LoginMockTestCase(BaseTestGenerator): profile={}, id_token_claims=None, )), + ('OAuth2 Failed Callback Clears Stored Logout URL', dict( + oauth2_provider='github', + kind='callback_login_failure_clears_logout_url', + profile={}, + id_token_claims=None, + )), ('OAuth2 openid Scope Without Metadata URL Fails Fast', dict( oauth2_provider='oidc-no-metadata', kind='openid_without_metadata_url', @@ -327,6 +333,9 @@ def runTest(self): self._test_session_state_after_redirect(self.oauth2_provider) elif self.kind == 'callback_missing_provider_state': self._test_oauth2_callback_missing_provider_state() + elif self.kind == 'callback_login_failure_clears_logout_url': + self._test_oauth2_callback_failure_clears_logout_url( + self.oauth2_provider) elif self.kind == 'openid_without_metadata_url': self._test_openid_scope_without_metadata_url_fails_fast() else: @@ -785,6 +794,26 @@ def _test_oauth2_callback_missing_provider_state(self): # an unhandled exception. self.assertLess(res.status_code, 500) + def _test_oauth2_callback_failure_clears_logout_url(self, provider): + """A failed OAuth2 callback must drop any logout URL already stored + in the session, since get_user_profile() stores it before the login + can fail, and a later logout would otherwise redirect to it. + """ + with self.tester.session_transaction() as sess: + sess['oauth2_current_client'] = provider + sess['oauth2_logout_url'] = 'https://idp.example.com/logout' + + with patch('pgadmin.authenticate.AuthSourceManager.login', + return_value=(False, 'Login failed')): + res = self.tester.get('/oauth2/authorize', + follow_redirects=False) + + self.assertEqual(res.status_code, 302) + with self.tester.session_transaction() as sess: + self.assertNotIn('oauth2_logout_url', sess) + self.assertNotIn('oauth2_current_client', sess) + self._assert_oauth2_session_not_logged_in() + def _test_openid_scope_without_metadata_url_fails_fast(self): """'openid' in OAUTH2_SCOPE without OAUTH2_SERVER_METADATA_URL must fail fast with actionable guidance, before any network round-trip.