diff --git a/src/Controller/API/Users.php b/src/Controller/API/Users.php index 092865ad91..89539afdaf 100644 --- a/src/Controller/API/Users.php +++ b/src/Controller/API/Users.php @@ -14,6 +14,7 @@ use OpenApi\Attributes as OA; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; @@ -345,8 +346,13 @@ public function put( /** @var SessionUserInterface $sessionUser */ $sessionUser = $this->tokenStorage->getToken()->getUser(); if ( - $obj->root && - (!$sessionUser->isRoot() && !$entity->isRoot()) + //adding root by a non root user + ($obj->root && + (!$sessionUser->isRoot() && !$entity->isRoot())) || + + //removing root be a non root user + (!$obj->root && + (!$sessionUser->isRoot() && $entity->isRoot())) ) { throw new AccessDeniedException('Unauthorized access!'); } @@ -369,6 +375,27 @@ public function patch( AuthorizationCheckerInterface $authorizationChecker, ApiResponseBuilder $builder ): Response { + $type = $request->getAcceptableContentTypes(); + if (!in_array("application/vnd.api+json", $type)) { + throw new BadRequestHttpException("PATCH is only allowed for JSON:API requests, use PUT instead"); + } + $entity = $this->repository->findOneBy(['id' => $id]); + if ($entity) { + $arr = $requestParser->extractJsonApiPatchDataFromRequest($request); + /** @var SessionUserInterface $sessionUser */ + $sessionUser = $this->tokenStorage->getToken()->getUser(); + if ( + //adding root by a non root user + ($arr['root'] && + (!$sessionUser->isRoot() && !$entity->isRoot())) || + + //removing root be a non root user + (!$arr['root'] && + (!$sessionUser->isRoot() && $entity->isRoot())) + ) { + throw new AccessDeniedException('Unauthorized access!'); + } + } return $this->handlePatch($version, $id, $request, $requestParser, $validator, $authorizationChecker, $builder); } diff --git a/src/Service/ApiRequestParser.php b/src/Service/ApiRequestParser.php index f34fd4acc8..28bb74ec74 100644 --- a/src/Service/ApiRequestParser.php +++ b/src/Service/ApiRequestParser.php @@ -190,8 +190,7 @@ public function extractEntityFromPutRequest(Request $request, object $entity, st { $type = $request->getAcceptableContentTypes(); if (in_array("application/vnd.api+json", $type)) { - $obj = json_decode($request->getContent()); - $data = $this->dataShaper->flattenJsonApiData($obj->data); + $data = $this->extractJsonApiPatchDataFromRequest($request); } else { $data = $this->extractPutDataFromRequest($request, $object); } @@ -199,4 +198,13 @@ public function extractEntityFromPutRequest(Request $request, object $entity, st return $this->serializer->deserialize($json, $entity::class, 'json', ['object_to_populate' => $entity]); } + + /** + * Parse json:api PATCH request and pull out data, + */ + public function extractJsonApiPatchDataFromRequest(Request $request): array + { + $obj = json_decode($request->getContent()); + return $this->dataShaper->flattenJsonApiData($obj->data); + } } diff --git a/tests/Endpoints/AbstractEndpoint.php b/tests/Endpoints/AbstractEndpoint.php index 0974af4492..90741c1315 100644 --- a/tests/Endpoints/AbstractEndpoint.php +++ b/tests/Endpoints/AbstractEndpoint.php @@ -1073,7 +1073,7 @@ protected function putOne( /** * PUT a single item to the JSON:API */ - protected function patchOneJsonApi(object $data): object + protected function patchOneJsonApi(object $data, $userId = 2): object { $endpoint = strtolower($data->data->type); $this->createJsonApiRequest( @@ -1084,7 +1084,7 @@ protected function patchOneJsonApi(object $data): object ['version' => $this->apiVersion, 'id' => $data->data->id] ), json_encode($data), - $this->getAuthenticatedUserToken($this->kernelBrowser) + $this->getTokenForUser($this->kernelBrowser, $userId) ); $response = $this->kernelBrowser->getResponse(); $this->assertJsonApiResponse($response, Response::HTTP_OK); diff --git a/tests/Endpoints/UserTest.php b/tests/Endpoints/UserTest.php index 1ede4bb703..f1fe924af5 100644 --- a/tests/Endpoints/UserTest.php +++ b/tests/Endpoints/UserTest.php @@ -327,31 +327,70 @@ public function testPostRootUserAsRootUser() $this->assertJsonResponse($response, Response::HTTP_CREATED); } - public function testRejectUnprivilegedChangeUserToRoot() + public function testRejectUnprivilegedPutUserToRoot() { $dataLoader = $this->getDataLoader(); + + // create a new user with admin permissions in school 1, who is not root + $newUser = $dataLoader->create(); + $newUser['administeredSchools'] = [1]; + $administrator = $this->postOne('users', 'user', 'users', $newUser); + $this->assertFalse($administrator['root'], 'Administrator must not be root or this test is garbage'); + $this->assertContains('1', $administrator['administeredSchools']); + $all = $dataLoader->getAll(); - $user = $all[2]; - $this->assertFalse($user['root'], 'User #3 is supposed to not be root or this test is garbage'); - $userId = $user['id']; + $user3 = $all[2]; - $postData = $user; - $postData['root'] = true; + // ensure our new user *can* make some changes to user3 + $this->putOne('users', 'user', $user3['id'], $user3, false, $administrator['id']); + + $user3['root'] = true; $this->canNot( $this->kernelBrowser, - $userId, + $administrator['id'], 'PUT', $this->getUrl( $this->kernelBrowser, 'app_api_users_put', - ['version' => $this->apiVersion, 'id' => $postData['id']] + ['version' => $this->apiVersion, 'id' => $user3['id']] ), - json_encode(['user' => $postData]) + json_encode(['user' => $user3]) ); } + public function testRejectUnprivilegedPatchUserToRoot() + { + $dataLoader = $this->getDataLoader(); - public function testRejectUnprivilegedAddDeveloperRoleToOwnAccount() + // create a new user with admin permissions in school 1, who is not root + $newUser = $dataLoader->create(); + $newUser['administeredSchools'] = [1]; + $administrator = $this->postOne('users', 'user', 'users', $newUser); + $this->assertFalse($administrator['root'], 'Administrator must not be root or this test is garbage'); + $this->assertContains('1', $administrator['administeredSchools']); + + $all = $dataLoader->getAll(); + $user3 = $all[2]; + + // ensure our new user *can* make some changes to user3 + $this->patchOneJsonApi($dataLoader->createJsonApi($user3), $administrator['id']); + + $user3['root'] = true; + + $this->canNotJsonApi( + $this->kernelBrowser, + $administrator['id'], + 'PATCH', + $this->getUrl( + $this->kernelBrowser, + 'app_api_users_put', + ['version' => $this->apiVersion, 'id' => $user3['id']] + ), + json_encode($dataLoader->createJsonApi($user3)) + ); + } + + public function testRejectUnprivilegedPutDeveloperRoleToOwnAccount() { $dataLoader = $this->getDataLoader(); $all = $dataLoader->getAll(); @@ -375,33 +414,72 @@ public function testRejectUnprivilegedAddDeveloperRoleToOwnAccount() ); } - public function testRejectUnprivilegedRemoveRootFromUser() + public function testRejectUnprivilegedPutRemoveRootFromUser() { $dataLoader = $this->getDataLoader(); + + // create a new user with admin permissions in school 1, who is not root + $newUser = $dataLoader->create(); + $newUser['administeredSchools'] = [1]; + $administrator = $this->postOne('users', 'user', 'users', $newUser); + $this->assertFalse($administrator['root'], 'Administrator must not be root or this test is garbage'); + $this->assertContains('1', $administrator['administeredSchools']); + $all = $dataLoader->getAll(); - $user = $all[2]; - $this->assertFalse($user['root'], 'User #3 is supposed to not be root or this test is garbage'); - $userId = $user['id']; + $user2 = $all[1]; - $postData = $all[1]; - $this->assertTrue($postData['root'], 'User #2 is supposed to be root or this test is garbage'); + // ensure our new user *can* make some changes to user3 + $this->putOne('users', 'user', $user2['id'], $user2, false, $administrator['id']); + $this->assertTrue($user2['root'], 'User #2 is supposed to be root or this test is garbage'); - $postData['root'] = false; + $user2['root'] = false; $this->canNot( $this->kernelBrowser, - $userId, + $administrator['id'], 'PUT', $this->getUrl( $this->kernelBrowser, 'app_api_users_put', - ['version' => $this->apiVersion, 'id' => $postData['id']] + ['version' => $this->apiVersion, 'id' => $user2['id']] ), - json_encode(['user' => $postData]) + json_encode(['user' => $user2]) + ); + } + public function testRejectUnprivilegedPatchRemoveRootFromUser() + { + $dataLoader = $this->getDataLoader(); + + // create a new user with admin permissions in school 1, who is not root + $newUser = $dataLoader->create(); + $newUser['administeredSchools'] = [1]; + $administrator = $this->postOne('users', 'user', 'users', $newUser); + $this->assertFalse($administrator['root'], 'Administrator must not be root or this test is garbage'); + $this->assertContains('1', $administrator['administeredSchools']); + + $all = $dataLoader->getAll(); + $user2 = $all[1]; + + // ensure our new user *can* make some changes to user3 + $this->patchOneJsonApi($dataLoader->createJsonApi($user2), $administrator['id']); + $this->assertTrue($user2['root'], 'User #2 is supposed to be root or this test is garbage'); + + $user2['root'] = false; + + $this->canNotJsonApi( + $this->kernelBrowser, + $administrator['id'], + 'PATCH', + $this->getUrl( + $this->kernelBrowser, + 'app_api_users_put', + ['version' => $this->apiVersion, 'id' => $user2['id']] + ), + json_encode($dataLoader->createJsonApi($user2)) ); } - public function testUpdateRootAttributeAsRootUser() + public function testPutUpdateRootAttributeAsRootUser() { $dataLoader = $this->getDataLoader(); $all = $dataLoader->getAll(); diff --git a/tests/Traits/JsonControllerTest.php b/tests/Traits/JsonControllerTest.php index cc1e701579..51d17774ee 100644 --- a/tests/Traits/JsonControllerTest.php +++ b/tests/Traits/JsonControllerTest.php @@ -217,15 +217,14 @@ public function makeJsonApiRequest( /** * Tests to ensure that a user cannot access a certain function - * - * @param KernelBrowser $browser - * @param string $userId - * @param string $method - * @param string $url - * @param string $data */ - protected function canNot(KernelBrowser $browser, $userId, $method, $url, $data = null) - { + protected function canNot( + KernelBrowser $browser, + int $userId, + string $method, + string $url, + string $data = null + ): void { $this->makeJsonRequest( $browser, $method, @@ -242,4 +241,31 @@ protected function canNot(KernelBrowser $browser, $userId, $method, $url, $data substr(var_export($response->getContent(), true), 0, 400) ); } + + /** + * Tests to ensure that a user cannot access a certain function + */ + protected function canNotJsonApi( + KernelBrowser $browser, + int $userId, + string $method, + string $url, + string $data = null + ): void { + $this->makeJsonApiRequest( + $browser, + $method, + $url, + $data, + $this->getTokenForUser($browser, $userId) + ); + + $response = $browser->getResponse(); + $this->assertEquals( + Response::HTTP_FORBIDDEN, + $response->getStatusCode(), + "User #{$userId} should be prevented from {$method} at {$url}" . + substr(var_export($response->getContent(), true), 0, 400) + ); + } }