diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 01c58014a..60fed3dab 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -733,10 +733,17 @@ public function viewPackageAction(Request $req, string $name, CsrfTokenManagerIn } } - $data['addMaintainerForm'] = $this->createAddMaintainerForm($package)->createView(); - $data['removeMaintainerForm'] = $this->createRemoveMaintainerForm($package)->createView(); - $data['transferPackageForm'] = $this->createTransferPackageForm($package)->createView(); - $data['deleteForm'] = $this->createDeletePackageForm($package)->createView(); + // The template renders each of these behind the matching voter grant, and building them + // is not free - createView() on the remove-maintainer form materialises an EntityType + // choice list with a DB query - so visitors who cannot see a form must not pay for it. + $data['addMaintainerForm'] = $this->isGranted(PackageActions::AddMaintainer->value, $package) + ? $this->createAddMaintainerForm($package)->createView() : null; + $data['removeMaintainerForm'] = $this->isGranted(PackageActions::RemoveMaintainer->value, $package) + ? $this->createRemoveMaintainerForm($package)->createView() : null; + $data['transferPackageForm'] = $this->isGranted(PackageActions::TransferPackage->value, $package) + ? $this->createTransferPackageForm($package)->createView() : null; + $data['deleteForm'] = $this->isGranted(PackageActions::Delete->value, $package) + ? $this->createDeletePackageForm($package)->createView() : null; } else { $data['hasVersionSecurityAdvisories'] = []; $data['hasVersionsFlaggedAsMalware'] = []; diff --git a/tests/Controller/PackageControllerTest.php b/tests/Controller/PackageControllerTest.php index e13fad0ba..6396761e3 100644 --- a/tests/Controller/PackageControllerTest.php +++ b/tests/Controller/PackageControllerTest.php @@ -49,6 +49,35 @@ public function testView(): void self::assertStringContainsString('noindex', (string) $auditLink->attr('rel')); } + public function testPackagePageOmitsManagementFormsForVisitorsWhoCannotUseThem(): void + { + $owner = self::createUser('owner', 'owner@example.org'); + // a second maintainer is required for remove_maintainer to be granted at all + $comaintainer = self::createUser('comaintainer', 'comaintainer@example.org'); + $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$owner, $comaintainer]); + $this->store($owner, $comaintainer, $package); + + $crawler = $this->client->request('GET', '/packages/test/pkg'); + self::assertResponseIsSuccessful(); + + // The controller skips building these entirely when the visitor lacks the grant, which also + // skips the EntityType choice-list query behind the remove-maintainer form. + self::assertCount(0, $crawler->filter('[name="add_maintainer_form"]')); + self::assertCount(0, $crawler->filter('[name="remove_maintainer_form"]')); + self::assertCount(0, $crawler->filter('[name="transfer_package_form"]')); + self::assertCount(0, $crawler->filter('form.delete.action')); + + // ...and still builds them for someone who can, so the skip is keyed on the grant only. + $this->client->loginUser($owner); + $crawler = $this->client->request('GET', '/packages/test/pkg'); + self::assertResponseIsSuccessful(); + + self::assertCount(1, $crawler->filter('[name="add_maintainer_form"]')); + self::assertCount(1, $crawler->filter('[name="remove_maintainer_form"]')); + self::assertCount(1, $crawler->filter('[name="transfer_package_form"]')); + self::assertCount(1, $crawler->filter('form.delete.action')); + } + public function testPackagePageOnlyCountsViewsWhileTheSpamHeuristicCanUseThem(): void { $fresh = self::createPackage('test/fresh', 'https://example.com/test/fresh');