Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AzureCliCredential to treat datetime without TZ info as local time. #5179

Merged
merged 12 commits into from
Dec 16, 2023
1 change: 1 addition & 0 deletions sdk/core/azure-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
### Other Changes

- [[#4756]] (https://github.com/Azure/azure-sdk-for-cpp/issues/4756) `BearerTokenAuthenticationPolicy` now uses shared mutex lock for read operations.
- Added internal provisions for extended `Azure::DateTime` parsing.
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

## 1.11.0-beta.2 (2023-11-02)

Expand Down
64 changes: 63 additions & 1 deletion sdk/core/azure-core/inc/azure/core/datetime.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ namespace _detail {
};
} // namespace _detail

namespace Core { namespace _internal {
class DateTimeExtensions;
}} // namespace Core::_internal

/**
* @brief Manages date and time in standardized string formats.
* @details Supports date range from year 0001 to end of year 9999 with 100ns (7 decimal places
Expand All @@ -52,6 +56,7 @@ namespace _detail {
* @remark This class is supposed to be able to handle a DateTime that comes over the wire.
*/
class DateTime final : public _detail::Clock::time_point {
friend class Core::_internal::DateTimeExtensions;

private:
AZ_CORE_DLLEXPORT static DateTime const SystemClockEpoch;
Expand Down Expand Up @@ -168,6 +173,14 @@ class DateTime final : public _detail::Clock::time_point {
Rfc3339,
};

private:
static DateTime Parse(
std::string const& dateTime,
DateFormat format,
bool rfc3339NoTimeZoneMeansLocal,
int const* localTimeToUtcDiffSeconds);

public:
/**
* @brief Create #Azure::DateTime from a string representing time in UTC in the specified
* format.
Expand All @@ -179,7 +192,10 @@ class DateTime final : public _detail::Clock::time_point {
*
* @throw std::invalid_argument If \p format is not recognized, or if parsing error.
*/
static DateTime Parse(std::string const& dateTime, DateFormat format);
static DateTime Parse(std::string const& dateTime, DateFormat format)
{
return Parse(dateTime, format, false, nullptr);
}

/**
* @brief Get a string representation of the #Azure::DateTime.
Expand Down Expand Up @@ -324,6 +340,52 @@ namespace Core { namespace _internal {
*/
~PosixTimeConverter() = delete;
};

/**
* @brief Provides additional methods for #Azure::DateTime.
*
*/
class DateTimeExtensions final {
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
public:
/**
* @brief Create #Azure::DateTime from a string representing time in UTC in the specified
* format.
*
* @param dateTime A string with the date and time.
* @param format A format to which \p dateTime string adheres to.
* @param rfc3339NoTimeZoneMeansLocal If time zone is not specified, treat the \p dateTime as
* local time.
* @param localTimeToUtcDiffSeconds Assume this offset in seconds between local time and UTC
* time, for unit test reproduceability. Detect current time zone if `nullptr`.
*
* @return #Azure::DateTime that was constructed from the \p dateTime string.
*
* @throw std::invalid_argument If \p format is not recognized, or if parsing error.
*/
static DateTime Parse(
std::string const& dateTime,
DateTime::DateFormat format,
bool rfc3339NoTimeZoneMeansLocal,
int const* localTimeToUtcDiffSeconds)
{
return DateTime::Parse(
dateTime, format, rfc3339NoTimeZoneMeansLocal, localTimeToUtcDiffSeconds);
}

private:
/**
* @brief An instance of `%DateTimeExtensions` class cannot be created.
*
*/
DateTimeExtensions() = delete;

/**
* @brief An instance of `%DateTimeExtensions` class cannot be destructed, because no instance
* can be created.
*
*/
~DateTimeExtensions() = delete;
};
}} // namespace Core::_internal

} // namespace Azure
41 changes: 40 additions & 1 deletion sdk/core/azure-core/src/datetime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,11 @@ DateTime::operator std::chrono::system_clock::time_point() const
+ std::chrono::duration_cast<std::chrono::system_clock::duration>(*this - SystemClockEpoch);
}

DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)
DateTime DateTime::Parse(
std::string const& dateTime,
DateFormat format,
bool rfc3339NoTimeZoneMeansLocal,
int const* localTimeToUtcDiffSeconds)
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
{
// The values that are not supposed to be read before they are written are set to -123... to avoid
// warnings on some compilers, yet provide a clearly bad value to make it obvious if things don't
Expand All @@ -464,6 +468,7 @@ DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)
int8_t localDiffHours = 0;
int8_t localDiffMinutes = 0;
bool roundFracSecUp = false;
bool adjustAsLocal = false;
{
std::string::size_type const DateTimeLength = dateTime.length();
std::string::size_type minDateTimeLength = 0;
Expand Down Expand Up @@ -721,6 +726,16 @@ DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)
&cursor, dateTime, DateTimeLength, parsingArea, 2, 2);
}
}
else if (rfc3339NoTimeZoneMeansLocal) // Fractional seconds were present, but not the time
// zone
{
adjustAsLocal = true;
}
}
else if ( // No fractional seconds and no time zone were present
rfc3339NoTimeZoneMeansLocal && cursor == DateTimeLength)
{
adjustAsLocal = true;
}
}
}
Expand All @@ -729,6 +744,30 @@ DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)
throw std::invalid_argument("Unrecognized date format.");
}

if (adjustAsLocal)
{
auto const timeTNow = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());

#ifdef _MSC_VER
#pragma warning(push)
// warning C4996: 'localtime': This function or variable may be unsafe. Consider using localtime_s
// instead.
#pragma warning(disable : 4996)
#endif
// LCOV_EXCL_START
auto const utcDiffSeconds = localTimeToUtcDiffSeconds != nullptr
? *localTimeToUtcDiffSeconds
: static_cast<int>(std::difftime(
std::mktime(std::localtime(&timeTNow)), std::mktime(std::gmtime(&timeTNow))));
// LCOV_EXCL_STOP
#ifdef _MSC_VER
#pragma warning(pop)
#endif

localDiffHours = static_cast<int8_t>(utcDiffSeconds / (60 * 60));
localDiffMinutes = static_cast<int8_t>((utcDiffSeconds % (60 * 60)) / 60);
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
}

return DateTime(
year,
month,
Expand Down
161 changes: 161 additions & 0 deletions sdk/core/azure-core/test/ut/datetime_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -902,3 +902,164 @@ TEST(DateTime, Rfc3339Space)
= DateTime::Parse("2022-08-24 00:43:08.0004308Z", DateTime::DateFormat::Rfc3339);
EXPECT_EQ(datetime.ToString(DateTime::DateFormat::Rfc3339), "2022-08-24T00:43:08.0004308Z");
}

TEST(DateTime, LocalTime)
{
using Azure::Core::_internal::DateTimeExtensions;

{
auto const uniformOffsetSeconds = -28800; // Redmond (no DST)

// No fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, true, &uniformOffsetSeconds)
.ToString(),
"2023-11-23T02:33:44Z"); // Local time adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, false, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // no adjustment

// With fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, true, &uniformOffsetSeconds)
.ToString(),
"2023-11-23T02:33:44.4308Z"); // Local time adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, false, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // no adjustment

// No fractions, with time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44-05:00", DateTime::DateFormat::Rfc3339, true, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T23:33:44Z"); // no local time adjustment, just the regular time zone adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44-05:00",
DateTime::DateFormat::Rfc3339,
false,
&uniformOffsetSeconds)
.ToString(),
"2023-11-22T23:33:44Z"); // no local time adjustment, just the regular time zone adjustment

// With fractions, with time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308-05:00",
DateTime::DateFormat::Rfc3339,
true,
&uniformOffsetSeconds)
.ToString(),
"2023-11-22T23:33:44.4308Z"); // no local time adjustment, just the regular time zone
// adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308-05:00",
DateTime::DateFormat::Rfc3339,
false,
&uniformOffsetSeconds)
.ToString(),
"2023-11-22T23:33:44.4308Z"); // no local time adjustment, just the regular time zone
// adjustment

// No fractions, Zulu time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44Z", DateTime::DateFormat::Rfc3339, true, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // no adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44Z", DateTime::DateFormat::Rfc3339, false, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // no adjustment

// With fractions, Zulu time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308Z", DateTime::DateFormat::Rfc3339, true, &uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // no adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308Z",
DateTime::DateFormat::Rfc3339,
false,
&uniformOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // no adjustment
}

{
auto const bravoOffsetSeconds = 7200; // Kyiv (no DST)

// No fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, true, &bravoOffsetSeconds)
.ToString(),
"2023-11-22T16:33:44Z"); // Local time adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, false, &bravoOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // no adjustment

// With fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, true, &bravoOffsetSeconds)
.ToString(),
"2023-11-22T16:33:44.4308Z"); // Local time adjustment

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, false, &bravoOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // no adjustment
}

{
auto const zuluOffsetSeconds = 0; // London (no DST)

// No fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, true, &zuluOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // Local time adjustment, no effect

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44", DateTime::DateFormat::Rfc3339, false, &zuluOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44Z"); // no adjustment

// With fractions, no time zone
EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, true, &zuluOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // Local time adjustment, no effect

EXPECT_EQ(
DateTimeExtensions::Parse(
"2023-11-22T18:33:44.4308", DateTime::DateFormat::Rfc3339, false, &zuluOffsetSeconds)
.ToString(),
"2023-11-22T18:33:44.4308Z"); // no adjustment
}
}
2 changes: 2 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

- [[#5075]](https://github.com/Azure/azure-sdk-for-cpp/issues/5075) `AzureCliCredential` assumes token expiration time without local time zone adjustment.

### 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ namespace Azure { namespace Identity {
protected:
#endif
virtual std::string GetAzCommand(std::string const& scopes, std::string const& tenantId) const;
virtual int const* GetLocalTimeToUtcDiffSeconds() const { return nullptr; }
};

}} // namespace Azure::Identity
7 changes: 6 additions & 1 deletion sdk/identity/azure-identity/src/azure_cli_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,12 @@ AccessToken AzureCliCredential::GetToken(
try
{
return TokenCredentialImpl::ParseToken(
azCliResult, "accessToken", "expiresIn", "expiresOn");
azCliResult,
"accessToken",
"expiresIn",
"expiresOn",
true,
GetLocalTimeToUtcDiffSeconds());
}
catch (json::exception const&)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ namespace Azure { namespace Identity { namespace _detail {
* @param expiresOnPropertyName Name of a property in the JSON object that represents token
* expiration as absolute date-time stamp. Can be empty, in which case no attempt to parse it is
* made.
* @param rfc33339NoTimeZoneMeansLocal If an RFC3339 datetime has no time zone information,
* treat it as local time.
* @param localTimeToUtcDiffSeconds Assume this offset in seconds between local time and UTC
* time, for unit test reproduceability. Detect current time zone if `nullptr`.
*
* @return A successfully parsed access token.
*
Expand All @@ -79,7 +83,9 @@ 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,
bool rfc33339NoTimeZoneMeansLocal = false,
int const* localTimeToUtcDiffSeconds = nullptr);

/**
* @brief Holds `#Azure::Core::Http::Request` and all the associated resources for the HTTP
Expand Down
Loading