diff --git a/src/Umbraco.Core/Cache/MemberCacheRefresher.cs b/src/Umbraco.Core/Cache/MemberCacheRefresher.cs index 68c89c0bc7..a9a648acff 100644 --- a/src/Umbraco.Core/Cache/MemberCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/MemberCacheRefresher.cs @@ -1,21 +1,37 @@ -using System; +//using Newtonsoft.Json; +using System; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Models; -using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.Repositories.Implement; +using Umbraco.Core.Serialization; using Umbraco.Core.Services; namespace Umbraco.Web.Cache { - public sealed class MemberCacheRefresher : TypedCacheRefresherBase + public sealed class MemberCacheRefresher : PayloadCacheRefresherBase { private readonly IIdKeyMap _idKeyMap; + private readonly LegacyMemberCacheRefresher _legacyMemberRefresher; - public MemberCacheRefresher(AppCaches appCaches, IIdKeyMap idKeyMap) - : base(appCaches) + public MemberCacheRefresher(AppCaches appCaches, IJsonSerializer serializer, IIdKeyMap idKeyMap) + : base(appCaches, serializer) { _idKeyMap = idKeyMap; + _legacyMemberRefresher = new LegacyMemberCacheRefresher(this, appCaches); + } + + public class JsonPayload + { + //[JsonConstructor] + public JsonPayload(int id, string username) + { + Id = id; + Username = username; + } + + public int Id { get; } + public string Username { get; } } #region Define @@ -32,38 +48,45 @@ namespace Umbraco.Web.Cache #region Refresher + public override void Refresh(JsonPayload[] payloads) + { + ClearCache(payloads); + base.Refresh(payloads); + } + public override void Refresh(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Refresh(id); } public override void Remove(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Remove(id); } - public override void Refresh(IMember instance) - { - ClearCache(instance.Id); - base.Refresh(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Refresh(IMember instance) => _legacyMemberRefresher.Refresh(instance); - public override void Remove(IMember instance) - { - ClearCache(instance.Id); - base.Remove(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Remove(IMember instance) => _legacyMemberRefresher.Remove(instance); - private void ClearCache(int id) + private void ClearCache(params JsonPayload[] payloads) { - _idKeyMap.ClearCache(id); AppCaches.ClearPartialViewCache(); - var memberCache = AppCaches.IsolatedCaches.Get(); - if (memberCache) - memberCache.Result.Clear(RepositoryCacheKeys.GetKey(id)); + + foreach (var p in payloads) + { + _idKeyMap.ClearCache(p.Id); + if (memberCache) + { + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Id)); + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Username)); + } + } + } #endregion @@ -76,5 +99,38 @@ namespace Umbraco.Web.Cache } #endregion + + #region Backwards Compat + + // TODO: this is here purely for backwards compat but should be removed in netcore + private class LegacyMemberCacheRefresher : TypedCacheRefresherBase + { + private readonly MemberCacheRefresher _parent; + + public LegacyMemberCacheRefresher(MemberCacheRefresher parent, AppCaches appCaches) : base(appCaches) + { + _parent = parent; + } + + public override Guid RefresherUniqueId => _parent.RefresherUniqueId; + + public override string Name => _parent.Name; + + protected override MemberCacheRefresher This => _parent; + + public override void Refresh(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Refresh(instance.Id); + } + + public override void Remove(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Remove(instance); + } + } + + #endregion } } diff --git a/src/Umbraco.Core/Services/IMembershipMemberService.cs b/src/Umbraco.Core/Services/IMembershipMemberService.cs index d3ca954de8..839d8e3af3 100644 --- a/src/Umbraco.Core/Services/IMembershipMemberService.cs +++ b/src/Umbraco.Core/Services/IMembershipMemberService.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.Querying; @@ -107,6 +108,18 @@ namespace Umbraco.Core.Services /// or to Delete void Delete(T membershipUser); + /// + /// Sets the last login date for the member if they are found by username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); + /// /// Saves an /// diff --git a/src/Umbraco.Examine.Lucene/UmbracoContentIndex.cs b/src/Umbraco.Examine.Lucene/UmbracoContentIndex.cs index 4cb26d5ae5..25413b5ea8 100644 --- a/src/Umbraco.Examine.Lucene/UmbracoContentIndex.cs +++ b/src/Umbraco.Examine.Lucene/UmbracoContentIndex.cs @@ -67,7 +67,7 @@ namespace Umbraco.Examine /// protected override void PerformIndexItems(IEnumerable values, Action onComplete) { - //We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. + // We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. // The Invalid items will be deleted, these are items that have invalid paths (i.e. moved to the recycle bin, etc...) // Then we'll index the Value group all together. // We return 0 or 1 here so we can order the results and do the invalid first and then the valid. diff --git a/src/Umbraco.Infrastructure/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Infrastructure/Cache/DistributedCacheExtensions.cs index 7cfac1b9a0..ef968e8fa6 100644 --- a/src/Umbraco.Infrastructure/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Infrastructure/Cache/DistributedCacheExtensions.cs @@ -128,15 +128,16 @@ namespace Umbraco.Web.Cache public static void RefreshMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Refresh(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } public static void RemoveMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Remove(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } - #endregion #region MemberGroupCache diff --git a/src/Umbraco.Infrastructure/Compose/NotificationsComponent.cs b/src/Umbraco.Infrastructure/Compose/NotificationsComponent.cs index 790cab0913..2530ba3a08 100644 --- a/src/Umbraco.Infrastructure/Compose/NotificationsComponent.cs +++ b/src/Umbraco.Infrastructure/Compose/NotificationsComponent.cs @@ -145,7 +145,7 @@ namespace Umbraco.Web.Compose private void UserServiceUserGroupPermissionsAssigned(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.EntityId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } @@ -162,7 +162,7 @@ namespace Umbraco.Web.Compose .Where(m => m.OriginalPath.Contains(Constants.System.RecycleBinContentString)) .Select(m => m.Entity) .ToArray(); - if(restoredEntities.Any()) + if (restoredEntities.Any()) { _notifier.Notify(_actions.GetAction(), restoredEntities); } @@ -171,7 +171,7 @@ namespace Umbraco.Web.Compose private void PublicAccessServiceSaved(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.ProtectedNodeId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } diff --git a/src/Umbraco.Infrastructure/Examine/ContentValueSetBuilder.cs b/src/Umbraco.Infrastructure/Examine/ContentValueSetBuilder.cs index 01e6d993b4..f03fcca181 100644 --- a/src/Umbraco.Infrastructure/Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Infrastructure/Examine/ContentValueSetBuilder.cs @@ -53,6 +53,11 @@ namespace Umbraco.Examine scope.Complete(); } + return GetValueSetsEnumerable(content, creatorIds, writerIds); + } + + private IEnumerable GetValueSetsEnumerable(IContent[] content, Dictionary creatorIds, Dictionary writerIds) + { // TODO: There is a lot of boxing going on here and ultimately all values will be boxed by Lucene anyways // but I wonder if there's a way to reduce the boxing that we have to do or if it will matter in the end since // Lucene will do it no matter what? One idea was to create a `FieldValue` struct which would contain `object`, `object[]`, `ValueType` and `ValueType[]` diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/IMemberRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/IMemberRepository.cs index 245c024356..c737c2bf66 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/IMemberRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/IMemberRepository.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Querying; @@ -6,6 +7,8 @@ namespace Umbraco.Core.Persistence.Repositories { public interface IMemberRepository : IContentRepository { + IMember GetByUsername(string username); + /// /// Finds members in a given role /// @@ -35,5 +38,17 @@ namespace Umbraco.Core.Persistence.Repositories /// /// int GetCountByQuery(IQuery query); + + /// + /// Sets a members last login date based on their username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs index 64266f9df8..8c3c2b3b11 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs @@ -26,6 +26,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement private readonly ITagRepository _tagRepository; private readonly IPasswordHasher _passwordHasher; private readonly IMemberGroupRepository _memberGroupRepository; + private readonly IRepositoryCachePolicy _memberByUsernameCachePolicy; public MemberRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, ITagRepository tagRepository, ILanguageRepository languageRepository, IRelationRepository relationRepository, IRelationTypeRepository relationTypeRepository, @@ -39,6 +40,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement _tagRepository = tagRepository ?? throw new ArgumentNullException(nameof(tagRepository)); _passwordHasher = passwordHasher; _memberGroupRepository = memberGroupRepository; + + _memberByUsernameCachePolicy = new DefaultRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, DefaultOptions); } protected override MemberRepository This => this; @@ -383,12 +386,27 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (changedCols.Count > 0) Database.Update(dto, changedCols); - // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == member.VersionId); - Database.Execute(deletePropertyDataSql); + // Replace the property data + // Lookup the data to update with a UPDLOCK (using ForUpdate()) this is because we have another method that doesn't take an explicit WriteLock + // in SetLastLogin which is called very often and we want to avoid the lock timeout for the explicit lock table but we still need to ensure atomic + // operations between that method and this one. + + var propDataSql = SqlContext.Sql().Select("*").From().Where(x => x.VersionId == member.VersionId).ForUpdate(); + var existingPropData = Database.Fetch(propDataSql).ToDictionary(x => x.PropertyTypeId); var propertyDataDtos = PropertyFactory.BuildDtos(member.ContentType.Variations, member.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); + { + // Check if this already exists and update, else insert a new one + if (existingPropData.TryGetValue(propertyDataDto.PropertyTypeId, out var propData)) + { + propertyDataDto.Id = propData.Id; + Database.Update(propertyDataDto); + } + else + { + Database.Insert(propertyDataDto); + } + } SetEntityTags(entity, _tagRepository); @@ -506,6 +524,56 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return Database.ExecuteScalar(fullSql); } + /// + public void SetLastLogin(string username, DateTime date) + { + // Important - these queries are designed to execute without an exclusive WriteLock taken in our distributed lock + // table. However due to the data that we are updating which relies on version data we cannot update this data + // without taking some locks, otherwise we'll end up with strange situations because when a member is updated, that operation + // deletes and re-inserts all property data. So if there are concurrent transactions, one deleting and re-inserting and another trying + // to update there can be problems. This is only an issue for cmsPropertyData, not umbracoContentVersion because that table just + // maintains a single row and it isn't deleted/re-inserted. + // So the important part here is the ForUpdate() call on the select to fetch the property data to update. + + // Update the cms property value for the member + + var sqlSelectTemplateProperty = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin1", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.Id == r.PropertyTypeId) + .InnerJoin().On((l, r) => l.Id == r.VersionId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.Alias == SqlTemplate.Arg("propertyTypeAlias")) + .Where(x => x.LoginName == SqlTemplate.Arg("username")) + .ForUpdate()); + var sqlSelectProperty = sqlSelectTemplateProperty.Sql(Constants.ObjectTypes.Member, Constants.Conventions.Member.LastLoginDate, username); + + var update = Sql() + .Update(u => u + .Set(x => x.DateValue, date)) + .WhereIn(x => x.Id, sqlSelectProperty); + + Database.Execute(update); + + // Update the umbracoContentVersion value for the member + + var sqlSelectTemplateVersion = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin2", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.LoginName == SqlTemplate.Arg("username"))); + var sqlSelectVersion = sqlSelectTemplateVersion.Sql(Constants.ObjectTypes.Member, username); + + Database.Execute(Sql() + .Update(u => u + .Set(x => x.VersionDate, date)) + .WhereIn(x => x.Id, sqlSelectVersion)); + } + /// /// Gets paged member results. /// @@ -529,20 +597,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement ordering); } - private string _pagedResultsByQueryWhere; - - private string GetPagedResultsByQueryWhere() - { - if (_pagedResultsByQueryWhere == null) - _pagedResultsByQueryWhere = " AND (" - + $"({SqlSyntax.GetQuotedTableName("umbracoNode")}.{SqlSyntax.GetQuotedColumnName("text")} LIKE @0)" - + " OR " - + $"({SqlSyntax.GetQuotedTableName("cmsMember")}.{SqlSyntax.GetQuotedColumnName("LoginName")} LIKE @0)" - + ")"; - - return _pagedResultsByQueryWhere; - } - protected override string ApplySystemOrdering(ref Sql sql, Ordering ordering) { if (ordering.OrderBy.InvariantEquals("email")) @@ -632,5 +686,22 @@ namespace Umbraco.Core.Persistence.Repositories.Implement member.ResetDirtyProperties(false); return member; } + + public IMember GetByUsername(string username) + { + return _memberByUsernameCachePolicy.Get(username, PerformGetByUsername, PerformGetAllByUsername); + } + + private IMember PerformGetByUsername(string username) + { + var query = Query().Where(x => x.Username.Equals(username)); + return PerformGetByQuery(query).FirstOrDefault(); + } + + private IEnumerable PerformGetAllByUsername(params string[] usernames) + { + var query = Query().WhereIn(x => x.Username, usernames); + return PerformGetByQuery(query); + } } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs index f7e59820c3..d327fba67f 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs @@ -11,6 +11,8 @@ using Umbraco.Core.Scoping; namespace Umbraco.Core.Persistence.Repositories.Implement { + // TODO: Obsolete this, change all implementations of this like in Dictionary to just use custom Cache policies like in the member repository. + /// /// Simple abstract ReadOnly repository used to simply have PerformGet and PeformGetAll with an underlying cache /// diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index 23da951efe..219957bd34 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -168,10 +168,7 @@ ORDER BY colName"; } public Guid CreateLoginSession(int userId, string requestingIpAddress, bool cleanStaleSessions = true) - { - // TODO: I know this doesn't follow the normal repository conventions which would require us to create a UserSessionRepository - //and also business logic models for these objects but that's just so overkill for what we are doing - //and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore + { var now = DateTime.UtcNow; var dto = new UserLoginDto { @@ -201,13 +198,14 @@ ORDER BY colName"; // that query is going to run a *lot*, make it a template var t = SqlContext.Templates.Get("Umbraco.Core.UserRepository.ValidateLoginSession", s => s .Select() + .SelectTop(1) .From() .Where(x => x.SessionId == SqlTemplate.Arg("sessionId")) .ForUpdate()); var sql = t.Sql(sessionId); - var found = Database.Query(sql).FirstOrDefault(); + var found = Database.FirstOrDefault(sql); if (found == null || found.UserId != userId || found.LoggedOutUtc.HasValue) return false; diff --git a/src/Umbraco.Infrastructure/Scheduling/SimpleTask.cs b/src/Umbraco.Infrastructure/Scheduling/SimpleTask.cs new file mode 100644 index 0000000000..a8603915b0 --- /dev/null +++ b/src/Umbraco.Infrastructure/Scheduling/SimpleTask.cs @@ -0,0 +1,32 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Umbraco.Web.Scheduling +{ + /// + /// A simple task that executes a delegate synchronously + /// + internal class SimpleTask : IBackgroundTask + { + private readonly Action _action; + + public SimpleTask(Action action) + { + _action = action; + } + + public bool IsAsync => false; + + public void Run() => _action(); + + public Task RunAsync(CancellationToken token) + { + throw new NotImplementedException(); + } + + public void Dispose() + { + } + } +} diff --git a/src/Umbraco.Infrastructure/Search/ExamineComponent.cs b/src/Umbraco.Infrastructure/Search/ExamineComponent.cs index ae5bfd628b..639d5d1a7c 100644 --- a/src/Umbraco.Infrastructure/Search/ExamineComponent.cs +++ b/src/Umbraco.Infrastructure/Search/ExamineComponent.cs @@ -5,6 +5,7 @@ using System.Linq; using Examine; using Umbraco.Core; using Umbraco.Core.Cache; +using Umbraco.Core.Hosting; using Umbraco.Core.Logging; using Umbraco.Core.Models; using Umbraco.Core.Scoping; @@ -13,9 +14,12 @@ using Umbraco.Core.Services.Changes; using Umbraco.Core.Sync; using Umbraco.Web.Cache; using Umbraco.Examine; +using Umbraco.Web.Scheduling; namespace Umbraco.Web.Search { + + public sealed class ExamineComponent : Umbraco.Core.Composing.IComponent { private readonly IExamineManager _examineManager; @@ -29,7 +33,7 @@ namespace Umbraco.Web.Search private readonly IMainDom _mainDom; private readonly IProfilingLogger _logger; private readonly IUmbracoIndexesCreator _indexCreator; - + private readonly BackgroundTaskRunner _indexItemTaskRunner; // the default enlist priority is 100 // enlist with a lower priority to ensure that anything "default" runs after us @@ -44,7 +48,8 @@ namespace Umbraco.Web.Search IPublishedContentValueSetBuilder publishedContentValueSetBuilder, IValueSetBuilder mediaValueSetBuilder, IValueSetBuilder memberValueSetBuilder, - BackgroundIndexRebuilder backgroundIndexRebuilder) + BackgroundIndexRebuilder backgroundIndexRebuilder, + IApplicationShutdownRegistry applicationShutdownRegistry) { _services = services; _scopeProvider = scopeProvider; @@ -57,6 +62,7 @@ namespace Umbraco.Web.Search _mainDom = mainDom; _logger = profilingLogger; _indexCreator = indexCreator; + _indexItemTaskRunner = new BackgroundTaskRunner(_logger, applicationShutdownRegistry); } public void Initialize() @@ -562,12 +568,18 @@ namespace Umbraco.Web.Search } } + /// + /// An action that will execute at the end of the Scope being completed + /// private abstract class DeferedAction { public virtual void Execute() { } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForContent : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -588,41 +600,32 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IContent content, bool isPublished) { - // TODO: This is ugly, it is going to build the value set 3x and for 2 of those times it will be the same value - // set. We can do better. - - // TODO: We are .ToList() ing each of the calls to GetValueSets here. This is currently required but isn't the way - // that this was intended to work. Ideally, the IEnumerable package gets passed to Examine and it is only iterated/executed - // when the indexing takes place which would occur on a background thread. This is problematic with how the ContentValueSetBuilder - // in combination with UmbracoContentIndex.PerformIndexItems works because (at least what I've come to believe) we are using yield - // return in the GetValueSets call in combination with trying to lazily resolve the enumerable but because we GroupBy in - // UmbracoContentIndex.PerformIndexItems it's eagerly executed but then lazily executed again on the background thread and I believe - // that in doing this when the call is made to _userService.GetProfilesById it's trying to resolve a scope from an AsyncLocal instance - // that has already been disposed higher up it's chain. I 'think' to how the eager/lazy enumeration happens with yield return that it's - // capturing a scope/AsyncLocal instance that it shouldn't really be using. - - // TODO: We don't want these value sets to be eagerly built in this thread since this is most likely going to be a request thread. - // This is why the lazy execution of the Enumerable had the intended affect of executing only when requested on the background thread. - // This could still be acheived: Either we have a custom Enumerable/Enumerator to do this, or we simply call the below code - // on a background thread... which would be much easier! - - // TODO: I think this is an issue in v8 too! - - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - //for content we have a different builder for published vs unpublished - var builder = index.PublishedValuesOnly - ? examineComponent._publishedContentValueSetBuilder - : (IValueSetBuilder)examineComponent._contentValueSetBuilder; + // for content we have a different builder for published vs unpublished + // we don't want to build more value sets than is needed so we'll lazily build 2 one for published one for non-published + var builders = new Dictionary>> + { + [true] = new Lazy>(() => examineComponent._publishedContentValueSetBuilder.GetValueSets(content).ToList()), + [false] = new Lazy>(() => examineComponent._contentValueSetBuilder.GetValueSets(content).ToList()) + }; - index.IndexItems(builder.GetValueSets(content).ToList()); - } + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + var valueSet = builders[index.PublishedValuesOnly].Value; + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMedia : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -643,18 +646,25 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMedia media, bool isPublished) { - var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); - - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); + + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMember : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -673,13 +683,17 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMember member) { - var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } diff --git a/src/Umbraco.Infrastructure/Services/Implement/MemberService.cs b/src/Umbraco.Infrastructure/Services/Implement/MemberService.cs index 5143cd6203..b5c0dd6a46 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/MemberService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/MemberService.cs @@ -435,15 +435,10 @@ namespace Umbraco.Core.Services.Implement /// public IMember GetByUsername(string username) { - // TODO: Somewhere in here, whether at this level or the repository level, we need to add - // a caching mechanism since this method is used by all the membership providers and could be - // called quite a bit when dealing with members. - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { - scope.ReadLock(Constants.Locks.MemberTree); - var query = Query().Where(x => x.Username.Equals(username)); - return _memberRepository.Get(query).FirstOrDefault(); + scope.ReadLock(Constants.Locks.MemberTree); + return _memberRepository.GetByUsername(username); } } @@ -793,12 +788,17 @@ namespace Umbraco.Core.Services.Implement #region Save - /// - /// Saves an - /// - /// to Save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// + public void SetLastLogin(string username, DateTime date) + { + using (var scope = ScopeProvider.CreateScope()) + { + _memberRepository.SetLastLogin(username, date); + scope.Complete(); + } + } + + /// public void Save(IMember member, bool raiseEvents = true) { //trimming username and email to make sure we have no trailing space @@ -834,12 +834,7 @@ namespace Umbraco.Core.Services.Implement } } - /// - /// Saves a list of objects - /// - /// to save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// public void Save(IEnumerable members, bool raiseEvents = true) { var membersA = members.ToArray(); diff --git a/src/Umbraco.Infrastructure/Services/Implement/UserService.cs b/src/Umbraco.Infrastructure/Services/Implement/UserService.cs index fdd79e7264..65fb235054 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/UserService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/UserService.cs @@ -254,6 +254,13 @@ namespace Umbraco.Core.Services.Implement } } + // explicit implementation because we don't need it now but due to the way that the members membership provider is put together + // this method must exist in this service as an implementation (legacy) + void IMembershipMemberService.SetLastLogin(string username, DateTime date) + { + throw new NotSupportedException("This method is not implemented or supported for users"); + } + /// /// Saves an /// diff --git a/src/Umbraco.Tests.Integration/Umbraco.Tests.Integration.csproj b/src/Umbraco.Tests.Integration/Umbraco.Tests.Integration.csproj index 1bfbd8afe8..b72f1d2612 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Tests.Integration.csproj +++ b/src/Umbraco.Tests.Integration/Umbraco.Tests.Integration.csproj @@ -55,4 +55,8 @@ + + + + diff --git a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs index fb017385ce..74176e10f0 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs @@ -16,6 +16,11 @@ using Umbraco.Tests.Common.Builders; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Entities; using Umbraco.Tests.Testing; +using Umbraco.Core.Configuration; +using Umbraco.Core.Persistence; +using Umbraco.Core.Persistence.Dtos; +using Umbraco.Core.Persistence.Mappers; +using Umbraco.Tests.Common.Builders.Extensions; using MockedUser = Umbraco.Tests.TestHelpers.Entities.MockedUser; namespace Umbraco.Tests.Persistence.Repositories @@ -83,6 +88,104 @@ namespace Umbraco.Tests.Persistence.Repositories return new UserGroupRepository(accessor, AppCaches.Disabled, Logger, ShortStringHelper); } + [Test] + public void Validate_Login_Session() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + var user = MockedUser.CreateUser(); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + repository.Save(user); + } + + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + var sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + + // manually update this record to be in the past + scope.Database.Execute(SqlContext.Sql() + .Update(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow.AddDays(-100))) + .Where(x => x.SessionId == sessionId)); + + var isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsFalse(isValid); + + // create a new one + sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsTrue(isValid); + } + } + + [Test] + public void Can_Perform_Add_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var user = MockedUser.CreateUser(); + + // Act + repository.Save(user); + + + // Assert + Assert.That(user.HasIdentity, Is.True); + } + } + + [Test] + public void Can_Perform_Multiple_Adds_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var user1 = MockedUser.CreateUser("1"); + var use2 = MockedUser.CreateUser("2"); + + // Act + repository.Save(user1); + + repository.Save(use2); + + + // Assert + Assert.That(user1.HasIdentity, Is.True); + Assert.That(use2.HasIdentity, Is.True); + } + } + + [Test] + public void Can_Verify_Fresh_Entity_Is_Not_Dirty() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var user = MockedUser.CreateUser(); + repository.Save(user); + + + // Act + var resolved = repository.Get((int)user.Id); + bool dirty = ((User)resolved).IsDirty(); + + // Assert + Assert.That(dirty, Is.False); + } + } + [Test] public void Can_Perform_Update_On_UserRepository() { @@ -91,7 +194,7 @@ namespace Umbraco.Tests.Persistence.Repositories // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var userRepository = CreateRepository(provider); var contentRepository = CreateContentRepository(provider, out var contentTypeRepo); @@ -145,6 +248,272 @@ namespace Umbraco.Tests.Persistence.Repositories } } + [Test] + public void Can_Perform_Delete_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var user = MockedUser.CreateUser(); + + // Act + repository.Save(user); + + var id = user.Id; + + var repository2 = new UserRepository((IScopeAccessor) provider, AppCaches.Disabled, Logger, + Mock.Of(), + Microsoft.Extensions.Options.Options.Create(new GlobalSettings()), + Microsoft.Extensions.Options.Options.Create(new UserPasswordConfigurationSettings()), + Mock.Of()); + + repository2.Delete(user); + + + var resolved = repository2.Get((int) id); + + // Assert + Assert.That(resolved, Is.Null); + } + } + + [Test] + public void Can_Perform_Get_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + var userGroupRepository = CreateUserGroupRepository(provider); + + var user = CreateAndCommitUserWithGroup(repository, userGroupRepository); + + // Act + var updatedItem = repository.Get(user.Id); + + // FIXME: this test cannot work, user has 2 sections but the way it's created, + // they don't show, so the comparison with updatedItem fails - fix! + + // Assert + AssertPropertyValues(updatedItem, user); + } + } + + [Test] + public void Can_Perform_GetByQuery_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + CreateAndCommitMultipleUsers(repository); + + // Act + var query = scope.SqlContext.Query().Where(x => x.Username == "TestUser1"); + var result = repository.Get(query); + + // Assert + Assert.That(result.Count(), Is.GreaterThanOrEqualTo(1)); + } + } + + [Test] + public void Can_Perform_GetAll_By_Param_Ids_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var users = CreateAndCommitMultipleUsers(repository); + + // Act + var result = repository.GetMany((int) users[0].Id, (int) users[1].Id); + + // Assert + Assert.That(result, Is.Not.Null); + Assert.That(result.Any(), Is.True); + Assert.That(result.Count(), Is.EqualTo(2)); + } + } + + [Test] + public void Can_Perform_GetAll_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + CreateAndCommitMultipleUsers(repository); + + // Act + var result = repository.GetMany(); + + // Assert + Assert.That(result, Is.Not.Null); + Assert.That(result.Any(), Is.True); + Assert.That(result.Count(), Is.GreaterThanOrEqualTo(3)); + } + } + + [Test] + public void Can_Perform_Exists_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var users = CreateAndCommitMultipleUsers(repository); + + // Act + var exists = repository.Exists(users[0].Id); + + // Assert + Assert.That(exists, Is.True); + } + } + + [Test] + public void Can_Perform_Count_On_UserRepository() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var users = CreateAndCommitMultipleUsers(repository); + + // Act + var query = scope.SqlContext.Query().Where(x => x.Username == "TestUser1" || x.Username == "TestUser2"); + var result = repository.Count(query); + + // Assert + Assert.AreEqual(2, result); + } + } + + [Test] + public void Can_Get_Paged_Results_By_Query_And_Filter_And_Groups() + { + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var users = CreateAndCommitMultipleUsers(repository); + var query = provider.SqlContext.Query().Where(x => x.Username == "TestUser1" || x.Username == "TestUser2"); + + try + { + scope.Database.AsUmbracoDatabase().EnableSqlTrace = true; + scope.Database.AsUmbracoDatabase().EnableSqlCount = true; + + // Act + var result = repository.GetPagedResultsByQuery(query, 0, 10, out var totalRecs, user => user.Id, Direction.Ascending, + excludeUserGroups: new[] { Constants.Security.TranslatorGroupAlias }, + filter: provider.SqlContext.Query().Where(x => x.Id > -1)); + + // Assert + Assert.AreEqual(2, totalRecs); + } + finally + { + scope.Database.AsUmbracoDatabase().EnableSqlTrace = false; + scope.Database.AsUmbracoDatabase().EnableSqlCount = false; + } + } + + } + + [Test] + public void Can_Get_Paged_Results_With_Filter_And_Groups() + { + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + + var users = CreateAndCommitMultipleUsers(repository); + + try + { + scope.Database.AsUmbracoDatabase().EnableSqlTrace = true; + scope.Database.AsUmbracoDatabase().EnableSqlCount = true; + + // Act + var result = repository.GetPagedResultsByQuery(null, 0, 10, out var totalRecs, user => user.Id, Direction.Ascending, + includeUserGroups: new[] { Constants.Security.AdminGroupAlias, Constants.Security.SensitiveDataGroupAlias }, + excludeUserGroups: new[] { Constants.Security.TranslatorGroupAlias }, + filter: provider.SqlContext.Query().Where(x => x.Id == -1)); + + // Assert + Assert.AreEqual(1, totalRecs); + } + finally + { + scope.Database.AsUmbracoDatabase().EnableSqlTrace = false; + scope.Database.AsUmbracoDatabase().EnableSqlCount = false; + } + } + } + + [Test] + public void Can_Invalidate_SecurityStamp_On_Username_Change() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + var userGroupRepository = CreateUserGroupRepository(provider); + + var user = CreateAndCommitUserWithGroup(repository, userGroupRepository); + var originalSecurityStamp = user.SecurityStamp; + + // Ensure when user generated a security stamp is present + Assert.That(user.SecurityStamp, Is.Not.Null); + Assert.That(user.SecurityStamp, Is.Not.Empty); + + // Update username + user.Username = user.Username + "UPDATED"; + repository.Save(user); + + // Get the user + var updatedUser = repository.Get(user.Id); + + // Ensure the Security Stamp is invalidated & no longer the same + Assert.AreNotEqual(originalSecurityStamp, updatedUser.SecurityStamp); + } + } + + private void AssertPropertyValues(IUser updatedItem, IUser originalUser) + { + Assert.That(updatedItem.Id, Is.EqualTo(originalUser.Id)); + Assert.That(updatedItem.Name, Is.EqualTo(originalUser.Name)); + Assert.That(updatedItem.Language, Is.EqualTo(originalUser.Language)); + Assert.That(updatedItem.IsApproved, Is.EqualTo(originalUser.IsApproved)); + Assert.That(updatedItem.RawPasswordValue, Is.EqualTo(originalUser.RawPasswordValue)); + Assert.That(updatedItem.IsLockedOut, Is.EqualTo(originalUser.IsLockedOut)); + Assert.IsTrue(updatedItem.StartContentIds.UnsortedSequenceEqual(originalUser.StartContentIds)); + Assert.IsTrue(updatedItem.StartMediaIds.UnsortedSequenceEqual(originalUser.StartMediaIds)); + Assert.That(updatedItem.Email, Is.EqualTo(originalUser.Email)); + Assert.That(updatedItem.Username, Is.EqualTo(originalUser.Username)); + Assert.That(updatedItem.AllowedSections.Count(), Is.EqualTo(originalUser.AllowedSections.Count())); + foreach (var allowedSection in originalUser.AllowedSections) + Assert.IsTrue(updatedItem.AllowedSections.Contains(allowedSection)); + } private static User CreateAndCommitUserWithGroup(IUserRepository repository, IUserGroupRepository userGroupRepository) { @@ -159,6 +528,16 @@ namespace Umbraco.Tests.Persistence.Repositories return user; } + private IUser[] CreateAndCommitMultipleUsers(IUserRepository repository) + { + var user1 = new UserBuilder().WithoutIdentity().WithSuffix("1").Build(); + var user2 = new UserBuilder().WithoutIdentity().WithSuffix("2").Build(); + var user3 = new UserBuilder().WithoutIdentity().WithSuffix("3").Build(); + repository.Save(user1); + repository.Save(user2); + repository.Save(user3); + return new IUser[] { user1, user2, user3 }; + } } } diff --git a/src/Umbraco.Tests/Services/MemberServiceTests.cs b/src/Umbraco.Tests/Services/MemberServiceTests.cs index e7adbdacbf..79fbfb28ed 100644 --- a/src/Umbraco.Tests/Services/MemberServiceTests.cs +++ b/src/Umbraco.Tests/Services/MemberServiceTests.cs @@ -38,6 +38,60 @@ namespace Umbraco.Tests.Services base.SetUp(); } + [Test] + public void Can_Update_Member_Property_Value() + { + IMemberType memberType = MockedContentTypes.CreateSimpleMemberType(); + ServiceContext.MemberTypeService.Save(memberType); + IMember member = MockedMember.CreateSimpleMember(memberType, "hello", "helloworld@test123.com", "hello", "hello"); + member.SetValue("title", "title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + member.SetValue("title", "another title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + Assert.AreEqual("another title of mine", member.GetValue("title")); + } + + [Test] + public void Can_Get_By_Username() + { + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true); + ServiceContext.MemberService.Save(member); + + var member2 = ServiceContext.MemberService.GetByUsername(member.Username); + + Assert.IsNotNull(member2); + Assert.AreEqual(member.Email, member2.Email); + } + + [Test] + public void Can_Set_Last_Login_Date() + { + var now = DateTime.Now; + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true) + { + LastLoginDate = now, + UpdateDate = now + }; + ServiceContext.MemberService.Save(member); + + var newDate = now.AddDays(10); + ServiceContext.MemberService.SetLastLogin(member.Username, newDate); + + //re-get + member = ServiceContext.MemberService.GetById(member.Id); + + Assert.That(member.LastLoginDate, Is.EqualTo(newDate).Within(1).Seconds); + Assert.That(member.UpdateDate, Is.EqualTo(newDate).Within(1).Seconds); + } + [Test] public void Can_Create_Member_With_Properties() { diff --git a/src/Umbraco.Web/Logging/WebProfilerComponent.cs b/src/Umbraco.Web/Logging/WebProfilerComponent.cs index 2edeea6a1b..1cb2142199 100755 --- a/src/Umbraco.Web/Logging/WebProfilerComponent.cs +++ b/src/Umbraco.Web/Logging/WebProfilerComponent.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Web; using Umbraco.Core.Composing; using Umbraco.Core.Logging; @@ -9,6 +10,7 @@ namespace Umbraco.Web.Logging { private readonly WebProfiler _profiler; private readonly bool _profile; + private readonly List _terminate = new List(); public WebProfilerComponent(IProfiler profiler, ILogger logger) { @@ -37,6 +39,7 @@ namespace Umbraco.Web.Logging public void Terminate() { UmbracoApplicationBase.ApplicationInit -= InitializeApplication; + foreach (var t in _terminate) t(); } private void InitializeApplication(object sender, EventArgs args) @@ -44,8 +47,13 @@ namespace Umbraco.Web.Logging if (!(sender is HttpApplication app)) return; // for *each* application (this will run more than once) - app.BeginRequest += (s, a) => _profiler.UmbracoApplicationBeginRequest(s, a); - app.EndRequest += (s, a) => _profiler.UmbracoApplicationEndRequest(s, a); + void beginRequest(object s, EventArgs a) => _profiler.UmbracoApplicationBeginRequest(s, a); + app.BeginRequest += beginRequest; + _terminate.Add(() => app.BeginRequest -= beginRequest); + + void endRequest(object s, EventArgs a) => _profiler.UmbracoApplicationEndRequest(s, a); + app.EndRequest += endRequest; + _terminate.Add(() => app.EndRequest -= endRequest); } } } diff --git a/src/Umbraco.Web/Security/MembershipHelper.cs b/src/Umbraco.Web/Security/MembershipHelper.cs index f13be1c6ea..8ca5af397b 100644 --- a/src/Umbraco.Web/Security/MembershipHelper.cs +++ b/src/Umbraco.Web/Security/MembershipHelper.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Text; using System.Web.Security; using Umbraco.Core; using Umbraco.Core.Logging; @@ -300,8 +299,9 @@ namespace Umbraco.Web.Security { return false; } - //Set member online - var member = provider.GetUser(username, true); + // Get the member, do not set to online - this is done implicitly as part of ValidateUser which is consistent with + // how the .NET framework SqlMembershipProvider works. Passing in true will just cause more unnecessary SQL queries/locks. + var member = provider.GetUser(username, false); if (member == null) { //this should not happen @@ -770,29 +770,12 @@ namespace Umbraco.Web.Security /// private IMember GetCurrentPersistedMember() { - return _appCaches.RequestCache.GetCacheItem( - GetCacheKey("GetCurrentPersistedMember"), () => - { - var provider = _membershipProvider; + var provider = _membershipProvider; var username = provider.GetCurrentUserName(); - var member = _memberService.GetByUsername(username); - return member; - }); - } - - private static string GetCacheKey(string key, params object[] additional) - { - var sb = new StringBuilder(); - sb.Append(typeof(MembershipHelper).Name); - sb.Append("-"); - sb.Append(key); - foreach (var s in additional) - { - sb.Append("-"); - sb.Append(s); - } - return sb.ToString(); + // The result of this is cached by the MemberRepository + var member = _memberService.GetByUsername(username); + return member; } /// diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index b099110911..d059113e0a 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -12,7 +12,6 @@ using Umbraco.Core.Logging; using Umbraco.Core.Models.Membership; using Umbraco.Net; using Umbraco.Core.Persistence.Querying; -using Umbraco.Core.Security; using Umbraco.Core.Services; using Umbraco.Web.Composing; @@ -321,15 +320,16 @@ namespace Umbraco.Web.Security.Providers if (userIsOnline) { - member.LastLoginDate = DateTime.Now; - member.UpdateDate = DateTime.Now; - //don't raise events for this! It just sets the member dates, if we do raise events this will - // cause all distributed cache to execute - which will clear out some caches we don't want. - // http://issues.umbraco.org/issue/U4-3451 - // 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); + { + var now = DateTime.Now; + // update the database data directly instead of a full member save which requires DB locks + MemberService.SetLastLogin(username, now); + member.LastLoginDate = now; + member.UpdateDate = now; + } + } return ConvertToMembershipUser(member); @@ -521,6 +521,8 @@ namespace Umbraco.Web.Security.Providers var authenticated = PasswordSecurity.VerifyPassword(password, member.RawPasswordValue); + var requiresFullSave = false; + if (authenticated == false) { // TODO: Increment login attempts - lock if too many. @@ -540,6 +542,8 @@ namespace Umbraco.Web.Security.Providers { Current.Logger.Info("Login attempt failed for username {Username} from IP address {IpAddress}", username, _ipResolver.GetCurrentRequestIpAddress()); } + + requiresFullSave = true; } else { @@ -547,6 +551,7 @@ namespace Umbraco.Web.Security.Providers { //we have successfully logged in, reset the AccessFailedCount member.FailedPasswordAttempts = 0; + requiresFullSave = true; } member.LastLoginDate = DateTime.Now; @@ -554,15 +559,23 @@ namespace Umbraco.Web.Security.Providers Current.Logger.Info("Login attempt succeeded for username {Username} from IP address {IpAddress}", username, _ipResolver.GetCurrentRequestIpAddress()); } - //don't raise events for this! It just sets the member dates, if we do raise events this will + // don't raise events for this! It just sets the member dates, if we do raise events this will // cause all distributed cache to execute - which will clear out some caches we don't want. // http://issues.umbraco.org/issue/U4-3451 // TODO: In v8 we aren't going to have an overload to disable events, so we'll need to make a different method // for this type of thing (i.e. UpdateLastLogin or similar). - // 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); + if (requiresFullSave) + { + // 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); + } + else + { + // set the last login date without full save (fast, no locks) + MemberService.SetLastLogin(member.Username, member.LastLoginDate); + } return new ValidateUserResult { diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 739bd2c95e..51090aca7e 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -406,8 +406,5 @@ Mvc\web.config - - - \ No newline at end of file