-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(user_ldap): Do not block access to configuration page upon bad configuration #43527
base: master
Are you sure you want to change the base?
Conversation
…nfiguration It should be possible to access configuration page with the local admin user even when LDAP configuration is not valid. This prevent ldap backend from throwing in countUsers to allow this. Signed-off-by: Côme Chilliet <[email protected]>
/backport to stable28 |
/backport to stable27 |
/backport to stable26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment in code, for one would not be able to tell why the Exception is caught here and nowhere else.
Without having a better idea, it also feels weird to solve it this way, because
- the countUsers() method is not hard wired to the settings page
- a change there can have the side effect to make this solutions fail as side effect
- for completeness, side effects from this are also possible, but unlikely to be problematic
Not sure if it is possible to catch is somewhere higher, more to the configuration logic?
Well the method was not documented to throw anything, but is documented to return false if an error happens. |
I am not sure the throw tag was set consistently back then. |
@blizzz Is it a strong no on this one? |
It is not a strong no, but it would cause me stomachaches. Unfortunately without looking into the matter I have no better idea at the moment either, and it is unlikely to change in the near future. That said a pragmatic approach is to go ahead with this solution, but opening an issue to have an improvement in mind at least. And add some comments into the code to clearly state the intention and meaning of the change, for by looking at it you would not have the slightest idea about it's actual purpose. |
Summary
It should be possible to access configuration page with the local admin
user even when LDAP configuration is not valid.
This prevent ldap backend from throwing in countUsers to allow this.
Checklist