Fix unique email address handling #27652
Conversation
Because email addresses must be unique so there can be only a single account entry for each email address.
|
openLDAP doesn't let me set multiple duplicate email addresses, which is good. So couldn't test. As for having a duplicate email, one in DB and then one in LDAP, this fix is not testable because LDAP already tries to write the email earlier, bug raised here owncloud/user_ldap#72 |
When trying to insert/update a user with a duplicate email address, change that one to empty to avoid failure or loss of access.
| return $this->findEntities($qb->getSQL(), $qb->getParameters()); | ||
| $results = $this->findEntities($qb->getSQL(), $qb->getParameters()); | ||
| if (empty($results)) { | ||
| return null; |
There was a problem hiding this comment.
I'm not convinced about these changes: if the plan is to support duplicated emails at some point, I think it's better to keep this as it is and handle the duplication at a higher level. As long as the callers are aware that several accounts can be returned by the same email, it should be fine.
There was a problem hiding this comment.
if the plan is to support duplicated emails at some point
It's not, see #27626 (comment).
There was a problem hiding this comment.
@DeepDiver1975 your feedback ? This will tell whether to revert the above and use the below appraoch instead.
| throw $ex; | ||
| } | ||
| // find out if there is another account with the same email address | ||
| $existingAccount = $this->mapper->getByEmail($email); |
There was a problem hiding this comment.
I'd fetch the accounts, check the number of accounts, and if it's only one then verify it's the same account or another account than the one we're trying to insert.
There was a problem hiding this comment.
Hmmm... that could be another approach, yes.
| } | ||
|
|
||
| // it's a duplicate email, so clear the field and try again | ||
| $a->setEmail(null); |
There was a problem hiding this comment.
❗️ Note that it will set an empty email instead of null in the DB, which will likely cause trouble. I'm affraid this could lead to an infinite loop if there are 3 or more users with the same email.
There was a problem hiding this comment.
Okay, I'll replace this to an empty string. Would that prevent the infinite loop ?
When testing this worked for me with null, but better be safe.
There was a problem hiding this comment.
I don't think so. You'll be setting an empty email for the first duplicate; for the next duplicate you'll set another empty email causing another duplication exception because you've already set an empty email for another user.
On a second thought, it might not cause an infinite loop, but I don't think that will be handled properly. There is no progress other than "skip one duplicated email"
|
Obsoleted by #27662 (comment) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Fixes upgrade and user:sync issues when one user backend provides multiple users with the same email address, or multiple backends provide different users with the same email address.
Related Issue
Fixes critical part of #27626
Motivation and Context
See ticket
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
TODOs