Localize expiration timestamps for device assurance grace period by trevoring-okta · Pull Request #3703 · okta/okta-signin-widget

trevoring-okta · 2024-08-29T06:08:21Z

Description:

Uses Date.toLocaleString() (has extensive browser coverage, including IE11) to render an ISO8601 formatted string in a format that is essentially Okta's short-with-timezone format (e.g. 11/29/2021, 4:15 PM EDT)
Uses the languageCode widget prop/setting to pass the client's locale to the function

PR Checklist

Have you verified the basic functionality for this change?
Did you add tests, as appropriate, following our Automated Test guidelines?
Did you follow our Security Best Practices?
Did you verify the change by running downstream monolith artifact? (Provide link to build below)
Does this PR include noticeable changes to the UI? (If yes, attach screenshots/video below)

Issue:

OKTA-798277

Reviewers:

Screenshot/Video:

Downstream Monolith Build:

…ace period

denysoblohin-okta · 2024-08-29T07:32:49Z

+        let localizedExpiry;
+        switch (key.split(GRACE_PERIOD_TITLE_KEY)[1]) {
+          case GRACE_PERIOD_DUE_BY_DATE_SUFFIX:
+            localizedExpiry = TimeUtil.formatUnixEpochToDeviceAssuranceGracePeriodDueDate(


Proposal for Gen3: after merging PR to use i18next we can utilize DateTime localization

Oh nice, I didn't know this was inflight. Looks like it would work well, I'll make a follow up ticket and include it as a comment

Internal Ref: OKTA-798446

trevoring-okta · 2024-08-29T16:14:07Z

  const stepper: StepperLayout = {
    type: UISchemaLayoutType.STEPPER,
-    key: 'stepper_' + channelType,
+    key: `stepper_${channelType}`,


This was pulled in as an automatic lint fix. I'll leave it here but it's unrelated to my actual code changes

trevoring-okta · 2024-09-03T21:49:30Z

-# {0} is the number of days until the grace period expires
-idx.device_assurance.grace_period.warning.title.due_by_days = Your device doesn't meet the security requirements. Fix the issue within {0} days to prevent lockout.


We are consolidating to a single string for the expiration message. This can get deleted in the next translation run

trevoring-okta · 2024-09-03T21:56:41Z

      "value": [
          {
-            "message": "Your device doesn't meet the security requirements. Fix the issue within 7 days to prevent lockout.",
+            "message": "Your device doesn't meet the security requirements. Fix the issue by 09/05/2024, 12:00 AM UTC to prevent lockout.",


Update: we will send a localized string that specifies UTC as the timezone for this message field to still provide a semantically correct expiry string from the server. This will cover cases where we might have a L10N error and client-side loc() fails.

E.g. For this specific mock, the server will send an ISO8601 format string of "2024-09-05T00:00:00.000Z" as the param and the message field will be that same string localized to UTC rather than the client's timezone -> "09/05/2024, 12:00 AM UTC"

This is a good thought to have the fallback, although does the code currently handle if a call to loc returns L10N_ERROR well?

You're right, I just checked and it will still render L10N_ERROR as the string. The main case I'm looking to add coverage for is something going wrong in the TimeUtil function when creating the localized expiry param. I'll add a check to use the fallback message if the param comes back falsy because of an invalid Date object.

Digging into this more, I don't think this fallback message with the server-side localized string will ever actually get rendered to the SIW because the generic transformer that localizes all IDX transaction messages will always run before my grace period code is hit, so the actual fallback string users might see is "Your device doesn't meet the security requirements. Fix the issue by 2024-09-05T00:00:00.000Z to prevent lockout." with the ISO8601 string as the parameter before it's processed into the ideal format via TimeUtil. Regardless, I think having this be the server-side message in the IDX payload is better than what I originally had, which was a Unix timestamp

shawyu-okta · 2024-09-03T22:22:23Z


 test.requestHooks(oneOptionMock)('should render correct messaging for grace period with one option', async t => {
  const deviceAssuranceGracePeriodPage = await setup(t);
+  const expiryLocaleString = TimeUtil.formatDateToDeviceAssuranceGracePeriodExpiryLocaleString(new Date('2024-09-05T00:00:00.000Z'));


Is there any way to avoid using the TimeUtil here in the test as well? I assume you are doing this so the string is always formatted for the TZ of the machine running the test rather than using a static string. Not ideal, but could be acceptable if we're unit testing formatDateToDeviceAssuranceGracePeriodExpiryLocaleString well

There might be ways to override the timezone in testcafe, although forcing a timezone in a unit test is a lot easier.

Yeah looks like TestCafe doesn't have a built-in way to enforce a specific timezone. During my first pass at this PR I was struggling to set a hard-coded timezone for Jest, but I just found a solution that works. Will add some unit tests

shawyu-okta · 2024-09-03T22:27:20Z

+   * @param {LanguageCode} languageCode The user's language code / locale
+   * @return {String} The formatted `short-with-timezone` local string
+   */
+  formatDateToDeviceAssuranceGracePeriodExpiryLocaleString: (date, languageCode) => date.toLocaleString(languageCode, {


Do we need to do any input validation or checks for null/undefined?

Will move the check for an invalid Date object in here 👍 I just checked and toLocaleString() handles null/undefined values for the locale param so we shouldn't need to validate languageCode

Actually looks like toLocaleString() only handles undefined. Will add some validation for other cases

shawyu-okta · 2024-09-03T22:28:11Z

+   * @param {LanguageCode} languageCode The user's language code / locale
+   * @return {String} The formatted `short-with-timezone` local string
+   */
+  formatDateToDeviceAssuranceGracePeriodExpiryLocaleString: (date, languageCode) => date.toLocaleString(languageCode, {


Related but separate: We might want to add a few simple unit tests here to show our expected output.

Unit tests aren't super interesting besides defining behavior since this is just wrapping toLocaleString but you might consider moving some more of the fallback and business logic into this utility, which would make the unit tests more useful and also would test the fallback behavior you're expecting.

lesterchoi-okta · 2024-09-04T16:07:05Z

+        month: '2-digit',
+        day: '2-digit',
+        hour: 'numeric',
+        minute: 'numeric',


I thought you were truncating the minute -- or maybe you meant you'd round down? I thought minute: 'numeric' would show a single digit for values <10m after the hour, but it seems to be overridden by one of the other values

Ah, yeah we're rounding down, though I actually forgot to enforce that. I looked up why numeric doesn't show a single digit value and apparently that happens for specific combinations of these options, but to be safe I'll use '2-digit' for hour and minute

trevoring-okta added 2 commits August 28, 2024 23:55

Localize expiration timestamps on client side for device assurance gr…

126e7de

…ace period

Add unit tests

fce3c3e

trevoring-okta marked this pull request as ready for review August 29, 2024 06:57

trevoring-okta requested review from denysoblohin-okta, glenfannin-okta, jaredperreault-okta, leemchale-okta, lesterchoi-okta, robertdamphousse-okta, shawyu-okta, shuowu-okta and vijetmahabaleshwar-okta as code owners August 29, 2024 06:57

denysoblohin-okta reviewed Aug 29, 2024

View reviewed changes

Comment thread playground/mocks/data/idp/idx/device-assurance-grace-period-due-by-date.json Outdated

trevoring-okta commented Aug 29, 2024

View reviewed changes

trevoring-okta added 2 commits August 29, 2024 12:23

Add follow up ticket comment to migrate gen3 to use i18next datetime loc

229a85e

Switch to usng browser native toLocaleString()

18c0cf4

trevoring-okta commented Sep 3, 2024

View reviewed changes

Lint fix

657adcb

shawyu-okta reviewed Sep 3, 2024

View reviewed changes

Address PR comments, add unit test coverage

a7848fe

lesterchoi-okta previously approved these changes Sep 4, 2024

View reviewed changes

Add rounding to nearest hour

ab0faef

trevoring-okta dismissed lesterchoi-okta’s stale review via ab0faef September 4, 2024 16:55

lesterchoi-okta approved these changes Sep 4, 2024

View reviewed changes

shawyu-okta approved these changes Sep 4, 2024

View reviewed changes

oktapp-aperture-okta Bot merged commit e3e2ad9 into master Sep 4, 2024

oktapp-aperture-okta Bot deleted the ti-OKTA-798277-localize-gp-expiry branch September 4, 2024 20:03

		# {0} is the number of days until the grace period expires
		idx.device_assurance.grace_period.warning.title.due_by_days = Your device doesn't meet the security requirements. Fix the issue within {0} days to prevent lockout.

Uh oh!

Conversation

trevoring-okta commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

PR Checklist

Issue:

Reviewers:

Screenshot/Video:

Downstream Monolith Build:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevoring-okta Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawyu-okta Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

trevoring-okta commented Aug 29, 2024 •

edited

Loading

trevoring-okta Sep 4, 2024 •

edited

Loading

shawyu-okta Sep 3, 2024 •

edited

Loading