Repository: Azure/azure-sdk-for-cpp (cpp)

Issue:
`BearerTokenAuthenticationPolicy` can be more efficient by utilizing shared/unique token locking
We can shared_lock when checking if the token has not expired, and then unique_lock when we know that we need to update the token, but just before updating (which sends network request), check for the second time in case another thread has just updated it.

include `<shared_nutex>` nut use shared_timed_mutex and not shared_mutex, because the first is C++14, while the other is C++17.

Pull request:
`BearerTokenAuthenticationPolicy` to use shared_lock in case no refresh is needed
Closes #4756.

--
UWP CI is currently blocked until https://github.com/microsoft/vcpkg/issues/35279 is fixed / https://github.com/microsoft/vcpkg/pull/35116 is merged (#5181 would unblock)

Patch:
diff --git a/.vscode/cspell.json b/.vscode/cspell.json
index 9f399c9e7..48993be3b 100644
--- a/.vscode/cspell.json
+++ b/.vscode/cspell.json
@@ -52,6 +52,7 @@
     "ABFS",
     "ABNF",
     "adamdebreceni",
+    "adfs",
     "Adls",
     "ahojnnes",
     "ahsonkhan",
diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md
index be3b2d465..c5b9b56ff 100644
--- a/sdk/core/azure-core/CHANGELOG.md
+++ b/sdk/core/azure-core/CHANGELOG.md
@@ -13,6 +13,8 @@
 
 ### Other Changes
 
+- [[#4756]] (https://github.com/Azure/azure-sdk-for-cpp/issues/4756) `BearerTokenAuthenticationPolicy` now uses shared mutex lock for read operations.
+
 ## 1.11.0-beta.2 (2023-11-02)
 
 ### Features Added
diff --git a/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp b/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
index c0dbe7d9e..186c17308 100644
--- a/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
+++ b/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
@@ -25,6 +25,7 @@
 #include <memory>
 #include <mutex>
 #include <set>
+#include <shared_mutex>
 #include <string>
 #include <utility>
 #include <vector>
@@ -549,7 +550,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
       Credentials::TokenRequestContext m_tokenRequestContext;
 
       mutable Credentials::AccessToken m_accessToken;
-      mutable std::mutex m_accessTokenMutex;
+      mutable std::shared_timed_mutex m_accessTokenMutex;
       mutable Credentials::TokenRequestContext m_accessTokenContext;
 
     public:
@@ -581,6 +582,9 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
       BearerTokenAuthenticationPolicy(BearerTokenAuthenticationPolicy const& other)
           : BearerTokenAuthenticationPolicy(other.m_credential, other.m_tokenRequestContext)
       {
+        std::shared_lock<std::shared_timed_mutex> readLock(other.m_accessTokenMutex);
+        m_accessToken = other.m_accessToken;
+        m_accessTokenContext = other.m_accessTokenContext;
       }
 
       void operator=(BearerTokenAuthenticationPolicy const&) = delete;
diff --git a/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp b/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp
index 66cd6b00e..bfce41f86 100644
--- a/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp
+++ b/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp
@@ -61,21 +61,49 @@ bool BearerTokenAuthenticationPolicy::AuthorizeRequestOnChallenge(
   return false;
 }
 
+namespace {
+bool TokenNeedsRefresh(
+    Azure::Core::Credentials::AccessToken const& cachedToken,
+    Azure::Core::Credentials::TokenRequestContext const& cachedTokenRequestContext,
+    Azure::DateTime const& currentTime,
+    Azure::Core::Credentials::TokenRequestContext const& newTokenRequestContext)
+{
+  return newTokenRequestContext.TenantId != cachedTokenRequestContext.TenantId
+      || newTokenRequestContext.Scopes != cachedTokenRequestContext.Scopes
+      || currentTime > (cachedToken.ExpiresOn - newTokenRequestContext.MinimumExpiration);
+}
+
+void ApplyBearerToken(
+    Azure::Core::Http::Request& request,
+    Azure::Core::Credentials::AccessToken const& token)
+{
+  request.SetHeader("authorization", "Bearer " + token.Token);
+}
+} // namespace
+
 void BearerTokenAuthenticationPolicy::AuthenticateAndAuthorizeRequest(
     Request& request,
     Credentials::TokenRequestContext const& tokenRequestContext,
     Context const& context) const
 {
-  std::lock_guard<std::mutex> lock(m_accessTokenMutex);
+  DateTime const currentTime = std::chrono::system_clock::now();
+
+  {
+    std::shared_lock<std::shared_timed_mutex> readLock(m_accessTokenMutex);
+    if (!TokenNeedsRefresh(m_accessToken, m_accessTokenContext, currentTime, tokenRequestContext))
+    {
+      ApplyBearerToken(request, m_accessToken);
+      return;
+    }
+  }
 
-  if (tokenRequestContext.TenantId != m_accessTokenContext.TenantId
-      || tokenRequestContext.Scopes != m_accessTokenContext.Scopes
-      || std::chrono::system_clock::now()
-          > (m_accessToken.ExpiresOn - tokenRequestContext.MinimumExpiration))
+  std::unique_lock<std::shared_timed_mutex> writeLock(m_accessTokenMutex);
+  // Check if token needs refresh for the second time in case another thread has just updated it.
+  if (TokenNeedsRefresh(m_accessToken, m_accessTokenContext, currentTime, tokenRequestContext))
   {
     m_accessToken = m_credential->GetToken(tokenRequestContext, context);
     m_accessTokenContext = tokenRequestContext;
   }
 
-  request.SetHeader("authorization", "Bearer " + m_accessToken.Token);
+  ApplyBearerToken(request, m_accessToken);
 }
diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md
index a024f17a0..3e9081429 100644
--- a/sdk/identity/azure-identity/CHANGELOG.md
+++ b/sdk/identity/azure-identity/CHANGELOG.md
@@ -10,6 +10,8 @@
 
 ### Other Changes
 
+- [[#5141]](https://github.com/Azure/azure-sdk-for-cpp/issues/5141) Added error response details to the Identity exceptions thrown when the authority host returns error response.
+
 ## 1.6.0 (2023-11-10)
 
 ### Features Added
diff --git a/sdk/identity/azure-identity/src/client_certificate_credential.cpp b/sdk/identity/azure-identity/src/client_certificate_credential.cpp
index 1599bd456..b257130fd 100644
--- a/sdk/identity/azure-identity/src/client_certificate_credential.cpp
+++ b/sdk/identity/azure-identity/src/client_certificate_credential.cpp
@@ -9,6 +9,7 @@
 #include <azure/core/base64.hpp>
 #include <azure/core/datetime.hpp>
 #include <azure/core/internal/cryptography/sha_hash.hpp>
+#include <azure/core/internal/strings.hpp>
 #include <azure/core/io/body_stream.hpp>
 #include <azure/core/platform.hpp>
 #include <azure/core/uuid.hpp>
@@ -399,16 +400,33 @@ ClientCertificateCredential::ClientCertificateCredential(
     CertificateThumbprint mdVec;
     try
     {
+      if (clientCertificatePath.empty())
+      {
+        throw AuthenticationException("Certificate file path is empty.");
+      }
+
+      using Azure::Core::_internal::StringExtensions;
+      std::string const PemExtension = ".pem";
+      auto const certFileExtensionStart = clientCertificatePath.find_last_of('.');
+      auto const certFileExtension = certFileExtensionStart != std::string::npos
+          ? clientCertificatePath.substr(certFileExtensionStart)
+          : std::string{};
+
+      if (!StringExtensions::LocaleInvariantCaseInsensitiveEqual(certFileExtension, PemExtension))
+      {
+        throw AuthenticationException(
+            "Certificate format"
+            + (certFileExtension.empty() ? " " : " ('" + certFileExtension + "') ")
+            + "is not supported. Please convert your certificate to '" + PemExtension + "'.");
+      }
+
       std::tie(mdVec, m_pkey) = ReadPemCertificate(clientCertificatePath);
     }
-    catch (AuthenticationException&)
-    {
-      throw;
-    }
-    catch (std::exception& e)
+    catch (std::exception const& e)
     {
       // WIL does not throw AuthenticationException.
-      throw AuthenticationException(e.what());
+      throw AuthenticationException(
+          std::string("Identity: ClientCertificateCredential: ") + e.what());
     }
 
     // Get thumbprint as hex string:
diff --git a/sdk/identity/azure-identity/src/token_credential_impl.cpp b/sdk/identity/azure-identity/src/token_credential_impl.cpp
index aeb8b1397..474e4e5c0 100644
--- a/sdk/identity/azure-identity/src/token_credential_impl.cpp
+++ b/sdk/identity/azure-identity/src/token_credential_impl.cpp
@@ -72,16 +72,13 @@ std::string TokenCredentialImpl::FormatScopes(
 
     if (scopesIter != scopesEnd)
     {
-      auto const scope = *scopesIter;
-      scopesStr += OptionalUrlEncode(scope, urlEncode);
-    }
-
-    for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
-    {
-      auto const Separator = std::string(" "); // Element separator never gets URL-encoded
+      scopesStr += OptionalUrlEncode(*scopesIter, urlEncode);
 
-      auto const scope = *scopesIter;
-      scopesStr += Separator + OptionalUrlEncode(scope, urlEncode);
+      constexpr auto Separator = " "; // Element separator never gets URL-encoded
+      for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
+      {
+        scopesStr += Separator + OptionalUrlEncode(*scopesIter, urlEncode);
+      }
     }
   }
 
@@ -117,11 +114,16 @@ AccessToken TokenCredentialImpl::GetToken(
         request = shouldRetry(statusCode, *response);
         if (request == nullptr)
         {
+          auto const bodyStream = response->ExtractBodyStream();
+          auto const bodyVec = bodyStream ? bodyStream->ReadToEnd(context) : response->GetBody();
+          auto const responseBody
+              = std::string(reinterpret_cast<char const*>(bodyVec.data()), bodyVec.size());
+
           throw std::runtime_error(
               std::string("error response: ")
               + std::to_string(
                   static_cast<std::underlying_type<decltype(statusCode)>::type>(statusCode))
-              + " " + response->GetReasonPhrase());
+              + " " + response->GetReasonPhrase() + "\n\n" + responseBody);
         }
 
         response.reset();


Test patch:
diff --git a/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp b/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp
index 6103da818..511b73393 100644
--- a/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp
+++ b/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp
@@ -148,6 +148,77 @@ TEST_P(GetCredentialName, )
   EXPECT_EQ(cred.GetCredentialName(), "ClientCertificateCredential");
 }
 
+TEST(ClientCertificateCredential, UnsupportedExtension)
+{
+  try
+  {
+    ClientCertificateCredential const cred(
+        "01234567-89ab-cdef-fedc-ba8976543210", "fedcba98-7654-3210-0123-456789abcdef", "file.pfx");
+
+    EXPECT_TRUE(
+        !"ClientCertificateCredential with unsupported extension (.pfx) is supposed to throw.");
+  }
+  catch (Azure::Core::Credentials::AuthenticationException const& ex)
+  {
+    EXPECT_EQ(
+        ex.what(),
+        std::string("Identity: ClientCertificateCredential: "
+                    "Certificate format ('.pfx') is not supported. "
+                    "Please convert your certificate to '.pem'."));
+  }
+
+  try
+  {
+    ClientCertificateCredential const cred(
+        "01234567-89ab-cdef-fedc-ba8976543210",
+        "fedcba98-7654-3210-0123-456789abcdef",
+        "file.cert");
+
+    EXPECT_TRUE(
+        !"ClientCertificateCredential with unsupported extension (.cert) is supposed to throw.");
+  }
+  catch (Azure::Core::Credentials::AuthenticationException const& ex)
+  {
+    EXPECT_EQ(
+        ex.what(),
+        std::string("Identity: ClientCertificateCredential: "
+                    "Certificate format ('.cert') is not supported. "
+                    "Please convert your certificate to '.pem'."));
+  }
+
+  try
+  {
+    ClientCertificateCredential const cred(
+        "01234567-89ab-cdef-fedc-ba8976543210",
+        "fedcba98-7654-3210-0123-456789abcdef",
+        "noextension");
+
+    EXPECT_TRUE(!"ClientCertificateCredential without an extension is supposed to throw.");
+  }
+  catch (Azure::Core::Credentials::AuthenticationException const& ex)
+  {
+    EXPECT_EQ(
+        ex.what(),
+        std::string("Identity: ClientCertificateCredential: "
+                    "Certificate format is not supported. "
+                    "Please convert your certificate to '.pem'."));
+  }
+
+  try
+  {
+    ClientCertificateCredential const cred(
+        "01234567-89ab-cdef-fedc-ba8976543210", "fedcba98-7654-3210-0123-456789abcdef", "");
+
+    EXPECT_TRUE(!"ClientCertificateCredential with an empty path is supposed to throw.");
+  }
+  catch (Azure::Core::Credentials::AuthenticationException const& ex)
+  {
+    EXPECT_EQ(
+        ex.what(),
+        std::string("Identity: ClientCertificateCredential: Certificate file path is empty."));
+  }
+}
+
 TEST(ClientCertificateCredential, GetOptionsFromEnvironment)
 {
   {
diff --git a/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp b/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp
index ab07a4c0a..5ff80f5a7 100644
--- a/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp
+++ b/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp
@@ -710,6 +710,49 @@ TEST(TokenCredentialImpl, MaxValues)
       std::exception);
 }
 
+TEST(TokenCredentialImpl, AuthErrorResponse)
+{
+  std::string const errorJson
+      = "{\"error\":\"invalid_request\","
+        "\"error_description\":\"AADSTS90014: "
+        "The required field 'scope' is missing from the credential. "
+        "Ensure that you have all the necessary parameters for the login request. "
+        "Trace ID: 01234567-89ab-cdef-0123-456789abcdef "
+        "Correlation ID: fedcba98-7654-3210-0123-456789abcdef "
+        "Timestamp: 2023-11-30 00:51:37Z\","
+        "\"error_codes\":[90014],"
+        "\"timestamp\":\"2023-11-30 00:51:37Z\","
+        "\"trace_id\":\"01234567-89ab-cdef-0123-456789abcdef\","
+        "\"correlation_id\":\"fedcba98-7654-3210-0123-456789abcdef\","
+        "\"error_uri\":\"https://login.microsoftonline.com/error?code=90014\"}";
+
+  try
+  {
+    using Azure::Core::Http::HttpStatusCode;
+    std::vector<std::string> const testScopes;
+    CredentialTestHelper::TokenRequestSimulationServerResponse testResponse;
+    testResponse.StatusCode = HttpStatusCode::BadRequest;
+    testResponse.Body = errorJson;
+
+    static_cast<void>(CredentialTestHelper::SimulateTokenRequest(
+        [](auto transport) {
+          TokenCredentialOptions options;
+          options.Transport.Transport = transport;
+
+          return std::make_unique<TokenCredentialImplTester>(
+              HttpMethod::Delete, Url("https://outlook.com/"), options);
+        },
+        {testScopes},
+        {testResponse}));
+
+    EXPECT_TRUE(!"TokenCredentialImpl should throw given the response above.");
+  }
+  catch (AuthenticationException const& ex)
+  {
+    EXPECT_EQ(ex.what(), "GetToken(): error response: 400 Test\n\n" + errorJson);
+  }
+}
+
 TEST(TokenCredentialImpl, Diagnosability)
 {
   using Azure::Core::Diagnostics::Logger;