Skip to content

fix(oauth): preserve provider error detail on refresh token failure - #1138

Draft
Anatolii Yatsuk (tolik0) wants to merge 1 commit into
mainfrom
tolik0/oauth-preserve-provider-error-detail
Draft

fix(oauth): preserve provider error detail on refresh token failure#1138
Anatolii Yatsuk (tolik0) wants to merge 1 commit into
mainfrom
tolik0/oauth-preserve-provider-error-detail

Conversation

@tolik0

Copy link
Copy Markdown
Contributor

What

When an OAuth refresh request is rejected, AbstractOauth2Authenticator._make_handled_request raised an AirbyteTracedException whose user-facing message was one fixed sentence, and it threw away the provider's own diagnostic. _wrap_refresh_token_exception had already parsed the error body with exception.response.json() in order to decide whether the failure was a refresh-token failure, and then discarded that parsed body.

That matters most for Microsoft Entra, where the body carries an AADSTS code that separates root causes needing completely different fixes:

  • AADSTS50173 — the grant was revoked, typically because the user changed or reset their password. Re-authenticating is the fix.
  • AADSTS7000218 / AADSTS700025 — the app registration's client type or client credential is misconfigured. Re-authenticating does not help; the app registration has to change.
  • AADSTS50076 / AADSTS50078 / AADSTS700082 — Conditional Access requires an interactive sign-in or MFA. Again a different fix, and one the workspace admin has to make.

Today all three collapse into the same string. Across 300 sampled production failure summaries for source-bing-ads, zero contain an AADSTS code, so support has no way to tell these apart from the failure summary. See airbytehq/oncall#12835.

How

  • The error body is parsed once, in _make_handled_request, and passed into _wrap_refresh_token_exception through a new optional response_content argument, so the response is no longer parsed twice. The argument defaults to None and the method parses on demand when it is absent, so existing callers keep working.
  • internal_message carries the full provider response (HTTP <status>: <body>, truncated at 1000 characters), so the whole payload lands in the logs.
  • The user-facing message deliberately keeps the existing actionable sentence as its lead: "Refresh token was rejected by the OAuth provider (invalid, expired, or already used). Re-authenticate this source's credentials in its connection settings." Only after that is a short provider detail appended, as Provider error: <error>: <error_description>, built from the standard OAuth 2.0 error and error_description fields, collapsed to a single line and truncated at 200 characters. Two hundred characters is enough to keep the AADSTS<code> and the beginning of its description, since Entra puts the code at the front of error_description, while staying short enough that the failure summary is still readable. No raw provider blob becomes the primary message, and when the body has no usable error / error_description the message is byte-for-byte what it was before.
  • Bodies that are empty, non-JSON, or valid JSON but not an object now resolve to "no parsed content" rather than raising. Previously a JSON array body would have hit AttributeError on .get(...) inside the already-failing error path.
  • Everything surfaced, in both the user-facing and the internal message, is run through filter_secrets, and the authenticator's own refresh token and client secret are redacted explicitly on top of that, in case a provider echoes submitted credentials back in its payload. Only response bodies are read, so request headers, including Authorization, are never echoed.

No changelog entry or version bump is included: CONTRIBUTING.md states releases are drafted automatically by semantic-pr-release-drafter from the PR title, and the package version is computed by poetry-dynamic-versioning. CHANGELOG.md is frozen and points at GitHub Release Notes.

Test plan

New tests in unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py:

  • a parametrized test over the three Entra failure modes above, asserting that the AADSTS code and the full response body reach internal_message while message still starts with the re-authenticate guidance and then carries the provider code on a single line;
  • truncation of an oversized error_description;
  • redaction, using a payload that echoes the refresh token and client secret back;
  • empty, HTML, and JSON-array bodies falling back to the raw RequestException instead of raising something new;
  • a provider whose body has no error / error_description, asserting the user-facing message is exactly the unchanged sentence.

The existing test_refresh_access_token_wrapped assertion on message was relaxed from equality to startswith, since the wrapped case now appends Provider error: invalid_grant.

This repo is Poetry-managed, so uv run pytest cannot resolve the dev dependencies; the tests were run with the project's Poetry virtualenv:

$ python -m pytest unit_tests/sources/streams/http/requests_native_auth/ -q
======================== 49 passed, 3 warnings in 0.86s ========================

$ python -m pytest unit_tests/sources/declarative/auth -q
======================= 166 passed, 3 warnings in 34.80s =======================

$ ruff check airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
All checks passed!

$ ruff format --check airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
2 files already formatted

$ mypy --config-file mypy.ini airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Success: no issues found in 1 source file

🤖 Generated with Claude Code

When an OAuth refresh request is rejected, the CDK replaced the provider's
own diagnostic with one fixed sentence. `_wrap_refresh_token_exception`
already parsed the error body to decide whether the failure was a refresh
token failure and then discarded it.

For Microsoft Entra that body carries an AADSTS code which separates
completely different root causes: AADSTS50173 (grant revoked, e.g. the user
changed their password), AADSTS7000218 / AADSTS700025 (client type or secret
misconfiguration) and AADSTS50076 / AADSTS50078 / AADSTS700082 (Conditional
Access requiring an interactive sign-in). All of them collapsed into the same
string, and no AADSTS code reached production failure summaries.

The parsed body is now reused instead of being parsed a second time, the full
provider response goes to `internal_message` so it lands in the logs, and a
short single-line `error` / `error_description` detail is appended after the
existing actionable guidance in the user-facing message. Bodies that are
empty, non-JSON or not a JSON object degrade to the previous behaviour
without raising. Everything surfaced is run through secret redaction, and the
authenticator's own refresh token and client secret are redacted explicitly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@tolik0/oauth-preserve-provider-error-detail#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch tolik0/oauth-preserve-provider-error-detail

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

Copy link
Copy Markdown

PyTest Results (Fast)

4 372 tests  +9   4 361 ✅ +9   10m 16s ⏱️ + 1m 29s
    1 suites ±0      11 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit ef2c164. ± Comparison against base commit 4855c2d.

@github-actions

Copy link
Copy Markdown

PyTest Results (Full)

4 375 tests  +9   4 363 ✅ +9   13m 33s ⏱️ -27s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit ef2c164. ± Comparison against base commit 4855c2d.

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.

1 participant