From 391e119990787d5fc522a628101c46bf36bcfb7f Mon Sep 17 00:00:00 2001 From: Nicky Gerritsen Date: Tue, 19 Mar 2024 14:04:20 +0100 Subject: [PATCH 1/4] Support external ID's when POSTing users. Fixes #2186. --- webapp/src/Controller/API/UserController.php | 5 ++ webapp/src/DataTransferObject/AddUser.php | 2 + .../Controller/API/UserControllerTest.php | 50 +++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/webapp/src/Controller/API/UserController.php b/webapp/src/Controller/API/UserController.php index 2e3dc496a0..995e3b8aab 100644 --- a/webapp/src/Controller/API/UserController.php +++ b/webapp/src/Controller/API/UserController.php @@ -307,12 +307,17 @@ public function addAction( AddUser $addUser, Request $request ): Response { + if ($this->eventLogService->externalIdFieldForEntity(User::class) && !$addUser->id) { + throw new BadRequestHttpException('`id` field is required'); + } + if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) { throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username)); } $user = new User(); $user + ->setExternalid($addUser->id) ->setUsername($addUser->username) ->setName($addUser->name) ->setEmail($addUser->email) diff --git a/webapp/src/DataTransferObject/AddUser.php b/webapp/src/DataTransferObject/AddUser.php index 9a26af0cdf..205820d9ac 100644 --- a/webapp/src/DataTransferObject/AddUser.php +++ b/webapp/src/DataTransferObject/AddUser.php @@ -12,6 +12,8 @@ class AddUser * @param array $roles */ public function __construct( + #[OA\Property(nullable: true)] + public readonly ?string $id, public readonly string $username, public readonly string $name, #[OA\Property(format: 'email', nullable: true)] diff --git a/webapp/tests/Unit/Controller/API/UserControllerTest.php b/webapp/tests/Unit/Controller/API/UserControllerTest.php index 0a7d930daa..c748b63419 100644 --- a/webapp/tests/Unit/Controller/API/UserControllerTest.php +++ b/webapp/tests/Unit/Controller/API/UserControllerTest.php @@ -2,6 +2,8 @@ namespace App\Tests\Unit\Controller\API; +use App\Service\DOMJudgeService; + class UserControllerTest extends AccountBaseTestCase { protected ?string $apiEndpoint = 'users'; @@ -45,4 +47,52 @@ class UserControllerTest extends AccountBaseTestCase "enabled" => true ], ]; + + public function testAddLocal(): void + { + $data = [ + 'username' => 'testuser', + 'name' => 'Test User', + 'roles' => ['team'], + 'password' => 'testpassword', + ]; + + $response = $this->verifyApiJsonResponse('POST', $this->helperGetEndpointURL($this->apiEndpoint), 201, 'admin', $data); + static::assertArrayHasKey('id', $response); + static::assertEquals('testuser', $response['username']); + static::assertEquals('Test User', $response['name']); + static::assertEquals(['team'], $response['roles']); + } + + public function testAddNonLocal(): void + { + $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); + $data = [ + 'id' => 'someid', + 'username' => 'testuser', + 'name' => 'Test User', + 'roles' => ['team'], + 'password' => 'testpassword', + ]; + + $response = $this->verifyApiJsonResponse('POST', $this->helperGetEndpointURL($this->apiEndpoint), 201, 'admin', $data); + static::assertEquals('someid', $response['id']); + static::assertEquals('testuser', $response['username']); + static::assertEquals('Test User', $response['name']); + static::assertEquals(['team'], $response['roles']); + } + + public function testAddNonLocalNoId(): void + { + $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); + $data = [ + 'username' => 'testuser', + 'name' => 'Test User', + 'roles' => ['team'], + 'password' => 'testpassword', + ]; + + $response = $this->verifyApiJsonResponse('POST', $this->helperGetEndpointURL($this->apiEndpoint), 400, 'admin', $data); + static::assertStringContainsString('`id` field is required', $response['message']); + } } From 55de58b7543ae5d807edb8c08a117079e1f7469e Mon Sep 17 00:00:00 2001 From: Nicky Gerritsen Date: Tue, 19 Mar 2024 14:25:23 +0100 Subject: [PATCH 2/4] Allow PUT for groups. Fixes #2381. --- webapp/src/Controller/API/GroupController.php | 29 ++++++---- .../DataTransferObject/TeamCategoryPost.php | 2 + .../Controller/API/GroupControllerTest.php | 55 +++++++++++++++++++ 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/webapp/src/Controller/API/GroupController.php b/webapp/src/Controller/API/GroupController.php index 93cbd76f04..ebc0e897f2 100644 --- a/webapp/src/Controller/API/GroupController.php +++ b/webapp/src/Controller/API/GroupController.php @@ -73,6 +73,7 @@ public function singleAction(Request $request, string $id): Response */ #[IsGranted('ROLE_API_WRITER')] #[Rest\Post] + #[Rest\Put('/{id}')] #[OA\RequestBody( required: true, content: [ @@ -91,19 +92,25 @@ public function addAction( #[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)] TeamCategoryPost $teamCategoryPost, Request $request, - ImportExportService $importExport + ImportExportService $importExport, + ?string $id = null ): Response { $saved = []; - $importExport->importGroupsJson([ - [ - 'name' => $teamCategoryPost->name, - 'hidden' => $teamCategoryPost->hidden, - 'icpc_id' => $teamCategoryPost->icpcId, - 'sortorder' => $teamCategoryPost->sortorder, - 'color' => $teamCategoryPost->color, - 'allow_self_registration' => $teamCategoryPost->allowSelfRegistration, - ], - ], $message, $saved); + $groupData = [ + 'name' => $teamCategoryPost->name, + 'hidden' => $teamCategoryPost->hidden, + 'icpc_id' => $teamCategoryPost->icpcId, + 'sortorder' => $teamCategoryPost->sortorder, + 'color' => $teamCategoryPost->color, + 'allow_self_registration' => $teamCategoryPost->allowSelfRegistration, + ]; + if ($id !== null) { + if ($id !== $teamCategoryPost->id) { + throw new BadRequestHttpException('ID in URL does not match ID in payload'); + } + $groupData['id'] = $id; + } + $importExport->importGroupsJson([$groupData], $message, $saved); if (!empty($message)) { throw new BadRequestHttpException("Error while adding group: $message"); } diff --git a/webapp/src/DataTransferObject/TeamCategoryPost.php b/webapp/src/DataTransferObject/TeamCategoryPost.php index 9225d6ed69..8487db47df 100644 --- a/webapp/src/DataTransferObject/TeamCategoryPost.php +++ b/webapp/src/DataTransferObject/TeamCategoryPost.php @@ -8,6 +8,8 @@ class TeamCategoryPost { public function __construct( + #[OA\Property(description: 'The ID of the group. Only allowed with PUT requests', nullable: true)] + public readonly ?string $id, #[OA\Property(description: 'How to name this group on the scoreboard')] public readonly string $name, #[OA\Property(description: 'Show this group on the scoreboard?')] diff --git a/webapp/tests/Unit/Controller/API/GroupControllerTest.php b/webapp/tests/Unit/Controller/API/GroupControllerTest.php index 6b9053fd37..cf90038274 100644 --- a/webapp/tests/Unit/Controller/API/GroupControllerTest.php +++ b/webapp/tests/Unit/Controller/API/GroupControllerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\Controller\API; +use App\Service\DOMJudgeService; use Generator; class GroupControllerTest extends BaseTestCase @@ -85,6 +86,60 @@ public function testNewAddedGroupPostWithId(): void self::assertNotEquals($returnedObject['id'], $postWithId['id']); } + /** + * @dataProvider provideNewAddedGroup + */ + public function testNewAddedGroupPut(array $newGroupPostData): void + { + // This only works for non-local data sources + $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); + + $url = $this->helperGetEndpointURL($this->apiEndpoint); + $objectsBeforeTest = $this->verifyApiJsonResponse('GET', $url, 200, $this->apiUser); + + $newGroupPostData['id'] = 'someid'; + + $returnedObject = $this->verifyApiJsonResponse('PUT', $url . '/someid', 201, 'admin', $newGroupPostData); + foreach ($newGroupPostData as $key => $value) { + self::assertEquals($value, $returnedObject[$key]); + } + + $objectsAfterTest = $this->verifyApiJsonResponse('GET', $url, 200, $this->apiUser); + $newItems = array_map('unserialize', array_diff(array_map('serialize', $objectsAfterTest), array_map('serialize', $objectsBeforeTest))); + self::assertEquals(1, count($newItems)); + $listKey = array_keys($newItems)[0]; + foreach ($newGroupPostData as $key => $value) { + self::assertEquals($value, $newItems[$listKey][$key]); + } + } + + /** + * @dataProvider provideNewAddedGroup + */ + public function testNewAddedGroupPutWithoutId(array $newGroupPostData): void + { + // This only works for non-local data sources + $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); + + $url = $this->helperGetEndpointURL($this->apiEndpoint); + $returnedObject = $this->verifyApiJsonResponse('PUT', $url . '/someid', 400, 'admin', $newGroupPostData); + self::assertStringContainsString('ID in URL does not match ID in payload', $returnedObject['message']); + } + + /** + * @dataProvider provideNewAddedGroup + */ + public function testNewAddedGroupPutWithDifferentId(array $newGroupPostData): void + { + // This only works for non-local data sources + $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); + + $newGroupPostData['id'] = 'someotherid'; + $url = $this->helperGetEndpointURL($this->apiEndpoint); + $returnedObject = $this->verifyApiJsonResponse('PUT', $url . '/someid', 400, 'admin', $newGroupPostData); + self::assertStringContainsString('ID in URL does not match ID in payload', $returnedObject['message']); + } + public function provideNewAddedGroup(): Generator { foreach ($this->newGroupsPostData as $group) { From 71e4275e2a68a39778b93d21752594e78569ce8d Mon Sep 17 00:00:00 2001 From: Nicky Gerritsen Date: Tue, 19 Mar 2024 16:40:28 +0100 Subject: [PATCH 3/4] Split PUT and POST so we only use the ID for PUT. --- webapp/src/Controller/API/GroupController.php | 59 ++++++++++++++++--- .../DataTransferObject/TeamCategoryPost.php | 2 - .../DataTransferObject/TeamCategoryPut.php | 22 +++++++ 3 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 webapp/src/DataTransferObject/TeamCategoryPut.php diff --git a/webapp/src/Controller/API/GroupController.php b/webapp/src/Controller/API/GroupController.php index ebc0e897f2..5331e66557 100644 --- a/webapp/src/Controller/API/GroupController.php +++ b/webapp/src/Controller/API/GroupController.php @@ -3,6 +3,7 @@ namespace App\Controller\API; use App\DataTransferObject\TeamCategoryPost; +use App\DataTransferObject\TeamCategoryPut; use App\Entity\TeamCategory; use App\Service\ImportExportService; use Doctrine\ORM\NonUniqueResultException; @@ -73,7 +74,6 @@ public function singleAction(Request $request, string $id): Response */ #[IsGranted('ROLE_API_WRITER')] #[Rest\Post] - #[Rest\Put('/{id}')] #[OA\RequestBody( required: true, content: [ @@ -93,7 +93,6 @@ public function addAction( TeamCategoryPost $teamCategoryPost, Request $request, ImportExportService $importExport, - ?string $id = null ): Response { $saved = []; $groupData = [ @@ -104,11 +103,57 @@ public function addAction( 'color' => $teamCategoryPost->color, 'allow_self_registration' => $teamCategoryPost->allowSelfRegistration, ]; - if ($id !== null) { - if ($id !== $teamCategoryPost->id) { - throw new BadRequestHttpException('ID in URL does not match ID in payload'); - } - $groupData['id'] = $id; + $importExport->importGroupsJson([$groupData], $message, $saved); + if (!empty($message)) { + throw new BadRequestHttpException("Error while adding group: $message"); + } + + $group = $saved[0]; + $idField = $this->eventLogService->externalIdFieldForEntity(TeamCategory::class) ?? 'categoryid'; + $method = sprintf('get%s', ucfirst($idField)); + $id = call_user_func([$group, $method]); + + return $this->renderCreateData($request, $saved[0], 'group', $id); + } + + /** + * Update an existing group or create one with the given ID + */ + #[IsGranted('ROLE_API_WRITER')] + #[Rest\Put('/{id}')] + #[OA\RequestBody( + required: true, + content: [ + new OA\MediaType( + mediaType: 'multipart/form-data', + schema: new OA\Schema(ref: new Model(type: TeamCategoryPut::class)) + ), + ] + )] + #[OA\Response( + response: 201, + description: 'Returns the updated / added group', + content: new Model(type: TeamCategory::class) + )] + public function updateAction( + #[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)] + TeamCategoryPut $teamCategoryPut, + Request $request, + ImportExportService $importExport, + string $id, + ): Response { + $saved = []; + $groupData = [ + 'id' => $teamCategoryPut->id, + 'name' => $teamCategoryPut->name, + 'hidden' => $teamCategoryPut->hidden, + 'icpc_id' => $teamCategoryPut->icpcId, + 'sortorder' => $teamCategoryPut->sortorder, + 'color' => $teamCategoryPut->color, + 'allow_self_registration' => $teamCategoryPut->allowSelfRegistration, + ]; + if ($id !== $teamCategoryPut->id) { + throw new BadRequestHttpException('ID in URL does not match ID in payload'); } $importExport->importGroupsJson([$groupData], $message, $saved); if (!empty($message)) { diff --git a/webapp/src/DataTransferObject/TeamCategoryPost.php b/webapp/src/DataTransferObject/TeamCategoryPost.php index 8487db47df..9225d6ed69 100644 --- a/webapp/src/DataTransferObject/TeamCategoryPost.php +++ b/webapp/src/DataTransferObject/TeamCategoryPost.php @@ -8,8 +8,6 @@ class TeamCategoryPost { public function __construct( - #[OA\Property(description: 'The ID of the group. Only allowed with PUT requests', nullable: true)] - public readonly ?string $id, #[OA\Property(description: 'How to name this group on the scoreboard')] public readonly string $name, #[OA\Property(description: 'Show this group on the scoreboard?')] diff --git a/webapp/src/DataTransferObject/TeamCategoryPut.php b/webapp/src/DataTransferObject/TeamCategoryPut.php new file mode 100644 index 0000000000..982f584e25 --- /dev/null +++ b/webapp/src/DataTransferObject/TeamCategoryPut.php @@ -0,0 +1,22 @@ + Date: Tue, 19 Mar 2024 16:51:20 +0100 Subject: [PATCH 4/4] Use PUT endpoint for updating / setting users to be in sync with CLICS. --- webapp/src/Controller/API/UserController.php | 56 ++++++++++++++++--- webapp/src/DataTransferObject/AddUser.php | 2 - webapp/src/DataTransferObject/UpdateUser.php | 23 ++++++++ .../Controller/API/UserControllerTest.php | 10 ++-- 4 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 webapp/src/DataTransferObject/UpdateUser.php diff --git a/webapp/src/Controller/API/UserController.php b/webapp/src/Controller/API/UserController.php index 995e3b8aab..46bd51f2e1 100644 --- a/webapp/src/Controller/API/UserController.php +++ b/webapp/src/Controller/API/UserController.php @@ -3,6 +3,7 @@ namespace App\Controller\API; use App\DataTransferObject\AddUser; +use App\DataTransferObject\UpdateUser; use App\Entity\Role; use App\Entity\Team; use App\Entity\User; @@ -64,7 +65,7 @@ public function __construct( description: 'The groups.json files to import.', type: 'string', format: 'binary' - ) + ), ] ) ) @@ -106,7 +107,7 @@ public function addGroupsAction(Request $request): string property: 'json', description: 'The organizations.json files to import.', type: 'string', - format: 'binary') + format: 'binary'), ] ) ) @@ -150,7 +151,7 @@ public function addOrganizationsAction(Request $request): string description: 'The teams.json files to import.', type: 'string', format: 'binary' - ) + ), ] ) ) @@ -205,7 +206,7 @@ public function addTeamsAction(Request $request): string description: 'The accounts.yaml files to import.', type: 'string', format: 'binary' - ) + ), ] ) ) @@ -294,7 +295,7 @@ public function singleAction(Request $request, string $id): Response new OA\MediaType( mediaType: 'multipart/form-data', schema: new OA\Schema(ref: new Model(type: AddUser::class)) - ) + ), ] )] #[OA\Response( @@ -307,7 +308,39 @@ public function addAction( AddUser $addUser, Request $request ): Response { - if ($this->eventLogService->externalIdFieldForEntity(User::class) && !$addUser->id) { + return $this->addOrUpdateUser($addUser, $request); + } + + /** + * Update an existing User or create one with the given ID + */ + #[IsGranted('ROLE_API_WRITER')] + #[Rest\Put('/{id}')] + #[OA\RequestBody( + required: true, + content: [ + new OA\MediaType( + mediaType: 'multipart/form-data', + schema: new OA\Schema(ref: new Model(type: UpdateUser::class)) + ), + ] + )] + #[OA\Response( + response: 201, + description: 'Returns the added user', + content: new Model(type: User::class) + )] + public function updateAction( + #[MapRequestPayload(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)] + UpdateUser $updateUser, + Request $request + ): Response { + return $this->addOrUpdateUser($updateUser, $request); + } + + protected function addOrUpdateUser(AddUser $addUser, Request $request): Response + { + if ($addUser instanceof UpdateUser && $this->eventLogService->externalIdFieldForEntity(User::class) && !$addUser->id) { throw new BadRequestHttpException('`id` field is required'); } @@ -316,8 +349,13 @@ public function addAction( } $user = new User(); + if ($addUser instanceof UpdateUser) { + $existingUser = $this->em->getRepository(User::class)->findOneBy([$this->eventLogService->externalIdFieldForEntity(User::class) => $addUser->id]); + if ($existingUser) { + $user = $existingUser; + } + } $user - ->setExternalid($addUser->id) ->setUsername($addUser->username) ->setName($addUser->name) ->setEmail($addUser->email) @@ -325,6 +363,10 @@ public function addAction( ->setPlainPassword($addUser->password) ->setEnabled($addUser->enabled ?? true); + if ($addUser instanceof UpdateUser) { + $user->setExternalid($addUser->id); + } + if ($addUser->teamId) { /** @var Team|null $team */ $team = $this->em->createQueryBuilder() diff --git a/webapp/src/DataTransferObject/AddUser.php b/webapp/src/DataTransferObject/AddUser.php index 205820d9ac..9a26af0cdf 100644 --- a/webapp/src/DataTransferObject/AddUser.php +++ b/webapp/src/DataTransferObject/AddUser.php @@ -12,8 +12,6 @@ class AddUser * @param array $roles */ public function __construct( - #[OA\Property(nullable: true)] - public readonly ?string $id, public readonly string $username, public readonly string $name, #[OA\Property(format: 'email', nullable: true)] diff --git a/webapp/src/DataTransferObject/UpdateUser.php b/webapp/src/DataTransferObject/UpdateUser.php new file mode 100644 index 0000000000..ab6e1fd0fb --- /dev/null +++ b/webapp/src/DataTransferObject/UpdateUser.php @@ -0,0 +1,23 @@ +setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); $data = [ @@ -75,14 +75,14 @@ public function testAddNonLocal(): void 'password' => 'testpassword', ]; - $response = $this->verifyApiJsonResponse('POST', $this->helperGetEndpointURL($this->apiEndpoint), 201, 'admin', $data); + $response = $this->verifyApiJsonResponse('PUT', $this->helperGetEndpointURL($this->apiEndpoint) . '/someid', 201, 'admin', $data); static::assertEquals('someid', $response['id']); static::assertEquals('testuser', $response['username']); static::assertEquals('Test User', $response['name']); static::assertEquals(['team'], $response['roles']); } - public function testAddNonLocalNoId(): void + public function testUpdateNonLocalNoId(): void { $this->setupDataSource(DOMJudgeService::DATA_SOURCE_CONFIGURATION_EXTERNAL); $data = [ @@ -92,7 +92,7 @@ public function testAddNonLocalNoId(): void 'password' => 'testpassword', ]; - $response = $this->verifyApiJsonResponse('POST', $this->helperGetEndpointURL($this->apiEndpoint), 400, 'admin', $data); - static::assertStringContainsString('`id` field is required', $response['message']); + $response = $this->verifyApiJsonResponse('PUT', $this->helperGetEndpointURL($this->apiEndpoint) . '/someid', 400, 'admin', $data); + static::assertMatchesRegularExpression('/id:\n.*This value should be of type unknown./', $response['message']); } }