Skip to content

Do not convert calculated lockedOutUntil time to UTC#17007

Merged
AndyButland merged 2 commits intov13/devfrom
v13/fix/utc-lockout-date
Oct 8, 2025
Merged

Do not convert calculated lockedOutUntil time to UTC#17007
AndyButland merged 2 commits intov13/devfrom
v13/fix/utc-lockout-date

Conversation

@Migaroez
Copy link
Contributor

@Migaroez Migaroez commented Sep 4, 2024

Prerequisites

  • I have added steps to test this contribution in the description below

Fixes #16988

Description

Back when Identity was upgraded to identity core, an oversight was made in the code conversion that ended up in a UTC time being stored inside a dateTimeOffset which results in Identity LockoutEnd value being set to values that do not correspond to the intend with the following timezone ranges

  • For timezones in the +X range this results in lockouts taking longer than intended.
  • For timezones in the -X range this results in a lockout never happening if the offset was bigger than the configured DefaultLockoutTimeInMinutes.

Testing

  • Set the Umbraco::CMS::Security::MemberDefaultLockoutTimeInMinutes setting to a managable time (like 1 minute)
  • Implement a member login/logout flow, member group, member and protected content
  • Lock the member out by trying to login with an incorrect password equal to Umbraco::CMS::Security::MemberPassword::MaxFailedAccessAttemptsBeforeLockout setting
  • Check that the behavior in the description no longer happens and the lockout is lifted correctly on the next correct login attempt after the LockoutTimeInMinutes has passed. (and not before)

@AndyButland
Copy link
Contributor

I had a look at this one @Migaroez as can see it's not been reviewed yet. Unfortunately I couldn't get it to work for me.

What I found was that if I run locally - so local time is CET (i.e. UTC + 1) - if I was logged out as a member I couldn't login again after the one minute configured MemberDefaultLockoutTimeInMinutes had passed. It seemed I would have to wait for one hour and one minute.

The reason I believe is that the lockout end, defined on Microsoft's IdentityUser is meant to be in UTC (see comment):

    /// <summary>
    /// Gets or sets the date and time, in UTC, when any user lockout ends.
    /// </summary>
    public virtual DateTimeOffset? LockoutEnd { get; set; }

So that seems like the original code was correct in trying to set this to universal time.

The problem seemed to be that the dates set on IMember that we are mapping from here are server time, but they have a Kind set of Utc. Hence the call to .ToUniveralTime() wasn't doing anything.

I've amended this to explicitly set the kind to local before doing the conversion to UTC, and that seems to work.

My expectation is that this is kind of a plaster on the problem that we are getting these "invalid" dates here (i.e. server time, but with a Kind of Utc), but it does look to resolve the issue for these dates that have to be correctly converted to UTC for the lockout feature to work.

Can you see what you think please? And re-verify the fix?

@Migaroez Migaroez force-pushed the v13/fix/utc-lockout-date branch from e9de110 to ce097df Compare October 8, 2025 08:59
@Migaroez
Copy link
Contributor Author

Migaroez commented Oct 8, 2025

Seems to work perfectly now, thx @AndyButland

Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I'll approve and merge then. Thanks for checking it over again.

@AndyButland AndyButland merged commit e840a0c into v13/dev Oct 8, 2025
19 checks passed
@AndyButland AndyButland deleted the v13/fix/utc-lockout-date branch October 8, 2025 14:15
AndyButland added a commit that referenced this pull request Oct 10, 2025
AndyButland added a commit that referenced this pull request Oct 10, 2025
…17007 for 16 (#20441)

* Port PR #17007

* Update src/Umbraco.Infrastructure/Security/IdentityMapDefinition.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants