Warn when OAuth2 provider settings are misplaced at the top level of the config#10113
Warn when OAuth2 provider settings are misplaced at the top level of the config#10113dpage wants to merge 2 commits into
Conversation
…the config pgAdmin reads OAuth2 provider settings only from the OAUTH2_CONFIG list, so values supplied at the top level of the configuration (for example a bare OAUTH2_CLIENT_ID or OAUTH2_SSL_CERT_VERIFICATION) are silently ignored. This is an easy trap in container deployments, where every other option is set via an individual PGADMIN_CONFIG_<KEY> environment variable and it is natural to assume PGADMIN_CONFIG_OAUTH2_CLIENT_ID and friends behave the same way, when in fact the whole provider list must be supplied through a single PGADMIN_CONFIG_OAUTH2_CONFIG. Detect this at startup and log an actionable warning, turning a silent misconfiguration into a diagnosable one, and document the supported approach for container deployments. Closes pgadmin-org#10053
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesOAuth2 misplaced config detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary
pgAdmin reads OAuth2 provider settings only from the
OAUTH2_CONFIGlist; each provider is a single dictionary entry within it. Settings supplied at the top level of the configuration (for example a bareOAUTH2_CLIENT_IDorOAUTH2_SSL_CERT_VERIFICATION) are silently ignored.This is an easy trap in container deployments: nearly every other option is set with an individual
PGADMIN_CONFIG_<KEY>environment variable, so it is natural to assumePGADMIN_CONFIG_OAUTH2_CLIENT_ID,PGADMIN_CONFIG_OAUTH2_SSL_CERT_VERIFICATIONand friends will behave the same way. They do not: OAuth2 must be configured through a singlePGADMIN_CONFIG_OAUTH2_CONFIGholding the whole provider list. The result is that a user's settings, including their attempt to disable SSL certificate verification for a self-signed provider, are dropped without any feedback.This change:
warn_on_misplaced_oauth2_config(), called once at startup from the OAuth2 module'sinit_app, which logs an actionable warning when per-providerOAUTH2_*keys are present at the top level of the configuration and no provider is configured inOAUTH2_CONFIG. There is no behavioural change: a correctly configured deployment, or one with no OAuth2 settings at all, stays silent.oauth2.rst, making clear that individualPGADMIN_CONFIG_OAUTH2_*variables do not configure a provider.This is deliberately a fail-loud fix rather than a compatibility shim:
OAUTH2_CONFIGremains the single source of truth, and we avoid quietly expanding the supported configuration surface (and the reach ofverify=False).Note on the "regression" in the linked issue: the OAuth2
verifyhandling did not change in code between the affected releases. The actual trigger is stricter certificate verification (OpenSSL/urllib3) in the newer image rejecting a CA certificate whose Basic Constraints extension is not marked critical; the user'sOAUTH2_SSL_CERT_VERIFICATION=Falseescape hatch never took effect because it was supplied as an individual environment variable. This change makes that misconfiguration visible.Test plan
test_oauth2_misplaced_config.pycovering: warns on misplaced keys + unconfiguredOAUTH2_CONFIG; quiet when a provider is configured; quiet for legitimate top-level settings only; quiet with no OAuth2 config.test_oauth2_with_mockingstill passes (58/58).pycodestyleclean on changed Python files.Closes #10053
Summary by CodeRabbit
Documentation
OAUTH2_CONFIGlist; other top-level OAuth2 keys are ignored.PGADMIN_CONFIG_OAUTH2_CONFIG, and described the startup warning behavior.New Features
Tests