From 0b3a4c0fa881031cdac7281af8f44762d431fcfc Mon Sep 17 00:00:00 2001 From: Nikki Nikkhoui Date: Fri, 8 Oct 2021 16:05:15 -0400 Subject: [PATCH] Do not cache private wiki completion results Previously, when a user with correct permissions uses completion search on a private wiki, the results are returned and cached. Since we are on a private wiki, we don't want to cache results since the content is not accessible to all users. Now, content that is not accessible to all users will not be cached. This patch achieves this by setting the appropriate Cache-Control response headers for the MW REST Search endpoint. Bug: T292763 Change-Id: I693b4088df9c0520d5238c286312ec52ab273604 --- includes/Rest/Handler/SearchHandler.php | 16 +++++++-- includes/Rest/coreRoutes.json | 6 ++-- .../Rest/Handler/SearchHandlerTest.php | 36 +++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/includes/Rest/Handler/SearchHandler.php b/includes/Rest/Handler/SearchHandler.php index d0147ac8b99..6d5c9d46543 100644 --- a/includes/Rest/Handler/SearchHandler.php +++ b/includes/Rest/Handler/SearchHandler.php @@ -6,6 +6,7 @@ use Config; use InvalidArgumentException; use ISearchResultSet; use MediaWiki\Page\ProperPageIdentity; +use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\Handler; use MediaWiki\Rest\LocalizedHttpException; use MediaWiki\Rest\Response; @@ -32,6 +33,9 @@ class SearchHandler extends Handler { /** @var SearchEngineConfig */ private $searchEngineConfig; + /** @var PermissionManager */ + private $permissionManager; + /** * Search page body and titles. */ @@ -73,14 +77,17 @@ class SearchHandler extends Handler { * @param Config $config * @param SearchEngineFactory $searchEngineFactory * @param SearchEngineConfig $searchEngineConfig + * @param PermissionManager $permissionManager */ public function __construct( Config $config, SearchEngineFactory $searchEngineFactory, - SearchEngineConfig $searchEngineConfig + SearchEngineConfig $searchEngineConfig, + PermissionManager $permissionManager ) { $this->searchEngineFactory = $searchEngineFactory; $this->searchEngineConfig = $searchEngineConfig; + $this->permissionManager = $permissionManager; // @todo Avoid injecting the entire config, see T246377 $this->completionCacheExpiry = $config->get( 'SearchSuggestCacheExpiry' ); @@ -327,14 +334,17 @@ class SearchHandler extends Handler { $this->buildDescriptionsFromPageIdentities( $pageIdentities ), $this->buildThumbnailsFromPageIdentities( $pageIdentities ) ); - $response = $this->getResponseFactory()->createJson( [ 'pages' => $result ] ); if ( $this->mode === self::COMPLETION_MODE && $this->completionCacheExpiry ) { // Type-ahead completion matches should be cached by the client and // in the CDN, especially for short prefixes. // See also $wgSearchSuggestCacheExpiry and ApiOpenSearch - $response->setHeader( 'Cache-Control', 'public, max-age=' . $this->completionCacheExpiry ); + if ( $this->permissionManager->isEveryoneAllowed( 'read' ) ) { + $response->setHeader( 'Cache-Control', 'public, max-age=' . $this->completionCacheExpiry ); + } else { + $response->setHeader( 'Cache-Control', 'no-store, max-age=0' ); + } } return $response; diff --git a/includes/Rest/coreRoutes.json b/includes/Rest/coreRoutes.json index 3cd9c6e4037..57a4ae05a06 100644 --- a/includes/Rest/coreRoutes.json +++ b/includes/Rest/coreRoutes.json @@ -49,7 +49,8 @@ "services": [ "MainConfig", "SearchEngineFactory", - "SearchEngineConfig" + "SearchEngineConfig", + "PermissionManager" ], "mode": "fulltext" }, @@ -59,7 +60,8 @@ "services": [ "MainConfig", "SearchEngineFactory", - "SearchEngineConfig" + "SearchEngineConfig", + "PermissionManager" ], "mode": "completion" }, diff --git a/tests/phpunit/unit/includes/Rest/Handler/SearchHandlerTest.php b/tests/phpunit/unit/includes/Rest/Handler/SearchHandlerTest.php index 4b464d5c37a..e899408a2e9 100644 --- a/tests/phpunit/unit/includes/Rest/Handler/SearchHandlerTest.php +++ b/tests/phpunit/unit/includes/Rest/Handler/SearchHandlerTest.php @@ -6,6 +6,7 @@ use HashConfig; use InvalidArgumentException; use Language; use MediaWiki\Page\PageIdentity; +use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\Handler\SearchHandler; use MediaWiki\Rest\LocalizedHttpException; use MediaWiki\Rest\RequestData; @@ -40,6 +41,7 @@ class SearchHandlerTest extends \MediaWikiUnitTestCase { * @param SearchResultSet|Status $titleResult * @param SearchResultSet|Status $textResult * @param SearchSuggestionSet|null $completionResult + * @param PermissionManager|null $permissionManager * * @return SearchHandler */ @@ -47,7 +49,8 @@ class SearchHandlerTest extends \MediaWikiUnitTestCase { $query, $titleResult, $textResult, - $completionResult = null + $completionResult = null, + $permissionManager = null ) { $config = new HashConfig( [ 'SearchType' => 'test', @@ -61,6 +64,13 @@ class SearchHandlerTest extends \MediaWikiUnitTestCase { $hookContainer = $this->createHookContainer(); $searchEngineConfig = new \SearchEngineConfig( $config, $language, $hookContainer, [] ); + if ( !$permissionManager ) { + $permissionManager = $this->createMock( PermissionManager::class ); + $permissionManager->method( 'isEveryoneAllowed' ) + ->with( 'read' ) + ->willReturn( true ); + } + /** @var SearchEngine|MockObject $searchEngine */ $this->searchEngine = $this->createMock( SearchEngine::class ); $this->searchEngine->method( 'searchTitle' ) @@ -84,7 +94,8 @@ class SearchHandlerTest extends \MediaWikiUnitTestCase { return new SearchHandler( $config, $searchEngineFactory, - $searchEngineConfig + $searchEngineConfig, + $permissionManager ); } @@ -204,6 +215,27 @@ class SearchHandlerTest extends \MediaWikiUnitTestCase { $this->assertSame( 'Frobnitz', $data['pages'][1]['excerpt'] ); } + public function testCompletionSearchNotCachedForPublicPages() { + $titleResults = new MockSearchResultSet( [] ); + $textResults = new MockSearchResultSet( [] ); + $completionResults = new SearchSuggestionSet( [] ); + + $query = 'foo'; + $request = new RequestData( [ 'queryParams' => [ 'q' => $query ] ] ); + + $permissionManager = $this->createMock( PermissionManager::class ); + $permissionManager->method( 'isEveryoneAllowed' ) + ->with( 'read' ) + ->willReturn( false ); + + $handler = $this->newHandler( $query, $titleResults, $textResults, $completionResults, $permissionManager ); + $config = [ 'mode' => SearchHandler::COMPLETION_MODE ]; + + $response = $this->executeHandler( $handler, $request, $config ); + $this->assertSame( 'no-store, max-age=0', $response->getHeaderLine( 'Cache-Control' ) ); + $this->assertSame( 200, $response->getStatusCode() ); + } + public function testExecute_limit() { $titleResults = new MockSearchResultSet( [ $this->makeMockSearchResult( 'Foo' ),