diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index f1005e5d1b..e68162e6ef 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -19,6 +19,8 @@ public class SecuritySettings internal const bool StaticAllowEditInvariantFromNonDefault = false; internal const bool StaticAllowConcurrentLogins = false; internal const string StaticAuthCookieName = "UMB_UCONTEXT"; + internal const bool StaticUsernameIsEmail = true; + internal const bool StaticMemberRequireUniqueEmail = true; internal const string StaticAllowedUserNameCharacters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._@+\\"; @@ -58,7 +60,14 @@ public class SecuritySettings /// /// Gets or sets a value indicating whether the user's email address is to be considered as their username. /// - public bool UsernameIsEmail { get; set; } = true; + [DefaultValue(StaticUsernameIsEmail)] + public bool UsernameIsEmail { get; set; } = StaticUsernameIsEmail; + + /// + /// Gets or sets a value indicating whether the member's email address must be unique. + /// + [DefaultValue(StaticMemberRequireUniqueEmail)] + public bool MemberRequireUniqueEmail { get; set; } = StaticMemberRequireUniqueEmail; /// /// Gets or sets the set of allowed characters for a username diff --git a/src/Umbraco.Core/Services/IMemberService.cs b/src/Umbraco.Core/Services/IMemberService.cs index a1be0b4a4c..7d78a979c8 100644 --- a/src/Umbraco.Core/Services/IMemberService.cs +++ b/src/Umbraco.Core/Services/IMemberService.cs @@ -210,6 +210,21 @@ public interface IMemberService : IMembershipMemberService /// IMember? GetById(int id); + /// + /// Get an list of for all members with the specified email. + /// + //// Email to use for retrieval + /// + /// + /// + IEnumerable GetMembersByEmail(string email) + => + // TODO (V16): Remove this default implementation. + // The following is very inefficient, but will return the correct data, so probably better than throwing a NotImplementedException + // in the default implentation here, for, presumably rare, cases where a custom IMemberService implementation has been registered and + // does not override this method. + GetAllMembers().Where(x => x.Email.Equals(email)); + /// /// Gets all Members for the specified MemberType alias /// diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index 43b5b8f28b..493ab313a7 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -389,16 +389,23 @@ namespace Umbraco.Cms.Core.Services } /// - /// Get an by email + /// Get an by email. If RequireUniqueEmailForMembers is set to false, then the first member found with the specified email will be returned. /// /// Email to use for retrieval /// - public IMember? GetByEmail(string email) + public IMember? GetByEmail(string email) => GetMembersByEmail(email).FirstOrDefault(); + + /// + /// Get an list of for all members with the specified email. + /// + /// Email to use for retrieval + /// + public IEnumerable GetMembersByEmail(string email) { using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); scope.ReadLock(Constants.Locks.MemberTree); IQuery query = Query().Where(x => x.Email.Equals(email)); - return _memberRepository.Get(query)?.FirstOrDefault(); + return _memberRepository.Get(query); } /// diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 8851a73a2b..4a18bf4620 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -8,8 +8,11 @@ using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.ContentApps; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Dictionary; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Mapping; @@ -26,7 +29,6 @@ using Umbraco.Cms.Web.BackOffice.Filters; using Umbraco.Cms.Web.BackOffice.ModelBinders; using Umbraco.Cms.Web.Common.Attributes; using Umbraco.Cms.Web.Common.Authorization; -using Umbraco.Cms.Web.Common.DependencyInjection; using Umbraco.Cms.Web.Common.Filters; using Umbraco.Cms.Web.Common.Security; using Umbraco.Extensions; @@ -55,6 +57,7 @@ public class MemberController : ContentControllerBase private readonly ITwoFactorLoginService _twoFactorLoginService; private readonly IShortStringHelper _shortStringHelper; private readonly IUmbracoMapper _umbracoMapper; + private readonly SecuritySettings _securitySettings; /// /// Initializes a new instance of the class. @@ -75,6 +78,7 @@ public class MemberController : ContentControllerBase /// The password changer /// The core scope provider /// The two factor login service + /// The security settings [ActivatorUtilitiesConstructor] public MemberController( ICultureDictionary cultureDictionary, @@ -92,7 +96,8 @@ public class MemberController : ContentControllerBase IJsonSerializer jsonSerializer, IPasswordChanger passwordChanger, ICoreScopeProvider scopeProvider, - ITwoFactorLoginService twoFactorLoginService) + ITwoFactorLoginService twoFactorLoginService, + IOptions securitySettings) : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, jsonSerializer) { _propertyEditors = propertyEditors; @@ -108,9 +113,49 @@ public class MemberController : ContentControllerBase _passwordChanger = passwordChanger; _scopeProvider = scopeProvider; _twoFactorLoginService = twoFactorLoginService; + _securitySettings = securitySettings.Value; } - [Obsolete("Use constructor that also takes an ITwoFactorLoginService. Scheduled for removal in V13")] + [Obsolete("Please use the constructor that takes all paramters. Scheduled for removal in V14")] + public MemberController( + ICultureDictionary cultureDictionary, + ILoggerFactory loggerFactory, + IShortStringHelper shortStringHelper, + IEventMessagesFactory eventMessages, + ILocalizedTextService localizedTextService, + PropertyEditorCollection propertyEditors, + IUmbracoMapper umbracoMapper, + IMemberService memberService, + IMemberTypeService memberTypeService, + IMemberManager memberManager, + IDataTypeService dataTypeService, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor, + IJsonSerializer jsonSerializer, + IPasswordChanger passwordChanger, + ICoreScopeProvider scopeProvider, + ITwoFactorLoginService twoFactorLoginService) + : this( + cultureDictionary, + loggerFactory, + shortStringHelper, + eventMessages, + localizedTextService, + propertyEditors, + umbracoMapper, + memberService, + memberTypeService, + memberManager, + dataTypeService, + backOfficeSecurityAccessor, + jsonSerializer, + passwordChanger, + scopeProvider, + twoFactorLoginService, + StaticServiceProvider.Instance.GetRequiredService>()) + { + } + + [Obsolete("Please use the constructor that takes all paramters. Scheduled for removal in V14")] public MemberController( ICultureDictionary cultureDictionary, ILoggerFactory loggerFactory, @@ -461,7 +506,7 @@ public class MemberController : ContentControllerBase } // now re-look up the member, which will now exist - IMember? member = _memberService.GetByEmail(contentItem.Email); + IMember? member = _memberService.GetByUsername(contentItem.Username); if (member is null) { @@ -699,13 +744,16 @@ public class MemberController : ContentControllerBase return false; } - IMember? byEmail = _memberService.GetByEmail(contentItem.Email); - if (byEmail != null && byEmail.Key != contentItem.Key) + if (_securitySettings.MemberRequireUniqueEmail) { - ModelState.AddPropertyError( - new ValidationResult("Email address is already in use", new[] { "value" }), - $"{Constants.PropertyEditors.InternalGenericPropertiesPrefix}email"); - return false; + IMember? byEmail = _memberService.GetByEmail(contentItem.Email); + if (byEmail != null && byEmail.Key != contentItem.Key) + { + ModelState.AddPropertyError( + new ValidationResult("Email address is already in use", new[] { "value" }), + $"{Constants.PropertyEditors.InternalGenericPropertiesPrefix}email"); + return false; + } } return true; diff --git a/src/Umbraco.Web.BackOffice/Filters/MemberSaveModelValidator.cs b/src/Umbraco.Web.BackOffice/Filters/MemberSaveModelValidator.cs index 6b29803e05..68d37bba3c 100644 --- a/src/Umbraco.Web.BackOffice/Filters/MemberSaveModelValidator.cs +++ b/src/Umbraco.Web.BackOffice/Filters/MemberSaveModelValidator.cs @@ -2,8 +2,12 @@ using System.ComponentModel.DataAnnotations; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Security; @@ -16,27 +20,29 @@ namespace Umbraco.Cms.Web.BackOffice.Filters; /// /// Validator for /// -internal class - MemberSaveModelValidator : ContentModelValidator> +internal class MemberSaveModelValidator : ContentModelValidator> { private readonly IBackOfficeSecurity? _backofficeSecurity; private readonly IMemberService _memberService; private readonly IMemberTypeService _memberTypeService; private readonly IShortStringHelper _shortStringHelper; + private readonly SecuritySettings _securitySettings; public MemberSaveModelValidator( - ILogger logger, - IBackOfficeSecurity? backofficeSecurity, - IMemberTypeService memberTypeService, - IMemberService memberService, - IShortStringHelper shortStringHelper, - IPropertyValidationService propertyValidationService) - : base(logger, propertyValidationService) + ILogger logger, + IBackOfficeSecurity? backofficeSecurity, + IMemberTypeService memberTypeService, + IMemberService memberService, + IShortStringHelper shortStringHelper, + IPropertyValidationService propertyValidationService, + SecuritySettings securitySettings) + : base(logger, propertyValidationService) { _backofficeSecurity = backofficeSecurity; _memberTypeService = memberTypeService ?? throw new ArgumentNullException(nameof(memberTypeService)); _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); _shortStringHelper = shortStringHelper ?? throw new ArgumentNullException(nameof(shortStringHelper)); + _securitySettings = securitySettings; } public override bool ValidatePropertiesData( @@ -64,8 +70,7 @@ internal class $"{Constants.PropertyEditors.InternalGenericPropertiesPrefix}email"); } - var validEmail = ValidateUniqueEmail(model); - if (validEmail == false) + if (_securitySettings.MemberRequireUniqueEmail && ValidateUniqueEmail(model) is false) { modelState.AddPropertyError( new ValidationResult("Email address is already in use", new[] { "value" }), diff --git a/src/Umbraco.Web.BackOffice/Filters/MemberSaveValidationAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/MemberSaveValidationAttribute.cs index 61e119b66a..568d1e8240 100644 --- a/src/Umbraco.Web.BackOffice/Filters/MemberSaveValidationAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/MemberSaveValidationAttribute.cs @@ -1,6 +1,8 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; @@ -25,6 +27,7 @@ internal sealed class MemberSaveValidationAttribute : TypeFilterAttribute private readonly IMemberTypeService _memberTypeService; private readonly IPropertyValidationService _propertyValidationService; private readonly IShortStringHelper _shortStringHelper; + private readonly SecuritySettings _securitySettings; public MemberSaveValidationFilter( ILoggerFactory loggerFactory, @@ -32,16 +35,16 @@ internal sealed class MemberSaveValidationAttribute : TypeFilterAttribute IMemberTypeService memberTypeService, IMemberService memberService, IShortStringHelper shortStringHelper, - IPropertyValidationService propertyValidationService) + IPropertyValidationService propertyValidationService, + IOptions securitySettings) { - _loggerFactory = loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory)); - _backofficeSecurityAccessor = backofficeSecurityAccessor ?? - throw new ArgumentNullException(nameof(backofficeSecurityAccessor)); - _memberTypeService = memberTypeService ?? throw new ArgumentNullException(nameof(memberTypeService)); - _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); - _shortStringHelper = shortStringHelper ?? throw new ArgumentNullException(nameof(shortStringHelper)); - _propertyValidationService = propertyValidationService ?? - throw new ArgumentNullException(nameof(propertyValidationService)); + _loggerFactory = loggerFactory; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _memberTypeService = memberTypeService; + _memberService = memberService; + _shortStringHelper = shortStringHelper; + _propertyValidationService = propertyValidationService; + _securitySettings = securitySettings.Value; } public void OnActionExecuting(ActionExecutingContext context) @@ -53,7 +56,8 @@ internal sealed class MemberSaveValidationAttribute : TypeFilterAttribute _memberTypeService, _memberService, _shortStringHelper, - _propertyValidationService); + _propertyValidationService, + _securitySettings); //now do each validation step if (contentItemValidator.ValidateExistingContent(model, context)) { diff --git a/src/Umbraco.Web.Common/Security/ConfigureMemberIdentityOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureMemberIdentityOptions.cs index 0fcc41d9d0..1c9b88b6cf 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureMemberIdentityOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureMemberIdentityOptions.cs @@ -24,7 +24,7 @@ public sealed class ConfigureMemberIdentityOptions : IConfigureOptions passwordChanger, IOptions globalSettings, - IUser user, ITwoFactorLoginService twoFactorLoginService) { // arrange SetupMemberTestData(out var fakeMemberData, out _, ContentSaveAction.SaveNew); + + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -84,7 +86,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); sut.ModelState.AddModelError("key", "Invalid model state"); Mock.Get(umbracoMembersUserManager) @@ -116,7 +119,6 @@ public class MemberControllerUnitTests IBackOfficeSecurity backOfficeSecurity, IPasswordChanger passwordChanger, IOptions globalSettings, - IUser user, ITwoFactorLoginService twoFactorLoginService) { // arrange @@ -138,6 +140,8 @@ public class MemberControllerUnitTests .Returns(() => member); Mock.Get(memberService).Setup(x => x.GetByUsername(It.IsAny())).Returns(() => member); + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -147,7 +151,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = await sut.PostSave(fakeMemberData); @@ -170,7 +175,6 @@ public class MemberControllerUnitTests IBackOfficeSecurity backOfficeSecurity, IPasswordChanger passwordChanger, IOptions globalSettings, - IUser user, ITwoFactorLoginService twoFactorLoginService) { // arrange @@ -192,6 +196,8 @@ public class MemberControllerUnitTests .Returns(() => member); Mock.Get(memberService).Setup(x => x.GetByUsername(It.IsAny())).Returns(() => member); + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -201,7 +207,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = await sut.PostSave(fakeMemberData); @@ -256,6 +263,8 @@ public class MemberControllerUnitTests .Returns(() => null) .Returns(() => member); + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -265,7 +274,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = await sut.PostSave(fakeMemberData); @@ -316,6 +326,8 @@ public class MemberControllerUnitTests .Returns(() => null) .Returns(() => member); + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -325,7 +337,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = await sut.PostSave(fakeMemberData); @@ -382,7 +395,6 @@ public class MemberControllerUnitTests IBackOfficeSecurity backOfficeSecurity, IPasswordChanger passwordChanger, IOptions globalSettings, - IUser user, ITwoFactorLoginService twoFactorLoginService) { // arrange @@ -403,6 +415,8 @@ public class MemberControllerUnitTests x => x.GetByEmail(It.IsAny())) .Returns(() => member); + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -412,7 +426,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = sut.PostSave(fakeMemberData).Result; @@ -424,6 +439,66 @@ public class MemberControllerUnitTests Assert.AreEqual(StatusCodes.Status400BadRequest, validation?.StatusCode); } + [Test] + [AutoMoqData] + public void PostSaveMember_SaveNew_WhenMemberEmailAlreadyExists_AndDuplicateEmailsAreAllowed_ExpectSuccessResponse( + [Frozen] IMemberManager umbracoMembersUserManager, + IMemberService memberService, + IMemberTypeService memberTypeService, + IMemberGroupService memberGroupService, + IDataTypeService dataTypeService, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor, + IBackOfficeSecurity backOfficeSecurity, + IPasswordChanger passwordChanger, + IOptions globalSettings, + ITwoFactorLoginService twoFactorLoginService) + { + // arrange + var member = SetupMemberTestData(out var fakeMemberData, out var memberDisplay, ContentSaveAction.SaveNew); + Mock.Get(umbracoMembersUserManager) + .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(() => IdentityResult.Success); + Mock.Get(umbracoMembersUserManager) + .Setup(x => x.ValidatePasswordAsync(It.IsAny())) + .ReturnsAsync(() => IdentityResult.Success); + Mock.Get(umbracoMembersUserManager) + .Setup(x => x.GetRolesAsync(It.IsAny())) + .ReturnsAsync(() => Array.Empty()); + Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); + Mock.Get(backOfficeSecurityAccessor).Setup(x => x.BackOfficeSecurity).Returns(backOfficeSecurity); + Mock.Get(memberService).SetupSequence( + x => x.GetByEmail(It.IsAny())) + .Returns(() => null) + .Returns(() => member); + Mock.Get(memberService).Setup(x => x.GetByUsername(It.IsAny())).Returns(() => member); + + Mock.Get(memberService).SetupSequence( + x => x.GetByEmail(It.IsAny())) + .Returns(() => member); + + var securitySettings = Options.Create(new SecuritySettings { MemberRequireUniqueEmail = false }); + + var sut = CreateSut( + memberService, + memberTypeService, + memberGroupService, + umbracoMembersUserManager, + dataTypeService, + backOfficeSecurityAccessor, + passwordChanger, + globalSettings, + twoFactorLoginService, + securitySettings); + + // act + var result = sut.PostSave(fakeMemberData).Result; + var validation = result.Result as ValidationErrorResult; + + // assert + Assert.IsNull(result.Result); + Assert.IsNotNull(result.Value); + } + [Test] [AutoMoqData] public async Task PostSaveMember_SaveExistingMember_WithNoRoles_Add1Role_ExpectSuccessResponse( @@ -472,6 +547,9 @@ public class MemberControllerUnitTests x => x.GetByEmail(It.IsAny())) .Returns(() => null) .Returns(() => member); + + var securitySettings = Options.Create(new SecuritySettings()); + var sut = CreateSut( memberService, memberTypeService, @@ -481,7 +559,8 @@ public class MemberControllerUnitTests backOfficeSecurityAccessor, passwordChanger, globalSettings, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); // act var result = await sut.PostSave(fakeMemberData); @@ -512,6 +591,7 @@ public class MemberControllerUnitTests /// Password changer class /// The global settings /// The two factor login service + /// The security settings /// A member controller for the tests private MemberController CreateSut( IMemberService memberService, @@ -522,7 +602,8 @@ public class MemberControllerUnitTests IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IPasswordChanger passwordChanger, IOptions globalSettings, - ITwoFactorLoginService twoFactorLoginService) + ITwoFactorLoginService twoFactorLoginService, + IOptions securitySettings) { var httpContextAccessor = new HttpContextAccessor(); @@ -623,7 +704,8 @@ public class MemberControllerUnitTests new ConfigurationEditorJsonSerializer(), passwordChanger, scopeProvider, - twoFactorLoginService); + twoFactorLoginService, + securitySettings); } ///