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..e30774e358 100644 --- a/sdk/identity/azure-identity/src/chained_token_credential.cpp +++ b/sdk/identity/azure-identity/src/chained_token_credential.cpp @@ -69,67 +69,34 @@ AccessToken ChainedTokenCredentialImpl::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - auto const sourcesSize = m_sources.size(); - - if (sourcesSize == 0) + for (auto const& source : m_sources) { - const auto logLevel = Logger::Level::Warning; - if (Log::ShouldWrite(logLevel)) + try { + auto token = source->GetToken(tokenRequestContext, context); + Log::Write( - logLevel, - IdentityPrefix + credentialName - + ": Authentication did not succeed: List of sources is empty."); + Logger::Level::Informational, + IdentityPrefix + credentialName + ": Successfully got token from " + + source->GetCredentialName() + '.'); + + return token; } - } - else - { - for (size_t i = 0; i < sourcesSize; ++i) + catch (AuthenticationException const& e) { - try - { - 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() + '.'); - } - } - - 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()); - } - } - - 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."); - } - } - } + Log::Write( + Logger::Level::Verbose, + IdentityPrefix + credentialName + ": Failed to get token from " + + source->GetCredentialName() + ": " + e.what()); } } + Log::Write( + Logger::Level::Warning, + IdentityPrefix + credentialName + + (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.")); + throw AuthenticationException("Failed to get token from " + credentialName + '.'); } 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;