Review: Allow Duplicate Email for Members (#16202)

* init

* Aligned default values on security settings.

* Added validator for security settings.

* Provide default implementation for get members by email.

* Refactored constructor of MemberController.

* Validate on unique member email only when configured to do so.

* Further code tidy and use of DI in constructor.

* Used new constructor in tests.

* Add unit test for modified behaviour.

* Removed validator for security settings (it's not necessary, I got confused with users and members).

* Spelling.

---------

Co-authored-by: Andy Butland <[email protected]>

Author: jasont0101

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
     /// <summary>
     ///     Gets or sets a value indicating whether the user's email address is to be considered as their username.
     /// </summary>
-    public bool UsernameIsEmail { get; set; } = true;
+    [DefaultValue(StaticUsernameIsEmail)]
+    public bool UsernameIsEmail { get; set; } = StaticUsernameIsEmail;
+
+    /// <summary>
+    ///     Gets or sets a value indicating whether the member's email address must be unique.
+    /// </summary>
+    [DefaultValue(StaticMemberRequireUniqueEmail)]
+    public bool MemberRequireUniqueEmail { get; set; } = StaticMemberRequireUniqueEmail;
 
     /// <summary>
     ///     Gets or sets the set of allowed characters for a username

src/Umbraco.Core/Services/IMemberService.cs

@@ -210,6 +210,21 @@ IMember CreateMemberWithIdentity(string username, string email, string name, str
     /// </returns>
     IMember? GetById(int id);
 
+    /// <summary>
+    ///     Get an list of <see cref="IMember"/> for all members with the specified email.
+    /// </summary>
+    //// <param name="email">Email to use for retrieval</param>
+    /// <returns>
+    ///     <see cref="IEnumerable{IMember}" />
+    /// </returns>
+    IEnumerable<IMember> 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));
+
     /// <summary>
     ///     Gets all Members for the specified MemberType alias
     /// </summary>

src/Umbraco.Core/Services/MemberService.cs

@@ -389,16 +389,23 @@ public IEnumerable<IMember> GetAll(
         }
 
         /// <summary>
-        /// Get an <see cref="IMember"/> by email
+        /// Get an <see cref="IMember"/> by email. If RequireUniqueEmailForMembers is set to false, then the first member found with the specified email will be returned.
         /// </summary>
         /// <param name="email">Email to use for retrieval</param>
         /// <returns><see cref="IMember"/></returns>
-        public IMember? GetByEmail(string email)
+        public IMember? GetByEmail(string email) => GetMembersByEmail(email).FirstOrDefault();
+
+        /// <summary>
+        /// Get an list of <see cref="IMember"/> for all members with the specified email.
+        /// </summary>
+        /// <param name="email">Email to use for retrieval</param>
+        /// <returns><see cref="IEnumerable{IMember}"/></returns>
+        public IEnumerable<IMember> GetMembersByEmail(string email)
         {
             using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
             scope.ReadLock(Constants.Locks.MemberTree);
             IQuery<IMember> query = Query<IMember>().Where(x => x.Email.Equals(email));
-            return _memberRepository.Get(query)?.FirstOrDefault();
+            return _memberRepository.Get(query);
         }
 
         /// <summary>

src/Umbraco.Web.BackOffice/Controllers/MemberController.cs

@@ -8,8 +8,11 @@
 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.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;
 
     /// <summary>
     ///     Initializes a new instance of the <see cref="MemberController" /> class.
@@ -75,6 +78,7 @@ public class MemberController : ContentControllerBase
     /// <param name="passwordChanger">The password changer</param>
     /// <param name="scopeProvider">The core scope provider</param>
     /// <param name="twoFactorLoginService">The two factor login service</param>
+    /// <param name="securitySettings">The security settings</param>
     [ActivatorUtilitiesConstructor]
     public MemberController(
         ICultureDictionary cultureDictionary,
@@ -92,7 +96,8 @@ public MemberController(
         IJsonSerializer jsonSerializer,
         IPasswordChanger<MemberIdentityUser> passwordChanger,
         ICoreScopeProvider scopeProvider,
-        ITwoFactorLoginService twoFactorLoginService)
+        ITwoFactorLoginService twoFactorLoginService,
+        IOptions<SecuritySettings> securitySettings)
         : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, jsonSerializer)
     {
         _propertyEditors = propertyEditors;
@@ -108,9 +113,49 @@ public MemberController(
         _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<MemberIdentityUser> 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<IOptions<SecuritySettings>>())
+    {
+    }
+
+    [Obsolete("Please use the constructor that takes all paramters. Scheduled for removal in V14")]
     public MemberController(
         ICultureDictionary cultureDictionary,
         ILoggerFactory loggerFactory,
@@ -461,7 +506,7 @@ private async Task<ActionResult<bool>> CreateMemberAsync(MemberSave contentItem)
         }
 
         // 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 @@ private async Task<bool> ValidateMemberDataAsync(MemberSave contentItem)
             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;

src/Umbraco.Web.BackOffice/Filters/MemberSaveModelValidator.cs

@@ -2,8 +2,12 @@
 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;
 /// <summary>
 ///     Validator for <see cref="ContentItemSave" />
 /// </summary>
-internal class
-    MemberSaveModelValidator : ContentModelValidator<IMember, MemberSave, IContentProperties<ContentPropertyBasic>>
+internal class MemberSaveModelValidator : ContentModelValidator<IMember, MemberSave, IContentProperties<ContentPropertyBasic>>
 {
     private readonly IBackOfficeSecurity? _backofficeSecurity;
     private readonly IMemberService _memberService;
     private readonly IMemberTypeService _memberTypeService;
     private readonly IShortStringHelper _shortStringHelper;
+    private readonly SecuritySettings _securitySettings;
 
     public MemberSaveModelValidator(
-        ILogger<MemberSaveModelValidator> logger,
-        IBackOfficeSecurity? backofficeSecurity,
-        IMemberTypeService memberTypeService,
-        IMemberService memberService,
-        IShortStringHelper shortStringHelper,
-        IPropertyValidationService propertyValidationService)
-        : base(logger, propertyValidationService)
+       ILogger<MemberSaveModelValidator> 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 @@ public override bool ValidatePropertiesData(
                 $"{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" }),

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,23 +27,24 @@ private sealed class MemberSaveValidationFilter : IActionFilter
         private readonly IMemberTypeService _memberTypeService;
         private readonly IPropertyValidationService _propertyValidationService;
         private readonly IShortStringHelper _shortStringHelper;
+        private readonly SecuritySettings _securitySettings;
 
         public MemberSaveValidationFilter(
             ILoggerFactory loggerFactory,
             IBackOfficeSecurityAccessor backofficeSecurityAccessor,
             IMemberTypeService memberTypeService,
             IMemberService memberService,
             IShortStringHelper shortStringHelper,
-            IPropertyValidationService propertyValidationService)
+            IPropertyValidationService propertyValidationService,
+            IOptions<SecuritySettings> 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 @@ public void OnActionExecuting(ActionExecutingContext context)
                 _memberTypeService,
                 _memberService,
                 _shortStringHelper,
-                _propertyValidationService);
+                _propertyValidationService,
+                _securitySettings);
             //now do each validation step
             if (contentItemValidator.ValidateExistingContent(model, context))
             {

src/Umbraco.Web.Common/Security/ConfigureMemberIdentityOptions.cs

@@ -24,7 +24,7 @@ public void Configure(IdentityOptions options)
         options.SignIn.RequireConfirmedEmail = false; // not implemented
         options.SignIn.RequireConfirmedPhoneNumber = false; // not implemented
 
-        options.User.RequireUniqueEmail = true;
+        options.User.RequireUniqueEmail = _securitySettings.MemberRequireUniqueEmail;
 
         // Support validation of member names using Down-Level Logon Name format
         options.User.AllowedUserNameCharacters = _securitySettings.AllowedUserNameCharacters;