[#35347] - [4] Catch LDAP Connection exceptions
- Fixed in Code Base
- 26 Aug 2021
- Medium
- Build: 4.0-dev
- # 35347
- Diff
- PhilETaylor:catchldapconnectionissues
User tests: Successful: Unsuccessful:
Summary of Changes
When trying to login, with a username/password that doesnt match the internal Joomla database of users, and with LDAP configured, but the LDAP server is down, Symfony LDAP currently throws an LdapException that is not caught
This PR catches that (using PHP 7.1 piped exceptions - thanks @mbabker) and displays a normal Joomla based message instead of a displaying an uncaught exception.
Testing Instructions
YOU DONT NEED LDAP EXPERIENCE OR AN LDAP SERVER TO TEST THIS !!
Configure the LDAP Authentication Plugin, making sure that whatever you put in the HOST field is a valid IP or server, but NOT a valid LDAP service running on that host - my settings:
try to login to Joomla admin with fake credentials.
Actual result BEFORE applying this Pull Request
or with debug enabled
Expected result AFTER applying this Pull Request
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Plugins |
| Labels |
Added:
?
|
||
I have tested this item
without a LDAP server
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35347.
@PhilETaylor The fix works nice for Authorisation Method = "Bind and search". But with Authorisation Method = "Bind Directly as User" it doesn't. Would it need the same change which you did in line 88 also in line 146?
I.e. change there to catch (ConnectionException|LdapException $exception), too.
I've tested that here and it worked.
@PhilETaylor Beside the above suggestions for code style, did you check my comment above about the other place where I think the changes is also needed? #35347 (comment)
Im not really bothered, now knowing this plugin is not worth the bytes it consumes and needs rewriting from scratch to support all the features Joomla 3 supported, and to fix other bugs :-) if you can suggest the other places in the PR I'll hit the merge button on those ... a bit busy today catching up with non-joomla work.
if you can suggest the other places in the PR I'll hit the merge button on those
@PhilETaylor Can't use suggestion on GitHub because too far away from changed code. Will make PR for your branch instead.
ah sorry
@PhilETaylor See PhilETaylor#11 .. it also contains testing instructions for the part which it adds to your PR.
Merged.
I have tested this item
With the PR applied, I get an error alert depending on the authorisation method chosen in the plugin's options:
- Authorisation Method "Bind Directly as User": Message is "Username and password do not match or you do not have an account yet.".
- Authorisation Method "Bind and search": Message is "Unable to connect to authentication service.".
Without the PR I get an unhandled exception in both cases.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35347.
thanks guys
I have tested this item
without a LDAP server
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35347.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35347.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-08-26 14:35:48 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Thanks!
This PR seems to have broken Drone?