From af316509f15d045d76825c7c806d5252fae061cf Mon Sep 17 00:00:00 2001 From: BacLuc Date: Wed, 2 Jul 2025 23:12:05 +0200 Subject: [PATCH] api: disallow a guest to change his role to member/manager Fix it in CampCollaborationUpdateProcessor for now, because that's the only place this is a problem now. Maybe we should later fix this in the CampRoleVoter. It only is a problem when the own campCollaboration is changed. --- .../CampCollaborationUpdateProcessor.php | 26 +++- .../State/Util/AbstractPersistProcessor.php | 4 +- api/src/State/Util/PropertyChangeListener.php | 19 ++- .../UpdateCampCollaborationTest.php | 117 ++++++++++++++++-- 4 files changed, 152 insertions(+), 14 deletions(-) diff --git a/api/src/State/CampCollaborationUpdateProcessor.php b/api/src/State/CampCollaborationUpdateProcessor.php index f038a21dc8..6c185be03f 100644 --- a/api/src/State/CampCollaborationUpdateProcessor.php +++ b/api/src/State/CampCollaborationUpdateProcessor.php @@ -4,11 +4,13 @@ use ApiPlatform\State\ProcessorInterface; use App\Entity\CampCollaboration; +use App\Entity\User; use App\Service\MailService; use App\State\Util\AbstractPersistProcessor; use App\State\Util\PropertyChangeListener; use App\Util\IdGenerator; use Symfony\Bundle\SecurityBundle\Security; +use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface; /** @@ -29,9 +31,17 @@ public function __construct( afterAction: fn (CampCollaboration $data) => $this->onAfterStatusChange($data) ); + $roleChangeListener = PropertyChangeListener::of( + extractProperty: fn (CampCollaboration $data) => $data->role, + beforeAction: fn (CampCollaboration $data, CampCollaboration $previous) => $this->onBeforeRoleChange($data, $previous), + ); + parent::__construct( $decorated, - propertyChangeListeners: [$statusChangeListener] + propertyChangeListeners: [ + $statusChangeListener, + $roleChangeListener, + ] ); } @@ -44,6 +54,20 @@ public function onBeforeStatusChange(CampCollaboration $data): CampCollaboration return $data; } + public function onBeforeRoleChange(CampCollaboration $data, CampCollaboration $previous): CampCollaboration { + /** @var User $user */ + $user = $this->security->getUser(); + // If the update does not affect the own collaboration, the voter works. + if ($data->user->getId() !== $user->getId()) { + return $data; + } + if (in_array($previous->role, [CampCollaboration::ROLE_MANAGER, CampCollaboration::ROLE_MEMBER], true)) { + return $data; + } + + throw new HttpException(403, 'Not authorized to change role'); + } + public function onAfterStatusChange(CampCollaboration $data): void { $this->sendInviteEmail($data); } diff --git a/api/src/State/Util/AbstractPersistProcessor.php b/api/src/State/Util/AbstractPersistProcessor.php index 601b09581d..ba0e70feed 100644 --- a/api/src/State/Util/AbstractPersistProcessor.php +++ b/api/src/State/Util/AbstractPersistProcessor.php @@ -39,7 +39,7 @@ public function process(mixed $data, Operation $operation, array $uriVariables = $propertyBefore = call_user_func($listener->getExtractProperty(), $dataBefore); $propertyNow = call_user_func($listener->getExtractProperty(), $data); if ($propertyBefore !== $propertyNow) { - $data = call_user_func($listener->getBeforeAction(), $data); + $data = call_user_func($listener->getBeforeAction(), $data, $dataBefore); } } } @@ -53,7 +53,7 @@ public function process(mixed $data, Operation $operation, array $uriVariables = $propertyBefore = call_user_func($listener->getExtractProperty(), $dataBefore); $propertyNow = call_user_func($listener->getExtractProperty(), $data); if ($propertyBefore !== $propertyNow) { - call_user_func($listener->getAfterAction(), $data); + call_user_func($listener->getAfterAction(), $data, $dataBefore); } } } diff --git a/api/src/State/Util/PropertyChangeListener.php b/api/src/State/Util/PropertyChangeListener.php index d347526b41..702dd0a0d0 100644 --- a/api/src/State/Util/PropertyChangeListener.php +++ b/api/src/State/Util/PropertyChangeListener.php @@ -26,11 +26,11 @@ public static function of( if (self::hasOneParameter($extractProperty)) { throw new \InvalidArgumentException('extractProperty must have exactly one parameter'); } - if (self::hasOneParameter($beforeAction)) { - throw new \InvalidArgumentException('afterAction must have exactly one parameter'); + if (self::hasAtMostTwoParameter($beforeAction)) { + throw new \InvalidArgumentException('afterAction must have between 1 and 2 parameters'); } - if (self::hasOneParameter($afterAction)) { - throw new \InvalidArgumentException('afterAction must have exactly one parameter'); + if (self::hasAtMostTwoParameter($afterAction)) { + throw new \InvalidArgumentException('afterAction must have between 1 and 2 parameters'); } return new PropertyChangeListener($extractProperty, $beforeAction, $afterAction); @@ -56,4 +56,15 @@ private static function hasOneParameter(?\Closure $beforeAction): bool { return 1 != $beforeActionReflectionFunction->getNumberOfParameters(); } + + /** + * @throws \ReflectionException + */ + private static function hasAtMostTwoParameter(?\Closure $beforeAction): bool { + $beforeActionReflectionFunction = new \ReflectionFunction($beforeAction); + + $numberOfParameters = $beforeActionReflectionFunction->getNumberOfParameters(); + + return $numberOfParameters < 1 || $numberOfParameters > 2; + } } diff --git a/api/tests/Api/CampCollaborations/UpdateCampCollaborationTest.php b/api/tests/Api/CampCollaborations/UpdateCampCollaborationTest.php index 51b89f16a2..d5a7a0440c 100644 --- a/api/tests/Api/CampCollaborations/UpdateCampCollaborationTest.php +++ b/api/tests/Api/CampCollaborations/UpdateCampCollaborationTest.php @@ -6,6 +6,11 @@ use App\Entity\User; use App\Tests\Api\ECampApiTestCase; use PHPUnit\Framework\Attributes\DataProvider; +use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface; +use Symfony\Contracts\HttpClient\Exception\DecodingExceptionInterface; +use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface; +use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface; +use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; /** * @internal @@ -26,7 +31,7 @@ public function testPatchCampCollaborationIsDeniedForAnonymousUser() { public function testPatchCampCollaborationIsDeniedForUnrelatedUser() { $campCollaboration = static::getFixture('campCollaboration1manager'); - static::createClientWithCredentials(['email' => static::$fixtures['user4unrelated']->getEmail()]) + static::createClientWithCredentials(['email' => static::getFixture('user4unrelated')->getEmail()]) ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', 'role' => 'guest', @@ -41,7 +46,7 @@ public function testPatchCampCollaborationIsDeniedForUnrelatedUser() { public function testPatchCampCollaborationIsDeniedForInactiveCollaborator() { $campCollaboration = static::getFixture('campCollaboration1manager'); - static::createClientWithCredentials(['email' => static::$fixtures['user5inactive']->getEmail()]) + static::createClientWithCredentials(['email' => static::getFixture('user5inactive')->getEmail()]) ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', 'role' => 'guest', @@ -56,7 +61,7 @@ public function testPatchCampCollaborationIsDeniedForInactiveCollaborator() { public function testPatchCampCollaborationIsDeniedForGuest() { $campCollaboration = static::getFixture('campCollaboration1manager'); - static::createClientWithCredentials(['email' => static::$fixtures['user3guest']->getEmail()]) + static::createClientWithCredentials(['email' => static::getFixture('user3guest')->getEmail()]) ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', 'role' => 'guest', @@ -69,9 +74,9 @@ public function testPatchCampCollaborationIsDeniedForGuest() { ]); } - public function testOwnPatchCampCollaborationIsAllowedForGuest() { + public function testPatchOwnCampCollaborationIsAllowedForGuest() { $campCollaboration = static::getFixture('campCollaboration3guest'); - static::createClientWithCredentials(['email' => static::$fixtures['user3guest']->getEmail()]) + static::createClientWithCredentials(['email' => $campCollaboration->getEmail()]) ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', 'role' => 'guest', @@ -84,9 +89,29 @@ public function testOwnPatchCampCollaborationIsAllowedForGuest() { ]); } + public function testPatchOwnCampCollaborationToMemberIsRejectedForGuest() { + $campCollaboration = static::getFixture('campCollaboration3guest'); + static::createClientWithCredentials(['email' => $campCollaboration->getEmail()]) + ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => 'member', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]) + ; + $this->assertResponseStatusCodeSame(403); + } + + public function testPatchOwnCampCollaborationToManagerIsRejectedForGuest() { + $campCollaboration = static::getFixture('campCollaboration3guest'); + static::createClientWithCredentials(['email' => $campCollaboration->getEmail()]) + ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => 'manager', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]) + ; + $this->assertResponseStatusCodeSame(403); + } + public function testPatchCampCollaborationIsAllowedForMember() { $campCollaboration = static::getFixture('campCollaboration1manager'); - static::createClientWithCredentials(['email' => static::$fixtures['user2member']->getEmail()]) + static::createClientWithCredentials(['email' => static::getFixture('user2member')->getEmail()]) ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', 'role' => 'guest', @@ -99,7 +124,72 @@ public function testPatchCampCollaborationIsAllowedForMember() { ]); } - public function testPatchCampCollaborationIsAllowedForManager() { + /** + * Silly, but for now members can change the collaborations in the api. + * + * @throws ClientExceptionInterface + * @throws RedirectionExceptionInterface + * @throws ServerExceptionInterface + * @throws DecodingExceptionInterface|TransportExceptionInterface + */ + public function testPatchOwnCampCollaborationToManagerIsAllowedForMember() { + $campCollaboration = static::getFixture('campCollaboration2member'); + static::createClientWithCredentials(['email' => $campCollaboration->getEmail()]) + ->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => 'manager', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]) + ; + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'role' => 'manager', + ]); + } + + public function testPatchOwnStatusOfCampCollaborationIsAllowedForManager() { + $campCollaboration = static::getFixture('campCollaboration1manager'); + static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'status' => 'inactive', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'status' => 'inactive', + ]); + } + + public function testPatchOwnRoleToMemberOfCampCollaborationIsAllowedForManager() { + $campCollaboration = static::getFixture('campCollaboration1manager'); + static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => 'member', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'role' => 'member', + ]); + } + + public function testPatchOwnRoleToGuestOfCampCollaborationIsAllowedForMember() { + $campCollaboration = static::getFixture('campCollaboration1manager'); + static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => CampCollaboration::ROLE_GUEST, + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'role' => 'guest', + ]); + } + + public function testPatchOwnRoleToGuestOfCampCollaborationIsAllowedForManager() { + $campCollaboration = static::getFixture('campCollaboration1manager'); + static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'role' => 'guest', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'role' => 'guest', + ]); + } + + public function testPatchOwnStatusAndRoleCampCollaborationIsAcceptedForManager() { $campCollaboration = static::getFixture('campCollaboration1manager'); static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ 'status' => 'inactive', @@ -112,6 +202,19 @@ public function testPatchCampCollaborationIsAllowedForManager() { ]); } + public function testPatchOwnStatusAndRoleOfCampCollaborationIsAcceptedForMember() { + $campCollaboration = static::getFixture('campCollaboration2member'); + static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [ + 'status' => 'inactive', + 'role' => 'guest', + ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); + $this->assertResponseStatusCodeSame(200); + $this->assertJsonContains([ + 'status' => 'inactive', + 'role' => 'guest', + ]); + } + public function testPatchCampCollaborationInCampPrototypeIsDeniedForUnrelatedUser() { $campCollaboration = static::getFixture('campCollaboration1campPrototype'); static::createClientWithCredentials()->request('PATCH', '/camp_collaborations/'.$campCollaboration->getId(), ['json' => [