Skip to content

api: disallow a guest to change his role to member/manager #7740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BacLuc merged 1 commit into from
Jul 22, 2025
Merged

api: disallow a guest to change his role to member/manager #7740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion api/src/State/CampCollaborationUpdateProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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,
]
);
}

Expand All @@ -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)) {
Copy link
Member

@carlobeltrame carlobeltrame Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Members also aren't allowed to change their own role, or are they? Doesn't the same apply for members?
And is there some camp collaboration state which has the same flaw?

Copy link
Contributor Author

@BacLuc BacLuc Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are with the current implementation.
A similar issue: #3440
https://github.com/ecamp/ecamp3/blob/devel/api/src/Entity/Camp.php#L39
https://github.com/ecamp/ecamp3/blob/devel/api/src/Entity/CampCollaboration.php#L44

I did not wan't to change that too in the same PR.

Copy link
Member

@carlobeltrame carlobeltrame Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this PR is only concerned with restricting guests, not members. I'm fine with that, as long as we don't keep the equivalent flaw for members around for long.

return $data;
}

throw new HttpException(403, 'Not authorized to change role');
}

public function onAfterStatusChange(CampCollaboration $data): void {
$this->sendInviteEmail($data);
}
Expand Down
4 changes: 2 additions & 2 deletions api/src/State/Util/AbstractPersistProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand All @@ -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);
}
}
}
Expand Down
19 changes: 15 additions & 4 deletions api/src/State/Util/PropertyChangeListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member

@carlobeltrame carlobeltrame Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of these parameter checking functions seems backwards to me. It's not returning whether the callback has at most two parameters, it's returning whether it has more than two or less than one

Copy link
Contributor Author

@BacLuc BacLuc Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i also ran into it when developing.
But as far as i remember php-cs-fixer did not like !self::hasOneParameter because the method would only be used with negation.
Should i change this in this PR?

e.g.
validationFailsFoOneParamter
validationFailsForAtMostTwoParamter
?

Copy link
Member

@carlobeltrame carlobeltrame Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. Since you had to rewrite the PropertyChangeListener for this feature anyways, I'd have no problem accepting this refactoring in the same PR. But we can also do it later to get things moving here.

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);
Expand All @@ -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;
}
}
117 changes: 110 additions & 7 deletions api/tests/Api/CampCollaborations/UpdateCampCollaborationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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' => [
Expand Down