From 0b5bace2f77f39d822c3aeb1e54eed834b0b2ad1 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 3 Mar 2021 15:00:45 +0100 Subject: [PATCH] Bugfix: Members not locked out after failed login attempts (#9921) * https://github.com/umbraco/Umbraco-CMS/issues/9861 Added test that shows the error * https://github.com/umbraco/Umbraco-CMS/issues/9861 Fixed test that shows the error. Caches needs to be active, otherwise it do not show the error * https://github.com/umbraco/Umbraco-CMS/issues/9861 More fix of test that shows the error. Caches needs to be active, otherwise it do not show the error * https://github.com/umbraco/Umbraco-CMS/issues/9861 Fix issue by raise event and thereby update caches. Co-authored-by: Elitsa Marinovska (cherry picked from commit 1bac97e0b042ed3096cc49dcdc37cdde74f00fee) --- .../MembersMembershipProviderTests.cs | 114 ++++++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + .../Providers/UmbracoMembershipProvider.cs | 18 ++- 3 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 src/Umbraco.Tests/Membership/MembersMembershipProviderTests.cs diff --git a/src/Umbraco.Tests/Membership/MembersMembershipProviderTests.cs b/src/Umbraco.Tests/Membership/MembersMembershipProviderTests.cs new file mode 100644 index 0000000000..fd87330971 --- /dev/null +++ b/src/Umbraco.Tests/Membership/MembersMembershipProviderTests.cs @@ -0,0 +1,114 @@ +using System.Collections.Specialized; +using System.Web.Security; +using Moq; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Cache; +using Umbraco.Core.Composing; +using Umbraco.Core.Logging; +using Umbraco.Core.Models; +using Umbraco.Core.Services; +using Umbraco.Core.Sync; +using Umbraco.Tests.Integration; +using Umbraco.Tests.TestHelpers; +using Umbraco.Tests.TestHelpers.Entities; +using Umbraco.Tests.Testing; +using Umbraco.Web; +using Umbraco.Web.Cache; +using Umbraco.Web.Security.Providers; + +namespace Umbraco.Tests.Membership +{ + [TestFixture] + [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerFixture)] + public class MembersMembershipProviderTests : TestWithDatabaseBase + { + private MembersMembershipProvider MembersMembershipProvider { get; set; } + private IDistributedCacheBinder DistributedCacheBinder { get; set; } + + public IMemberService MemberService => Current.Factory.GetInstance(); + public IMemberTypeService MemberTypeService => Current.Factory.GetInstance(); + public ILogger Logger => Current.Factory.GetInstance(); + + public override void SetUp() + { + base.SetUp(); + + MembersMembershipProvider = new MembersMembershipProvider(MemberService, MemberTypeService); + + MembersMembershipProvider.Initialize("test", new NameValueCollection { { "passwordFormat", MembershipPasswordFormat.Clear.ToString() } }); + + DistributedCacheBinder = new DistributedCacheBinder(new DistributedCache(), Mock.Of(), Logger); + DistributedCacheBinder.BindEvents(true); + } + + [TearDown] + public void Teardown() + { + DistributedCacheBinder?.UnbindEvents(); + DistributedCacheBinder = null; + } + + protected override void Compose() + { + base.Compose(); + + // the cache refresher component needs to trigger to refresh caches + // but then, it requires a lot of plumbing ;( + // FIXME: and we cannot inject a DistributedCache yet + // so doing all this mess + Composition.RegisterUnique(); + Composition.RegisterUnique(f => Mock.Of()); + Composition.WithCollectionBuilder() + .Add(() => Composition.TypeLoader.GetCacheRefreshers()); + } + + protected override AppCaches GetAppCaches() + { + // this is what's created core web runtime + return new AppCaches( + new DeepCloneAppCache(new ObjectCacheAppCache()), + NoAppCache.Instance, + new IsolatedCaches(type => new DeepCloneAppCache(new ObjectCacheAppCache()))); + } + + /// + /// MembersMembershipProvider.ValidateUser is expected to increase the number of failed attempts and also read that same number. + /// + /// + /// This test requires the caching to be enabled, as it already is correct in the database. + /// Shows the error described here: https://github.com/umbraco/Umbraco-CMS/issues/9861 + /// + [Test] + public void ValidateUser__must_lock_out_users_after_max_attempts_of_wrong_password() + { + // Arrange + IMemberType memberType = MockedContentTypes.CreateSimpleMemberType(); + ServiceContext.MemberTypeService.Save(memberType); + var member = MockedMember.CreateSimpleMember(memberType, "test", "test@test.com", "password","test"); + ServiceContext.MemberService.Save(member); + + var wrongPassword = "wrongPassword"; + var numberOfFailedAttempts = MembersMembershipProvider.MaxInvalidPasswordAttempts+2; + + // Act + var memberBefore = ServiceContext.MemberService.GetById(member.Id); + for (int i = 0; i < numberOfFailedAttempts; i++) + { + MembersMembershipProvider.ValidateUser(member.Username, wrongPassword); + } + var memberAfter = ServiceContext.MemberService.GetById(member.Id); + + // Assert + Assert.Multiple(() => + { + Assert.AreEqual(0 , memberBefore.FailedPasswordAttempts, "Expected 0 failed password attempts before"); + Assert.IsFalse(memberBefore.IsLockedOut, "Expected the member NOT to be locked out before"); + + Assert.AreEqual(MembersMembershipProvider.MaxInvalidPasswordAttempts, memberAfter.FailedPasswordAttempts, "Expected exactly the max possible failed password attempts after"); + Assert.IsTrue(memberAfter.IsLockedOut, "Expected the member to be locked out after"); + }); + + } + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index e6f273301f..8d44c03602 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -132,6 +132,7 @@ + diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index bf9ee654c4..ad53949c28 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -3,7 +3,6 @@ using System.Collections.Specialized; using System.Configuration.Provider; using System.Linq; using System.Text; -using System.Web; using System.Web.Configuration; using System.Web.Security; using Umbraco.Core; @@ -14,7 +13,6 @@ using Umbraco.Core.Persistence.Querying; using Umbraco.Core.Security; using Umbraco.Core.Services; using Umbraco.Web.Composing; -using Umbraco.Core.Models.Identity; namespace Umbraco.Web.Security.Providers { @@ -357,7 +355,7 @@ namespace Umbraco.Web.Security.Providers member.LastLoginDate = now; member.UpdateDate = now; } - + } return ConvertToMembershipUser(member); @@ -604,11 +602,21 @@ namespace Umbraco.Web.Security.Providers { // when upgrading from 7.2 to 7.3 trying to save will throw if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.Save(member, false); + { + // We need to raise event to ensure caches are updated. (e.g. the cache that uses username as key). + // Even that this is a heavy operation, because indexes are updates, we consider that okay, as it + // is still cheap to do a successful login. + MemberService.Save(member, true); + } + } else { - // set the last login date without full save (fast, no locks) + // set the last login date without full save (fast, no locks). + // We do not update caches. This is to the best of our knowledge okay, as this info are only stored + // because it is required by the membership provider. + // If we one day have to revisit this, we will most likely need to spilt the events in membership info + // saved and umbraco info saved. We don't want to update indexes etc when it is just membership info that is saved MemberService.SetLastLogin(member.Username, member.LastLoginDate); }