Skip to content

Fix OAuth2 userinfo request crash when endpoint is present but None - #10353

Open
dpage wants to merge 1 commit into
pgadmin-org:masterfrom
dpage:fix/issue-10349-oauth2-userinfo-none
Open

Fix OAuth2 userinfo request crash when endpoint is present but None#10353
dpage wants to merge 1 commit into
pgadmin-org:masterfrom
dpage:fix/issue-10349-oauth2-userinfo-none

Conversation

@dpage

@dpage dpage commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • OAuth2Authentication.get_user_profile() guarded the userinfo request with 'OAUTH2_USERINFO_ENDPOINT' not in self.oauth2_config[...], which tests key presence, not truthiness. config.py's shipped OAUTH2_CONFIG template entry ships 'OAUTH2_USERINFO_ENDPOINT': None, so a config copied from that template (as documented) has the key present with a None value — the check passes, and the code proceeds to call client.get(None), raising requests.exceptions.MissingSchema: Invalid URL 'None' instead of skipping the call (for an OIDC provider that supplies claims via ID token / discovery) or logging the intended "not configured" message.
  • Changed the check to test truthiness (if not self.oauth2_config[...].get('OAUTH2_USERINFO_ENDPOINT')), so an explicit None is treated the same as an absent key.

Test plan

  • Added web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py, exercising get_user_profile() directly with a config carrying OAUTH2_USERINFO_ENDPOINT: None, asserting the userinfo client is never called.
  • Confirmed the test fails against the pre-fix not in check and passes with the fix.
  • pycodestyle clean on the changed/added files.

Closes #10349

Summary by CodeRabbit

  • Bug Fixes

    • Improved OAuth2 authentication handling when the user information endpoint is missing or unavailable.
    • Prevented unnecessary requests to unavailable endpoints, returning an empty profile instead.
  • Tests

    • Added regression coverage for OAuth2 configurations without a user information endpoint.

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 pgadmin-org#10349
@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e40e68e-aa75-4716-9c3f-c17911e2377d

📥 Commits

Reviewing files that changed from the base of the PR and between bc58657 and 9f77a12.

📒 Files selected for processing (2)
  • web/pgadmin/authenticate/oauth2.py
  • web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


Walkthrough

The OAuth2 profile lookup now treats a missing or falsy OAUTH2_USERINFO_ENDPOINT as unavailable. A regression test verifies that an explicit None value returns an empty profile without making an HTTP request.

Changes

OAuth2 user-info handling

Layer / File(s) Summary
Guard unavailable endpoints and add regression coverage
web/pgadmin/authenticate/oauth2.py, web/pgadmin/authenticate/tests/test_oauth2_userinfo_endpoint.py
The endpoint check now handles missing or falsy values. The regression test verifies that None returns {} without calling the HTTP client.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 9f77a

This PR prevents an invalid userinfo request when the endpoint is explicitly unset and adds coverage for that behavior; no actionable merge-blocking risk remains.

Suggested reviewers: asheshv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the OAuth2 crash caused by a userinfo endpoint set to None.
Linked Issues check ✅ Passed The change satisfies issue #10349 by treating a missing or None OAUTH2_USERINFO_ENDPOINT as unconfigured and adding a regression test that confirms no HTTP request occurs.
Out of Scope Changes check ✅ Passed All changes support issue #10349. The implementation change and regression test are directly related to the reported OAuth2 crash.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kundansable kundansable added this to the 9.18 milestone Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth2: userinfo request crashes with MissingSchema when OAUTH2_USERINFO_ENDPOINT is present but None

2 participants