Skip to content

Commit 7d5bcfe

Browse files
LuukBlankenstijnLuukBlankenstijn
committed
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 e81837d commit 7d5bcfe

File tree

5 files changed

+163
-100
lines changed

5 files changed

+163
-100
lines changed

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

Lines changed: 73 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
<?php declare(strict_types=1);
1+
<?php
2+
3+
declare(strict_types=1);
24

35
namespace App\Controller\API;
46

@@ -23,7 +25,10 @@
2325
use Symfony\Component\HttpFoundation\Response;
2426
use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
2527
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
28+
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
2629
use Symfony\Component\Security\Http\Attribute\IsGranted;
30+
use Symfony\Component\Validator\Exception\ValidationFailedException;
31+
use Symfony\Component\Validator\Validator\ValidatorInterface;
2732

2833
/**
2934
* @extends AbstractRestController<User, User>
@@ -41,7 +46,8 @@ public function __construct(
4146
DOMJudgeService $dj,
4247
ConfigurationService $config,
4348
EventLogService $eventLogService,
44-
protected readonly ImportExportService $importExportService
49+
protected readonly ImportExportService $importExportService,
50+
protected readonly ValidatorInterface $validator,
4551
) {
4652
parent::__construct($entityManager, $dj, $config, $eventLogService);
4753
}
@@ -86,9 +92,10 @@ public function addGroupsAction(Request $request): string
8692
$message = null;
8793
$result = -1;
8894
if ((($tsvFile && ($result = $this->importExportService->importTsv('groups', $tsvFile, $message))) ||
89-
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
90-
$result >= 0) {
91-
return "$result new group(s) successfully added.";
95+
($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
96+
$result >= 0
97+
) {
98+
return "$result new group(s) successfully added.";
9299
} else {
93100
throw new BadRequestHttpException("Error while adding groups: $message");
94101
}
@@ -110,7 +117,8 @@ public function addGroupsAction(Request $request): string
110117
property: 'json',
111118
description: 'The organizations.json files to import.',
112119
type: 'string',
113-
format: 'binary'),
120+
format: 'binary'
121+
),
114122
]
115123
)
116124
)
@@ -121,7 +129,8 @@ public function addOrganizationsAction(Request $request): string
121129
$message = null;
122130
/** @var UploadedFile|null $jsonFile */
123131
$jsonFile = $request->files->get('json');
124-
if ($jsonFile &&
132+
if (
133+
$jsonFile &&
125134
($result = $this->importExportService->importJson('organizations', $jsonFile, $message)) &&
126135
$result >= 0
127136
) {
@@ -171,8 +180,9 @@ public function addTeamsAction(Request $request): string
171180
}
172181
$message = null;
173182
if ((($tsvFile && ($result = $this->importExportService->importTsv('teams', $tsvFile, $message))) ||
174-
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
175-
$result >= 0) {
183+
($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
184+
$result >= 0
185+
) {
176186
// TODO: better return all teams here?
177187
return "$result new team(s) successfully added.";
178188
} else {
@@ -235,8 +245,9 @@ public function addAccountsAction(Request $request): string
235245

236246
$message = null;
237247
if ((($tsvFile && ($result = $this->importExportService->importTsv('accounts', $tsvFile, $message))) ||
238-
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
239-
$result >= 0) {
248+
($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
249+
$result >= 0
250+
) {
240251
// TODO: better return all accounts here?
241252
return "$result new account(s) successfully added.";
242253
} else {
@@ -259,11 +270,13 @@ public function addAccountsAction(Request $request): string
259270
)
260271
)]
261272
#[OA\Parameter(ref: '#/components/parameters/idlist')]
262-
#[OA\Parameter(
263-
name: 'team_id',
264-
description: 'Only show users for the given team',
265-
in: 'query',
266-
schema: new OA\Schema(type: 'string'))
273+
#[
274+
OA\Parameter(
275+
name: 'team_id',
276+
description: 'Only show users for the given team',
277+
in: 'query',
278+
schema: new OA\Schema(type: 'string')
279+
)
267280
]
268281
public function listAction(Request $request): Response
269282
{
@@ -291,13 +304,12 @@ public function singleAction(Request $request, string $id): Response
291304
* Add a new user.
292305
*/
293306
#[IsGranted('ROLE_API_WRITER')]
294-
#[Rest\Post]
307+
#[Rest\Post()]
295308
#[OA\RequestBody(
296309
required: true,
297310
content: [
298-
new OA\MediaType(
299-
mediaType: 'multipart/form-data',
300-
schema: new OA\Schema(ref: new Model(type: AddUser::class))
311+
new OA\JsonContent(
312+
ref: new Model(type: AddUser::class)
301313
),
302314
]
303315
)]
@@ -307,24 +319,23 @@ public function singleAction(Request $request, string $id): Response
307319
content: new Model(type: User::class)
308320
)]
309321
public function addAction(
310-
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
322+
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST, validationGroups: ['add'])]
311323
AddUser $addUser,
312324
Request $request
313325
): Response {
314-
return $this->addOrUpdateUser($addUser, $request);
326+
return $this->addOrUpdateUser($addUser, new User(), $request);
315327
}
316328

317329
/**
318-
* Update an existing User or create one with the given ID
330+
* Update an existing User
319331
*/
320332
#[IsGranted('ROLE_API_WRITER')]
321-
#[Rest\Put('/{id}')]
333+
#[Rest\Patch('/{id}')]
322334
#[OA\RequestBody(
323335
required: true,
324336
content: [
325-
new OA\MediaType(
326-
mediaType: 'multipart/form-data',
327-
schema: new OA\Schema(ref: new Model(type: UpdateUser::class))
337+
new OA\JsonContent(
338+
ref: new Model(type: UpdateUser::class)
328339
),
329340
]
330341
)]
@@ -335,58 +346,55 @@ public function addAction(
335346
)]
336347
public function updateAction(
337348
#[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)]
338-
UpdateUser $updateUser,
349+
UpdateUser $mutateUser,
350+
string $id,
339351
Request $request
340352
): Response {
341-
return $this->addOrUpdateUser($updateUser, $request);
353+
/** @var User|null $user */
354+
$user = $this->em->getRepository(User::class)->findOneBy(['externalid' => $id]);
355+
if ($user === null) {
356+
throw new NotFoundHttpException(sprintf("User with id %s not found", $id));
357+
}
358+
return $this->addOrUpdateUser($mutateUser, $user, $request);
342359
}
343360

344-
protected function addOrUpdateUser(AddUser $addUser, Request $request): Response
361+
protected function addOrUpdateUser(AddUser|UpdateUser $mutateUser, User $user, Request $request): Response
345362
{
346-
if ($addUser instanceof UpdateUser && !$addUser->id) {
347-
throw new BadRequestHttpException('`id` field is required');
363+
if ($mutateUser->username !== null) {
364+
$user->setUsername($mutateUser->username);
348365
}
349-
350-
if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
351-
throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
366+
if ($mutateUser->name !== null) {
367+
$user->setName($mutateUser->name);
352368
}
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-
}
369+
if ($mutateUser->email !== null) {
370+
$user->setEmail($mutateUser->email);
360371
}
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);
372+
if ($mutateUser->ip !== null) {
373+
$user->setIpAddress($mutateUser->ip);
371374
}
372-
373-
if ($addUser->teamId) {
375+
if ($mutateUser->password !== null) {
376+
$user->setPlainPassword($mutateUser->password);
377+
}
378+
if ($mutateUser->enabled !== null) {
379+
$user->setEnabled($mutateUser->enabled);
380+
}
381+
if ($mutateUser->teamId) {
374382
/** @var Team|null $team */
375383
$team = $this->em->createQueryBuilder()
376384
->from(Team::class, 't')
377385
->select('t')
378386
->andWhere('t.externalid = :team')
379-
->setParameter('team', $addUser->teamId)
387+
->setParameter('team', $mutateUser->teamId)
380388
->getQuery()
381389
->getOneOrNullResult();
382390

383391
if ($team === null) {
384-
throw new BadRequestHttpException(sprintf("Team %s not found", $addUser->teamId));
392+
throw new BadRequestHttpException(sprintf("Team %s not found", $mutateUser->teamId));
385393
}
386394
$user->setTeam($team);
387395
}
388396

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

419+
$validation = $this->validator->validate($user);
420+
if (count($validation) > 0) {
421+
throw new ValidationFailedException($user, $validation);
422+
}
423+
411424
$this->em->persist($user);
412425
$this->em->flush();
413-
$this->dj->auditlog('user', $user->getUserid(), 'added');
426+
$this->dj->auditlog('user', $user->getUserid(), 'updated');
414427

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

418431
protected function getQueryBuilder(Request $request): QueryBuilder
419432
{
420433
$queryBuilder = $this->em->createQueryBuilder()
421-
->from(User::class, 'u')
422-
->select('u');
434+
->from(User::class, 'u')
435+
->select('u');
423436

424437
if ($request->query->has('team')) {
425438
$queryBuilder

‎webapp/src/DataTransferObject/AddUser.php‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,29 @@
1-
<?php declare(strict_types=1);
1+
<?php
2+
3+
declare(strict_types=1);
24

35
namespace App\DataTransferObject;
46

57
use JMS\Serializer\Annotation as Serializer;
68
use OpenApi\Attributes as OA;
9+
use Symfony\Component\Validator\Constraints as Assert;
710

8-
#[OA\Schema(required: ['username', 'name', 'roles'])]
911
class AddUser
1012
{
13+
#[Assert\NotBlank]
14+
public string $username;
15+
#[Assert\NotBlank]
16+
public string $name;
17+
public ?string $id = null;
18+
public ?string $email = null;
19+
public ?string $ip = null;
20+
#[OA\Property(format: 'password')]
21+
public ?string $password = null;
22+
public ?bool $enabled = null;
23+
public ?string $teamId = null;
1124
/**
12-
* @param array<string> $roles
25+
* @var array<string>
1326
*/
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-
) {}
27+
#[Serializer\Type('array<string>')]
28+
public array $roles = [];
3029
}

‎webapp/src/DataTransferObject/UpdateUser.php‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,26 @@
1-
<?php declare(strict_types=1);
1+
<?php
2+
3+
declare(strict_types=1);
24

35
namespace App\DataTransferObject;
46

7+
use JMS\Serializer\Annotation as Serializer;
58
use OpenApi\Attributes as OA;
69

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

‎webapp/src/Entity/User.php‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
<?php declare(strict_types=1);
1+
<?php
2+
3+
declare(strict_types=1);
4+
25
namespace App\Entity;
36

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

@@ -439,9 +442,14 @@ public function getType(): ?string
439442
// Types allowed by the CCS Specs Contest API in order of most permissions to least.
440443
// Either key=>value where key is the DOMjudge role and value is the API account type or
441444
// 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'];
445+
$allowedTypes = [
446+
'admin',
447+
'api_writer' => 'admin',
448+
'api_reader' => 'admin',
449+
'jury' => 'judge',
450+
'api_source_reader' => 'judge',
451+
'team'
452+
];
445453
foreach ($allowedTypes as $role => $allowedType) {
446454
if (is_numeric($role)) {
447455
$role = $allowedType;

0 commit comments

Comments
 (0)