From d3dc85f31fe632d7f8c64e7ccafba43f8287a850 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 11 Jan 2024 10:20:58 +0100 Subject: [PATCH] Only update security stamp once per request (#15562) * Add item in requestcache when security stamp is already updated in request * Propagate constructur obsoletion to implementing services and fix unit tests --------- Co-authored-by: kjac --- .../Security/BackOfficeSignInManager.cs | 35 +++++++++++++++- .../Security/MemberSignInManager.cs | 33 ++++++++++++++- .../Security/UmbracoSignInManager.cs | 40 +++++++++++++++++-- .../Security/MemberSignInManagerTests.cs | 4 +- 4 files changed, 104 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 8af883135d..7448a303a0 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Events; @@ -37,8 +38,9 @@ public class BackOfficeSignInManager : UmbracoSignInManager confirmation, IEventAggregator eventAggregator, - IOptions securitySettings) - : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation, securitySettings) + IOptions securitySettings, + IRequestCache requestCache) + : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation, securitySettings, requestCache) { _userManager = userManager; _externalLogins = externalLogins; @@ -46,6 +48,35 @@ public class BackOfficeSignInManager : UmbracoSignInManager claimsFactory, + IOptions optionsAccessor, + IOptions globalSettings, + ILogger> logger, + IAuthenticationSchemeProvider schemes, + IUserConfirmation confirmation, + IEventAggregator eventAggregator, + IOptions securitySettings) + : this( + userManager, + contextAccessor, + externalLogins, + claimsFactory, + optionsAccessor, + globalSettings, + logger, + schemes, + confirmation, + eventAggregator, + securitySettings, + StaticServiceProvider.Instance.GetRequiredService()) + { + } + [Obsolete("Use non-obsolete constructor. This is scheduled for removal in V14.")] public BackOfficeSignInManager( BackOfficeUserManager userManager, diff --git a/src/Umbraco.Web.Common/Security/MemberSignInManager.cs b/src/Umbraco.Web.Common/Security/MemberSignInManager.cs index 9a8aaa72f4..1139c04270 100644 --- a/src/Umbraco.Web.Common/Security/MemberSignInManager.cs +++ b/src/Umbraco.Web.Common/Security/MemberSignInManager.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Events; @@ -32,13 +33,41 @@ public class MemberSignInManager : UmbracoSignInManager, IMe IUserConfirmation confirmation, IMemberExternalLoginProviders memberExternalLoginProviders, IEventAggregator eventAggregator, - IOptions securitySettings) - : base(memberManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation, securitySettings) + IOptions securitySettings, + IRequestCache requestCache) + : base(memberManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation, securitySettings, requestCache) { _memberExternalLoginProviders = memberExternalLoginProviders; _eventAggregator = eventAggregator; } + [Obsolete("Use non-obsolete constructor. This is scheduled for removal in V15.")] + public MemberSignInManager( + UserManager memberManager, + IHttpContextAccessor contextAccessor, + IUserClaimsPrincipalFactory claimsFactory, + IOptions optionsAccessor, + ILogger> logger, + IAuthenticationSchemeProvider schemes, + IUserConfirmation confirmation, + IMemberExternalLoginProviders memberExternalLoginProviders, + IEventAggregator eventAggregator, + IOptions securitySettings) + : this( + memberManager, + contextAccessor, + claimsFactory, + optionsAccessor, + logger, + schemes, + confirmation, + memberExternalLoginProviders, + eventAggregator, + securitySettings, + StaticServiceProvider.Instance.GetRequiredService()) + { + } + [Obsolete("Use non-obsolete constructor. This is scheduled for removal in V14.")] public MemberSignInManager( UserManager memberManager, diff --git a/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs b/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs index 84cbce6d8d..f52db46241 100644 --- a/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs +++ b/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Security; @@ -19,6 +20,7 @@ namespace Umbraco.Cms.Web.Common.Security; public abstract class UmbracoSignInManager : SignInManager where TUser : UmbracoIdentityUser { + private readonly IRequestCache _requestCache; private SecuritySettings _securitySettings; // borrowed from https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs @@ -44,7 +46,31 @@ public abstract class UmbracoSignInManager : SignInManager logger, schemes, confirmation, - StaticServiceProvider.Instance.GetRequiredService>()) + StaticServiceProvider.Instance.GetRequiredService>(), + StaticServiceProvider.Instance.GetRequiredService()) + { + } + + [Obsolete("Use non-obsolete constructor. This is scheduled for removal in V15.")] + public UmbracoSignInManager( + UserManager userManager, + IHttpContextAccessor contextAccessor, + IUserClaimsPrincipalFactory claimsFactory, + IOptions optionsAccessor, + ILogger> logger, + IAuthenticationSchemeProvider schemes, + IUserConfirmation confirmation, + IOptions securitySettingsOptions) + : this( + userManager, + contextAccessor, + claimsFactory, + optionsAccessor, + logger, + schemes, + confirmation, + securitySettingsOptions, + StaticServiceProvider.Instance.GetRequiredService()) { } @@ -56,9 +82,11 @@ public abstract class UmbracoSignInManager : SignInManager ILogger> logger, IAuthenticationSchemeProvider schemes, IUserConfirmation confirmation, - IOptions securitySettingsOptions) + IOptions securitySettingsOptions, + IRequestCache requestCache) : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation) { + _requestCache = requestCache; _securitySettings = securitySettingsOptions.Value; } @@ -370,7 +398,13 @@ public abstract class UmbracoSignInManager : SignInManager if (_securitySettings.AllowConcurrentLogins is false) { - await UserManager.UpdateSecurityStampAsync(user); + + if (_requestCache.Get("SecurityStampUpdated") is null) + { + await UserManager.UpdateSecurityStampAsync(user); + _requestCache.Set("SecurityStampUpdated", true); + } + } Logger.LogInformation("User: {UserName} logged in from IP address {IpAddress}", username, Context.Connection.RemoteIpAddress); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/Security/MemberSignInManagerTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/Security/MemberSignInManagerTests.cs index 5394de5fd7..b80555bd43 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/Security/MemberSignInManagerTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/Security/MemberSignInManagerTests.cs @@ -10,6 +10,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Net; @@ -71,7 +72,8 @@ public class MemberSignInManagerTests Mock.Of>(), Mock.Of(), Mock.Of(), - Mock.Of>(x => x.Value == new SecuritySettings())); + Mock.Of>(x => x.Value == new SecuritySettings()), + new DictionaryAppCache()); } private static Mock MockMemberManager()