From e525555e818b8566cfb9c6d5aeef60cbfe14c269 Mon Sep 17 00:00:00 2001 From: Urban Suppiger Date: Fri, 27 Sep 2024 21:56:52 +0200 Subject: [PATCH] voters deny access when camp is null --- api/src/Entity/Activity.php | 2 +- api/src/Entity/ActivityProgressLabel.php | 2 +- api/src/Entity/ActivityResponsible.php | 2 +- api/src/Entity/CampCollaboration.php | 2 +- api/src/Entity/Category.php | 2 +- api/src/Entity/Checklist.php | 2 +- api/src/Entity/ChecklistItem.php | 2 +- api/src/Entity/ContentNode/ChecklistNode.php | 2 +- api/src/Entity/ContentNode/ColumnLayout.php | 2 +- api/src/Entity/ContentNode/MaterialNode.php | 2 +- api/src/Entity/ContentNode/MultiSelect.php | 2 +- api/src/Entity/ContentNode/ResponsiveLayout.php | 2 +- api/src/Entity/ContentNode/SingleText.php | 2 +- api/src/Entity/ContentNode/Storyboard.php | 2 +- api/src/Entity/DayResponsible.php | 2 +- api/src/Entity/MaterialItem.php | 2 +- api/src/Entity/MaterialList.php | 2 +- api/src/Entity/Period.php | 2 +- api/src/Entity/ScheduleEntry.php | 2 +- api/src/Security/Voter/CampIsPrototypeVoter.php | 6 +----- api/src/Security/Voter/CampRoleVoter.php | 6 +----- api/tests/Security/Voter/CampIsPrototypeVoterTest.php | 11 ++--------- api/tests/Security/Voter/CampRoleVoterTest.php | 11 ++--------- 23 files changed, 25 insertions(+), 47 deletions(-) diff --git a/api/src/Entity/Activity.php b/api/src/Entity/Activity.php index fa7a263738..5e02c2df43 100644 --- a/api/src/Entity/Activity.php +++ b/api/src/Entity/Activity.php @@ -58,7 +58,7 @@ validationContext: ['groups' => ['Default', 'create']], denormalizationContext: ['groups' => ['write', 'create']], normalizationContext: self::ITEM_NORMALIZATION_CONTEXT, - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.category === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/ActivityProgressLabel.php b/api/src/Entity/ActivityProgressLabel.php index e4805e7819..c198027575 100644 --- a/api/src/Entity/ActivityProgressLabel.php +++ b/api/src/Entity/ActivityProgressLabel.php @@ -47,7 +47,7 @@ validationContext: ['groups' => ['Default', 'create']], denormalizationContext: ['groups' => ['write', 'create']], normalizationContext: self::ITEM_NORMALIZATION_CONTEXT, - securityPostDenormalize: 'is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MANAGER", object) or object.camp === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/ActivityResponsible.php b/api/src/Entity/ActivityResponsible.php index 7a4f27c177..33ede527ed 100644 --- a/api/src/Entity/ActivityResponsible.php +++ b/api/src/Entity/ActivityResponsible.php @@ -32,7 +32,7 @@ security: 'is_authenticated()' ), new Post( - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.activity === null' ), ], denormalizationContext: ['groups' => ['write']], normalizationContext: ['groups' => ['read']] diff --git a/api/src/Entity/CampCollaboration.php b/api/src/Entity/CampCollaboration.php index 2aceb2dc1d..cad0a58bfb 100644 --- a/api/src/Entity/CampCollaboration.php +++ b/api/src/Entity/CampCollaboration.php @@ -65,7 +65,7 @@ denormalizationContext: ['groups' => ['write', 'create']], normalizationContext: self::ITEM_NORMALIZATION_CONTEXT, openapi: new OpenApiOperation(description: 'Also sends an invitation email to the inviteEmail address, if specified.'), - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.camp === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/Category.php b/api/src/Entity/Category.php index b1972babd0..6298d315a8 100644 --- a/api/src/Entity/Category.php +++ b/api/src/Entity/Category.php @@ -55,7 +55,7 @@ processor: CategoryCreateProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], normalizationContext: self::ITEM_NORMALIZATION_CONTEXT, - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.camp === null' ), new GetCollection( uriTemplate: self::CAMP_SUBRESOURCE_URI_TEMPLATE, diff --git a/api/src/Entity/Checklist.php b/api/src/Entity/Checklist.php index 1746c30021..12eebd1f8d 100644 --- a/api/src/Entity/Checklist.php +++ b/api/src/Entity/Checklist.php @@ -43,7 +43,7 @@ new Post( processor: ChecklistCreateProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.camp === null' ), new GetCollection( uriTemplate: self::CAMP_SUBRESOURCE_URI_TEMPLATE, diff --git a/api/src/Entity/ChecklistItem.php b/api/src/Entity/ChecklistItem.php index 2f355203dc..0a61af91eb 100644 --- a/api/src/Entity/ChecklistItem.php +++ b/api/src/Entity/ChecklistItem.php @@ -45,7 +45,7 @@ ), new Post( denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.checklist === null' ), new GetCollection( uriTemplate: self::CHECKLIST_SUBRESOURCE_URI_TEMPLATE, diff --git a/api/src/Entity/ContentNode/ChecklistNode.php b/api/src/Entity/ContentNode/ChecklistNode.php index 2f6dcc8c39..874b1b2d01 100644 --- a/api/src/Entity/ContentNode/ChecklistNode.php +++ b/api/src/Entity/ContentNode/ChecklistNode.php @@ -41,7 +41,7 @@ new Post( processor: ChecklistNodePersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']], ), ], diff --git a/api/src/Entity/ContentNode/ColumnLayout.php b/api/src/Entity/ContentNode/ColumnLayout.php index 8ad30968ab..a2ce982d57 100644 --- a/api/src/Entity/ContentNode/ColumnLayout.php +++ b/api/src/Entity/ContentNode/ColumnLayout.php @@ -42,7 +42,7 @@ new Post( processor: ContentNodePersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/ContentNode/MaterialNode.php b/api/src/Entity/ContentNode/MaterialNode.php index a4095b3e4a..541773fdf5 100644 --- a/api/src/Entity/ContentNode/MaterialNode.php +++ b/api/src/Entity/ContentNode/MaterialNode.php @@ -40,7 +40,7 @@ new Post( processor: ContentNodePersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/ContentNode/MultiSelect.php b/api/src/Entity/ContentNode/MultiSelect.php index 748f58d5c4..0913476eb8 100644 --- a/api/src/Entity/ContentNode/MultiSelect.php +++ b/api/src/Entity/ContentNode/MultiSelect.php @@ -38,7 +38,7 @@ new Post( processor: MultiSelectCreateProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/ContentNode/ResponsiveLayout.php b/api/src/Entity/ContentNode/ResponsiveLayout.php index f323a378ad..d57061c5ad 100644 --- a/api/src/Entity/ContentNode/ResponsiveLayout.php +++ b/api/src/Entity/ContentNode/ResponsiveLayout.php @@ -41,7 +41,7 @@ new Post( processor: ContentNodePersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/ContentNode/SingleText.php b/api/src/Entity/ContentNode/SingleText.php index 9b0299f0e9..2d9f41ae21 100644 --- a/api/src/Entity/ContentNode/SingleText.php +++ b/api/src/Entity/ContentNode/SingleText.php @@ -37,7 +37,7 @@ new Post( processor: SingleTextPersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/ContentNode/Storyboard.php b/api/src/Entity/ContentNode/Storyboard.php index fb39300ba0..9d97acf115 100644 --- a/api/src/Entity/ContentNode/Storyboard.php +++ b/api/src/Entity/ContentNode/Storyboard.php @@ -37,7 +37,7 @@ new Post( processor: StoryboardPersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.parent === null', validationContext: ['groups' => ['Default', 'create']] ), ], diff --git a/api/src/Entity/DayResponsible.php b/api/src/Entity/DayResponsible.php index eba2b6cb46..3d575f359c 100644 --- a/api/src/Entity/DayResponsible.php +++ b/api/src/Entity/DayResponsible.php @@ -43,7 +43,7 @@ ], ), new Post( - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.day === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/MaterialItem.php b/api/src/Entity/MaterialItem.php index b076318d5a..dafa0084cb 100644 --- a/api/src/Entity/MaterialItem.php +++ b/api/src/Entity/MaterialItem.php @@ -44,7 +44,7 @@ security: 'is_authenticated()' ), new Post( - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.materialList === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/MaterialList.php b/api/src/Entity/MaterialList.php index 3485e82241..725a81824b 100644 --- a/api/src/Entity/MaterialList.php +++ b/api/src/Entity/MaterialList.php @@ -43,7 +43,7 @@ ), new Post( denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.camp === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/Period.php b/api/src/Entity/Period.php index bdecf059f7..5ef03164c9 100644 --- a/api/src/Entity/Period.php +++ b/api/src/Entity/Period.php @@ -52,7 +52,7 @@ new Post( processor: PeriodPersistProcessor::class, denormalizationContext: ['groups' => ['write', 'create']], - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)' + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.camp === null' ), ], denormalizationContext: ['groups' => ['write']], diff --git a/api/src/Entity/ScheduleEntry.php b/api/src/Entity/ScheduleEntry.php index b5424bdabf..de78271400 100644 --- a/api/src/Entity/ScheduleEntry.php +++ b/api/src/Entity/ScheduleEntry.php @@ -58,7 +58,7 @@ new Post( denormalizationContext: ['groups' => ['write', 'create']], normalizationContext: self::ITEM_NORMALIZATION_CONTEXT, - securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)', + securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object) or object.activity === null', validationContext: ['groups' => ScheduleEntryPostGroupSequence::class] ), ], diff --git a/api/src/Security/Voter/CampIsPrototypeVoter.php b/api/src/Security/Voter/CampIsPrototypeVoter.php index c03a1f91b4..c2ea8dcdcb 100644 --- a/api/src/Security/Voter/CampIsPrototypeVoter.php +++ b/api/src/Security/Voter/CampIsPrototypeVoter.php @@ -4,7 +4,6 @@ use App\Entity\BelongsToCampInterface; use App\Entity\BelongsToContentNodeTreeInterface; -use App\Entity\Camp; use App\HttpCache\ResponseTagger; use App\Util\GetCampFromContentNodeTrait; use Doctrine\ORM\EntityManagerInterface; @@ -31,10 +30,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter $camp = $this->getCampFromInterface($subject, $this->em); if (null === $camp) { - // Allow access when camp is null. - // In write operations, this should be handled by validation. - // Therefore, in read operations this should never happen. - return true; + return false; } if ($camp->isPrototype) { diff --git a/api/src/Security/Voter/CampRoleVoter.php b/api/src/Security/Voter/CampRoleVoter.php index 7ffcd3119d..f1895a013a 100644 --- a/api/src/Security/Voter/CampRoleVoter.php +++ b/api/src/Security/Voter/CampRoleVoter.php @@ -4,7 +4,6 @@ use App\Entity\BelongsToCampInterface; use App\Entity\BelongsToContentNodeTreeInterface; -use App\Entity\Camp; use App\Entity\CampCollaboration; use App\Entity\User; use App\HttpCache\ResponseTagger; @@ -45,10 +44,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter $camp = $this->getCampFromInterface($subject, $this->em); if (null === $camp) { - // Allow access when camp is null. - // In write operations, this should be handled by validation. - // Therefore, in read operations this should never happen. - return true; + return false; } $campCollaboration = $camp->collaborations diff --git a/api/tests/Security/Voter/CampIsPrototypeVoterTest.php b/api/tests/Security/Voter/CampIsPrototypeVoterTest.php index ac3e2c847f..529fd767e8 100644 --- a/api/tests/Security/Voter/CampIsPrototypeVoterTest.php +++ b/api/tests/Security/Voter/CampIsPrototypeVoterTest.php @@ -64,14 +64,7 @@ public function testDoesntVoteWhenSubjectIsNull() { $this->assertEquals(VoterInterface::ACCESS_ABSTAIN, $result); } - /** - * When the camp associated with an entity is null, this isn't a security question, - * but rather should have been caught by validation rules. - * If the association with a camp really is optional for some entity, the security - * rules can easily add a check manually: - * is_granted("CAMP_COLLABORATOR", object) and null != object.getCamp(). - */ - public function testGrantsAccessWhenGetCampYieldsNull() { + public function testDeniesAccessWhenGetCampYieldsNull() { // given $this->token->method('getUser')->willReturn(new User()); $subject = $this->createMock(Period::class); @@ -81,7 +74,7 @@ public function testGrantsAccessWhenGetCampYieldsNull() { $result = $this->voter->vote($this->token, $subject, ['CAMP_IS_PROTOTYPE']); // then - $this->assertEquals(VoterInterface::ACCESS_GRANTED, $result); + $this->assertEquals(VoterInterface::ACCESS_DENIED, $result); } public function testDeniesAccessWhenCampIsntPrototype() { diff --git a/api/tests/Security/Voter/CampRoleVoterTest.php b/api/tests/Security/Voter/CampRoleVoterTest.php index 9c779a5713..4b5fa5eedf 100644 --- a/api/tests/Security/Voter/CampRoleVoterTest.php +++ b/api/tests/Security/Voter/CampRoleVoterTest.php @@ -76,14 +76,7 @@ public function testDeniesAccessWhenNotLoggedIn() { $this->assertEquals(VoterInterface::ACCESS_DENIED, $result); } - /** - * When the camp associated with an entity is null, this isn't a security question, - * but rather should have been caught by validation rules. - * If the association with a camp really is optional for some entity, the security - * rules can easily add a check manually: - * is_granted("CAMP_COLLABORATOR", object) and null != object.getCamp(). - */ - public function testGrantsAccessWhenGetCampYieldsNull() { + public function testDeniesAccessWhenGetCampYieldsNull() { // given $this->token->method('getUser')->willReturn(new User()); $subject = $this->createMock(Period::class); @@ -93,7 +86,7 @@ public function testGrantsAccessWhenGetCampYieldsNull() { $result = $this->voter->vote($this->token, $subject, ['CAMP_COLLABORATOR']); // then - $this->assertEquals(VoterInterface::ACCESS_GRANTED, $result); + $this->assertEquals(VoterInterface::ACCESS_DENIED, $result); } public function testDeniesAccessWhenGetCampYieldsNullAndNotLoggedIn() {