Skip to content
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

user_ldap fix ldap connection resets #31421

Merged
merged 6 commits into from
Mar 17, 2022

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Mar 3, 2022

First commit is a patch sent to a customer to fix a problem where LDAP connection was reseted during executeRead, and because the connection resource var was not managed consistently the get first entry would fail.
Patch was tested and approved by customer.
The second commit is a clean up of typing.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc self-assigned this Mar 3, 2022
@come-nc come-nc added this to the Nextcloud 24 milestone Mar 3, 2022
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@solracsf solracsf added the 3. to review Waiting for reviews label Mar 3, 2022
@come-nc come-nc added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 3, 2022
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 7, 2022
@come-nc come-nc requested review from ChristophWurst, blizzz, a team, nickvergessen and CarlSchwan and removed request for ChristophWurst and a team March 7, 2022 16:53
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good in general, please check the one comment about suspicious code removal

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one concern

@@ -1183,7 +1147,7 @@ private function processPagedSearchStatus(
bool $pagedSearchOK,
bool $skipHandling
): bool {
$cookie = null;
$cookie = '';
Copy link
Member

@blizzz blizzz Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to resort to null here for behaviour of LDAP servers, cf. comment on line 1175ff – null and empty string may have different meanings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controlPagedResultResponse is forcing $cookie to be a string.

null cookies are turned into empty string here: https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/PagedResults/Php73.php#L81

@come-nc come-nc requested a review from blizzz March 14, 2022 10:32
@come-nc come-nc merged commit 475a859 into master Mar 17, 2022
@come-nc come-nc deleted the fix/user_ldap-fix-ldap-connection-resets branch March 17, 2022 08:13
come-nc added a commit that referenced this pull request Mar 31, 2022
[stable23] user_ldap fix ldap connection resets #31421
blizzz added a commit that referenced this pull request Apr 14, 2022
[stable22] user_ldap fix ldap connection resets #31421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants