Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetCredentialName() #4428

Merged
merged 17 commits into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/core/azure-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Added the ability to ignore invalid certificate common name for TLS connections in WinHTTP transport.
- Added `DisableTlsCertificateValidation` in `TransportOptions`.
- Added `TokenCredential::GetCredentialName()` to be utilized in diagnostic messages. If you have any custom implementations of `TokenCredential`, it is recommended to pass the name of your credential to `TokenCredential` constructor. The old parameterless constructor is deprecated.

### Breaking Changes

Expand Down
1 change: 1 addition & 0 deletions sdk/core/azure-core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ set(
src/azure_assert.cpp
src/base64.cpp
src/context.cpp

antkmsft marked this conversation as resolved.
Show resolved Hide resolved
src/cryptography/md5.cpp
src/cryptography/sha_hash.cpp
src/datetime.cpp
Expand Down
28 changes: 26 additions & 2 deletions sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ namespace Azure { namespace Core { namespace Credentials {
* @brief A base type of credential that uses Azure::Core::AccessToken to authenticate requests.
*/
class TokenCredential {
private:
std::string m_credentialName;

public:
/**
* @brief Gets an authentication token.
Expand All @@ -73,7 +76,14 @@ namespace Azure { namespace Core { namespace Credentials {
*/
virtual AccessToken GetToken(
TokenRequestContext const& tokenRequestContext,
Context const& context) const = 0;
Context const& context) const
= 0;

/**
* @brief Gets the name of the credential.
*
*/
std::string const& GetCredentialName() const { return m_credentialName; }

/**
* @brief Destructs `%TokenCredential`.
Expand All @@ -82,11 +92,25 @@ namespace Azure { namespace Core { namespace Credentials {
virtual ~TokenCredential() = default;

protected:
/**
* @brief Constructs an instance of `%TokenCredential`.
*
* @param credentialName Name of the credential for diagnostic messages.
*/
TokenCredential(std::string credentialName)
: m_credentialName(std::move(credentialName.empty() ? "Custom Credential" : credentialName))
{
}

/**
* @brief Constructs a default instance of `%TokenCredential`.
*
* @deprecated Use the constructor with parameter.
*/
TokenCredential() {}
[[deprecated("Use the construtor with parameter.")]] TokenCredential()
: TokenCredential(std::string{})
{
}

private:
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class TestTokenCredential final : public Azure::Core::Credentials::TokenCredenti
public:
explicit TestTokenCredential(
std::shared_ptr<Azure::Core::Credentials::AccessToken const> accessToken)
: m_accessToken(accessToken)
: TokenCredential("TestTokenCredential"), m_accessToken(accessToken)
{
}

Expand Down
2 changes: 2 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

### Other Changes

- Improved diagnostics to utilize `Azure::Core::Credentials::TokenCredential::GetCredentialName()`.

## 1.5.0-beta.1 (2023-03-07)

### Features Added
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,9 @@ namespace Azure { namespace Identity {
Core::Context const& context) const override;

private:
explicit ChainedTokenCredential(
Sources sources,
std::string const& enclosingCredential,
std::vector<std::string> sourcesFriendlyNames);
explicit ChainedTokenCredential(Sources sources, std::string const& enclosingCredential);
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

Sources m_sources;
std::vector<std::string> m_sourcesFriendlyNames;
std::string m_logPrefix;
};

Expand Down
24 changes: 16 additions & 8 deletions sdk/identity/azure-identity/src/azure_cli_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,12 @@ using Azure::Identity::_detail::TokenCache;
using Azure::Identity::_detail::TokenCredentialImpl;

namespace {
std::string const MsgPrefix = "Identity: AzureCliCredential";
constexpr auto IdentityPrefix = "Identity: ";

void ThrowIfNotSafeCmdLineInput(std::string const& input, std::string const& description)
void ThrowIfNotSafeCmdLineInput(
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
std::string const& credentialName,
std::string const& input,
std::string const& description)
{
for (auto const c : input)
{
Expand All @@ -71,7 +74,8 @@ void ThrowIfNotSafeCmdLineInput(std::string const& input, std::string const& des
if (!std::isalnum(c, std::locale::classic()))
{
throw AuthenticationException(
MsgPrefix + ": Unsafe command line input found in " + description + ": " + input);
IdentityPrefix + credentialName + ": Unsafe command line input found in "
+ description + ": " + input);
}
}
}
Expand All @@ -82,17 +86,20 @@ AzureCliCredential::AzureCliCredential(
std::string tenantId,
DateTime::duration cliProcessTimeout,
Core::Credentials::TokenCredentialOptions const& options)
: m_tenantId(std::move(tenantId)), m_cliProcessTimeout(std::move(cliProcessTimeout))
: TokenCredential("AzureCliCredential"), m_tenantId(std::move(tenantId)),
m_cliProcessTimeout(std::move(cliProcessTimeout))
{
static_cast<void>(options);
ThrowIfNotSafeCmdLineInput(m_tenantId, "TenantID");

auto const credentialName = GetCredentialName();
ThrowIfNotSafeCmdLineInput(credentialName, m_tenantId, "TenantID");

auto const logLevel = Logger::Level::Informational;
if (Log::ShouldWrite(logLevel))
{
Log::Write(
logLevel,
MsgPrefix
IdentityPrefix + credentialName
+ " created.\n"
"Successful creation does not guarantee further successful token retrieval.");
}
Expand All @@ -114,7 +121,7 @@ AzureCliCredential::AzureCliCredential(TokenCredentialOptions const& options)
std::string AzureCliCredential::GetAzCommand(std::string const& scopes, std::string const& tenantId)
const
{
ThrowIfNotSafeCmdLineInput(scopes, "Scopes");
ThrowIfNotSafeCmdLineInput(GetCredentialName(), scopes, "Scopes");
std::string command = "az account get-access-token --output json --scope \"" + scopes + "\"";

if (!tenantId.empty())
Expand Down Expand Up @@ -161,7 +168,8 @@ AccessToken AzureCliCredential::GetToken(
}
catch (std::exception const& e)
{
auto const errorMsg = MsgPrefix + " didn't get the token: \"" + e.what() + '\"';
auto const errorMsg
= IdentityPrefix + GetCredentialName() + " didn't get the token: \"" + e.what() + '\"';

auto const logLevel = Logger::Level::Warning;
if (Log::ShouldWrite(logLevel))
Expand Down
70 changes: 30 additions & 40 deletions sdk/identity/azure-identity/src/chained_token_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include "azure/identity/chained_token_credential.hpp"

#include "azure/core/internal/diagnostics/log.hpp"
#include <azure/core/azure_assert.hpp>

#include <utility>

Expand All @@ -15,63 +14,53 @@ using Azure::Core::Diagnostics::Logger;
using Azure::Core::Diagnostics::_internal::Log;

namespace {
std::string const IdentityPrefix = "Identity: ";
}
constexpr auto IdentityPrefix = "Identity: ";
} // namespace

ChainedTokenCredential::ChainedTokenCredential(ChainedTokenCredential::Sources sources)
: ChainedTokenCredential(sources, {}, {})
: ChainedTokenCredential(sources, {})
{
}

ChainedTokenCredential::ChainedTokenCredential(
ChainedTokenCredential::Sources sources,
std::string const& enclosingCredential,
std::vector<std::string> sourcesFriendlyNames)
: m_sources(std::move(sources)), m_sourcesFriendlyNames(std::move(sourcesFriendlyNames))
std::string const& enclosingCredential)
: TokenCredential("ChainedTokenCredential"), m_sources(std::move(sources))
{
// LCOV_EXCL_START
AZURE_ASSERT(m_sourcesFriendlyNames.empty() || m_sourcesFriendlyNames.size() == m_sources.size());
// LCOV_EXCL_STOP
auto const credentialName = GetCredentialName();
m_logPrefix = IdentityPrefix
+ (enclosingCredential.empty() ? credentialName
: (enclosingCredential + " -> " + credentialName))
+ ": ";

auto const logLevel = m_sources.empty() ? Logger::Level::Warning : Logger::Level::Informational;
if (Log::ShouldWrite(logLevel))
{
std::string credentialsList;
if (!m_sourcesFriendlyNames.empty())
std::string credSourceDetails = " with EMPTY chain of credentials.";
if (!m_sources.empty())
{
auto const sourceDescriptionsSize = m_sourcesFriendlyNames.size();
for (size_t i = 0; i < (sourceDescriptionsSize - 1); ++i)
credSourceDetails = " with the following credentials: ";

auto const sourcesSize = m_sources.size();
for (size_t i = 0; i < (sourcesSize - 1); ++i)
{
credentialsList += m_sourcesFriendlyNames[i] + ", ";
if (i != 0)
{
credSourceDetails += ", ";
}

credSourceDetails += m_sources[i]->GetCredentialName();
}

credentialsList += m_sourcesFriendlyNames.back();
credSourceDetails += '.';
}

Log::Write(
logLevel,
IdentityPrefix
+ (enclosingCredential.empty()
? "ChainedTokenCredential: Created"
: (enclosingCredential + ": Created ChainedTokenCredential"))
+ " with "
+ (m_sourcesFriendlyNames.empty()
? (std::to_string(m_sources.size()) + " credentials.")
: (std::string("the following credentials: ") + credentialsList + '.')));
}

m_logPrefix = IdentityPrefix
+ (enclosingCredential.empty() ? "ChainedTokenCredential"
: (enclosingCredential + " -> ChainedTokenCredential"))
+ ": ";

if (m_sourcesFriendlyNames.empty())
{
auto const sourcesSize = m_sources.size();
for (size_t i = 1; i <= sourcesSize; ++i)
{
m_sourcesFriendlyNames.push_back(std::string("credential #") + std::to_string(i));
}
+ (enclosingCredential.empty() ? (credentialName + ": Created")
: (enclosingCredential + ": Created " + credentialName))
+ credSourceDetails);
}
}

Expand Down Expand Up @@ -106,7 +95,8 @@ AccessToken ChainedTokenCredential::GetToken(
{
Log::Write(
logLevel,
m_logPrefix + "Successfully got token from " + m_sourcesFriendlyNames[i] + '.');
m_logPrefix + "Successfully got token from " + m_sources[i]->GetCredentialName()
+ '.');
}
}

Expand All @@ -120,7 +110,7 @@ AccessToken ChainedTokenCredential::GetToken(
{
Log::Write(
logLevel,
m_logPrefix + "Failed to get token from " + m_sourcesFriendlyNames[i] + ": "
m_logPrefix + "Failed to get token from " + m_sources[i]->GetCredentialName() + ": "
+ e.what());
}
}
Expand All @@ -139,5 +129,5 @@ AccessToken ChainedTokenCredential::GetToken(
}
}

throw AuthenticationException("Failed to get token from ChainedTokenCredential.");
throw AuthenticationException("Failed to get token from " + GetCredentialName() + '.');
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ ClientCertificateCredential::ClientCertificateCredential(
std::string const& clientCertificatePath,
std::string const& authorityHost,
TokenCredentialOptions const& options)
: m_clientCredentialCore(tenantId, authorityHost),
: TokenCredential("ClientCertificateCredential"),
m_clientCredentialCore(tenantId, authorityHost),
m_tokenCredentialImpl(std::make_unique<TokenCredentialImpl>(options)),
m_requestBody(
std::string(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ ClientSecretCredential::ClientSecretCredential(
std::string const& clientSecret,
std::string const& authorityHost,
TokenCredentialOptions const& options)
: m_clientCredentialCore(tenantId, authorityHost),
: TokenCredential("ClientSecretCredential"), m_clientCredentialCore(tenantId, authorityHost),
m_tokenCredentialImpl(std::make_unique<TokenCredentialImpl>(options)),
m_requestBody(
std::string("grant_type=client_credentials&client_id=") + Url::Encode(clientId)
Expand Down
35 changes: 20 additions & 15 deletions sdk/identity/azure-identity/src/default_azure_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,31 @@ using Azure::Core::Diagnostics::Logger;
using Azure::Core::Diagnostics::_internal::Log;

namespace {
std::string const IdentityPrefix = "Identity: ";
}
constexpr auto IdentityPrefix = "Identity: ";
} // namespace

DefaultAzureCredential::DefaultAzureCredential(TokenCredentialOptions const& options)
: TokenCredential("DefaultAzureCredential")
{
auto const credentialName = GetCredentialName();

// 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,
IdentityPrefix
+ "Creating DefaultAzureCredential which combines mutiple parameterless credentials "
"into a single one (by using ChainedTokenCredential)."
"\nDefaultAzureCredential is only recommended for the early stages of development, "
std::string(IdentityPrefix) + "Creating " + credentialName
+ " which combines mutiple parameterless credentials "
"into a single one (by using ChainedTokenCredential).\n"
+ credentialName
+ " 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, DefaultAzureCredential needs to be replaced with the credential that "
"\nOnce the developer focuses on the Credentials and Authentication aspects "
"of their application, "
+ credentialName
+ " needs to be replaced with the credential that "
"is the better fit for the application.");
}

Expand All @@ -47,9 +53,7 @@ DefaultAzureCredential::DefaultAzureCredential(TokenCredentialOptions const& opt
// Using the ChainedTokenCredential's private constructor for more detailed log messages.
m_credentials.reset(new ChainedTokenCredential(
ChainedTokenCredential::Sources{envCred, azCliCred, managedIdentityCred},
"DefaultAzureCredential", // extra args for the ChainedTokenCredential's private constructor.
std::vector<std::string>{
"EnvironmentCredential", "AzureCliCredential", "ManagedIdentityCredential"}));
credentialName)); // extra arg for the ChainedTokenCredential's private constructor.
}

DefaultAzureCredential::~DefaultAzureCredential() = default;
Expand All @@ -64,9 +68,10 @@ AccessToken DefaultAzureCredential::GetToken(
}
catch (AuthenticationException const&)
{
throw AuthenticationException("Failed to get token from DefaultAzureCredential."
"\nSee Azure::Core::Diagnostics::Logger for details "
"(https://github.com/Azure/azure-sdk-for-cpp/tree/main/sdk/"
"identity/azure-identity#troubleshooting).");
throw AuthenticationException(
"Failed to get token from " + GetCredentialName()
+ ".\nSee Azure::Core::Diagnostics::Logger for details "
"(https://github.com/Azure/azure-sdk-for-cpp/tree/main/sdk/"
"identity/azure-identity#troubleshooting).");
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading