Merge pull request #5011 from jrjohnson/close-root-patch

Prevent Changing Root Status Through Patch Request
ilios · Oct 13, 2023 · 4e6b2c6 · 4e6b2c6
2 parents d762624 + 642188c
commit 4e6b2c6
Show file tree

Hide file tree

Showing 5 changed files with 174 additions and 35 deletions.
diff --git 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
@@ -190,13 +190,21 @@ 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);
         }
         $json = json_encode($data);
 
         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
@@ -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
@@ -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
@@ -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)
+        );
+    }
 }