Skip to content

Commit 53ce316

Browse files
stevenrombautsstevenrombauts
authored
Audit log: track maintainer added/removed (#1607)
* Track when maintainer is added or removed * Add missing controller tests for adding and removing maintainer
1 parent decd0cf commit 53ce316

File tree

4 files changed

+122
-15
lines changed

4 files changed

+122
-15
lines changed

‎src/Audit/AuditRecordType.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515
enum AuditRecordType: string
1616
{
1717
// package ownership
18-
case MaintainerAdded = 'maintainer_added'; // TODO
19-
case MaintainerRemoved = 'maintainer_removed'; // TODO
18+
case MaintainerAdded = 'maintainer_added';
19+
case MaintainerRemoved = 'maintainer_removed';
2020
case PackageTransferred = 'package_transferred';
2121

2222
// package management

‎src/Controller/PackageController.php‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
namespace App\Controller;
1414

15+
use App\Entity\AuditRecord;
1516
use App\Entity\Dependent;
1617
use App\Entity\Download;
1718
use App\Entity\Job;
@@ -904,7 +905,7 @@ public function deletePackageAction(Request $req, string $name): Response
904905
}
905906

906907
#[Route(path: '/packages/{name:package}/maintainers/', name: 'add_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])]
907-
public function createMaintainerAction(Request $req, #[MapEntity] Package $package, LoggerInterface $logger): RedirectResponse
908+
public function createMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): RedirectResponse
908909
{
909910
$this->denyAccessUnlessGranted(PackageActions::AddMaintainer->value, $package);
910911

@@ -914,19 +915,20 @@ public function createMaintainerAction(Request $req, #[MapEntity] Package $packa
914915
try {
915916
$em = $this->getEM();
916917
if ($username = $form->getData()->getUser()) {
917-
$user = $em->getRepository(User::class)->findOneByUsernameOrEmail($username);
918+
$maintainer = $em->getRepository(User::class)->findOneByUsernameOrEmail($username);
918919
}
919920

920-
if (!empty($user)) {
921-
if (!$package->isMaintainer($user)) {
922-
$package->addMaintainer($user);
923-
$this->packageManager->notifyNewMaintainer($user, $package);
921+
if (!empty($maintainer)) {
922+
if (!$package->isMaintainer($maintainer)) {
923+
$package->addMaintainer($maintainer);
924+
$em->persist(AuditRecord::maintainerAdded($package, $maintainer, $user));
925+
$this->packageManager->notifyNewMaintainer($maintainer, $package);
924926
}
925927

926928
$em->persist($package);
927929
$em->flush();
928930

929-
$this->addFlash('success', $user->getUsername().' is now a '.$package->getName().' maintainer.');
931+
$this->addFlash('success', $maintainer->getUsername().' is now a '.$package->getName().' maintainer.');
930932

931933
return $this->redirectToRoute('view_package', ['name' => $package->getName()]);
932934
}
@@ -941,7 +943,7 @@ public function createMaintainerAction(Request $req, #[MapEntity] Package $packa
941943
}
942944

943945
#[Route(path: '/packages/{name:package}/maintainers/delete', name: 'remove_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])]
944-
public function removeMaintainerAction(Request $req, #[MapEntity] Package $package, LoggerInterface $logger): Response
946+
public function removeMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): Response
945947
{
946948
$this->denyAccessUnlessGranted(PackageActions::RemoveMaintainer->value, $package);
947949

@@ -951,18 +953,19 @@ public function removeMaintainerAction(Request $req, #[MapEntity] Package $packa
951953
try {
952954
$em = $this->getEM();
953955
if ($username = $removeMaintainerForm->getData()->getUser()) {
954-
$user = $em->getRepository(User::class)->findOneByUsernameOrEmail($username);
956+
$maintainer = $em->getRepository(User::class)->findOneByUsernameOrEmail($username);
955957
}
956958

957-
if (!empty($user)) {
958-
if ($package->isMaintainer($user)) {
959-
$package->getMaintainers()->removeElement($user);
959+
if (!empty($maintainer)) {
960+
if ($package->isMaintainer($maintainer)) {
961+
$package->getMaintainers()->removeElement($maintainer);
962+
$em->persist(AuditRecord::maintainerRemoved($package, $maintainer, $user));
960963
}
961964

962965
$em->persist($package);
963966
$em->flush();
964967

965-
$this->addFlash('success', $user->getUsername().' is no longer a '.$package->getName().' maintainer.');
968+
$this->addFlash('success', $maintainer->getUsername().' is no longer a '.$package->getName().' maintainer.');
966969

967970
return $this->redirectToRoute('view_package', ['name' => $package->getName()]);
968971
}

‎src/Entity/AuditRecord.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ private function __construct(
4545
public readonly ?string $vendor = null,
4646
#[ORM\Column(nullable: true)]
4747
public readonly ?int $packageId = null,
48+
#[ORM\Column(nullable: true)]
49+
public readonly ?int $userId = null,
4850
) {
4951
$this->id = new Ulid();
5052
$this->datetime = new \DateTimeImmutable();
@@ -97,6 +99,16 @@ public static function versionReferenceChange(Version $version, ?string $oldSour
9799
);
98100
}
99101

102+
public static function maintainerAdded(Package $package, User $maintainer, ?User $actor): self
103+
{
104+
return new self(AuditRecordType::MaintainerAdded, ['name' => $package->getName(), 'maintainer' => self::getUserData($maintainer), 'actor' => self::getUserData($actor)], $actor?->getId(), $package->getVendor(), $package->getId(), $maintainer->getId());
105+
}
106+
107+
public static function maintainerRemoved(Package $package, User $maintainer, ?User $actor): self
108+
{
109+
return new self(AuditRecordType::MaintainerRemoved, ['name' => $package->getName(), 'maintainer' => self::getUserData($maintainer), 'actor' => self::getUserData($actor)], $actor?->getId(), $package->getVendor(), $package->getId(), $maintainer->getId());
110+
}
111+
100112
/**
101113
* @return array{id: int, username: string}|string
102114
*/

‎tests/Controller/PackageControllerTest.php‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212

1313
namespace App\Tests\Controller;
1414

15+
use App\Audit\AuditRecordType;
16+
use App\Entity\Package;
17+
use App\Entity\User;
1518
use App\Tests\IntegrationTestCase;
1619

1720
class PackageControllerTest extends IntegrationTestCase
@@ -52,4 +55,93 @@ public function testEdit(): void
5255
self::assertResponseIsSuccessful();
5356
self::assertSame('github.com/composer/composer', $crawler->filter('.canonical')->text());
5457
}
58+
59+
public function testCreateMaintainer(): void
60+
{
61+
$owner = self::createUser('owner', '[email protected]');
62+
$newMaintainer = self::createUser('maintainer', '[email protected]');
63+
$package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$owner]);
64+
65+
$this->store($owner, $newMaintainer, $package);
66+
67+
$this->client->loginUser($owner);
68+
69+
$this->assertFalse($package->isMaintainer($newMaintainer));
70+
71+
$crawler = $this->client->request('GET', '/packages/test/pkg');
72+
73+
$form = $crawler->filter('[name="add_maintainer_form"]')->form();
74+
$form->setValues([
75+
'add_maintainer_form[user]' => 'maintainer',
76+
]);
77+
78+
$this->client->enableProfiler(); // This is required in 7.3.4 to assert emails were sent, see https://github.com/symfony/symfony/issues/61873
79+
$this->client->submit($form);
80+
81+
$this->assertEmailCount(1);
82+
$email = $this->getMailerMessage();
83+
$this->assertNotNull($email);
84+
$this->assertEmailHeaderSame($email, 'To', $newMaintainer->getEmail());
85+
86+
$this->assertResponseRedirects('/packages/test/pkg');
87+
$this->client->followRedirect();
88+
$this->assertResponseIsSuccessful();
89+
90+
$em = self::getEM();
91+
$em->clear();
92+
93+
$maintainer = $em->getRepository(User::class)->find($newMaintainer->getId());
94+
$package = $em->getRepository(Package::class)->find($package->getId());
95+
96+
$this->assertTrue($package->isMaintainer($maintainer));
97+
98+
$auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([
99+
'type' => AuditRecordType::MaintainerAdded->value,
100+
'packageId' => $package->getId(),
101+
'actorId' => $owner->getId(),
102+
]);
103+
$this->assertNotNull($auditRecord);
104+
}
105+
106+
public function testRemoveMaintainer(): void
107+
{
108+
$owner = self::createUser('owner', '[email protected]');
109+
$maintainer = self::createUser('maintainer', '[email protected]');
110+
$package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$owner, $maintainer]);
111+
112+
$this->store($owner, $maintainer, $package);
113+
114+
$this->client->loginUser($owner);
115+
116+
$this->assertTrue($package->isMaintainer($maintainer));
117+
118+
$crawler = $this->client->request('GET', '/packages/test/pkg');
119+
120+
$form = $crawler->filter('[name="remove_maintainer_form"]')->form();
121+
$form->setValues([
122+
'remove_maintainer_form[user]' => $maintainer->getId(),
123+
]);
124+
125+
$this->client->submit($form);
126+
127+
$this->assertResponseRedirects('/packages/test/pkg');
128+
$this->client->followRedirect();
129+
$this->assertResponseIsSuccessful();
130+
131+
$em = self::getEM();
132+
$em->clear();
133+
134+
$maintainer = $em->getRepository(User::class)->find($maintainer->getId());
135+
$package = $em->getRepository(Package::class)->find($package->getId());
136+
137+
$this->assertFalse($package->isMaintainer($maintainer));
138+
139+
$auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([
140+
'type' => AuditRecordType::MaintainerRemoved->value,
141+
'packageId' => $package->getId(),
142+
'actorId' => $owner->getId(),
143+
]);
144+
145+
$this->assertNotNull($auditRecord);
146+
}
55147
}

0 commit comments

Comments
 (0)