TimedScope improvements and login duration clean-up

[ronaldbarendse]

Prerequisites

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

Description

Although the changes in the latest (security) patches do fix the issue, I noticed TimedScope (backported from 65bb280 or 839b681) doesn't dispose the CancellationTokenSource that's created in the constructors. Also, CancellationToken.None was used in the login method, instead of using HttpContext.RequestAborted (which would be the same as adding a new action parameter, see Model Binding in ASP.NET Core): this ensures the server stops waiting as soon as the request is aborted.

The UserDefaultFailedLoginDurationInMilliseconds and UserMinimumFailedLoginDurationInMilliseconds settings are now also validated to ensure they aren't set to negative values and I've removed the randomization of the average login duration. Although the idea of adding 'noise' to the timings would make it harder to correlate successful and failed responses, the fact that a fixed minimum and calculated average of successful logins is used for failed logins already ensures there's no difference between a non-existent user and failed password check on an existing user...

These changes should be easy to merge up to the latest versions as well (or update the PR base branch to the latest version, if we don't want to release this for v13).

[Copilot]

Pull Request Overview

This pull request addresses improvements to the TimedScope and cleans up the login duration handling, ensuring more consistent timeout behavior and proper resource disposal.

• Removed randomization in login duration calculation by removing the GetLoginDuration method and related constant.
• Updated the login process to use HttpContext.RequestAborted instead of CancellationToken.None.
• Added Range validation attributes to security settings properties and ensured that CancellationTokenSource is disposed in TimedScope.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs	Removed random offset and updated login duration calculation; now using HttpContext.RequestAborted.
src/Umbraco.Core/TimedScope.cs	Added disposal of _cancellationTokenSource in both synchronous and asynchronous dispose methods.
src/Umbraco.Core/Configuration/Models/SecuritySettings.cs	Added [Range] attributes to ensure non-negative login duration settings.

Comments suppressed due to low confidence (1)

src/Umbraco.Core/TimedScope.cs:137

• If there is any chance that _cancellationTokenSource could be null, consider adding a null-check before disposing it to avoid potential runtime exceptions.

_cancellationTokenSource.Dispose();

[AndyButland]

Thanks @ronaldbarendse - we can certainly bring some of this in for 13.9, which we are planning in a few weeks. Could you see what you think on my comments on a couple of your suggestions though please (I've added some context of why the differences from the 14+ solution you came up with were made).

The randomness was added as in testing we could see a small but perceptible difference between the two login situations (with a valid account and without).

[AndyButland]

Thanks for this @ronaldbarendse - I'll merge in and include for the next release.

I re-tested and agree we don't need the randomness. It seems we did in earlier versions as we found some repeatable discrepancy, but for 13 it doesn't appear to be necessary.