Merge pull request #6010 from usu/chore/voter-camp-null

chore(api): voters deny access when camp is null
ecamp · Sep 27, 2024 · c3ab369 · c3ab369
2 parents 08a4e3c + e525555
commit c3ab369
Show file tree

Hide file tree

Showing 23 changed files with 25 additions and 47 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {