fix: surface the token-exchange error instead of KeyError('access_token') - #928
Open
TangoEnSkai wants to merge 1 commit into
Open
fix: surface the token-exchange error instead of KeyError('access_token')#928TangoEnSkai wants to merge 1 commit into
TangoEnSkai wants to merge 1 commit into
Conversation
…en')
When token federation exchange fails, `_exchange_token` read
`token_response["access_token"]` unconditionally. An OAuth error body
(`{"error": ..., "error_description": ...}`) therefore raised
`KeyError('access_token')`, and the handler in `_get_token` logged the
name of the missing key:
Token exchange failed, using external token: 'access_token'
The endpoint's own reason was never read. Since `TokenFederationProvider`
wraps every provider and `_should_exchange_token` returns True whenever
the issuer host differs from the workspace host, this warning fires on
every connection for cross-issuer tokens with no way to tell whether the
exchange was misconfigured, unauthorized, or unsupported.
Check for `access_token` before reading it and raise a ValueError that
names the endpoint, the HTTP status, and the returned `error` /
`error_description`. A non-JSON body now reports the endpoint and status
rather than surfacing a JSONDecodeError. The response carries no token in
either case, so nothing sensitive is exposed.
The fallback to the external token is unchanged: `_get_token` still
catches the exception and connections keep working.
Resolves databricks#904
Signed-off-by: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
TokenFederationProviderwraps every credentials provider unconditionally(
auth.py:68, "Always wrap with token federation"), and_should_exchange_tokenreturns
Truewhenever the token's issuer host differs from the workspace host —for example a Microsoft Entra ID token against an Azure Databricks workspace. An
exchange is therefore attempted on every such connection.
When the exchange endpoint refuses, it replies with an OAuth error body
(
{"error": ..., "error_description": ...}) rather than a token._exchange_tokenreadtoken_response["access_token"]unconditionally, so thatreply raised
KeyError('access_token'). The handler in_get_tokenlogs theexception, and
str(KeyError("access_token"))renders as the key name:The endpoint's own
erroranderror_descriptionwere never read, so there wasno way to tell whether the exchange was misconfigured, unauthorized, or simply
unsupported for that workspace. Note
token_typeon the very next line isalready read defensively with
.get().What
_exchange_tokenchecks foraccess_tokenbefore reading it. If it ismissing, it raises a
ValueErrornaming the endpoint, the HTTP status, and thereturned
error/error_description.ValueErrornaming the endpoint andstatus instead of surfacing a bare
JSONDecodeError.tests/unit/test_token_federation.py:test_exchange_token_failurepreviouslyasserted
pytest.raises(KeyError)— it pinned the behaviour this issue isabout — and is updated to assert the endpoint's reason is surfaced. Three tests
are added for the no-error-fields body, the non-JSON body, and the unchanged
fallback to the external token.
The response carries no token on the failure paths, so nothing sensitive is
placed in the message.
Why
This is diagnostic only — the fallback to the external token works and queries
succeed, which is exactly why it is worth fixing: the warning fires on every
connection and tells the operator nothing actionable. The fix is contained to the
one function and preserves the graceful degradation, since
_get_tokenstillcatches the exception and falls through to the external token.
Completion Criteria
_exchange_tokensurfaces the endpoint'serror/error_descriptionJSONDecodeErrorpytest tests/unit— 531 passed, 217 skipped, 185 subtests passednothing else
black --check srcclean# Unreleasedclose #904