Fix user_ldap login on master by using userSession to retrieve the user object

[fredreichbier]

Description

After having successfully validated the user credentials, the tryLogin function calls

$loginResult = $this->userManager->get($user)

to get hold of the user object. Here, $user is the login name provided by the user on login.

This causes a problem if the user_ldap app is used because the UID does not correspond to the login name given by the user in that case. Rather, the UID is the UUID of the LDAP entry (e.g. 7c1247da-d30e-1036-8350-cd5127ce8b3b). Consequently, userManager->get($user) does not find the user and returns null, which causes an exception when $loginResult->getUID() is called in the next line.

This PR works around this problem by retrieving the user object via userSession->getUser instead. However, I don't know much about the ownCloud codebase, so please let me know if there is anything wrong with this approach :-)

Related Issue

#28449

Motivation and Context

It fixes user login when using the user_ldap app.

How Has This Been Tested?

I tested the login manually.

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.

[CLAassistant]

All committers have signed the CLA.

[cornelinux]

Maybe the usersession is null in certain cases, so that getUser/getUID will fail?

[fredreichbier]

Sorry, obviously I should have run the tests locally before doing the PR.
I think the three test cases fail because they operate on a mock userSession and this mock object doesn't know about the additional getUser call. I'll see if that's right and what I can do about it. :)

[karakayasemi]

IMHO, it makes sense to get the user who is logged in from the userSession instead of userManager. 👍

[jvillafanez]

Mind changing the variable name? That's not a login result, nor it doesn't make sense to get a UID from a login result.

[jvillafanez]

I wonder if there is something else behind that which is causing the problem. Anyway, I don't have a strong opposition for this change.

[karakayasemi]

It is related to #28206 . Recently I added failed login hook and move login validation logic to userSession instead of userManager. Before my commit, $loginResult variable was assigning by userManager's checkPassword method as User object on success case. Now, the password verification is done during into userSession's login method. Therefore, $loginResult variable should assign from userSession's getUser method in my POV.

[karakayasemi]

It may be better to change the variable name also.

[fredreichbier]

Thank you for looking into this @karakayasemi @jvillafanez! I changed the variable name to $userObject -- I guess that's better?

[jvillafanez]

Changes are good 👍

Maybe @DeepDiver1975 or @PVince81 can kick jenkins.

[PVince81]

kicked

[PVince81]

09:55:13 ERROR: Timeout after 10 minutes
09:55:13 ERROR: Error fetching remote repo 'origin'
09:55:13 hudson.plugins.git.GitException: Failed to fetch from https://github.com/owncloud/core.git

@DeepDiver1975 😞

[PVince81]

kicked Jenkins again...

[PVince81]

@jvillafanez can you backport this to stable10 ?

[PVince81]

(assuming the problem exists on stable10 too)

[karakayasemi]

It is related to #28206. It was only merged to master. stable10 was not affected. No need to backport.

[PVince81]

@karakayasemi thanks for the info. I added a note here to link the tickets #28206 (comment).

Now considering that master will not be released any time soon, we might want to backport both #28206 and #28450 to stable10. @DeepDiver1975 thoughts ?

[DeepDiver1975]

Yes - please backport them all @karakayasemi - THX

[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.