diff --git a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs b/src/Umbraco.Infrastructure/Security/MembersUserStore.cs index 9405992ba8..6b4a735988 100644 --- a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MembersUserStore.cs @@ -595,6 +595,12 @@ namespace Umbraco.Cms.Core.Security } } + if (member.IsApproved != identityUserMember.IsApproved) + { + anythingChanged = true; + member.IsApproved = identityUserMember.IsApproved; + } + if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.UserName)) && member.Username != identityUserMember.UserName && identityUserMember.UserName.IsNullOrWhiteSpace() == false) { diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 1ba7f41d97..ad428d3b0f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -20,6 +20,7 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.PropertyEditors; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; @@ -58,6 +59,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IJsonSerializer _jsonSerializer; private readonly IShortStringHelper _shortStringHelper; private readonly IPasswordChanger _passwordChanger; + private readonly IScopeProvider _scopeProvider; /// /// Initializes a new instance of the class. @@ -90,7 +92,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IJsonSerializer jsonSerializer, - IPasswordChanger passwordChanger) + IPasswordChanger passwordChanger, + IScopeProvider scopeProvider) : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, jsonSerializer) { _propertyEditors = propertyEditors; @@ -104,6 +107,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers _jsonSerializer = jsonSerializer; _shortStringHelper = shortStringHelper; _passwordChanger = passwordChanger; + _scopeProvider = scopeProvider; } /// @@ -260,9 +264,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(forDisplay); } + // Create a scope here which will wrap all child data operations in a single transaction. + // We'll complete this at the end of this method if everything succeeeds, else + // all data operations will roll back. + using IScope scope = _scopeProvider.CreateScope(); + // Depending on the action we need to first do a create or update using the membership manager // this ensures that passwords are formatted correctly and also performs the validation on the provider itself. - switch (contentItem.Action) { case ContentSaveAction.Save: @@ -286,9 +294,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return NotFound(); } - // TODO: There's 3 things saved here and we should do this all in one transaction, which we can do here by wrapping in a scope - // but it would be nicer to have this taken care of within the Save method itself - // return the updated model MemberDisplay display = _umbracoMapper.Map(contentItem.PersistedContent); @@ -299,19 +304,20 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(display, StatusCodes.Status403Forbidden); } - ILocalizedTextService localizedTextService = _localizedTextService; - // put the correct messages in switch (contentItem.Action) { case ContentSaveAction.Save: case ContentSaveAction.SaveNew: display.AddSuccessNotification( - localizedTextService.Localize("speechBubbles/editMemberSaved"), - localizedTextService.Localize("speechBubbles/editMemberSaved")); + _localizedTextService.Localize("speechBubbles/editMemberSaved"), + _localizedTextService.Localize("speechBubbles/editMemberSaved")); break; } + // Mark transaction to commit all changes + scope.Complete(); + return display; } @@ -328,7 +334,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } // map the custom properties - this will already be set for new entities in our member binder - contentItem.PersistedContent.Email = contentItem.Email; + contentItem.PersistedContent.IsApproved = contentItem.IsApproved; + contentItem.PersistedContent.Email = contentItem.Email.Trim(); contentItem.PersistedContent.Username = contentItem.Username; // use the base method to map the rest of the properties @@ -394,15 +401,29 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers _memberService.Save(member); contentItem.PersistedContent = member; - await AddOrUpdateRoles(contentItem, identityMember); + ActionResult rolesChanged = await AddOrUpdateRoles(contentItem.Groups, identityMember); + if (!rolesChanged.Value && rolesChanged.Result != null) + { + return rolesChanged.Result; + } + return true; } /// - /// Update the member security data - /// If the password has been reset then this method will return the reset/generated password, otherwise will return null. + /// Update existing member data /// /// The member to save + /// + /// We need to use both IMemberService and ASP.NET Identity to do our updates because Identity is responsible for passwords/security. + /// When this method is called, the IMember will already have updated/mapped values from the http POST. + /// So then we do this in order: + /// 1. Deal with sensitive property values on IMember + /// 2. Use IMemberService to persist all changes + /// 3. Use ASP.NET and MemberUserManager to deal with lockouts + /// 4. Use ASP.NET, MemberUserManager and password changer to deal with passwords + /// 5. Deal with groups/roles + /// private async Task> UpdateMemberAsync(MemberSave contentItem) { contentItem.PersistedContent.WriterId = _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser.Id; @@ -420,6 +441,9 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers foreach (IPropertyType sensitiveProperty in sensitiveProperties) { + // TODO: This logic seems to deviate from the logic that is in v8 where we are explitly checking + // against 3 properties: Comments, IsApproved, IsLockedOut, is the v8 version incorrect? + ContentPropertyBasic destProp = contentItem.Properties.FirstOrDefault(x => x.Alias == sensitiveProperty.Alias); if (destProp != null) { @@ -430,27 +454,37 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } } - bool isLockedOut = contentItem.IsLockedOut; + // First save the IMember with mapped values before we start updating data with aspnet identity + _memberService.Save(contentItem.PersistedContent); - // if they were locked but now they are trying to be unlocked - if (contentItem.PersistedContent.IsLockedOut && isLockedOut == false) - { - contentItem.PersistedContent.IsLockedOut = false; - contentItem.PersistedContent.FailedPasswordAttempts = 0; - } - else if (!contentItem.PersistedContent.IsLockedOut && isLockedOut) - { - // NOTE: This should not ever happen unless someone is mucking around with the request data. - // An admin cannot simply lock a user, they get locked out by password attempts, but an admin can un-approve them - ModelState.AddModelError("custom", "An admin cannot lock a user"); - } + bool needsResync = false; MembersIdentityUser identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); if (identityMember == null) { - return new ValidationErrorResult("Identity member was not found"); + return new ValidationErrorResult("Member was not found"); } + // Handle unlocking with the member manager (takes care of other nuances) + if (identityMember.IsLockedOut && contentItem.IsLockedOut == false) + { + IdentityResult unlockResult = await _memberManager.SetLockoutEndDateAsync(identityMember, DateTimeOffset.Now.AddMinutes(-1)); + if (unlockResult.Succeeded == false) + { + return new ValidationErrorResult( + $"Could not unlock for member {contentItem.Id} - error {unlockResult.Errors.ToErrorMessage()}"); + } + needsResync = true; + } + else if (identityMember.IsLockedOut == false && contentItem.IsLockedOut) + { + // NOTE: This should not ever happen unless someone is mucking around with the request data. + // An admin cannot simply lock a user, they get locked out by password attempts, but an admin can unlock them + return new ValidationErrorResult("An admin cannot lock a member"); + } + + // If we're changing the password... + // Handle changing with the member manager & password changer (takes care of other nuances) if (contentItem.Password != null) { IdentityResult validatePassword = await _memberManager.ValidatePasswordAsync(contentItem.Password.NewPassword); @@ -465,19 +499,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult("Member ID was not valid"); } - IMember foundMember = _memberService.GetById(intId.Result); - if (foundMember == null) - { - return new ValidationErrorResult("Member was not found"); - } - - IUser currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser; - // if the current user has access to reset/manually change the password - if (currentUser.HasSectionAccess(Constants.Applications.Members) == false) - { - return new ValidationErrorResult("The current user is not authorized"); - } - var changingPasswordModel = new ChangingPasswordModel { Id = intId.Result, @@ -485,9 +506,10 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers NewPassword = contentItem.Password.NewPassword, }; + // Change and persist the password Attempt passwordChangeResult = await _passwordChanger.ChangePasswordWithIdentityAsync(changingPasswordModel, _memberManager); - if (passwordChangeResult.Result.ChangeError?.MemberNames != null) + if (!passwordChangeResult.Success) { foreach (string memberName in passwordChangeResult.Result.ChangeError?.MemberNames) { @@ -496,33 +518,27 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(new SimpleValidationModel(ModelState.ToErrorDictionary())); } - if (!passwordChangeResult.Success) - { - return new ValidationErrorResult("The password could not be changed"); - } - - // get the identity member now the password and dates have changed - identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); - - //TODO: confirm this is correct - contentItem.PersistedContent.RawPasswordValue = identityMember.PasswordHash; - - if (identityMember.LastPasswordChangeDateUtc != null) - { - contentItem.PersistedContent.LastPasswordChangeDate = (DateTime)identityMember.LastPasswordChangeDateUtc; - } + needsResync = true; } - IdentityResult updatedResult = await _memberManager.UpdateAsync(identityMember); - - if (updatedResult.Succeeded == false) + // Update the roles and check for changes + ActionResult rolesChanged = await AddOrUpdateRoles(contentItem.Groups, identityMember); + if (!rolesChanged.Value && rolesChanged.Result != null) { - return new ValidationErrorResult(updatedResult.Errors.ToErrorMessage()); + return rolesChanged.Result; + } + else + { + needsResync = true; } - _memberService.Save(contentItem.PersistedContent); + // If there have been underlying changes made by ASP.NET Identity, then we need to resync the + // IMember on the PersistedContent with what is stored since it will be mapped to display. + if (needsResync) + { + contentItem.PersistedContent = _memberService.GetById(contentItem.PersistedContent.Id); + } - await AddOrUpdateRoles(contentItem, identityMember); return true; } @@ -585,10 +601,12 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// Add or update the identity roles /// - /// The member content item + /// The groups to updates /// The member as an identity user - private async Task AddOrUpdateRoles(MemberSave contentItem, MembersIdentityUser identityMember) + private async Task> AddOrUpdateRoles(IEnumerable groups, MembersIdentityUser identityMember) { + var hasChanges = false; + // We're gonna look up the current roles now because the below code can cause // events to be raised and developers could be manually adding roles to members in // their handlers. If we don't look this up now there's a chance we'll just end up @@ -597,22 +615,34 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers // find the ones to remove and remove them IEnumerable roles = currentRoles.ToList(); - string[] rolesToRemove = roles.Except(contentItem.Groups).ToArray(); + string[] rolesToRemove = roles.Except(groups).ToArray(); // Now let's do the role provider stuff - now that we've saved the content item (that is important since // if we are changing the username, it must be persisted before looking up the member roles). if (rolesToRemove.Any()) { - IdentityResult rolesIdentityResult = await _memberManager.RemoveFromRolesAsync(identityMember, rolesToRemove); + IdentityResult identityResult = await _memberManager.RemoveFromRolesAsync(identityMember, rolesToRemove); + if (!identityResult.Succeeded) + { + return ValidationErrorResult.CreateNotificationValidationErrorResult(identityResult.Errors.ToErrorMessage()); + } + hasChanges = true; } // find the ones to add and add them - string[] toAdd = contentItem.Groups.Except(roles).ToArray(); + string[] toAdd = groups.Except(roles).ToArray(); if (toAdd.Any()) { // add the ones submitted IdentityResult identityResult = await _memberManager.AddToRolesAsync(identityMember, toAdd); + if (!identityResult.Succeeded) + { + return ValidationErrorResult.CreateNotificationValidationErrorResult(identityResult.Errors.ToErrorMessage()); + } + hasChanges = true; } + + return hasChanges; } /// diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 54da40ac18..7ca83b00ff 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -789,7 +789,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers continue; } - var unlockResult = await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.Now); + var unlockResult = await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.Now.AddMinutes(-1)); if (unlockResult.Succeeded == false) { return new ValidationErrorResult( diff --git a/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml b/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml index 79cbe8305a..605e355894 100644 --- a/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml +++ b/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml @@ -1,4 +1,4 @@ -@using Umbraco.Extensions +@using Umbraco.Extensions @{ Layout = null; } @@ -74,6 +74,6 @@ }; - +