Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor (#4447)

* Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor

* Clang warning fix

---------

Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>

sdk/identity/azure-identity/CMakeLists.txt

@@ -62,6 +62,7 @@ set(
 
 set(
   AZURE_IDENTITY_SOURCE
+    src/private/chained_token_credential_impl.hpp
     src/private/managed_identity_source.hpp
     src/private/package_version.hpp
     src/private/token_credential_impl.hpp

sdk/identity/azure-identity/README.md

@@ -58,7 +58,7 @@ The `DefaultAzureCredential` attempts to authenticate via the following mechanis
 1. **Azure CLI** - If the developer has authenticated an account via the Azure CLI `az login` command, the `DefaultAzureCredential` will authenticate with that account.
 1. **Managed Identity** - If the application is deployed to an Azure host with Managed Identity enabled, the `DefaultAzureCredential` will authenticate with that account.
 
-`DefaultAzureCredential` uses [`ChainedTokenCredential`](#chained-token-credential) that consists of a chain of `EnvironmentCredential`, `AzureCliCredential`, and `ManagedIdentityCredential`. Implementation, including the order in which credentials are applied is documented, but it may change from release to release.
+Even though the credentials being used and their order is documented, it may change from release to release.
 
 `DefaultAzureCredential` intends to provide a credential that "just works out of the box and without requiring any information", if only the environment is set up sufficiently for the credential to work.
 Therefore, it could be simple to use, but since it uses a chain of credentials, it could be a bit complicated to diagnose if the environment setup is not sufficient.

sdk/identity/azure-identity/inc/azure/identity/chained_token_credential.hpp

@@ -15,7 +15,9 @@
 #include <vector>
 
 namespace Azure { namespace Identity {
-  class DefaultAzureCredential;
+  namespace _detail {
+    class ChainedTokenCredentialImpl;
+  }
 
   /**
    * @brief Chained Token Credential provides a token credential implementation which chains
@@ -24,10 +26,6 @@ namespace Azure { namespace Identity {
    *
    */
   class ChainedTokenCredential final : public Core::Credentials::TokenCredential {
-    // Friend declaration is needed for DefaultAzureCredential to access ChainedTokenCredential's
-    // private constructor built to be used specifically by it.
-    friend class DefaultAzureCredential;
-
   public:
     /**
      * @brief A container type to store the ordered chain of credentials.
@@ -62,10 +60,7 @@ namespace Azure { namespace Identity {
         Core::Context const& context) const override;
 
   private:
-    explicit ChainedTokenCredential(Sources sources, std::string const& enclosingCredential);
-
-    Sources m_sources;
-    std::string m_logPrefix;
+    std::unique_ptr<_detail::ChainedTokenCredentialImpl> m_impl;
   };
 
 }} // namespace Azure::Identity

sdk/identity/azure-identity/inc/azure/identity/default_azure_credential.hpp

@@ -8,13 +8,15 @@
 
 #pragma once
 
+#include <azure/core/credentials/credentials.hpp>
 #include <azure/core/credentials/token_credential_options.hpp>
 
-#include <azure/identity/chained_token_credential.hpp>
-
 #include <memory>
 
 namespace Azure { namespace Identity {
+  namespace _detail {
+    class ChainedTokenCredentialImpl;
+  }
 
   /**
    * @brief Default Azure Credential combines multiple credentials that depend on the setup
@@ -22,7 +24,7 @@ namespace Azure { namespace Identity {
    * sufficiently for at least one of such credentials to work, `DefaultAzureCredential` will work
    * as well.
    *
-   * @details This credential is using the #ChainedTokenCredential of 3 credentials in the order:
+   * @details This credential is using several credentials in the following order:
    * #EnvironmentCredential, #AzureCliCredential, and #ManagedIdentityCredential. Even though the
    * credentials being used and their order is documented, it may be changed in the future versions
    * of the SDK, potentially bringing breaking changes in its behavior.
@@ -68,7 +70,7 @@ namespace Azure { namespace Identity {
         Core::Context const& context) const override;
 
   private:
-    std::shared_ptr<ChainedTokenCredential> m_credentials;
+    std::unique_ptr<_detail::ChainedTokenCredentialImpl> m_impl;
   };
 
 }} // namespace Azure::Identity

sdk/identity/azure-identity/src/chained_token_credential.cpp

@@ -2,36 +2,42 @@
 // SPDX-License-Identifier: MIT
 
 #include "azure/identity/chained_token_credential.hpp"
-
 #include "azure/core/internal/diagnostics/log.hpp"
+#include "private/chained_token_credential_impl.hpp"
 
 #include <utility>
 
 using namespace Azure::Identity;
+using namespace Azure::Identity::_detail;
 using namespace Azure::Core::Credentials;
 using Azure::Core::Context;
 using Azure::Core::Diagnostics::Logger;
 using Azure::Core::Diagnostics::_internal::Log;
 
+ChainedTokenCredential::ChainedTokenCredential(ChainedTokenCredential::Sources sources)
+    : TokenCredential("ChainedTokenCredential"),
+      m_impl(std::make_unique<ChainedTokenCredentialImpl>(GetCredentialName(), std::move(sources)))
+{
+}
+
+ChainedTokenCredential::~ChainedTokenCredential() = default;
+
+AccessToken ChainedTokenCredential::GetToken(
+    TokenRequestContext const& tokenRequestContext,
+    Context const& context) const
+{
+  return m_impl->GetToken(GetCredentialName(), tokenRequestContext, context);
+}
+
 namespace {
 constexpr auto IdentityPrefix = "Identity: ";
 } // namespace
 
-ChainedTokenCredential::ChainedTokenCredential(ChainedTokenCredential::Sources sources)
-    : ChainedTokenCredential(sources, {})
+ChainedTokenCredentialImpl::ChainedTokenCredentialImpl(
+    std::string const& credentialName,
+    ChainedTokenCredential::Sources&& sources)
+    : m_sources(std::move(sources))
 {
-}
-
-ChainedTokenCredential::ChainedTokenCredential(
-    ChainedTokenCredential::Sources sources,
-    std::string const& enclosingCredential)
-    : TokenCredential("ChainedTokenCredential"), m_sources(std::move(sources))
-{
-  m_logPrefix = IdentityPrefix
-      + (enclosingCredential.empty() ? GetCredentialName()
-                                     : (enclosingCredential + " -> " + GetCredentialName()))
-      + ": ";
-
   auto const logLevel = m_sources.empty() ? Logger::Level::Warning : Logger::Level::Informational;
   if (Log::ShouldWrite(logLevel))
   {
@@ -54,19 +60,12 @@ ChainedTokenCredential::ChainedTokenCredential(
       credSourceDetails += '.';
     }
 
-    Log::Write(
-        logLevel,
-        IdentityPrefix
-            + (enclosingCredential.empty()
-                   ? (GetCredentialName() + ": Created")
-                   : (enclosingCredential + ": Created " + GetCredentialName()))
-            + credSourceDetails);
+    Log::Write(logLevel, IdentityPrefix + credentialName + ": Created" + credSourceDetails);
   }
 }
 
-ChainedTokenCredential::~ChainedTokenCredential() = default;
-
-AccessToken ChainedTokenCredential::GetToken(
+AccessToken ChainedTokenCredentialImpl::GetToken(
+    std::string const& credentialName,
     TokenRequestContext const& tokenRequestContext,
     Context const& context) const
 {
@@ -78,7 +77,9 @@ AccessToken ChainedTokenCredential::GetToken(
     if (Log::ShouldWrite(logLevel))
     {
       Log::Write(
-          logLevel, m_logPrefix + "Authentication did not succeed: List of sources is empty.");
+          logLevel,
+          IdentityPrefix + credentialName
+              + ": Authentication did not succeed: List of sources is empty.");
     }
   }
   else
@@ -95,8 +96,8 @@ AccessToken ChainedTokenCredential::GetToken(
           {
             Log::Write(
                 logLevel,
-                m_logPrefix + "Successfully got token from " + m_sources[i]->GetCredentialName()
-                    + '.');
+                IdentityPrefix + credentialName + ": Successfully got token from "
+                    + m_sources[i]->GetCredentialName() + '.');
           }
         }
 
@@ -110,8 +111,8 @@ AccessToken ChainedTokenCredential::GetToken(
           {
             Log::Write(
                 logLevel,
-                m_logPrefix + "Failed to get token from " + m_sources[i]->GetCredentialName() + ": "
-                    + e.what());
+                IdentityPrefix + credentialName + ": Failed to get token from "
+                    + m_sources[i]->GetCredentialName() + ": " + e.what());
           }
         }
 
@@ -122,12 +123,13 @@ AccessToken ChainedTokenCredential::GetToken(
           {
             Log::Write(
                 logLevel,
-                m_logPrefix + "Didn't succeed to get a token from any credential in the chain.");
+                IdentityPrefix + credentialName
+                    + ": Didn't succeed to get a token from any credential in the chain.");
           }
         }
       }
     }
   }
 
-  throw AuthenticationException("Failed to get token from " + GetCredentialName() + '.');
+  throw AuthenticationException("Failed to get token from " + credentialName + '.');
 }

sdk/identity/azure-identity/src/default_azure_credential.cpp

@@ -6,6 +6,7 @@
 #include "azure/identity/azure_cli_credential.hpp"
 #include "azure/identity/environment_credential.hpp"
 #include "azure/identity/managed_identity_credential.hpp"
+#include "private/chained_token_credential_impl.hpp"
 
 #include "azure/core/internal/diagnostics/log.hpp"
 
@@ -31,8 +32,7 @@ DefaultAzureCredential::DefaultAzureCredential(TokenCredentialOptions const& opt
     Log::Write(
         logLevel,
         std::string(IdentityPrefix) + "Creating " + GetCredentialName()
-            + " which combines mutiple parameterless credentials "
-              "into a single one (by using ChainedTokenCredential).\n"
+            + " 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."
@@ -48,10 +48,9 @@ DefaultAzureCredential::DefaultAzureCredential(TokenCredentialOptions const& opt
   auto const azCliCred = std::make_shared<AzureCliCredential>(options);
   auto const managedIdentityCred = std::make_shared<ManagedIdentityCredential>(options);
 
-  // Using the ChainedTokenCredential's private constructor for more detailed log messages.
-  m_credentials.reset(new ChainedTokenCredential(
-      ChainedTokenCredential::Sources{envCred, azCliCred, managedIdentityCred},
-      GetCredentialName())); // extra arg for the ChainedTokenCredential's private constructor.
+  m_impl = std::make_unique<_detail::ChainedTokenCredentialImpl>(
+      GetCredentialName(),
+      ChainedTokenCredential::Sources{envCred, azCliCred, managedIdentityCred});
 }
 
 DefaultAzureCredential::~DefaultAzureCredential() = default;
@@ -62,7 +61,7 @@ AccessToken DefaultAzureCredential::GetToken(
 {
   try
   {
-    return m_credentials->GetToken(tokenRequestContext, context);
+    return m_impl->GetToken(GetCredentialName(), tokenRequestContext, context);
   }
   catch (AuthenticationException const&)
   {

sdk/identity/azure-identity/src/private/chained_token_credential_impl.hpp

@@ -0,0 +1,25 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+#pragma once
+
+#include "azure/identity/chained_token_credential.hpp"
+
+namespace Azure { namespace Identity { namespace _detail {
+
+  class ChainedTokenCredentialImpl final {
+  public:
+    ChainedTokenCredentialImpl(
+        std::string const& credentialName,
+        ChainedTokenCredential::Sources&& sources);
+
+    Core::Credentials::AccessToken GetToken(
+        std::string const& credentialName,
+        Core::Credentials::TokenRequestContext const& tokenRequestContext,
+        Core::Context const& context) const;
+
+  private:
+    ChainedTokenCredential::Sources m_sources;
+  };
+
+}}} // namespace Azure::Identity::_detail

sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp

@@ -74,9 +74,8 @@ TEST(DefaultAzureCredential, LogMessages)
         EXPECT_EQ(log[0].first, Logger::Level::Verbose);
         EXPECT_EQ(
             log[0].second,
-            "Identity: Creating DefaultAzureCredential which combines mutiple parameterless "
-            "credentials "
-            "into a single one (by using ChainedTokenCredential)."
+            "Identity: Creating DefaultAzureCredential which combines "
+            "mutiple parameterless credentials into a single one."
             "\nDefaultAzureCredential 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 "
@@ -131,8 +130,7 @@ TEST(DefaultAzureCredential, LogMessages)
         EXPECT_EQ(log[8].first, Logger::Level::Informational);
         EXPECT_EQ(
             log[8].second,
-            "Identity: DefaultAzureCredential: Created ChainedTokenCredential "
-            "with the following credentials: "
+            "Identity: DefaultAzureCredential: Created with the following credentials: "
             "EnvironmentCredential, AzureCliCredential, ManagedIdentityCredential.");
 
         log.clear();
@@ -149,8 +147,7 @@ TEST(DefaultAzureCredential, LogMessages)
   EXPECT_EQ(log[3].first, Logger::Level::Informational);
   EXPECT_EQ(
       log[3].second,
-      "Identity: DefaultAzureCredential -> ChainedTokenCredential: "
-      "Successfully got token from EnvironmentCredential.");
+      "Identity: DefaultAzureCredential: Successfully got token from EnvironmentCredential.");
 
   Logger::SetListener(nullptr);
 }