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.

Sign up for GitHub

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

Jump to bottom

Fix unique email address handling #27652

Closed
wants to merge 2 commits into from
Closed

Fix unique email address handling #27652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29ab0b0
AccountMapper only return a single account
Apr 18, 2017
b9441a0
Prevent duplicate email address when syncing users
Apr 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/private/User/AccountMapper.php
View file
Open in desktop
Expand Up @@ -34,16 +34,22 @@ public function __construct(IDBConnection $db) {
}

/**
* @param string $email
* @return Account[]
* Return account matching the given email address
*
* @param string $email email address
* @return Account|null account matching the address or null
*/
public function getByEmail($email) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->getTableName())
->where($qb->expr()->eq('email', $qb->createNamedParameter($email)));

return $this->findEntities($qb->getSQL(), $qb->getParameters());
$results = $this->findEntities($qb->getSQL(), $qb->getParameters());
if (empty($results)) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the plan is to support duplicated emails at some point

It's not, see #27626 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeepDiver1975 your feedback ? This will tell whether to revert the above and use the below appraoch instead.

}
return $results[0];
}

/**
Expand Down
9 changes: 5 additions & 4 deletions lib/private/User/Manager.php
View file
Open in desktop
Expand Up @@ -367,10 +367,11 @@ public function callForSeenUsers (\Closure $callback) {
* @since 9.1.0
*/
public function getByEmail($email) {
$accounts = $this->accountMapper->getByEmail($email);
return array_map(function(Account $account) {
return $this->getUserObject($account);
}, $accounts);
$account = $this->accountMapper->getByEmail($email);
if ($account === null) {
return null;
}
return [$this->getUserObject($account)];
}

/**
Expand Down
36 changes: 32 additions & 4 deletions lib/private/User/SyncService.php
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
use OCP\IConfig;
use OCP\ILogger;
use OCP\UserInterface;
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;

/**
* Class SyncService
Expand Down Expand Up @@ -97,19 +98,46 @@ public function run(\Closure $callback) {

// update existing and insert new users
foreach ($users as $uid) {
$isNew = false;
try {
$a = $this->mapper->getByUid($uid);
if ($a->getBackend() !== $this->backendClass) {
$this->logger->debug("User <$uid> already provided by another backend({$a->getBackend()} != {$this->backendClass})");
continue;
}
$a = $this->setupAccount($a, $uid);
$this->mapper->update($a);
} catch(DoesNotExistException $ex) {
} catch (DoesNotExistException $ex) {
$isNew = true;
$a = $this->createNewAccount($uid);
$this->setupAccount($a, $uid);
$this->mapper->insert($a);
}

do {
$retry = false;
try {
if ($isNew) {
$this->mapper->insert($a);
} else {
$this->mapper->update($a);
}
} catch (UniqueConstraintViolationException $ex) {
// could be due to a duplicate email
$email = $a->getEmail();
if ($email === null || $email === '') {
// it's a different issue, rethrow
throw $ex;
}
// find out if there is another account with the same email address
$existingAccount = $this->mapper->getByEmail($email);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... that could be another approach, yes.

if (!$existingAccount) {
// it's a different issue, rethrow
throw $ex;
}

// it's a duplicate email, so clear the field and try again
$a->setEmail(null);
Copy link
Member

Choose a reason for hiding this comment

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

❗️ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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"

$retry = true;
}
} while ($retry);
// clean the user's preferences
$this->cleanPreferences($uid);

Expand Down