Skip to content

Commit

Permalink
GH-43197: [C++][AzureFS] Ignore password field in URI (#44220)
Browse files Browse the repository at this point in the history 
### Rationale for this change

Other Azure Blob Storage based filesystem API implementations don't use password field in URI.

We don't use it too for compatibility.

### What changes are included in this PR?

Ignore password field.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**
* GitHub Issue: #43197

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
@kou
kou authored Sep 30, 2024
1 parent f4034ba commit b1d9f85
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 74 deletions.
52 changes: 16 additions & 36 deletions cpp/src/arrow/filesystem/azurefs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ void AzureOptions::ExtractFromUriSchemeAndHierPart(const Uri& uri,
}

Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
const auto account_key = uri.password();
std::optional<CredentialKind> credential_kind;
std::optional<std::string> credential_kind_value;
std::string tenant_id;
Expand Down Expand Up @@ -155,10 +154,6 @@ Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
}

if (credential_kind) {
if (!account_key.empty()) {
return Status::Invalid("Password must not be specified with credential_kind=",
*credential_kind_value);
}
if (!tenant_id.empty()) {
return Status::Invalid("tenant_id must not be specified with credential_kind=",
*credential_kind_value);
Expand Down Expand Up @@ -190,40 +185,25 @@ Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
break;
}
} else {
if (!account_key.empty()) {
// With password
if (!tenant_id.empty()) {
return Status::Invalid("tenant_id must not be specified with password");
}
if (!client_id.empty()) {
return Status::Invalid("client_id must not be specified with password");
}
if (!client_secret.empty()) {
return Status::Invalid("client_secret must not be specified with password");
if (tenant_id.empty() && client_id.empty() && client_secret.empty()) {
// No related parameters
if (account_name.empty()) {
RETURN_NOT_OK(ConfigureAnonymousCredential());
} else {
// Default credential
}
RETURN_NOT_OK(ConfigureAccountKeyCredential(account_key));
} else {
// Without password
if (tenant_id.empty() && client_id.empty() && client_secret.empty()) {
// No related parameters
if (account_name.empty()) {
RETURN_NOT_OK(ConfigureAnonymousCredential());
} else {
// Default credential
}
// One or more tenant_id, client_id or client_secret are specified
if (client_id.empty()) {
return Status::Invalid("client_id must be specified");
}
if (tenant_id.empty() && client_secret.empty()) {
RETURN_NOT_OK(ConfigureManagedIdentityCredential(client_id));
} else if (!tenant_id.empty() && !client_secret.empty()) {
RETURN_NOT_OK(
ConfigureClientSecretCredential(tenant_id, client_id, client_secret));
} else {
// One or more tenant_id, client_id or client_secret are specified
if (client_id.empty()) {
return Status::Invalid("client_id must be specified");
}
if (tenant_id.empty() && client_secret.empty()) {
RETURN_NOT_OK(ConfigureManagedIdentityCredential(client_id));
} else if (!tenant_id.empty() && !client_secret.empty()) {
RETURN_NOT_OK(
ConfigureClientSecretCredential(tenant_id, client_id, client_secret));
} else {
return Status::Invalid("Both of tenant_id and client_secret must be specified");
}
return Status::Invalid("Both of tenant_id and client_secret must be specified");
}
}
}
Expand Down
10 changes: 4 additions & 6 deletions cpp/src/arrow/filesystem/azurefs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,10 @@ struct ARROW_EXPORT AzureOptions {
///
/// Supported formats:
///
/// 1. abfs[s]://[:\<password\>@]\<account\>.blob.core.windows.net
/// [/\<container\>[/\<path\>]]
/// 2. abfs[s]://\<container\>[:\<password\>]\@\<account\>.dfs.core.windows.net[/path]
/// 3. abfs[s]://[\<account[:\<password\>]@]\<host[.domain]\>[\<:port\>]
/// [/\<container\>[/path]]
/// 4. abfs[s]://[\<account[:\<password\>]@]\<container\>[/path]
/// 1. abfs[s]://\<account\>.blob.core.windows.net[/\<container\>[/\<path\>]]
/// 2. abfs[s]://\<container\>\@\<account\>.dfs.core.windows.net[/path]
/// 3. abfs[s]://[\<account@]\<host[.domain]\>[\<:port\>][/\<container\>[/path]]
/// 4. abfs[s]://[\<account@]\<container\>[/path]
///
/// (1) and (2) are compatible with the Azure Data Lake Storage Gen2 URIs
/// [1], (3) is for Azure Blob Storage compatible service including Azurite,
Expand Down
50 changes: 18 additions & 32 deletions cpp/src/arrow/filesystem/azurefs_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,60 +562,58 @@ class TestAzureOptions : public ::testing::Test {

void TestFromUriAbfs() {
std::string path;
ASSERT_OK_AND_ASSIGN(
auto options,
AzureOptions::FromUri(
"abfs://account:password@127.0.0.1:10000/container/dir/blob", &path));
ASSERT_OK_AND_ASSIGN(auto options,
AzureOptions::FromUri(
"abfs://account@127.0.0.1:10000/container/dir/blob", &path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "https");
ASSERT_EQ(options.dfs_storage_scheme, "https");
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kStorageSharedKey);
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}

void TestFromUriAbfss() {
std::string path;
ASSERT_OK_AND_ASSIGN(
auto options,
AzureOptions::FromUri(
"abfss://account:password@127.0.0.1:10000/container/dir/blob", &path));
auto options, AzureOptions::FromUri(
"abfss://account@127.0.0.1:10000/container/dir/blob", &path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "https");
ASSERT_EQ(options.dfs_storage_scheme, "https");
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kStorageSharedKey);
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}

void TestFromUriEnableTls() {
std::string path;
ASSERT_OK_AND_ASSIGN(auto options,
AzureOptions::FromUri(
"abfs://account:password@127.0.0.1:10000/container/dir/blob?"
"enable_tls=false",
&path));
ASSERT_OK_AND_ASSIGN(
auto options,
AzureOptions::FromUri("abfs://account@127.0.0.1:10000/container/dir/blob?"
"enable_tls=false",
&path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "http");
ASSERT_EQ(options.dfs_storage_scheme, "http");
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kStorageSharedKey);
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}

void TestFromUriDisableBackgroundWrites() {
std::string path;
ASSERT_OK_AND_ASSIGN(auto options,
AzureOptions::FromUri(
"abfs://account:password@127.0.0.1:10000/container/dir/blob?"
"background_writes=false",
&path));
ASSERT_OK_AND_ASSIGN(
auto options,
AzureOptions::FromUri("abfs://account@127.0.0.1:10000/container/dir/blob?"
"background_writes=false",
&path));
ASSERT_EQ(options.background_writes, false);
}

Expand All @@ -637,15 +635,6 @@ class TestAzureOptions : public ::testing::Test {
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kAnonymous);
}

void TestFromUriCredentialStorageSharedKey() {
ASSERT_OK_AND_ASSIGN(
auto options,
AzureOptions::FromUri(
"abfs://:password@account.blob.core.windows.net/container/dir/blob",
nullptr));
ASSERT_EQ(options.credential_kind_, AzureOptions::CredentialKind::kStorageSharedKey);
}

void TestFromUriCredentialClientSecret() {
ASSERT_OK_AND_ASSIGN(
auto options,
Expand Down Expand Up @@ -767,9 +756,6 @@ TEST_F(TestAzureOptions, FromUriDisableBackgroundWrites) {
}
TEST_F(TestAzureOptions, FromUriCredentialDefault) { TestFromUriCredentialDefault(); }
TEST_F(TestAzureOptions, FromUriCredentialAnonymous) { TestFromUriCredentialAnonymous(); }
TEST_F(TestAzureOptions, FromUriCredentialStorageSharedKey) {
TestFromUriCredentialStorageSharedKey();
}
TEST_F(TestAzureOptions, FromUriCredentialClientSecret) {
TestFromUriCredentialClientSecret();
}
Expand Down

0 comments on commit b1d9f85

Please sign in to comment.