Review fixes

carlobeltrame · Apr 7, 2024 · 2fbe07b · 2fbe07b
1 parent 826e5ec
commit 2fbe07b
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 51 deletions.
diff --git a/api/src/State/CampCollaborationCreateProcessor.php b/api/src/State/CampCollaborationCreateProcessor.php
@@ -43,7 +43,8 @@ public function onBefore($data, Operation $operation, array $uriVariables = [],
             $profileByInviteEmail = $this->profileRepository->findOneBy(['email' => $inviteEmail]);
             if (null != $profileByInviteEmail) {
                 // Create a personal invitation, which the invited user will be able to see and
-                // accept / reject in the UI, even without receiving the invitation email
+                // accept / reject in the UI, even without receiving the invitation email.
+                // This is done by setting the user field instead of the inviteEmail field.
                 $data->user = $profileByInviteEmail->user;
                 $data->inviteEmail = null;
                 $this->validator->validate($data, ['groups' => ['Default', 'create']]);

diff --git a/api/src/State/PersonalInvitationAcceptProcessor.php b/api/src/State/PersonalInvitationAcceptProcessor.php
@@ -13,6 +13,7 @@
 use Doctrine\ORM\NonUniqueResultException;
 use Doctrine\ORM\NoResultException;
 use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 /**
  * @implements ProcessorInterface<PersonalInvitation,PersonalInvitation>
@@ -35,6 +36,10 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
         $user = $this->getUser();
 
         $campCollaboration = $this->campCollaborationRepository->findByUserAndIdAndInvited($user, $data->id);
+        if (null === $campCollaboration) {
+            throw new NoResultException();
+        }
+
         $campCollaboration->status = CampCollaboration::STATUS_ESTABLISHED;
         $campCollaboration->inviteKey = null;
         $campCollaboration->inviteKeyHash = null;
@@ -49,10 +54,12 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
      * @throws NonUniqueResultException
      * @throws NoResultException
      */
-    protected function getUser(): ?User {
+    private function getUser(): ?User {
         $user = $this->security->getUser();
         if (null == $user) {
-            return null;
+            // This should never happen because it should be caught earlier by our security settings
+            // on all API operations using this processor.
+            throw new AccessDeniedHttpException();
         }
 
         return $this->userRepository->loadUserByIdentifier($user->getUserIdentifier());

diff --git a/api/src/State/PersonalInvitationProvider.php b/api/src/State/PersonalInvitationProvider.php
@@ -43,37 +43,37 @@ public function provide(Operation $operation, array $uriVariables = [], array $c
      * @throws NoResultException
      * @throws NonUniqueResultException
      */
-    protected function provideCollection(): ?array {
+    private function provideCollection(): array {
         $user = $this->getUser();
         if (null == $user) {
             return [];
         }
         $campCollaborations = $this->campCollaborationRepository->findAllByPersonallyInvitedUser($user);
 
         return array_map(function (CampCollaboration $campCollaboration) {
-            return $this->getInvitation($campCollaboration);
+            return $this->toInvitation($campCollaboration);
         }, $campCollaborations);
     }
 
     /**
      * @throws NoResultException
      * @throws NonUniqueResultException
      */
-    protected function provideItem(string $id): ?PersonalInvitation {
+    private function provideItem(string $id): ?PersonalInvitation {
         $user = $this->getUser();
         if (null == $id || null == $user) {
             return null;
         }
         $campCollaboration = $this->campCollaborationRepository->findByUserAndIdAndInvited($user, $id);
 
-        return $this->getInvitation($campCollaboration);
+        return $this->toInvitation($campCollaboration);
     }
 
     /**
      * @throws NonUniqueResultException
      * @throws NoResultException
      */
-    protected function getUser(): ?User {
+    private function getUser(): ?User {
         $user = $this->security->getUser();
         if (null == $user) {
             return null;
@@ -82,7 +82,7 @@ protected function getUser(): ?User {
         return $this->userRepository->loadUserByIdentifier($user->getUserIdentifier());
     }
 
-    protected function getInvitation(?CampCollaboration $campCollaboration): ?PersonalInvitation {
+    private function toInvitation(?CampCollaboration $campCollaboration): ?PersonalInvitation {
         if (null == $campCollaboration) {
             return null;
         }

diff --git a/api/src/State/PersonalInvitationRejectProcessor.php b/api/src/State/PersonalInvitationRejectProcessor.php
@@ -13,6 +13,7 @@
 use Doctrine\ORM\NonUniqueResultException;
 use Doctrine\ORM\NoResultException;
 use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 /**
  * @implements ProcessorInterface<PersonalInvitation,PersonalInvitation>
@@ -35,6 +36,10 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
         $user = $this->getUser();
 
         $campCollaboration = $this->campCollaborationRepository->findByUserAndIdAndInvited($user, $data->id);
+        if (null === $campCollaboration) {
+            throw new NoResultException();
+        }
+
         $campCollaboration->status = CampCollaboration::STATUS_INACTIVE;
         $campCollaboration->inviteKey = null;
         $campCollaboration->inviteKeyHash = null;
@@ -48,10 +53,12 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
      * @throws NonUniqueResultException
      * @throws NoResultException
      */
-    protected function getUser(): ?User {
+    private function getUser(): ?User {
         $user = $this->security->getUser();
         if (null == $user) {
-            return null;
+            // This should never happen because it should be caught earlier by our security settings
+            // on all API operations using this processor.
+            throw new AccessDeniedHttpException();
         }
 
         return $this->userRepository->loadUserByIdentifier($user->getUserIdentifier());

diff --git a/api/tests/Api/Invitations/AcceptInvitationTest.php b/api/tests/Api/Invitations/AcceptInvitationTest.php
@@ -111,11 +111,6 @@ public function testCannotFindInvitationAfterSuccessfulAccept() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,
-            'userDisplayName' => 'Bi-Pi',
-            'userAlreadyInCamp' => false, */
             '_links' => [
                 'self' => ['href' => "/invitations/{$campCollaboration->inviteKey}/find"],
             ],
@@ -212,7 +207,14 @@ public static function invalidMethods(): array {
      * @throws TransportExceptionInterface
      */
     public function testNotFoundWhenInviteKeyDoesNotMatch() {
-        static::createClientWithCredentials()->request('PATCH', '/invitations/notExisting/'.Invitation::ACCEPT);
+        static::createClientWithCredentials()->request(
+            'PATCH',
+            '/invitations/notExisting/'.Invitation::ACCEPT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 
@@ -223,7 +225,14 @@ public function testNotFoundWhenInviteKeyDoesNotMatch() {
      * @throws ClientExceptionInterface
      */
     public function testNotFoundWhenNoInviteKey() {
-        static::createClientWithCredentials()->request('PATCH', '/invitations/'.Invitation::ACCEPT);
+        static::createClientWithCredentials()->request(
+            'PATCH',
+            '/invitations/'.Invitation::ACCEPT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 }
diff --git a/api/tests/Api/Invitations/RejectInvitationTest.php b/api/tests/Api/Invitations/RejectInvitationTest.php
@@ -64,11 +64,6 @@ public function testCannotFindInvitationAfterSuccessfulReject() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,
-            'userDisplayName' => null,
-            'userAlreadyInCamp' => null,*/
             '_links' => [
                 'self' => ['href' => "/invitations/{$campCollaboration->inviteKey}/find"],
             ],
@@ -98,11 +93,6 @@ public function testRejectInvitationSucceedsWhenLoggedIn() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,
-            'userDisplayName' => 'Bi-Pi',
-            'userAlreadyInCamp' => false,*/
             '_links' => [
                 'self' => ['href' => "/invitations/{$campCollaboration->inviteKey}/find"],
             ],
@@ -151,11 +141,6 @@ public function testRejectInvitationSucceedsWhenUserAlreadyInCamp() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,
-            'userDisplayName' => 'Bi-Pi',
-            'userAlreadyInCamp' => true,*/
             '_links' => [
                 'self' => ['href' => "/invitations/{$campCollaboration->inviteKey}/find"],
             ],
@@ -187,7 +172,14 @@ public static function invalidMethods(): array {
      * @throws TransportExceptionInterface
      */
     public function testNotFoundWhenInviteKeyDoesNotMatch() {
-        static::createClientWithCredentials()->request('PATCH', '/invitations/notExisting/'.Invitation::REJECT);
+        static::createClientWithCredentials()->request(
+            'PATCH',
+            '/invitations/notExisting/'.Invitation::REJECT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 
@@ -198,7 +190,14 @@ public function testNotFoundWhenInviteKeyDoesNotMatch() {
      * @throws ClientExceptionInterface
      */
     public function testNotFoundWhenNoInviteKey() {
-        static::createClientWithCredentials()->request('PATCH', '/invitations/'.Invitation::REJECT);
+        static::createClientWithCredentials()->request(
+            'PATCH',
+            '/invitations/'.Invitation::REJECT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 }
diff --git a/api/tests/Api/PersonalInvitations/AcceptPersonalInvitationTest.php b/api/tests/Api/PersonalInvitations/AcceptPersonalInvitationTest.php
@@ -118,10 +118,6 @@ public function testCannotFindPersonalInvitationAfterSuccessfulAccept() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'id' => $campCollaboration->getId(),
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title, */
             '_links' => [
                 'self' => ['href' => "/personal_invitations/{$campCollaboration->getId()}"],
             ],
@@ -183,7 +179,37 @@ public static function invalidMethods(): array {
     public function testNotFoundWhenIdDoesNotMatch() {
         /** @var Profile $profile */
         $profile = static::getFixture('profile6invited');
-        static::createClientWithCredentials(['email' => $profile->email])->request('PATCH', '/personal_invitations/notExisting/'.PersonalInvitation::ACCEPT);
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            '/personal_invitations/notExisting/'.PersonalInvitation::ACCEPT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
+        $this->assertResponseStatusCodeSame(404);
+    }
+
+    /**
+     * @throws ClientExceptionInterface
+     * @throws RedirectionExceptionInterface
+     * @throws ServerExceptionInterface
+     * @throws TransportExceptionInterface
+     */
+    public function testNotFoundWhenUserHasNoAccessToPersonalInvitation() {
+        /** @var CampCollaboration $campCollaboration */
+        $campCollaboration = static::getFixture('campCollaboration6invitedWithUser');
+
+        /** @var Profile $profile */
+        $profile = static::getFixture('profile8memberOnlyInCamp2');
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            "/personal_invitations/{$campCollaboration->getId()}/".PersonalInvitation::ACCEPT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 
@@ -196,7 +222,14 @@ public function testNotFoundWhenIdDoesNotMatch() {
     public function testMethodNotAllowedWhenNoId() {
         /** @var Profile $profile */
         $profile = static::getFixture('profile6invited');
-        static::createClientWithCredentials(['email' => $profile->email])->request('PATCH', '/personal_invitations/'.PersonalInvitation::ACCEPT);
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            '/personal_invitations/'.PersonalInvitation::ACCEPT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(405);
     }
 }
diff --git a/api/tests/Api/PersonalInvitations/RejectPersonalInvitationTest.php b/api/tests/Api/PersonalInvitations/RejectPersonalInvitationTest.php
@@ -61,10 +61,6 @@ public function testRejectPersonalInvitationSucceedsWhenLoggedIn() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'id' => $campCollaboration->getId(),
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,*/
             '_links' => [
                 'self' => ['href' => "/personal_invitations/{$campCollaboration->getId()}"],
             ],
@@ -96,10 +92,6 @@ public function testCannotFindPersonalInvitationAfterSuccessfulReject() {
         );
         $this->assertResponseStatusCodeSame(200);
         $this->assertJsonContains([
-            /*
-            'id' => $campCollaboration->getId(),
-            'campId' => $campCollaboration->camp->getId(),
-            'campTitle' => $campCollaboration->camp->title,*/
             '_links' => [
                 'self' => ['href' => "/personal_invitations/{$campCollaboration->getId()}"],
             ],
@@ -164,7 +156,37 @@ public static function invalidMethods(): array {
     public function testNotFoundWhenIdDoesNotMatch() {
         /** @var Profile $profile */
         $profile = static::getFixture('profile6invited');
-        static::createClientWithCredentials(['email' => $profile->email])->request('PATCH', '/personal_invitations/notExisting/'.PersonalInvitation::REJECT);
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            '/personal_invitations/notExisting/'.PersonalInvitation::REJECT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
+        $this->assertResponseStatusCodeSame(404);
+    }
+
+    /**
+     * @throws ClientExceptionInterface
+     * @throws RedirectionExceptionInterface
+     * @throws ServerExceptionInterface
+     * @throws TransportExceptionInterface
+     */
+    public function testNotFoundWhenUserHasNoAccessToPersonalInvitation() {
+        /** @var CampCollaboration $campCollaboration */
+        $campCollaboration = static::getFixture('campCollaboration6invitedWithUser');
+
+        /** @var Profile $profile */
+        $profile = static::getFixture('profile8memberOnlyInCamp2');
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            "/personal_invitations/{$campCollaboration->getId()}/".PersonalInvitation::REJECT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(404);
     }
 
@@ -177,7 +199,14 @@ public function testNotFoundWhenIdDoesNotMatch() {
     public function testMethodNotAllowedWhenNoId() {
         /** @var Profile $profile */
         $profile = static::getFixture('profile6invited');
-        static::createClientWithCredentials(['email' => $profile->email])->request('PATCH', '/personal_invitations/'.PersonalInvitation::REJECT);
+        static::createClientWithCredentials(['email' => $profile->email])->request(
+            'PATCH',
+            '/personal_invitations/'.PersonalInvitation::REJECT,
+            [
+                'json' => [],
+                'headers' => ['Content-Type' => 'application/merge-patch+json'],
+            ]
+        );
         $this->assertResponseStatusCodeSame(405);
     }
 }