Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review: Allow Duplicate Email for Members #16202

Conversation

jasont0101
Copy link
Contributor

@jasont0101 jasont0101 commented May 1, 2024

fixes #15676

Prerequisites

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

Description

This is a redo of this PR #14784. That PR went stale and the Contrib branch advanced to v14, so this is work for v13.

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

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

This item has been added to our backlog AB#47446

Copy link

github-actions bot commented May 1, 2024

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 🤖 🙂

@georgebid
Copy link
Contributor

Hey @jasont0101, thanks a lot for creating a second PR for this one, sorry it's taken so long to get it reviewed. Have you addressed all of @ronaldbarendse's comments on this PR? I just want to make sure we don't lose those conversations 😄 Someone on the core collabs team will review this soon!

@jasont0101
Copy link
Contributor Author

@georgebid & @ronaldbarendse Yes, I incorporated Ronald's input from the prior PR (#14784).

@connectisl
Copy link

connectisl commented May 16, 2024

@jasont0101 I'm hoping you could answer a question that may help to unblock this issue for us. In your opening message for this pull request, you said: "While there are other way to resolve this issue...with out having to work out complicated alternative contact email options."

Did your alternative approach involve appending +1 to a member's email address or similar? I'm really keen to understand the other ways you've managed to overcome this issue. We're unlikely to be in a position to upgrade to v13 in the immediate future, so any information you can share would be useful. Feel free to drop me an email at seo@connect.org.uk to discuss.

@jasont0101
Copy link
Contributor Author

@connectisl Sorry for the very slow reply. The only thing I can think of for handling this without being able to allow members to have duplicate emails is to add a new property to the member for a Contact Email, then make the email in the email field unique in some way. The email field would just be ignored in your code in favor of the new Contact Email field. This could be a fair bit of refactor work depending on your code.

One of our clients did something similar as they are also reluctant to upgrade to v13 as well. There upgrade to v10 from v8 was very painful. Having done a few v10 to v13 upgrades now I have found that they are fairly painless. The change over from .Net Framework to .Net Core as well as some of Umbraco's updates was the worst part. I am hoping v14 will turn out to not be that bad.

Being fearful of upgrades is a sign that there is an architectural issue with their project. Unfortunately, this is not an isolated issue and some of our clients whos sites, which we inherited, have gotten so complicated that they are fearful of upgrades. Over the years with Umbraco I have learned that upgrades are just part of the Umbraco life cycle and need to factored in to my designs. I will get off of my soap box now. :-)

@TimPickin
Copy link

TimPickin commented Sep 2, 2024

I have had the same problem, migrating an Umbraco 7 project to Umbraco 13. I was able to create more than one member with the same email address using the MemberService, rather than doing it in the backoffice. However I had a problem that I could not set a password for these duplicate members. I think that this is something to do with the Microsoft dotnet identity logic. I think that this logic still uses the email address when handling the password hash. Please make sure that the password setting logic is also addressed in this pull request.

@jasont0101
Copy link
Contributor Author

@TimPickin Unfortunately, I don't think Umbraco is going to get this merged in anytime soon, if ever. I was at Code Garden and talked to them about this an they are just too busy for this one. I believe they are focused on v14 and since I also have been busy and not able to make this update in v14, there is just no desire to merge this in, in my belief. I have updated this a few times and it goes stale because of their back log and priorities. I also believe they do not understand the problem. I suspect from their point of view, why would you want duplicate emails. There is an obvious push to have the email be the user's login name so duplicate email should not be allowed. If you want a secondary email, they might say, just added a new email property field to the member and customize the frontend login to use that property. If this is their position, I am just guessing, I can see their point and why they might not want to make this a priority. If they ignore it long enough, it will go away because everyone will be forced to move to a work around.

@ChristopherBass
Copy link

ChristopherBass commented Nov 8, 2024

I have the same problem in developing an Umbraco 13 site, in my case it is because email addresses are considered PII that are not allowed by company policy to be contained in the Umbraco website's database. For logins I'll be creating a unique ID for each Member and using that as the UserName, and logging them in via that - but the site is not expected to email these Members through Umbraco, the use of Umbraco Membership is only for managing role-based Access Permissions.

My scenario would also be fulfilled if emails were simply an optional property of a Member.

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Dec 20, 2024
@azure-devops-sync azure-devops-sync bot changed the title Allow Duplicate Email for Members Review: Allow Duplicate Email for Members Dec 20, 2024
Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @jasont0101, I appreciate your input on this over several months.

I've had a look at it today and all seems to work as expected. I made a few minor updates and tested the following scenarios:

  • Creating and updating members via the back-office.
  • Registering members through a front-end implementation.
  • Logging in as those members.
  • Login via the delivery API authorization.

All looks to be working as expected with the different values of the new MemberRequireUniqueEmail setting.

@AndyButland AndyButland merged commit 095a731 into umbraco:v13/contrib Feb 5, 2025
18 of 19 checks passed
@AndyButland AndyButland added community/pr release/13.7.0 and removed state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks labels Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants