Remove AzureOptions::GenerateSASToken

Tom-Newton · Dec 14, 2024 · c8fd94d · c8fd94d
1 parent 744e111
commit c8fd94d
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 40 deletions.
diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
@@ -433,38 +433,6 @@ AzureOptions::MakeDataLakeServiceClient() const {
   return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
 }
 
-Result<std::string> AzureOptions::GenerateSASToken(
-    Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
-  using SasProtocol = Storage::Sas::SasProtocol;
-  builder->Protocol =
-      blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
-  switch (credential_kind_) {
-    case CredentialKind::kAnonymous:
-      // Anonymous is for public storage accounts so no SAS token needed.
-    case CredentialKind::kSASToken:
-      // When using SAS token auth BlobClient::GetUrl() will return a URL with the
-      // original SAS token, so we don't need to generate a new one.
-      return "";
-    case CredentialKind::kStorageSharedKey:
-      return builder->GenerateSasToken(*storage_shared_key_credential_);
-    case CredentialKind::kClientSecret:
-    case CredentialKind::kManagedIdentity:
-    case CredentialKind::kCLI:
-    case CredentialKind::kWorkloadIdentity:
-    case CredentialKind::kEnvironment:
-    case CredentialKind::kDefault:
-      // GH-39344: This part isn't tested.
-      try {
-        auto delegation_key_response = client->GetUserDelegationKey(builder->ExpiresOn);
-        return builder->GenerateSasToken(delegation_key_response.Value, account_name);
-      } catch (const Storage::StorageException& exception) {
-        return ExceptionToStatus(exception, "GetUserDelegationKey failed for '",
-                                 client->GetUrl(), "'.");
-      }
-  }
-  return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
-}
-
 namespace {
 
 // An AzureFileSystem represents an Azure storage account. An AzureLocation describes a

diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
@@ -213,10 +213,6 @@ struct ARROW_EXPORT AzureOptions {
 
   Result<std::unique_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>>
   MakeDataLakeServiceClient() const;
-
-  Result<std::string> GenerateSASToken(
-      Azure::Storage::Sas::BlobSasBuilder* builder,
-      Azure::Storage::Blobs::BlobServiceClient* client) const;
 };
 
 /// \brief FileSystem implementation backed by Azure Blob Storage (ABS) [1] and

diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc
@@ -936,15 +936,18 @@ class TestAzureFileSystem : public ::testing::Test {
         .Value;
   }
 
-  Result<std::string> GetContainerSasToken(const std::string& container_name) {
+  Result<std::string> GetContainerSasToken(
+      const std::string& container_name,
+      Azure::Storage::StorageSharedKeyCredential storage_shared_key_credential) {
     std::string sas_token;
     Azure::Storage::Sas::BlobSasBuilder builder;
     std::chrono::seconds available_period(60);
     builder.ExpiresOn = std::chrono::system_clock::now() + available_period;
     builder.BlobContainerName = container_name;
     builder.Resource = Azure::Storage::Sas::BlobSasResource::BlobContainer;
     builder.SetPermissions(Azure::Storage::Sas::BlobContainerSasPermissions::All);
-    return options_.GenerateSASToken(&builder, blob_service_client_.get());
+    builder.Protocol = Azure::Storage::Sas::SasProtocol::HttpsAndHttp;
+    return builder.GenerateSasToken(storage_shared_key_credential);
   }
 
   void UploadLines(const std::vector<std::string>& lines, const std::string& path,
@@ -964,6 +967,8 @@ class TestAzureFileSystem : public ::testing::Test {
     } else {
       auto container_client = CreateContainer(data.container_name);
       CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+      // CreateBlob(container_client, data.kObjectName, data.RandomChars((1 << 30),
+      // rng_));
     }
     return data;
   }
@@ -1657,7 +1662,11 @@ class TestAzureFileSystem : public ::testing::Test {
 
     ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv());
     ASSERT_OK_AND_ASSIGN(auto options, MakeOptions(env));
-    ASSERT_OK_AND_ASSIGN(auto sas_token, GetContainerSasToken(data.container_name));
+    ASSERT_OK_AND_ASSIGN(
+        auto sas_token,
+        GetContainerSasToken(data.container_name,
+                             Azure::Storage::StorageSharedKeyCredential(
+                                 env->account_name(), env->account_key())));
     // AzureOptions::FromUri will not cut off extra query parameters that it consumes, so
     // make sure these don't cause problems.
     ARROW_EXPECT_OK(options.ConfigureSasCredential(
@@ -1667,7 +1676,7 @@ class TestAzureFileSystem : public ::testing::Test {
 
     AssertFileInfo(fs.get(), data.ObjectPath(), FileType::File);
 
-    // Test copying because the most obvious implementation requires generating a SAS
+    // Test CopyFile because the most obvious implementation requires generating a SAS
     // token at runtime which doesn't work when the original auth is SAS token.
     ASSERT_OK(fs->CopyFile(data.ObjectPath(), data.ObjectPath() + "_copy"));
     AssertFileInfo(fs.get(), data.ObjectPath() + "_copy", FileType::File);