Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Storage] Fix for bug on Rename where a SAS appended to the sourceUri, does not get passed to a Destination that has no credentials. #42004

Merged
merged 1 commit into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/storage/Azure.Storage.Files.DataLake/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Breaking Changes

### Bugs Fixed
- Fixed bug where on `DataLakeDirectoryClient`, `DataLakeFileClient` and `DataLakePathClient`, calling `.Rename(..)` will throw a 403 Authentication Error, if the source storage client was instantiated with a SAS on the `Uri`, will not pass the SAS to the destination- Fixed bug where on `ShareDirectoryClient`, `ShareFileClient` and `DataLakePathClient`, calling `.Rename(..)` will throw a 403 Authentication Error, if the source storage client was instantiated with a SAS on the `Uri`, will not pass the SAS to the destination client, when the destination does not have any credentials.

### Other Changes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Globalization;
using System.Linq;
using System.Text;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Azure.Core;
Expand Down Expand Up @@ -2251,7 +2250,7 @@ private async Task<Response<DataLakePathClient>> RenameInternal(
};
destUriBuilder.FileSystemName = destinationFileSystem ?? destUriBuilder.FileSystemName;

// DataLakeUriBuider will encode the DirectoryOrFilePath.
// DataLakeUriBuilder will encode the DirectoryOrFilePath.
// We don't want the query parameters, especially SAS, to be encoded.
// We also have to build the destination client depending on if a SAS was passed with the destination.
DataLakePathClient destPathClient;
Expand Down Expand Up @@ -2282,6 +2281,7 @@ private async Task<Response<DataLakePathClient>> RenameInternal(
{
// No SAS in the destination, use the source credentials as a default
destUriBuilder.DirectoryOrFilePath = destinationPath;
destUriBuilder.Sas = sourceUriBuilder.Sas;
destPathClient = new DataLakePathClient(
destUriBuilder.ToUri(),
ClientConfiguration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,40 @@ public async Task RenameAsync_DifferentSasUri()
Response<PathProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
amnguye marked this conversation as resolved.
Show resolved Hide resolved
[Test]
public async Task RenameAsync_SourceSasUri()
{
// Arrange
string fileSystemName = GetNewFileSystemName();
await using DisposingFileSystem test = await GetNewFileSystem(fileSystemName: fileSystemName);
string sourceDirectoryName = GetNewDirectoryName();
DataLakeDirectoryClient directoryClient = await test.FileSystem.CreateDirectoryAsync(GetNewDirectoryName());
DataLakeDirectoryClient sourceDirectoryClient = await directoryClient.CreateSubDirectoryAsync(sourceDirectoryName);
DataLakeFileClient sourceFile = await sourceDirectoryClient.CreateFileAsync(GetNewFileName());

// Make unique source sas
DataLakeSasQueryParameters sourceSas = GetNewDataLakeServiceSasCredentialsFileSystem(fileSystemName);
DataLakeUriBuilder sourceUriBuilder = new DataLakeUriBuilder(sourceDirectoryClient.Uri)
{
Sas = sourceSas
};

string destFileName = GetNewFileName();

DataLakeDirectoryClient sasDirectoryClient = InstrumentClient(new DataLakeDirectoryClient(sourceUriBuilder.ToUri(), GetOptions()));
DataLakeFileClient sasFileClient = InstrumentClient(sasDirectoryClient.GetFileClient(sourceFile.Name));

// Make unique destination sas
string newPath = directoryClient.Path + "/" + destFileName;

// Act
DataLakeFileClient destFile = await sasFileClient.RenameAsync(destinationPath: newPath);

// Assert
Response<PathProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasCredentialDestSasUri()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,40 @@ public async Task RenameAsync_SasCredentialFromDirectory()
Response<PathProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasUri()
{
// Arrange
string fileSystemName = GetNewFileSystemName();
await using DisposingFileSystem test = await GetNewFileSystem(fileSystemName: fileSystemName);
string sourceDirectoryName = GetNewDirectoryName();
DataLakeDirectoryClient directoryClient = await test.FileSystem.CreateDirectoryAsync(GetNewDirectoryName());
DataLakeDirectoryClient sourceDirectoryClient = await directoryClient.CreateSubDirectoryAsync(sourceDirectoryName);
DataLakeFileClient sourceFile = await sourceDirectoryClient.CreateFileAsync(GetNewFileName());

// Make unique source sas
DataLakeSasQueryParameters sourceSas = GetNewDataLakeServiceSasCredentialsFileSystem(fileSystemName);
DataLakeUriBuilder sourceUriBuilder = new DataLakeUriBuilder(sourceDirectoryClient.Uri)
{
Sas = sourceSas
};

string destFileName = GetNewFileName();

DataLakeDirectoryClient sasDirectoryClient = InstrumentClient(new DataLakeDirectoryClient(sourceUriBuilder.ToUri(), GetOptions()));
DataLakeFileClient sasFileClient = InstrumentClient(sasDirectoryClient.GetFileClient(sourceFile.Name));

// Make unique destination sas
string newPath = directoryClient.Path + "/" + destFileName;

// Act
DataLakeFileClient destFile = await sasFileClient.RenameAsync(destinationPath: newPath);

// Assert
Response<PathProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_DifferentSasUri()
Expand Down
1 change: 1 addition & 0 deletions sdk/storage/Azure.Storage.Files.Shares/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Breaking Changes

### Bugs Fixed
- Fixed bug where on `ShareDirectoryClient` and `ShareFileClient`, calling `.Rename(..)` will throw a 403 Authentication Error, if the source storage client was instantiated with a SAS on the `Uri`, will not pass the SAS to the destination client, when the destination does not have any credentials.

### Other Changes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using Azure.Core.Pipeline;
using Azure.Storage.Files.Shares.Models;
using Azure.Storage.Sas;
using Azure.Storage.Shared;
using Metadata = System.Collections.Generic.IDictionary<string, string>;

#pragma warning disable SA1402 // File may only contain a single type
Expand Down Expand Up @@ -2560,7 +2559,7 @@ private async Task<Response<ShareDirectoryClient>> RenameInternal(
// TODO: Change this so that the ShareUriBuilder can accept a string for the SAS
// Or have an extension UriBuilder which can parse out the SAS.
ShareUriBuilder shareUriBuilder = new ShareUriBuilder(Uri);
UriBuilder sourceUriBuilder = new UriBuilder(Uri);
ShareUriBuilder sourceUriBuilder = new ShareUriBuilder(Uri);
// There's already a check in at the client constructor to prevent both SAS in Uri and AzureSasCredential
if (shareUriBuilder.Sas == null && ClientConfiguration.SasCredential != null)
{
Expand Down Expand Up @@ -2605,6 +2604,7 @@ private async Task<Response<ShareDirectoryClient>> RenameInternal(
{
// No SAS in the destination, use the source credentials to build the destination path
destUriBuilder.DirectoryOrFilePath = destinationPath;
destUriBuilder.Sas = sourceUriBuilder.Sas;
destDirectoryClient = new ShareDirectoryClient(
destUriBuilder.ToUri(),
ClientConfiguration);
Expand All @@ -2624,7 +2624,7 @@ private async Task<Response<ShareDirectoryClient>> RenameInternal(
if (async)
{
response = await destDirectoryClient.DirectoryRestClient.RenameAsync(
renameSource: sourceUriBuilder.Uri.AbsoluteUri,
renameSource: sourceUriBuilder.ToUri().AbsoluteUri,
replaceIfExists: options?.ReplaceIfExists,
ignoreReadOnly: options?.IgnoreReadOnly,
sourceLeaseId: options?.SourceConditions?.LeaseId,
Expand All @@ -2639,7 +2639,7 @@ private async Task<Response<ShareDirectoryClient>> RenameInternal(
else
{
response = destDirectoryClient.DirectoryRestClient.Rename(
renameSource: sourceUriBuilder.Uri.AbsoluteUri,
renameSource: sourceUriBuilder.ToUri().AbsoluteUri,
replaceIfExists: options?.ReplaceIfExists,
ignoreReadOnly: options?.IgnoreReadOnly,
sourceLeaseId: options?.SourceConditions?.LeaseId,
Expand Down
11 changes: 4 additions & 7 deletions sdk/storage/Azure.Storage.Files.Shares/src/ShareFileClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@
using System.Buffers;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using Azure.Core;
Expand All @@ -16,8 +14,6 @@
using Azure.Storage.Shared;
using Azure.Storage.Sas;
using Metadata = System.Collections.Generic.IDictionary<string, string>;
using System.Net.Http.Headers;
using System.Linq;

#pragma warning disable SA1402 // File may only contain a single type

Expand Down Expand Up @@ -6290,7 +6286,7 @@ private async Task<Response<ShareFileClient>> RenameInternal(
// TODO: this seems weird to do, probably should build a way to append to the query.. without altering it somehow
// in the case that the customer wants the query to be in a certain order
ShareUriBuilder shareUriBuilder = new ShareUriBuilder(Uri);
UriBuilder sourceUriBuilder = new UriBuilder(Uri);
ShareUriBuilder sourceUriBuilder = new ShareUriBuilder(Uri);
// There's already a check in at the client constructor to prevent both SAS in Uri and AzureSasCredential
if (shareUriBuilder.Sas == null && ClientConfiguration.SasCredential != null)
{
Expand Down Expand Up @@ -6335,6 +6331,7 @@ private async Task<Response<ShareFileClient>> RenameInternal(
{
// No SAS in the destination, use the source credentials to build the destination path
destUriBuilder.DirectoryOrFilePath = destinationPath;
destUriBuilder.Sas = sourceUriBuilder.Sas;
destFileClient = new ShareFileClient(
destUriBuilder.ToUri(),
ClientConfiguration);
Expand All @@ -6359,7 +6356,7 @@ private async Task<Response<ShareFileClient>> RenameInternal(
if (async)
{
response = await destFileClient.FileRestClient.RenameAsync(
renameSource: sourceUriBuilder.Uri.AbsoluteUri,
renameSource: sourceUriBuilder.ToUri().AbsoluteUri,
replaceIfExists: options?.ReplaceIfExists,
ignoreReadOnly: options?.IgnoreReadOnly,
sourceLeaseId: options?.SourceConditions?.LeaseId,
Expand All @@ -6375,7 +6372,7 @@ private async Task<Response<ShareFileClient>> RenameInternal(
else
{
response = destFileClient.FileRestClient.Rename(
renameSource: sourceUriBuilder.Uri.AbsoluteUri,
renameSource: sourceUriBuilder.ToUri().AbsoluteUri,
replaceIfExists: options?.ReplaceIfExists,
ignoreReadOnly: options?.IgnoreReadOnly,
sourceLeaseId: options?.SourceConditions?.LeaseId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using Azure.Core.TestFramework;
using Azure.Identity;
using Azure.Storage.Files.Shares.Models;
using Azure.Storage.Files.Shares.Specialized;
using Azure.Storage.Sas;
using Azure.Storage.Test;
using Azure.Storage.Tests.Shared;
using Microsoft.Diagnostics.Symbols;
using Microsoft.Extensions.Options;
using Moq;
using NUnit.Framework;

Expand Down Expand Up @@ -2320,6 +2316,39 @@ public async Task RenameAsync_DifferentSasUri()
Response<ShareDirectoryProperties> response = await destDirectory.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasUri()
{
// Arrange
string shareName = GetNewShareName();
await using DisposingShare test = await GetTestShareAsync(shareName: shareName);
string sourceDirectoryName = GetNewDirectoryName();
ShareDirectoryClient directoryClient = await test.Share.CreateDirectoryAsync(GetNewDirectoryName());
ShareDirectoryClient sourceDirectoryClient = await directoryClient.CreateSubdirectoryAsync(sourceDirectoryName);

// Make unique source sas
SasQueryParameters sourceSas = GetNewFileServiceSasCredentialsShare(shareName);
ShareUriBuilder sourceUriBuilder = new ShareUriBuilder(sourceDirectoryClient.Uri)
{
Sas = sourceSas
};

string destDirName = GetNewDirectoryName();

ShareClient sasShareClient = InstrumentClient(new ShareClient(sourceUriBuilder.ToUri(), GetOptions()));
ShareDirectoryClient sasDirectoryClient = InstrumentClient(sasShareClient.GetDirectoryClient(sourceDirectoryClient.Path));

// Make unique destination sas
string newPath = directoryClient.Path + "/" + destDirName;

// Act
ShareDirectoryClient destDirectory = await sasDirectoryClient.RenameAsync(destinationPath: newPath);

// Assert
Response<ShareDirectoryProperties> response = await destDirectory.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasCredentialDestSasUri()
Expand Down
37 changes: 34 additions & 3 deletions sdk/storage/Azure.Storage.Files.Shares/tests/FileClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,13 @@
using System.Threading;
using System.Threading.Tasks;
using Azure.Core.TestFramework;
using Azure.Identity;
using Azure.Storage.Files.Shares.Models;
using Azure.Storage.Files.Shares.Specialized;
using Azure.Storage.Sas;
using Azure.Storage.Test;
using Azure.Storage.Test.Shared;
using Microsoft.Diagnostics.Symbols;
using Moq;
using NUnit.Framework;
using NUnit.Framework.Internal;

namespace Azure.Storage.Files.Shares.Tests
{
Expand Down Expand Up @@ -6069,6 +6066,40 @@ public async Task RenameAsync_DifferentSasUri()
Response<ShareFileProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasUri()
{
// Arrange
string shareName = GetNewShareName();
await using DisposingShare test = await GetTestShareAsync(shareName: shareName);
string sourceDirectoryName = GetNewDirectoryName();
ShareDirectoryClient directoryClient = await test.Share.CreateDirectoryAsync(GetNewDirectoryName());
ShareDirectoryClient sourceDirectoryClient = await directoryClient.CreateSubdirectoryAsync(sourceDirectoryName);
ShareFileClient sourceFile = await sourceDirectoryClient.CreateFileAsync(GetNewFileName(), Constants.MB);

// Make unique source sas
SasQueryParameters sourceSas = GetNewFileServiceSasCredentialsShare(shareName);
ShareUriBuilder sourceUriBuilder = new ShareUriBuilder(sourceDirectoryClient.Uri)
{
Sas = sourceSas
};

string destFileName = GetNewFileName();

ShareDirectoryClient sasDirectoryClient = InstrumentClient(new ShareDirectoryClient(sourceUriBuilder.ToUri(), GetOptions()));
ShareFileClient sasFileClient = InstrumentClient(sasDirectoryClient.GetFileClient(sourceFile.Name));

// Make unique destination sas
string newPath = sourceDirectoryClient.Path + "/" + destFileName;

// Act
ShareFileClient destFile = await sasFileClient.RenameAsync(destinationPath: newPath);

// Assert
Response<ShareFileProperties> response = await destFile.GetPropertiesAsync();
}

[LiveOnly]
[Test]
public async Task RenameAsync_SourceSasCredentialDestSasUri()
Expand Down
Loading