Skip to content

feat(oauth): add refresh_token_error_messages to OAuthAuthenticator - #1214

Open
ZaneHyattAB wants to merge 5 commits into
mainfrom
devin/1791305828-entra-aadsts-guidance
Open

ZaneHyattAB wants to merge 5 commits into
mainfrom
devin/1791305828-entra-aadsts-guidance

Conversation

@ZaneHyattAB

@ZaneHyattAB ZaneHyattAB commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds a vendor-neutral refresh_token_error_messages field to OAuthAuthenticator. A connector manifest can use it to replace the generic refresh-token rejection sentence with fixed, provider-specific text. It replaces the hardcoded Microsoft Entra map (_ENTRA_REFRESH_ERROR_GUIDANCE) that earlier revisions of this PR added. With the text owned by the connector, a wording change doesn't need a CDK release or fleet bumps, and the CDK carries no vendor catalogs. The design is Anatolii Yatsuk (@tolik0)'s (review on this PR).

Context: https://github.com/airbytehq/oncall/issues/12835. The first consumer is airbytehq/airbyte#88968 (source-bing-ads, draft until this is released).

authenticator:
  type: OAuthAuthenticator
  refresh_token_error_messages:
    AADSTS50078: "<fixed sentence>"

The change sits at the existing message site in AbstractOauth2Authenticator._make_handled_request, inside the existing _wrap_refresh_token_exception gate:

message = self._get_refresh_token_error_message(error_content) or _REFRESH_TOKEN_REJECTED_GUIDANCE
# candidates, first configured key wins:
#   _extract_provider_error_code(content)       e.g. "AADSTS50078"
#   " ".join(content["error_description"].split())
#   " ".join(content["error"].split())
# non-str / empty candidates are skipped
# unchanged: message += f" Provider error: {detail}", internal_message, FailureType.config_error

Semantics

  • Gate-limited. The messages apply only to responses that refresh_token_error_status_codes / _key / _values (or their defaults) already flag. They don't flag new responses and don't change failure_type. The schema field description says so.
  • Fallback. If there's no hit, the field is unset, or the value is empty, the generic sentence is used, byte-for-byte as today.
  • Fixed values. Values are used verbatim, with no interpolation, so the message stays the same across attempts and failure-summary grouping stays deterministic.
  • Scope. The field is on OAuthAuthenticator only, not the deprecated RefreshTokenUpdater copies.
  • Shared keys. Codes, descriptions and error values share one key space. A key like invalid_grant covers every invalid_grant that has no more specific hit.

Factory-defaults trap and its guard

_get_refresh_token_error_information returns the default gate (400,), "error", ("invalid_grant", "invalid_permissions") only when none of codes/key/values is set. If messages joined that "defined on OAuthAuthenticator" check, a manifest that sets only messages would get an empty gate. Bing Ads is exactly that case:

  • _wrap_refresh_token_exception would always return False.
  • The mapped text would never fire.
  • Today's invalid_grant config error would become a raw HTTPError.

The new _get_refresh_token_error_messages(model) resolves messages separately, and the 3-tuple helper is untouched. The regression guards are test_oauth_refresh_token_error_messages_only_keeps_default_gate and test_single_use_oauth_with_refresh_token_error_messages_keeps_default_gate.

Plumbing

Schema → generated model → factory (both the DeclarativeOauth2Authenticator and DeclarativeSingleUseRefreshTokenOauth2Authenticator constructions) → DeclarativeOauth2Authenticator → Oauth2Authenticator / SingleUseRefreshTokenOauth2Authenticator → AbstractOauth2Authenticator.__init__. In the native classes the new parameter is the last one, defaults to None, and is passed to super() by keyword, so existing positional callers keep working (test_abstract_authenticator_positional_init_still_supported).

The model was generated with poe assemble. On current main the generator also rewrites about 430 unrelated lines, so only the generated hunk for the new field is committed.

Kept / reverted from earlier revisions

  • Kept: _extract_provider_error_code and the determinism test (now driven by configured messages).
  • Removed: _ENTRA_REFRESH_ERROR_GUIDANCE and all Entra wording from the CDK.
  • Reverted: the AADSTS50173 assertion change in unit_tests/sources/declarative/auth/test_oauth.py.

Tests

  • test_requests_native_auth.py:
    • lookup order (code > description > error)
    • whitespace-normalized description
    • error-value match
    • no hit / None / {} give a byte-identical generic message
    • deterministic across attempts with differing trace IDs
    • not applied outside the gate
    • non-string fields
    • empty key/value fallback
    • positional init
    • single-use path
  • test_oauth.py: a declarative authenticator with configured messages.
  • test_model_to_component_factory.py:
    • messages-only keeps the default gate (regular and single-use)
    • an explicit gate plus messages
    • an absent field gives {}
  • test_concurrent_declarative_source.py: manifest validation accepts string values and rejects non-string ones.

Local results: the four touched test files gave 673 passed, 1 skipped. ruff check, ruff format --check and mypy pass. End-to-end, I validated the Bing Ads manifest against this branch and against CDK 7.35.0; the results are in airbytehq/airbyte#88968.

Link to Devin session: https://app.devin.ai/sessions/6164c1b453b74e53ba1c3a6b60f6e273
Open in Devin Desktop: https://app.devin.ai/desktop/session/6164c1b453b74e53ba1c3a6b60f6e273?variant=devin
Requested by: ZaneHyattAB

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Oct 6, 2026

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@devin/1791305828-entra-aadsts-guidance#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 devin/1791305828-entra-aadsts-guidance

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

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

PyTest Results (Fast)

5 783 tests  +29   5 752 ✅ +29   11m 56s ⏱️ +17s
    1 suites ± 0      31 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 09220fe. ± Comparison against base commit 73ccaa9.

This pull request removes 3 and adds 32 tests. Note that renamed tests count towards both.
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_client-AADSTS7000218: The request body must contain the following parameter: 'client_assertion' or 'client_secret'.\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS7000218]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_grant-AADSTS50076: Due to a configuration change made by your administrator, or because you moved to a new location, you must use multi-factor authentication to access the resource.\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS50076]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_grant-AADSTS50173: The provided grant has expired due to it being revoked, a fresh auth token is needed. The user might have changed or reset their password.\r\nTrace ID: 00000000-0000-0000-0000-000000000000\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS50173]
unit_tests.sources.declarative.auth.test_oauth.TestOauth2Authenticator ‑ test_refresh_access_token_rejected_uses_configured_message
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_oauth_refresh_token_error_messages_only_keeps_default_gate
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_oauth_refresh_token_error_messages_preserves_explicit_gate
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_single_use_oauth_with_refresh_token_error_messages_keeps_default_gate
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_accepts_string_values_in_manifest
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_rejects_non_string_values[123]
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_rejects_non_string_values[value1]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_abstract_authenticator_positional_init_still_supported
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_mapped_message_is_deterministic[AADSTS50078]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_mapped_message_is_deterministic[AADSTS700082]
…

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

PyTest Results (Full)

5 786 tests  +29   5 755 ✅ +29   14m 20s ⏱️ + 4m 12s
    1 suites ± 0      31 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 09220fe. ± Comparison against base commit 73ccaa9.

This pull request removes 3 and adds 32 tests. Note that renamed tests count towards both.
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_client-AADSTS7000218: The request body must contain the following parameter: 'client_assertion' or 'client_secret'.\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS7000218]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_grant-AADSTS50076: Due to a configuration change made by your administrator, or because you moved to a new location, you must use multi-factor authentication to access the resource.\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS50076]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_refresh_access_token_surfaces_provider_error_code[invalid_grant-AADSTS50173: The provided grant has expired due to it being revoked, a fresh auth token is needed. The user might have changed or reset their password.\r\nTrace ID: 00000000-0000-0000-0000-000000000000\r\nCorrelation ID: 11111111-2222-3333-4444-555555555555-AADSTS50173]
unit_tests.sources.declarative.auth.test_oauth.TestOauth2Authenticator ‑ test_refresh_access_token_rejected_uses_configured_message
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_oauth_refresh_token_error_messages_only_keeps_default_gate
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_oauth_refresh_token_error_messages_preserves_explicit_gate
unit_tests.sources.declarative.parsers.test_model_to_component_factory ‑ test_single_use_oauth_with_refresh_token_error_messages_keeps_default_gate
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_accepts_string_values_in_manifest
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_rejects_non_string_values[123]
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_refresh_token_error_messages_rejects_non_string_values[value1]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_abstract_authenticator_positional_init_still_supported
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_mapped_message_is_deterministic[AADSTS50078]
unit_tests.sources.streams.http.requests_native_auth.test_requests_native_auth.TestOauth2Authenticator ‑ test_mapped_message_is_deterministic[AADSTS700082]
…

♻️ This comment has been updated with latest results.

@ZaneHyattAB
ZaneHyattAB marked this pull request as ready for review October 13, 2026 22:52
@ZaneHyattAB
ZaneHyattAB requested a review from a team as a code owner October 13, 2026 22:52
@tolik0

Copy link
Copy Markdown
Contributor

Thanks for the thorough write-up and the controlled 50078 test — the finding that a confidential-client token survived the MFA expiry is useful on its own.

I'd push back on where the mapping lives, though. _ENTRA_REFRESH_ERROR_GUIDANCE puts one vendor's error codes and prose into AbstractOauth2Authenticator, which every OAuth connector shares:

  • Any rewording or new AADSTS code needs a CDK release and a fleet-wide bump instead of a connector change.
  • The next provider with its own codes would add a second vendor map next to it, and the CDK becomes the place where vendor error catalogs accumulate.
  • Manifest-only connectors have no way to override or extend the text, so a connector that knows better about its provider is stuck with whatever the CDK says.

The same outcome is reachable with a generic, connector-owned setting. The refresh call runs inside the authenticator before any error_handler, so an HttpResponseFilter in the manifest can't see the token endpoint's response — a small CDK hook is still needed, but it can be vendor-neutral:

authenticator:
  type: OAuthAuthenticator
  ...
  refresh_token_error_messages:
    AADSTS50078: "Microsoft Entra refused the refresh because the tenant's MFA policy ..."
    AADSTS70043: "..."
  • Plumbed exactly like refresh_token_error_status_codes / refresh_token_error_key / refresh_token_error_values (schema, models, _get_refresh_token_error_information, DeclarativeOauth2Authenticator, Oauth2Authenticator, SingleUseRefreshTokenOauth2Authenticator, AbstractOauth2Authenticator.__init__), including the "defined on either OAuthAuthenticator or RefreshTokenUpdater, not both" rule.
  • Lookup at the message site tries, in order: the code _extract_provider_error_code already pulls out, then the whitespace-normalized full error_description, then the RFC 6749 error value. First hit wins; no hit keeps today's generic sentence byte-for-byte. The _extract_provider_error_code split from this PR is reused as-is.
  • Values are fixed strings from the manifest; nothing from the response is interpolated, so the failure-summary grouping property you preserved here still holds. An error_description with per-request values (trace IDs, timestamps) simply never matches the exact-text key.

Why three key kinds rather than just the code: I checked recent prod failures that carry the Provider error: suffix from #1138. Entra is the only provider that leads error_description with a code, so a code-only map (hardcoded or generic) is effectively Bing Ads / Entra only. Other providers put the signal elsewhere: Google returns short fixed descriptions — "Token has been expired or revoked.", "Account has been deleted", "Bad Request" — that mean different things (re-auth / account gone / usually a client mismatch) and all collapse into the same sentence today; Greenhouse returns "Incorrect Token type or clientID", where re-auth doesn't help although the current text says it does; Uptick and Linear differ only in the error value (invalid_client, invalid_request). With exact-description and error keys, the same field serves the Google connectors, Greenhouse, Uptick and Linear, not only the five Entra manifests.

Concretely, I'd suggest: keep _extract_provider_error_code and the determinism tests from this PR, replace _ENTRA_REFRESH_ERROR_GUIDANCE with the refresh_token_error_messages field, and move the four AADSTS sentences into the Bing Ads manifest (and later the other Entra manifests) in airbytehq/airbyte. Happy to review that version, or to pair on the factory plumbing if useful.

def test_abstract_authenticator_positional_init_still_supported(self):
class PositionalInitAuthenticator(Oauth2Authenticator):
def __init__(self):
super(Oauth2Authenticator, self).__init__((400,), "error", ("invalid_grant",))
@devin-ai-integration devin-ai-integration Bot changed the title fix(oauth): map Entra AADSTS refresh errors to accurate user-facing guidance feat(oauth): add refresh_token_error_messages to OAuthAuthenticator Oct 14, 2026

@tolik0 Anatolii Yatsuk (tolik0) left a comment

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.

Approving — this is the connector-owned design from the earlier comment, implemented faithfully: the Entra catalog is out of the CDK, the lookup order (provider code → normalized error_description → error) is in place and tested, values stay verbatim so failure-summary grouping holds, and a messages-only manifest keeps the default gate (the Bing Ads case). I also checked locally that $parameters propagation through ManifestComponentTransformer leaves the map intact when the authenticator sits under a requester with stream-level $parameters, and that positional super().__init__ callers still work. The destination-motherduck failure is unrelated — it fails the same way on every recent connector-tests.yml run.

Three small things, none blocking:

  1. _get_refresh_token_error_message reads only error_description and error, while the gate honours a custom refresh_token_error_key. A provider that reports under e.g. errorCode can be gated but never mapped. Adding self._refresh_token_error_key as a last candidate closes that gap.
  2. The CodeQL hit on super(Oauth2Authenticator, self).__init__(...) in test_abstract_authenticator_positional_init_still_supported is a false positive — the call is intentional — but its suggested fix would change the test's meaning. Calling AbstractOauth2Authenticator.__init__(self, ...) directly, or a one-line comment, keeps the next reader from "fixing" it.
  3. The generated-model hunk is hand-committed and nothing in CI regenerates the model, so a follow-up that re-syncs declarative_component_schema.py in one commit would make the next poe assemble a clean diff.

One request before this goes into a release: please publish a CDK prerelease from this branch (/prerelease) and run source-bing-ads against it end to end — the manifest from airbytehq/airbyte#88968 on the prerelease SDM image, with a real refresh failure if you can reproduce one, otherwise at least check/read on a working connection to confirm the field is accepted and nothing else in the auth path regressed. The unit tests cover the lookup, but the connector is the first consumer and the field only has value once the two sides agree.

This branch has not been deployed

No deployments
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.

2 participants