From 17b951c7e13e848efecc93076a2f93999599afeb Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 8 Sep 2026 20:09:59 -0400 Subject: [PATCH] Restore the documented status-code contract of DELETE /subject/{id} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/ProfessionalWiki/NeoWiki/issues/1312 `DELETE /neowiki/v0/subject/{subjectId}` broke the status-code contract that [`docs/api/rest-api.md`](https://github.com/ProfessionalWiki/NeoWiki/blob/master/docs/api/rest-api.md) documents and [ADR 027](https://github.com/ProfessionalWiki/NeoWiki/blob/master/docs/adr/027-access-control.md) governs, in four ways. ## The read gate The action resolved the Subject to its page and authorized the write against that page, but never checked the caller may read it — unlike `ReplaceSubjectAction` and `UpdateStatementAction`. Two consequences: a Subject on an unreadable page answered `403` where the contract promises `404`, so `403` versus `404` told a caller which Subject ids exist; and a caller holding `edit` but not `read` could delete Subjects there. The action now takes a `PageReadAuthorizer` and answers the not-found path before reaching the write check — the shape `UpdateStatementAction::getPageOfSubjectToEdit()` already uses. A page the caller can read still answers `403` when they cannot edit it. ## Three further breaches of the same contract **A malformed id answered `500`.** `new SubjectId( 'notavalidid' )` throws `InvalidArgumentException`, a `LogicException`, which no catch arm matched. It now answers `400`, as every sibling Subject-keyed handler does. **Internal failures answered `403` carrying their own message.** The handler caught every `RuntimeException` and reported it as a permission denial, so a `DBError` raised inside the authorizers surfaced as `403 {"message": ""}` — and the read gate added here runs the `getUserPermissionsErrors` hook, which is arbitrary ACL-extension code. The delete denial now raises the typed `SubjectEditNotAuthorizedException`, keeping its own message, and an internal error is a `500`. **A delete that removed nothing answered `200`.** `SubjectRepository::deleteSubject()` returned `void` and the implementation discarded the write's outcome. It now returns `PageContentSavingStatus`, as `savePageSubjects()` already does, and the action answers not-found unless a revision was created. The repository asks the slot whether it still holds the Subject rather than inferring removal from the save: `mutatePageSubjects` re-serializes the slot whatever the mutation did, so a slot written by anything other than the serializer — an import, or `Special:NeoJson` — yields new bytes and a real revision even when nothing was removed. ## Open for review A write the wiki refuses — read-only mode, an edit-filter abort — now answers `404 Subject not found` while the Subject still exists. `CreateSubjectAction`, `SetMainSubjectAction`, `SetSubjectsOrderingAction` and `MoveSubjectAction` all map `ERROR` to a not-found presentation, so the shape is the established one, and it replaces a false `200`. Giving `ERROR` its own answer is the alternative, and the one that would keep the documented `404` causes exact. Left as it is here because the choice belongs to all five endpoints at once, not to this one. ## Considered, omitted Sharing the resolve-then-read-gate half with `Application\Rdf\SubjectHostingPageResolver`. Adopting it here alone would leave the three Subject-id-keyed write actions in three different shapes, and moving it out of the `Rdf` namespace reaches past this fix. Worth doing across all of them at once. ## Not fixed here [#1383](https://github.com/ProfessionalWiki/NeoWiki/issues/1383) — `Special:NeoJson` reads and writes Subject content with no read gate at all, the same threat model on a path that is not a REST route. It is unregistered unless `$wgNeoWikiEnableDevelopmentUI` is set, which defaults to false. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AfaXavUCWB237b7NmErXjg --- .../DeleteSubject/DeleteSubjectAction.php | 24 +++- src/Application/SubjectRepository.php | 6 +- src/EntryPoints/REST/DeleteSubjectApi.php | 10 +- src/NeoWikiExtension.php | 1 + .../Subject/MediaWikiSubjectRepository.php | 16 ++- .../Actions/DeleteSubjectActionTest.php | 109 +++++++++++++++++- .../EntryPoints/REST/DeleteSubjectApiTest.php | 75 +++++++++--- .../MediaWikiSubjectRepositoryTest.php | 67 +++++++++-- .../TestDoubles/InMemorySubjectRepository.php | 17 ++- .../TestDoubles/SpyPageReadAuthorizer.php | 34 ++++++ 10 files changed, 321 insertions(+), 38 deletions(-) create mode 100644 tests/phpunit/TestDoubles/SpyPageReadAuthorizer.php diff --git a/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php b/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php index fb3102171..13a5c19c4 100644 --- a/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php +++ b/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php @@ -5,16 +5,19 @@ namespace ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject; use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup; +use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer; use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer; use ProfessionalWiki\NeoWiki\Application\SubjectRepository; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException; use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; -use RuntimeException; +use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentSavingStatus; readonly class DeleteSubjectAction { public function __construct( private SubjectRepository $subjectRepository, + private PageReadAuthorizer $readAuthorizer, private SubjectWriteAuthorizer $writeAuthorizer, private PageIdentifiersLookup $pageIdentifiersLookup ) { @@ -23,17 +26,26 @@ public function __construct( public function deleteSubject( SubjectId $subjectId, ?string $comment ): void { $pageId = $this->pageIdentifiersLookup->getPageIdOfSubject( $subjectId )?->getId(); - // A Subject on no page has no page rights to check, so it is answered as absent rather than as - // forbidden. - if ( $pageId === null ) { + // Gate on read before write: a Subject on a page the caller may not read answers exactly like + // one that does not exist, and so does a Subject on no page, which has no page rights to + // check. Reaching the write check first would answer 403 where a restricted page answers + // 404. See PageReadAuthorizer for why a denied read takes the not-found shape. + if ( $pageId === null || !$this->readAuthorizer->authorizeReadByPageId( $pageId ) ) { throw SubjectNotFoundException::forId( $subjectId ); } if ( !$this->writeAuthorizer->authorize( $pageId ) ) { - throw new RuntimeException( 'You do not have the necessary permissions to delete this subject' ); + throw new SubjectEditNotAuthorizedException( 'You do not have the necessary permissions to delete this subject' ); } - $this->subjectRepository->deleteSubject( $subjectId, $comment ); + $status = $this->subjectRepository->deleteSubject( $subjectId, $comment ); + + // Only a new revision means the Subject was removed. The index can name a page whose slot no + // longer holds it, and the page can go away between the checks above and the write; either + // way the Subject the caller named is not there, which is what an absent one answers. + if ( $status->status !== PageContentSavingStatus::REVISION_CREATED ) { + throw SubjectNotFoundException::forId( $subjectId ); + } } } diff --git a/src/Application/SubjectRepository.php b/src/Application/SubjectRepository.php index d61ed278e..4681a3f85 100644 --- a/src/Application/SubjectRepository.php +++ b/src/Application/SubjectRepository.php @@ -21,8 +21,12 @@ public function updateSubject( Subject $subject, ?string $comment = null ): void /** * TODO: document exceptions + * + * Answers REVISION_CREATED only when the Subject was actually removed. The index can name a page + * whose slot no longer holds it, and the page can go away under the write; both answer + * NO_CHANGES or ERROR rather than reporting a deletion that did not happen. */ - public function deleteSubject( SubjectId $id, ?string $comment ): void; + public function deleteSubject( SubjectId $id, ?string $comment ): PageContentSavingStatus; /** * TODO: document exceptions diff --git a/src/EntryPoints/REST/DeleteSubjectApi.php b/src/EntryPoints/REST/DeleteSubjectApi.php index 3458780cb..1526405b1 100644 --- a/src/EntryPoints/REST/DeleteSubjectApi.php +++ b/src/EntryPoints/REST/DeleteSubjectApi.php @@ -4,14 +4,15 @@ namespace ProfessionalWiki\NeoWiki\EntryPoints\REST; +use InvalidArgumentException; use MediaWiki\Rest\HttpException; use MediaWiki\Rest\Response; use MediaWiki\Rest\SimpleHandler; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException; use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Presentation\CsrfValidator; -use RuntimeException; use Wikimedia\ParamValidator\ParamValidator; class DeleteSubjectApi extends SimpleHandler { @@ -36,12 +37,17 @@ public function run( string $subjectId ): Response { new SubjectId( $subjectId ), $comment ); + } catch ( InvalidArgumentException $e ) { + return $this->getResponseFactory()->createHttpError( 400, [ + 'status' => 'error', + 'message' => $e->getMessage(), + ] ); } catch ( SubjectNotFoundException $e ) { return $this->getResponseFactory()->createHttpError( 404, [ 'status' => 'error', 'message' => $e->getMessage(), ] ); - } catch ( RuntimeException $e ) { + } catch ( SubjectEditNotAuthorizedException $e ) { return $this->getResponseFactory()->createHttpError( 403, [ 'status' => 'error', 'message' => $e->getMessage(), diff --git a/src/NeoWikiExtension.php b/src/NeoWikiExtension.php index 464f1f153..6df2a7c3b 100644 --- a/src/NeoWikiExtension.php +++ b/src/NeoWikiExtension.php @@ -1242,6 +1242,7 @@ private function getPageIdentifiersLookup(): PageIdentifiersLookup { public function newDeleteSubjectAction( Authority $authority ): DeleteSubjectAction { return new DeleteSubjectAction( subjectRepository: $this->getSubjectRepository(), + readAuthorizer: $this->newPageReadAuthorizer( $authority ), writeAuthorizer: $this->newSubjectWriteAuthorizer( $authority ), pageIdentifiersLookup: $this->getPageIdentifiersLookup() ); diff --git a/src/Persistence/MediaWiki/Subject/MediaWikiSubjectRepository.php b/src/Persistence/MediaWiki/Subject/MediaWikiSubjectRepository.php index 2745092ca..67462d9e9 100644 --- a/src/Persistence/MediaWiki/Subject/MediaWikiSubjectRepository.php +++ b/src/Persistence/MediaWiki/Subject/MediaWikiSubjectRepository.php @@ -105,24 +105,32 @@ private function saveContent( SubjectContent $content, PageId $pageId, ?string $ ); } - public function deleteSubject( SubjectId $id, ?string $comment ): void { + public function deleteSubject( SubjectId $id, ?string $comment ): PageContentSavingStatus { $pageId = $this->getPageIdForSubject( $id ); if ( $pageId === null ) { - return; + return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES ); } $content = $this->getContentByPageId( $pageId ); if ( $content === null ) { - return; + return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES ); + } + + // Asked of the slot rather than inferred from the save: mutatePageSubjects re-serializes the + // slot whatever the mutation did, so a slot written by anything other than the serializer - + // an import, or Special:NeoJson - yields new bytes and a real revision even when the removal + // removed nothing. + if ( $content->getPageSubjects()->getAllSubjects()->getSubject( $id ) === null ) { + return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES ); } $content->mutatePageSubjects( function( PageSubjects $pageSubjects ) use ( $id ): void { $pageSubjects->removeSubject( $id ); } ); - $this->saveContent( $content, $pageId, $comment ); + return $this->saveContent( $content, $pageId, $comment ); } public function getMainSubject( PageId $pageId ): ?Subject { diff --git a/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php b/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php index 307c372e9..969359816 100644 --- a/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php +++ b/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php @@ -7,8 +7,10 @@ use PHPUnit\Framework\TestCase; use ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject\DeleteSubjectAction; use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup; +use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer; use ProfessionalWiki\NeoWiki\Application\SubjectRepository; use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException; use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; use ProfessionalWiki\NeoWiki\Domain\Page\PageIdentifiers; @@ -16,7 +18,9 @@ use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemoryPageIdentifiersLookup; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectRepository; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyPageReadAuthorizer; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectWriteAuthorizer; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubPageReadAuthorizer; /** * @covers \ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject\DeleteSubjectAction @@ -57,6 +61,21 @@ public function testAuthorizesAgainstTheSubjectsResolvedPage(): void { $this->assertEquals( new PageId( 7 ), $authorizer->authorizedPageId ); } + public function testGatesTheReadOnTheSubjectsResolvedPage(): void { + $readAuthorizer = new SpyPageReadAuthorizer( allowed: true ); + + $this->newAction( + $this->newRepositoryWithSubject(), + new SpySubjectWriteAuthorizer( allowed: true ), + new InMemoryPageIdentifiersLookup( [ + [ new SubjectId( self::SUBJECT_ID ), new PageIdentifiers( new PageId( 7 ), 'Owning page', 0 ) ] + ] ), + readAuthorizer: $readAuthorizer + )->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + + $this->assertEquals( new PageId( 7 ), $readAuthorizer->authorizedPageId ); + } + public function testThrowsWhenUserMayNotDeleteSubject(): void { $action = $this->newAction( new InMemorySubjectRepository(), @@ -64,7 +83,7 @@ public function testThrowsWhenUserMayNotDeleteSubject(): void { $this->pageIdentifiersLookupWithSubject() ); - $this->expectException( \RuntimeException::class ); + $this->expectException( SubjectEditNotAuthorizedException::class ); $this->expectExceptionMessage( 'You do not have the necessary permissions to delete this subject' ); $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); @@ -86,6 +105,73 @@ public function testUnresolvableSubjectIsReportedAsNotFound(): void { $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); } + public function testUnreadablePageAnswersNotFound(): void { + $action = $this->newActionOnUnreadablePage( $this->newRepositoryWithSubject() ); + + $this->expectException( SubjectNotFoundException::class ); + // Anchored: a read denial that added anything of its own would tell the two answers apart. + $this->expectExceptionMessageMatches( '/^Subject not found: ' . self::SUBJECT_ID . '$/' ); + + $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } + + public function testUnreadablePageIsRejectedBeforeTheDeletion(): void { + $repository = $this->newRepositoryWithSubject(); + + try { + $this->newActionOnUnreadablePage( $repository ) + ->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } catch ( SubjectNotFoundException ) { + } + + $this->assertNotNull( $repository->getSubject( new SubjectId( self::SUBJECT_ID ) ) ); + } + + public function testReadDenialTakesPrecedenceOverWriteDenial(): void { + // A page the caller can neither read nor edit answers not-found, never the write 403, so a + // hidden page is indistinguishable from an absent one. + $action = $this->newAction( + $this->newRepositoryWithSubject(), + new SpySubjectWriteAuthorizer( allowed: false ), + $this->pageIdentifiersLookupWithSubject(), + readAuthorizer: new StubPageReadAuthorizer( allowed: false ) + ); + + $this->expectException( SubjectNotFoundException::class ); + + $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } + + public function testWriteThatRemovedNothingAnswersNotFound(): void { + // Reporting success would tell the caller a Subject that is not there was deleted. Which of + // the repository's reasons produced it is its own business, and pinned in its tests. + $action = $this->newAction( + new InMemorySubjectRepository(), + new SpySubjectWriteAuthorizer( allowed: true ), + $this->pageIdentifiersLookupWithSubject() + ); + + $this->expectException( SubjectNotFoundException::class ); + + $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } + + public function testPageLostUnderTheWriteAnswersNotFound(): void { + // The page passed both checks and then went away before the save landed. + $repository = $this->newRepositoryWithSubject(); + $repository->failNextSave = true; + + $action = $this->newAction( + $repository, + new SpySubjectWriteAuthorizer( allowed: true ), + $this->pageIdentifiersLookupWithSubject() + ); + + $this->expectException( SubjectNotFoundException::class ); + + $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } + private function newRepositoryWithSubject(): InMemorySubjectRepository { $repository = new InMemorySubjectRepository(); $repository->updateSubject( TestSubject::build( id: self::SUBJECT_ID ) ); @@ -104,12 +190,31 @@ private function newAllowingAction( SubjectRepository $repository ): DeleteSubje ); } + /** + * The caller may edit the Subject's page but may not read it: the case a missing read gate would + * let through. + */ + private function newActionOnUnreadablePage( SubjectRepository $repository ): DeleteSubjectAction { + return $this->newAction( + $repository, + new SpySubjectWriteAuthorizer( allowed: true ), + $this->pageIdentifiersLookupWithSubject(), + readAuthorizer: new StubPageReadAuthorizer( allowed: false ) + ); + } + private function newAction( SubjectRepository $repository, SubjectWriteAuthorizer $authorizer, PageIdentifiersLookup $pageIdentifiersLookup, + ?PageReadAuthorizer $readAuthorizer = null, ): DeleteSubjectAction { - return new DeleteSubjectAction( $repository, $authorizer, $pageIdentifiersLookup ); + return new DeleteSubjectAction( + subjectRepository: $repository, + readAuthorizer: $readAuthorizer ?? new StubPageReadAuthorizer( allowed: true ), + writeAuthorizer: $authorizer, + pageIdentifiersLookup: $pageIdentifiersLookup + ); } private function pageIdentifiersLookupWithSubject(): InMemoryPageIdentifiersLookup { diff --git a/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php b/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php index ab6f75005..068df9ab3 100644 --- a/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php +++ b/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php @@ -5,13 +5,14 @@ namespace ProfessionalWiki\NeoWiki\Tests\EntryPoints\REST; use MediaWiki\Rest\RequestData; +use MediaWiki\Rest\ResponseInterface; use MediaWiki\Tests\Rest\Handler\HandlerTestTrait; -use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectLabel; use ProfessionalWiki\NeoWiki\EntryPoints\REST\DeleteSubjectApi; use ProfessionalWiki\NeoWiki\Presentation\CsrfValidator; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; +use ProfessionalWiki\NeoWiki\Tests\NeoWikiMockAuthorityTrait; /** * @covers \ProfessionalWiki\NeoWiki\EntryPoints\REST\DeleteSubjectApi @@ -20,7 +21,9 @@ */ class DeleteSubjectApiTest extends NeoWikiIntegrationTestCase { use HandlerTestTrait; - use MockAuthorityTrait; + use NeoWikiMockAuthorityTrait; + + private const string SUBJECT_ID = 'sTestDSA1111111'; public function testDeletesSubjectWithoutBody(): void { $this->createPages(); @@ -43,14 +46,14 @@ private function newDeleteSubjectApi(): DeleteSubjectApi { } private function createValidRequestData(): RequestData { - return $this->createRequestData( [] ); + return $this->createRequestDataFor( self::SUBJECT_ID ); } - private function createRequestData( array $body ): RequestData { + private function createRequestDataFor( string $subjectId, array $body = [] ): RequestData { return new RequestData( [ 'method' => 'DELETE', 'pathParams' => [ - 'subjectId' => 'sTestDSA1111111' + 'subjectId' => $subjectId ], 'bodyContents' => json_encode( $body ), 'headers' => [ @@ -63,8 +66,8 @@ private function createPages(): void { $this->createPageWithSubjects( 'DeleteSubjectApiTest', mainSubject: TestSubject::build( - id: 'sTestDSA1111111', - label: new SubjectLabel( 'Test subject sTestDSA1111111' ), + id: self::SUBJECT_ID, + label: new SubjectLabel( 'Test subject ' . self::SUBJECT_ID ), ) ); } @@ -74,16 +77,15 @@ public function testDeleteWithComment(): void { $response = $this->executeHandler( $this->newDeleteSubjectApi(), - $this->createRequestData( [ 'comment' => 'Test edit summary' ] ) + $this->createRequestDataFor( self::SUBJECT_ID, [ 'comment' => 'Test edit summary' ] ) ); $this->assertSame( 200, $response->getStatusCode() ); } /** - * A Subject the index does not resolve is answered as absent. Pinned at this layer because - * SubjectNotFoundException is a RuntimeException: catching the two in the other order would answer - * the 403 below instead, which no action-level test can see. + * A Subject the index does not resolve is answered as absent. Pinned at this layer because only + * the handler turns the exception into a status code. */ public function testUnresolvableSubjectIsNotFound(): void { $response = $this->executeHandler( @@ -94,13 +96,13 @@ public function testUnresolvableSubjectIsNotFound(): void { $this->assertSame( 404, $response->getStatusCode() ); } - public function testPermissionDenied(): void { + public function testReadableButNotEditablePageReturns403(): void { $this->createPages(); $response = $this->executeHandler( $this->newDeleteSubjectApi(), $this->createValidRequestData(), - authority: $this->mockAnonAuthorityWithPermissions( [] ) + authority: $this->authorityWithGlobalEditButNoPageEdit() ); $responseData = json_decode( $response->getBody()->getContents(), true ); @@ -110,4 +112,51 @@ public function testPermissionDenied(): void { $this->assertSame( 'You do not have the necessary permissions to delete this subject', $responseData['message'] ); } + public function testMalformedSubjectIdIsRejected(): void { + // SubjectId rejects the format with an InvalidArgumentException, which is a LogicException: + // without its own catch arm it escapes run() and answers 500 rather than 400. + $response = $this->executeHandler( + $this->newDeleteSubjectApi(), + $this->createRequestDataFor( 'notavalidid' ) + ); + + $this->assertSame( 400, $response->getStatusCode() ); + } + + public function testSubjectOnAnUnreadablePageAnswersLikeAnAbsentSubject(): void { + $this->createPages(); + + // One Authority for both requests: comparing responses obtained under two different + // Authorities says nothing about what any single caller can tell apart. + $authority = $this->authorityWithGlobalReadButNoPageRead(); + + $unreadable = $this->executeHandler( + $this->newDeleteSubjectApi(), + $this->createValidRequestData(), + authority: $authority + ); + + $absent = $this->executeHandler( + $this->newDeleteSubjectApi(), + $this->createRequestDataFor( 'sDoesNotExist99' ), + authority: $authority + ); + + // A caller holding a harvested Subject id learns nothing about whether it exists. + $this->assertSame( 404, $unreadable->getStatusCode() ); + $this->assertSame( 404, $absent->getStatusCode() ); + $this->assertSame( + $this->bodyWithIdMasked( $absent, 'sDoesNotExist99' ), + $this->bodyWithIdMasked( $unreadable, self::SUBJECT_ID ) + ); + } + + /** + * Masking the echoed id, which is only what the caller supplied, leaves everything that could + * tell the two answers apart. + */ + private function bodyWithIdMasked( ResponseInterface $response, string $subjectId ): array { + return json_decode( str_replace( $subjectId, '', $response->getBody()->getContents() ), true ); + } + } diff --git a/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php b/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php index 9f8b7de58..3c40f6240 100644 --- a/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php +++ b/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php @@ -13,6 +13,7 @@ use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectLabel; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectMap; use ProfessionalWiki\NeoWiki\NeoWikiExtension; +use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentSavingStatus; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; @@ -91,19 +92,69 @@ public function testDeleteSubject(): void { ); } - public function testDeleteSubjectForUnknownSubject(): void { + public function testDeleteSubjectReportsTheRevisionItCreated(): void { $this->createPages(); - $this->newRepository()->deleteSubject( - new SubjectId( 'sTestMSR1111113' ), - null + $status = $this->newRepository()->deleteSubject( new SubjectId( 'sTestMSR1111113' ), null ); + + $this->assertSame( PageContentSavingStatus::REVISION_CREATED, $status->status ); + } + + public function testDeleteSubjectForUnknownSubjectReportsNoChanges(): void { + $this->createPages(); + + $status = $this->newRepository()->deleteSubject( new SubjectId( 'sTestMSR1111119' ), null ); + + $this->assertSame( PageContentSavingStatus::NO_CHANGES, $status->status ); + } + + /** + * The index names a page that carries no Subject slot at all - a page deleted and recreated, or + * one whose latest revision dropped the slot. Nothing is there to remove. + */ + public function testDeleteSubjectOnAPageWithoutSubjectContentReportsNoChanges(): void { + $this->createPages(); + + $pageIdentifiersLookup = new InMemoryPageIdentifiersLookup(); + $pageIdentifiersLookup->addIdentifiers( + new SubjectId( 'sTestMSR1111119' ), + new PageIdentifiers( new PageId( 999999 ), 'NoSuchPage', 0 ) ); - $this->assertNull( - $this->newRepository()->getSubject( - new SubjectId( 'sTestMSR1111113' ) - ) + $repository = new MediaWikiSubjectRepository( + pageIdentifiersLookup: $pageIdentifiersLookup, + revisionLookup: $this->getServiceContainer()->getRevisionLookup(), + pageContentSaver: NeoWikiExtension::getInstance()->getPageContentSaver(), ); + + $status = $repository->deleteSubject( new SubjectId( 'sTestMSR1111119' ), null ); + + $this->assertSame( PageContentSavingStatus::NO_CHANGES, $status->status ); + } + + /** + * The index names a page whose slot does not hold the Subject. The delete then rewrites the slot + * unchanged, and only the saved status says nothing was removed - which is what the API turns + * into its not-found answer. + */ + public function testDeleteSubjectMissingFromTheIndexedPageReportsNoChanges(): void { + $this->createPages(); + + $pageIdentifiersLookup = new InMemoryPageIdentifiersLookup(); + $pageIdentifiersLookup->addIdentifiers( + new SubjectId( 'sTestMSR1111119' ), + new PageIdentifiers( $this->getPageId( 'SubjectRepoTestOne' ), 'SubjectRepoTestOne', 0 ) + ); + + $repository = new MediaWikiSubjectRepository( + pageIdentifiersLookup: $pageIdentifiersLookup, + revisionLookup: $this->getServiceContainer()->getRevisionLookup(), + pageContentSaver: NeoWikiExtension::getInstance()->getPageContentSaver(), + ); + + $status = $repository->deleteSubject( new SubjectId( 'sTestMSR1111119' ), null ); + + $this->assertSame( PageContentSavingStatus::NO_CHANGES, $status->status ); } public function testGetMainSubjectReturnsNullForUnknownPage(): void { diff --git a/tests/phpunit/TestDoubles/InMemorySubjectRepository.php b/tests/phpunit/TestDoubles/InMemorySubjectRepository.php index edcb7de36..8a0fd52f2 100644 --- a/tests/phpunit/TestDoubles/InMemorySubjectRepository.php +++ b/tests/phpunit/TestDoubles/InMemorySubjectRepository.php @@ -90,9 +90,22 @@ public function updateSubject( Subject $subject, ?string $comment = null ): void $this->updateSubjectCallCount++; } - public function deleteSubject( SubjectId $id, ?string $comment ): void { - unset( $this->subjects[$id->text] ); + public function deleteSubject( SubjectId $id, ?string $comment ): PageContentSavingStatus { $this->comments[$id->text] = $comment; + + if ( $this->failNextSave ) { + return new PageContentSavingStatus( PageContentSavingStatus::ERROR, 'Page not found' ); + } + + // Nothing to remove is nothing changed, as it is in production when the page's slot does not + // hold the Subject the index named. + if ( !array_key_exists( $id->text, $this->subjects ) ) { + return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES ); + } + + unset( $this->subjects[$id->text] ); + + return new PageContentSavingStatus( PageContentSavingStatus::REVISION_CREATED ); } /** diff --git a/tests/phpunit/TestDoubles/SpyPageReadAuthorizer.php b/tests/phpunit/TestDoubles/SpyPageReadAuthorizer.php new file mode 100644 index 000000000..79a270182 --- /dev/null +++ b/tests/phpunit/TestDoubles/SpyPageReadAuthorizer.php @@ -0,0 +1,34 @@ +authorizedPageId = $pageId; + + return $this->allowed; + } + + public function authorizeReadByPageTitle( Title $title ): bool { + return $this->allowed; + } + +}