From ffd79fcc74b25a6bb09bf44fb0640a027bd7d731 Mon Sep 17 00:00:00 2001 From: Kan Tang Date: Wed, 20 Jan 2021 12:08:49 +0800 Subject: [PATCH] Refined the ACL/Permission APIs according to review comments. (#1399) * Refined the ACL/Permission APIs according to review comments. * Update CHANGELOG.md * Update datalake_options.hpp --- .../azure-storage-files-datalake/CHANGELOG.md | 5 ++ .../files/datalake/datalake_options.hpp | 33 +++++++--- .../files/datalake/datalake_path_client.hpp | 29 ++++++--- .../files/datalake/datalake_responses.hpp | 6 +- .../src/datalake_path_client.cpp | 37 +++++++++-- .../test/datalake_directory_client_test.cpp | 2 +- .../test/datalake_path_client_test.cpp | 64 ++++++++++++++++--- .../test/datalake_sas_test.cpp | 2 +- 8 files changed, 146 insertions(+), 32 deletions(-) diff --git a/sdk/storage/azure-storage-files-datalake/CHANGELOG.md b/sdk/storage/azure-storage-files-datalake/CHANGELOG.md index 0b280d6e4..6f50821f4 100644 --- a/sdk/storage/azure-storage-files-datalake/CHANGELOG.md +++ b/sdk/storage/azure-storage-files-datalake/CHANGELOG.md @@ -2,9 +2,14 @@ ## 12.0.0-beta.7 (Unreleased) +### New Features + +- Added `Owner`, `Permissions`, and `Group` to `GetDataLakePathAccessControlResult`. + ### Breaking Changes - Removed `GetDfsUri` in all clients since they are currently implementation details. +- `DataLakePathClient` can no longer set permissions with `SetAccessControl`, instead, a new API `SetPermissions` is created for such functionality. Renamed the original API to `SetAccessControlList` to be more precise. ## 12.0.0-beta.6 (2020-01-14) diff --git a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_options.hpp b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_options.hpp index 9f8ab4202..48605a941 100644 --- a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_options.hpp +++ b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_options.hpp @@ -283,9 +283,9 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { }; /** - * @brief Optional parameters for PathClient::SetAccessControl + * @brief Optional parameters for PathClient::SetAccessControlList */ - struct SetDataLakePathAccessControlOptions + struct SetDataLakePathAccessControlListOptions { /** * @brief Context for cancelling long running operations. @@ -303,13 +303,30 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { Azure::Core::Nullable Group; /** - * @brief only valid if Hierarchical Namespace is enabled for the account. Sets POSIX - * access permissions for the file owner, the file owning group, and others. - * Each class may be granted read, write, or execute permission. - * The sticky bit is also supported. Both symbolic (rwxrw-rw-) and 4-digit octal - * notation (e.g. 0766) are supported. + * @brief Specify the access condition for the path. */ - Azure::Core::Nullable Permissions; + PathAccessConditions AccessConditions; + }; + + /** + * @brief Optional parameters for PathClient::SetPermissions + */ + struct SetDataLakePathPermissionsOptions + { + /** + * @brief Context for cancelling long running operations. + */ + Azure::Core::Context Context; + + /** + * @brief The owner of the path or directory. + */ + Azure::Core::Nullable Owner; + + /** + * @brief The owning group of the path or directory. + */ + Azure::Core::Nullable Group; /** * @brief Specify the access condition for the path. diff --git a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_path_client.hpp b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_path_client.hpp index 433528b27..526ae851e 100644 --- a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_path_client.hpp +++ b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_path_client.hpp @@ -122,24 +122,37 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { const DeleteDataLakePathOptions& options = DeleteDataLakePathOptions()) const; /** - * @brief Sets the owner, group, permissions, or access control list for a file or directory. + * @brief Sets the owner, group, and access control list for a file or directory. * Note that Hierarchical Namespace must be enabled for the account in order to use - * access control. Also note that the Access Control List (ACL) includes permissions for - * the owner, owning group, and others, so the x-ms-permissions and x-ms-acl request - * headers are mutually exclusive. + * access control. * @param acls Sets POSIX access control rights on files and directories. Each access control * entry (ACE) consists of a scope, a type, a user or group identifier, and * permissions. * @param options Optional parameters to set an access control to the resource the path points * to. - * @return Azure::Core::Response containing the + * @return Azure::Core::Response containing the * information returned when setting path's access control. * @remark This request is sent to dfs endpoint. */ - Azure::Core::Response SetAccessControl( + Azure::Core::Response SetAccessControlList( std::vector acls, - const SetDataLakePathAccessControlOptions& options - = SetDataLakePathAccessControlOptions()) const; + const SetDataLakePathAccessControlListOptions& options + = SetDataLakePathAccessControlListOptions()) const; + + /** + * @brief Sets the owner, group, and permissions for a file or directory. + * Note that Hierarchical Namespace must be enabled for the account in order to use + * access control. + * @param permissions Sets the permissions on the path + * @param options Optional parameters to set permissions to the resource the path points to. + * @return Azure::Core::Response containing the + * information returned when setting path's permissions. + * @remark This request is sent to dfs endpoint. + */ + Azure::Core::Response SetPermissions( + std::string permissions, + const SetDataLakePathPermissionsOptions& options + = SetDataLakePathPermissionsOptions()) const; /** * @brief Sets the properties of a resource the path points to. diff --git a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_responses.hpp b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_responses.hpp index 63f836e6e..92020288d 100644 --- a/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_responses.hpp +++ b/sdk/storage/azure-storage-files-datalake/inc/azure/storage/files/datalake/datalake_responses.hpp @@ -122,6 +122,9 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { nam { std::string ETag; Core::DateTime LastModified; + std::string Owner; + std::string Group; + std::string Permissions; std::vector Acls; }; @@ -145,7 +148,8 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { nam Azure::Core::Nullable ContentLength; }; - using SetDataLakePathAccessControlResult = PathSetAccessControlResult; + using SetDataLakePathAccessControlListResult = PathSetAccessControlResult; + using SetDataLakePathPermissionsResult = PathSetAccessControlResult; // FileClient models: diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp index ec9797b8d..0127eba6b 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp @@ -188,16 +188,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { m_pipeline = std::make_shared(policies); } - Azure::Core::Response - DataLakePathClient::SetAccessControl( + Azure::Core::Response + DataLakePathClient::SetAccessControlList( std::vector acls, - const SetDataLakePathAccessControlOptions& options) const + const SetDataLakePathAccessControlListOptions& options) const { Details::DataLakeRestClient::Path::SetAccessControlOptions protocolLayerOptions; protocolLayerOptions.LeaseIdOptional = options.AccessConditions.LeaseId; protocolLayerOptions.Owner = options.Owner; protocolLayerOptions.Group = options.Group; - protocolLayerOptions.Permissions = options.Permissions; protocolLayerOptions.Acl = Models::Acl::SerializeAcls(acls); protocolLayerOptions.IfMatch = options.AccessConditions.IfMatch; protocolLayerOptions.IfNoneMatch = options.AccessConditions.IfNoneMatch; @@ -207,6 +206,24 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { m_dfsUri, *m_pipeline, options.Context, protocolLayerOptions); } + Azure::Core::Response + DataLakePathClient::SetPermissions( + std::string permissions, + const SetDataLakePathPermissionsOptions& options) const + { + Details::DataLakeRestClient::Path::SetAccessControlOptions protocolLayerOptions; + protocolLayerOptions.LeaseIdOptional = options.AccessConditions.LeaseId; + protocolLayerOptions.Owner = options.Owner; + protocolLayerOptions.Group = options.Group; + protocolLayerOptions.Permissions = permissions; + protocolLayerOptions.IfMatch = options.AccessConditions.IfMatch; + protocolLayerOptions.IfNoneMatch = options.AccessConditions.IfNoneMatch; + protocolLayerOptions.IfModifiedSince = options.AccessConditions.IfModifiedSince; + protocolLayerOptions.IfUnmodifiedSince = options.AccessConditions.IfUnmodifiedSince; + return Details::DataLakeRestClient::Path::SetAccessControl( + m_dfsUri, *m_pipeline, options.Context, protocolLayerOptions); + } + Azure::Core::Response DataLakePathClient::SetHttpHeaders( Models::PathHttpHeaders httpHeaders, @@ -395,6 +412,18 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { throw std::runtime_error("Got null value returned when getting access control."); } ret.Acls = std::move(acl.GetValue()); + if (result->Owner.HasValue()) + { + ret.Owner = result->Owner.GetValue(); + } + if (result->Group.HasValue()) + { + ret.Group = result->Group.GetValue(); + } + if (result->Permissions.HasValue()) + { + ret.Permissions = result->Permissions.GetValue(); + } return Azure::Core::Response( std::move(ret), result.ExtractRawResponse()); } diff --git a/sdk/storage/azure-storage-files-datalake/test/datalake_directory_client_test.cpp b/sdk/storage/azure-storage-files-datalake/test/datalake_directory_client_test.cpp index b0eb0320c..c7366f134 100644 --- a/sdk/storage/azure-storage-files-datalake/test/datalake_directory_client_test.cpp +++ b/sdk/storage/azure-storage-files-datalake/test/datalake_directory_client_test.cpp @@ -344,7 +344,7 @@ namespace Azure { namespace Storage { namespace Test { { // Set/Get Acls recursive works. std::vector acls = GetValidAcls(); - EXPECT_NO_THROW(directoryClient1.SetAccessControl(acls)); + EXPECT_NO_THROW(directoryClient1.SetAccessControlList(acls)); EXPECT_NO_THROW(rootDirectoryClient.SetAccessControlRecursive( Files::DataLake::Models::PathSetAccessControlRecursiveMode::Modify, acls)); std::vector resultAcls1; diff --git a/sdk/storage/azure-storage-files-datalake/test/datalake_path_client_test.cpp b/sdk/storage/azure-storage-files-datalake/test/datalake_path_client_test.cpp index a8a9c7ac8..9cab7fe7d 100644 --- a/sdk/storage/azure-storage-files-datalake/test/datalake_path_client_test.cpp +++ b/sdk/storage/azure-storage-files-datalake/test/datalake_path_client_test.cpp @@ -193,7 +193,7 @@ namespace Azure { namespace Storage { namespace Test { { // Set/Get Acls works. std::vector acls = GetValidAcls(); - EXPECT_NO_THROW(m_pathClient->SetAccessControl(acls)); + EXPECT_NO_THROW(m_pathClient->SetAccessControlList(acls)); std::vector resultAcls; EXPECT_NO_THROW(resultAcls = m_pathClient->GetAccessControls()->Acls); EXPECT_EQ(resultAcls.size(), acls.size() + 1); // Always append mask::rwx @@ -216,24 +216,70 @@ namespace Azure { namespace Storage { namespace Test { std::vector acls = GetValidAcls(); auto response = m_pathClient->GetProperties(); - Files::DataLake::SetDataLakePathAccessControlOptions options1; + Files::DataLake::SetDataLakePathAccessControlListOptions options1; options1.AccessConditions.IfModifiedSince = response->LastModified; - EXPECT_THROW(m_pathClient->SetAccessControl(acls, options1), StorageException); - Files::DataLake::SetDataLakePathAccessControlOptions options2; + EXPECT_THROW(m_pathClient->SetAccessControlList(acls, options1), StorageException); + Files::DataLake::SetDataLakePathAccessControlListOptions options2; options2.AccessConditions.IfUnmodifiedSince = response->LastModified; - EXPECT_NO_THROW(m_pathClient->SetAccessControl(acls, options2)); + EXPECT_NO_THROW(m_pathClient->SetAccessControlList(acls, options2)); } { // Set/Get Acls works with if match access condition. std::vector acls = GetValidAcls(); auto response = m_pathClient->GetProperties(); - Files::DataLake::SetDataLakePathAccessControlOptions options1; + Files::DataLake::SetDataLakePathAccessControlListOptions options1; options1.AccessConditions.IfNoneMatch = response->ETag; - EXPECT_THROW(m_pathClient->SetAccessControl(acls, options1), StorageException); - Files::DataLake::SetDataLakePathAccessControlOptions options2; + EXPECT_THROW(m_pathClient->SetAccessControlList(acls, options1), StorageException); + Files::DataLake::SetDataLakePathAccessControlListOptions options2; options2.AccessConditions.IfMatch = response->ETag; - EXPECT_NO_THROW(m_pathClient->SetAccessControl(acls, options2)); + EXPECT_NO_THROW(m_pathClient->SetAccessControlList(acls, options2)); + } + } + + TEST_F(DataLakePathClientTest, PathSetPermissions) + { + { + auto pathClient = Files::DataLake::DataLakePathClient::CreateFromConnectionString( + AdlsGen2ConnectionString(), m_fileSystemName, RandomString()); + pathClient.Create(Files::DataLake::Models::PathResourceType::File); + std::string pathPermissions = "rwxrw-rw-"; + EXPECT_NO_THROW(pathClient.SetPermissions(pathPermissions)); + auto result = pathClient.GetAccessControls(); + EXPECT_EQ(pathPermissions, result->Permissions); + + pathPermissions = "rw-rw-rw-"; + EXPECT_NO_THROW(pathClient.SetPermissions(pathPermissions)); + result = pathClient.GetAccessControls(); + EXPECT_EQ(pathPermissions, result->Permissions); + + EXPECT_NO_THROW(pathClient.SetPermissions("0766")); + result = pathClient.GetAccessControls(); + EXPECT_EQ("rwxrw-rw-", result->Permissions); + } + { + // Set/Get Permissions works with last modified access condition. + auto pathClient = Files::DataLake::DataLakePathClient::CreateFromConnectionString( + AdlsGen2ConnectionString(), m_fileSystemName, RandomString()); + auto response = pathClient.Create(Files::DataLake::Models::PathResourceType::File); + Files::DataLake::SetDataLakePathPermissionsOptions options1, options2; + options1.AccessConditions.IfUnmodifiedSince = response->LastModified; + options2.AccessConditions.IfModifiedSince = response->LastModified; + std::string pathPermissions = "rwxrw-rw-"; + EXPECT_THROW(pathClient.SetPermissions(pathPermissions, options2), StorageException); + EXPECT_NO_THROW(pathClient.SetPermissions(pathPermissions, options1)); + } + { + // Set/Get Permissions works with if match access condition. + auto pathClient = Files::DataLake::DataLakePathClient::CreateFromConnectionString( + AdlsGen2ConnectionString(), m_fileSystemName, RandomString()); + auto response = pathClient.Create(Files::DataLake::Models::PathResourceType::File); + Files::DataLake::SetDataLakePathPermissionsOptions options1, options2; + options1.AccessConditions.IfMatch = response->ETag; + options2.AccessConditions.IfNoneMatch = response->ETag; + std::string pathPermissions = "rwxrw-rw-"; + EXPECT_THROW(pathClient.SetPermissions(pathPermissions, options2), StorageException); + EXPECT_NO_THROW(pathClient.SetPermissions(pathPermissions, options1)); } } diff --git a/sdk/storage/azure-storage-files-datalake/test/datalake_sas_test.cpp b/sdk/storage/azure-storage-files-datalake/test/datalake_sas_test.cpp index aa79eb330..1b9549326 100644 --- a/sdk/storage/azure-storage-files-datalake/test/datalake_sas_test.cpp +++ b/sdk/storage/azure-storage-files-datalake/test/datalake_sas_test.cpp @@ -145,7 +145,7 @@ namespace Azure { namespace Storage { namespace Test { fileClient0.Create(); auto fileClient = Files::DataLake::DataLakeFileClient(fileUri + sas); auto acls = fileClient0.GetAccessControls()->Acls; - EXPECT_NO_THROW(fileClient.SetAccessControl(acls)); + EXPECT_NO_THROW(fileClient.SetAccessControlList(acls)); }; for (auto permissions : {