Audit log: track maintainer added/removed

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';
 
     // package management

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()]);
                 }

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();
@@ -97,6 +99,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
      */

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);
+    }
 }