diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index a024f17a03..c35e6dcf25 100644 --- 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 diff --git a/sdk/identity/azure-identity/src/azure_cli_credential.cpp b/sdk/identity/azure-identity/src/azure_cli_credential.cpp index 911fee6833..c401f1d318 100644 --- 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{"expires_on", "expiresOn"}); } catch (json::exception const&) { diff --git a/sdk/identity/azure-identity/src/private/token_credential_impl.hpp b/sdk/identity/azure-identity/src/private/token_credential_impl.hpp index 4f8fa04cf7..f4c761dce9 100644 --- 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 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{expiresOnPropertyName}); + } /** * @brief Holds `#Azure::Core::Http::Request` and all the associated resources for the HTTP diff --git a/sdk/identity/azure-identity/src/token_credential_impl.cpp b/sdk/identity/azure-identity/src/token_credential_impl.cpp index aeb8b1397d..2b780ff4b1 100644 --- 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 #include +#include #include +#include #include #include @@ -167,14 +169,14 @@ std::string TokenAsDiagnosticString( json const& jsonObject, std::string const& accessTokenPropertyName, std::string const& expiresInPropertyName, - std::string const& expiresOnPropertyName); + std::vector 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 const& expiresOnPropertyNames) { if (IdentityLog::ShouldWrite(IdentityLog::Level::Verbose)) { @@ -182,7 +184,10 @@ std::string TokenAsDiagnosticString( IdentityLog::Level::Verbose, ParseTokenLogPrefix + TokenAsDiagnosticString( - jsonObject, accessTokenPropertyName, expiresInPropertyName, expiresOnPropertyName)); + jsonObject, + accessTokenPropertyName, + expiresInPropertyName, + expiresOnPropertyNames)); } throw std::runtime_error( @@ -196,7 +201,7 @@ AccessToken TokenCredentialImpl::ParseToken( std::string const& jsonString, std::string const& accessTokenPropertyName, std::string const& expiresInPropertyName, - std::string const& expiresOnPropertyName) + std::vector const& expiresOnPropertyNames) { json parsedJson; try @@ -220,7 +225,7 @@ AccessToken TokenCredentialImpl::ParseToken( parsedJson, accessTokenPropertyName, expiresInPropertyName, - expiresOnPropertyName); + expiresOnPropertyNames); } AccessToken accessToken; @@ -274,77 +279,88 @@ AccessToken TokenCredentialImpl::ParseToken( } } - if (expiresOnPropertyName.empty()) + std::vector 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); } - if (parsedJson.contains(expiresOnPropertyName)) + for (auto const& expiresOnPropertyName : nonEmptyExpiresOnPropertyNames) { - auto const& expiresOn = parsedJson[expiresOnPropertyName]; - - if (expiresOn.is_number_unsigned()) + if (parsedJson.contains(expiresOnPropertyName)) { - try - { - // 'expires_on' as number (posix time representing an absolute timestamp) - auto const value = expiresOn.get(); - if (value <= MaxPosixTimestamp) - { - accessToken.ExpiresOn = PosixTimeConverter::PosixTimeToDateTime(value); - return accessToken; - } - } - catch (std::exception const&) - { - // expiresIn.get() has thrown, we may throw later. - } - } + auto const& expiresOn = parsedJson[expiresOnPropertyName]; - if (expiresOn.is_string()) - { - auto const expiresOnAsString = expiresOn.get(); - for (auto const& parse : { - std::function([](auto const& s) { - // 'expires_on' as RFC3339 date string (absolute timestamp) - return DateTime::Parse(s, DateTime::DateFormat::Rfc3339); - }), - std::function([](auto const& s) { - // 'expires_on' as numeric string (posix time representing an absolute timestamp) - return PosixTimeConverter::PosixTimeToDateTime( - ParseNumericExpiration(s, MaxPosixTimestamp)); - }), - std::function([](auto const& s) { - // 'expires_on' as RFC1123 date string (absolute timestamp) - return DateTime::Parse(s, DateTime::DateFormat::Rfc1123); - }), - }) + if (expiresOn.is_number_unsigned()) { try { - accessToken.ExpiresOn = parse(expiresOnAsString); - return accessToken; + // 'expires_on' as number (posix time representing an absolute timestamp) + auto const value = expiresOn.get(); + if (value <= MaxPosixTimestamp) + { + accessToken.ExpiresOn = PosixTimeConverter::PosixTimeToDateTime(value); + return accessToken; + } } catch (std::exception const&) { - // parse() has thrown, we may throw later. + // expiresIn.get() has thrown, we may throw later. + } + } + + if (expiresOn.is_string()) + { + auto const expiresOnAsString = expiresOn.get(); + for (auto const& parse : { + std::function([](auto const& s) { + // 'expires_on' as RFC3339 date string (absolute timestamp) + return DateTime::Parse(s, DateTime::DateFormat::Rfc3339); + }), + std::function([](auto const& s) { + // 'expires_on' as numeric string (posix time representing an absolute timestamp) + return PosixTimeConverter::PosixTimeToDateTime( + ParseNumericExpiration(s, MaxPosixTimestamp)); + }), + std::function([](auto const& s) { + // 'expires_on' as RFC1123 date string (absolute timestamp) + return DateTime::Parse(s, DateTime::DateFormat::Rfc1123); + }), + }) + { + try + { + accessToken.ExpiresOn = parse(expiresOnAsString); + return accessToken; + } + catch (std::exception const&) + { + // parse() has thrown, we may throw later. + } } } } } ThrowJsonPropertyError( - expiresOnPropertyName, + nonEmptyExpiresOnPropertyNames.back(), parsedJson, accessTokenPropertyName, expiresInPropertyName, - expiresOnPropertyName); + nonEmptyExpiresOnPropertyNames); } namespace { @@ -433,7 +449,7 @@ std::string TokenAsDiagnosticString( json const& jsonObject, std::string const& accessTokenPropertyName, std::string const& expiresInPropertyName, - std::string const& expiresOnPropertyName) + std::vector const& expiresOnPropertyNames) { std::string result = "Please report an issue with the following details:\nToken JSON"; @@ -456,22 +472,29 @@ std::string TokenAsDiagnosticString( : PrintSanitizedJsonObject(accessTokenProperty, false); } - for (auto const& p : { - std::pair{"relative", &expiresInPropertyName}, - std::pair{"absolute", &expiresOnPropertyName}, - }) { - result += ", "; - result += p.first; - result += " expiration property ('" + *p.second + "'): "; + std::vector> expirationProperties{ + {"relative", &expiresInPropertyName}}; - if (!jsonObject.contains(*p.second)) + for (auto const& ap : expiresOnPropertyNames) { - result += "undefined"; + expirationProperties.emplace_back(std::make_pair("absolute", &ap)); } - else + + for (auto const& p : expirationProperties) { - result += PrintSanitizedJsonObject(jsonObject[*p.second], true); + result += ", "; + result += p.first; + result += " expiration property ('" + *p.second + "'): "; + + if (!jsonObject.contains(*p.second)) + { + result += "undefined"; + } + else + { + result += PrintSanitizedJsonObject(jsonObject[*p.second], true); + } } } @@ -479,7 +502,8 @@ std::string TokenAsDiagnosticString( 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(); } diff --git a/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp b/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp index da4853882f..e13c71e7b2 100644 --- 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; 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 ab07a4c0ae..58c29e73d4 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 @@ -872,7 +872,7 @@ TEST(TokenCredentialImpl, Diagnosability) "}", "TokenForAccessing", "TokenExpiresInSeconds", - {})); + std::string{})); } catch (std::exception const& e) { @@ -894,7 +894,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); @@ -945,6 +944,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(TokenCredentialImpl::ParseToken( + "{\"TokenForAccessing\":\"ACCESSTOKEN\"," + "\"TokenExpiresInSeconds\":null," + "\"TokenExpiresAtTime\":null," + "\"token_expires_at_time\":null" + "}", + "TokenForAccessing", + "TokenExpiresInSeconds", + std::vector{"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. { @@ -2465,3 +2521,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>; + + 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(TokenCredentialImpl::ParseToken( + "{\"token\": \"X\", \"expires_at\": 1700692424}", + "token", + "expires_in", + std::vector{})); + } + 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(TokenCredentialImpl::ParseToken( + "{\"token\": \"X\", \"expires_at\": 1700692424}", + "token", + "expires_in", + std::vector{"", "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); + } +}