Repository navigation
feat(oauth): add refresh_token_error_messages to OAuthAuthenticator - #1214
ZaneHyattAB wants to merge 5 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou 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-guidancePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)5 783 tests +29 5 752 ✅ +29 11m 56s ⏱️ +17s 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.♻️ This comment has been updated with latest results. |
PyTest Results (Full)5 786 tests +29 5 755 ✅ +29 14m 20s ⏱️ + 4m 12s 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.♻️ This comment has been updated with latest results. |
|
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.
The same outcome is reachable with a generic, connector-owned setting. The refresh call runs inside the authenticator before any authenticator:
type: OAuthAuthenticator
...
refresh_token_error_messages:
AADSTS50078: "Microsoft Entra refused the refresh because the tenant's MFA policy ..."
AADSTS70043: "..."
Why three key kinds rather than just the code: I checked recent prod failures that carry the Concretely, I'd suggest: keep |
Anatolii Yatsuk (tolik0)
left a comment
There was a problem hiding this comment.
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:
_get_refresh_token_error_messagereads onlyerror_descriptionanderror, while the gate honours a customrefresh_token_error_key. A provider that reports under e.g.errorCodecan be gated but never mapped. Addingself._refresh_token_error_keyas a last candidate closes that gap.- The CodeQL hit on
super(Oauth2Authenticator, self).__init__(...)intest_abstract_authenticator_positional_init_still_supportedis a false positive — the call is intentional — but its suggested fix would change the test's meaning. CallingAbstractOauth2Authenticator.__init__(self, ...)directly, or a one-line comment, keeps the next reader from "fixing" it. - The generated-model hunk is hand-committed and nothing in CI regenerates the model, so a follow-up that re-syncs
declarative_component_schema.pyin one commit would make the nextpoe assemblea 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.
Summary
Adds a vendor-neutral
refresh_token_error_messagesfield toOAuthAuthenticator. 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).
The change sits at the existing message site in
AbstractOauth2Authenticator._make_handled_request, inside the existing_wrap_refresh_token_exceptiongate:Semantics
refresh_token_error_status_codes/_key/_values(or their defaults) already flag. They don't flag new responses and don't changefailure_type. The schema field description says so.OAuthAuthenticatoronly, not the deprecatedRefreshTokenUpdatercopies.invalid_grantcovers everyinvalid_grantthat has no more specific hit.Factory-defaults trap and its guard
_get_refresh_token_error_informationreturns 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_exceptionwould always return False.invalid_grantconfig error would become a rawHTTPError.The new
_get_refresh_token_error_messages(model)resolves messages separately, and the 3-tuple helper is untouched. The regression guards aretest_oauth_refresh_token_error_messages_only_keeps_default_gateandtest_single_use_oauth_with_refresh_token_error_messages_keeps_default_gate.Plumbing
Schema → generated model → factory (both the
DeclarativeOauth2AuthenticatorandDeclarativeSingleUseRefreshTokenOauth2Authenticatorconstructions) →DeclarativeOauth2Authenticator→Oauth2Authenticator/SingleUseRefreshTokenOauth2Authenticator→AbstractOauth2Authenticator.__init__. In the native classes the new parameter is the last one, defaults toNone, and is passed tosuper()by keyword, so existing positional callers keep working (test_abstract_authenticator_positional_init_still_supported).The model was generated with
poe assemble. On currentmainthe generator also rewrites about 430 unrelated lines, so only the generated hunk for the new field is committed.Kept / reverted from earlier revisions
_extract_provider_error_codeand the determinism test (now driven by configured messages)._ENTRA_REFRESH_ERROR_GUIDANCEand all Entra wording from the CDK.unit_tests/sources/declarative/auth/test_oauth.py.Tests
test_requests_native_auth.py:None/{}give a byte-identical generic messagetest_oauth.py: a declarative authenticator with configured messages.test_model_to_component_factory.py:{}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 --checkandmypypass. 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