Refactor: extract shared PolicyTokenAcquirer + ChangeSafetyParameters (Change Safety – Stage 0)#454
Open
YangAn-microsoft wants to merge 4 commits into
Open
Conversation
… (Change Safety Stage 0)
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Author
|
/azp run |
Supported commands
See additional documentation. |
|
Commenter does not have sufficient privileges for PR 454 in repo Azure/azure-powershell-common |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request performs a behavior-preserving refactor of the Change Safety “AcquirePolicyToken” implementation by extracting the wire-contract/token-stamping logic and the dynamic parameter definitions into reusable, pipeline-agnostic types. This supports upcoming AutoRest-based cmdlets and cross-assembly reuse (e.g., in Az.Accounts) without duplicating the compliance-sensitive acquirePolicyToken contract.
Changes:
- Introduces
PolicyTokenAcquirerto centralize acquiring/stamping thex-ms-policy-external-evaluationsheader in a pipeline-agnostic way. - Introduces
ChangeSafetyParametersto centralize dynamic parameter names and construction for-AcquirePolicyTokenand-ChangeReference. - Refactors
AcquirePolicyTokenHandlerandAzurePSCmdlet.GetDynamicParameters()to delegate to the new shared types.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Common/PolicyTokenAcquirer.cs | New shared implementation for token acquisition + request header stamping. |
| src/Common/ChangeSafetyParameters.cs | New shared helper for change safety dynamic parameters and name constants. |
| src/Common/AzurePSCmdlet.cs | Switches cmdlet dynamic parameter plumbing to use ChangeSafetyParameters. |
| src/Common/AcquirePolicyTokenHandler.cs | Delegates handler behavior to PolicyTokenAcquirer to preserve the same wire logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…efix, throw on null dict
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
The acquirer now disposes the token HttpRequestMessage (and its Content) after sending, so the E2E tests could no longer read capturedTokenRequest.Content afterward (ObjectDisposedException). Capture the request body string inside the mock server callback instead.
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
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.
Summary
Behavior-preserving refactor that extracts the Change Safety policy-token logic (shipped for SDK cmdlets) into two reusable, pipeline-agnostic types so it can be shared by the upcoming AutoRest Change Safety work — without duplicating a compliance-sensitive wire contract.
src/Common/PolicyTokenAcquirer.cs— the pipeline-agnostic wire core (write-verb gate, subscription-id extraction,acquirePolicyTokenPOST, andx-ms-policy-external-evaluationsheader stamping) exposed viaStampPolicyTokenAsync(...). Sends via its ownHttpClient, so it has no dependency onbase.SendAsyncor any pipeline.publicbecause it will be consumed cross-assembly byContextAdapterin Az.Accounts (see "Why").src/Common/ChangeSafetyParameters.cs— factory + name constants for the-AcquirePolicyToken/-ChangeReferencedynamic parameters.AcquirePolicyTokenHandlernow delegates itsSendAsynctoPolicyTokenAcquirerthen callsbase.SendAsync(public ctor / fields /Clone()/ICloneableretained).AzurePSCmdlet.GetDynamicParameters()keeps the read-verb gate and now usesChangeSafetyParameters.Why (this is Stage 0 of a multi-repo feature)
AutoRest-generated cmdlets don't inherit
AzurePSCmdlet, so they can't reuse the SDK Change Safety handler by inheritance. The upcoming AutoRest work adds anAcquirePolicyTokenstep inContextAdapter.OnNewRequest(Az.Accounts, inazure-powershell) that must run the identical wire logic. Extracting it here gives both cmdlet families a single source of truth for theacquirePolicyTokencontract instead of a duplicated copy. This PR is the prerequisite for that Az.Accounts change and is intentionally a small, standalone, behavior-preserving refactor.Behavior change
None. Parameter names, help text, gating, payload, headers, and error mapping are unchanged. Existing tests pass unmodified:
AcquirePolicyTokenHandlerTests(Common.Test) andAzureRMCmdletUnitTests(ResourceManager.Test).