[DO NOT MERGE]Add support for delegation sas #28951
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
EmmaZhu
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for user delegation SAS tokens in Azure Storage cmdlets by updating to newer Azure Storage SDK beta versions (12.27.0-beta.1 and 12.25.0-beta.1) and Azure.Core (1.50.0). The changes enable generation of user delegation SAS tokens with OAuth authentication, which provides enhanced security by using Azure AD credentials instead of storage account keys.
Key Changes:
- Added
DelegationObjectIDparameter to three SAS token generation cmdlets for specifying the object ID when creating user delegation SAS tokens - Added
SASTokenWithConnectedAccountswitch parameter toNew-AzureStorageContextto enable OAuth with SAS tokens - Updated client initialization logic throughout the codebase to support OAuth tokens when using SAS credentials
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Common.Netcore.Dependencies.targets | Updates Azure.Core package version to 1.50.0 |
| src/lib/manifest.json | Updates package manifests for Azure.Core and System.ClientModel |
| src/lib/cgmanifest.json | Updates component governance manifest with new package versions |
| src/Storage/Storage/Storage.csproj | Updates Azure Storage SDK packages to beta versions (12.27.0-beta.1) |
| src/Storage/Storage/Queue/Cmdlet/NewAzureStorageQueueSasToken.cs | Adds DelegationObjectID parameter and user delegation SAS generation logic for queues |
| src/Storage/Storage/Model/Contract/StorageBlobManagement.cs | Adds helper method to detect SAS with OAuth credential |
| src/Storage/Storage/Model/Contract/IStorageBlobManagement.cs | Adds interface method for detecting SAS with OAuth credential |
| src/Storage/Storage/DatalakeGen2/Cmdlet/GetAzDataLakeGen2ChildItem.cs | Updates GetPaths API call to use DataLakeGetPathsOptions object |
| src/Storage/Storage/Common/Util.cs | Updates blob and queue client initialization to support OAuth tokens with SAS |
| src/Storage/Storage/Common/SasTokenHelper.cs | Adds user delegation SAS support for queues and DelegationObjectID parameter handling |
| src/Storage/Storage/Common/Cmdlet/NewAzureStorageContext.cs | Adds SASTokenWithConnectedAccount parameter for OAuth with SAS tokens |
| src/Storage/Storage/Common/AzureStorageContainer.cs | Updates container client initialization to support OAuth tokens |
| src/Storage/Storage/Common/AzureStorageBlob.cs | Updates blob client initialization to support OAuth tokens |
| src/Storage/Storage/Blob/StorageCloudBlobCmdletBase.cs | Updates blob client initialization to support OAuth tokens |
| src/Storage/Storage/Blob/Cmdlet/SetAzureStorageBlobContent.cs | Updates blob upload logic to use OAuth-aware client initialization |
| src/Storage/Storage/Blob/Cmdlet/NewAzureStorageContainerSasToken.cs | Adds DelegationObjectID parameter for container SAS token generation |
| src/Storage/Storage/Blob/Cmdlet/NewAzureStorageBlobSasToken.cs | Adds DelegationObjectID parameter and enhanced OAuth detection for blob SAS tokens |
| src/Storage/Storage/Blob/Cmdlet/GetAzureStorageBlobContent.cs | Adds helper method to detect SAS with OAuth for blob downloads |
| src/Storage/Storage/Blob/Cmdlet/CopyAzureStorageBlob.cs | Updates blob copy operations to pass OAuth token for source authentication |
| src/Storage/Storage.common/Storage.common.csproj | Updates Azure.Storage.Files.Shares to beta version |
| src/Storage/Storage.common/Common/AzureStorageContext.cs | Adds constructor overload to support OAuth with SAS tokens |
| src/Storage/Storage.common/Common/AzureContextAdapterExtensions.cs | Updates context creation call to match new constructor signature |
| src/Storage/Storage.Test/Service/MockStorageBlobManagement.cs | Implements IsSasWithOAuthCredential method in mock object |
| src/Storage/Storage.Test/Common/StorageCloudCmdletBaseTest.cs | Updates test calls to match new AzureStorageContext constructor signature |
| src/Storage/Storage.Test/Common/Cmdlet/NewAzureStorageContextTest.cs | Updates test calls to match new constructor signature |
| src/Storage/Storage.Test/Blob/StorageCloudBlobCmdletBaseTest.cs | Updates test call to match new constructor signature |
| src/Accounts/AssemblyLoading/ConditionalAssemblyProvider.cs | Updates assembly version references for Azure.Core and System.ClientModel |
| get { return sasTokenWithConnectedAcount; } | ||
| set { sasTokenWithConnectedAcount = value; } | ||
| } | ||
|
|
||
| private bool sasTokenWithConnectedAcount = false; |
There was a problem hiding this comment.
Typo in variable name: sasTokenWithConnectedAcount should be sasTokenWithConnectedAccount (missing 'c' in 'Account').
| get { return sasTokenWithConnectedAcount; } | |
| set { sasTokenWithConnectedAcount = value; } | |
| } | |
| private bool sasTokenWithConnectedAcount = false; | |
| get { return sasTokenWithConnectedAccount; } | |
| set { sasTokenWithConnectedAccount = value; } | |
| } | |
| private bool sasTokenWithConnectedAccount = false; |
|
|
||
| if (sourceChannel.StorageContext != null && sourceChannel.StorageContext.Track2OauthToken != null) | ||
| { | ||
| string oauthToken = sourceChannel.StorageContext.Track2OauthToken.GetToken(new TokenRequestContext(), this.CmdletCancellationToken).Token; |
There was a problem hiding this comment.
Inconsistency in GetToken usage: Line 539 uses GetToken(new TokenRequestContext(), ...).Token while lines 467, 508, and 581 use GetToken(null, ...).TokenValue. For consistency, this should likely use the same pattern as the other calls: GetToken(null, this.CmdletCancellationToken).TokenValue.
| string oauthToken = sourceChannel.StorageContext.Track2OauthToken.GetToken(new TokenRequestContext(), this.CmdletCancellationToken).Token; | |
| string oauthToken = sourceChannel.StorageContext.Track2OauthToken.GetToken(null, this.CmdletCancellationToken).TokenValue; |
| { | ||
| sasBuilder.EncryptionScope = EncryptionScope; | ||
| } | ||
| if(DelegatedUserObjectId != null) |
There was a problem hiding this comment.
[nitpick] Missing space after if keyword. Should be if (DelegatedUserObjectId != null) for consistency with the rest of the codebase.
| if(DelegatedUserObjectId != null) | |
| if (DelegatedUserObjectId != null) |
| { | ||
| if (String.IsNullOrEmpty(Name)) return; | ||
|
|
||
| // When the input context is Oauth bases, can't generate normal SAS, but UserDelegationSas |
There was a problem hiding this comment.
Typo in comment: Oauth bases should be OAuth based.
| // When the input context is Oauth bases, can't generate normal SAS, but UserDelegationSas | |
| // When the input context is OAuth based, can't generate normal SAS, but UserDelegationSas |
| [Parameter( | ||
| Mandatory = false, | ||
| HelpMessage = "Delegation object id")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string DelegationObjectID { get; set; } |
There was a problem hiding this comment.
Missing test coverage for the new DelegationObjectID parameter. The Storage.Test project contains existing SAS token tests (e.g., SasTokenHelperTest.cs), but there are no tests for this new parameter or the user delegation SAS functionality it enables. Consider adding tests to verify the parameter is correctly passed through and used in SAS token generation.
| /// <param name="logWriter"></param> | ||
| public AzureStorageContext(CloudStorageAccount account, string accountName = null, IAzureContext DefaultContext = null, DebugLogWriter logWriter = null) | ||
| public AzureStorageContext(CloudStorageAccount account, string accountName = null, IAzureContext DefaultContext = null, DebugLogWriter logWriter = null) : | ||
| this(account, accountName, false, null, null) |
There was a problem hiding this comment.
Bug: The constructor chaining is incorrect. The original constructor accepts DefaultContext and logWriter parameters but passes null, null instead of DefaultContext, logWriter to the new constructor. This should be: this(account, accountName, false, DefaultContext, logWriter) to properly pass through these parameters.
| this(account, accountName, false, null, null) | |
| this(account, accountName, false, DefaultContext, logWriter) |
| if (context.Track2OauthToken != null) | ||
| { | ||
| blobClient = new BlobBaseClient(blobUri, context.Track2OauthToken, options); | ||
| } | ||
| else | ||
| { | ||
| blobClient = new BlobBaseClient(blobUri, options); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| switch (blobType.Value) | ||
| { | ||
| case global::Azure.Storage.Blobs.Models.BlobType.Page: | ||
| blobClient = new PageBlobClient(blobUri, options); | ||
| if (context.Track2OauthToken != null) | ||
| { | ||
| blobClient = new PageBlobClient(blobUri, context.Track2OauthToken, options); | ||
| } | ||
| else | ||
| { | ||
| blobClient = new PageBlobClient(blobUri, options); | ||
| } | ||
| break; | ||
| case global::Azure.Storage.Blobs.Models.BlobType.Append: | ||
| blobClient = new AppendBlobClient(blobUri, options); | ||
| if (context.Track2OauthToken != null) | ||
| { | ||
| blobClient = new AppendBlobClient(blobUri, context.Track2OauthToken, options); | ||
| } | ||
| else | ||
| { | ||
| blobClient = new AppendBlobClient(blobUri, options); | ||
| } | ||
| break; | ||
| default: //Block | ||
| blobClient = new BlockBlobClient(blobUri, options); | ||
| if (context.Track2OauthToken != null) | ||
| { | ||
| blobClient = new BlockBlobClient(blobUri, context.Track2OauthToken, options); | ||
| } | ||
| else | ||
| { | ||
| blobClient = new BlockBlobClient(blobUri, options); | ||
| } |
There was a problem hiding this comment.
Code duplication: The pattern if (context.Track2OauthToken != null) { new Client(uri, context.Track2OauthToken, options) } else { new Client(uri, options) } is repeated multiple times (lines 490-497, 504-511, 514-521, 524-531). Consider extracting this into a helper method to reduce duplication and improve maintainability.
| [Parameter( | ||
| Mandatory = false, | ||
| HelpMessage = "Delegation object id")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string DelegationObjectID { get; set; } |
There was a problem hiding this comment.
Missing test coverage for the new DelegationObjectID parameter. The Storage.Test project contains existing SAS token tests, but there are no tests for this new parameter. Consider adding tests to verify the parameter is correctly passed through and used in container SAS token generation.
| [Parameter( | ||
| Mandatory = false, | ||
| HelpMessage = "Delegation object id")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string DelegationObjectID { get; set; } |
There was a problem hiding this comment.
Missing test coverage for the new DelegationObjectID parameter. The Storage.Test project contains existing SAS token tests, but there are no tests for this new parameter. Consider adding tests to verify the parameter is correctly passed through and used in blob SAS token generation.
|
|
||
| [Parameter( | ||
| Mandatory = false, | ||
| HelpMessage = "Delegation object id")] |
There was a problem hiding this comment.
The help message "Delegation object id" is too brief and doesn't explain what this parameter is used for or when it should be provided. Consider a more descriptive help message like: "The object ID of the user or service principal for which the user delegation SAS token is being created. Used when generating user delegation SAS tokens with OAuth authentication."
| HelpMessage = "Delegation object id")] | |
| HelpMessage = "The object ID of the user or service principal for which the user delegation SAS token is being created. Used when generating user delegation SAS tokens with OAuth authentication.")] |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
2 participants
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.