Include a flag to mark if the loginName2userName has been processed#39105
Include a flag to mark if the loginName2userName has been processed#39105JammingBen merged 3 commits intomasterfrom
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
This is pending to confirm that it fixes the original issue. Code should be ready to review, and only the changelog is missing. |
jvillafanez
force-pushed
the
loginname_process_chain
branch
from
113f0a0 to
aae4449
Compare
AlexAndBear
left a comment
AlexAndBear
left a comment
There was a problem hiding this comment.
I don't like how the value is passed by reference through multiple files, maybe we should use a class to handle this?
|
I don't think we have a better choice. The result comes from an event, so we shouldn't change the approach. We have multiple listeners and all of them have to respond, so that flag will indicate that we have a valid result and the next listeners shouldn't need to process anything. A better approach would be to use an interface with a registration process so core could have a better control on who and how the calls are done. The problem is that this is a breaking change which will need changes in all the related apps. Note that the goal of the fix is that if someone has already returned a valid value for the event, the rest of the listeners shouldn't need to bother to process the event because we already have a result. Without changing the approach of using an event (which, as said, is questionable), using a flag in the event is the easiest solution. |
|
Restarted CI |
jvillafanez
force-pushed
the
loginname_process_chain
branch
from
0bb489f to
362bb6e
Compare
|
💥 Acceptance tests pipeline webUIComments-chrome-mariadb10.2-php7.4 failed. The build has been cancelled. |
jvillafanez
force-pushed
the
loginname_process_chain
branch
from
362bb6e to
8c5e5fb
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
Confirmed fixed in 10.9.0 beta1 When user admin types his correct password, no ldap requests are done. |
|
Weird... owncloud/user_ldap#675 should be needed and it hasn't been merged yet... 🤔 |
6 participants
Description
Add a flag when asking for the username to indicate that someone has already processed the request and there is no need for others to check further.
Requires:
Related Issue
Motivation and Context
When the user_ldap app is enabled, logging in with the admin user triggered an LDAP request. This shouldn't happen when the admin's account is empty and there is no relation with any ldap user.
If the LDAP server is busy, it will slow down the login process even for non-ldap users.
How Has This Been Tested?
Having the user_ldap app enabled (and patched), log in with the admin user. With both patches, there is no bind request send to the LDAP server when logged in with the admin user.
Screenshots (if appropriate):
Types of changes
Checklist: