fix: Move login via email logic to local backend #47686
Conversation
susnux
added
feature: users and groups
technical debt
php
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lib/private/User/Database.php
Outdated
| private function loadUser($uid) { | ||
| $this->fixDI(); | ||
| private function loadUser(string $loginName, bool $tryEmail = true): bool { | ||
| $uid = (string)$loginName; |
Check failure
Code scanning / Psalm
RedundantCast
lib/private/User/Database.php
Outdated
| if ($result) { | ||
| // Also add cache result for the email | ||
| $this->cache[$uid] = [ | ||
| ...$this->cache[$emailUId], |
Check failure
Code scanning / Psalm
InvalidOperand
susnux
force-pushed
the
fix/move-email-logic-local-user-backend
branch
3 times, most recently
from
1b08425 to
e7fc0bd
Compare
Backends can decide which names they accept for login, e.g. with user_ldap you can configure arbitrary login fields. This was a hacky approach to allow login via email, so instead this is now only handled by the local user backend. This also fixes some other related problems: Other logic relys on `backend::get()` which was not handling email, so e.g. password policy could not block users logged in via email if they use out-dated passwords. Similar for other integrations, as the user backend was not consistent with what is a login name and what not. Signed-off-by: Ferdinand Thiessen <[email protected]>
susnux
force-pushed
the
fix/move-email-logic-local-user-backend
branch
from
e7fc0bd to
5536284
Compare
susnux
added
3. to review
susnux
requested review from
nickvergessen,
ChristophWurst,
come-nc and
blizzz
|
This could log out people from their instance unexpectedly when they log in with email instead of user id with LDAP. Similarly it breaks the https://github.com/nextcloud/user_external/ app users that logged in with email. |
For LDAP we use the login attribute filter, so this should not be affected see the workaround in the removed login flow file.
This could be true but in that case we should fix that app, no? But maybe we need to pause this for 32 instead? |
Sound like a good idea to merge next week after stable31 is branched off, and then leave the user_external app an issue what they need to do |
| /** | ||
| * FIXME: This function should not be required! | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * FIXME: This function should not be required! | |
| */ |
| //guests $uid could be NULL or '' | ||
| if ($loginName === '') { | ||
| $this->cache[$loginName] = false; | ||
| return true; |
There was a problem hiding this comment.
according to phpdoc return true here means user was found, but here it was not?
| if (isset($this->cache[$loginName])) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
| if (isset($this->cache[$loginName])) { | |
| return true; | |
| } | |
| if (isset($this->cache[$loginName])) { | |
| return $this->cache[$loginName] !== false; | |
| } |
I think?
| return false; | ||
| // Not found by UID so we try also for email, load uid for email. | ||
| if ($tryEmail) { | ||
| @[$emailUId] = $this->config->getUsersForUserValue('settings', 'email', mb_strtolower($loginName)); |
There was a problem hiding this comment.
no @ operator please. Especially on a list construction like this it’s highly confusing.
| if ($result) { | ||
| // Also add cache result for the email | ||
| $this->cache[$loginName] = $this->cache[$emailUId]; | ||
| // Set a reference to the uid cache entry for also delete email entry on user delete | ||
| $this->cache[$emailUId]['email'] = $loginName; | ||
| } | ||
| return $result; |
There was a problem hiding this comment.
Why is a false result not cached here while it’s cached for the other situations?
Summary
Backends can decide which names they accept for login, e.g. with user_ldap you can configure arbitrary login fields. This was a hacky approach to allow login via email, so instead this is now only handled by the local user backend.
This also fixes some other related problems:
Other logic relys on
backend::get()which was not handling email, so e.g. password policy could not block users logged in via email if they use out-dated passwords.Similar for other integrations, as the user backend was not consistent with what is a login name and what not.
Checklist