loginController now emitting failedLogin hook after failed login attempts

[karakayasemi]

Description

There is an error in the logic of emitting preLogin signal in the LoginController. UserSession's login function is checking password and emitting preLogin signal, but loginController is either checking password before UserSession and returning view response without hook signal when failed login attempts.

With this change, unnecessary checkPassword functions are removed. Code simplified and now we are emitting preLogin hook also before failed login attempts.

Motivation and Context

preLogin hook is not emitting on failed login attempts. To resolve following issue
owncloud-archive/security#1

How Has This Been Tested?

Unit tests are working

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.

[DeepDiver1975]

this totally removes the capability to login with the login name - right?

[karakayasemi]

If someone is trying to login with uid, getByEmail does not return anything. If a user's uid is the same as another user's email, it creates a problem.

[DeepDiver1975]

so login is enough - we not longer need to call checkPassword? looks strange ....

[karakayasemi]

login is also using userManager's checkPassword inside of it with emitting hook signal before and after checkPassword. IMHO we can trust login.

[DeepDiver1975]

an please squash your commits - thx

[DeepDiver1975]

We miss a login failed hook - right?

[karakayasemi]

we can add login failed hook to userSession's login method. However, I am not sure, where should we emit login failed signal also? For now, this change allows to use preLogin hook and it is enough to start security app. Should I work on login failed hook?
And also should I revert e-mail login part of the code? IMHO, it is simplifying logic and does not create any problem unless if a user's uid is the same as another user's email.

[DeepDiver1975]

And also should I revert e-mail login part of the code? IMHO, it is simplifying logic and does not create any problem unless if a user's uid is the same as another user's email.

yes please - we shall keep the behavior

[DeepDiver1975]

we can add login failed hook to userSession's login method. However, I am not sure, where should we emit login failed signal also? For now, this change allows to use preLogin hook and it is enough to start security app. Should I work on login failed hook?

we need that hook to log a failed login - or am I missing something?

[karakayasemi]

we need that hook to log a failed login - or am I missing something?

IMHO, Login hook signal should emit from UserSession's login method. Since failed login check is done by userManager's checkPassword method in our current password correction logic, we can not emit any hook signal from userSessions login method in failed login attempts. Because of that, I removed checkPassword methods and used userSession's login function.
I will try to make changes you said in today.

[DeepDiver1975]

shall we really expose the password?

[karakayasemi]

I thought, maybe it can be useful for some warning like "it is your previous password".

[karakayasemi]

Should I change password exposition? Still, I think it might be useful in fact.

[DeepDiver1975]

I would not expose the password here - @Peter-Prochaska objections?

[phil-davis]

As a user, I might have accidentally put in a password for an account of mine on some other site, so would rather not have it displayed/logged... anywhere.

[karakayasemi]

Now, failed login hook is exposing the only username.

[PVince81]

@DeepDiver1975 is this now acceptable for merging ?

[PVince81]

This introduced a regression which was now fixed in #28450

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