[Key Vault] Remove stale no-op insecure_domain_change pop for parity with azure-core #45518#47320
Open
RadikHoroshev wants to merge 1 commit into
Open
Conversation
…with azure-core Azure#45518 PR Azure#45518 fixed the cross-domain redirect Authorization-header strip in azure-core by persisting the insecure_domain_change flag on request.context instead of the ephemeral request.context.options kwargs dict, and removed the now-obsolete request.context.options.pop("insecure_domain_change", False) line from azure-core's BearerTokenCredentialPolicy. The Key Vault packages carry independent forked copies of the challenge authentication policy that still contained that exact line. Since the flag now lives on request.context and .options is the stale kwargs dict, the pop is a no-op. This removes it (sync + async) across azure-keyvault-keys, -secrets, -certificates, -administration and -securitydomain for parity with the azure-core fix. No functional change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Community Contribution
Contributor
|
Thank you for your contribution @RadikHoroshev! We will review the pull request and get back to you soon. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes a previously redundant context-option mutation from Key Vault challenge authentication policies to match an azure-core change, and records the cleanup in select package changelogs.
Changes:
- Removed
request.context.options.pop("insecure_domain_change", False)from sync/async challenge auth policies across multiple Key Vault packages. - Added changelog notes in securitydomain, keys, certificates, and administration packages referencing
azure-corePR #45518.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/keyvault/azure-keyvault-securitydomain/azure/keyvault/securitydomain/_internal/challenge_auth_policy.py | Removes option-pop line in sync challenge flow |
| sdk/keyvault/azure-keyvault-securitydomain/azure/keyvault/securitydomain/_internal/async_challenge_auth_policy.py | Removes option-pop line in async challenge flow |
| sdk/keyvault/azure-keyvault-securitydomain/CHANGELOG.md | Documents the internal cleanup / parity note |
| sdk/keyvault/azure-keyvault-secrets/azure/keyvault/secrets/_shared/challenge_auth_policy.py | Removes option-pop line in sync challenge flow |
| sdk/keyvault/azure-keyvault-secrets/azure/keyvault/secrets/_shared/async_challenge_auth_policy.py | Removes option-pop line in async challenge flow |
| sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_shared/challenge_auth_policy.py | Removes option-pop line in sync challenge flow |
| sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_shared/async_challenge_auth_policy.py | Removes option-pop line in async challenge flow |
| sdk/keyvault/azure-keyvault-keys/CHANGELOG.md | Documents the internal cleanup / parity note |
| sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_shared/challenge_auth_policy.py | Removes option-pop line in sync challenge flow |
| sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_shared/async_challenge_auth_policy.py | Removes option-pop line in async challenge flow |
| sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md | Documents the internal cleanup / parity note |
| sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_internal/challenge_auth_policy.py | Removes option-pop line in sync challenge flow |
| sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_internal/async_challenge_auth_policy.py | Removes option-pop line in async challenge flow |
| sdk/keyvault/azure-keyvault-administration/CHANGELOG.md | Documents the internal cleanup / parity note |
|
|
||
| ### Other Changes | ||
|
|
||
| - Removed a stale, no-op line from the internal challenge authentication policy for parity with the `azure-core` fix in [#45518](https://github.com/Azure/azure-sdk-for-python/pull/45518). This is an internal cleanup with no functional impact. |
Comment on lines
148
to
153
|
|
||
| request_authorized = self.on_challenge(request, response) | ||
| if request_authorized: | ||
| # if we receive a challenge response, we retrieve a new token | ||
| # which matches the new target. In this case, we don't want to remove | ||
| # token from the request so clear the 'insecure_domain_change' tag | ||
| request.context.options.pop("insecure_domain_change", False) | ||
| try: | ||
| response = self.next.send(request) | ||
| except Exception: # pylint:disable=broad-except |
Author
|
@microsoft-github-policy-service agree |
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.
Description
#45518 fixed the cross-domain redirect
Authorization-header strip inazure-coreby persisting theinsecure_domain_changeflag onrequest.context(aDictsubclass) instead of the ephemeralrequest.context.optionskwargs dict. As part of that fix it removed the now-obsolete line fromazure-core'sBearerTokenCredentialPolicy:The Key Vault packages each carry an independent forked copy of the challenge-authentication policy (
challenge_auth_policy.py+ its async variant under_shared/or_internal/) that still contained that exact line. Because the live flag now lives onrequest.contextand.optionsis the stale kwargs dict, thepopis a no-op. This PR removes it (sync + async) for parity with theazure-corefix.This mirrors
azure-coreexactly: the surrounding block now goes straight fromif request_authorized:to thetry:that re-sends the request, identical to_authentication.pyafter #45518.No functional change — the removed line had no effect once the flag moved off
request.context.options.Files (5 packages, sync + async = 10)
azure-keyvault-keys—_shared/challenge_auth_policy.py,_shared/async_challenge_auth_policy.pyazure-keyvault-secrets— same pair under_shared/azure-keyvault-certificates— same pair under_shared/azure-keyvault-administration— same pair under_internal/azure-keyvault-securitydomain— same pair under_internal/CHANGELOG
Added an Other Changes note to the four packages with an open
(Unreleased)section (administration, certificates, keys, securitydomain).azure-keyvault-secretscurrently has no open unreleased version (top entry is the released4.11.0), so no changelog entry was added there to avoid an unwarranted version bump for an internal no-op — happy to add one if maintainers prefer.All SDK Contribution checklist:
secrets).General Guidelines and Best Practices
Testing Guidelines
🤖 Generated with Claude Code