Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitoff admin only roles for API #2616

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Splitoff admin only roles for API #2616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
265be80
Add new roles for problem/contest changes via API
vmcj Jun 30, 2024
11d2823
Implement Jaap his feedback
vmcj Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions webapp/config/packages/security.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_EDITOR, ROLE_API_CONTEST_EDITOR]
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:
Expand Down
46 changes: 46 additions & 0 deletions webapp/migrations/Version20240629154640.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

final class Version20240629154640 extends AbstractMigration
{
private const NEW_ROLES = ['api_problem_editor' => 'API Problem Editor',
'api_contest_editor' => 'API Contest Editor'];

public function getDescription(): string
{
return "Add new roles to the database.
Problem editor can add/delete/edit anything related to problems; files, testcases.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any (end) user will read this and I think @eldering his feedback is that people now don't know what the roles do without checking the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. What the roles do should ideally be clear from the role constant names, but at least from the role descriptions .

Contest editor can add/delete/edit the time & connected problems, but not the files
or testcases of those problems.
They are a subset of the ADMIN role in the API but not a proper superset of the API_WRITER
as that also has access to push teams etc.";
}

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;
}
}
12 changes: 6 additions & 6 deletions webapp/src/Controller/API/ContestController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function __construct(
* Add a new contest.
* @throws BadRequestHttpException
*/
#[IsGranted('ROLE_ADMIN')]
#[IsGranted('ROLE_API_CONTEST_EDITOR')]
#[Rest\Post('')]
#[OA\RequestBody(
required: true,
Expand Down Expand Up @@ -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_EDITOR')]
#[Rest\Delete('/{cid}/banner', name: 'delete_contest_banner')]
#[OA\Response(response: 204, description: 'Deleting banner succeeded')]
#[OA\Parameter(ref: '#/components/parameters/cid')]
Expand All @@ -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_EDITOR')]
#[Rest\Post("/{cid}/banner", name: 'post_contest_banner')]
#[Rest\Put("/{cid}/banner", name: 'put_contest_banner')]
#[OA\RequestBody(
Expand Down Expand Up @@ -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_EDITOR')]
#[Rest\Delete('/{cid}/problemset', name: 'delete_contest_problemset')]
#[OA\Response(response: 204, description: 'Deleting problemset document succeeded')]
#[OA\Parameter(ref: '#/components/parameters/cid')]
Expand All @@ -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_EDITOR')]
#[Rest\Post("/{cid}/problemset", name: 'post_contest_problemset')]
#[Rest\Put("/{cid}/problemset", name: 'put_contest_problemset')]
#[OA\RequestBody(
Expand Down Expand Up @@ -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_EDITOR')"))]
#[Rest\Patch('/{cid}')]
#[OA\RequestBody(
required: true,
Expand Down
8 changes: 4 additions & 4 deletions webapp/src/Controller/API/ProblemController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public function __construct(
* @throws BadRequestHttpException
* @throws NonUniqueResultException
*/
#[IsGranted('ROLE_ADMIN')]
#[IsGranted('ROLE_API_PROBLEM_EDITOR')]
#[Rest\Post('/add-data')]
#[OA\RequestBody(
required: true,
Expand Down Expand Up @@ -176,7 +176,7 @@ public function listAction(Request $request): Response
* @return array{problem_id: string, messages: array<string, string[]>}
* @throws NonUniqueResultException
*/
#[IsGranted('ROLE_ADMIN')]
#[IsGranted('ROLE_API_PROBLEM_EDITOR')]
#[Rest\Post('')]
#[OA\RequestBody(
required: true,
Expand Down Expand Up @@ -237,7 +237,7 @@ public function addProblemAction(Request $request): array
/**
* Unlink a problem from this contest.
*/
#[IsGranted('ROLE_ADMIN')]
#[IsGranted('ROLE_API_PROBLEM_EDITOR')]
#[Rest\Delete('/{id}')]
#[OA\Response(response: 204, description: 'Problem unlinked from contest succeeded')]
#[OA\Parameter(ref: '#/components/parameters/id')]
Expand Down Expand Up @@ -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_EDITOR')]
#[Rest\Put('/{id}')]
#[OA\Response(
response: 200,
Expand Down
20 changes: 11 additions & 9 deletions webapp/src/DataFixtures/DefaultData/RoleFixture.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_editor' => 'API Problem Editor',
'api_contest_editor' => 'API Contest Editor'
];
foreach ($roles as $roleName => $description) {
if (!($role = $manager->getRepository(Role::class)->findOneBy(['dj_role' => $roleName]))) {
Expand Down
7 changes: 7 additions & 0 deletions webapp/tests/Unit/Controller/API/BaseTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -373,4 +374,10 @@ public function provideSingleNotFound(): Generator
yield [$id];
}
}

protected function provideAllowedUsers(): Generator
{
yield ['admin', ['admin']];
yield ['team', [static::$testedRole]];
}
}
40 changes: 35 additions & 5 deletions webapp/tests/Unit/Controller/API/ContestControllerAdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
class ContestControllerAdminTest extends ContestControllerTest
{
protected ?string $apiUser = 'admin';
protected static string $testedRole = 'api_contest_editor';

private function parseSortYaml(string $yamlString): array
{
Expand All @@ -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 = <<<EOF
duration: 2:00:00
Expand Down Expand Up @@ -69,6 +73,8 @@ public function testAddYaml(): void
scoreboard_freeze_duration: 0:30:00
EOF;

$this->roles = $newRoles;
self::setUp();
$url = $this->helperGetEndpointURL($this->apiEndpoint);
$tempYamlFile = tempnam(sys_get_temp_dir(), "/contest-yaml-");
file_put_contents($tempYamlFile, $yaml);
Expand All @@ -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 = <<<EOF
{
Expand All @@ -103,6 +112,8 @@ public function testAddJson(): void
}
EOF;

$this->roles = $newRoles;
self::setUp();
$url = $this->helperGetEndpointURL($this->apiEndpoint);
$tempJsonFile = tempnam(sys_get_temp_dir(), "/contest-json-");
file_put_contents($tempJsonFile, $json);
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -299,14 +323,18 @@ 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_editor']];
}

/**
* @dataProvider provideNewContest
*/
public function testActivateTimeContestYaml(
string $activateTime, string $startTime, ?string $deactivateTime,
bool $setActivate, bool $setDeactivate
bool $setActivate, bool $setDeactivate, array $newRoles = [],
): void {
$yaml = <<<EOF
duration: 2:00:00
Expand All @@ -322,6 +350,8 @@ public function testActivateTimeContestYaml(
id: anothereruption
EOF;

$this->roles = $newRoles;
self::setUp();
if ($setActivate) {
$yaml = "activate_time: ".$activateTime."\n".$yaml;
}
Expand Down
Loading
Loading