From fae688b896a5f1b6d8471c60e51541c59ca596bf Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 8 Jun 2016 15:52:24 +0200 Subject: [PATCH] Cleanup - membership --- src/Umbraco.Web/Security/MembershipHelper.cs | 127 +++++++++--------- .../businesslogic/member/Member.cs | 100 +++++++------- 2 files changed, 111 insertions(+), 116 deletions(-) diff --git a/src/Umbraco.Web/Security/MembershipHelper.cs b/src/Umbraco.Web/Security/MembershipHelper.cs index 8e47762d93..1d9885a1c3 100644 --- a/src/Umbraco.Web/Security/MembershipHelper.cs +++ b/src/Umbraco.Web/Security/MembershipHelper.cs @@ -8,7 +8,6 @@ using System.Web.Security; using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; -using Umbraco.Core.Models.PublishedContent; using Umbraco.Core.Security; using Umbraco.Web.Models; using Umbraco.Web.PublishedCache; @@ -18,7 +17,6 @@ using MPE = global::Umbraco.Core.Security.MembershipProviderExtensions; namespace Umbraco.Web.Security { - /// /// A helper class for handling Members /// @@ -31,29 +29,27 @@ namespace Umbraco.Web.Security private readonly IPublishedMemberCache _memberCache; #region Constructors - public MembershipHelper(ApplicationContext applicationContext, HttpContextBase httpContext) - : this(applicationContext, httpContext, MPE.GetMembersMembershipProvider(), Roles.Enabled ? Roles.Provider : new MembersRoleProvider(applicationContext.Services.MemberService)) - { - } - public MembershipHelper(ApplicationContext applicationContext, HttpContextBase httpContext, MembershipProvider membershipProvider, RoleProvider roleProvider) + // used here and there for IMember operations (not front-end stuff, no need for _memberCache) + public MembershipHelper(ApplicationContext applicationContext, HttpContextBase httpContext) { if (applicationContext == null) throw new ArgumentNullException(nameof(applicationContext)); if (httpContext == null) throw new ArgumentNullException(nameof(httpContext)); - if (membershipProvider == null) throw new ArgumentNullException(nameof(membershipProvider)); - if (roleProvider == null) throw new ArgumentNullException(nameof(roleProvider)); _applicationContext = applicationContext; _httpContext = httpContext; - _membershipProvider = membershipProvider; - _roleProvider = roleProvider; - _memberCache = UmbracoContext.Current?.Facade?.MemberCache; // fixme - } + _membershipProvider = MPE.GetMembersMembershipProvider(); + _roleProvider = Roles.Enabled ? Roles.Provider : new MembersRoleProvider(applicationContext.Services.MemberService); - public MembershipHelper(UmbracoContext umbracoContext) - : this(umbracoContext, MPE.GetMembersMembershipProvider(), Roles.Enabled ? Roles.Provider: new MembersRoleProvider(umbracoContext.Application.Services.MemberService)) - { + // _memberCache remains null - not supposed to use it + // alternatively we'd need to get if from the 'current' UmbracoContext? } + // used everywhere + public MembershipHelper(UmbracoContext umbracoContext) + : this(umbracoContext, MPE.GetMembersMembershipProvider(), Roles.Enabled ? Roles.Provider: new MembersRoleProvider(umbracoContext.Application.Services.MemberService)) + { } + + // used in tests and (this) public MembershipHelper(UmbracoContext umbracoContext, MembershipProvider membershipProvider, RoleProvider roleProvider) { if (umbracoContext == null) throw new ArgumentNullException(nameof(umbracoContext)); @@ -65,8 +61,19 @@ namespace Umbraco.Web.Security _roleProvider = roleProvider; _memberCache = umbracoContext.Facade.MemberCache; } + #endregion + protected IPublishedMemberCache MemberCache + { + get + { + if (_memberCache == null) + throw new InvalidOperationException("No MemberCache."); + return _memberCache; + } + } + /// /// Returns true if the current membership provider is the Umbraco built-in one. /// @@ -115,8 +122,8 @@ namespace Umbraco.Web.Security var member = GetCurrentPersistedMember(); - //NOTE: If changing the username is a requirement, than that needs to be done via the IMember directly since MembershipProvider's natively do - // not support changing a username! + //NOTE: If changing the username is a requirement, than that needs to be done via the IMember directly since MembershipProvider's natively do + // not support changing a username! if (model.Name != null && member.Name != model.Name) { member.Name = model.Name; @@ -158,7 +165,7 @@ namespace Umbraco.Web.Security MembershipUser membershipUser; var provider = _membershipProvider; - //update their real name + //update their real name if (provider.IsUmbracoMembershipProvider()) { membershipUser = ((UmbracoMembershipProviderBase)provider).CreateUser( @@ -198,7 +205,7 @@ namespace Umbraco.Web.Security { //Set member online provider.GetUser(model.Username, true); - + //Log them in FormsAuthentication.SetAuthCookie(membershipUser.UserName, model.CreatePersistentLoginCookie); } @@ -225,7 +232,7 @@ namespace Umbraco.Web.Security if (member == null) { //this should not happen - LogHelper.Warn("The member validated but then no member was returned with the username " + username); + LogHelper.Warn("The member validated but then no member was returned with the username " + username); return false; } //Log them in @@ -245,22 +252,22 @@ namespace Umbraco.Web.Security public virtual IPublishedContent GetByProviderKey(object key) { - return _memberCache.GetByProviderKey(key); + return MemberCache.GetByProviderKey(key); } public virtual IPublishedContent GetById(int memberId) { - return _memberCache.GetById(memberId); + return MemberCache.GetById(memberId); } public virtual IPublishedContent GetByUsername(string username) { - return _memberCache.GetByUsername(username); + return MemberCache.GetByUsername(username); } public virtual IPublishedContent GetByEmail(string email) { - return _memberCache.GetByEmail(email); + return MemberCache.GetByEmail(email); } /// @@ -274,7 +281,7 @@ namespace Umbraco.Web.Security return null; } var result = GetCurrentPersistedMember(); - return result == null ? null : _memberCache.GetByMember(result); + return result == null ? null : MemberCache.GetByMember(result); } /// @@ -288,9 +295,9 @@ namespace Umbraco.Web.Security return -1; } var result = GetCurrentMember(); - return result == null ? -1 : result.Id; + return result?.Id ?? -1; } - + #endregion #region Model Creation methods for member data editing on the front-end @@ -309,7 +316,7 @@ namespace Umbraco.Web.Security var provider = _membershipProvider; if (provider.IsUmbracoMembershipProvider()) - { + { var membershipUser = provider.GetCurrentUserOnline(); var member = GetCurrentPersistedMember(); //this shouldn't happen but will if the member is deleted in the back office while the member is trying @@ -396,7 +403,7 @@ namespace Umbraco.Web.Security if (propValue != null && propValue.Value != null) { value = propValue.Value.ToString(); - } + } } var viewProperty = new UmbracoProperty @@ -406,11 +413,11 @@ namespace Umbraco.Web.Security Value = value }; - //TODO: Perhaps one day we'll ship with our own EditorTempates but for now developers + //TODO: Perhaps one day we'll ship with our own EditorTempates but for now developers // can just render their own. ////This is a rudimentary check to see what data template we should render - //// if developers want to change the template they can do so dynamically in their views or controllers + //// if developers want to change the template they can do so dynamically in their views or controllers //// for a given property. ////These are the default built-in MVC template types: “Boolean”, “Decimal”, “EmailAddress”, “HiddenInput”, “Html”, “Object”, “String”, “Text”, and “Url” //// by default we'll render a text box since we've defined that metadata on the UmbracoProperty.Value property directly. @@ -419,7 +426,7 @@ namespace Umbraco.Web.Security // viewProperty.EditorTemplate = "UmbracoBoolean"; //} //else - //{ + //{ // switch (prop.DataTypeDatabaseType) // { // case DataTypeDatabaseType.Integer: @@ -505,10 +512,7 @@ namespace Umbraco.Web.Security /// /// Returns the currently logged in username /// - public string CurrentUserName - { - get { return _httpContext.User.Identity.Name; } - } + public string CurrentUserName => _httpContext.User.Identity.Name; /// /// Returns true or false if the currently logged in member is authorized based on the parameters provided @@ -536,7 +540,7 @@ namespace Umbraco.Web.Security // Allow by default var allowAction = true; - + if (IsLoggedIn() == false) { // If not logged on, not allowed @@ -571,7 +575,7 @@ namespace Umbraco.Web.Security var member = provider.GetCurrentUser(); username = member.UserName; } - + // If groups defined, check member is of one of those groups var allowGroupsList = allowGroups as IList ?? allowGroups.ToList(); if (allowAction && allowGroupsList.Any(allowGroup => allowGroup != string.Empty)) @@ -580,8 +584,6 @@ namespace Umbraco.Web.Security var groups = _roleProvider.GetRolesForUser(username); allowAction = allowGroupsList.Select(s => s.ToLowerInvariant()).Intersect(groups.Select(myGroup => myGroup.ToLowerInvariant())).Any(); } - - } return allowAction; @@ -610,13 +612,13 @@ namespace Umbraco.Web.Security /// /// /// - /// + /// public virtual Attempt ChangePassword(string username, ChangingPasswordModel passwordModel, MembershipProvider membershipProvider) { - // YES! It is completely insane how many options you have to take into account based on the membership provider. yikes! + // YES! It is completely insane how many options you have to take into account based on the membership provider. yikes! - if (passwordModel == null) throw new ArgumentNullException("passwordModel"); - if (membershipProvider == null) throw new ArgumentNullException("membershipProvider"); + if (passwordModel == null) throw new ArgumentNullException(nameof(passwordModel)); + if (membershipProvider == null) throw new ArgumentNullException(nameof(membershipProvider)); //Are we resetting the password?? if (passwordModel.Reset.HasValue && passwordModel.Reset.Value) @@ -735,7 +737,7 @@ namespace Umbraco.Web.Security return Attempt.Fail(new PasswordChangedModel { ChangeError = new ValidationResult("Could not change password, error: " + ex2.Message + " (see log for full details)", new[] { "value" }) }); } } - + /// /// Updates a membership user with all of it's writable properties /// @@ -756,45 +758,41 @@ namespace Umbraco.Web.Security DateTime? lastActivityDate = null, string comment = null) { - var needsUpdating = new List(); + var update = false; - //set the writable properties if (email != null) { - needsUpdating.Add(member.Email != email); + if (member.Email != email) update = true; member.Email = email; } if (isApproved.HasValue) { - needsUpdating.Add(member.IsApproved != isApproved.Value); + if (member.IsApproved != isApproved.Value) update = true; member.IsApproved = isApproved.Value; } if (lastLoginDate.HasValue) { - needsUpdating.Add(member.LastLoginDate != lastLoginDate.Value); + if (member.LastLoginDate != lastLoginDate.Value) update = true; member.LastLoginDate = lastLoginDate.Value; } if (lastActivityDate.HasValue) { - needsUpdating.Add(member.LastActivityDate != lastActivityDate.Value); + if (member.LastActivityDate != lastActivityDate.Value) update = true; member.LastActivityDate = lastActivityDate.Value; } if (comment != null) { - needsUpdating.Add(member.Comment != comment); + if (member.Comment != comment) update = true; member.Comment = comment; } - //Don't persist anything if nothing has changed - if (needsUpdating.Any(x => x == true)) - { - provider.UpdateUser(member); - return Attempt.Succeed(member); - } + if (update == false) + return Attempt.Fail(member); - return Attempt.Fail(member); + provider.UpdateUser(member); + return Attempt.Succeed(member); } - + /// /// Returns the currently logged in IMember object - this should never be exposed to the front-end since it's returning a business logic entity! /// @@ -816,9 +814,12 @@ namespace Umbraco.Web.Security }); } - private string GetCacheKey(string key, params object[] additional) + private static string GetCacheKey(string key, params object[] additional) { - var sb = new StringBuilder(string.Format("{0}-{1}", typeof (MembershipHelper).Name, key)); + var sb = new StringBuilder(); + sb.Append(typeof (MembershipHelper).Name); + sb.Append("-"); + sb.Append(key); foreach (var s in additional) { sb.Append("-"); diff --git a/src/umbraco.cms/businesslogic/member/Member.cs b/src/umbraco.cms/businesslogic/member/Member.cs index f84803dc92..d2db9a4df6 100644 --- a/src/umbraco.cms/businesslogic/member/Member.cs +++ b/src/umbraco.cms/businesslogic/member/Member.cs @@ -24,10 +24,10 @@ namespace umbraco.cms.businesslogic.member { /// /// The Member class represents a member of the public website (not to be confused with umbraco users) - /// - /// Members are used when creating communities and collaborative applications using umbraco, or if there are a + /// + /// Members are used when creating communities and collaborative applications using umbraco, or if there are a /// need for identifying or authentifying the visitor. (extranets, protected/private areas of the public website) - /// + /// /// Inherits generic datafields from it's baseclass content. /// [Obsolete("Use the MemberService and the Umbraco.Core.Models.Member models instead")] @@ -40,16 +40,16 @@ namespace umbraco.cms.businesslogic.member // zb-00004 #29956 : refactor cookies names & handling - private const string _sQLOptimizedMany = @" - select - umbracoNode.id, umbracoNode.uniqueId, umbracoNode.level, - umbracoNode.parentId, umbracoNode.path, umbracoNode.sortOrder, umbracoNode.createDate, - umbracoNode.nodeUser, umbracoNode.text, + private const string _sQLOptimizedMany = @" + select + umbracoNode.id, umbracoNode.uniqueId, umbracoNode.level, + umbracoNode.parentId, umbracoNode.path, umbracoNode.sortOrder, umbracoNode.createDate, + umbracoNode.nodeUser, umbracoNode.text, cmsMember.Email, cmsMember.LoginName, cmsMember.Password - from umbracoNode + from umbracoNode inner join cmsContent on cmsContent.nodeId = umbracoNode.id inner join cmsMember on cmsMember.nodeId = cmsContent.nodeId - where umbracoNode.nodeObjectType = @nodeObjectType AND {0} + where umbracoNode.nodeObjectType = @nodeObjectType AND {0} order by {1}"; #endregion @@ -82,7 +82,7 @@ namespace umbraco.cms.businesslogic.member public Member(Guid id) : base(id) { } /// - /// Initializes a new instance of the Member class, with an option to only initialize + /// Initializes a new instance of the Member class, with an option to only initialize /// the data used by the tree in the umbraco console. /// /// Identifier @@ -96,7 +96,7 @@ namespace umbraco.cms.businesslogic.member #region Static methods /// /// A list of all members in the current umbraco install - /// + /// /// Note: is ressource intensive, use with care. /// public static Member[] GetAll @@ -122,7 +122,7 @@ namespace umbraco.cms.businesslogic.member public static Member[] getAllOtherMembers() { - //NOTE: This hasn't been ported to the new service layer because it is an edge case, it is only used to render the tree nodes but in v7 we plan on + //NOTE: This hasn't been ported to the new service layer because it is an edge case, it is only used to render the tree nodes but in v7 we plan on // changing how the members are shown and not having to worry about letters. var ids = new List(); @@ -130,10 +130,10 @@ namespace umbraco.cms.businesslogic.member string.Format(_sQLOptimizedMany.Trim(), "LOWER(SUBSTRING(text, 1, 1)) NOT IN ('a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z')", "umbracoNode.text"), SqlHelper.CreateParameter("@nodeObjectType", Member._objectType))) { - + while (dr.Read()) { - ids.Add(dr.GetInt("id")); + ids.Add(dr.GetInt("id")); } } @@ -215,7 +215,7 @@ namespace umbraco.cms.businesslogic.member /// The new member public static Member MakeNew(string Name, string LoginName, string Email, MemberType mbt, IUser u) { - if (mbt == null) throw new ArgumentNullException("mbt"); + if (mbt == null) throw new ArgumentNullException("mbt"); var loginName = (string.IsNullOrEmpty(LoginName) == false) ? LoginName : Name; var provider = MembershipProviderExtensions.GetMembersMembershipProvider(); @@ -236,7 +236,7 @@ namespace umbraco.cms.businesslogic.member return null; var legacy = new Member(model); - + legacy.Save(); return legacy; @@ -244,7 +244,7 @@ namespace umbraco.cms.businesslogic.member /// /// Retrieve a member given the loginname - /// + /// /// Used when authentifying the Member /// /// The unique Loginname @@ -261,7 +261,7 @@ namespace umbraco.cms.businesslogic.member /// /// Retrieve a Member given an email, the first if there multiple members with same email - /// + /// /// Used when authentifying the Member /// /// The email of the member @@ -279,7 +279,7 @@ namespace umbraco.cms.businesslogic.member /// /// Retrieve Members given an email - /// + /// /// Used when authentifying a Member /// /// The email of the member(s) @@ -298,7 +298,7 @@ namespace umbraco.cms.businesslogic.member /// /// Retrieve a Member given the credentials - /// + /// /// Used when authentifying the member /// /// Member login @@ -373,9 +373,9 @@ namespace umbraco.cms.businesslogic.member /// /// Deletes all members of the membertype specified - /// + /// /// Used when a membertype is deleted - /// + /// /// Use with care /// /// The membertype which are being deleted @@ -581,16 +581,16 @@ namespace umbraco.cms.businesslogic.member var x = base.ToXml(xd, Deep); if (x.Attributes != null && x.Attributes["loginName"] == null) { - x.Attributes.Append(XmlHelper.AddAttribute(xd, "loginName", LoginName)); + x.Attributes.Append(XmlHelper.AddAttribute(xd, "loginName", LoginName)); } if (x.Attributes != null && x.Attributes["email"] == null) { - x.Attributes.Append(XmlHelper.AddAttribute(xd, "email", Email)); + x.Attributes.Append(XmlHelper.AddAttribute(xd, "email", Email)); } if (x.Attributes != null && x.Attributes["key"] == null) { - x.Attributes.Append(XmlHelper.AddAttribute(xd, "key", UniqueId.ToString())); - } + x.Attributes.Append(XmlHelper.AddAttribute(xd, "key", UniqueId.ToString())); + } return x; } @@ -616,13 +616,13 @@ namespace umbraco.cms.businesslogic.member } /// - /// Sets the password for the user - ensure it is encrypted or hashed based on the active membership provider - you must + /// Sets the password for the user - ensure it is encrypted or hashed based on the active membership provider - you must /// call Save() after using this method /// /// public void ChangePassword(string newPassword) { - MemberItem.RawPasswordValue = newPassword; + MemberItem.RawPasswordValue = newPassword; } /// @@ -649,7 +649,7 @@ namespace umbraco.cms.businesslogic.member SqlHelper.ExecuteNonQuery("INSERT INTO cmsMember2MemberGroup (member, memberGroup) values (@id, @groupId)", parameters); PopulateGroups(); - + } /// @@ -665,8 +665,6 @@ namespace umbraco.cms.businesslogic.member } #endregion - - #region Private methods private void PopulateGroups() @@ -682,9 +680,9 @@ namespace umbraco.cms.businesslogic.member if (group != null) { temp.Add(dr.GetInt("memberGroup"), group); - } + } } - + } _groups = temp; } @@ -700,11 +698,11 @@ namespace umbraco.cms.businesslogic.member /// /// Method is used when logging a member in. - /// + /// /// Adds the member to the cache of logged in members - /// + /// /// Uses cookiebased recognition - /// + /// /// Can be used in the runtime /// /// The member to log in @@ -741,11 +739,11 @@ namespace umbraco.cms.businesslogic.member // zb-00035 #29931 : remove old cookie code /// /// Method is used when logging a member in. - /// + /// /// Adds the member to the cache of logged in members - /// + /// /// Uses cookie or session based recognition - /// + /// /// Can be used in the runtime /// /// The member to log in @@ -781,7 +779,7 @@ namespace umbraco.cms.businesslogic.member /// /// Removes the member from the cache - /// + /// /// Can be used in the public website /// /// Member to remove @@ -793,7 +791,7 @@ namespace umbraco.cms.businesslogic.member /// /// Removes the member from the cache - /// + /// /// Can be used in the public website /// /// Node Id of the member to remove @@ -801,11 +799,11 @@ namespace umbraco.cms.businesslogic.member public static void RemoveMemberFromCache(int NodeId) { ApplicationContext.Current.ApplicationCache.RuntimeCache.ClearCacheItem(GetCacheKey(NodeId)); - } + } /// /// Retrieve a collection of members in the cache - /// + /// /// Can be used from the public website /// /// A collection of cached members @@ -824,7 +822,7 @@ namespace umbraco.cms.businesslogic.member /// /// Retrieve a member from the cache - /// + /// /// Can be used from the public website /// /// Id of the member @@ -840,7 +838,7 @@ namespace umbraco.cms.businesslogic.member /// /// An indication if the current visitor is logged in - /// + /// /// Can be used from the public website /// /// True if the the current visitor is logged in @@ -907,16 +905,15 @@ namespace umbraco.cms.businesslogic.member #endregion - #region Membership helper class used for encryption methods /// /// ONLY FOR INTERNAL USE. - /// This is needed due to a design flaw where the Umbraco membership provider is located + /// This is needed due to a design flaw where the Umbraco membership provider is located /// in a separate project referencing this project, which means we can't call special methods /// directly on the UmbracoMemberShipMember class. - /// This is a helper implementation only to be able to use the encryption functionality + /// This is a helper implementation only to be able to use the encryption functionality /// of the membership provides (which are protected). - /// + /// /// ... which means this class should have been marked internal with a Friend reference to the other assembly right?? /// internal class MemberShipHelper : MembershipProvider @@ -1085,8 +1082,5 @@ namespace umbraco.cms.businesslogic.member } } #endregion - } - - } \ No newline at end of file