AccountMapper get by email is now case insensitive

[PVince81]

Description

AccountMapper::getByEmail() is now case insensitive.
This is important to be able to detect duplicates through this method.

Related Issue

Required for owncloud/guests#168

Motivation and Context

How Has This Been Tested?

Manual testing guests app + unit tests

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[mrow4a]

To me, email should be stored lower case in the database to ensure uniqueness. Is there any guard in the code which prevents insertion of many email versions?

[PVince81]

@mrow4a we currently don't have a constraint in the database as it might happen that more than one user have the same email address depending on which backend they come from (guests, LDAP, etc). This is a topic still to be discussed and solved. Currently we don't want to block all duplicates automatically, only the creation of guests with email addresses that already exist in the system.

Looking at the Account and AccountMapper code, it seems that we store the email address as is, not lower-cased...

[mrow4a]

So when user searches for user by email, user might see many entries to select, pointing to many different users ? How user will know which one to share with?

[PVince81]

@mrow4a not really. The case where multiple user backends are providing the same email addresses is a corner case that we need to look into.

For the regular case there will only be a single email address per user.

The problem we have currently is only with the guests app which makes it possible for any user to invite by email and create an account, which becomes problematic when dealing with duplicates. The guests PR uses the method AccountMapper->getByEmail() to find if there is already an account to decide whether to allow creation or not.

Regardless of your concerns (which are also mine), I think this PR is a step in the right direction.

[mrow4a]

I think this makes no sense... if you are inviting person via email, and that person already exists in the system, you should get error that this person already exists in the system. Email is kind'a unique identifier of the person in real world.

And system should be designed to handle corner cases, and not hope it will be fixed later, I think

[mrow4a]

I understand, that the idea is to allow people have multiple user accounts, with the same email addresses?

If yes, 👍

[PVince81]

I understand, that the idea is to allow people have multiple user accounts, with the same email addresses?

No, we do not want to allow that. But currently due to the architecture it could happen for exotic setups. So we cannot add a constraint at this point without thinking about a migration for said exotic setups. This is why we removed the DB constraint to avoid breaking older setups. We'll likely add it back in the future.

[PVince81]

See #27626 for some context

[codecov]

Codecov Report

Merging #29332 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #29332      +/-   ##
============================================
+ Coverage      60.2%   60.21%   +0.01%     
  Complexity    17182    17182              
============================================
  Files          1030     1030              
  Lines         57271    57271              
============================================
+ Hits          34479    34485       +6     
+ Misses        22792    22786       -6

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/AccountMapper.php	66.32% <100%> (+7.14%)	25 <0> (ø)	⬇️
lib/private/Files/Cache/Propagator.php	97.4% <0%> (-1.3%)	16% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddba9b4...7706c01. Read the comment docs.

[DeepDiver1975]

we shall not mix the lower functions - both lower operations shall be handled by sql

[butonic]

Even for LDAP there is a prmary email address. But ldap can be configured to use additional login attributes. And for them something like additionalEmail can be multivalue. No need to store them in the oc db unless you want to be able to search by these emails, which is what the additional search terms are for.

[PVince81]

@DeepDiver1975 fixed, please recheck. I hope the format is ok (let's see what Oracle says...).

@butonic was this your response to @mrow4a ? I think getByEmail should not use the search terms anyway as those could be truncated.

[DeepDiver1975]

Jenkis was successfull -> merge

[PVince81]

stable10: #29341

[lock]

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.