Identity: Improve diagnosability

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

@@ -8,6 +8,7 @@
 #include "private/token_credential_impl.hpp"
 
 #include <azure/core/internal/environment.hpp>
+#include <azure/core/internal/json/json.hpp>
 #include <azure/core/internal/strings.hpp>
 #include <azure/core/internal/unique_handle.hpp>
 #include <azure/core/platform.hpp>
@@ -47,6 +48,7 @@ using Azure::Core::Credentials::AccessToken;
 using Azure::Core::Credentials::AuthenticationException;
 using Azure::Core::Credentials::TokenCredentialOptions;
 using Azure::Core::Credentials::TokenRequestContext;
+using Azure::Core::Json::_internal::json;
 using Azure::Identity::AzureCliCredentialOptions;
 using Azure::Identity::_detail::IdentityLog;
 using Azure::Identity::_detail::TenantIdResolver;
@@ -160,10 +162,18 @@ AccessToken AzureCliCredential::GetToken(
         return TokenCredentialImpl::ParseToken(
             azCliResult, "accessToken", "expiresIn", "expiresOn");
       }
-      catch (std::exception const&)
+      catch (json::exception const&)
       {
-        // Throw the az command output (error message)
-        // limited to 250 characters (250 has no special meaning).
+        // json::exception gets thrown when a string we provided for parsing is not a json object.
+        // It should not get thrown if the string is a valid JSON, but there are specific problems
+        // with the token JSON object - missing property, failure to parse a specific property etc.
+        // I.e. this means that the az commnd has rather printed some error message
+        // (such as "ERROR: Please run az login to setup account.") instead of producing a JSON
+        // object output. In this case, we want the exception to be thrown with the output from the
+        // command (which is likely the error message) and not with the details of the exception
+        // that was thrown from ParseToken() (which most likely will be "Unexpected token ...").
+        // So, we limit the az command output (error message) limited to 250 characters so it is not
+        // too long, and throw that.
         throw std::runtime_error(azCliResult.substr(0, 250));
       }
     }

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

@@ -3,16 +3,20 @@
 
 #include "private/token_credential_impl.hpp"
 
+#include "private/identity_log.hpp"
 #include "private/package_version.hpp"
 
 #include <azure/core/internal/json/json.hpp>
 #include <azure/core/url.hpp>
 
 #include <chrono>
+#include <map>
+#include <sstream>
 #include <type_traits>
 
 using Azure::Identity::_detail::TokenCredentialImpl;
 
+using Azure::Identity::_detail::IdentityLog;
 using Azure::Identity::_detail::PackageVersion;
 
 using Azure::DateTime;
@@ -24,6 +28,7 @@ using Azure::Core::Credentials::AuthenticationException;
 using Azure::Core::Credentials::TokenCredentialOptions;
 using Azure::Core::Http::HttpStatusCode;
 using Azure::Core::Http::RawResponse;
+using Azure::Core::Json::_internal::json;
 
 TokenCredentialImpl::TokenCredentialImpl(TokenCredentialOptions const& options)
     : m_httpPipeline(options, "identity", PackageVersion::ToString(), {}, {})
@@ -141,10 +146,127 @@ AccessToken TokenCredentialImpl::GetToken(
 }
 
 namespace {
-[[noreturn]] void ThrowJsonPropertyError(std::string const& propertyName)
+std::string const ParseTokenLogPrefix = "TokenCredentialImpl::ParseToken(): ";
+
+std::string TokenAsDiagnosticString(
+    json const& jsonObject,
+    std::string const& accessTokenPropertyName,
+    std::string const& expiresInPropertyName,
+    std::string const& expiresOnPropertyName)
 {
+  std::stringstream ss;
+  ss << "Token JSON";
+
+  if (!jsonObject.is_object())
+  {
+    ss << " is not an object (value='" << jsonObject.dump() << "')";
+  }
+  else
+  {
+    ss << ": Access token property ('" << accessTokenPropertyName << "') ";
+    if (!jsonObject.contains(accessTokenPropertyName))
+    {
+      ss << "is NOT present";
+    }
+    else
+    {
+      auto const& accessTokenProperty = jsonObject[accessTokenPropertyName];
+      if (!accessTokenProperty.is_string())
+      {
+        ss << "is NOT a string (value='" << accessTokenProperty.dump() << "')";
+      }
+      else
+      {
+        ss << "is string (length=" << accessTokenProperty.get<std::string>().length() << ")";
+      }
+    }
+
+    for (auto const& p : {
+             std::pair<char const*, std::string const*>{"relative", &expiresInPropertyName},
+             std::pair<char const*, std::string const*>{"absolute", &expiresOnPropertyName},
+         })
+    {
+      ss << ", " << p.first << " expiration property ('" << *p.second << "') ";
+      if (!jsonObject.contains(*p.second))
+      {
+        ss << "is NOT present";
+      }
+      else
+      {
+        ss << "is present (value='" << jsonObject[*p.second].dump() << "')";
+      }
+    }
+
+    std::map<std::string, json> otherProperties;
+    for (auto const& property : jsonObject.items())
+    {
+      if (property.key() != accessTokenPropertyName && property.key() != expiresInPropertyName
+          && property.key() != expiresOnPropertyName)
+      {
+        otherProperties[property.key()] = property.value();
+      }
+    }
+
+    ss << ", ";
+    if (otherProperties.empty())
+    {
+      ss << "and there are no other properties";
+    }
+    else
+    {
+      ss << "other properties";
+      const char* delimiter = ": ";
+      for (auto const& property : otherProperties)
+      {
+        ss << delimiter << "'" << property.first << "' (";
+        delimiter = ", ";
+
+        auto const dump = property.second.dump();
+        if (dump.size() <= 100)
+        {
+          ss << "value='" << dump << "'";
+        }
+        else
+        {
+          if (property.second.is_string())
+          {
+            ss << "string length=" << property.second.get<std::string>().length();
+          }
+          else
+          {
+            ss << "value size=" << dump.size();
+          }
+        }
+
+        ss << ")";
+      }
+    }
+  }
+
+  ss << ".";
+  return ss.str();
+}
+
+[[noreturn]] void ThrowJsonPropertyError(
+    std::string const& failedPropertyName,
+    json const& jsonObject,
+    std::string const& accessTokenPropertyName,
+    std::string const& expiresInPropertyName,
+    std::string const& expiresOnPropertyName)
+{
+  if (IdentityLog::ShouldWrite(IdentityLog::Level::Verbose))
+  {
+    IdentityLog::Write(
+        IdentityLog::Level::Verbose,
+        ParseTokenLogPrefix
+            + TokenAsDiagnosticString(
+                jsonObject, accessTokenPropertyName, expiresInPropertyName, expiresOnPropertyName));
+  }
+
   throw std::runtime_error(
-      std::string("Token JSON object: can't find or parse \'") + propertyName + "\' property.");
+      "Token JSON object: can't find or parse \'" + failedPropertyName
+      + "\' property.\nSee Azure::Core::Diagnostics::Logger for details"
+        " (https://aka.ms/azsdk/cpp/identity/troubleshooting).");
 }
 } // namespace
 
@@ -154,12 +276,29 @@ AccessToken TokenCredentialImpl::ParseToken(
     std::string const& expiresInPropertyName,
     std::string const& expiresOnPropertyName)
 {
-  auto const parsedJson = Azure::Core::Json::_internal::json::parse(jsonString);
+  json parsedJson;
+  try
+  {
+    parsedJson = Azure::Core::Json::_internal::json::parse(jsonString);
+  }
+  catch (json::exception const&)
+  {
+    IdentityLog::Write(
+        IdentityLog::Level::Verbose,
+        ParseTokenLogPrefix + "Cannot parse the string '" + jsonString + "' as JSON.");
+
+    throw;
+  }
 
   if (!parsedJson.contains(accessTokenPropertyName)
       || !parsedJson[accessTokenPropertyName].is_string())
   {
-    ThrowJsonPropertyError(accessTokenPropertyName);
+    ThrowJsonPropertyError(
+        accessTokenPropertyName,
+        parsedJson,
+        accessTokenPropertyName,
+        expiresInPropertyName,
+        expiresOnPropertyName);
   }
 
   AccessToken accessToken;
@@ -198,7 +337,12 @@ AccessToken TokenCredentialImpl::ParseToken(
   if (expiresOnPropertyName.empty())
   {
     // 'expires_in' is undefined, 'expires_on' is not expected.
-    ThrowJsonPropertyError(expiresInPropertyName);
+    ThrowJsonPropertyError(
+        expiresInPropertyName,
+        parsedJson,
+        accessTokenPropertyName,
+        expiresInPropertyName,
+        expiresOnPropertyName);
   }
 
   if (parsedJson.contains(expiresOnPropertyName))
@@ -245,5 +389,10 @@ AccessToken TokenCredentialImpl::ParseToken(
     }
   }
 
-  ThrowJsonPropertyError(expiresOnPropertyName);
+  ThrowJsonPropertyError(
+      expiresOnPropertyName,
+      parsedJson,
+      accessTokenPropertyName,
+      expiresInPropertyName,
+      expiresOnPropertyName);
 }

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

@@ -359,4 +359,54 @@ TEST(AzureCliCredential, StrictIso8601TimeFormat)
       token.ExpiresOn,
       DateTime::Parse("2022-08-24T00:43:08.000000Z", DateTime::DateFormat::Rfc3339));
 }
+
+TEST(AzureCliCredential, Diagnosability)
+{
+  {
+    AzureCliTestCredential const azCliCred(
+        EchoCommand("az is not recognized as an internal or external command, "
+                    "operable program or batch file."));
+
+    TokenRequestContext trc;
+    trc.Scopes.push_back("https://storage.azure.com/.default");
+    try
+    {
+      static_cast<void>(azCliCred.GetToken(trc, {}));
+    }
+    catch (AuthenticationException const& e)
+    {
+      std::string const expectedMsgStart
+          = "AzureCliCredential didn't get the token: "
+            "\"az is not recognized as an internal or external command, "
+            "operable program or batch file.";
+
+      std::string actualMsgStart = e.what();
+      actualMsgStart.resize(expectedMsgStart.length());
+
+      // It is enough to compare StartsWith() and not deal with
+      // the entire string due to '/n' and '/r/n' differences.
+      EXPECT_EQ(actualMsgStart, expectedMsgStart);
+    }
+  }
+
+  {
+    AzureCliTestCredential const azCliCred(EchoCommand("{\"property\":\"value\"}"));
+
+    TokenRequestContext trc;
+    trc.Scopes.push_back("https://storage.azure.com/.default");
+    try
+    {
+      static_cast<void>(azCliCred.GetToken(trc, {}));
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_EQ(
+          e.what(),
+          std::string("AzureCliCredential didn't get the token: "
+                      "\"Token JSON object: can't find or parse 'accessToken' property.\n"
+                      "See Azure::Core::Diagnostics::Logger for details "
+                      "(https://aka.ms/azsdk/cpp/identity/troubleshooting).\""));
+    }
+  }
+}
 #endif // not UWP