add a catch block for expected exceptions in password check #453
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #453 +/- ##
=========================================
Coverage 35.72% 35.72%
Complexity 1340 1340
=========================================
Files 31 31
Lines 3790 3790
=========================================
Hits 1354 1354
Misses 2436 2436
Continue to review full report at Codecov.
|
karakayasemi
changed the title
karakayasemi
force-pushed
the
bind-ex
branch
3 times, most recently
from
da8613d to
74140f6
Compare
karakayasemi
changed the title
lib/Connection.php
Outdated
| @@ -574,6 +574,9 @@ private function establishConnection() { | |||
| } | |||
|
|
|||
| /** | |||
| * Performs syntactic check against given host and port, | |||
| * If ldapTLS disabled, the server will not be contacted! | |||
There was a problem hiding this comment.
this sounds like a configuration issue. Did you check with a server without TLS? I mean, if your LDAP server requires a TLS connection, you won't be able to connect to that server if the option is disabled, but that's part of the connection configuration.
There was a problem hiding this comment.
yep. IIRC ldapTLS is autoprobed while testing the connection.
So "If ldapTLS was changed on the server after the connection had been saved, the connection will fail" is true and just "If ldapTLS disabled, the server will not be contacted!" doesn't look correct.
There was a problem hiding this comment.
So "If ldapTLS was changed on the server after the connection had been saved, the connection will fail" is true
No, in the stated case doConnect method will not fail, it will return true if the host and port parameters are in the right format. This is what I am trying to explain in the phpdoc of the method.
I tried to highlight the documentation note of the ldap_connect method: https://www.php.net/manual/en/function.ldap-connect.php . According to its documentationldap_connect method is only used for checks whether the given host and port are plausible, the actual connect happens with the next calls to ldap_*. In our doConnect method, we only make the next call if ldapTLS configuration is enabled (by calling ldap_start_tls).
However,
Line 531 in 19205d7
ldap_bind method will run after doConnect method, it can only fail in the wrong password case, which is wrong. In addition, Line 319 in 19205d7
ldap_bind fail by disabling error code check.
If you want, we can think of a better sentence for doConnect method phpdoc to explain the described behavior.
There was a problem hiding this comment.
@jvillafanez, @VicDeo Do you have any suggestions to complete this PR?
There was a problem hiding this comment.
Maybe change the doc to something like:
This method will perform some setup required to connect to the LDAP server,
but it won't connect to the LDAP server
which also makes me think if the method should throw a "ServerNotAvailableException"... probably something to review in a different PR.
There was a problem hiding this comment.
@jvillafanez the thing is if TLS enabled, it will run ldap_start_tls and perform a connect.
There was a problem hiding this comment.
This method will perform some setup required to connect to the LDAP server,
but it won't connect to the LDAP server unless ldap_start_tls is activated
or something along those lines
There was a problem hiding this comment.
Thank you for redirection, phpdoc updated.
|
code changes looks fine, though I haven't tested them. |
|
Some of the newly added acceptance tests are failing, unrelated to this PR: owncloud/core#36283 |
|
I will restart tests after owncloud/core#36293 resolved. |
|
Fix for the drone test failure is done in owncloud/core#36294. Hopefully it will be available in next qa-tarball. |
phil-davis
changed the title
Fixes https://github.com/owncloud/enterprise/issues/3496
This PR adds a catch blog for expected exceptions in password check, to not show internal error for users.
ldap_connectonly applies syntactic checks for given parameters, does not contact the server. In our implementation, as far as I see, we assume the server will be available if a request passes from this check. I clarified this situation with comment lines for further usages.