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.

Sign up for GitHub

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

Jump to bottom

Fix Date-Time Parsing in GetDurationFromNowInSeconds for Multiple Formats #4964

Merged

gladjohn merged 5 commits into main from gladjohn/expires_on_fix

Oct 22, 2024

Contributor

gladjohn commented Oct 21, 2024 •

edited

Loading

Fixes #4963

Changes proposed in this request
This PR add a new GetDurationFromNowInSeconds specific to MI in MSAL to handle multiple date-time formats while calculating the duration from now. Specifically, the fix:

Adds support for Unix timestamps, ISO 8601 and other common date-time formats.
Adds improved error handling for unsupported or invalid date formats by throwing an MsalClientException.
Ensures that fallback logic only throws an exception after attempting valid date-time parsing strategies.

Testing

Added additional test cases to validate handling of various date-time formats including Unix timestamps, ISO 8601, and common date-time formats (MM/dd/yyyy HH:mm:ss, yyyy-MM-dd HH:mm:ss).
Added a test case for invalid date-time formats to ensure that the method throws an MsalClientException for malformed input.
Extended the Managed Identity tests as well

Performance impact
None

Documentation

All relevant documentation is updated.


          init

838ea28

gladjohn requested a review from a team as a code owner

October 21, 2024 16:36


          dateTimeStamp

846bc54

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ManagedIdentityTests.cs Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ManagedIdentityTests.cs Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/DateTimeHelpers.cs Show resolved Hide resolved

bgavrilMS requested changes

View reviewed changes

Member

bgavrilMS left a comment

The unit test doesn't seem ok.

fix

791b8b6

bgavrilMS requested changes

View reviewed changes

Member

bgavrilMS left a comment

Not fixed.

fix

984affa

bgavrilMS reviewed

View reviewed changes

src/client/Microsoft.Identity.Client/OAuth2/MsalTokenResponse.cs Outdated Show resolved Hide resolved

bgavrilMS approved these changes

View reviewed changes


          pr comment

dec2811

gladjohn merged commit afb2fa9 into main

5 checks passed

gladjohn deleted the gladjohn/expires_on_fix branch

October 22, 2024 14:44

Member

bgavrilMS commented Jan 2, 2025

@gladjohn - was this bug fixed in all other MSALs?

Contributor Author

gladjohn commented Jan 2, 2025

@gladjohn - was this bug fixed in all other MSALs?

no, we have these bugs still in open state

AzureAD/microsoft-authentication-library-for-python#760
AzureAD/microsoft-authentication-library-for-js#7393
AzureAD/microsoft-authentication-library-for-java#873

FYI @Robbie-Microsoft @Avery-Dunn @rayluo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet