Allow Duplicate Email for Members #14784

jasont0101 · 2023-09-08T22:48:58Z

Prerequisites

I have added steps to test this contribution in the description below

Description

Oddly have had two project in a row that the client had the business need to allow member to have duplicate email address. While there are other way to resolve this issue, want to add (re-add?) this feature to Umbraco so that it was easy to enable with out having to work out complicated alternative contact email options. So, added the option to allow member accounts to be created with duplicate emails.

The current default behavior is that members must have unique emails. While it was possible to override the ASP.NET Identity provider value for RequireUniqueEmail in startup.cs or program.cs, validators still checked if the email existed and prevented the member from being added if the email was a duplicate.

Add a new appsettings.json option to the Umbraco:CMS:Security section, RequireUniqueEmailForMembers with a default value of true. This now sets the value for options.User.RequireUniqueEmail in the ConfigureMemberIdentityOptions class.

Also added a new method to the MemberService, GetMembersByEmail.The GetByEmail method will only return FirstOrDefault. This new method allows for the retrieval of all member with the same email.

This option only allows duplicate emails for members and not users. Usernames must still be unique.

TEST

Start up a new solution.
Go to the Members section and create a new member.
Create a second new member using the same email as the one just created. Validation should prevent the creation of this member.
Stop the solution.
Add the new security option to the appsettings.json, Umbraco:CMS:Security:RequireUniqueEmailForMembers, set the value to true and save file
Start up a the solution, go to the Member section, and repeat step 3 - 4, Validation should still prevent the creation of the member.
Set RequireUniqueEmailForMembers to false and save file.
Start up a the solution, go to the Member section, and repeat step 3 - 4, the member should now be created.
Stop the solution.

…o honor RequireUniqueEmailForMembers.

…sByEmail method

…o honor RequireUniqueEmailForMembers.

…sByEmail method

…/jasont0101/Umbraco-CMS into temp-RequireUniqueEmailForMember

github-actions · 2023-09-08T22:49:11Z

Hi there @jasont0101, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
The automated tests all pass (see "Checks" tab on this PR)
The level of security for this contribution is the same or improved
The level of performance for this contribution is the same or improved
Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

emmagarland · 2023-09-11T07:04:29Z

Hi @jasont0101!

Thanks for your PR to allow for duplicate member emails.

I'm just going to check with HQ about adding this into Umbraco.

Best wishes

Emma

src/Umbraco.Core/Configuration/Models/SecuritySettings.cs

src/Umbraco.Core/Services/MemberService.cs

ronaldbarendse · 2023-09-11T07:48:32Z

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

@@ -92,7 +96,8 @@ public class MemberController : ContentControllerBase
        IJsonSerializer jsonSerializer,
        IPasswordChanger<MemberIdentityUser> passwordChanger,
        ICoreScopeProvider scopeProvider,
-        ITwoFactorLoginService twoFactorLoginService)
+        ITwoFactorLoginService twoFactorLoginService,
+        IOptionsSnapshot<SecuritySettings>? securitySettings)


Any reason for using IOptionsSnapshot<> instead of IOptions<> and making this nullable? Making it optional will result in different behaviour, where using the 'wrong' constructor will not honour the setting value.

I have change this to IOption. I was not sure what was best to use here. I was looking at other controllers that were using IOptionSnapshot and mimicking what was done in those classes. IOptionSnapshot changed to IOption.

ronaldbarendse · 2023-09-11T07:50:02Z

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

@@ -143,7 +149,8 @@ public class MemberController : ContentControllerBase
              jsonSerializer,
              passwordChanger,
              scopeProvider,
-              StaticServiceProvider.Instance.GetRequiredService<ITwoFactorLoginService>())
+              StaticServiceProvider.Instance.GetRequiredService<ITwoFactorLoginService>(),
+              null)


This should get the options/settings using the StaticServiceProvider.Instance as fallback:

Suggested change

null)

StaticServiceProvider.Instance.GetRequiredService<IOptions<SecuritySettings>>())

Thanks, change made.

ronaldbarendse · 2023-09-11T07:53:49Z

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

+        if(_securitySettings.RequireUniqueEmailForMembers == false)
+        {
+            return true;
+        }


This check should be moved to the ValidatePropertiesData() method, so this ValidateUniqueEmail() keeps doing what you expect it to do (based on the method name) and is only called when you require unique emails.

I see your point. I have moved the conditional check to the ValidatePropertiesData() method. I am not in loving with my change. I did it as a separate conditional statement, but the number of conditional statements in this method is become a lot but the trade off is that the current validEmail variable check would become more complex. i.e. drop my conditional on _securitySettings.MemberRequireUniqueEmail and change if(validEmail == false) { ... to if (_securitySettings.MemberRequireUniqueEmail && ValidateUniqueEmail(model) == false) { ... . Thoughts?

New to committing to Umbraco so not sure if there is a preference for verbose easier to read code or more concise code.

Well, I changes my code to the more concise version. Without major class refactoring I am not sure how to make CodeScene happy.

Need help to know if there are actions I need to take? Builds local, but fails some check on the GitHub pipeline that I need help understanding. Also would like to know if this will be considered for inclusion. Thank you.

…ller unit tests to support the addition of the SecuritySetting object.

jasont0101 · 2023-09-11T17:10:32Z

This was building on Friday, but not today. Looks like the changes I made are causing the Run dotnet pack step to fail since they do not match the baseline. Not sure it this is something I can fix. What do I need to do here, anything?

# Conflicts: # tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs

emmagarland · 2023-12-08T13:51:49Z

Hi @jasont0101

Apologies for the delay. It looked like there was since a conflict introduced, so I have merged contrib into your branch, resolved the MemberControllerUnitTests.cs conflict, and pushed the latest.

Hopefully the builds will now pass but if not, I'll ask HQ to review what we need to do.

Thanks

Emma

emmagarland · 2024-02-01T15:57:56Z

Hi @jasont0101

The build seemed to pass, except CodeScene, but I can't think how to get around that for now without bigger refactoring.

Other than that do you have any other questions, or are you awaiting further review of your changes to this PR from @ronaldbarendse and then we could be ready to merge (presuming all good from HQ)?

Thanks

Emma

jasont0101 · 2024-02-01T16:03:00Z

Emma, I do not think I have other questions. Since this is my first really contribution to the core, I am just learning the process, so thanks for the feedback. If there is nothing I can do, and HQ is cool with the code I am good to go on having it merged. Jason Thomas Lead of Software Development simpli__FYIN since 2016 [cid:83d89d1e-41a1-4977-8e50-d252c5a501b6]

…

________________________________ From: Emma L Garland ***@***.***> Sent: Thursday, February 1, 2024 8:58 AM To: umbraco/Umbraco-CMS ***@***.***> Cc: Jason Thomas ***@***.***>; Mention ***@***.***> Subject: Re: [umbraco/Umbraco-CMS] Allow Duplicate Email for Members (PR #14784) Hi @jasont0101<https://github.com/jasont0101> The build seemed to pass, except CodeScene, but I can't think how to get around that for now without bigger refactoring. Other than that do you have any other questions, or are you awaiting further review of your changes to this PR from @ronaldbarendse<https://github.com/ronaldbarendse> and then we could be ready to merge (presuming all good from HQ)? Thanks Emma — Reply to this email directly, view it on GitHub<#14784 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGMU54LNX26AIFOFJL4U2GDYRO3RDAVCNFSM6AAAAAA4Q7MXD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRRGY2TCNBWGM>. You are receiving this because you were mentioned. Fyin.com is an Umbraco Gold Certified Partner<https://umbraco.com/partners/partnerlist/fyin/> NOTICE: This message and any replies are subject to FYIN LLC.'s<https://Fyin.com?Signature> standard Terms & Conditions available online. All correspondence is tracked for billing purposes in fifteen minute increments. Fyin.com<https://www.Fyin.com?Signature> : 888.466.0101

emmagarland · 2024-02-01T17:08:58Z

That's awesome @jasont0101 and love that this is your first contribution!

Sorry it's taken a little while. I'll check it on my side and run it past HQ tomorrow, and hopefully we can get it merged asap 😃

Emma

ronaldbarendse

@emmagarland I've left some additional comments that need to be fixed first and I'd recommend doing a fair bit of testing on these changes after that as well (as we don't want any regressions in creating/fetching members) 😃

@jasont0101 Awesome work on this PR, especially if it's your first! 🙌🏻 The CodeScene build errors can sometimes be triggered on existing code that already didn't follow the rules/suggestions, so don't be too afraid of this single step not passing 😉

ronaldbarendse · 2024-02-01T22:31:41Z

src/Umbraco.Core/Services/IMemberService.cs

+    /// <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)
+    {
+        return Enumerable.Empty<IMember>();
+    }


Adding default implementations to interfaces to avoid breaking changes is a great solution, but it should still return a usable result or (worst case) an exception indicating the method isn't implemented in the inherited class...

In this case, it could lookup all members and filter in-memory (which is really inefficient, but still return the expected value), only return a single member by using the GetByEmail() method or actually use the paged FindByEmail() method using StringPropertyMatchType.Exact:

Suggested change

/// <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)

{

return Enumerable.Empty<IMember>();

}

/// <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)

=> FindByEmail(email, 0, int.MaxValue, out _, StringPropertyMatchType.Exact); // TODO: Remove default implementation in v14

Although this means there's already an existing method implemented that allows you to get all members by email, it does have the overhead of pagination and ordering by the email column... And having an easy to use GetMembersByEmail() method still adds value, so I'd keep this new method 👍🏻

ronaldbarendse · 2024-02-01T22:35:26Z

src/Umbraco.Core/Services/MemberService.cs

+        {
+            var members = this.GetMembersByEmail(email);
+            return members?.FirstOrDefault();
+        }


This can be simplified to a single line expression:

Suggested change

{

var members = this.GetMembersByEmail(email);

return members?.FirstOrDefault();

}

=> GetMembersByEmail(email).FirstOrDefault();

The CMS codebase also doesn't use the this qualifier and since GetMembersByEmail() doesn't return null, shouldn't require the null-coalescing operator...

ronaldbarendse · 2024-02-01T22:40:13Z

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

+        _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService));
+        _shortStringHelper = shortStringHelper ?? throw new ArgumentNullException(nameof(shortStringHelper));
+        _securitySettings = securitySettings;
+    }

    public MemberSaveModelValidator(


You can remove this constructor, because it's not used and MemberSaveModelValidator is an internal class 😄

If it was a public class however, it should me marked as obsolete and call the new constructor using this(...), so the fields (and null checks) don't have to be duplicated.

ronaldbarendse · 2024-02-01T22:48:01Z

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

@@ -55,6 +56,7 @@ public class MemberController : ContentControllerBase
    private readonly ITwoFactorLoginService _twoFactorLoginService;
    private readonly IShortStringHelper _shortStringHelper;
    private readonly IUmbracoMapper _umbracoMapper;
+    private readonly SecuritySettings? _securitySettings;


I don't see this field being set anywhere, so it ends up always being null and not applying the correct setting: have you (re-)tested creating a member using a duplicate e-mail address?

I'd also recommend using IOptionsMonitor<SecuritySettings>, so any updates to the configuration is always used (since controllers are registered as singletons). Also keep the constructor updates/obsoletion recommendations in mind when fixing this!

connectisl · 2024-03-22T09:04:57Z

@jasont0101 We're in a similar situation to yourself with this issue. Are you planning to implement the amendments requested by @ronaldbarendse?

jasont0101 · 2024-04-26T20:33:18Z

@connectisl Got a bit side tracked, but yes, picking up work on this again.

connectisl · 2024-04-29T16:20:04Z

@connectisl Got a bit side tracked, but yes, picking up work on this again.

That's excellent news, @jasont0101. Without wanting to sound too direct, are you working to a particular timeline to implement the suggested changes? We're just looking at the date for Umbraco to end support for v10 (https://umbraco.com/products/knowledge-center/long-term-support-and-end-of-life/) to try and see which release(s) this pull request/fork could be included in.

We'd be more than happy to help with any testing on this, too. Feel free to drop me an email at seo@connect.org.uk if you need an extra pair of eyes/hands.

jasont0101 · 2024-04-29T16:43:48Z

@connectisl Originally I started this with v12, but it got ignored by Umbraco for a long time so I lost my momentum and let this sit too long. Now the contrib branch is on a RC of v14. I think what I will need to do is abandon this PR and start new. I tried to rebase my fork with v13/contrib branch but it did not going so well (assuming that was even the correct thing to do). I am now trying to targeting v13 first then v14. I need to get the done this week, after that it is up to Umbraco to approve and merge, fairly sure they will have move suggestions. I need this in at least v13 as we have client that needs this but does not want to go to v14. Might even be a hard sell to get them to v13.

jasont0101 · 2024-05-01T22:19:42Z

Abandoning this PR.
New PR submitted: #16202

jasont0101 · 2024-05-01T22:21:44Z

@connectisl Please see #16202

jasont0101 added 5 commits September 8, 2023 13:51

Added RequireUniqueEmailForMembers option. Updated member validator t…

81f1018

…o honor RequireUniqueEmailForMembers.

Fixed issue that member info was being overwritten. Add new GetMember…

2e32baf

…sByEmail method

Added RequireUniqueEmailForMembers option. Updated member validator t…

a02117b

…o honor RequireUniqueEmailForMembers.

Fixed issue that member info was being overwritten. Add new GetMember…

5b5eb6c

…sByEmail method

Merge branch 'temp-RequireUniqueEmailForMember' of https://github.com…

c39e0d6

…/jasont0101/Umbraco-CMS into temp-RequireUniqueEmailForMember

jasont0101 changed the title ~~Temp require unique email for member~~ Allow Duplicate Email for Members Sep 8, 2023

jasont0101 added 3 commits September 8, 2023 17:06

Removed condition statement.

17f7763

Update condition statement and moved it to new spot in the class

b0ced91

Revert conditional changes.

58c1cf3

ronaldbarendse reviewed Sep 11, 2023

View reviewed changes

jasont0101 added 3 commits September 11, 2023 08:28

Made changes from code review request and suggestions.

52702b8

Fixed errors due to renaming of property. As updated the MemberContro…

b138692

…ller unit tests to support the addition of the SecuritySetting object.

Updated contional statement to be more concise.

dc955d6

jasont0101 and others added 5 commits October 3, 2023 08:26

Reverted breaking change in MemberController.

56d4087

Refactoed constructors to have overloads to prevent breaking changes.

5980453

removed overloaded constructor for MemberSaveValidationFilter

6d468c0

Modified GetMembersByEmail to be a default method.

7113f6c

Merge branch 'contrib' into pr/14784

003c164

# Conflicts: # tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs

emmagarland self-assigned this Dec 8, 2023

emmagarland added the community/pr label Dec 8, 2023

ronaldbarendse requested changes Feb 1, 2024

View reviewed changes

Zeegaan mentioned this pull request Feb 5, 2024

Unable to allow duplicate email addresses for member with RequireUniqueEmail = false #15676

Closed

jasont0101 mentioned this pull request May 1, 2024

Review: Allow Duplicate Email for Members #16202

Merged

1 task

jasont0101 closed this May 1, 2024

emmagarland removed their assignment Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Duplicate Email for Members #14784

Allow Duplicate Email for Members #14784

jasont0101 commented Sep 8, 2023

github-actions bot commented Sep 8, 2023 •

edited by emmagarland

Loading

emmagarland commented Sep 11, 2023

ronaldbarendse Sep 11, 2023

jasont0101 Sep 11, 2023

ronaldbarendse Sep 11, 2023

jasont0101 Sep 11, 2023

ronaldbarendse Sep 11, 2023

jasont0101 Sep 11, 2023

jasont0101 Sep 11, 2023

jasont0101 Oct 2, 2023

jasont0101 commented Sep 11, 2023

emmagarland commented Dec 8, 2023

emmagarland commented Feb 1, 2024

jasont0101 commented Feb 1, 2024 via email

emmagarland commented Feb 1, 2024

ronaldbarendse left a comment

ronaldbarendse Feb 1, 2024

ronaldbarendse Feb 1, 2024

ronaldbarendse Feb 1, 2024

ronaldbarendse Feb 1, 2024

connectisl commented Mar 22, 2024

jasont0101 commented Apr 26, 2024

connectisl commented Apr 29, 2024

jasont0101 commented Apr 29, 2024

jasont0101 commented May 1, 2024

jasont0101 commented May 1, 2024

	null)
	StaticServiceProvider.Instance.GetRequiredService<IOptions<SecuritySettings>>())

Allow Duplicate Email for Members #14784

Allow Duplicate Email for Members #14784

Conversation

jasont0101 commented Sep 8, 2023

Prerequisites

Description

TEST

github-actions bot commented Sep 8, 2023 • edited by emmagarland Loading

emmagarland commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasont0101 commented Sep 11, 2023

emmagarland commented Dec 8, 2023

emmagarland commented Feb 1, 2024

jasont0101 commented Feb 1, 2024 via email

emmagarland commented Feb 1, 2024

ronaldbarendse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connectisl commented Mar 22, 2024

jasont0101 commented Apr 26, 2024

connectisl commented Apr 29, 2024

jasont0101 commented Apr 29, 2024

jasont0101 commented May 1, 2024

jasont0101 commented May 1, 2024

github-actions bot commented Sep 8, 2023 •

edited by emmagarland

Loading