From 265be80a771bd8a26c2caf5402707db37c32c55d Mon Sep 17 00:00:00 2001 From: Michael Vasseur <14887731+vmcj@users.noreply.github.com> Date: Sun, 30 Jun 2024 11:04:18 +0200 Subject: [PATCH] Add new roles for problem/contest changes via API This is for an usecase like EUC where there is an Ad-Hoc group which doesn't know each other yet (or even the system). The responsibility for the upload of the problems lies with one team which does not want admin access to make sure nothing gets broken. The same for changing the contest as BAPCtools does for example. Also extended the tests for admin access to now also check for the new roles while making sure admin also keeps the rights by transitivity. --- webapp/config/packages/security.yaml | 4 +- webapp/migrations/Version20240629154640.php | 41 +++++++++++++ .../src/Controller/API/ContestController.php | 12 ++-- .../src/Controller/API/ProblemController.php | 8 +-- .../DataFixtures/DefaultData/RoleFixture.php | 20 ++++--- .../Unit/Controller/API/BaseTestCase.php | 7 +++ .../API/ContestControllerAdminTest.php | 40 +++++++++++-- .../API/ProblemControllerAdminTest.php | 57 ++++++++++++++++--- 8 files changed, 155 insertions(+), 34 deletions(-) create mode 100644 webapp/migrations/Version20240629154640.php diff --git a/webapp/config/packages/security.yaml b/webapp/config/packages/security.yaml index 33361a36db..eaae1a40df 100644 --- a/webapp/config/packages/security.yaml +++ b/webapp/config/packages/security.yaml @@ -3,10 +3,10 @@ security: role_hierarchy: ROLE_JURY: [ROLE_CLARIFICATION_RW, ROLE_API, ROLE_API_READER, ROLE_API_SOURCE_READER] - ROLE_ADMIN: [ROLE_JURY, ROLE_JUDGEHOST, ROLE_API_WRITER] + ROLE_ADMIN: [ROLE_JURY, ROLE_JUDGEHOST, ROLE_API_WRITER, + ROLE_API_PROBLEM_CHANGE, ROLE_API_CONTEST_CHANGE] ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH] - # https://symfony.com/doc/current/security.html#registering-the-user-hashing-passwords password_hashers: App\Entity\User: diff --git a/webapp/migrations/Version20240629154640.php b/webapp/migrations/Version20240629154640.php new file mode 100644 index 0000000000..b2446e32d3 --- /dev/null +++ b/webapp/migrations/Version20240629154640.php @@ -0,0 +1,41 @@ + 'API Problem Changer', + 'api_contest_change' => 'API Contest Changer']; + + public function getDescription(): string + { + return 'Add new roles to the database.'; + } + + public function up(Schema $schema): void + { + foreach (self::NEW_ROLES as $role => $description) { + $this->addSql( + 'INSERT INTO role (`role`, `description`) VALUES (:role, :desc)', + ['role' => $role, 'desc' => $description] + ); + } + } + + public function down(Schema $schema): void + { + foreach (array_keys(self::NEW_ROLES) as $role) { + $this->addSql('DELETE FROM role WHERE role = ' . $role ); + } + } + + public function isTransactional(): bool + { + return false; + } +} diff --git a/webapp/src/Controller/API/ContestController.php b/webapp/src/Controller/API/ContestController.php index bea5588e4d..cef0f4352f 100644 --- a/webapp/src/Controller/API/ContestController.php +++ b/webapp/src/Controller/API/ContestController.php @@ -74,7 +74,7 @@ public function __construct( * Add a new contest. * @throws BadRequestHttpException */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_CONTEST_CHANGE')] #[Rest\Post('')] #[OA\RequestBody( required: true, @@ -200,7 +200,7 @@ public function bannerAction(Request $request, string $cid): Response /** * Delete the banner for the given contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_CONTEST_CHANGE')] #[Rest\Delete('/{cid}/banner', name: 'delete_contest_banner')] #[OA\Response(response: 204, description: 'Deleting banner succeeded')] #[OA\Parameter(ref: '#/components/parameters/cid')] @@ -220,7 +220,7 @@ public function deleteBannerAction(Request $request, string $cid): Response /** * Set the banner for the given contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_CONTEST_CHANGE')] #[Rest\Post("/{cid}/banner", name: 'post_contest_banner')] #[Rest\Put("/{cid}/banner", name: 'put_contest_banner')] #[OA\RequestBody( @@ -268,7 +268,7 @@ public function setBannerAction(Request $request, string $cid, ValidatorInterfac /** * Delete the problemset document for the given contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_CONTEST_CHANGE')] #[Rest\Delete('/{cid}/problemset', name: 'delete_contest_problemset')] #[OA\Response(response: 204, description: 'Deleting problemset document succeeded')] #[OA\Parameter(ref: '#/components/parameters/cid')] @@ -288,7 +288,7 @@ public function deleteProblemsetAction(Request $request, string $cid): Response /** * Set the problemset document for the given contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_CONTEST_CHANGE')] #[Rest\Post("/{cid}/problemset", name: 'post_contest_problemset')] #[Rest\Put("/{cid}/problemset", name: 'put_contest_problemset')] #[OA\RequestBody( @@ -384,7 +384,7 @@ public function problemsetAction(Request $request, string $cid): Response * Change the start time or unfreeze (thaw) time of the given contest. * @throws NonUniqueResultException */ - #[IsGranted('ROLE_API_WRITER')] + #[IsGranted(new Expression("is_granted('ROLE_API_WRITER') or is_granted('ROLE_API_CONTEST_CHANGE')"))] #[Rest\Patch('/{cid}')] #[OA\RequestBody( required: true, diff --git a/webapp/src/Controller/API/ProblemController.php b/webapp/src/Controller/API/ProblemController.php index ecfab13749..90b54ee1af 100644 --- a/webapp/src/Controller/API/ProblemController.php +++ b/webapp/src/Controller/API/ProblemController.php @@ -61,7 +61,7 @@ public function __construct( * @throws BadRequestHttpException * @throws NonUniqueResultException */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_PROBLEM_CHANGE')] #[Rest\Post('/add-data')] #[OA\RequestBody( required: true, @@ -176,7 +176,7 @@ public function listAction(Request $request): Response * @return array{problem_id: string, messages: array} * @throws NonUniqueResultException */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_PROBLEM_CHANGE')] #[Rest\Post('')] #[OA\RequestBody( required: true, @@ -237,7 +237,7 @@ public function addProblemAction(Request $request): array /** * Unlink a problem from this contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_PROBLEM_CHANGE')] #[Rest\Delete('/{id}')] #[OA\Response(response: 204, description: 'Problem unlinked from contest succeeded')] #[OA\Parameter(ref: '#/components/parameters/id')] @@ -290,7 +290,7 @@ public function unlinkProblemAction(Request $request, string $id): Response /** * Link an existing problem to this contest. */ - #[IsGranted('ROLE_ADMIN')] + #[IsGranted('ROLE_API_PROBLEM_CHANGE')] #[Rest\Put('/{id}')] #[OA\Response( response: 200, diff --git a/webapp/src/DataFixtures/DefaultData/RoleFixture.php b/webapp/src/DataFixtures/DefaultData/RoleFixture.php index 088fe210ec..5ec8c63de5 100644 --- a/webapp/src/DataFixtures/DefaultData/RoleFixture.php +++ b/webapp/src/DataFixtures/DefaultData/RoleFixture.php @@ -20,15 +20,17 @@ public function load(ObjectManager $manager): void { // Mapping from role to description $roles = [ - 'admin' => 'Administrative User', - 'jury' => 'Jury User', - 'team' => 'Team Member', - 'balloon' => 'Balloon runner', - 'judgehost' => '(Internal/System) Judgehost', - 'api_reader' => 'API reader', - 'api_writer' => 'API writer', - 'api_source_reader' => 'Source code reader', - 'clarification_rw' => 'Clarification handler', + 'admin' => 'Administrative User', + 'jury' => 'Jury User', + 'team' => 'Team Member', + 'balloon' => 'Balloon runner', + 'judgehost' => '(Internal/System) Judgehost', + 'api_reader' => 'API reader', + 'api_writer' => 'API writer', + 'api_source_reader' => 'Source code reader', + 'clarification_rw' => 'Clarification handler', + 'api_problem_change' => 'API Problem Changer', + 'api_contest_change' => 'API Contest Changer' ]; foreach ($roles as $roleName => $description) { if (!($role = $manager->getRepository(Role::class)->findOneBy(['dj_role' => $roleName]))) { diff --git a/webapp/tests/Unit/Controller/API/BaseTestCase.php b/webapp/tests/Unit/Controller/API/BaseTestCase.php index 1549cfcb80..b58ef2e3a7 100644 --- a/webapp/tests/Unit/Controller/API/BaseTestCase.php +++ b/webapp/tests/Unit/Controller/API/BaseTestCase.php @@ -12,6 +12,7 @@ abstract class BaseTestCase extends BaseBaseTestCase { protected static array $rootEndpoints = ['contests', 'judgehosts', 'users']; + protected static string $testedRole = 'unset'; /** @var KernelBrowser */ protected KernelBrowser $client; @@ -373,4 +374,10 @@ public function provideSingleNotFound(): Generator yield [$id]; } } + + protected function provideAllowedUsers(): Generator + { + yield ['admin', ['admin']]; + yield ['team', [static::$testedRole]]; + } } diff --git a/webapp/tests/Unit/Controller/API/ContestControllerAdminTest.php b/webapp/tests/Unit/Controller/API/ContestControllerAdminTest.php index 5655e97c80..8ca2a9e94a 100644 --- a/webapp/tests/Unit/Controller/API/ContestControllerAdminTest.php +++ b/webapp/tests/Unit/Controller/API/ContestControllerAdminTest.php @@ -21,6 +21,7 @@ class ContestControllerAdminTest extends ContestControllerTest { protected ?string $apiUser = 'admin'; + protected static string $testedRole = 'api_contest_change'; private function parseSortYaml(string $yamlString): array { @@ -29,7 +30,10 @@ private function parseSortYaml(string $yamlString): array return $new; } - public function testAddYaml(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddYaml(string $user, array $newRoles): void { $yaml = <<roles = $newRoles; + self::setUp(); $url = $this->helperGetEndpointURL($this->apiEndpoint); $tempYamlFile = tempnam(sys_get_temp_dir(), "/contest-yaml-"); file_put_contents($tempYamlFile, $yaml); @@ -89,7 +95,10 @@ public function testAddYaml(): void self::assertNull($this->getContest($cid)->getDeactivatetime()); } - public function testAddJson(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddJson(string $user, array $newRoles): void { $json = <<roles = $newRoles; + self::setUp(); $url = $this->helperGetEndpointURL($this->apiEndpoint); $tempJsonFile = tempnam(sys_get_temp_dir(), "/contest-json-"); file_put_contents($tempJsonFile, $json); @@ -121,8 +132,13 @@ protected function getContest(int|string $cid): Contest return static::getContainer()->get(EntityManagerInterface::class)->getRepository(Contest::class)->findOneBy(['externalid' => $cid]); } - public function testBannerManagement(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testBannerManagement(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); // First, make sure we have no banner $id = 1; if ($this->objectClassForExternalId !== null) { @@ -163,8 +179,13 @@ public function testBannerManagement(): void self::assertArrayNotHasKey('banner', $object); } - public function testProblemsetManagement(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testProblemsetManagement(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); // First, make sure we have no problemset document $id = 1; if ($this->objectClassForExternalId !== null) { @@ -233,7 +254,10 @@ public function testChangeTimes( array $extraFixtures = [], bool $checkUnfreezeTime = false, bool $convertRelativeTimes = false, + array $newRoles = [], ): void { + $this->roles = $newRoles; + self::setUp(); $this->loadFixture(DemoPreStartContestFixture::class); $this->loadFixtures($extraFixtures); $id = 1; @@ -299,6 +323,10 @@ public function provideChangeTimes(): Generator yield [['id' => 1, 'scoreboard_thaw_time' => '+15 seconds', 'force' => true], 204, null, [DemoPostUnfreezeContestFixture::class], false, true]; yield [['id' => 1, 'scoreboard_thaw_time' => '+15 seconds'], 204, null, [], false, true]; yield [['id' => 1, 'scoreboard_thaw_time' => '-15 seconds'], 200, 'Demo contest', [], true, true]; + + // Show that this works for both roles + yield [['id' => 1, 'scoreboard_thaw_time' => '-14 seconds'], 200, 'Demo contest', [], true, true, ['admin']]; + yield [['id' => 1, 'scoreboard_thaw_time' => '-13 seconds'], 200, 'Demo contest', [], true, true, ['api_contest_change']]; } /** @@ -306,7 +334,7 @@ public function provideChangeTimes(): Generator */ public function testActivateTimeContestYaml( string $activateTime, string $startTime, ?string $deactivateTime, - bool $setActivate, bool $setDeactivate + bool $setActivate, bool $setDeactivate, array $newRoles = [], ): void { $yaml = <<roles = $newRoles; + self::setUp(); if ($setActivate) { $yaml = "activate_time: ".$activateTime."\n".$yaml; } diff --git a/webapp/tests/Unit/Controller/API/ProblemControllerAdminTest.php b/webapp/tests/Unit/Controller/API/ProblemControllerAdminTest.php index 30247b78b2..822269faaa 100644 --- a/webapp/tests/Unit/Controller/API/ProblemControllerAdminTest.php +++ b/webapp/tests/Unit/Controller/API/ProblemControllerAdminTest.php @@ -11,6 +11,7 @@ class ProblemControllerAdminTest extends ProblemControllerTest { protected ?string $apiUser = 'admin'; + protected static string $testedRole = 'api_problem_change'; protected function setUp(): void { @@ -21,7 +22,10 @@ protected function setUp(): void parent::setUp(); } - public function testAddJson(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddJson(string $user, array $newRoles): void { $json = <<roles = $newRoles; + self::setUp(); $url = $this->helperGetEndpointURL($this->apiEndpoint) . '/add-data'; $tempJsonFile = tempnam(sys_get_temp_dir(), "/problems-json-"); file_put_contents($tempJsonFile, $json); @@ -86,8 +92,13 @@ public function testAddJson(): void self::assertEquals($expectedProblems, $addedProblems); } - public function testDelete(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testDelete(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); // Check that we can delete the problem $url = $this->helperGetEndpointURL($this->apiEndpoint) . '/fltcmp'; $this->verifyApiJsonResponse('DELETE', $url, 204, $this->apiUser); @@ -98,15 +109,25 @@ public function testDelete(): void self::assertCount(2, $problems); } - public function testDeleteNotFound(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testDeleteNotFound(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); // Check that we can delete the problem $url = $this->helperGetEndpointURL($this->apiEndpoint) . '/4'; $this->verifyApiJsonResponse('DELETE', $url, 404, $this->apiUser); } - public function testAdd(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAdd(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); $this->loadFixture(DummyProblemFixture::class); $body = [ @@ -145,16 +166,26 @@ public function testAdd(): void self::assertCount(4, $problems); } - public function testAddNotFound(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddNotFound(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); // Check that we can delete the problem $url = $this->helperGetEndpointURL($this->apiEndpoint) . '/notfound'; $response = $this->verifyApiJsonResponse('PUT', $url, 404, $this->apiUser, ['label' => 'dummy']); self::assertEquals("Object with ID 'notfound' not found", $response['message']); } - public function testAddExisting(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddExisting(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); $this->loadFixture(DummyProblemFixture::class); // Check that we can not add a problem that is already added @@ -163,8 +194,13 @@ public function testAddExisting(): void self::assertEquals('Problem already linked to contest', $response['message']); } - public function testAddToLocked(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testAddToLocked(string $user, array $newRoles): void { + $this->roles = $newRoles; + self::setUp(); $this->loadFixture(LockedContestFixture::class); $this->loadFixture(DummyProblemFixture::class); @@ -184,9 +220,14 @@ public function testAddToLocked(): void self::assertStringContainsString('Contest is locked', $problemResponse['message']); } - public function testDeleteFromLocked(): void + /** + * @dataProvider provideAllowedUsers + */ + public function testDeleteFromLocked(string $user, array $newRoles): void { $this->loadFixture(LockedContestFixture::class); + $this->roles = $newRoles; + self::setUp(); // Check that we cannot delete the problem. $url = $this->helperGetEndpointURL($this->apiEndpoint) . '/fltcmp';