Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Fix user add and update api endpoints #3040

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.

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

Draft
LuukBlankenstijn wants to merge 1 commit into DOMjudge:main
base: main
Choose a base branch
Loading
from LuukBlankenstijn:fix/api-fixes
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
106 changes: 57 additions & 49 deletions webapp/src/Controller/API/UserController.php
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Component\Security\Http\Attribute\IsGranted;
use Symfony\Component\Validator\Exception\ValidationFailedException;
use Symfony\Component\Validator\Validator\ValidatorInterface;

/**
* @extends AbstractRestController<User, User>
Expand All @@ -41,7 +44,8 @@ public function __construct(
DOMJudgeService $dj,
ConfigurationService $config,
EventLogService $eventLogService,
protected readonly ImportExportService $importExportService
protected readonly ImportExportService $importExportService,
protected readonly ValidatorInterface $validator,
) {
parent::__construct($entityManager, $dj, $config, $eventLogService);
}
Expand Down Expand Up @@ -86,9 +90,9 @@ public function addGroupsAction(Request $request): string
$message = null;
$result = -1;
if ((($tsvFile && ($result = $this->importExportService->importTsv('groups', $tsvFile, $message))) ||
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
$result >= 0) {
return "$result new group(s) successfully added.";
return "$result new group(s) successfully added.";
} else {
throw new BadRequestHttpException("Error while adding groups: $message");
}
Expand Down Expand Up @@ -171,7 +175,7 @@ public function addTeamsAction(Request $request): string
}
$message = null;
if ((($tsvFile && ($result = $this->importExportService->importTsv('teams', $tsvFile, $message))) ||
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
$result >= 0) {
// TODO: better return all teams here?
return "$result new team(s) successfully added.";
Expand Down Expand Up @@ -235,7 +239,7 @@ public function addAccountsAction(Request $request): string

$message = null;
if ((($tsvFile && ($result = $this->importExportService->importTsv('accounts', $tsvFile, $message))) ||
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
$result >= 0) {
// TODO: better return all accounts here?
return "$result new account(s) successfully added.";
Expand Down Expand Up @@ -291,13 +295,12 @@ public function singleAction(Request $request, string $id): Response
* Add a new user.
*/
#[IsGranted('ROLE_API_WRITER')]
#[Rest\Post]
#[Rest\Post()]
#[OA\RequestBody(
required: true,
content: [
new OA\MediaType(
mediaType: 'multipart/form-data',
schema: new OA\Schema(ref: new Model(type: AddUser::class))
new OA\JsonContent(
ref: new Model(type: AddUser::class)
),
]
)]
Expand All @@ -307,86 +310,86 @@ public function singleAction(Request $request, string $id): Response
content: new Model(type: User::class)
)]
public function addAction(
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST, validationGroups: ['add'])]
AddUser $addUser,
Request $request
): Response {
return $this->addOrUpdateUser($addUser, $request);
}

/**
* Update an existing User or create one with the given ID
* Update an existing User
*/
#[IsGranted('ROLE_API_WRITER')]
#[Rest\Put('/{id}')]
#[Rest\Patch('/{id}')]
#[OA\RequestBody(
required: true,
content: [
new OA\MediaType(
mediaType: 'multipart/form-data',
schema: new OA\Schema(ref: new Model(type: UpdateUser::class))
new OA\JsonContent(
ref: new Model(type: UpdateUser::class)
),
]
)]
#[OA\Response(
response: 201,
description: 'Returns the added user',
description: 'Returns the updated user',
content: new Model(type: User::class)
)]
public function updateAction(
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
UpdateUser $updateUser,
UpdateUser $mutateUser,
string $id,
Request $request
): Response {
return $this->addOrUpdateUser($updateUser, $request);
/** @var User|null $user */
$user = $this->em->getRepository(User::class)->findOneBy(['externalid' => $id]);
if ($user === null) {
throw new NotFoundHttpException(sprintf("User with id %s not found", $id));
}
return $this->addOrUpdateUser($mutateUser, $request, $user);
}

protected function addOrUpdateUser(AddUser $addUser, Request $request): Response
protected function addOrUpdateUser(AddUser|UpdateUser $mutateUser, Request $request, ?User $user = null): Response
{
if ($addUser instanceof UpdateUser && !$addUser->id) {
throw new BadRequestHttpException('`id` field is required');
// if the user to update is not set, create a new user
if (!$user) {
$user = new User();
}

if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
if ($mutateUser->username !== null) {
$user->setUsername($mutateUser->username);
}

$user = new User();
if ($addUser instanceof UpdateUser) {
$existingUser = $this->em->getRepository(User::class)->findOneBy(['externalid' => $addUser->id]);
if ($existingUser) {
$user = $existingUser;
}
if ($mutateUser->name !== null) {
$user->setName($mutateUser->name);
}
$user
->setUsername($addUser->username)
->setName($addUser->name)
->setEmail($addUser->email)
->setIpAddress($addUser->ip)
->setPlainPassword($addUser->password)
->setEnabled($addUser->enabled ?? true);

if ($addUser instanceof UpdateUser) {
$user->setExternalid($addUser->id);
if ($mutateUser->email !== null) {
$user->setEmail($mutateUser->email);
}

if ($addUser->teamId) {
if ($mutateUser->ip !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All these if conditions mean that we can no longer remove e.g. the IP address from an user using the API. For this, we would either still need a PUT endpoint or differentiate between the absence of properties in the request vs explicit null values.

Copy link
Member

Choose a reason for hiding this comment

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

All these if conditions mean that we can no longer remove e.g. the IP address from an user using the API. For this, we would either still need a PUT endpoint or differentiate between the absence of properties in the request vs explicit null values.

The last is not allowed in CLICS, leaving a property out or setting it to null are the same action.

Kevinjil reacted with thumbs up emoji
$user->setIpAddress($mutateUser->ip);
}
if ($mutateUser->password !== null) {
$user->setPlainPassword($mutateUser->password);
}
if ($mutateUser->enabled !== null) {
$user->setEnabled($mutateUser->enabled);
}
if ($mutateUser->teamId) {
/** @var Team|null $team */
$team = $this->em->createQueryBuilder()
->from(Team::class, 't')
->select('t')
->andWhere('t.externalid = :team')
->setParameter('team', $addUser->teamId)
->setParameter('team', $mutateUser->teamId)
->getQuery()
->getOneOrNullResult();

if ($team === null) {
throw new BadRequestHttpException(sprintf("Team %s not found", $addUser->teamId));
throw new BadRequestHttpException(sprintf("Team %s not found", $mutateUser->teamId));
}
$user->setTeam($team);
}

$roles = $addUser->roles;
$roles = $mutateUser->roles;
// For the file import we change a CDS user to the roles needed for ICPC CDS.
if ($user->getUsername() === 'cds') {
$roles = ['cds'];
Expand All @@ -408,18 +411,23 @@ protected function addOrUpdateUser(AddUser $addUser, Request $request): Response
$user->addUserRole($role);
}

$validation = $this->validator->validate($user);
if (count($validation) > 0) {
throw new ValidationFailedException($user, $validation);
}

$this->em->persist($user);
$this->em->flush();
$this->dj->auditlog('user', $user->getUserid(), 'added');
$this->dj->auditlog('user', $user->getUserid(), 'updated');

return $this->renderCreateData($request, $user, 'user', $user->getUserid());
}

protected function getQueryBuilder(Request $request): QueryBuilder
{
$queryBuilder = $this->em->createQueryBuilder()
->from(User::class, 'u')
->select('u');
->from(User::class, 'u')
->select('u');

if ($request->query->has('team')) {
$queryBuilder
Expand Down
33 changes: 15 additions & 18 deletions webapp/src/DataTransferObject/AddUser.php
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,24 @@

use JMS\Serializer\Annotation as Serializer;
use OpenApi\Attributes as OA;
use Symfony\Component\Validator\Constraints as Assert;

#[OA\Schema(required: ['username', 'name', 'roles'])]
class AddUser
{
#[Assert\NotBlank]
public string $username;
#[Assert\NotBlank]
public string $name;
public ?string $id = null;
public ?string $email = null;
public ?string $ip = null;
#[OA\Property(format: 'password')]
public ?string $password = null;
public ?bool $enabled = null;
public ?string $teamId = null;
/**
* @param array<string> $roles
* @var array<string>
*/
public function __construct(
public readonly string $username,
public readonly string $name,
#[OA\Property(format: 'email', nullable: true)]
public readonly ?string $email,
#[OA\Property(nullable: true)]
public readonly ?string $ip,
#[OA\Property(format: 'password', nullable: true)]
public readonly ?string $password,
#[OA\Property(nullable: true)]
public readonly ?bool $enabled,
#[OA\Property(nullable: true)]
public readonly ?string $teamId,
#[Serializer\Type('array<string>')]
public readonly array $roles,
) {}
#[Serializer\Type('array<string>')]
public array $roles = [];
}
31 changes: 16 additions & 15 deletions webapp/src/DataTransferObject/UpdateUser.php
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,23 @@

namespace App\DataTransferObject;

use JMS\Serializer\Annotation as Serializer;
use OpenApi\Attributes as OA;

#[OA\Schema(required: ['id', 'username', 'name', 'roles'])]
class UpdateUser extends AddUser
class UpdateUser
{
public function __construct(
public readonly string $id,
string $username,
string $name,
?string $email,
?string $ip,
?string $password,
?bool $enabled,
?string $teamId,
array $roles
) {
parent::__construct($username, $name, $email, $ip, $password, $enabled, $teamId, $roles);
}
public ?string $id = null;
public ?string $username = null;
public ?string $name = null;
public ?string $email = null;
public ?string $ip = null;
#[OA\Property(format: 'password')]
public ?string $password = null;
public ?bool $enabled = null;
public ?string $teamId = null;
/**
* @var array<string>
*/
#[Serializer\Type('array<string>')]
public array $roles = [];
}
14 changes: 10 additions & 4 deletions webapp/src/Entity/User.php
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php declare(strict_types=1);

namespace App\Entity;

use App\Controller\API\AbstractRestController as ARC;
Expand Down Expand Up @@ -132,7 +133,7 @@ class User extends BaseApiEntity implements
#[ORM\JoinTable(name: 'userrole')]
#[ORM\JoinColumn(name: 'userid', referencedColumnName: 'userid', onDelete: 'CASCADE')]
#[ORM\InverseJoinColumn(name: 'roleid', referencedColumnName: 'roleid', onDelete: 'CASCADE')]
#[Assert\Count(min: 1)]
#[Assert\Count(min: 1, minMessage: 'User should have at least {{ limit }} role')]
#[Serializer\Exclude]
private Collection $user_roles;

Expand Down Expand Up @@ -439,9 +440,14 @@ public function getType(): ?string
// Types allowed by the CCS Specs Contest API in order of most permissions to least.
// Either key=>value where key is the DOMjudge role and value is the API account type or
// only value, where both the DOMjudge role and API type are the same.
$allowedTypes = ['admin', 'api_writer' => 'admin', 'api_reader' => 'admin',
'jury' => 'judge', 'api_source_reader' => 'judge',
'team'];
$allowedTypes = [
'admin',
'api_writer' => 'admin',
'api_reader' => 'admin',
'jury' => 'judge',
'api_source_reader' => 'judge',
'team'
];
foreach ($allowedTypes as $role => $allowedType) {
if (is_numeric($role)) {
$role = $allowedType;
Expand Down
38 changes: 38 additions & 0 deletions webapp/src/EventListener/ValidationExceptionListener.php
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php declare(strict_types=1);

namespace App\EventListener;

use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
use Symfony\Component\Validator\Exception\ValidationFailedException;
use Symfony\Component\HttpFoundation\JsonResponse;

#[AsEventListener(event: 'kernel.exception', priority: 0)]
class ValidationExceptionListener
{
public function __invoke(ExceptionEvent $event): void
{
$e = $event->getThrowable();

if (!$e instanceof ValidationFailedException) {
return;
}

$errors = [];
foreach ($e->getViolations() as $violation) {
$errors[] = [
'property' => $violation->getPropertyPath(),
'message' => $violation->getMessage(),
'code' => $violation->getCode(),
];
}

$response = new JsonResponse([
'title' => 'Bad Request',
'status' => 400,
'errors' => $errors,
], 400);

$event->setResponse($response);
}
}
Loading

AltStyle によって変換されたページ (->オリジナル) /