Enhance OAUTH2 and OIDC authentication support #9525
Conversation
…andling and configuration options
WalkthroughAdds OpenID Connect (OIDC) discovery and ID-token aware flows to OAuth2 authentication: ID token validation and claim extraction, OIDC-first username/email resolution with userinfo fallback, guarded auth-source updates, expanded docs and examples, and data-driven tests (including PKCE) with centralized test helpers. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant PgAdmin as pgAdmin
participant OIDCProv as OIDC Provider
participant Userinfo as Userinfo Endpoint
participant DB as User Database
User->>PgAdmin: Click OAuth2/OIDC Login
PgAdmin->>OIDCProv: Fetch discovery metadata (server_metadata_url)
OIDCProv-->>PgAdmin: Return token_endpoint & auth_endpoint
PgAdmin->>OIDCProv: Redirect to authorization endpoint
OIDCProv->>OIDCProv: User authenticates
OIDCProv-->>PgAdmin: Return auth code (and possibly ID token)
PgAdmin->>OIDCProv: Exchange code for tokens (validate ID token if present)
OIDCProv-->>PgAdmin: Return access_token and ID token with claims
PgAdmin->>PgAdmin: Extract claims from ID token
alt ID token lacks required claims
PgAdmin->>Userinfo: Fetch user profile using access_token
Userinfo-->>PgAdmin: Return profile data
end
PgAdmin->>DB: Resolve/create user with resolved username/claims
DB-->>PgAdmin: User record
PgAdmin-->>User: Login successful, redirect
sequenceDiagram
participant User as User
participant PgAdmin as pgAdmin
participant OAuth2Prov as OAuth2 Provider
participant Userinfo as Userinfo Endpoint
participant DB as User Database
User->>PgAdmin: Click OAuth2 Login
PgAdmin->>OAuth2Prov: Redirect to configured authorization endpoint
OAuth2Prov->>OAuth2Prov: User authenticates
OAuth2Prov-->>PgAdmin: Return auth code
PgAdmin->>OAuth2Prov: Exchange code for access_token
OAuth2Prov-->>PgAdmin: Return access_token
PgAdmin->>Userinfo: Fetch user profile (userinfo used for non-OIDC)
Userinfo-->>PgAdmin: Return profile data
PgAdmin->>PgAdmin: Extract username from profile (OAUTH2_USERNAME_CLAIM or fallback)
PgAdmin->>DB: Resolve/create user
DB-->>PgAdmin: User record
PgAdmin-->>User: Login successful, redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @web/pgadmin/authenticate/oauth2.py:
- Around line 331-333: Replace the incorrect use of
current_app.logger.exception(error_msg) with current_app.logger.error(error_msg)
so you log the message without implying an exception/stacktrace; keep the
subsequent return False, gettext(error_msg) unchanged and ensure the same
error_msg variable is used when calling gettext.
- Around line 305-313: The code uses current_app.logger.exception() when no
exception is present (in the OIDC/ non-OIDC profile checks), which incorrectly
emits a traceback; change those calls to current_app.logger.error() (or
logger.warning() if preferred) so you log the error message without an exception
context, keeping the same error_msg variable and return behavior in the
functions that use _is_oidc_provider() and profile_dict checks.
🧹 Nitpick comments (1)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (1)
277-288: Consider catching a more specific exception type.The broad
except Exceptioncatch works for the test's purpose (verifying external redirect handling), but if the test framework raises a specific exception type for external redirects, catching that would be more precise and avoid masking unexpected errors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/en_US/oauth2.rstweb/pgadmin/authenticate/__init__.pyweb/pgadmin/authenticate/oauth2.pyweb/pgadmin/browser/tests/__init__.pyweb/pgadmin/browser/tests/test_oauth2_with_mocking.py
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/authenticate/__init__.py (1)
web/pgadmin/tools/user_management/__init__.py (1)
auth_sources(499-507)
🪛 Ruff (0.14.10)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py
248-248: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
284-284: Do not catch blind exception: Exception
(BLE001)
301-301: Unused function argument: self
(ARG001)
334-334: Unused function argument: self
(ARG001)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
288-288: Unused method argument: form
(ARG002)
🔇 Additional comments (14)
web/pgadmin/authenticate/__init__.py (1)
229-250: Well-structured guard logic for auth source mutation.The refactored
update_auth_sourcesmethod correctly prevents incorrect authentication source selection during login. The early return for non-POST requests and explicit button checks ensure that internal login attempts don't accidentally trigger OAuth2 flows.One consideration: if a POST request is made without either
internal_buttonoroauth2_button(e.g., a malformed request), the method returns without modifyingauth_sources, which preserves the full configured list. This seems like reasonable default behavior.web/pgadmin/authenticate/oauth2.py (5)
106-109: Good defensive initialization.Initializing
oauth2_current_clienttoNoneprevents potentialAttributeErrorwhen methods likeget_friendly_name()or_is_oidc_provider()are called beforeauthenticate()sets the client.
162-185: LGTM - Clean OIDC detection helpers.The
_is_oidc_provider()method correctly uses the presence ofOAUTH2_SERVER_METADATA_URLas the OIDC indicator. The_get_id_token_claims()safely retrieves claims with appropriate fallbacks to empty dict.
200-286: Well-structured username resolution with clear fallback hierarchy.The
_resolve_username()method implements a sensible resolution order:
- Configured
OAUTH2_USERNAME_CLAIM(from ID token, then profile)- OIDC standard claims: email → preferred_username → sub
- Non-OIDC: email required
The logging at each decision point provides good observability for debugging authentication issues.
416-463: Good optimization to skip userinfo endpoint for OIDC providers.The logic correctly:
- Checks if ID token contains sufficient claims (email, preferred_username, or sub)
- Returns ID token claims directly if sufficient, avoiding an extra network call
- Falls back to userinfo endpoint when needed
- Handles edge cases with appropriate logging
This aligns well with the OIDC specification where the ID token is the primary identity source.
465-477: Safer provider selection with explicit error handling.Using
request.form.get()instead of direct dictionary access preventsBadRequestKeyErrorand provides a clear error message when no OAuth2 provider is selected.docs/en_US/oauth2.rst (2)
14-37: Excellent documentation of OAuth2 vs OIDC differences.The explanation clearly differentiates between OAuth2 and OIDC, and the note about
OAUTH2_SERVER_METADATA_URLtriggering OIDC mode is helpful for users configuring their providers.
109-210: Comprehensive configuration examples.The documentation provides clear examples for:
- OIDC with discovery (recommended approach)
- Username resolution order
- Additional claims authorization
- Legacy OAuth2 without OIDC
This covers the main use cases and helps users migrate to OIDC where appropriate.
web/pgadmin/browser/tests/__init__.py (1)
13-24: Placeholder test class with explicit lifecycle methods.The added
setUp()andtearDown()methods are no-op implementations, which is appropriate for a smoke test that ensures the test package is discoverable. The comment clearly explains the purpose.web/pgadmin/browser/tests/test_oauth2_with_mocking.py (5)
25-91: Good data-driven test scenarios.The refactored scenarios provide comprehensive coverage for:
- External redirect flow
- Successful and failed logins
- PKCE workflow
- OIDC ID token claims usage
- Userinfo endpoint skip/call behavior
This approach makes it easy to add new scenarios without duplicating test logic.
237-253: Proper test isolation with state reset.The
_reset_oauth2_state()method ensures clean state for each test by clearing:
AuthSourceRegistry._objectssingleton cache- Per-app auth sources cache
OAuth2Authenticationclass-level cachesUsing
setattrforcurrent_app(line 248) is appropriate sincecurrent_appis a FlaskLocalProxyobject.
290-356: Clean test orchestrators for success and failure scenarios.The
_test_oauth2_login_successand_test_oauth2_login_failuremethods properly:
- Mock
authenticate()to set the current client- Mock
get_user_profile()to return test data and set up session state- Assert expected session state after login
The
selfparameter in inner functions (lines 301, 334) is required since they're patching instance methods.
358-390: PKCE parameter verification test.The test correctly verifies that PKCE configuration parameters are passed through to the OAuth client registration, including
code_challenge_method,response_type, and defaultclient_kwargs.
392-435: Good coverage for userinfo endpoint skip optimization.These tests validate the OIDC optimization:
_test_oidc_get_user_profile_skip_userinfo: Confirms userinfo endpoint is skipped when ID token contains sufficient claims_test_oidc_get_user_profile_calls_userinfo: Confirms userinfo endpoint is called as fallback when ID token lacks claimsThe use of
AssertionErrorin the mock and explicitassert_not_called()/assert_called_once()provides clear test assertions.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @web/pgadmin/authenticate/oauth2.py:
- Around line 292-297: The code logs full user data (profile, profile_dict,
id_token_claims) which can expose PII; update the debug logging in the OAuth2
handling code to avoid printing full structures and instead log only
non-sensitive metadata (e.g., presence/absence and keys) for variables
profile_dict and id_token_claims and when calling self._get_id_token_claims()
use current_app.logger.debug to output minimal info such as "profile_dict keys"
or "empty" and "id_token_claims keys" or "empty" rather than the full contents;
ensure you update the statements that call current_app.logger.debug for
profile/profile_dict/id_token_claims accordingly.
- Around line 230-284: The debug logs in the username/email extraction flow of
the OAuth2 handler leak PII (actual usernames, emails and sub values); update
the logging in the method that inspects
id_token_claims/profile_dict/username_claim (the block that calls
self._is_oidc_provider(), and references id_token_claims, profile_dict,
username_claim, and email) to avoid printing raw values—log only which claim was
used (e.g., "Using email from ID token as username" or "Found username claim
'preferred_username' in ID token") or log a redacted/masked version of the value
instead of the full identifier; keep existing log levels and messages otherwise.
- Around line 385-391: The audit message currently logs PII by including full
profile and id_token_claims; change the logic that builds audit_msg (the
f-string using username, additional_claims, profile, id_token_claims) to omit
claim values and instead log only non-sensitive metadata such as the username,
the additional_claims checked, and either the keys or counts of profile and
id_token_claims (e.g., profile.keys() or len(profile), id_token_claims.keys() or
len(id_token_claims)); update the call to current_app.logger.warning to use the
sanitized audit_msg so no full claim values are written to logs.
- Around line 360-378: The code passes the raw profile (which may be a list)
into __is_any_claim_valid and debug logs, causing AttributeError when
identity.get is called; change those usages to use the dict returned by
get_profile_dict (profile_dict) instead of profile: call
__is_any_claim_valid(profile_dict, additional_claims) and log profile_dict in
the current_app.logger.debug lines so __is_any_claim_valid and the debug output
always receive the dict representation produced by
get_profile_dict/get_user_profile.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/pgadmin/authenticate/oauth2.py
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/authenticate/oauth2.py (1)
web/pgadmin/authenticate/__init__.py (2)
login(94-100)login(309-323)
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
288-288: Unused method argument: form
(ARG002)
🔇 Additional comments (6)
web/pgadmin/authenticate/oauth2.py (6)
106-109: LGTM!Good defensive initialization. Setting
oauth2_current_clienttoNonepreventsAttributeErrorin edge cases where the property might be accessed beforeauthenticate()is called.
153-157: LGTM!Safe handling of missing provider with sensible fallback to the
OAUTH2constant.
162-171: LGTM!Clean OIDC provider detection based on the presence of
OAUTH2_SERVER_METADATA_URL.
173-185: LGTM!Defensive extraction of ID token claims with proper fallbacks.
416-455: LGTM!Good optimization to skip the userinfo endpoint call when the ID token already contains sufficient claims. The differentiated logging (warning for OIDC, error for non-OIDC) appropriately reflects the severity of missing endpoint configuration in each context.
465-477: LGTM!Using
request.form.get()instead of direct index access preventsBadRequestKeyErrorwhen the OAuth2 button isn't present in the form. The explicit error message when no provider is selected improves the user experience.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @web/pgadmin/authenticate/oauth2.py:
- Around line 288-345: The login() method can call get_user_profile() when
self.oauth2_current_client is None, causing an uncaught exception; add an early
defensive check at the start of login() that verifies self.oauth2_current_client
is set, and if not, log a clear error via current_app.logger.error and return
False, gettext("No OAuth2 provider available" or similar) to fail gracefully;
place this guard before calling get_user_profile() and other provider-dependent
methods (e.g., _get_id_token_claims(), _is_oidc_provider(), _resolve_username())
so the code never looks up the client with a None key.
🧹 Nitpick comments (3)
web/pgadmin/authenticate/oauth2.py (3)
153-158:get_friendly_name()behavior is reasonable, but the return value is effectively the provider key.
If the intent was a “display name”, consider a separate config key (e.g.,OAUTH2_DISPLAY_NAME) rather than returningOAUTH2_NAME(which is also used as the registry key).
200-287: Username/email resolution misses a key OIDC edge case: email may exist only in the ID token.
Whenprofile_dictis empty (or when you intentionally skip userinfo),Noneeven ifid_token_claims['email']is present—this can lead to creating a user without an email or failing downstream assumptions.Proposed patch (also pull email from ID token)
@@ - email = profile_dict[email_key[0]] if email_key else None + email = profile_dict[email_key[0]] if email_key else None + # If userinfo is skipped/unavailable, email may only exist in the ID token. + if not email and isinstance(id_token_claims, dict): + email = id_token_claims.get('email')
485-490: Prefer using the passedformarg (and silence Ruff warnings) instead ofrequest.formdirectly.
This improves testability and resolves RuffARG002(same forvalidate()andlogin()).Proposed patch
@@ - def authenticate(self, form): + def authenticate(self, form): @@ - self.oauth2_current_client = request.form.get('oauth2_button') + self.oauth2_current_client = (form or request.form).get('oauth2_button')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/pgadmin/authenticate/oauth2.py
🧰 Additional context used
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
288-288: Unused method argument: form
(ARG002)
🔇 Additional comments (1)
web/pgadmin/authenticate/oauth2.py (1)
106-110: Good: initializeself.oauth2_current_clientto avoid edge-caseAttributeError.
This makesget_friendly_name()/OIDC helpers safer in tests and unusual flows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/pgadmin/authenticate/oauth2.py (1)
456-486: Consider initializingneeds_userinfobefore the conditional block for robustness.The variable
needs_userinfois defined only inside theif has_sufficient_claims:block (line 466), and then referenced in the condition on line 477. While this works correctly due to short-circuit evaluation, it's fragile—future modifications adding an early return before line 466 could introduce aNameError.♻️ Suggested improvement
+ needs_userinfo = False if has_sufficient_claims: provider = self.oauth2_config.get( self.oauth2_current_client, {} ) username_claim = provider.get('OAUTH2_USERNAME_CLAIM') additional_claims = provider.get('OAUTH2_ADDITIONAL_CLAIMS') # If custom username claim or additional authorization # claims are configured, they may exist only in userinfo; # don't skip userinfo unless ID token has them. - needs_userinfo = False if username_claim and username_claim not in id_token_claims: needs_userinfo = True
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/pgadmin/authenticate/oauth2.py
🧰 Additional context used
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
288-288: Unused method argument: form
(ARG002)
🔇 Additional comments (7)
web/pgadmin/authenticate/oauth2.py (7)
107-109: LGTM!Good defensive initialization to prevent
AttributeErrorwhen methods like_is_oidc_provider()orget_friendly_name()are called beforeauthenticate()sets the provider.
162-171: LGTM!Clean helper method that correctly identifies OIDC providers by checking for the presence of
OAUTH2_SERVER_METADATA_URL. The null-safe access pattern handles the case when no provider is selected.
173-185: LGTM!The method safely extracts ID token claims with appropriate fallbacks. The docstring correctly documents Authlib's behavior of storing parsed ID token claims in the
userinfokey of the token response.
200-286: Well-structured username resolution with clear fallback logic.The method properly implements OIDC-aware username resolution with a sensible claim priority hierarchy. The documented resolution order (custom claim → email → preferred_username → sub) aligns with OIDC best practices.
288-349: LGTM!The login flow properly handles both OIDC and non-OIDC providers with appropriate validation and informative error messages. The debug logging of claim keys (not values) is helpful for troubleshooting without exposing sensitive data.
359-417: LGTM!The two-tier validation approach for OIDC providers (ID token claims first, then userinfo fallback) correctly prioritizes the ID token as the authoritative identity source while maintaining backward compatibility. The audit logging provides good visibility without exposing sensitive claim values.
511-516: LGTM!Good defensive change using
request.form.get()to avoidBadRequestKeyErrorwhen the OAuth2 button value is missing, with a clear user-facing error message.
…ng tests to mock claims extraction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/pgadmin/authenticate/oauth2.py (1)
195-203: Narrow the exception handling.Catching bare
Exceptionmasks different failure modes (network errors, JWT validation failures, parsing errors). Consider catching specific Authlib exceptions or at minimum logging the exception type.♻️ Suggested refinement
try: claims = client.parse_id_token(token) if isinstance(claims, dict): return claims - except Exception as e: + except (ValueError, KeyError, AttributeError) as e: current_app.logger.warning( - f'Failed to parse ID token via authlib: {e}' + f'Failed to parse ID token via authlib: {type(e).__name__}: {e}' )Note: Verify which specific exceptions Authlib raises for ID token parsing failures and adjust accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/pgadmin/authenticate/oauth2.pyweb/pgadmin/browser/tests/test_oauth2_with_mocking.py
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (1)
web/pgadmin/authenticate/oauth2.py (4)
authenticate(527-539)OAuth2Authentication(96-583)login(307-447)get_user_profile(449-525)
web/pgadmin/authenticate/oauth2.py (1)
web/pgadmin/authenticate/__init__.py (1)
validate(273-283)
🪛 Ruff (0.14.10)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py
248-248: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
284-284: Do not catch blind exception: Exception
(BLE001)
301-301: Unused function argument: self
(ARG001)
341-341: Unused function argument: self
(ARG001)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
199-199: Do not catch blind exception: Exception
(BLE001)
307-307: Unused method argument: form
(ARG002)
🔇 Additional comments (15)
web/pgadmin/authenticate/oauth2.py (8)
107-109: LGTM: Good defensive initialization.This safeguard prevents
AttributeErrorin edge cases whereoauth2_current_clientmight be accessed before being set duringauthenticate().
153-157: LGTM: Safe fallback for missing provider.The defensive check ensures
get_friendly_name()won't fail when called before a provider is selected, returning a sensible default.
162-171: LGTM: Clear OIDC provider detection.The method correctly identifies OIDC providers by checking for the presence of
OAUTH2_SERVER_METADATA_URLconfiguration.
219-305: LGTM: Well-structured username resolution with clear precedence.The method implements a sensible fallback hierarchy:
- Custom
OAUTH2_USERNAME_CLAIM(if configured)- For OIDC: email → preferred_username → sub
- For OAuth2: email only
The logging at each decision point aids debugging.
308-348: LGTM: Robust validation distinguishes OIDC from OAuth2 requirements.The logic correctly requires either ID token claims OR profile for OIDC providers (allowing ID-token-only authentication), while non-OIDC providers must have profile data.
378-413: LGTM: OIDC-aware additional claims validation.For OIDC providers, the code correctly checks ID token claims first (since they're cryptographically signed and trusted), then falls back to userinfo profile. For non-OIDC providers, it only validates against profile. This aligns with OIDC best practices.
463-502: LGTM: Smart optimization to skip userinfo endpoint when ID token is sufficient.The logic correctly determines when the userinfo endpoint can be skipped by checking:
- ID token has standard identity claims
- Custom
OAUTH2_USERNAME_CLAIM(if configured) is present in ID token- All
OAUTH2_ADDITIONAL_CLAIMSkeys are present in ID tokenThis optimization reduces latency and external calls while maintaining correctness.
530-532: LGTM: Safe form access prevents KeyError.Using
request.form.get('oauth2_button')instead of direct dictionary access preventsBadRequestKeyErrorwhen OAuth2 isn't the selected authentication method, improving user experience with a clearer error message.web/pgadmin/browser/tests/test_oauth2_with_mocking.py (7)
25-92: LGTM: Comprehensive OIDC test scenario coverage.The expanded scenarios test critical OIDC flows:
- ID token claims as primary identity source
- Fallback to userinfo profile
- Custom username claim precedence
- Additional claims authorization (success & failure paths)
- Userinfo endpoint optimization (skip vs. call)
The data-driven approach with the
kindfield makes the test matrix clear and maintainable.
160-202: LGTM: Well-structured OIDC provider configurations for testing.The test configurations cover key OIDC scenarios:
oidc-basic: Standard OIDC with server metadata URLoidc-username-claim: Custom username claim mappingoidc-additional-claims: Authorization via ID token claimsThese configurations enable thorough testing of the new OIDC-aware authentication logic.
237-253: LGTM: Centralized state management ensures test isolation.The
_reset_oauth2_state()helper correctly clears:
AuthSourceRegistrysingleton- Per-app cached auth sources
- OAuth2 class-level caches (
oauth2_clients,oauth2_config)This prevents test pollution from shared state.
290-330: LGTM: Realistic mocking of OIDC ID token flow.The
_test_oauth2_login_successmethod properly simulates OIDC authentication by:
- Setting up session with OAuth2 token including
id_token- Mocking
parse_id_token()to return test claims- Verifying successful login via session assertions
The pattern correctly separates concerns between authenticate phase and profile retrieval.
406-428: LGTM: Validates userinfo endpoint optimization.The test ensures that when ID token contains sufficient claims (
sub), the userinfo endpoint is not called. The use ofside_effect=AssertionErrorcombined withassert_not_called()provides strong verification of the optimization.
430-455: LGTM: Validates userinfo endpoint fallback.The test correctly verifies that when the ID token lacks standard claims (empty dict from
parse_id_token), the implementation falls back to calling the userinfo endpoint. This ensures the optimization doesn't break standard OAuth2 flows.
379-404: LGTM: PKCE validation confirms correct client registration.The test verifies that PKCE-configured providers pass the correct
client_kwargsto OAuth client registration:
code_challenge_method: 'S256'response_type: 'code'- Default kwargs (
scope,verify) are preservedThis ensures PKCE configuration is properly propagated.
…ts to mock userinfo handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/pgadmin/authenticate/oauth2.py (1)
455-501: Consider simplifying thehas_sufficient_claimscheck.The
any()with a list works but could be slightly more efficient as a generator:has_sufficient_claims = any( claim in id_token_claims for claim in ('email', 'preferred_username', 'sub') )However, the current implementation is clear and the performance difference is negligible for 3 elements.
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (2)
237-253: Test isolation approach is acceptable but consider documenting the fragility.Directly modifying
AuthSourceRegistry._objectsand class-level caches is a common test pattern but creates coupling to implementation details. Consider adding a comment noting that this may need updates if the singleton implementation changes.The
setattron line 248 is flagged by static analysis but is reasonable here since the attribute name is dynamic based on the conditional check.
284-288: Consider catching a more specific exception type.The broad
except Exceptionis flagged by static analysis. If the test framework raises a specific exception for external redirects, catching that would be cleaner. If not, consider adding a comment explaining why the broad catch is intentional.except Exception as e: + # Test framework raises a generic exception for external redirect self.assertEqual( 'Following external redirects is not supported.', str(e) )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/pgadmin/authenticate/oauth2.pyweb/pgadmin/browser/tests/test_oauth2_with_mocking.py
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (1)
web/pgadmin/authenticate/oauth2.py (3)
authenticate(526-538)login(302-442)get_user_profile(444-524)
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
302-302: Unused method argument: form
(ARG002)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py
248-248: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
284-284: Do not catch blind exception: Exception
(BLE001)
301-301: Unused function argument: self
(ARG001)
336-336: Unused function argument: self
(ARG001)
🔇 Additional comments (13)
web/pgadmin/authenticate/oauth2.py (7)
106-109: LGTM!Good defensive initialization of
oauth2_current_clienttoNoneto preventAttributeErrorin edge cases where methods are called beforeauthenticate()sets the provider.
153-157: LGTM!Safe fallback to
OAUTH2when the provider is not found prevents potential errors in edge cases.
162-171: LGTM!Using
OAUTH2_SERVER_METADATA_URLpresence as the OIDC indicator is a reasonable approach since OIDC discovery relies on the well-known metadata endpoint.
173-199: LGTM!Solid defensive programming with type checks and early returns. The approach of extracting claims from the
userinfokey in the token response aligns with Authlib's behavior for OIDC flows.
214-300: LGTM!Well-structured username resolution with clear priority ordering:
- Custom
OAUTH2_USERNAME_CLAIM(from ID token, then profile)- OIDC standard claims hierarchy (email → preferred_username → sub)
- Profile email fallback for non-OIDC
The logging at each decision point aids debugging without exposing sensitive claim values.
302-442: LGTM!The login flow correctly handles:
- Missing provider with early exit
- OIDC vs non-OIDC validation paths
- Claims validation with appropriate fallback
- Comprehensive audit logging with claim keys (not values) for security
The error messages provide actionable guidance for administrators.
526-531: LGTM!Using
form.get('oauth2_button')instead of direct dictionary access preventsBadRequestKeyErrorwhen the OAuth2 button wasn't clicked, providing a graceful error message instead.web/pgadmin/browser/tests/test_oauth2_with_mocking.py (6)
25-92: LGTM!The data-driven approach with the
kindfield provides clear test organization and good coverage of OAuth2 and OIDC scenarios, including edge cases for claims handling and userinfo fallbacks.
99-203: LGTM!Comprehensive test configuration with multiple providers covering OAuth2 and OIDC scenarios. The new OIDC providers correctly include
OAUTH2_SERVER_METADATA_URLto trigger OIDC-specific behavior.
290-325: LGTM!The mock functions correctly simulate the OAuth2 flow, and the session injection of
id_token_claimsviauserinfokey matches how Authlib returns OIDC token data. Theselfparameter in the nested functions is required since they're patched as instance methods.
327-360: LGTM!The failure test correctly verifies that when additional claims don't match (e.g.,
group-binstead ofgroup-a), the session is not logged in.
362-394: LGTM!Good verification that PKCE configuration parameters are correctly propagated to the OAuth client registration.
396-444: LGTM!These tests effectively validate the userinfo optimization:
- When ID token has sufficient claims, userinfo endpoint is skipped
- When ID token lacks claims, userinfo endpoint is called
The use of
side_effect=AssertionErroras a failsafe combined withassert_not_called()provides double verification.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web/pgadmin/authenticate/oauth2.py:
- Around line 173-199: _get_id_token_claims currently looks for a non-existent
'userinfo' key in the session token and thus always returns empty; instead, read
session['oauth2_token']['id_token'] and parse it using Authlib's parse_id_token
on the appropriate client
(self.oauth2_clients[self.oauth2_current_client].parse_id_token) or a safe JWT
decode, returning the resulting dict when successful and falling back to {} on
exceptions; ensure you still check token is a dict and handle missing id_token
gracefully.
🧹 Nitpick comments (1)
web/pgadmin/authenticate/oauth2.py (1)
276-281: Consider checkingpreferred_usernamein profile as fallback.The
preferred_usernameclaim is only checked inid_token_claims. If the ID token is minimal but the userinfo endpoint returnspreferred_username, it won't be found. For consistency with the email fallback pattern (lines 271-274), consider also checkingprofile_dict:Suggested enhancement
# Priority 2: preferred_username if 'preferred_username' in id_token_claims: username = id_token_claims['preferred_username'] current_app.logger.debug( 'Using preferred_username from ID token') return username, email + elif 'preferred_username' in profile_dict: + username = profile_dict['preferred_username'] + current_app.logger.debug( + 'Using preferred_username from profile') + return username, email
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/pgadmin/authenticate/oauth2.py
🧰 Additional context used
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
159-159: Unused method argument: form
(ARG002)
302-302: Unused method argument: form
(ARG002)
🔇 Additional comments (10)
web/pgadmin/authenticate/oauth2.py (10)
107-109: Good defensive initialization.Initializing
oauth2_current_clienttoNonein__init__prevents potentialAttributeErrorwhen methods like_is_oidc_provider()orget_friendly_name()are called beforeauthenticate()sets the client.
142-148: Good use of.get()for optional endpoint URLs.This correctly handles OIDC providers that use
OAUTH2_SERVER_METADATA_URLfor endpoint discovery, where explicit URLs may not be configured.
153-157: LGTM.Safe fallback to
OAUTH2when provider configuration is missing.
162-172: Clean OIDC provider detection.Using the presence of
OAUTH2_SERVER_METADATA_URLas the OIDC indicator is a reasonable heuristic since OIDC providers typically support discovery.
302-306: Good guard against missing provider.The static analysis hint about unused
formparameter is a false positive—this method signature is defined by theBaseAuthenticationinterface and must match even if the OAuth2 flow doesn't use form data.
311-327: Good security practice in logging.Logging only the claim keys (not values) provides useful debugging information without exposing sensitive user data.
373-408: Well-structured claims validation with appropriate fallback.The validation correctly tries ID token claims first, then falls back to userinfo profile for OIDC providers. This ensures authorization works even when claims are split between sources.
455-501: Good optimization to skip userinfo endpoint when ID token is sufficient.The logic correctly determines whether the userinfo call can be skipped by checking:
- Basic claims presence (
preferred_username,sub)- Custom username claim availability
- Additional authorization claims availability
This reduces latency for properly configured OIDC providers.
505-516: Appropriate error handling for missing userinfo endpoint.The differentiated logging between OIDC and non-OIDC providers helps with debugging—OIDC providers should have provided claims in the ID token, while non-OIDC providers require explicit configuration.
526-531: Good defensive change to handle missing form data.Using
request.form.get()preventsBadRequestKeyErrorand provides a user-friendly error message when no OAuth2 provider is selected.
2 participants
This change improves compatibility with OpenID Connect providers by treating ID token claims as the primary identity source and
falling back to userinfo when needed.
The existing OAuth2 flow is preserved and remains backward-compatible.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.