From 06cac2b555ebe5a9e8ac9108be18196267e5ba80 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 9 Oct 2025 17:25:16 +0200 Subject: [PATCH 1/2] Track when maintainer is added or removed --- src/Audit/AuditRecordType.php | 4 ++-- src/Controller/PackageController.php | 29 +++++++++++++++------------- src/Entity/AuditRecord.php | 12 ++++++++++++ 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/Audit/AuditRecordType.php b/src/Audit/AuditRecordType.php index c2dcfcf78..0f3760555 100644 --- a/src/Audit/AuditRecordType.php +++ b/src/Audit/AuditRecordType.php @@ -15,8 +15,8 @@ enum AuditRecordType: string { // package ownership - case MaintainerAdded = 'maintainer_added'; // TODO - case MaintainerRemoved = 'maintainer_removed'; // TODO + case MaintainerAdded = 'maintainer_added'; + case MaintainerRemoved = 'maintainer_removed'; case PackageTransferred = 'package_transferred'; // TODO // package management diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 4777663fb..e870a9158 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -12,6 +12,7 @@ namespace App\Controller; +use App\Entity\AuditRecord; use App\Entity\Dependent; use App\Entity\Download; use App\Entity\Job; @@ -904,7 +905,7 @@ public function deletePackageAction(Request $req, string $name): Response } #[Route(path: '/packages/{name:package}/maintainers/', name: 'add_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])] - public function createMaintainerAction(Request $req, #[MapEntity] Package $package, LoggerInterface $logger): RedirectResponse + public function createMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): RedirectResponse { $this->denyAccessUnlessGranted(PackageActions::AddMaintainer->value, $package); @@ -914,19 +915,20 @@ public function createMaintainerAction(Request $req, #[MapEntity] Package $packa try { $em = $this->getEM(); if ($username = $form->getData()->getUser()) { - $user = $em->getRepository(User::class)->findOneByUsernameOrEmail($username); + $maintainer = $em->getRepository(User::class)->findOneByUsernameOrEmail($username); } - if (!empty($user)) { - if (!$package->isMaintainer($user)) { - $package->addMaintainer($user); - $this->packageManager->notifyNewMaintainer($user, $package); + if (!empty($maintainer)) { + if (!$package->isMaintainer($maintainer)) { + $package->addMaintainer($maintainer); + $em->persist(AuditRecord::maintainerAdded($package, $maintainer, $user)); + $this->packageManager->notifyNewMaintainer($maintainer, $package); } $em->persist($package); $em->flush(); - $this->addFlash('success', $user->getUsername().' is now a '.$package->getName().' maintainer.'); + $this->addFlash('success', $maintainer->getUsername().' is now a '.$package->getName().' maintainer.'); return $this->redirectToRoute('view_package', ['name' => $package->getName()]); } @@ -941,7 +943,7 @@ public function createMaintainerAction(Request $req, #[MapEntity] Package $packa } #[Route(path: '/packages/{name:package}/maintainers/delete', name: 'remove_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])] - public function removeMaintainerAction(Request $req, #[MapEntity] Package $package, LoggerInterface $logger): Response + public function removeMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): Response { $this->denyAccessUnlessGranted(PackageActions::RemoveMaintainer->value, $package); @@ -951,18 +953,19 @@ public function removeMaintainerAction(Request $req, #[MapEntity] Package $packa try { $em = $this->getEM(); if ($username = $removeMaintainerForm->getData()->getUser()) { - $user = $em->getRepository(User::class)->findOneByUsernameOrEmail($username); + $maintainer = $em->getRepository(User::class)->findOneByUsernameOrEmail($username); } - if (!empty($user)) { - if ($package->isMaintainer($user)) { - $package->getMaintainers()->removeElement($user); + if (!empty($maintainer)) { + if ($package->isMaintainer($maintainer)) { + $package->getMaintainers()->removeElement($maintainer); + $em->persist(AuditRecord::maintainerRemoved($package, $maintainer, $user)); } $em->persist($package); $em->flush(); - $this->addFlash('success', $user->getUsername().' is no longer a '.$package->getName().' maintainer.'); + $this->addFlash('success', $maintainer->getUsername().' is no longer a '.$package->getName().' maintainer.'); return $this->redirectToRoute('view_package', ['name' => $package->getName()]); } diff --git a/src/Entity/AuditRecord.php b/src/Entity/AuditRecord.php index 4f0bd5f01..eeab8be55 100644 --- a/src/Entity/AuditRecord.php +++ b/src/Entity/AuditRecord.php @@ -45,6 +45,8 @@ private function __construct( public readonly ?string $vendor = null, #[ORM\Column(nullable: true)] public readonly ?int $packageId = null, + #[ORM\Column(nullable: true)] + public readonly ?int $userId = null, ) { $this->id = new Ulid(); $this->datetime = new \DateTimeImmutable(); @@ -84,6 +86,16 @@ public static function versionReferenceChange(Version $version, ?string $oldSour ); } + public static function maintainerAdded(Package $package, User $maintainer, ?User $actor): self + { + return new self(AuditRecordType::MaintainerAdded, ['name' => $package->getName(), 'maintainer' => self::getUserData($maintainer), 'actor' => self::getUserData($actor)], $actor?->getId(), $package->getVendor(), $package->getId(), $maintainer->getId()); + } + + public static function maintainerRemoved(Package $package, User $maintainer, ?User $actor): self + { + return new self(AuditRecordType::MaintainerRemoved, ['name' => $package->getName(), 'maintainer' => self::getUserData($maintainer), 'actor' => self::getUserData($actor)], $actor?->getId(), $package->getVendor(), $package->getId(), $maintainer->getId()); + } + /** * @return array{id: int, username: string}|string */ From a93c9d426bcd3fad0d44f0745facc6c09fd66605 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 23 Oct 2025 17:21:49 +0200 Subject: [PATCH 2/2] Add missing controller tests for adding and removing maintainer --- tests/Controller/PackageControllerTest.php | 92 ++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/Controller/PackageControllerTest.php b/tests/Controller/PackageControllerTest.php index 64822618a..6c5214185 100644 --- a/tests/Controller/PackageControllerTest.php +++ b/tests/Controller/PackageControllerTest.php @@ -12,6 +12,9 @@ namespace App\Tests\Controller; +use App\Audit\AuditRecordType; +use App\Entity\Package; +use App\Entity\User; use App\Tests\IntegrationTestCase; class PackageControllerTest extends IntegrationTestCase @@ -52,4 +55,93 @@ public function testEdit(): void self::assertResponseIsSuccessful(); self::assertSame('github.com/composer/composer', $crawler->filter('.canonical')->text()); } + + public function testCreateMaintainer(): void + { + $owner = self::createUser('owner', 'owner@example.org'); + $newMaintainer = self::createUser('maintainer', 'maintainer@example.org'); + $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$owner]); + + $this->store($owner, $newMaintainer, $package); + + $this->client->loginUser($owner); + + $this->assertFalse($package->isMaintainer($newMaintainer)); + + $crawler = $this->client->request('GET', '/packages/test/pkg'); + + $form = $crawler->filter('[name="add_maintainer_form"]')->form(); + $form->setValues([ + 'add_maintainer_form[user]' => 'maintainer', + ]); + + $this->client->enableProfiler(); // This is required in 7.3.4 to assert emails were sent, see https://github.com/symfony/symfony/issues/61873 + $this->client->submit($form); + + $this->assertEmailCount(1); + $email = $this->getMailerMessage(); + $this->assertNotNull($email); + $this->assertEmailHeaderSame($email, 'To', $newMaintainer->getEmail()); + + $this->assertResponseRedirects('/packages/test/pkg'); + $this->client->followRedirect(); + $this->assertResponseIsSuccessful(); + + $em = self::getEM(); + $em->clear(); + + $maintainer = $em->getRepository(User::class)->find($newMaintainer->getId()); + $package = $em->getRepository(Package::class)->find($package->getId()); + + $this->assertTrue($package->isMaintainer($maintainer)); + + $auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([ + 'type' => AuditRecordType::MaintainerAdded->value, + 'packageId' => $package->getId(), + 'actorId' => $owner->getId(), + ]); + $this->assertNotNull($auditRecord); + } + + public function testRemoveMaintainer(): void + { + $owner = self::createUser('owner', 'owner@example.org'); + $maintainer = self::createUser('maintainer', 'maintainer@example.org'); + $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$owner, $maintainer]); + + $this->store($owner, $maintainer, $package); + + $this->client->loginUser($owner); + + $this->assertTrue($package->isMaintainer($maintainer)); + + $crawler = $this->client->request('GET', '/packages/test/pkg'); + + $form = $crawler->filter('[name="remove_maintainer_form"]')->form(); + $form->setValues([ + 'remove_maintainer_form[user]' => $maintainer->getId(), + ]); + + $this->client->submit($form); + + $this->assertResponseRedirects('/packages/test/pkg'); + $this->client->followRedirect(); + $this->assertResponseIsSuccessful(); + + $em = self::getEM(); + $em->clear(); + + $maintainer = $em->getRepository(User::class)->find($maintainer->getId()); + $package = $em->getRepository(Package::class)->find($package->getId()); + + $this->assertFalse($package->isMaintainer($maintainer)); + + $auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([ + 'type' => AuditRecordType::MaintainerRemoved->value, + 'packageId' => $package->getId(), + 'actorId' => $owner->getId(), + ]); + + $this->assertNotNull($auditRecord); + } }