Gate GET /subject-labels by per-page read permission

docs/api/rest-api.md

@@ -15,11 +15,11 @@ Every endpoint is also published as a complete [OpenAPI 3.0 description](#full-s
 
 ## Permissions
 
-The Subject, page-subjects, Schema, Layout, RDF export, and entity-dereference read endpoints enforce the caller's
-per-page `read` permission; page protection and `$wgNamespaceProtection` do not restrict them, because MediaWiki's
-`read` action ignores both. When you may not read a page they respond as if the data were absent — a `null` value, an
-empty list, or a `404` — never a `403`. `GET /subject-labels` is the exception: it filters only by wiki and Schema, not
-by per-page `read`.
+The Subject, page-subjects, subject-labels, Schema, Layout, RDF export, and entity-dereference read endpoints enforce
+the caller's per-page `read` permission; page protection and `$wgNamespaceProtection` do not restrict them, because
+MediaWiki's `read` action ignores both. When you may not read a page they respond as if the data were absent — a `null`
+value, an empty list, or a `404` — never a `403`. `GET /subject-labels` omits the labels of Subjects whose page you
+cannot read; because that filter runs per result, it caps `limit` at 50.
 
 Subject write endpoints require per-page `edit` permission and answer `403` on denial.
 

src/Application/SubjectLabelLookup.php

@@ -7,6 +7,9 @@
 interface SubjectLabelLookup {
 
 	/**
+	 * @param int $limit Maximum results to return. Callers must cap this to a small value: an
+	 *   implementation may over-fetch and run a per-result permission check, so an unbounded limit
+	 *   is unbounded work. The REST entry point caps it at 50.
 	 * @return SubjectLabelLookupResult[]
 	 */
 	public function getSubjectLabelsMatching( string $search, int $limit, string $schemaName ): array;

src/EntryPoints/REST/GetSubjectLabelsApi.php

@@ -10,6 +10,7 @@
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
 use ProfessionalWiki\NeoWiki\NeoWikiExtension;
 use Wikimedia\ParamValidator\ParamValidator;
+use Wikimedia\ParamValidator\TypeDef\IntegerDef;
 
 class GetSubjectLabelsApi extends SimpleHandler {
 
@@ -57,6 +58,8 @@ public function getParamSettings(): array {
 				ParamValidator::PARAM_TYPE => 'integer',
 				ParamValidator::PARAM_REQUIRED => false,
 				ParamValidator::PARAM_DEFAULT => 10,
+				IntegerDef::PARAM_MIN => 1,
+				IntegerDef::PARAM_MAX => 50,
 				self::PARAM_DESCRIPTION => 'Maximum number of items to return.',
 			],
 		];

src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jSubjectLabelLookup.php

@@ -7,14 +7,24 @@
 use Laudis\Neo4j\Contracts\ClientInterface;
 use Laudis\Neo4j\Contracts\TransactionInterface;
 use Laudis\Neo4j\Databags\SummarizedResult;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookup;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
+use ProfessionalWiki\NeoWiki\Domain\Page\PageId;
 
 class Neo4jSubjectLabelLookup implements SubjectLabelLookup {
 
+	/**
+	 * Rows are read past the requested limit so that dropping Subjects on pages the caller cannot
+	 * read still tends to fill it. A heuristic, not a guarantee: a run of unreadable Subjects longer
+	 * than the extra headroom can still shorten the result.
+	 */
+	private const int OVERFETCH_FACTOR = 4;
+
 	public function __construct(
 		private readonly ClientInterface $client,
 		private readonly string $wikiId,
+		private readonly PageReadAuthorizer $readAuthorizer,
 	) {
 	}
 
@@ -26,36 +36,63 @@ public function getSubjectLabelsMatching( string $search, int $limit, string $sc
 			return [];
 		}
 
+		$results = [];
+
+		// Authorize rows one at a time and stop once the limit is filled, so when readable rows are
+		// plentiful the expensive per-page check runs about `limit` times rather than across the
+		// whole over-fetched window. A mostly-unreadable search still checks the whole window.
+		foreach ( $this->fetchLabels( $search, $limit * self::OVERFETCH_FACTOR, $schemaName ) as $row ) {
+			if ( count( $results ) >= $limit ) {
+				break;
+			}
+
+			if ( $this->readAuthorizer->authorizeReadByPageId( new PageId( $row['pageId'] ) ) ) {
+				$results[] = new SubjectLabelLookupResult( id: $row['id'], label: $row['name'] );
+			}
+		}
+
+		return $results;
+	}
+
+	/**
+	 * The Subject is reached through a Page owned by the current wiki, so the caller can check
+	 * per-page read access and the returned page id always resolves within this wiki (page ids
+	 * are unique only per wiki).
+	 *
+	 * @return list<array{id: string, name: string, pageId: int}>
+	 */
+	private function fetchLabels( string $search, int $limit, string $schemaName ): array {
 		return $this->client->readTransaction(
 			function ( TransactionInterface $transaction ) use ( $search, $limit, $schemaName ): array {
 				/**
 				 * @var SummarizedResult $result
 				 */
 				$result = $transaction->run(
-					"MATCH (n:Subject)
+					"MATCH (page:Page { wiki_id: \$wikiId })-[:HasSubject]->(n:Subject)
 					 WHERE toLower(n.name) STARTS WITH toLower(\$search)
 					 AND \$schemaName IN labels(n)
 					 AND n.wiki_id = \$wikiId
-					 RETURN n.id AS id, n.name AS name
+					 RETURN n.id AS id, n.name AS name, page.id AS pageId
 					 ORDER BY n.name
 					 LIMIT \$limit",
 					[
 						'search' => $search,
-						'limit' => (int)$limit,
+						'limit' => $limit,
 						'schemaName' => $schemaName,
 						'wikiId' => $this->wikiId,
 					]
 				);
 
-				$subjects = [];
+				$rows = [];
 				foreach ( $result as $row ) {
-					$subjects[] = new SubjectLabelLookupResult(
-						id: $row->get( 'id' ),
-						label: $row->get( 'name' )
-					);
+					$rows[] = [
+						'id' => $row->get( 'id' ),
+						'name' => $row->get( 'name' ),
+						'pageId' => (int)$row->get( 'pageId' ),
+					];
 				}
 
-				return $subjects;
+				return $rows;
 			}
 		);
 	}

src/NeoWikiExtension.php

@@ -997,6 +997,7 @@ public function getSubjectLabelLookup(): SubjectLabelLookup {
 		return new Neo4jSubjectLabelLookup(
 			client: $this->getReadOnlyNeo4jClient(),
 			wikiId: $this->config->wikiId,
+			readAuthorizer: $this->newPageReadAuthorizer( $this->getRequestAuthority() ),
 		);
 	}
 

tests/phpunit/EntryPoints/REST/GetSubjectLabelsApiTest.php

@@ -0,0 +1,35 @@
+<?php
+
+declare( strict_types = 1 );
+
+namespace ProfessionalWiki\NeoWiki\Tests\EntryPoints\REST;
+
+use MediaWiki\Rest\HttpException;
+use MediaWiki\Rest\RequestData;
+use MediaWiki\Tests\Rest\Handler\HandlerTestTrait;
+use ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectLabelsApi;
+use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase;
+
+/**
+ * @covers \ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectLabelsApi
+ * @group Database
+ */
+class GetSubjectLabelsApiTest extends NeoWikiIntegrationTestCase {
+	use HandlerTestTrait;
+
+	public function testRejectsLimitAboveMaximum(): void {
+		// The endpoint runs a per-result read-permission check, so an uncapped limit is an
+		// unbounded-work vector. The cap is enforced before the handler runs any query (#1060).
+		$this->expectException( HttpException::class );
+		$this->expectExceptionCode( 400 );
+
+		$this->executeHandler(
+			new GetSubjectLabelsApi(),
+			new RequestData( [
+				'method' => 'GET',
+				'queryParams' => [ 'schema' => 'Person', 'limit' => '100000' ],
+			] )
+		);
+	}
+
+}

tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jSubjectLabelLookupTest.php

@@ -5,6 +5,7 @@
 namespace ProfessionalWiki\NeoWiki\Tests\GraphDatabasePlugins\Neo4j\Persistence;
 
 use Laudis\Neo4j\Contracts\ClientInterface;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
 use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin;
 use ProfessionalWiki\NeoWiki\Domain\Schema\SchemaName;
@@ -19,6 +20,8 @@
 use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject;
 use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase;
 use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySchemaLookup;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SelectivePageReadAuthorizer;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubPageReadAuthorizer;
 
 /**
  * @covers \ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jSubjectLabelLookup
@@ -142,6 +145,77 @@ public function testExcludesSubjectsWithoutWikiId(): void {
 		);
 	}
 
+	public function testExcludesForeignSubjectsReachableThroughALocalPage(): void {
+		$this->saveSubjects( new SubjectMap(
+			TestSubject::build( id: self::SUBJECT_ID_1, label: new SubjectLabel( 'Apple Pie' ) ),
+		) );
+
+		// A cross-wiki-shared Subject id (ADR 22) can leave a local Page holding a Subject stamped
+		// for another wiki. The Subject filter must still withhold it so its foreign label, and a
+		// page id that resolves against the wrong wiki, cannot surface.
+		$this->getClient()->run(
+			'CREATE (:Page { id: 500, wiki_id: $wikiId })-[:HasSubject { isMain: false }]->'
+				. '(:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID )
+				. ' { id: "sTestSLL4444441", name: "Apple Tart", wiki_id: $otherWikiId })',
+			[ 'wikiId' => $this->currentWikiId(), 'otherWikiId' => $this->currentWikiId() . '-other' ]
+		);
+
+		$this->assertEquals(
+			[ new SubjectLabelLookupResult( self::SUBJECT_ID_1, 'Apple Pie' ) ],
+			$this->getSubjectLabelsMatching( 'Apple' )
+		);
+	}
+
+	public function testReturnsALocallyOwnedSubjectOnceWhenAForeignPageAlsoReferencesIt(): void {
+		$this->saveSubjectOnPage( pageId: 7, subjectId: self::SUBJECT_ID_1, label: 'Apple Pie' );
+
+		// In a shared graph the same Subject node (ADR 22 keeps it by bare id) can be referenced by
+		// a foreign wiki's Page too. Only the local owning Page is followed, so the label appears
+		// once and the page id used for the read check resolves within this wiki, not the foreign
+		// one whose colliding page id would gate an unrelated local page.
+		$this->getClient()->run(
+			'MATCH (subject:Subject { id: $subjectId }) '
+				. 'CREATE (:Page { id: 7, wiki_id: $otherWikiId })-[:HasSubject { isMain: false }]->(subject)',
+			[ 'subjectId' => self::SUBJECT_ID_1, 'otherWikiId' => $this->currentWikiId() . '-other' ]
+		);
+
+		$this->assertEquals(
+			[ new SubjectLabelLookupResult( self::SUBJECT_ID_1, 'Apple Pie' ) ],
+			$this->getSubjectLabelsMatching( 'Apple' )
+		);
+	}
+
+	public function testSubjectsOnUnreadablePagesAreOmitted(): void {
+		$this->saveSubjects( new SubjectMap(
+			TestSubject::build( id: self::SUBJECT_ID_1, label: new SubjectLabel( 'Apple Pie' ) ),
+		) );
+
+		$this->assertSame(
+			[],
+			$this->newLookup( readAuthorizer: new StubPageReadAuthorizer( allowed: false ) )
+				->getSubjectLabelsMatching( 'Apple', 10, TestSubject::DEFAULT_SCHEMA_ID )
+		);
+	}
+
+	public function testOverFetchesPastUnreadableRowsToFillTheLimit(): void {
+		$this->saveSubjectOnPage( pageId: 1, subjectId: 'sTestSLL1111141', label: 'Apple 1' );
+		$this->saveSubjectOnPage( pageId: 2, subjectId: 'sTestSLL1111142', label: 'Apple 2' );
+		$this->saveSubjectOnPage( pageId: 3, subjectId: 'sTestSLL1111143', label: 'Apple 3' );
+
+		// Page 2 is hidden and sorts between the two readable rows, so a naive "LIMIT 2 then
+		// filter" would return only Apple 1. Over-fetching then re-limiting must still yield two.
+		$results = $this->newLookup( readAuthorizer: new SelectivePageReadAuthorizer( deniedPageIds: [ 2 ] ) )
+			->getSubjectLabelsMatching( 'Apple', 2, TestSubject::DEFAULT_SCHEMA_ID );
+
+		$this->assertEquals(
+			[
+				new SubjectLabelLookupResult( 'sTestSLL1111141', 'Apple 1' ),
+				new SubjectLabelLookupResult( 'sTestSLL1111143', 'Apple 3' ),
+			],
+			$results
+		);
+	}
+
 	private function saveSubjects( SubjectMap $subjects ): void {
 		$this->newProjectionStore()->savePage( TestPage::build(
 			id: 1,
@@ -150,6 +224,16 @@ private function saveSubjects( SubjectMap $subjects ): void {
 		) );
 	}
 
+	private function saveSubjectOnPage( int $pageId, string $subjectId, string $label ): void {
+		$this->newProjectionStore()->savePage( TestPage::build(
+			id: $pageId,
+			properties: TestPageProperties::build( title: 'Page ' . $pageId ),
+			childSubjects: new SubjectMap(
+				TestSubject::build( id: $subjectId, label: new SubjectLabel( $label ) )
+			)
+		) );
+	}
+
 	protected function newProjectionStore(): GraphDatabasePlugin {
 		return NeoWikiExtension::getInstance()->newNeo4jProjectionStore(
 			new InMemorySchemaLookup(
@@ -165,10 +249,14 @@ private function getSubjectLabelsMatching( string $search, int $limit = 10 ): ar
 		return $this->newLookup()->getSubjectLabelsMatching( $search, $limit, TestSubject::DEFAULT_SCHEMA_ID );
 	}
 
-	private function newLookup( ClientInterface $client = null ): Neo4jSubjectLabelLookup {
+	private function newLookup(
+		ClientInterface $client = null,
+		?PageReadAuthorizer $readAuthorizer = null
+	): Neo4jSubjectLabelLookup {
 		return new Neo4jSubjectLabelLookup(
 			client: $client ?? $this->getClient(),
 			wikiId: $this->currentWikiId(),
+			readAuthorizer: $readAuthorizer ?? new StubPageReadAuthorizer( allowed: true ),
 		);
 	}
 
@@ -179,17 +267,24 @@ private function currentWikiId(): string {
 	/**
 	 * Creates a Subject node directly in the graph, bypassing the projection store, so a test can
 	 * plant a node with a chosen wiki_id (or none at all, as written before wiki_id stamping existed).
+	 *
+	 * The node is attached to a Page carrying the same wiki_id via a HasSubject edge, so it is
+	 * reachable by the lookup's Page->Subject traversal. Without the Page it would be dropped for
+	 * having no Page at all, which would make the wiki filter these tests target untested.
 	 */
 	private function createSubjectNode( string $id, string $name, ?string $wikiId ): void {
-		$properties = [ 'id' => $id, 'name' => $name ];
+		$subjectProperties = [ 'id' => $id, 'name' => $name ];
+		$pageProperties = [ 'id' => 0 ];
 
 		if ( $wikiId !== null ) {
-			$properties['wiki_id'] = $wikiId;
+			$subjectProperties['wiki_id'] = $wikiId;
+			$pageProperties['wiki_id'] = $wikiId;
 		}
 
 		$this->getClient()->run(
-			'CREATE (n:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID ) . ' $properties)',
-			[ 'properties' => $properties ]
+			'CREATE (:Page $pageProperties)-[:HasSubject { isMain: false }]->'
+				. '(:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID ) . ' $subjectProperties)',
+			[ 'pageProperties' => $pageProperties, 'subjectProperties' => $subjectProperties ]
 		);
 	}
 

tests/phpunit/TestDoubles/SelectivePageReadAuthorizer.php

@@ -0,0 +1,34 @@
+<?php
+
+declare( strict_types=1 );
+
+namespace ProfessionalWiki\NeoWiki\Tests\TestDoubles;
+
+use MediaWiki\Title\Title;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
+use ProfessionalWiki\NeoWiki\Domain\Page\PageId;
+
+/**
+ * Authorizes every page read except for a fixed set of denied page ids. Lets a list-filter test
+ * hide specific pages without touching the MediaWiki database, so the caller's own filtering and
+ * over-fetch logic can be exercised in isolation.
+ */
+class SelectivePageReadAuthorizer implements PageReadAuthorizer {
+
+	/**
+	 * @param int[] $deniedPageIds
+	 */
+	public function __construct(
+		private array $deniedPageIds
+	) {
+	}
+
+	public function authorizeReadByPageId( PageId $pageId ): bool {
+		return !in_array( $pageId->id, $this->deniedPageIds, true );
+	}
+
+	public function authorizeReadByPageTitle( Title $title ): bool {
+		return !in_array( $title->getId(), $this->deniedPageIds, true );
+	}
+
+}