[Ldap] Always have a valid connection when using the EntryManager #20605
Conversation
|
Can we cover this with test? |
|
With some new insight from my side, we can say that you need to be bound to the LDAP-server (anonymous) before you can do anything with the EntryManager or the Query. This is solved in the Query class with an automatic anonymous bind if the connection is not yet bound, but the EntryManager ignores that. That means that this PR is not yet complete: we also need some automatic binding in the EntryManager for when the connection is not yet bound. Note that it is uncommon to edit entries with an anonymous bind, so the use case is small. In conclusion: the current Symfony source it works when you manually bind first, this PR does not fix the problem (yet). |
|
ping @csarrazi |
|
should be merged in 3.1, right? |
|
Regarding @bobvandevijver's comment, I'm against having an auto-bind mechanism. The Ldap and Connection classes should not be aware of the user it is currently bound to, nor should they actually bind to the server without the user explicitly using the If by "binding" you mean using an anonymous bind, then I agree that we need to run the Another way of solving this problem is simply to throw an exception if the connection object is not initialised, or if the connection is not bound already, which actually would be more secure. |
|
Wait @fabpot we still have things to discuss. I would actually prefer this to throw an exception instead of trying to connect automatically. Same thing if the connection is not bound. This will be more secure, especially since I want to prevent any auto-bind or auto-connecting logic from being inserted in the code. In my opinion, the bind or connection should be explicit, and done beforehand. |
|
I agree with @csarrazi, so I will try to update this PR with the exceptions this week. |
75435a7 to
3557d8e
Compare
3557d8e to
c87c3fd
Compare
f08249d to
3c2c3ba
Compare
|
I've updated the code to throw exceptions when a query or any data-edit is tried without binding the connection first. Also added some test to verify if the exception is thrown when the connection is not bound. I guess it should now have the functionality @csarrazi proposed. With c87c3fd Travis failed on a single run related to the HttpKernel (line 3504), so I expect that same error for the pending one. It is however not related to this PR. Edit: Indeed, again a fail on the HTTP-kernel (line 3496 this time). |
|
👍 |
|
👍 Status: reviewed |
|
Thank you @bobvandevijver. |
…Manager (bobvandevijver) This PR was submitted for the 3.2 branch but it was merged into the 3.1 branch instead (closes #20605). Discussion ---------- [Ldap] Always have a valid connection when using the EntryManager | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | They should | Fixed tickets | | License | MIT | Doc PR | The connection might be `null` when calling the `getEntryManager` function as it uses the `$this->connection` instead of retrieved it from it's own method which checks (and constructs if necessary) it first. Commits ------- 7775ec2 [Ldap] Always have a valid connection when using the EntryManager
The connection might be
nullwhen calling thegetEntryManagerfunction as it uses the$this->connectioninstead of retrieved it from it's own method which checks (and constructs if necessary) it first.