Skip to content

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.

@come-nc come-nc self-assigned this Mar 3, 2022
@come-nc come-nc added this to the Nextcloud 24 milestone Mar 3, 2022
@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 <[email protected]>
@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 a team, CarlSchwan, ChristophWurst, blizzz and nickvergessen and removed request for a team and ChristophWurst March 7, 2022 16:53
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

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.

6 participants