From f2081a687760c9e06d68693ddffeef20750135e8 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Sun, 4 Jun 2023 10:19:03 -0500 Subject: [PATCH] If an error occurs during a search and an operation exception is thrown, return the result code as part of the SearchResultDone message. This is how LDAP clients would expect it and how the RFC specifies that it would be done. Currently, the operation exception comes as an unexpected message after the SearchResultDone which would be considered an unsolicited message from a clients perspective and would cause an immediate disconnected from the server. --- .../ServerProtocolHandler/SearchResult.php | 111 ++++++++++++++++++ .../ServerPagingHandler.php | 54 +++++++-- .../ServerPagingUnsupportedHandler.php | 21 +++- .../ServerSearchHandler.php | 22 +++- .../ServerSearchTrait.php | 11 +- .../ServerPagingHandlerSpec.php | 24 +++- .../ServerPagingUnsupportedHandlerSpec.php | 47 +++++++- .../ServerSearchHandlerSpec.php | 42 ++++++- 8 files changed, 302 insertions(+), 30 deletions(-) create mode 100644 src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/SearchResult.php diff --git a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/SearchResult.php b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/SearchResult.php new file mode 100644 index 00000000..607e0689 --- /dev/null +++ b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/SearchResult.php @@ -0,0 +1,111 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Protocol\ServerProtocolHandler; + +use FreeDSx\Ldap\Entry\Entries; +use FreeDSx\Ldap\Exception\InvalidArgumentException; +use FreeDSx\Ldap\Operation\ResultCode; + +final class SearchResult +{ + /** + * @var Entries + */ + private $entries; + + /** + * @var string + */ + private $baseDn; + + /** + * @var int + */ + private $resultCode; + + /** + * @var string + */ + private $diagnosticMessage; + + private function __construct( + Entries $entries, + string $baseDn = '', + int $resultCode = ResultCode::SUCCESS, + string $diagnosticMessage = '' + ) { + $this->entries = $entries; + $this->baseDn = $baseDn; + $this->resultCode = $resultCode; + $this->diagnosticMessage = $diagnosticMessage; + } + + /** + * Make a successful server search result representation. + */ + public static function makeSuccessResult( + Entries $entries, + string $baseDn = '', + string $diagnosticMessage = '' + ): self { + return new self( + $entries, + $baseDn, + ResultCode::SUCCESS, + $diagnosticMessage + ); + } + + /** + * Make an error result for server search result representation. This could occur for any reason, such as a base DN + * not existing. This result MUST not return a success result code. + */ + public static function makeErrorResult( + int $resultCode, + string $baseDn = '', + string $diagnosticMessage = '', + ?Entries $entries = null + ): self { + if ($resultCode === ResultCode::SUCCESS) { + throw new InvalidArgumentException('You must not return a success result code on a search error.'); + } + + return new self( + $entries ?? new Entries(), + $baseDn, + $resultCode, + $diagnosticMessage + ); + } + + public function getEntries(): Entries + { + return $this->entries; + } + + public function getResultCode(): int + { + return $this->resultCode; + } + + public function getDiagnosticMessage(): string + { + return $this->diagnosticMessage; + } + + public function getBaseDn(): string + { + return $this->baseDn; + } +} diff --git a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandler.php b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandler.php index 184b2e64..853a6c40 100644 --- a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandler.php +++ b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandler.php @@ -80,16 +80,51 @@ public function handleRequest( $token ); $pagingRequest = $this->findOrMakePagingRequest($message); + $searchRequest = $this->getSearchRequestFromMessage($message); - $response = $this->handlePaging( - $context, - $pagingRequest, - $message - ); + $response = null; + $controls = []; + try { + $response = $this->handlePaging( + $context, + $pagingRequest, + $message + ); + $searchResult = SearchResult::makeSuccessResult( + $response->getEntries(), + (string) $searchRequest->getBaseDn() + ); + $controls[] = new PagingControl( + $response->getRemaining(), + $response->isComplete() + ? '' + : $pagingRequest->getNextCookie() + ); + } catch (OperationException $e) { + $searchResult = SearchResult::makeErrorResult( + $e->getCode(), + (string) $searchRequest->getBaseDn(), + $e->getMessage() + ); + $controls[] = new PagingControl( + 0, + '' + ); + } $pagingRequest->markProcessed(); - if ($response->isComplete()) { + /** + * Per Section 3 of RFC 2696: + * + * If, for any reason, the server cannot resume a paged search operation + * for a client, then it SHOULD return the appropriate error in a + * searchResultDone entry. If this occurs, both client and server should + * assume the paged result set is closed and no longer resumable. + * + * If a search result is anything other than success, we remove the paging request. + */ + if (($response && $response->isComplete()) || $searchResult->getResultCode() !== ResultCode::SUCCESS) { $this->requestHistory->pagingRequest() ->remove($pagingRequest); $this->pagingHandler->remove( @@ -99,13 +134,10 @@ public function handleRequest( } $this->sendEntriesToClient( - $response->getEntries(), + $searchResult, $message, $queue, - new PagingControl( - $response->getRemaining(), - $response->isComplete() ? '' : $pagingRequest->getNextCookie() - ) + ...$controls ); } diff --git a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandler.php b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandler.php index 5bc26916..6cadd2dd 100644 --- a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandler.php +++ b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandler.php @@ -60,13 +60,24 @@ public function handleRequest( ); } - $entries = $dispatcher->search( - $context, - $request - ); + try { + $searchResult = SearchResult::makeSuccessResult( + $dispatcher->search( + $context, + $request + ), + (string) $request->getBaseDn() + ); + } catch (OperationException $e) { + $searchResult = SearchResult::makeErrorResult( + $e->getCode(), + (string) $request->getBaseDn(), + $e->getMessage() + ); + } $this->sendEntriesToClient( - $entries, + $searchResult, $message, $queue ); diff --git a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandler.php b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandler.php index b626b232..e71af278 100644 --- a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandler.php +++ b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandler.php @@ -11,6 +11,7 @@ namespace FreeDSx\Ldap\Protocol\ServerProtocolHandler; +use FreeDSx\Ldap\Exception\OperationException; use FreeDSx\Ldap\Protocol\LdapMessageRequest; use FreeDSx\Ldap\Protocol\Queue\ServerQueue; use FreeDSx\Ldap\Server\RequestContext; @@ -42,13 +43,24 @@ public function handleRequest( ); $request = $this->getSearchRequestFromMessage($message); - $entries = $dispatcher->search( - $context, - $request - ); + try { + $searchResult = SearchResult::makeSuccessResult( + $dispatcher->search( + $context, + $request + ), + (string) $request->getBaseDn() + ); + } catch (OperationException $e) { + $searchResult = SearchResult::makeErrorResult( + $e->getCode(), + (string) $request->getBaseDn(), + $e->getMessage() + ); + } $this->sendEntriesToClient( - $entries, + $searchResult, $message, $queue ); diff --git a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchTrait.php b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchTrait.php index e9eba934..41b49d60 100644 --- a/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchTrait.php +++ b/src/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchTrait.php @@ -27,18 +27,19 @@ trait ServerSearchTrait { /** - * @param Entries $entries + * @param SearchResult $searchResult * @param LdapMessageRequest $message * @param ServerQueue $queue * @return void */ private function sendEntriesToClient( - Entries $entries, + SearchResult $searchResult, LdapMessageRequest $message, ServerQueue $queue, Control ...$controls ): void { $messages = []; + $entries = $searchResult->getEntries(); foreach ($entries->toArray() as $entry) { $messages[] = new LdapMessageResponse( @@ -49,7 +50,11 @@ private function sendEntriesToClient( $messages[] = new LdapMessageResponse( $message->getMessageId(), - new SearchResultDone(ResultCode::SUCCESS), + new SearchResultDone( + $searchResult->getResultCode(), + $searchResult->getBaseDn(), + $searchResult->getDiagnosticMessage() + ), ...$controls ); diff --git a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandlerSpec.php b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandlerSpec.php index 456958eb..dd73f4be 100644 --- a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandlerSpec.php +++ b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingHandlerSpec.php @@ -18,7 +18,9 @@ use FreeDSx\Ldap\Entry\Entry; use FreeDSx\Ldap\Exception\OperationException; use FreeDSx\Ldap\Operation\Request\SearchRequest; +use FreeDSx\Ldap\Operation\Response\SearchResultDone; use FreeDSx\Ldap\Operation\Response\SearchResultEntry; +use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Protocol\LdapMessageRequest; use FreeDSx\Ldap\Protocol\LdapMessageResponse; use FreeDSx\Ldap\Protocol\Queue\ServerQueue; @@ -191,7 +193,7 @@ public function it_should_send_the_correct_response_if_paging_is_abandoned( ); } - public function it_throws_an_exception_if_the_old_and_new_paging_requests_are_different( + public function it_sends_a_result_code_error_in_SearchResultDone_if_the_old_and_new_paging_requests_are_different( ServerQueue $queue, RequestHandlerInterface $handler, TokenInterface $token, @@ -207,15 +209,29 @@ public function it_throws_an_exception_if_the_old_and_new_paging_requests_are_di $pagingHandler->page(Argument::any(), Argument::any()) ->shouldNotBeCalled(); $pagingHandler->remove(Argument::any(), Argument::any()) - ->shouldNotBeCalled(); + ->shouldBeCalled(); + + $queue->sendMessage(new LdapMessageResponse( + $message->getMessageId(), + new SearchResultDone( + ResultCode::OPERATIONS_ERROR, + 'dc=foo,dc=bar', + "The search request and controls must be identical between paging requests." + ), + ...[new PagingControl( + 0, + '' + )] + ))->shouldBeCalled() + ->willReturn($queue); - $this->shouldThrow(new OperationException("The search request and controls must be identical between paging requests."))->during('handleRequest', [ + $this->handleRequest( $message, $token, $handler, $queue, [] - ]); + ); } public function it_throws_an_exception_if_the_paging_cookie_does_not_exist( diff --git a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandlerSpec.php b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandlerSpec.php index a10f8da6..402830cc 100644 --- a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandlerSpec.php +++ b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerPagingUnsupportedHandlerSpec.php @@ -66,7 +66,10 @@ public function it_should_send_a_search_request_to_the_request_handler_if_paging $resultEntry2, new LdapMessageResponse( 2, - new SearchResultDone(0) + new SearchResultDone( + 0, + 'dc=foo,dc=bar' + ) ) )->shouldBeCalled(); @@ -101,4 +104,46 @@ public function it_should_throw_an_unavailable_critical_extension_if_paging_is_m [] ]); } + + public function it_should_send_a_SearchResultDone_with_an_operation_exception_thrown_from_the_handler( + ServerQueue $queue, + RequestHandlerInterface $handler, + TokenInterface $token + ): void { + $search = new LdapMessageRequest( + 2, + (new SearchRequest(Filters::equal( + 'foo', + 'bar' + )))->base('dc=foo,dc=bar'), + new PagingControl( + 10, + '' + ) + ); + + $handler->search(Argument::any(), Argument::any()) + ->willThrow(new OperationException( + "Fail", + ResultCode::OPERATIONS_ERROR + )); + + $queue->sendMessage(new LdapMessageResponse( + 2, + new SearchResultDone( + ResultCode::OPERATIONS_ERROR, + 'dc=foo,dc=bar', + "Fail" + ) + ))->shouldBeCalled() + ->willReturn($queue); + + $this->handleRequest( + $search, + $token, + $handler, + $queue, + [] + ); + } } diff --git a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandlerSpec.php b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandlerSpec.php index 942789a4..72dee545 100644 --- a/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandlerSpec.php +++ b/tests/spec/FreeDSx/Ldap/Protocol/ServerProtocolHandler/ServerSearchHandlerSpec.php @@ -13,9 +13,11 @@ use FreeDSx\Ldap\Entry\Entries; use FreeDSx\Ldap\Entry\Entry; +use FreeDSx\Ldap\Exception\OperationException; use FreeDSx\Ldap\Operation\Request\SearchRequest; use FreeDSx\Ldap\Operation\Response\SearchResultDone; use FreeDSx\Ldap\Operation\Response\SearchResultEntry; +use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Protocol\LdapMessageRequest; use FreeDSx\Ldap\Protocol\LdapMessageResponse; use FreeDSx\Ldap\Protocol\Queue\ServerQueue; @@ -51,9 +53,47 @@ public function it_should_send_a_search_request_to_the_request_handler(ServerQue $queue->sendMessage( $resultEntry1, $resultEntry2, - new LdapMessageResponse(2, new SearchResultDone(0)) + new LdapMessageResponse(2, new SearchResultDone(0, 'dc=foo,dc=bar')) )->shouldBeCalled(); $this->handleRequest($search, $token, $handler, $queue, []); } + + public function it_should_send_a_SearchResultDone_with_an_operation_exception_thrown_from_the_handler( + ServerQueue $queue, + RequestHandlerInterface $handler, + TokenInterface $token + ): void { + $search = new LdapMessageRequest( + 2, + (new SearchRequest(Filters::equal( + 'foo', + 'bar' + )))->base('dc=foo,dc=bar') + ); + + $handler->search(Argument::any(), Argument::any()) + ->willThrow(new OperationException( + "Fail", + ResultCode::OPERATIONS_ERROR + )); + + $queue->sendMessage(new LdapMessageResponse( + 2, + new SearchResultDone( + ResultCode::OPERATIONS_ERROR, + 'dc=foo,dc=bar', + "Fail" + ) + ))->shouldBeCalled() + ->willReturn($queue); + + $this->handleRequest( + $search, + $token, + $handler, + $queue, + [] + ); + } }