Send 32 instead of 2 for empty SearchResultDone

[Redjard]

Send NO_SUCH_OBJECT (32) instead of SUCCESS (2) as code of SearchResultDone message when no entries where returned. This matches the ldap spec, and behavior of other ldap server implementations.

[Redjard]

I had first hacked together this monstrosity in my search() function in GenericRequestHandler

if (empty($elements))
  throw new FreeDSx\Ldap\Exception\OperationException('No such object', FreeDSx\Ldap\Operation\ResultCode::NO_SUCH_OBJECT);

but then later found This to cause random issues all over the place. I assume this kills the connection which breaks some requests for some clients, but haven't investigated further. This needs to be fixed in the library, not GenericRequestHandler.

[ChadSikorra]

Hi @Redjard ! Thanks for taking the time to report this. It appears like when to return NO_SUCH_OBJECT is a bit more subtle, according to the RFC. It states the following:

https://datatracker.ietf.org/doc/html/rfc4511#section-4.5.3

A server MUST NOT return any SearchResultReference messages if it has not located the baseObject and thus has not searched any entries. In this case, it would return a SearchResultDone containing either a referral or noSuchObject result code (depending on the server's knowledge of the entry named in the baseObject).

So I take that to mean that a NO_SUCH_OBJECT is returned if the base DN of the search does not exist, not if no entries were found underneath it.

So in the request handler search(), you would have to first check if the baseDn from the request exists, and if it does not then you return the NO_SUCH_OBJECT response.

$baseDn = $search->getBaseDn();

// Logic here to figure out if that base actually exists.
// ....

if ($noSuchObject) {
    throw new OperationException(
        'No such object',
        ResultCode::NO_SUCH_OBJECT
     );
}

This seems like an unfortunate detail that request handler implementations will need to be aware of, otherwise it would require a change in the interface to force them to specify whether the baseDn exists or not, along with the entries contained in it.

[Redjard]

Right, I completely missed that, thanks.
Checking with openldap though it doesn't drop the connection even when I search with a nonexistent baseDN, which my server would if I throw an error. It is probably far less of an issue as usually the kinds of requests that share a session are done by automated systems which shouldn't ask for nonexistent baseDNs, but it still feels wrong.
Unfortunately my fix also seems wrong with how the codebase looks, but here goes:

FreeDSx\Ldap\Protocol\ServerProtocolHandler\ServerSearchHandler->handleRequest()

$entries = $dispatcher->search(
    $context,
    $request
);

$this->sendEntriesToClient(
    $entries,
    $message,
    $queue
);

replaced with

try {
    $entries = $dispatcher->search(
        $context,
        $request
    );
    $resultCode = ResultCode::SUCCESS;
} catch ( OperationException $exception ) {
    // don't close connection on OperationException
    $entries = new Entries();
    $resultCode = $exception->getCode();
}

$this->sendEntriesToClient(
    $entries,
    $message,
    $queue,
    $resultCode
);

FreeDSx\Ldap\Protocol\ServerProtocolHandler\ServerSearchTrait->sendEntriesToClient()

private function sendEntriesToClient(
    Entries $entries,
    LdapMessageRequest $message,
    ServerQueue $queue,
    Control ...$controls
): void {
    [...]

    $messages[] = new LdapMessageResponse(
        $message->getMessageId(),
        new SearchResultDone(ResultCode::SUCCESS),
        ...$controls
    );
    [...]

replaced with

private function sendEntriesToClient(
    Entries $entries,
    LdapMessageRequest $message,
    ServerQueue $queue,
    Int $resultCode,
    Control ...$controls
): void {
    [...]

    $messages[] = new LdapMessageResponse(
        $message->getMessageId(),
        new SearchResultDone($resultCode),
        ...$controls
    );
    [...]

I assume this is horrendous, that's why I didn't open another merge request, this is how I implemented it in my project now.

[ChadSikorra]

Ah, that's right. Now that I look at your solution I understand the problem. The issue is that we need to intercept operation exception when the SearchResultDone is sent back and not in the spot where it's generally caught / acted upon in the ServerProtocolHandler. The issue is that there's also a few different spots where that dispatcher is called for searching before that SearchResultDone is made, because the server may or may not support paging.

I need to think a little bit about a good way to handle it. I don't think we want to put any potential operation exception code into SearchResultDone, only certain ones when we search. Maybe a new exception would make sense. Or just checking the exception for the NO_SUCH_OBJECT code only I guess.

[ChadSikorra]

Ok, I believe this PR should clear it up: #63. I'm not sure if you have any way to test if the changes in that PR helps with your situation. The tricky part was changing this in all the spots that handle searches in different ways:

• Paging based searches.
• Searches with no paging.
• Searches with paging when the server doesn't actually support it.

But I believe it now properly sets the diagnostic message / result code / baseDN from the search request in the SearchResultDone.

[ChadSikorra]

I've merged that fix into main. It's going to be rough keeping compatibility between my new 1.0 branch I'm working on and fixes for what is in master at the moment. I think the server request handler interface will have breaking changes in 1.0 to return the SearchResult instead of just Entries for a search. Let me know if you still have issues with changes. I will have one more small release / tag with these changes before 1.0.