PR follow-ups (#4201)

Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>
Co-authored-by: George Arama <50641385+gearama@users.noreply.github.com>

sdk/core/azure-core/CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ### Breaking Changes
 
+- Bearer token authentication will not work for endpoint URL protocol schemes other than `"https"`. This ensures token security and is consistent with the Azure SDKs for other languages.
 - Removed `noexcept` specification from `Azure::DateTime::clock::now()`.
 
 ### Bugs Fixed

sdk/identity/azure-identity/CHANGELOG.md

@@ -12,9 +12,10 @@
 
 ### Bugs Fixed
 
+- Changed token cache mode to per-credential-instance. In order to get benefits from token caching, share the same credential between multiple client instances.
+
 ### Other Changes
 
-- Changed token cache mode to per-credential-instance. In order to get benefits from token caching, share the same credential between multiple client instances.
 - Added token cache support to all credentials.
 
 ## 1.4.0-beta.2 (2022-11-08)

sdk/identity/azure-identity/inc/azure/identity/detail/token_cache.hpp

@@ -45,6 +45,19 @@ namespace Azure { namespace Identity { namespace _detail {
       std::shared_timed_mutex ElementMutex;
     };
 
+    // The current cache Key, std::string Scopes, may later evolve to a struct that contains more
+    // fields. All that depends on the fields in the TokenRequestContext that are used as
+    // characteristics that go into the network request that gets the token.
+    // If tomorrow we add Multi-Tenant Authentication, and the TenantID stops being an immutable
+    // characteristic of a credential instance, but instead becomes variable depending on the fields
+    // of the TokenRequestContext that are taken into consideration as network request for the token
+    // is being sent, it should go into what will form the new CacheKey struct.
+    // i.e. we want all the variable inputs for obtaining a token to be a part of the key, because
+    // we want to have the same kind of result. There should be no "hidden variables".
+    // Otherwise, the cache will stop functioning properly, because the value you'd get from cache
+    // for a given key will fail to authenticate, but if the cache ends up calling the getNewToken
+    // callback, you'll authenticate successfully (however the other caller who need to get the
+    // token for slightly different context will not be as lucky).
     mutable std::map<std::string, std::shared_ptr<CacheValue>> m_cache;
     mutable std::shared_timed_mutex m_cacheMutex;
 

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

@@ -19,6 +19,7 @@ decltype(ClientCredentialCore::AadGlobalAuthority) ClientCredentialCore::AadGlob
 ClientCredentialCore::ClientCredentialCore(std::string tenantId, std::string const& authorityHost)
     : m_authorityHost(Url(authorityHost)), m_tenantId(std::move(tenantId))
 {
+  // ADFS is the Active Directory Federation Service, a tenant ID that is used in Azure Stack.
   m_isAdfs = m_tenantId == "adfs";
 }
 

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

@@ -3,6 +3,8 @@
 
 #include "azure/identity/detail/token_cache.hpp"
 
+#include "azure/identity/client_secret_credential.hpp"
+
 #include <mutex>
 
 #include <gtest/gtest.h>
@@ -429,3 +431,113 @@ TEST(TokenCache, MultithreadedAccess)
     EXPECT_EQ(token3.Token, "T4");
   }
 }
+
+using Azure::Core::Context;
+using Azure::Core::Http::HttpTransport;
+using Azure::Core::Http::RawResponse;
+using Azure::Core::Http::Request;
+
+namespace {
+class TestTransport final : public HttpTransport {
+  int m_attemptNumber = 0;
+  std::vector<uint8_t> m_responseBuf;
+
+public:
+  // Returns token response with 3600 seconds expiration (1 hour), and the value of the
+  // client_secret parameter from the body + attempt number as token value.
+  std::unique_ptr<RawResponse> Send(Request& request, Context const&) override
+  {
+    using Azure::Core::Http::HttpStatusCode;
+    using Azure::Core::IO::BodyStream;
+    using Azure::Core::IO::MemoryBodyStream;
+
+    ++m_attemptNumber;
+
+    std::string clientSecret;
+    {
+      std::string const ClientSecretStart = "client_secret=";
+
+      auto const reqBodyVec = request.GetBodyStream()->ReadToEnd();
+      auto const reqBodyStr = std::string(reqBodyVec.cbegin(), reqBodyVec.cend());
+
+      auto clientSecretStartPos = reqBodyStr.find(ClientSecretStart);
+      if (clientSecretStartPos != std::string::npos)
+      {
+        clientSecretStartPos += ClientSecretStart.size();
+        auto const clientSecretEndPos = reqBodyStr.find('&', clientSecretStartPos);
+
+        clientSecret = (clientSecretEndPos == std::string::npos)
+            ? reqBodyStr.substr(clientSecretStartPos)
+            : reqBodyStr.substr(clientSecretStartPos, clientSecretEndPos - clientSecretStartPos);
+      }
+    }
+
+    auto const respBodyStr = std::string("{ \"access_token\" : \"") + clientSecret
+        + std::to_string(m_attemptNumber) + "\", \"expires_in\" : 3600 }";
+
+    m_responseBuf.assign(respBodyStr.cbegin(), respBodyStr.cend());
+
+    auto resp = std::make_unique<RawResponse>(1, 1, HttpStatusCode::Ok, "OK");
+    resp->SetBodyStream(std::make_unique<MemoryBodyStream>(m_responseBuf));
+    return resp;
+  }
+};
+} // namespace
+
+TEST(TokenCache, PerCredInstance)
+{
+  using Azure::Core::Credentials::TokenCredentialOptions;
+  using Azure::Core::Credentials::TokenRequestContext;
+  using Azure::Identity::ClientSecretCredential;
+
+  TokenRequestContext getCached;
+  getCached.Scopes = {"https://vault.azure.net/.default"};
+  getCached.MinimumExpiration = 1s;
+
+  TokenCredentialOptions credOptions;
+  credOptions.Transport.Transport = std::make_shared<TestTransport>();
+
+  ClientSecretCredential credA("TenantId", "ClientId", "SecretA", credOptions);
+  ClientSecretCredential credB("TenantId", "ClientId", "SecretB", credOptions);
+
+  {
+    auto const tokenA1 = credA.GetToken(getCached, {}); // Should populate
+    EXPECT_EQ(tokenA1.Token, "SecretA1");
+  }
+
+  {
+    auto const tokenA2 = credA.GetToken(getCached, {}); // Should get previously populated value
+    EXPECT_EQ(tokenA2.Token, "SecretA1");
+  }
+
+  {
+    auto const tokenB = credB.GetToken(getCached, {});
+    EXPECT_EQ(
+        tokenB.Token,
+        "SecretB2"); // if token cache was shared between instances, the value would be "SecretA1"
+  }
+
+  {
+    auto const tokenA3 = credA.GetToken(getCached, {}); // Should still get the cached value
+    EXPECT_EQ(tokenA3.Token, "SecretA1");
+  }
+
+  auto getNew = getCached;
+  getNew.MinimumExpiration += 3600s;
+
+  {
+    auto const tokenA4 = credA.GetToken(getNew, {}); // Should get the new value
+    EXPECT_EQ(tokenA4.Token, "SecretA3");
+  }
+
+  {
+    auto const tokenA5 = credA.GetToken(getNew, {}); // Should get the new value
+    EXPECT_EQ(tokenA5.Token, "SecretA4");
+  }
+
+  {
+    auto const tokenA6
+        = credA.GetToken(getCached, {}); // Should get the cached, recently refreshed value
+    EXPECT_EQ(tokenA6.Token, "SecretA4");
+  }
+}