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

Commit bf65174

Browse files
Fix user add and update endpoint logic. The following problems were
fixed. - Server did not accept the data in the format of the api spec. fixed by changing to json. - Updating using required username and name always to be set, even if when not changing one of them. - When updating a user (with the mandatory username), the server would return a 404 saying this username is already in use.
1 parent af92629 commit bf65174

File tree

5 files changed

+136
-86
lines changed

5 files changed

+136
-86
lines changed

‎webapp/src/Controller/API/UserController.php‎

Lines changed: 57 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@
2323
use Symfony\Component\HttpFoundation\Response;
2424
use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
2525
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
26+
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
2627
use Symfony\Component\Security\Http\Attribute\IsGranted;
28+
use Symfony\Component\Validator\Exception\ValidationFailedException;
29+
use Symfony\Component\Validator\Validator\ValidatorInterface;
2730

2831
/**
2932
* @extends AbstractRestController<User, User>
@@ -41,7 +44,8 @@ public function __construct(
4144
DOMJudgeService $dj,
4245
ConfigurationService $config,
4346
EventLogService $eventLogService,
44-
protected readonly ImportExportService $importExportService
47+
protected readonly ImportExportService $importExportService,
48+
protected readonly ValidatorInterface $validator,
4549
) {
4650
parent::__construct($entityManager, $dj, $config, $eventLogService);
4751
}
@@ -86,9 +90,9 @@ public function addGroupsAction(Request $request): string
8690
$message = null;
8791
$result = -1;
8892
if ((($tsvFile && ($result = $this->importExportService->importTsv('groups', $tsvFile, $message))) ||
89-
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
93+
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
9094
$result >= 0) {
91-
return "$result new group(s) successfully added.";
95+
return "$result new group(s) successfully added.";
9296
} else {
9397
throw new BadRequestHttpException("Error while adding groups: $message");
9498
}
@@ -171,7 +175,7 @@ public function addTeamsAction(Request $request): string
171175
}
172176
$message = null;
173177
if ((($tsvFile && ($result = $this->importExportService->importTsv('teams', $tsvFile, $message))) ||
174-
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
178+
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
175179
$result >= 0) {
176180
// TODO: better return all teams here?
177181
return "$result new team(s) successfully added.";
@@ -235,7 +239,7 @@ public function addAccountsAction(Request $request): string
235239

236240
$message = null;
237241
if ((($tsvFile && ($result = $this->importExportService->importTsv('accounts', $tsvFile, $message))) ||
238-
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
242+
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
239243
$result >= 0) {
240244
// TODO: better return all accounts here?
241245
return "$result new account(s) successfully added.";
@@ -291,13 +295,12 @@ public function singleAction(Request $request, string $id): Response
291295
* Add a new user.
292296
*/
293297
#[IsGranted('ROLE_API_WRITER')]
294-
#[Rest\Post]
298+
#[Rest\Post()]
295299
#[OA\RequestBody(
296300
required: true,
297301
content: [
298-
new OA\MediaType(
299-
mediaType: 'multipart/form-data',
300-
schema: new OA\Schema(ref: new Model(type: AddUser::class))
302+
new OA\JsonContent(
303+
ref: new Model(type: AddUser::class)
301304
),
302305
]
303306
)]
@@ -307,86 +310,86 @@ public function singleAction(Request $request, string $id): Response
307310
content: new Model(type: User::class)
308311
)]
309312
public function addAction(
310-
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
313+
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST, validationGroups: ['add'])]
311314
AddUser $addUser,
312315
Request $request
313316
): Response {
314317
return $this->addOrUpdateUser($addUser, $request);
315318
}
316319

317320
/**
318-
* Update an existing User or create one with the given ID
321+
* Update an existing User
319322
*/
320323
#[IsGranted('ROLE_API_WRITER')]
321-
#[Rest\Put('/{id}')]
324+
#[Rest\Patch('/{id}')]
322325
#[OA\RequestBody(
323326
required: true,
324327
content: [
325-
new OA\MediaType(
326-
mediaType: 'multipart/form-data',
327-
schema: new OA\Schema(ref: new Model(type: UpdateUser::class))
328+
new OA\JsonContent(
329+
ref: new Model(type: UpdateUser::class)
328330
),
329331
]
330332
)]
331333
#[OA\Response(
332334
response: 201,
333-
description: 'Returns the added user',
335+
description: 'Returns the updated user',
334336
content: new Model(type: User::class)
335337
)]
336338
public function updateAction(
337339
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
338-
UpdateUser $updateUser,
340+
UpdateUser $mutateUser,
341+
string $id,
339342
Request $request
340343
): Response {
341-
return $this->addOrUpdateUser($updateUser, $request);
344+
/** @var User|null $user */
345+
$user = $this->em->getRepository(User::class)->findOneBy(['externalid' => $id]);
346+
if ($user === null) {
347+
throw new NotFoundHttpException(sprintf("User with id %s not found", $id));
348+
}
349+
return $this->addOrUpdateUser($mutateUser, $request, $user);
342350
}
343351

344-
protected function addOrUpdateUser(AddUser$addUser, Request $request): Response
352+
protected function addOrUpdateUser(AddUser|UpdateUser$mutateUser, Request $request, ?User$user = null): Response
345353
{
346-
if ($addUser instanceof UpdateUser && !$addUser->id) {
347-
throw new BadRequestHttpException('`id` field is required');
354+
// if the user to update is not set, create a new user
355+
if (!$user) {
356+
$user = new User();
348357
}
349-
350-
if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
351-
throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
358+
if ($mutateUser->username !== null) {
359+
$user->setUsername($mutateUser->username);
352360
}
353-
354-
$user = new User();
355-
if ($addUser instanceof UpdateUser) {
356-
$existingUser = $this->em->getRepository(User::class)->findOneBy(['externalid' => $addUser->id]);
357-
if ($existingUser) {
358-
$user = $existingUser;
359-
}
361+
if ($mutateUser->name !== null) {
362+
$user->setName($mutateUser->name);
360363
}
361-
$user
362-
->setUsername($addUser->username)
363-
->setName($addUser->name)
364-
->setEmail($addUser->email)
365-
->setIpAddress($addUser->ip)
366-
->setPlainPassword($addUser->password)
367-
->setEnabled($addUser->enabled ?? true);
368-
369-
if ($addUser instanceof UpdateUser) {
370-
$user->setExternalid($addUser->id);
364+
if ($mutateUser->email !== null) {
365+
$user->setEmail($mutateUser->email);
371366
}
372-
373-
if ($addUser->teamId) {
367+
if ($mutateUser->ip !== null) {
368+
$user->setIpAddress($mutateUser->ip);
369+
}
370+
if ($mutateUser->password !== null) {
371+
$user->setPlainPassword($mutateUser->password);
372+
}
373+
if ($mutateUser->enabled !== null) {
374+
$user->setEnabled($mutateUser->enabled);
375+
}
376+
if ($mutateUser->teamId) {
374377
/** @var Team|null $team */
375378
$team = $this->em->createQueryBuilder()
376379
->from(Team::class, 't')
377380
->select('t')
378381
->andWhere('t.externalid = :team')
379-
->setParameter('team', $addUser->teamId)
382+
->setParameter('team', $mutateUser->teamId)
380383
->getQuery()
381384
->getOneOrNullResult();
382385

383386
if ($team === null) {
384-
throw new BadRequestHttpException(sprintf("Team %s not found", $addUser->teamId));
387+
throw new BadRequestHttpException(sprintf("Team %s not found", $mutateUser->teamId));
385388
}
386389
$user->setTeam($team);
387390
}
388391

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

414+
$validation = $this->validator->validate($user);
415+
if (count($validation) > 0) {
416+
throw new ValidationFailedException($user, $validation);
417+
}
418+
411419
$this->em->persist($user);
412420
$this->em->flush();
413-
$this->dj->auditlog('user', $user->getUserid(), 'added');
421+
$this->dj->auditlog('user', $user->getUserid(), 'updated');
414422

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

418426
protected function getQueryBuilder(Request $request): QueryBuilder
419427
{
420428
$queryBuilder = $this->em->createQueryBuilder()
421-
->from(User::class, 'u')
422-
->select('u');
429+
->from(User::class, 'u')
430+
->select('u');
423431

424432
if ($request->query->has('team')) {
425433
$queryBuilder

‎webapp/src/DataTransferObject/AddUser.php‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,24 @@
44

55
use JMS\Serializer\Annotation as Serializer;
66
use OpenApi\Attributes as OA;
7+
use Symfony\Component\Validator\Constraints as Assert;
78

8-
#[OA\Schema(required: ['username', 'name', 'roles'])]
99
class AddUser
1010
{
11+
#[Assert\NotBlank]
12+
public string $username;
13+
#[Assert\NotBlank]
14+
public string $name;
15+
public ?string $id = null;
16+
public ?string $email = null;
17+
public ?string $ip = null;
18+
#[OA\Property(format: 'password')]
19+
public ?string $password = null;
20+
public ?bool $enabled = null;
21+
public ?string $teamId = null;
1122
/**
12-
* @param array<string> $roles
23+
* @var array<string>
1324
*/
14-
public function __construct(
15-
public readonly string $username,
16-
public readonly string $name,
17-
#[OA\Property(format: 'email', nullable: true)]
18-
public readonly ?string $email,
19-
#[OA\Property(nullable: true)]
20-
public readonly ?string $ip,
21-
#[OA\Property(format: 'password', nullable: true)]
22-
public readonly ?string $password,
23-
#[OA\Property(nullable: true)]
24-
public readonly ?bool $enabled,
25-
#[OA\Property(nullable: true)]
26-
public readonly ?string $teamId,
27-
#[Serializer\Type('array<string>')]
28-
public readonly array $roles,
29-
) {}
25+
#[Serializer\Type('array<string>')]
26+
public array $roles = [];
3027
}

‎webapp/src/DataTransferObject/UpdateUser.php‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,22 +2,23 @@
22

33
namespace App\DataTransferObject;
44

5+
use JMS\Serializer\Annotation as Serializer;
56
use OpenApi\Attributes as OA;
67

7-
#[OA\Schema(required: ['id', 'username', 'name', 'roles'])]
8-
class UpdateUser extends AddUser
8+
class UpdateUser
99
{
10-
public function __construct(
11-
public readonly string $id,
12-
string $username,
13-
string $name,
14-
?string $email,
15-
?string $ip,
16-
?string $password,
17-
?bool $enabled,
18-
?string $teamId,
19-
array $roles
20-
) {
21-
parent::__construct($username, $name, $email, $ip, $password, $enabled, $teamId, $roles);
22-
}
10+
public ?string $id = null;
11+
public ?string $username = null;
12+
public ?string $name = null;
13+
public ?string $email = null;
14+
public ?string $ip = null;
15+
#[OA\Property(format: 'password')]
16+
public ?string $password = null;
17+
public ?bool $enabled = null;
18+
public ?string $teamId = null;
19+
/**
20+
* @var array<string>
21+
*/
22+
#[Serializer\Type('array<string>')]
23+
public array $roles = [];
2324
}

‎webapp/src/Entity/User.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<?php declare(strict_types=1);
2+
23
namespace App\Entity;
34

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

@@ -439,9 +440,14 @@ public function getType(): ?string
439440
// Types allowed by the CCS Specs Contest API in order of most permissions to least.
440441
// Either key=>value where key is the DOMjudge role and value is the API account type or
441442
// only value, where both the DOMjudge role and API type are the same.
442-
$allowedTypes = ['admin', 'api_writer' => 'admin', 'api_reader' => 'admin',
443-
'jury' => 'judge', 'api_source_reader' => 'judge',
444-
'team'];
443+
$allowedTypes = [
444+
'admin',
445+
'api_writer' => 'admin',
446+
'api_reader' => 'admin',
447+
'jury' => 'judge',
448+
'api_source_reader' => 'judge',
449+
'team'
450+
];
445451
foreach ($allowedTypes as $role => $allowedType) {
446452
if (is_numeric($role)) {
447453
$role = $allowedType;
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace App\EventListener;
4+
5+
use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
6+
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
7+
use Symfony\Component\Validator\Exception\ValidationFailedException;
8+
use Symfony\Component\HttpFoundation\JsonResponse;
9+
10+
#[AsEventListener(event: 'kernel.exception', priority: 0)]
11+
class ValidationExceptionListener
12+
{
13+
public function __invoke(ExceptionEvent $event): void
14+
{
15+
$e = $event->getThrowable();
16+
17+
if (!$e instanceof ValidationFailedException) {
18+
return;
19+
}
20+
21+
$errors = [];
22+
foreach ($e->getViolations() as $violation) {
23+
$errors[] = [
24+
'property' => $violation->getPropertyPath(),
25+
'message' => $violation->getMessage(),
26+
'code' => $violation->getCode(),
27+
];
28+
}
29+
30+
$response = new JsonResponse([
31+
'title' => 'Bad Request',
32+
'status' => 400,
33+
'errors' => $errors,
34+
], 400);
35+
36+
$event->setResponse($response);
37+
}
38+
}

0 commit comments

Comments
(0)

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