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

Duplicate emails are allowed #3720

Closed
rallek opened this issue Jul 28, 2017 · 16 comments · Fixed by #4271
Closed

Duplicate emails are allowed #3720

rallek opened this issue Jul 28, 2017 · 16 comments · Fixed by #4271

Comments

@rallek
Copy link
Contributor

rallek commented Jul 28, 2017

Q A
Zikula Version 1.5.0 ci 248
PHP Version 5.6

Expected behavior

If an email address is allready registered another usershould not be able to use the same one. This is also valid even if the user is registering with different authentication methods

Actual behavior

a user (me) is registered with an email via ZAuth. With the same email address I am registered at facebook. If I now register via facebook/oauth it is possible without any problem. I looked into the database and can see both users do have the same email address

Steps to reproduce

see actual behavior

Is it a bug or is it a feature?

@Guite Guite added this to the 1.5.0 milestone Jul 28, 2017
@craigh craigh self-assigned this Jul 28, 2017
@craigh craigh removed the Blocker label Jul 28, 2017
@craigh
Copy link
Member

craigh commented Jul 28, 2017

this is a broader issue.

When I refactored the users module, I tried to duplicate existing functionality to maintain BC

In the past, unique emails were not required if the login method was set to login as username.

This is still the case (you can set the method to uname and login with two unique unames but same email).

so, this ticket actually is not about ZAuth vs. OAuth but rather that the Users module allows duplicate emails in some situations. This was done by design.

I'm not sure this is something that can be changed for 1.5 because in order to maintain BC, I think duplicate emails must continue to be allowed.

I guess I am not certain how to proceed with this.

@craigh
Copy link
Member

craigh commented Jul 29, 2017

refs #3256

@rallek
Copy link
Contributor Author

rallek commented Jul 29, 2017

I would like to see a behavior to deactivate the new account after registration and show up with a message.
e.g.: It seams you have been already registered. The email address is already in use. The account has not been activated. Please contact the administrator
Similar text in the wellcome email.

The admin can descide how to proceed. One way could be to delete the fresh account. The other way would be to give the old account a new mail address and activate the new one. But that is than done not with a workflow, it is done by hand.

Would this work around be possible?

@craigh
Copy link
Member

craigh commented Jul 29, 2017

anything is possible given enough time.

possible in 2.0? no.

@Guite do you have any thoughts on this?

@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@zikula zikula deleted a comment from rallek Jul 29, 2017
@craigh craigh changed the title new user registration with the same email Duplicate emails are allowed Jul 29, 2017
@craigh
Copy link
Member

craigh commented Jul 29, 2017

@Guite - I think this can be moved to 2.1 or 3.0. thoughts?

@craigh craigh modified the milestones: 2.1.0, 1.5.0 Jul 29, 2017
@craigh
Copy link
Member

craigh commented Jul 29, 2017

after discussion with @Guite, postponed to 2.1.0 (or later)

@Guite
Copy link
Member

Guite commented Aug 8, 2017

@Guite Guite removed this from the 2.1.0 milestone Nov 2, 2018
@Guite Guite added this to the 3.0.0 milestone Nov 2, 2018
@gfr
Copy link
Contributor

gfr commented Jan 6, 2020

I have a zikula installation where duplicate emails are required for this use case and therefore I vote to not change this behaviour or at least introduce a config var for that.

@Guite
Copy link
Member

Guite commented Feb 24, 2020

Possibly related to 2915#4197:

Add ability to 'pair' (or merge) two or more accounts so that authenticating via e.g. Github can point to the same ZikulaUser as authenticating via email or any other method.

@rallek
Copy link
Contributor Author

rallek commented Mar 10, 2020

If duplicate emails are used as the same user I have also no problem with it. My issue: If a user is logged in regulary and at another day via facebook Zikula is not using both as the same user.

@Guite Guite added the Bug label Mar 10, 2020
@Guite Guite modified the milestones: 3.0.0, 3.1.0 Mar 31, 2020
@craigh craigh modified the milestones: 3.1.0, 3.0.0 Apr 11, 2020
@craigh craigh added the Blocker label Apr 11, 2020
@craigh
Copy link
Member

craigh commented Apr 11, 2020

This issue needs to be solved at 3.0.0 or 4.0.0 not between

@craigh
Copy link
Member

craigh commented Apr 21, 2020

This is a copy of the conversation from slack, moved here so that it does not get forgotten or lost on slack. @Guite @gfr @rallek if you wish your comments removed please let me know and I will remove them. thank you

Craig Heydenburg:coffee: 8:24 AM
does anyone have opinions about #3720
basically, the question is, should one email address === one account
current behavior is that multiple accounts can share the same email address.
this is actually necessary because in OAuth, a user may login with Github (for ex) which has the same email address as another ZAuth account.
so, if it is decided that one email address === one account, then it must also be possible to merge accounts that have the same email address when they register - as explained in #4197
(keep straight in your head: your account is different than your authentication (or how you login to your account))

Gabriel Freinbichler 9:42 AM
@craigh as already stated in the ticket it is important for me that several accounts for one email-address can exist. in this setup one admin with a central email-address manages different users (accounts) with different user names. Login via email is disabled of course (edited)

Craig Heydenburg:coffee: 9:50 AM
@gfr could you please tell me more about your use-case? why do several users need the same email account? Is this a teacher/student situation or something?

Gabriel Freinbichler 9:53 AM
@craigh this setup is related to firebrigades. the usually have one official email address from the municipality (eg. [email protected]) and the use this address for every offical communiaction and also accounts. therefore the local administrator registers different users (eg. firestation_truck1, firestation_truck2, ..) with the same address so that he has control over this users and no additional fake-mail accounts are required. at the moment i have over 1200 emails with more than one account

Craig Heydenburg:coffee: 9:55 AM
at the moment i have over 1200 emails with more than one account
Did you say this backward? Maybe you have 1200 accounts with same email?

Gabriel Freinbichler 9:56 AM
1200 email addresses have more than one account assigned. peak is 19 accounts for one email - just checked the db (edited)

Craig Heydenburg:coffee: 9:57 AM
do these “trucks” login to their account?

Gabriel Freinbichler 9:57 AM
yes the login to their account

Craig Heydenburg:coffee: 9:58 AM
thanks for the info. I’ll think more on your usecase

Ralf Köster 10:26 AM
I do have another situation. My church site is used by not very IT skilled users. Some do register first by email and some weeks later they login via Facebook. With this they created a second account. They claim they are not able to change their own content anymore and do not recognize that they do not login into the same account. That is my situation. (edited)

Gabriel Freinbichler 10:29 AM
maybe a solution could be a warning when a user registers via fb and the account already exists via email?

Ralf Köster 10:29 AM
If a warning is possible the login should be possible as well

Gabriel Freinbichler 10:30 AM
e.g.: warning: there already exists an user with the associated email address. do you really want to create another account via facebook or do you want to login to the user?

Axel Guckelsberger 10:32 AM
whereby the login would only be possible if there are not multiple users with this email address

Gabriel Freinbichler 10:32 AM
a direct login to this user without asking for password should not be possible as this could support account hijacking with a stolen facebook identity

Ralf Köster 10:34 AM
Is the email transferred while login via Facebook, Google, GitHub and so on?

Gabriel Freinbichler 10:35 AM
maybe an interface for account merging could be available if following conditions are met:
only one user exists for a given email
someone tries to login via oauth with this email

Ralf Köster 10:35 AM
With other words if the user can be identified the same with different authentication types we should not have any issue

Gabriel Freinbichler 10:35 AM
if the user wants to merge he has to enter the password an zk then knows this user is assigned to this email and also to this oauth identity

Ralf Köster 10:37 AM
If you have to remember to your password the benefit has gone I guess... Opposite will happen. Some will type the password of Facebook instead of Zikula.

Gabriel Freinbichler 10:38 AM
but if it is not required to enter a password account hijacking would be possible and this would be a security issue

Ralf Köster 10:39 AM
What is the standard in the web on other sites? I want to be able to follow that standard.

Gabriel Freinbichler 10:40 AM
Imagine you have a company portal where users are registered with their company mail address and where you share sensitive data. one of your employees ignores the security guidelines and creates a facebook profile with company email and an insecure password. one day an attacker discovery the facebook password of this employee and with the account merging interface he can login to your company portal without knowing the secure password - he only needs the fb-password
10:42
this issue is even bigger if the employee has admin permissions. then a complete takeover is possible and the attacker can read any information as he can change every password of every user registered in the system (edited)

Ralf Köster 10:44 AM
Hey, not all sites are high security 🙂 . Some are hobby and make fun. There no hacker can find anything important. There I want to be ABLE to use it like thousands other sites. We are talking about a generic system not about only company sites ;-)

Axel Guckelsberger 10:45 AM
a company website should not use external authentication methods

Gabriel Freinbichler 10:45 AM
i understand your concern but if this feature is available in the core the possible risk exists on every system

Ralf Köster 10:45 AM
Correct

Axel Guckelsberger 10:46 AM
I tend to agree with that we should not mix multiple authentication methods for the same account
if an account was registered using ZAuth it shouldn't accept Facebook logins
if an account was registered using Facebook it shouldn't accept ZAuth logins
if an account was registered using GitHub it shouldn't accept ZAuth nor Facebook logins
if another / a second registration is started for the same email address it could be prevented - or at least it needed to be confirmed explicitly (edited)

Ralf Köster 10:48 AM
That prevention is my demand (edited)

Gabriel Freinbichler 10:48 AM
i think shouldn't accept could be extended to shouldn't accept and inform the user about the existence of another account
maybe this could be done via veto-event?

Craig Heydenburg:coffee: 7:23 PM
@Guite is essentially saying we can’t allow duplicate email addresses in the user table.
@gfr says earlier he requires duplicate email addresses in the user table.
@rallek wants ease of use.
One solution is to only allow one type of authentication on the site. Make many available to the admin - they selecte ONLY ZAuth, Facebook, whatever.
It is very hard to meet all these disparate requests.

Axel Guckelsberger 1:32 AM
Here is a possible proposal:
We need two settings:
- Registration enabled yes/no (already exists)
- Duplicate email addresses allowed yes/no (existed earlier, but currently not)
When a user tries to login using authentication method X and another user with same email address (but created by another auth method Y) already exists:
- If registration is disabled: only display an error message (like "Another account for your email address already exists. Please login using these credentials (hint: Facebook)"). Instead of Facebook the hint could also read "user name" for ZAuth for example.
- If registration is enabled, but duplicate email addresses are not allowed: same behaviour.
- If registration is enabled, but duplicate email addresses are allowed: first login with method X causes registering another account (current behaviour).
(edited)

Simple PollAPP 2:11 AM
What do you think?
1️⃣ Nice 4
@rallek, @gfr, @paustian, @robbrandt
2️⃣ I am missing something
3️⃣ Not adequate
4️⃣ Unsure
5️⃣ Don't mind

Craig Heydenburg:coffee: 8:50 AM
Duplicate email addresses allowed yes/no (existed earlier, but currently not)
Fyi - this was replaced by the ability to select one of three choices: auth by uname, auth by email, auth by either.
if auth by email or either is selected, it enforces a no duplicate email policy (for ZAuth only)
so the functionality is still there. it is only implemented differently.
I’m not sure about the proposal yet. I must think it over.

Craig Heydenburg:coffee: 9:52 AM
above you have a scenario like:
if user tries to login…
… comments about registration
login & registration are different
registration should do nothing with regard to login. an attempt to login should be dependent on previous registration
I’m assuming this was a mistake in your scenario?

Axel Guckelsberger 10:15 AM
this only referred to how OAuth providers work; I thought there was some kind of auto registration as part of first login; sorry if this was confusing, wasn't on purpose.

Craig Heydenburg:coffee: 10:23 AM
auto-registration is a feature request that is unimplemented (edited)
was here - I cannot recall exactly, but I think you and I (or others) discussed and rejected the idea
so, now knowing that is not available, how does this change your proposal?

Axel Guckelsberger 10:29 AM
remove "first login with method X causes registering another account" ?

@craigh
Copy link
Member

craigh commented Apr 21, 2020

some thoughts on error messages on failed logins:

https://security.stackexchange.com/questions/158075/is-it-unsafe-to-show-message-that-username-account-does-not-exist-at-login

https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Authentication_Cheat_Sheet.md#authentication-and-error-messages

basically - the accepted practice is to make failed login error messages as generic as possible, but this of course comes at the cost of reduced usability. Zikula currently errs on the side of security with a simple message of:

Login Failed

The obvious problem with the security argument is the registration page problem as brought up in the linked discussions. Zikula currently has the following notice:

The user name you entered (foobar) has already been registered.

This makes is clear that 'foobar' is a valid username and the hacker can then begin their brute force attack on the password. So - in Zikula's case, this is less secure in the interest of usability.

This doesn't answer the main question in OP, but some other questions about login messages were raised, so I wanted to put my thoughts here.

@craigh
Copy link
Member

craigh commented Apr 22, 2020

My thinking is that we should disallow registration using an email address that is in use by another authentication method and that should solve this. This allows ZAuth to continue to allow duplicate emails per multiple account if Native UNAME is the selected Auth method. It also allows the admin to select Native EMAIL Auth method and then this prevents duplicate emails for ZAuth logins.

#4197 would - if desired - need to bypass all validation of duplicate emails and implement some kind of 'secondary authorization' method. To do this, the currently logged in user would proceed through a workflow where they authenticate by their new method (e.g. Facebook) and this creates a new mapping for that method and is linked to the current UID. Then the user could login with either method after that. (#4197 could therefore be postponed to a later release as it would not be incumbent on this issue).

opinions? @Guite @gfr @rallek

@Guite
Copy link
Member

Guite commented Apr 22, 2020

I agree with you entirely 👍

craigh added a commit that referenced this issue Apr 22, 2020
…4271)

* restrict ability to register same email with different auth methods. closes #3720. refs #4197
craigh added a commit that referenced this issue May 3, 2020
…4271)

* restrict ability to register same email with different auth methods. closes #3720. refs #4197
@Kaik
Copy link
Contributor

Kaik commented Aug 18, 2020

some thoughts on error messages on failed logins:

https://security.stackexchange.com/questions/158075/is-it-unsafe-to-show-message-that-username-account-does-not-exist-at-login

https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Authentication_Cheat_Sheet.md#authentication-and-error-messages

basically - the accepted practice is to make failed login error messages as generic as possible, but this of course comes at the cost of reduced usability. Zikula currently errs on the side of security with a simple message of:

Login Failed

The obvious problem with the security argument is the registration page problem as brought up in the linked discussions. Zikula currently has the following notice:

The user name you entered (foobar) has already been registered.

This makes is clear that 'foobar' is a valid username and the hacker can then begin their brute force attack on the password. So - in Zikula's case, this is less secure in the interest of usability.

This doesn't answer the main question in OP, but some other questions about login messages were raised, so I wanted to put my thoughts here.

This is a security issue only in special cases when the system has to be really secured. This security level implies that user is not able to choose/edit his username or user is known before registration and registration is manual. There is no way for hacker to not know that an account/username exists when registration is open... even error Login/Registration failed gives, a clue about username existence not to mention that usernames are publically known in news comments etc... In most of cases Zikula is used as an open system (I guess)(when users can register). We could provide an option to harden security "level" and remove information that username/email is registered or not but that is something extra, not default and for consistency it requires that users cannot chose username by themselves. They can provide an email address -> and system will send them an email with information to activate account or that someone tried to use their email address to register an account in case email already exists. User that is providing email is notified only that the email with instruction was sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants