Skip to content

fix(oauth2): log database token failures - #42644

Draft
aminghadersohi wants to merge 3 commits into
apache:masterfrom
aminghadersohi:aminghadersohi/log-oauth2-db-auth-failures
Draft

fix(oauth2): log database token failures#42644
aminghadersohi wants to merge 3 commits into
apache:masterfrom
aminghadersohi:aminghadersohi/log-oauth2-db-auth-failures

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Why

OAuth2 database token exchange and refresh failures lacked queryable context, and the callback emitted an outcome-neutral event metric that did not represent failures.

What

  • Log token exchange and refresh failures with database_id, engine, and exception type while excluding OAuth codes, tokens, and provider payloads.
  • Exclude OAuth callback query, form, and JSON data from event logs so authorization codes, state, scope, and provider error payloads are never persisted.
  • Use the standard REST API StatsD decorator for the OAuth2 callback so DatabaseRestApi.oauth2.success, .warning, and .error outcomes are distinguishable, while disabling the old duplicate unqualified event counter.
  • Add focused coverage for exchange logs, refresh logs, and callback success/error metrics.

Blast radius

Limited to OAuth2 database token exchange/refresh error handling and callback observability. Existing exceptions and token cleanup behavior are preserved.

TESTING INSTRUCTIONS

pytest -q tests/unit_tests/commands/databases/oauth2_test.py tests/unit_tests/utils/oauth2_tests.py tests/unit_tests/databases/oauth2_api_test.py tests/unit_tests/databases/api_test.py -k oauth2 --disable-warnings
uvx pre-commit run --files UPDATING.md superset/commands/database/oauth2.py superset/databases/api.py superset/utils/log.py superset/utils/oauth2.py tests/unit_tests/commands/databases/oauth2_test.py tests/unit_tests/databases/oauth2_api_test.py tests/unit_tests/utils/oauth2_tests.py

The OAuth2 test selection passes with 39 tests. All changed-file pre-commit hooks pass.

A full-repository pre-commit run was also attempted; it reaches unrelated existing mypy errors in version-restore tests and frontend custom-rule checks cannot load the uninstalled glob package in this worktree.

RISK & ROLLBACK

Low risk: no success-path or persistence semantics change. Roll back this commit to restore the prior logging and metric behavior.

REVIEW GUIDANCE

Please focus on log-field usefulness/redaction and the callback decorator ordering, which ensures outcome metrics wrap transaction completion.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@github-actions github-actions Bot added the api Related to the REST API label Jul 31, 2026
Comment thread superset/commands/database/oauth2.py Fixed
@bito-code-review

Copy link
Copy Markdown
Contributor

The logging changes in this pull request are designed to avoid logging sensitive information by using structured logging that captures only metadata, such as database_id, engine, and error_type. The code explicitly avoids logging the raw exception object or user-specific data that might contain credentials. These changes are appropriate and follow security best practices for structured logging.

superset/commands/database/oauth2.py

except Exception as ex:
            logger.error(
                "OAuth2 token exchange failed: database_id=%s engine=%s "
                "error_type=%s",
                self._state["database_id"],
                self._database.backend,
                type(ex).__name__,
            )

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.42%. Comparing base (22c305f) to head (3f50169).

Files with missing lines Patch % Lines
superset/commands/database/oauth2.py 25.00% 6 Missing ⚠️
superset/utils/oauth2.py 0.00% 2 Missing ⚠️
superset/utils/log.py 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42644      +/-   ##
==========================================
- Coverage   65.43%   65.42%   -0.01%     
==========================================
  Files        2810     2810              
  Lines      159460   159385      -75     
  Branches    36396    36367      -29     
==========================================
- Hits       104342   104282      -60     
+ Misses      53075    53060      -15     
  Partials     2043     2043              
Flag Coverage Δ
hive 38.07% <55.00%> (+<0.01%) ⬆️
mysql 57.79% <55.00%> (-0.01%) ⬇️
postgres 57.84% <55.00%> (+<0.01%) ⬆️
presto 39.96% <55.00%> (+<0.01%) ⬆️
python 59.22% <55.00%> (+<0.01%) ⬆️
sqlite 57.46% <55.00%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aminghadersohi
aminghadersohi marked this pull request as ready for review August 1, 2026 00:41
@dosubot dosubot Bot added authentication:sso Single Sign On logging Creates a UI or API endpoint that could benefit from logging. labels Aug 1, 2026
Comment thread superset/databases/api.py
return self.response_404()

@expose("/oauth2/", methods=["GET"])
@statsd_metrics

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The newly added statsd_metrics wrapper emits the failure counter from its exception handler without protecting self.incr_stats(...). If the configured StatsD client fails while recording an OAuth2 error, that secondary exception replaces the original callback failure and can change the response and traceback presented to the client. Make metric emission best-effort so observability failures cannot mask the OAuth2 exception. [error handling]

Severity Level: Minor 🧹
- ❌ OAuth2 failure responses can expose a StatsD error instead.
- ⚠️ Original token-exchange diagnostics can be lost.
- ⚠️ Callback error handling depends on StatsD availability.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/databases/api.py
**Line:** 1457:1457
**Comment:**
	*Error Handling: The newly added `statsd_metrics` wrapper emits the failure counter from its exception handler without protecting `self.incr_stats(...)`. If the configured StatsD client fails while recording an OAuth2 error, that secondary exception replaces the original callback failure and can change the response and traceback presented to the client. Make metric emission best-effort so observability failures cannot mask the OAuth2 exception.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

@aminghadersohi
aminghadersohi marked this pull request as draft August 1, 2026 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API authentication:sso Single Sign On logging Creates a UI or API endpoint that could benefit from logging. size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants