From 5435ac903e3e268d36de25af644f210172b29e00 Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Thu, 16 Mar 2023 07:07:43 -0700 Subject: [PATCH 1/5] Simpler identity logging --- .../src/azure_cli_credential.cpp | 21 ++--- .../src/chained_token_credential.cpp | 74 +++++++----------- .../src/default_azure_credential.cpp | 29 ++++--- .../src/environment_credential.cpp | 73 +++++++----------- .../src/managed_identity_source.cpp | 43 ++++------- .../test/ut/default_azure_credential_test.cpp | 37 +++++---- .../test/ut/environment_credential_test.cpp | 76 +++++++++++++------ 7 files changed, 162 insertions(+), 191 deletions(-) diff --git a/sdk/identity/azure-identity/src/azure_cli_credential.cpp b/sdk/identity/azure-identity/src/azure_cli_credential.cpp index f2c631adc1..f02c32f7e1 100644 --- a/sdk/identity/azure-identity/src/azure_cli_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_cli_credential.cpp @@ -91,15 +91,11 @@ AzureCliCredential::AzureCliCredential( ThrowIfNotSafeCmdLineInput(m_tenantId, "TenantID"); - auto const logLevel = Logger::Level::Informational; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + GetCredentialName() - + " created.\n" - "Successful creation does not guarantee further successful token retrieval."); - } + Log::Write( + Logger::Level::Informational, + IdentityPrefix + GetCredentialName() + + " created.\n" + "Successful creation does not guarantee further successful token retrieval."); } AzureCliCredential::AzureCliCredential(AzureCliCredentialOptions const& options) @@ -168,12 +164,7 @@ AccessToken AzureCliCredential::GetToken( auto const errorMsg = IdentityPrefix + GetCredentialName() + " didn't get the token: \"" + e.what() + '\"'; - auto const logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) - { - Log::Write(logLevel, errorMsg); - } - + Log::Write(Logger::Level::Warning, errorMsg); throw AuthenticationException(errorMsg); } }); diff --git a/sdk/identity/azure-identity/src/chained_token_credential.cpp b/sdk/identity/azure-identity/src/chained_token_credential.cpp index ee4b64cd03..f6c2b3a5a1 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -73,60 +73,38 @@ AccessToken ChainedTokenCredentialImpl::GetToken( if (sourcesSize == 0) { - const auto logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credentialName - + ": Authentication did not succeed: List of sources is empty."); - } + Log::Write( + Logger::Level::Warning, + IdentityPrefix + credentialName + + ": Authentication did not succeed: List of sources is empty."); } - else + + for (size_t i = 0; i < sourcesSize; ++i) { - for (size_t i = 0; i < sourcesSize; ++i) + try { - try - { - auto token = m_sources[i]->GetToken(tokenRequestContext, context); + auto token = m_sources[i]->GetToken(tokenRequestContext, context); - { - auto const logLevel = Logger::Level::Informational; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credentialName + ": Successfully got token from " - + m_sources[i]->GetCredentialName() + '.'); - } - } + Log::Write( + Logger::Level::Informational, + IdentityPrefix + credentialName + ": Successfully got token from " + + m_sources[i]->GetCredentialName() + '.'); - return token; - } - catch (AuthenticationException const& e) - { - { - auto const logLevel = Logger::Level::Verbose; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credentialName + ": Failed to get token from " - + m_sources[i]->GetCredentialName() + ": " + e.what()); - } - } + return token; + } + catch (AuthenticationException const& e) + { + Log::Write( + Logger::Level::Verbose, + IdentityPrefix + credentialName + ": Failed to get token from " + + m_sources[i]->GetCredentialName() + ": " + e.what()); - if ((sourcesSize - 1) == i) // On the last credential - { - auto const logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credentialName - + ": Didn't succeed to get a token from any credential in the chain."); - } - } + if ((sourcesSize - 1) == i) // On the last credential + { + Log::Write( + Logger::Level::Warning, + IdentityPrefix + credentialName + + ": Didn't succeed to get a token from any credential in the chain."); } } } diff --git a/sdk/identity/azure-identity/src/default_azure_credential.cpp b/sdk/identity/azure-identity/src/default_azure_credential.cpp index 4b4909192f..d50c2a6e73 100644 --- a/sdk/identity/azure-identity/src/default_azure_credential.cpp +++ b/sdk/identity/azure-identity/src/default_azure_credential.cpp @@ -26,22 +26,19 @@ DefaultAzureCredential::DefaultAzureCredential(TokenCredentialOptions const& opt { // Initializing m_credential below and not in the member initializer list to have a specific order // of log messages. - auto const logLevel = Logger::Level::Verbose; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - std::string(IdentityPrefix) + "Creating " + GetCredentialName() - + " which combines mutiple parameterless credentials into a single one.\n" - + GetCredentialName() - + " is only recommended for the early stages of development, " - "and not for usage in production environment." - "\nOnce the developer focuses on the Credentials and Authentication aspects " - "of their application, " - + GetCredentialName() - + " needs to be replaced with the credential that " - "is the better fit for the application."); - } + + Log::Write( + Logger::Level::Verbose, + std::string(IdentityPrefix) + "Creating " + GetCredentialName() + + " which combines mutiple parameterless credentials into a single one.\n" + + GetCredentialName() + + " is only recommended for the early stages of development, " + "and not for usage in production environment." + "\nOnce the developer focuses on the Credentials and Authentication aspects " + "of their application, " + + GetCredentialName() + + " needs to be replaced with the credential that " + "is the better fit for the application."); // Creating credentials in order to ensure the order of log messages. auto const envCred = std::make_shared(options); diff --git a/sdk/identity/azure-identity/src/environment_credential.cpp b/sdk/identity/azure-identity/src/environment_credential.cpp index f6dd764149..7d96727da6 100644 --- a/sdk/identity/azure-identity/src/environment_credential.cpp +++ b/sdk/identity/azure-identity/src/environment_credential.cpp @@ -129,38 +129,32 @@ EnvironmentCredential::EnvironmentCredential(TokenCredentialOptions options) if (!m_credentialImpl) { - auto const logLevel = Logger::Level::Warning; + Log::Write( + Logger::Level::Warning, logMsgPrefix + " was not initialized with underlying credential."); + + auto const logLevel = Logger::Level::Verbose; if (Log::ShouldWrite(logLevel)) { - auto const basicMessage = logMsgPrefix + " was not initialized with underlying credential"; - - if (!Log::ShouldWrite(Logger::Level::Verbose)) - { - Log::Write(logLevel, basicMessage + '.'); - } - else + auto logMsg = logMsgPrefix + ": Both '" + AzureTenantIdEnvVarName + "' and '" + + AzureClientIdEnvVarName + "', and at least one of '" + AzureClientSecretEnvVarName + + "', '" + AzureClientCertificatePathEnvVarName + "' needs to be set. Additionally, '" + + AzureAuthorityHostEnvVarName + + "' could be set to override the default authority host. Currently:\n"; + + std::pair envVarStatus[] = { + {AzureTenantIdEnvVarName, !tenantId.empty()}, + {AzureClientIdEnvVarName, !clientId.empty()}, + {AzureClientSecretEnvVarName, !clientSecret.empty()}, + {AzureClientCertificatePathEnvVarName, !clientCertificatePath.empty()}, + {AzureAuthorityHostEnvVarName, !authority.empty()}, + }; + for (auto const& status : envVarStatus) { - auto logMsg = basicMessage + ": Both '" + AzureTenantIdEnvVarName + "' and '" - + AzureClientIdEnvVarName + "', and at least one of '" + AzureClientSecretEnvVarName - + "', '" + AzureClientCertificatePathEnvVarName + "' needs to be set. Additionally, '" - + AzureAuthorityHostEnvVarName - + "' could be set to override the default authority host. Currently:\n"; - - std::pair envVarStatus[] = { - {AzureTenantIdEnvVarName, !tenantId.empty()}, - {AzureClientIdEnvVarName, !clientId.empty()}, - {AzureClientSecretEnvVarName, !clientSecret.empty()}, - {AzureClientCertificatePathEnvVarName, !clientCertificatePath.empty()}, - {AzureAuthorityHostEnvVarName, !authority.empty()}, - }; - for (auto const& status : envVarStatus) - { - logMsg += std::string(" * '") + status.first + "' " + "is" - + (status.second ? " " : " NOT ") + "set\n"; - } - - Log::Write(logLevel, logMsg); + logMsg += std::string(" * '") + status.first + "' " + "is" + (status.second ? " " : " NOT ") + + "set\n"; } + + Log::Write(logLevel, logMsg); } } } @@ -174,15 +168,9 @@ AccessToken EnvironmentCredential::GetToken( auto const AuthUnavailable = IdentityPrefix + GetCredentialName() + " authentication unavailable. "; - { - auto const logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - AuthUnavailable + "See earlier " + GetCredentialName() + " log messages for details."); - } - } + Log::Write( + Logger::Level::Warning, + AuthUnavailable + "See earlier " + GetCredentialName() + " log messages for details."); throw AuthenticationException( AuthUnavailable + "Environment variables are not fully configured."); @@ -197,15 +185,12 @@ void PrintCredentialCreationLogMessage( std::vector> const& envVarsToParams, char const* credThatGetsCreated) { + Log::Write( + Logger::Level::Informational, + logMsgPrefix + " gets created with " + credThatGetsCreated + '.'); + if (!Log::ShouldWrite(Logger::Level::Verbose)) { - if (Log::ShouldWrite(Logger::Level::Informational)) - { - Log::Write( - Logger::Level::Informational, - logMsgPrefix + " gets created with " + credThatGetsCreated + '.'); - } - return; } diff --git a/sdk/identity/azure-identity/src/managed_identity_source.cpp b/sdk/identity/azure-identity/src/managed_identity_source.cpp index 89dfa0a773..daea912fe3 100644 --- a/sdk/identity/azure-identity/src/managed_identity_source.cpp +++ b/sdk/identity/azure-identity/src/managed_identity_source.cpp @@ -28,14 +28,10 @@ std::string WithSourceMessage(std::string const& credSource) void PrintEnvNotSetUpMessage(std::string const& credName, std::string const& credSource) { - auto const logLevel = Logger::Level::Verbose; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credName + ": Environment is not set up for the credential to be created" - + WithSourceMessage(credSource) + '.'); - } + Log::Write( + Logger::Level::Verbose, + IdentityPrefix + credName + ": Environment is not set up for the credential to be created" + + WithSourceMessage(credSource) + '.'); } } // namespace @@ -52,13 +48,9 @@ Azure::Core::Url ManagedIdentitySource::ParseEndpointUrl( { auto const endpointUrl = Url(url); - auto const logLevel = Logger::Level::Informational; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credName + " will be created" + WithSourceMessage(credSource) + '.'); - } + Log::Write( + Logger::Level::Informational, + IdentityPrefix + credName + " will be created" + WithSourceMessage(credSource) + '.'); return endpointUrl; } @@ -73,12 +65,7 @@ Azure::Core::Url ManagedIdentitySource::ParseEndpointUrl( + ": Failed to create: The environment variable \'" + envVarName + "\' contains an invalid URL."; - auto const logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) - { - Log::Write(logLevel, IdentityPrefix + errorMessage); - } - + Log::Write(Logger::Level::Warning, IdentityPrefix + errorMessage); throw AuthenticationException(errorMessage); } @@ -385,15 +372,11 @@ std::unique_ptr ImdsManagedIdentitySource::Create( std::string const& clientId, Azure::Core::Credentials::TokenCredentialOptions const& options) { - auto const logLevel = Logger::Level::Informational; - if (Log::ShouldWrite(logLevel)) - { - Log::Write( - logLevel, - IdentityPrefix + credName + " will be created" - + WithSourceMessage("Azure Instance Metadata Service") - + ".\nSuccessful creation does not guarantee further successful token retrieval."); - } + Log::Write( + Logger::Level::Informational, + IdentityPrefix + credName + " will be created" + + WithSourceMessage("Azure Instance Metadata Service") + + ".\nSuccessful creation does not guarantee further successful token retrieval."); return std::unique_ptr(new ImdsManagedIdentitySource(clientId, options)); } diff --git a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp index d3498d6cc7..7ab26d4b50 100644 --- a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp @@ -69,7 +69,7 @@ TEST(DefaultAzureCredential, LogMessages) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(9)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(10)); EXPECT_EQ(log[0].first, Logger::Level::Verbose); EXPECT_EQ( @@ -82,54 +82,59 @@ TEST(DefaultAzureCredential, LogMessages) "application, DefaultAzureCredential needs to be replaced with the credential that " "is the better fit for the application."); - EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ(log[1].first, Logger::Level::Informational); EXPECT_EQ( log[1].second, + "Identity: EnvironmentCredential gets created with ClientSecretCredential."); + + EXPECT_EQ(log[2].first, Logger::Level::Verbose); + EXPECT_EQ( + log[2].second, "Identity: EnvironmentCredential: 'AZURE_TENANT_ID', 'AZURE_CLIENT_ID', " "'AZURE_CLIENT_SECRET', and 'AZURE_AUTHORITY_HOST' environment variables are set, so " "ClientSecretCredential with corresponding tenantId, clientId, clientSecret, and " "authorityHost gets created."); - EXPECT_EQ(log[2].first, Logger::Level::Informational); + EXPECT_EQ(log[3].first, Logger::Level::Informational); EXPECT_EQ( - log[2].second, + log[3].second, "Identity: AzureCliCredential created." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[3].first, Logger::Level::Verbose); - EXPECT_EQ( - log[3].second, - "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2019 source."); - EXPECT_EQ(log[4].first, Logger::Level::Verbose); EXPECT_EQ( log[4].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2017 source."); + "to be created with App Service 2019 source."); EXPECT_EQ(log[5].first, Logger::Level::Verbose); EXPECT_EQ( log[5].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with Cloud Shell source."); + "to be created with App Service 2017 source."); EXPECT_EQ(log[6].first, Logger::Level::Verbose); EXPECT_EQ( log[6].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with Azure Arc source."); + "to be created with Cloud Shell source."); - EXPECT_EQ(log[7].first, Logger::Level::Informational); + EXPECT_EQ(log[7].first, Logger::Level::Verbose); EXPECT_EQ( log[7].second, + "Identity: ManagedIdentityCredential: Environment is not set up for the credential " + "to be created with Azure Arc source."); + + EXPECT_EQ(log[8].first, Logger::Level::Informational); + EXPECT_EQ( + log[8].second, "Identity: ManagedIdentityCredential will be created " "with Azure Instance Metadata Service source." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[8].first, Logger::Level::Informational); + EXPECT_EQ(log[9].first, Logger::Level::Informational); EXPECT_EQ( - log[8].second, + log[9].second, "Identity: DefaultAzureCredential: Created with the following credentials: " "EnvironmentCredential, AzureCliCredential, ManagedIdentityCredential."); diff --git a/sdk/identity/azure-identity/test/ut/environment_credential_test.cpp b/sdk/identity/azure-identity/test/ut/environment_credential_test.cpp index 61066e7c2b..71f4b6c063 100644 --- a/sdk/identity/azure-identity/test/ut/environment_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/environment_credential_test.cpp @@ -53,14 +53,21 @@ TEST(EnvironmentCredential, RegularClientSecretCredential) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(1)); - EXPECT_EQ(log[0].first, Logger::Level::Verbose); + EXPECT_EQ(log.size(), LogMsgVec::size_type(2)); + + EXPECT_EQ(log[0].first, Logger::Level::Informational); EXPECT_EQ( log[0].second, + "Identity: EnvironmentCredential gets created with ClientSecretCredential."); + + EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ( + log[1].second, "Identity: EnvironmentCredential: 'AZURE_TENANT_ID', 'AZURE_CLIENT_ID', " "'AZURE_CLIENT_SECRET', and 'AZURE_AUTHORITY_HOST' environment variables are set, so " "ClientSecretCredential with corresponding tenantId, clientId, clientSecret, and " "authorityHost gets created."); + log.clear(); return credential; @@ -202,20 +209,26 @@ TEST(EnvironmentCredential, Unavailable) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(1)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(2)); + EXPECT_EQ(log[0].first, Logger::Level::Warning); EXPECT_EQ( log[0].second, - "Identity: EnvironmentCredential was not initialized with underlying credential: Both " - "'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID', and at least one of 'AZURE_CLIENT_SECRET', " - "'AZURE_CLIENT_CERTIFICATE_PATH' needs to be set. Additionally, " - "'AZURE_AUTHORITY_HOST' could be set to override the default authority host." - " Currently:\n" + "Identity: EnvironmentCredential was not initialized with underlying credential."); + + EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ( + log[1].second, + "Identity: EnvironmentCredential: Both 'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID'," + " and at least one of 'AZURE_CLIENT_SECRET', 'AZURE_CLIENT_CERTIFICATE_PATH'" + " needs to be set. Additionally, 'AZURE_AUTHORITY_HOST' could be set" + " to override the default authority host. Currently:\n" " * 'AZURE_TENANT_ID' is NOT set\n" " * 'AZURE_CLIENT_ID' is NOT set\n" " * 'AZURE_CLIENT_SECRET' is NOT set\n" " * 'AZURE_CLIENT_CERTIFICATE_PATH' is NOT set\n" " * 'AZURE_AUTHORITY_HOST' is NOT set\n"); + log.clear(); return credential; @@ -265,13 +278,20 @@ TEST(EnvironmentCredential, ClientSecretDefaultAuthority) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(1)); - EXPECT_EQ(log[0].first, Logger::Level::Verbose); + EXPECT_EQ(log.size(), LogMsgVec::size_type(2)); + + EXPECT_EQ(log[0].first, Logger::Level::Informational); EXPECT_EQ( log[0].second, + "Identity: EnvironmentCredential gets created with ClientSecretCredential."); + + EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ( + log[1].second, "Identity: EnvironmentCredential: 'AZURE_TENANT_ID', 'AZURE_CLIENT_ID', and " "'AZURE_CLIENT_SECRET' environment variables are set, so ClientSecretCredential with " "corresponding tenantId, clientId, and clientSecret gets created."); + log.clear(); return credential; @@ -342,20 +362,26 @@ TEST(EnvironmentCredential, ClientSecretNoTenantId) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(1)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(2)); + EXPECT_EQ(log[0].first, Logger::Level::Warning); EXPECT_EQ( log[0].second, - "Identity: EnvironmentCredential was not initialized with underlying credential: Both " - "'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID', and at least one of 'AZURE_CLIENT_SECRET', " - "'AZURE_CLIENT_CERTIFICATE_PATH' needs to be set. Additionally, " - "'AZURE_AUTHORITY_HOST' could be set to override the default authority host." - " Currently:\n" + "Identity: EnvironmentCredential was not initialized with underlying credential."); + + EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ( + log[1].second, + "Identity: EnvironmentCredential: Both 'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID'," + " and at least one of 'AZURE_CLIENT_SECRET', 'AZURE_CLIENT_CERTIFICATE_PATH'" + " needs to be set. Additionally, 'AZURE_AUTHORITY_HOST' could be set" + " to override the default authority host. Currently:\n" " * 'AZURE_TENANT_ID' is NOT set\n" " * 'AZURE_CLIENT_ID' is set\n" " * 'AZURE_CLIENT_SECRET' is set\n" " * 'AZURE_CLIENT_CERTIFICATE_PATH' is NOT set\n" " * 'AZURE_AUTHORITY_HOST' is set\n"); + log.clear(); return credential; @@ -466,20 +492,26 @@ TEST(EnvironmentCredential, ClientSecretNoClientSecret) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(1)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(2)); + EXPECT_EQ(log[0].first, Logger::Level::Warning); EXPECT_EQ( log[0].second, - "Identity: EnvironmentCredential was not initialized with underlying credential: Both " - "'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID', and at least one of 'AZURE_CLIENT_SECRET', " - "'AZURE_CLIENT_CERTIFICATE_PATH' needs to be set. Additionally, " - "'AZURE_AUTHORITY_HOST' could be set to override the default authority host." - " Currently:\n" + "Identity: EnvironmentCredential was not initialized with underlying credential."); + + EXPECT_EQ(log[1].first, Logger::Level::Verbose); + EXPECT_EQ( + log[1].second, + "Identity: EnvironmentCredential: Both 'AZURE_TENANT_ID' and 'AZURE_CLIENT_ID'," + " and at least one of 'AZURE_CLIENT_SECRET', 'AZURE_CLIENT_CERTIFICATE_PATH'" + " needs to be set. Additionally, 'AZURE_AUTHORITY_HOST' could be set" + " to override the default authority host. Currently:\n" " * 'AZURE_TENANT_ID' is set\n" " * 'AZURE_CLIENT_ID' is set\n" " * 'AZURE_CLIENT_SECRET' is NOT set\n" " * 'AZURE_CLIENT_CERTIFICATE_PATH' is NOT set\n" " * 'AZURE_AUTHORITY_HOST' is set\n"); + log.clear(); return credential; From 59c78df809b0f4f04fc2e8833b51d5be7ffa3454 Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Thu, 16 Mar 2023 07:26:15 -0700 Subject: [PATCH 2/5] Even simpler --- .../src/chained_token_credential.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/sdk/identity/azure-identity/src/chained_token_credential.cpp b/sdk/identity/azure-identity/src/chained_token_credential.cpp index f6c2b3a5a1..d9df726cc4 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -71,14 +71,6 @@ AccessToken ChainedTokenCredentialImpl::GetToken( { auto const sourcesSize = m_sources.size(); - if (sourcesSize == 0) - { - Log::Write( - Logger::Level::Warning, - IdentityPrefix + credentialName - + ": Authentication did not succeed: List of sources is empty."); - } - for (size_t i = 0; i < sourcesSize; ++i) { try @@ -101,13 +93,16 @@ AccessToken ChainedTokenCredentialImpl::GetToken( if ((sourcesSize - 1) == i) // On the last credential { - Log::Write( - Logger::Level::Warning, - IdentityPrefix + credentialName - + ": Didn't succeed to get a token from any credential in the chain."); } } } + Log::Write( + Logger::Level::Warning, + IdentityPrefix + credentialName + + (sourcesSize == 0 + ? ": Authentication did not succeed: List of sources is empty." + : ": Didn't succeed to get a token from any credential in the chain.")); + throw AuthenticationException("Failed to get token from " + credentialName + '.'); } From 58e629e5b58726959373e069f6220d5c1804333b Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Thu, 16 Mar 2023 07:27:00 -0700 Subject: [PATCH 3/5] Remove refactoring artifact --- sdk/identity/azure-identity/src/chained_token_credential.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/sdk/identity/azure-identity/src/chained_token_credential.cpp b/sdk/identity/azure-identity/src/chained_token_credential.cpp index d9df726cc4..3f338efb6e 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -90,10 +90,6 @@ AccessToken ChainedTokenCredentialImpl::GetToken( Logger::Level::Verbose, IdentityPrefix + credentialName + ": Failed to get token from " + m_sources[i]->GetCredentialName() + ": " + e.what()); - - if ((sourcesSize - 1) == i) // On the last credential - { - } } } From 81c0f4f0ceb020540dd5e9416aea9ddc332bd5ab Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Thu, 16 Mar 2023 07:27:56 -0700 Subject: [PATCH 4/5] Cosmetic change --- sdk/identity/azure-identity/src/chained_token_credential.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/identity/azure-identity/src/chained_token_credential.cpp b/sdk/identity/azure-identity/src/chained_token_credential.cpp index 3f338efb6e..cf9e16d1cb 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -70,7 +70,6 @@ AccessToken ChainedTokenCredentialImpl::GetToken( Context const& context) const { auto const sourcesSize = m_sources.size(); - for (size_t i = 0; i < sourcesSize; ++i) { try From a516c9afa4c5ddf3762c297fb3433d7136a2f26f Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Thu, 16 Mar 2023 08:22:54 -0700 Subject: [PATCH 5/5] foreach --- .../azure-identity/src/chained_token_credential.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sdk/identity/azure-identity/src/chained_token_credential.cpp b/sdk/identity/azure-identity/src/chained_token_credential.cpp index cf9e16d1cb..e30774e358 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -69,17 +69,16 @@ AccessToken ChainedTokenCredentialImpl::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - auto const sourcesSize = m_sources.size(); - for (size_t i = 0; i < sourcesSize; ++i) + for (auto const& source : m_sources) { try { - auto token = m_sources[i]->GetToken(tokenRequestContext, context); + auto token = source->GetToken(tokenRequestContext, context); Log::Write( Logger::Level::Informational, IdentityPrefix + credentialName + ": Successfully got token from " - + m_sources[i]->GetCredentialName() + '.'); + + source->GetCredentialName() + '.'); return token; } @@ -88,14 +87,14 @@ AccessToken ChainedTokenCredentialImpl::GetToken( Log::Write( Logger::Level::Verbose, IdentityPrefix + credentialName + ": Failed to get token from " - + m_sources[i]->GetCredentialName() + ": " + e.what()); + + source->GetCredentialName() + ": " + e.what()); } } Log::Write( Logger::Level::Warning, IdentityPrefix + credentialName - + (sourcesSize == 0 + + (m_sources.empty() ? ": Authentication did not succeed: List of sources is empty." : ": Didn't succeed to get a token from any credential in the chain."));