Merge pull request #51 from GoteoFoundation/fix/user_owned_security

[fix] User owned security

src/ApiResource/Project/ProjectApiResource.php

@@ -25,8 +25,8 @@ use Symfony\Component\Validator\Constraints as Assert;
 #[API\GetCollection()]
 #[API\Post(security: 'is_granted("ROLE_USER")')]
 #[API\Get()]
-#[API\Patch(security: 'is_granted("PROJECT_EDIT")')]
-#[API\Delete(security: 'is_granted("PROJECT_EDIT")')]
+#[API\Patch(security: 'is_granted("PROJECT_EDIT", object)')]
+#[API\Delete(security: 'is_granted("PROJECT_EDIT", object)')]
 class ProjectApiResource
 {
     #[API\ApiProperty(identifier: true, writable: false)]

src/ApiResource/User/UserTokenApiResource.php

@@ -2,8 +2,10 @@
 
 namespace App\ApiResource\User;
 
+use ApiPlatform\Doctrine\Orm\State\Options;
 use ApiPlatform\Metadata as API;
 use App\Dto\UserTokenLoginDto;
+use App\Entity\User\UserToken;
 use App\State\ApiResourceStateProvider;
 use App\State\User\UserTokenLoginProcessor;
 
@@ -13,7 +15,11 @@ use App\State\User\UserTokenLoginProcessor;
  * When a UserToken is created v4 generates a SHA-256 hash that is unique for each UserToken.
  * The value of a UserToken comes preceded by a 4-digit-length prefix based on the type of token it is.
  */
-#[API\ApiResource(shortName: 'UserToken', provider: ApiResourceStateProvider::class)]
+#[API\ApiResource(
+    shortName: 'UserToken',
+    stateOptions: new Options(entityClass: UserToken::class),
+    provider: ApiResourceStateProvider::class,
+)]
 #[API\Post(input: UserTokenLoginDto::class, processor: UserTokenLoginProcessor::class)]
 #[API\Get(security: 'is_granted("USER_OWNED", object)')]
 #[API\Delete(security: 'is_granted("USER_OWNED", object)')]

src/Entity/Interface/UserOwnedInterface.php

@@ -6,9 +6,20 @@ use App\Entity\User\User;
 
 interface UserOwnedInterface
 {
+    /**
+     * @return User|null The User who is the owner of this entity
+     */
     public function getOwner(): ?User;
 
-    public function isOwnedBy(User $user): bool;
+    /**
+     * @param User|null The User who is the owner of this entity, or null to remove ownership
+     */
+    public function setOwner(?User $owner): static;
 
-    public function setOwner(User $user): static;
+    /**
+     * Determines if the given User is the owner of this entity.
+     *
+     * @param User The User to check ownership against
+     */
+    public function isOwnedBy(User $user): bool;
 }

src/Entity/Project/Project.php

@@ -10,6 +10,7 @@ use App\Entity\Trait\LocalizedContent;
 use App\Entity\Trait\MigratedEntity;
 use App\Entity\Trait\TimestampedCreationEntity;
 use App\Entity\Trait\TimestampedUpdationEntity;
+use App\Entity\Trait\UserOwnedTrait;
 use App\Entity\User\User;
 use App\Mapping\Provider\EntityMapProvider;
 use App\Repository\Project\ProjectRepository;
@@ -27,6 +28,7 @@ class Project implements UserOwnedInterface, AccountingOwnerInterface, Localized
     use MigratedEntity;
     use TimestampedCreationEntity;
     use TimestampedUpdationEntity;
+    use UserOwnedTrait;
 
     #[ORM\Id]
     #[ORM\GeneratedValue]
@@ -109,23 +111,6 @@ class Project implements UserOwnedInterface, AccountingOwnerInterface, Localized
         return $this;
     }
 
-    public function getOwner(): ?User
-    {
-        return $this->owner;
-    }
-
-    public function isOwnedBy(User $user): bool
-    {
-        return $user->getId() === $this->owner->getId();
-    }
-
-    public function setOwner(?User $owner): static
-    {
-        $this->owner = $owner;
-
-        return $this;
-    }
-
     public function getStatus(): ProjectStatus
     {
         return $this->status;

src/Entity/Project/RewardClaim.php

@@ -3,6 +3,7 @@
 namespace App\Entity\Project;
 
 use App\Entity\Interface\UserOwnedInterface;
+use App\Entity\Trait\UserOwnedTrait;
 use App\Entity\User\User;
 use App\Mapping\Provider\EntityMapProvider;
 use App\Repository\Project\RewardClaimRepository;
@@ -13,6 +14,8 @@ use Doctrine\ORM\Mapping as ORM;
 #[ORM\Entity(repositoryClass: RewardClaimRepository::class)]
 class RewardClaim implements UserOwnedInterface
 {
+    use UserOwnedTrait;
+
     #[ORM\Id]
     #[ORM\GeneratedValue]
     #[ORM\Column]
@@ -42,21 +45,4 @@ class RewardClaim implements UserOwnedInterface
 
         return $this;
     }
-
-    public function getOwner(): ?User
-    {
-        return $this->owner;
-    }
-
-    public function isOwnedBy(User $user): bool
-    {
-        return $this->owner->getId() === $user->getId();
-    }
-
-    public function setOwner(?User $owner): static
-    {
-        $this->owner = $owner;
-
-        return $this;
-    }
 }

src/Entity/Trait/UserOwnedTrait.php

@@ -0,0 +1,25 @@
+<?php
+
+namespace App\Entity\Trait;
+
+use App\Entity\User\User;
+
+trait UserOwnedTrait
+{
+    public function getOwner(): ?User
+    {
+        return $this->owner;
+    }
+
+    public function setOwner(?User $owner): static
+    {
+        $this->owner = $owner;
+
+        return $this;
+    }
+
+    public function isOwnedBy(User $user): bool
+    {
+        return $this->owner->getId() === $user->getId();
+    }
+}

src/Entity/User/UserToken.php

@@ -4,6 +4,7 @@ namespace App\Entity\User;
 
 use App\Entity\Interface\UserOwnedInterface;
 use App\Entity\Trait\TimestampedCreationEntity;
+use App\Entity\Trait\UserOwnedTrait;
 use App\Repository\User\UserTokenRepository;
 use Doctrine\ORM\Mapping as ORM;
 
@@ -20,6 +21,7 @@ use Doctrine\ORM\Mapping as ORM;
 class UserToken implements UserOwnedInterface
 {
     use TimestampedCreationEntity;
+    use UserOwnedTrait;
 
     #[ORM\Id]
     #[ORM\GeneratedValue]
@@ -55,21 +57,4 @@ class UserToken implements UserOwnedInterface
 
         return $this;
     }
-
-    public function getOwner(): ?User
-    {
-        return $this->owner;
-    }
-
-    public function setOwner(?User $owner): static
-    {
-        $this->owner = $owner;
-
-        return $this;
-    }
-
-    public function isOwnedBy(User $user): bool
-    {
-        return $this->owner->getUserIdentifier() === $user->getUserIdentifier();
-    }
 }

src/Security/Voter/AccountingVoter.php

@@ -2,35 +2,38 @@
 
 namespace App\Security\Voter;
 
-use App\Entity\User;
+use App\ApiResource\Accounting\AccountingApiResource;
+use App\Entity\User\User;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\Voter;
-use Symfony\Component\Security\Core\User\UserInterface;
 
 class AccountingVoter extends Voter
 {
+    use UserOwnedVoterTrait;
+
     public const EDIT = 'ACCOUNTING_EDIT';
 
     protected function supports(string $attribute, mixed $subject): bool
     {
         return in_array($attribute, [self::EDIT])
-            && $subject instanceof \App\Entity\Accounting\Accounting;
+            && $subject instanceof AccountingApiResource;
     }
 
+    /**
+     * @param AccountingApiResource $subject
+     */
     protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
     {
-        /** @var User */
         $user = $token->getUser();
 
-        // if the user is anonymous, do not grant access
-        if (!$user instanceof UserInterface) {
+        if (!$user instanceof User) {
             return false;
         }
 
         switch ($attribute) {
             case self::EDIT:
                 return $user->hasRoles(['ROLE_ADMIN'])
-                    || $user->getAccounting()->getId() === $subject->getId();
+                    || $this->isOwnerOf($subject, $user);
                 break;
         }
 

src/Security/Voter/ProjectVoter.php

@@ -0,0 +1,38 @@
+<?php
+
+namespace App\Security\Voter;
+
+use App\ApiResource\Project\ProjectApiResource;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\Voter;
+
+final class ProjectVoter extends Voter
+{
+    use UserOwnedVoterTrait;
+
+    public const EDIT = 'PROJECT_EDIT';
+    public const VIEW = 'PROJECT_VIEW';
+
+    protected function supports(string $attribute, mixed $subject): bool
+    {
+        return in_array($attribute, [self::EDIT, self::VIEW])
+            && $subject instanceof ProjectApiResource;
+    }
+
+    /**
+     * @param ProjectApiResource $subject
+     */
+    protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
+    {
+        $user = $token->getUser();
+
+        switch ($attribute) {
+            case self::EDIT:
+                return $this->isOwnerOf($subject, $user);
+            case self::VIEW:
+                return true;
+        }
+
+        return false;
+    }
+}

src/Security/Voter/UserOwnedVoter.php

@@ -3,36 +3,35 @@
 namespace App\Security\Voter;
 
 use App\Entity\Interface\UserOwnedInterface;
-use App\Entity\User;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\Voter;
-use Symfony\Component\Security\Core\User\UserInterface;
 
 class UserOwnedVoter extends Voter
 {
+    use UserOwnedVoterTrait;
+
+    /**
+     * Exclusively grants access to the owner.
+     *
+     * This is intended for sensitive information that is meant for the owner only.
+     * SHOULD NEVER BE OVERRIDEN BY ADMINS, MODERATORS, SUPER-ADMINS OR ANY HIGHER ROLE.
+     */
     public const OWNED = 'USER_OWNED';
 
     protected function supports(string $attribute, mixed $subject): bool
     {
-        if (!in_array($attribute, [self::OWNED])) {
-            return false;
+        if (in_array($attribute, [self::OWNED])) {
+            return true;
         }
 
-        return $subject instanceof UserOwnedInterface;
+        return false;
     }
 
     /**
-     * @param User|UserOwnedInterface $subject
+     * @param UserOwnedInterface $subject
      */
     protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
     {
-        $user = $token->getUser();
-
-        // if the user is anonymous, do not grant access
-        if (!$user instanceof UserInterface) {
-            return false;
-        }
-
-        return $subject->isOwnedBy($user);
+        return $this->isOwnerOf($subject, $token->getUser());
     }
 }

src/Security/Voter/UserOwnedVoterTrait.php

@@ -0,0 +1,39 @@
+<?php
+
+namespace App\Security\Voter;
+
+use App\ApiResource\User\UserApiResource;
+use App\Entity\Interface\UserOwnedInterface;
+use App\Entity\User\User;
+
+trait UserOwnedVoterTrait
+{
+    /**
+     * Determines if the given User is the owner of the resource.
+     *
+     * @param object $subject A resource that might or might not be owned by the User
+     * @param ?User  $user    The User to check ownership against
+     *
+     * @return bool `false` if ownership could not be guaranteed
+     */
+    public function isOwnerOf(object $subject, ?User $user): bool
+    {
+        if ($user === null) {
+            return false;
+        }
+
+        if ($subject instanceof UserOwnedInterface) {
+            return $subject->isOwnedBy($user);
+        }
+
+        if (!\property_exists($subject, 'owner')) {
+            return false;
+        }
+
+        if ($subject->owner instanceof UserApiResource) {
+            return $subject->owner->id === $user->getId();
+        }
+
+        return false;
+    }
+}

src/Security/Voter/UserVoter.php

@@ -2,10 +2,10 @@
 
 namespace App\Security\Voter;
 
-use App\Entity\User;
+use App\ApiResource\User\UserApiResource;
+use App\Entity\User\User;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\Voter;
-use Symfony\Component\Security\Core\User\UserInterface;
 
 class UserVoter extends Voter
 {
@@ -15,28 +15,40 @@ class UserVoter extends Voter
     protected function supports(string $attribute, mixed $subject): bool
     {
         return in_array($attribute, [self::EDIT, self::VIEW])
-            && $subject instanceof User;
+            && $subject instanceof UserApiResource;
     }
 
     /**
-     * @param User $subject
+     * @param UserApiResource $subject
      */
     protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
     {
         $user = $token->getUser();
 
-        if (!$user instanceof UserInterface) {
-            return false;
-        }
-
         switch ($attribute) {
             case self::EDIT:
-                return $user->getUserIdentifier() === $subject->getUserIdentifier()
-                    || \in_array('ROLE_ADMIN', $user->getRoles());
+                return $this->canEdit($subject, $user);
             case self::VIEW:
                 return true;
         }
 
         return false;
     }
+
+    private function canEdit(UserApiResource $subject, User $user): bool
+    {
+        if (!$user instanceof User) {
+            return false;
+        }
+
+        if ($user->hasRoles(['ROLE_ADMIN'])) {
+            return true;
+        }
+
+        if ($subject->id === $user->getId()) {
+            return true;
+        }
+
+        return false;
+    }
 }