GNOME Bugzilla – Bug 502229
Eliminate useless generic ldap authentication error messages from lib/module.php
Last modified: 2007-12-08 14:49:44 UTC
If Bug 502226 is fixed, LDAPUtil (lib/ldap.php) will return PEAR error objects instead of null, if the connection to the ldap server fails. The generic error message "LDAP authentication error" will no longer be needed. It is enough to check, if the returned object is a PEAR error object to catch all errors coming out of the class.
Created attachment 100488 [details] [review] Patch for www/new_account.php Patch to eleminate useless generic error messages. Also cleans up if-condition that raises PEAR errors coming out of the class LDAPUtils to adhere to PEAR coding standards ("You are strongly encouraged to always use curly braces even in situations where they are technically optional").
Created attachment 100489 [details] [review] Patch to lib/module.php Sorry, I made a mistake. This patch should have been to lib/module.php, not www/new_account.php
Seems ok (didn't test). Still would like to make LDAP auth failures really fatal.
(In reply to comment #3) > Seems ok (didn't test). Still would like to make LDAP auth failures really > fatal. The errors are still fatal. Basically, there are three possible states: a) At the moment, you will get a fatal PHP error in some cases. This is not good, because the error message does not necessarily explain the real problem. Compare "did not expect PEAR error object here, give me an array please" (php message) with "LDAP authentication error" (real cause of the problem). Also, in a production environment, you would normally not show those errors to the user, but log them to a log file (don't know what Gnome does, though). In those cases the user gets a totally blank page or a message that there was an error and it was logged. -> Bad. b) The error is handled by the PHP application. This is good, because we can display a nice error message to the user that really explains the problem in the words of the user. Bug 502220 is about bringing www/new_account.php from a) to b). The error shown is still kind of fatal, because all the user gets is a blank page with the words "Error: LDAP authentication failed" or something like that. c) The error is still handled by PHP, but we do actually give more detailed error messages. Instead of "Error: LDAP authentication failed." we display "Error: Unable to connect to LDAP server" or "Error: Unable to authenticate, wrong credentials", something like this. Bug 502226 and Bug 502229 bring the app from b) to c). More patches will be required to other files, to eliminate all generic error messages, though. In many cases, pages are still in state a) any way, so most of the time the ldap server fails you get strange php errors that do not really describe the problem, at the moment. In state c) the user gets a blank page with just the words "Error: cause" printed on the screen, so its still kind of fatal There are other ways, error handling could be improved. We could write a custom error handler or we could improve the wording of the error messages further (e.g. include the sysadmin team contact email), but this would require more refactoring of the application and is probably not a good idea right now.
(In reply to comment #4) > There are other ways, error handling could be improved. We could write a custom > error handler or we could improve the wording of the error messages further > (e.g. include the sysadmin team contact email), but this would require more > refactoring of the application and is probably not a good idea right now. I meant more that. Now every function that possibly uses LDAP has to check for LDAP errors. IMO that should be wrapped at one point.