Can't set the same email to two users

[PVince81]

Steps:

1. Create two users "user1" and "user2"
2. Set the same email address to both users as admin

Expected result

Possibly to set to two users

Actual result

Setting it to the second user as admin shows a spinner and never stops.
Also: if setting it through the user's personal page, when opening the link to validate the email address there will be an "Internal Server Error" page.

Additional potential issues (to be tested)

• if LDAP has two users where both have the same email address (ex: an admin who has two accounts, one as admin, one as regular user but still wishes to receive email to the same address), it is likely to break on sync.
• if a previous setup already had users with the same email address, migrating to 10.0 will likely break somewhere. (I couldn't test with local users due to this other bug Email address not imported to oc_accounts from oc_preferences on upgrade #27625)

I suggest making the email column non-unique.
@DeepDiver1975 @butonic @jvillafanez

cc @pmaier1

[pmaier1]

Just discovered this myself when using the guest app and trying to add a user with an address that I specified for another user before. Anyway for the guest app it should share with the user that specified the respective address instead of creating a new user. Will provide this as feedback for guests separately.

[PVince81]

@DeepDiver1975 now that #27635 is merged it is possible to break the update this way:

1. Setup stable9.1
2. Create two users with the same email address
3. Upgrade to master

± % ./occ upgrade
ownCloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Updating database schema
Doctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'INSERT INTO `oc_accounts`(`lower_user_id`,`user_id`,`state`,`backend`,`home`,`display_name`,`email`) VALUES(?,?,?,?,?,?,?)' with params ["user2", "user2", 1, "OC\\User\\Database", "\/srv\/www\/htdocs\/owncloud\/data\/user2", "user2", "vincent@petry"]:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'vincent@petry' for key 'UNIQ_907AA303E7927C74'
Update failed
Maintenance mode is kept active
Reset log level

[felixboehm]

you can login with the email field, so I would expect that we prevent adding an email that is already used. What is the solution in mind?
@pmaier1 @hodyroff

[PVince81]

you can login with the email field

This only works if a single user has an email address set.

If two users have the same email address then the login is denied. This was the logic in 9.1.

If we do want to keep blocking duplicate emails, then we need to think about how to migrate setups that had duplicate emails in the past.

And here I'm only talking about DB users.

LDAP might also have entries where multiple users have the same email address, but login with email feature only applies to DB users. For LDAP users, the admin can configure the login fields in the LDAP wizard. Now I don't know what happens with LDAP if two users has the same email address and email is set as login field... @jvillafanez @davitol can you try this out ?

[PVince81]

Talked with @DeepDiver1975 and this is the way we'll go:

• duplicate email addresses are forbidden (previous versions did allow that)
• when upgrading, only pick the first email address found (first found user, not necessarily alphabetical). second user who has the email address will get an empty field, no failure
• when importing users from LDAP or other user backends (occ user:sync), write an empty email address in case it is duplicate with another one
• log a warning when a duplicate was found for admins to track the problem

[PVince81]

This also means that if a user tries to manually set an email address to an existing one, we'll show a warning "email address already in use in the system, please use another one". This could make it possible for a user to guess what users has an account on the system. Probably not a real security risk, @Peter-Prochaska please confirm.

If we consider this a potential risk, then we need to not show an error message but keep the regular "an email has been sent for confirmation" but not actually send anything ?

[PVince81]

And if an admin manually sets a duplicate email address there will directly get an error in the UI.

• set email address method must throw exception in case of duplicate
• catch exception and set email to empty again, log a warning
• when setting duplicate email as regular user, lie and say an email was sent when none was sent (see Can't set the same email to two users #27626 (comment)), and log a warning message for the admin
• when setting duplicate email as admin, show error message

[PVince81]

• provisioning API must return an error code in case of duplicates, don't set an empty email

[PVince81]

Estimate: at least 1 md as it seems to not be as easy as it looks.
Multiple code paths to cover, some with one behavior the others with another.

The tricky part: how to detect that the "Constraint Violation Exception" is actually from the email field and not another one.

[PVince81]

• make sure to properly handle cross-backend email duplication 😱 💣

[PVince81]

• change AccountMapper::getByEmail() to only return a single account instead of array

[PVince81]

I think I'll do a two-part fix because else this will take too long:

1. Fix import / sync to avoid breaking update and user:sync
2. Fix all the other API cases to return proper errors instead of 500

First to be fixed as critical for 10.0 release.
Second looks bad but not critical.

[PVince81]

PR here for 1: #27652

[felixboehm]

If it is more simple for now, we can have unique emails for guest users only.
And other users can keep multiple emails.
For LDAP we can't enforce it anyway.

[cdamken]

IMO:
Attributes like cn, name, distinguishedName, sAMAccountName and objectGUID normally are unique.

The same Email can be added for many purposes: Test accounts, unattended devices that should download information.

Using #27626 (comment) will generate a lot of problems with the users -> New Tickets how to restore the previous configurations

Also not all oC Admins have right to modify in the LDAP server to add/modify emails

[PVince81]

@felixboehm while this might be a good short solution, I don't think it's a good idea to change the behavior in a later minor release to avoid bad surprises. The migration to non-duplicates would have to be 10.1 then, a repair step will need to be implemented that scans all users to find duplicates and make a decision which to keep and which not.

Also @DeepDiver1975 insisted to prevent duplicates but @cdamken was surprised about the decision.

So what should we do now ?

On a side note I've discovered even more issues with duplicate email because the LDAP code isn't fully properly integrated with the account table: owncloud/user_ldap#72.
That piece of code is in LDAP and duplicate emails would fail there before even reaching the account table sync code.

I suspect there is more to be discovered.

From reading the code it seemed that there was no proper decision made because some parts of the account table code still allowed for multiple addresses while the DB structure didn't.

Personally I don't want to invest too much time right now, especially that it feels that this decision feels rushed and not fully thought through. If #27652 cannot work by the end of the day today I'd like to abandon this for 10.0 and adjust the DB constraints to allow duplicates.

[PVince81]

This PR would remove the unique constraint: #27662.

[DeepDiver1975]

but @cdamken was surprised about the decision.

uniquness of email is part of the account table concept for several months - sorry but time for changing this is up.

[cdamken]

uniquness of email is part of the account table concept for several months - sorry but time for changing this is up.

I wasn't consulted about this, I suppose you have a lot good reasons for use email as unique. Please share them asap, that way I can communicate to the customers when the tickets arrives.

[PVince81]

Regarding LDAP, if a setup has duplicate email addresses in LDAP, my fix for core cannot do anything about it. Need specific LDAP fixing.

Either integrate the LDAP app correctly as per owncloud/user_ldap#72

Or add another extra check for email duplicates inside the LDAP app as a quick fix. The quick fix would also need to set the email address to null when meeting a ConstraintViolationException. @jvillafanez

[PVince81]

downgrading to medium. To be looked into eventually...

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

[PVince81]

Reopening, unless @DeepDiver1975 confirms that we don't want to change this behavior.

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

[pmaier1]

At least endless spinner and internal server error issues (mentioned in OP) need to be fixed. For many users the mail address is the only unique identifier for e.g. receiving shares or for preventing duplicate accounts. Therefore I don't think we are going to change the basic design on this unless there are other important arguments.

[jvillafanez]

It should be possible to set the uid / login to an email-like string. This should provide something similar to a login by email although it's in fact a regular login. Note that in this case, that uid won't be changeable.

Another more complex approach could be that, from a list of emails that a user could have, mark one of those as "possible to log in with". In this case, we have to verify if such email is unique before allowing it. Then we can use that email to login with that user.

During user sync, any (valid?) email can be added into the DB, but none of them will be set as "possible to log in with".
While this should solve the problems regarding login with email, there could be other problems with the email usage. If I want to send an email, what email address should I use? what if several users have the same email?

In any case, I don't think this will be an easy change to do, so we have to check if this is worthy, and the scenarios and setups where this can be useful.

[stale]

This issue has been automatically closed.