Fix user add and update endpoint logic. The following problems were

LuukBlankenstijn · LuukBlankenstijn · commit 7873dfa83754 · 2025-08-08T19:28:25.000+02:00
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.
diff --git a/webapp/src/Controller/API/UserController.php b/webapp/src/Controller/API/UserController.php
@@ -1,4 +1,6 @@
-<?php declare(strict_types=1);
+<?php
+
+declare(strict_types=1);
 
 namespace App\Controller\API;
 
@@ -23,7 +25,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>
@@ -41,7 +46,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);
     }
@@ -86,9 +92,10 @@ 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)))) &&
-            $result >= 0) {
-             return "$result new group(s) successfully added.";
+                ($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
+            $result >= 0
+        ) {
+            return "$result new group(s) successfully added.";
         } else {
             throw new BadRequestHttpException("Error while adding groups: $message");
         }
@@ -110,7 +117,8 @@ public function addGroupsAction(Request $request): string
                         property: 'json',
                         description: 'The organizations.json files to import.',
                         type: 'string',
-                        format: 'binary'),
+                        format: 'binary'
+                    ),
                 ]
             )
         )
@@ -121,7 +129,8 @@ public function addOrganizationsAction(Request $request): string
         $message = null;
         /** @var UploadedFile|null $jsonFile */
         $jsonFile = $request->files->get('json');
-        if ($jsonFile &&
+        if (
+            $jsonFile &&
             ($result = $this->importExportService->importJson('organizations', $jsonFile, $message)) &&
             $result >= 0
         ) {
@@ -171,8 +180,9 @@ 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)))) &&
-            $result >= 0) {
+                ($jsonFile && ($result = $this->importExportService->importJson('teams', $jsonFile, $message)))) &&
+            $result >= 0
+        ) {
             // TODO: better return all teams here?
             return "$result new team(s) successfully added.";
         } else {
@@ -235,8 +245,9 @@ 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)))) &&
-            $result >= 0) {
+                ($jsonFile && ($result = $this->importExportService->importJson('accounts', $jsonFile, $message)))) &&
+            $result >= 0
+        ) {
             // TODO: better return all accounts here?
             return "$result new account(s) successfully added.";
         } else {
@@ -259,11 +270,13 @@ public function addAccountsAction(Request $request): string
         )
     )]
     #[OA\Parameter(ref: '#/components/parameters/idlist')]
-    #[OA\Parameter(
-        name: 'team_id',
-        description: 'Only show users for the given team',
-        in: 'query',
-        schema: new OA\Schema(type: 'string'))
+    #[
+        OA\Parameter(
+            name: 'team_id',
+            description: 'Only show users for the given team',
+            in: 'query',
+            schema: new OA\Schema(type: 'string')
+        )
     ]
     public function listAction(Request $request): Response
     {
@@ -291,13 +304,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)
             ),
         ]
     )]
@@ -307,24 +319,23 @@ 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);
+        return $this->addOrUpdateUser($addUser, new User(), $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)
             ),
         ]
     )]
@@ -335,58 +346,55 @@ public function addAction(
     )]
     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, $user, $request);
     }
 
-    protected function addOrUpdateUser(AddUser $addUser, Request $request): Response
+    protected function addOrUpdateUser(AddUser|UpdateUser $mutateUser, User $user, Request $request): Response
     {
-        if ($addUser instanceof UpdateUser && !$addUser->id) {
-            throw new BadRequestHttpException('`id` field is required');
+        if ($mutateUser->username !== null) {
+            $user->setUsername($mutateUser->username);
         }
-
-        if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
-            throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
+        if ($mutateUser->name !== null) {
+            $user->setName($mutateUser->name);
         }
-
-        $user = new User();
-        if ($addUser instanceof UpdateUser) {
-            $existingUser = $this->em->getRepository(User::class)->findOneBy(['externalid' => $addUser->id]);
-            if ($existingUser) {
-                $user = $existingUser;
-            }
+        if ($mutateUser->email !== null) {
+            $user->setEmail($mutateUser->email);
         }
-        $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->ip !== null) {
+            $user->setIpAddress($mutateUser->ip);
         }
-
-        if ($addUser->teamId) {
+        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'];
@@ -408,18 +416,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
diff --git a/webapp/src/DataTransferObject/AddUser.php b/webapp/src/DataTransferObject/AddUser.php
@@ -1,30 +1,29 @@
-<?php declare(strict_types=1);
+<?php
+
+declare(strict_types=1);
 
 namespace App\DataTransferObject;
 
 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 = [];
 }
diff --git a/webapp/src/DataTransferObject/UpdateUser.php b/webapp/src/DataTransferObject/UpdateUser.php
@@ -1,23 +1,26 @@
-<?php declare(strict_types=1);
+<?php
+
+declare(strict_types=1);
 
 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 = [];
 }
diff --git a/webapp/src/Entity/User.php b/webapp/src/Entity/User.php
@@ -1,4 +1,7 @@
-<?php declare(strict_types=1);
+<?php
+
+declare(strict_types=1);
+
 namespace App\Entity;
 
 use App\Controller\API\AbstractRestController as ARC;
@@ -132,7 +135,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;
 
@@ -439,9 +442,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;
diff --git a/webapp/src/EventListener/ValidationExceptionListener.php b/webapp/src/EventListener/ValidationExceptionListener.php
