AzureCliCredential: add support for `expires_on` field. (#5180)

* AzureCliCredential: add support for xpires_on field. * Remove unused variables * Remove unused variables * Clang-format * Update changelog * PR feedback * Update unit test expectation * PR Feedback * Add comment * Remove extra line * PR feedback --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>
2023-12-14 17:13:09 -08:00 · 2023-12-14 17:13:09 -08:00 · eba15a9ff4
--- a/sdk/identity/azure-identity/CHANGELOG.md
+++ b/sdk/identity/azure-identity/CHANGELOG.md
@ -4,6 +4,8 @@

 ### Features Added

+- [[#5116]](https://github.com/Azure/azure-sdk-for-cpp/issues/5116) `AzureCliCredential`: Added support for the new response field which represents token expiration timestamp as time zone agnostic value.
+
 ### Breaking Changes

 ### Bugs Fixed
--- a/sdk/identity/azure-identity/src/azure_cli_credential.cpp
+++ b/sdk/identity/azure-identity/src/azure_cli_credential.cpp
@ -153,8 +153,21 @@ AccessToken AzureCliCredential::GetToken(

      try
      {
+        // The order of elements in the vector below does matter - the code tries to find them
+        // consequently, and if finding the first one succeeds, we would not attempt to parse the
+        // second one. That is important, because the newer Azure CLI versions do have the new
+        // 'expires_on' field, which is not affected by time zone changes. The 'expiresOn' field was
+        // the only field that was present in the older versions, and it had problems, because it
+        // was a local timestamp without the time zone information.
+        // So, if only the 'expires_on' is available, we try to use it, and only if it is not
+        // available, we fall back to trying to get the value via 'expiresOn', which we also now are
+        // able to handle correctly, except when the token expiration crosses the time when the
+        // local system clock moves to and from DST.
        return TokenCredentialImpl::ParseToken(
-            azCliResult, "accessToken", "expiresIn", "expiresOn");
+            azCliResult,
+            "accessToken",
+            "expiresIn",
+            std::vector<std::string>{"expires_on", "expiresOn"});
      }
      catch (json::exception const&)
      {
--- a/sdk/identity/azure-identity/src/private/token_credential_impl.hpp
+++ b/sdk/identity/azure-identity/src/private/token_credential_impl.hpp
@ -59,6 +59,31 @@ namespace Azure { namespace Identity { namespace _detail {
        bool asResource,
        bool urlEncode = true);

+    /**
+     * @brief Parses JSON that contains access token and its expiration.
+     *
+     * @param jsonString String with a JSON object to parse.
+     * @param accessTokenPropertyName Name of a property in the JSON object that represents access
+     * token.
+     * @param expiresInPropertyName Name of a property in the JSON object that represents token
+     * expiration in number of seconds from now.
+     * @param expiresOnPropertyNames Names of properties in the JSON object that represent token
+     * expiration as absolute date-time stamp. Can be empty, in which case no attempt to parse the
+     * corresponding property will be made. Empty string elements will be ignored.
+     *
+     * @return A successfully parsed access token.
+     *
+     * @throw `std::exception` if there was a problem parsing the token.
+     *
+     * @note The order of elements in \p expiresOnPropertyNames does matter: the code will first try
+     * to find a property with the name of the first element, then of a second one, and so on.
+     */
+    static Core::Credentials::AccessToken ParseToken(
+        std::string const& jsonString,
+        std::string const& accessTokenPropertyName,
+        std::string const& expiresInPropertyName,
+        std::vector<std::string> const& expiresOnPropertyNames);
+
    /**
     * @brief Parses JSON that contains access token and its expiration.
     *
@ -79,7 +104,14 @@ namespace Azure { namespace Identity { namespace _detail {
        std::string const& jsonString,
        std::string const& accessTokenPropertyName,
        std::string const& expiresInPropertyName,
-        std::string const& expiresOnPropertyName);
+        std::string const& expiresOnPropertyName)
+    {
+      return ParseToken(
+          jsonString,
+          accessTokenPropertyName,
+          expiresInPropertyName,
+          std::vector<std::string>{expiresOnPropertyName});
+    }

    /**
     * @brief Holds `#Azure::Core::Http::Request` and all the associated resources for the HTTP
--- a/sdk/identity/azure-identity/src/token_credential_impl.cpp
+++ b/sdk/identity/azure-identity/src/token_credential_impl.cpp
@ -10,7 +10,9 @@
 #include <azure/core/internal/strings.hpp>
 #include <azure/core/url.hpp>

+#include <algorithm>
 #include <chrono>
+#include <iterator>
 #include <limits>
 #include <map>

@ -169,14 +171,14 @@ std::string TokenAsDiagnosticString(
    json const& jsonObject,
    std::string const& accessTokenPropertyName,
    std::string const& expiresInPropertyName,
-    std::string const& expiresOnPropertyName);
+    std::vector<std::string> const& expiresOnPropertyNames);

 [[noreturn]] void ThrowJsonPropertyError(
    std::string const& failedPropertyName,
    json const& jsonObject,
    std::string const& accessTokenPropertyName,
    std::string const& expiresInPropertyName,
-    std::string const& expiresOnPropertyName)
+    std::vector<std::string> const& expiresOnPropertyNames)
 {
  if (IdentityLog::ShouldWrite(IdentityLog::Level::Verbose))
  {
@ -184,7 +186,10 @@ std::string TokenAsDiagnosticString(
        IdentityLog::Level::Verbose,
        ParseTokenLogPrefix
            + TokenAsDiagnosticString(
-                jsonObject, accessTokenPropertyName, expiresInPropertyName, expiresOnPropertyName));
+                jsonObject,
+                accessTokenPropertyName,
+                expiresInPropertyName,
+                expiresOnPropertyNames));
  }

  throw std::runtime_error(
@ -198,7 +203,7 @@ AccessToken TokenCredentialImpl::ParseToken(
    std::string const& jsonString,
    std::string const& accessTokenPropertyName,
    std::string const& expiresInPropertyName,
-    std::string const& expiresOnPropertyName)
+    std::vector<std::string> const& expiresOnPropertyNames)
 {
  json parsedJson;
  try
@ -222,7 +227,7 @@ AccessToken TokenCredentialImpl::ParseToken(
        parsedJson,
        accessTokenPropertyName,
        expiresInPropertyName,
-        expiresOnPropertyName);
+        expiresOnPropertyNames);
  }

  AccessToken accessToken;
@ -276,17 +281,27 @@ AccessToken TokenCredentialImpl::ParseToken(
    }
  }

-  if (expiresOnPropertyName.empty())
+  std::vector<std::string> nonEmptyExpiresOnPropertyNames;
+  std::copy_if(
+      expiresOnPropertyNames.begin(),
+      expiresOnPropertyNames.end(),
+      std::back_inserter(nonEmptyExpiresOnPropertyNames),
+      [](auto const& propertyName) { return !propertyName.empty(); });
+
+  if (nonEmptyExpiresOnPropertyNames.empty())
  {
-    // 'expires_in' is undefined, 'expires_on' is not expected.
+    // The code was not able to parse the value of 'expires_in', and the caller did not pass any
+    // 'expires_on' for us to find and try parse.
    ThrowJsonPropertyError(
        expiresInPropertyName,
        parsedJson,
        accessTokenPropertyName,
        expiresInPropertyName,
-        expiresOnPropertyName);
+        nonEmptyExpiresOnPropertyNames);
  }

+  for (auto const& expiresOnPropertyName : nonEmptyExpiresOnPropertyNames)
+  {
    if (parsedJson.contains(expiresOnPropertyName))
    {
      auto const& expiresOn = parsedJson[expiresOnPropertyName];
@ -340,13 +355,14 @@ AccessToken TokenCredentialImpl::ParseToken(
        }
      }
    }
+  }

  ThrowJsonPropertyError(
-      expiresOnPropertyName,
+      nonEmptyExpiresOnPropertyNames.back(),
      parsedJson,
      accessTokenPropertyName,
      expiresInPropertyName,
-      expiresOnPropertyName);
+      nonEmptyExpiresOnPropertyNames);
 }

 namespace {
@ -435,7 +451,7 @@ std::string TokenAsDiagnosticString(
    json const& jsonObject,
    std::string const& accessTokenPropertyName,
    std::string const& expiresInPropertyName,
-    std::string const& expiresOnPropertyName)
+    std::vector<std::string> const& expiresOnPropertyNames)
 {
  std::string result = "Please report an issue with the following details:\nToken JSON";

@ -458,10 +474,16 @@ std::string TokenAsDiagnosticString(
          : PrintSanitizedJsonObject(accessTokenProperty, false);
    }

-    for (auto const& p : {
-             std::pair<char const*, std::string const*>{"relative", &expiresInPropertyName},
-             std::pair<char const*, std::string const*>{"absolute", &expiresOnPropertyName},
-         })
+    {
+      std::vector<std::pair<char const*, std::string const*>> expirationProperties{
+          {"relative", &expiresInPropertyName}};
+
+      for (auto const& ap : expiresOnPropertyNames)
+      {
+        expirationProperties.emplace_back(std::make_pair("absolute", &ap));
+      }
+
+      for (auto const& p : expirationProperties)
      {
        result += ", ";
        result += p.first;
@ -476,12 +498,14 @@ std::string TokenAsDiagnosticString(
          result += PrintSanitizedJsonObject(jsonObject[*p.second], true);
        }
      }
+    }

    std::map<std::string, json> otherProperties;
    for (auto const& property : jsonObject.items())
    {
      if (property.key() != accessTokenPropertyName && property.key() != expiresInPropertyName
-          && property.key() != expiresOnPropertyName)
+          && std::find(expiresOnPropertyNames.begin(), expiresOnPropertyNames.end(), property.key())
+              == expiresOnPropertyNames.end())
      {
        otherProperties[property.key()] = property.value();
      }
--- a/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp
+++ b/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp
@ -221,6 +221,56 @@ TEST(AzureCliCredential, ExpiresIn)
  EXPECT_LE(token.ExpiresOn, timestampAfter + std::chrono::seconds(30));
 }

+TEST(AzureCliCredential, ExpiresOnUnixTimestampInt)
+{
+  // 'expires_on' is 1700692424, which is a Unix timestamp of a date in 2023.
+  // 'ExpiresOn' is a date in 2022.
+  // The test checks that when both are present, 'expires_on' value (2023) is taken,
+  // and not that of 'ExpiresOn'.
+  constexpr auto Token = "{\"accessToken\":\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\","
+                         "\"expiresOn\":\"2022-08-24 00:43:08.000000\"," // <-- 2022
+                         "\"expires_on\":1700692424}"; // <-- 2023
+
+  AzureCliTestCredential const azCliCred(EchoCommand(Token));
+
+  TokenRequestContext trc;
+  trc.Scopes.push_back("https://storage.azure.com/.default");
+
+  auto const token = azCliCred.GetToken(trc, {});
+
+  EXPECT_EQ(token.Token, "ABCDEFGHIJKLMNOPQRSTUVWXYZ");
+
+  EXPECT_EQ(
+      token.ExpiresOn,
+      DateTime::Parse("2023-11-22T22:33:44.000000Z", DateTime::DateFormat::Rfc3339));
+}
+
+TEST(AzureCliCredential, ExpiresOnUnixTimestampString)
+{
+  // 'expires_on' is 1700692424, which is a Unix timestamp of a date in 2023.
+  // 'expiresOn' is a date in 2022.
+  // The test checks that when both are present, 'expires_on' value (2023) is taken,
+  // and not that of 'expiresOn'.
+  // The test is similar to the one above, but the Unix timestamp is represented as string
+  // containing an integer.
+  constexpr auto Token = "{\"accessToken\":\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\","
+                         "\"expiresOn\":\"2022-08-24 00:43:08.000000\"," // <-- 2022
+                         "\"expires_on\":\"1700692424\"}"; // <-- 2023
+
+  AzureCliTestCredential const azCliCred(EchoCommand(Token));
+
+  TokenRequestContext trc;
+  trc.Scopes.push_back("https://storage.azure.com/.default");
+
+  auto const token = azCliCred.GetToken(trc, {});
+
+  EXPECT_EQ(token.Token, "ABCDEFGHIJKLMNOPQRSTUVWXYZ");
+
+  EXPECT_EQ(
+      token.ExpiresOn,
+      DateTime::Parse("2023-11-22T22:33:44.000000Z", DateTime::DateFormat::Rfc3339));
+}
+
 TEST(AzureCliCredential, TimedOut)
 {
  AzureCliCredentialOptions options;
--- a/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp
+++ b/sdk/identity/azure-identity/test/ut/token_credential_impl_test.cpp
@ -915,7 +915,7 @@ TEST(TokenCredentialImpl, Diagnosability)
          "}",
          "TokenForAccessing",
          "TokenExpiresInSeconds",
-          {}));
+          std::string{}));
    }
    catch (std::exception const& e)
    {
@ -937,7 +937,6 @@ TEST(TokenCredentialImpl, Diagnosability)
        "Please report an issue with the following details:\n"
        "Token JSON: Access token property ('TokenForAccessing'): string.length=11, "
        "relative expiration property ('TokenExpiresInSeconds'): \"one\", "
-        "absolute expiration property (''): undefined, "
        "and there are no other properties.");

    Logger::SetListener(nullptr);
@ -988,6 +987,63 @@ TEST(TokenCredentialImpl, Diagnosability)
    Logger::SetListener(nullptr);
  }

+  // Token is ok, relative expiration can't be parsed, two absolute expiration property names were
+  // provided, none of them can be parsed.
+  // I.e.
+  // 1. Token is ok.
+  // 2. Relative expiration ('TokenExpiresInSeconds') is not ok (null),
+  // 3. Absolute expiration (two properties - 'TokenExpiresAtTime' and 'token_expires_at_time')
+  //    are not ok (both are null).
+  //
+  // The test verifies that we print the log message that involves the names of BOTH absolute
+  // expiration properties.
+  //
+  // For all other tests, the message would include only one absolute expiration property
+  // ('TokenExpiresAtTime'), now we check that both are printed.
+  {
+    LogMsgVec log;
+    Logger::SetListener([&](auto lvl, auto msg) { log.push_back(std::make_pair(lvl, msg)); });
+
+    auto exceptionThrown = false;
+    try
+    {
+      static_cast<void>(TokenCredentialImpl::ParseToken(
+          "{\"TokenForAccessing\":\"ACCESSTOKEN\","
+          "\"TokenExpiresInSeconds\":null,"
+          "\"TokenExpiresAtTime\":null,"
+          "\"token_expires_at_time\":null"
+          "}",
+          "TokenForAccessing",
+          "TokenExpiresInSeconds",
+          std::vector<std::string>{"token_expires_at_time", "TokenExpiresAtTime"}));
+    }
+    catch (std::exception const& e)
+    {
+      exceptionThrown = true;
+
+      EXPECT_EQ(
+          e.what(),
+          std::string("Token JSON object: can't find or parse 'TokenExpiresAtTime' property."
+                      "\nSee Azure::Core::Diagnostics::Logger for details"
+                      " (https://aka.ms/azsdk/cpp/identity/troubleshooting)."));
+    }
+
+    EXPECT_TRUE(exceptionThrown);
+    EXPECT_EQ(log.size(), 1U);
+    EXPECT_EQ(log.at(0).first, Logger::Level::Verbose);
+    EXPECT_EQ(
+        log.at(0).second,
+        "Identity: TokenCredentialImpl::ParseToken(): "
+        "Please report an issue with the following details:\n"
+        "Token JSON: Access token property ('TokenForAccessing'): string.length=11, "
+        "relative expiration property ('TokenExpiresInSeconds'): null, "
+        "absolute expiration property ('token_expires_at_time'): null, " // <-- this gets printed
+        "absolute expiration property ('TokenExpiresAtTime'): null, " // <-- as well as this
+        "and there are no other properties.");
+
+    Logger::SetListener(nullptr);
+  }
+
  // Token is ok, relative expiration is missing, absolute expiration can't be parsed,
  // And one other property has RFC3339 timestamp string, while the other is a number.
  {
@ -2508,3 +2564,89 @@ TEST(TokenCredentialImpl, Diagnosability)
    Logger::SetListener(nullptr);
  }
 }
+
+TEST(TokenCredentialImpl, ParseExpiresOnVectorEdgeCases)
+{
+  using Azure::Core::Diagnostics::Logger;
+  using Azure::Core::Json::_internal::json;
+  using LogMsgVec = std::vector<std::pair<Logger::Level, std::string>>;
+
+  Logger::SetLevel(Logger::Level::Verbose);
+
+  {
+    LogMsgVec log;
+    Logger::SetListener([&](auto lvl, auto msg) { log.push_back(std::make_pair(lvl, msg)); });
+
+    auto exceptionThrown = false;
+    try
+    {
+      static_cast<void>(TokenCredentialImpl::ParseToken(
+          "{\"token\": \"X\", \"expires_at\": 1700692424}",
+          "token",
+          "expires_in",
+          std::vector<std::string>{}));
+    }
+    catch (std::exception const& e)
+    {
+      exceptionThrown = true;
+
+      EXPECT_EQ(
+          e.what(),
+          std::string("Token JSON object: can't find or parse 'expires_in' property."
+                      "\nSee Azure::Core::Diagnostics::Logger for details"
+                      " (https://aka.ms/azsdk/cpp/identity/troubleshooting)."));
+    }
+
+    EXPECT_TRUE(exceptionThrown);
+    EXPECT_EQ(log.size(), 1U);
+    EXPECT_EQ(log.at(0).first, Logger::Level::Verbose);
+    EXPECT_EQ(
+        log.at(0).second,
+        "Identity: TokenCredentialImpl::ParseToken(): "
+        "Please report an issue with the following details:\n"
+        "Token JSON: Access token property ('token'): string.length=1, "
+        "relative expiration property ('expires_in'): undefined, "
+        "other properties: 'expires_at': 1700692424.");
+
+    Logger::SetListener(nullptr);
+  }
+
+  {
+    LogMsgVec log;
+    Logger::SetListener([&](auto lvl, auto msg) { log.push_back(std::make_pair(lvl, msg)); });
+
+    auto exceptionThrown = false;
+    try
+    {
+      static_cast<void>(TokenCredentialImpl::ParseToken(
+          "{\"token\": \"X\", \"expires_at\": 1700692424}",
+          "token",
+          "expires_in",
+          std::vector<std::string>{"", "expires_on", ""}));
+    }
+    catch (std::exception const& e)
+    {
+      exceptionThrown = true;
+
+      EXPECT_EQ(
+          e.what(),
+          std::string("Token JSON object: can't find or parse 'expires_on' property."
+                      "\nSee Azure::Core::Diagnostics::Logger for details"
+                      " (https://aka.ms/azsdk/cpp/identity/troubleshooting)."));
+    }
+
+    EXPECT_TRUE(exceptionThrown);
+    EXPECT_EQ(log.size(), 1U);
+    EXPECT_EQ(log.at(0).first, Logger::Level::Verbose);
+    EXPECT_EQ(
+        log.at(0).second,
+        "Identity: TokenCredentialImpl::ParseToken(): "
+        "Please report an issue with the following details:\n"
+        "Token JSON: Access token property ('token'): string.length=1, "
+        "relative expiration property ('expires_in'): undefined, "
+        "absolute expiration property ('expires_on'): undefined, "
+        "other properties: 'expires_at': 1700692424.");
+
+    Logger::SetListener(nullptr);
+  }
+}