OAuth2: Support public clients using Authorization Code + PKCE (no client secret) #9534
Conversation
…andling and configuration options
…ng tests to mock claims extraction
…ts to mock userinfo handling
…ients in authentication logic
WalkthroughAdds OIDC support and PKCE-aware handling to OAuth2 authentication: documentation expanded for OAuth2 vs OIDC, login flow prefers ID token claims with userinfo fallback, public clients must use PKCE, and code/tests updated to validate OIDC behaviors and PKCE registration/flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as pgAdmin App
participant OAuth as OAuth2 Provider
participant IdP as Identity Provider
User->>App: Click "Login with OAuth2"
App->>App: Determine client type and PKCE config
alt Public client (PKCE)
App->>App: Generate code_challenge/code_verifier
App->>OAuth: /authorize (code_challenge, scope=openid?)
else Confidential client
App->>OAuth: /authorize (client_id, scope)
end
OAuth->>IdP: Request authentication
IdP->>User: Prompt credentials
User->>IdP: Submit credentials
IdP->>OAuth: Issue authorization grant
OAuth->>App: Redirect with authorization code
alt Public client (PKCE)
App->>OAuth: /token (code, code_verifier, no client_secret)
else Confidential client
App->>OAuth: /token (code, client_secret)
end
OAuth->>App: Respond with access_token (+ id_token if OIDC)
alt OIDC provider (id_token present)
App->>App: Extract claims from id_token
App->>App: Resolve username from id_token (custom claim / email / preferred_username / sub)
alt Claims insufficient
App->>OAuth: GET /userinfo
OAuth->>App: Return userinfo
end
else Non-OIDC provider
App->>OAuth: GET /userinfo
OAuth->>App: Return userinfo
end
App->>App: Validate/create session and authorize
App->>User: Grant access
sequenceDiagram
participant Test as Test Case
participant Auth as OAuth2 Auth Module
participant MockReg as Mock OAuth.register
Test->>Test: _reset_oauth2_state()
Test->>Test: Configure OAUTH2 sources
Test->>MockReg: Register providers (github, public-pkce, oidc-*)
alt PKCE authentication test
Test->>Auth: authenticate() with PKCE
Auth->>Auth: Validate PKCE params
Auth->>MockReg: Exchange code with code_verifier
MockReg->>Auth: Return tokens + id_token
Auth->>Test: Return profile (from id_token or userinfo)
else Public client missing PKCE
Test->>Auth: register public client without PKCE
Auth-->>Test: Raise ValueError (PKCE required)
else OIDC id_token-only test
Test->>Auth: get_user_profile() with id_token
Auth->>Auth: Extract claims and skip userinfo
Auth->>Test: Return profile from id_token
end
Test->>Test: Assert session state (logged in / not logged in)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/en_US/oauth2.rst (1)
49-54: Minor typo: capitalize "you" at start of sentence.📝 Suggested fix
- for this parameter. you can modify the value as follows: + for this parameter. You can modify the value as follows:
🧹 Nitpick comments (1)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (1)
285-301: Test state reset is thorough but relies on internal access.The
_reset_oauth2_statemethod properly isolates test scenarios by clearing singleton caches. While accessingAuthSourceRegistry._objectsdirectly is fragile, it's acceptable for test infrastructure. Consider adding a comment noting this dependency on internal state.
📜 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/browser/tests/test_oauth2_with_mocking.py (1)
web/pgadmin/authenticate/oauth2.py (4)
authenticate(560-572)OAuth2Authentication(96-616)login(336-476)get_user_profile(478-558)
🪛 Ruff (0.14.10)
web/pgadmin/authenticate/oauth2.py
147-153: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Possible hardcoded password assigned to: "token_endpoint_auth_method"
(S105)
193-193: Unused method argument: form
(ARG002)
336-336: Unused method argument: form
(ARG002)
web/pgadmin/browser/tests/test_oauth2_with_mocking.py
122-124: Avoid specifying long messages outside the exception class
(TRY003)
296-296: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
332-332: Do not catch blind exception: Exception
(BLE001)
349-349: Unused function argument: self
(ARG001)
384-384: Unused function argument: self
(ARG001)
🔇 Additional comments (16)
web/pgadmin/browser/tests/__init__.py (1)
14-22: LGTM!The no-op
setUpandtearDownhooks provide standard test lifecycle scaffolding. The comment clearly documents the purpose as a placeholder for test discovery.docs/en_US/oauth2.rst (3)
14-37: Excellent documentation for OAuth2 vs OIDC distinction.The comparison section clearly explains when ID tokens are used vs userinfo endpoints, and the note about
OAUTH2_SERVER_METADATA_URLtriggering OIDC mode aligns with the implementation in_is_oidc_provider().
109-126: Public vs Confidential clients section is well documented.The explanation of PKCE requirements for public clients and the note about Authlib's unauthenticated token exchange matches the implementation logic in
OAuth2Authentication.__init__.
182-211: Username resolution documentation matches implementation.The documented priority order (OAUTH2_USERNAME_CLAIM → email → preferred_username → sub) correctly reflects the
_resolve_usernamemethod logic in oauth2.py.web/pgadmin/authenticate/oauth2.py (6)
126-153: PKCE validation logic is sound.The detection (
pkce_is_configured) triggers if either PKCE parameter is set, while the public client validation requires bothOAUTH2_CHALLENGE_METHOD="S256"andOAUTH2_RESPONSE_TYPE="code". This correctly catches partial PKCE configurations for public clients.Note: The static analysis hint S105 on line 178 (
token_endpoint_auth_method = 'none') is a false positive—'none'is a valid OAuth2 token endpoint authentication method, not a password.
196-234: OIDC detection and claim extraction helpers are well implemented.The
_is_oidc_providermethod correctly usesOAUTH2_SERVER_METADATA_URLas the OIDC indicator. The_get_id_token_claimsmethod safely extracts claims from Authlib's token response structure with appropriate defensive checks.
248-334: Username resolution logic is comprehensive and well-documented.The OIDC-aware fallback chain (custom claim → email → preferred_username → sub) correctly implements the documented behavior. The logging at each resolution step aids debugging. The non-OIDC fallback to email maintains backward compatibility.
407-442: Well-structured claim validation with OIDC priority.The validation correctly checks ID token claims first for OIDC providers, then falls back to userinfo profile. This aligns with the documented behavior and reduces unnecessary network calls when ID token contains sufficient claims.
489-535: Efficient OIDC profile retrieval with userinfo optimization.The logic correctly skips the userinfo endpoint call when the ID token contains sufficient claims (email, preferred_username, or sub) and all configured authorization claims. This optimization reduces latency and network overhead for OIDC flows.
560-572: Defensive handling of missing OAuth2 provider selection.Using
request.form.get('oauth2_button')instead of direct dictionary access preventsBadRequestKeyErrorwhen the form doesn't contain the OAuth2 button. The explicit error message improves user experience.web/pgadmin/authenticate/__init__.py (1)
229-250: Auth source filtering correctly respects user's login method choice.The POST-only guard and explicit button handling ensure authentication sources are properly filtered based on the user's selection:
internal_button→ filters to internal/LDAP authoauth2_button→ filters to OAuth2 onlyThis prevents the previous issue where internal login could incorrectly attempt OAuth2 authentication.
web/pgadmin/browser/tests/test_oauth2_with_mocking.py (5)
25-109: Comprehensive test scenario coverage.The test scenarios thoroughly cover:
- External authentication redirects
- Login success/failure paths
- PKCE registration (both public and confidential clients)
- OIDC ID token claim handling
- Username claim precedence
- Additional claims authorization
This aligns well with the implementation changes in oauth2.py.
314-336: External redirect test correctly verifies expected behavior.The generic
Exceptioncatch at line 332 is intentional—the test validates that the test client correctly raises an error when encountering external redirects. The exception message assertion confirms the expected behavior.
338-408: Login success/failure tests properly mock OAuth2 flow.The
_fake_authenticateand_fake_get_user_profilefunctions correctly set up the OAuth2 state including ID token claims in the session. The static analysis hint about unusedselfis a false positive—these functions replace instance methods and require the parameter for the patch to work correctly.
475-503: Public client PKCE validation test ensures fail-fast behavior.The test correctly verifies that a public client configuration without PKCE raises
ValueErrorduring initialization, and thatOAuth.registeris never called. This ensures misconfigurations are caught early.
505-553: OIDC userinfo optimization tests validate correct behavior.The tests properly verify:
- When ID token contains sufficient claims (
sub), the userinfo endpoint is not called- When ID token lacks claims, the userinfo endpoint is called
The use of
AssertionErroras a side effect for the skip test is a clever way to ensure the optimization works correctly.
|
PR reported in #9536 |
1 participant
This PR builds on top of the OIDC support introduced in #9525 (comment) and is not meant to be merged independently.
This PR adds first-class support for OAuth2 public clients in pgAdmin, enabling secure authentication without storing a client secret.
It allows pgAdmin to authenticate against modern OAuth2 / OIDC providers (such as Microsoft Entra ID) using the Authorization Code flow with PKCE, which is now the recommended approach for browser-based and containerized applications.
Why this change is needed
Many OAuth2 providers (including Entra ID) support public clients that:
Prior to this change, pgAdmin implicitly required a client_secret, preventing:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.