From 213971bc38553b05c5ecd3c5b4602a53343386e5 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 10:54:57 +0100 Subject: [PATCH 1/8] Improve user entity ownership code --- src/Entity/Interface/UserOwnedInterface.php | 15 +++++++++++-- src/Entity/Project/Project.php | 19 ++-------------- src/Entity/Project/RewardClaim.php | 20 +++-------------- src/Entity/Trait/UserOwnedTrait.php | 25 +++++++++++++++++++++ src/Entity/User/UserToken.php | 19 ++-------------- 5 files changed, 45 insertions(+), 53 deletions(-) create mode 100644 src/Entity/Trait/UserOwnedTrait.php diff --git a/src/Entity/Interface/UserOwnedInterface.php b/src/Entity/Interface/UserOwnedInterface.php index e2d7408..8f52d47 100644 --- a/src/Entity/Interface/UserOwnedInterface.php +++ b/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; } diff --git a/src/Entity/Project/Project.php b/src/Entity/Project/Project.php index 246e919..0475629 100644 --- a/src/Entity/Project/Project.php +++ b/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; diff --git a/src/Entity/Project/RewardClaim.php b/src/Entity/Project/RewardClaim.php index 1da5454..44d130b 100644 --- a/src/Entity/Project/RewardClaim.php +++ b/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; - } } diff --git a/src/Entity/Trait/UserOwnedTrait.php b/src/Entity/Trait/UserOwnedTrait.php new file mode 100644 index 0000000..0464d92 --- /dev/null +++ b/src/Entity/Trait/UserOwnedTrait.php @@ -0,0 +1,25 @@ +owner; + } + + public function setOwner(?User $owner): static + { + $this->owner = $owner; + + return $this; + } + + public function isOwnedBy(User $user): bool + { + return $this->owner->getId() === $user->getId(); + } +} diff --git a/src/Entity/User/UserToken.php b/src/Entity/User/UserToken.php index f6072aa..728a065 100644 --- a/src/Entity/User/UserToken.php +++ b/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(); - } } From b0dc3de297e22001ee76aeae6c602a6860f3aa4e Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:27:57 +0100 Subject: [PATCH 2/8] Add voter to check for projects security --- .../Project/ProjectApiResource.php | 4 +- src/Security/Voter/ProjectVoter.php | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 src/Security/Voter/ProjectVoter.php diff --git a/src/ApiResource/Project/ProjectApiResource.php b/src/ApiResource/Project/ProjectApiResource.php index 4e5d485..b95498b 100644 --- a/src/ApiResource/Project/ProjectApiResource.php +++ b/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)] diff --git a/src/Security/Voter/ProjectVoter.php b/src/Security/Voter/ProjectVoter.php new file mode 100644 index 0000000..b19abf6 --- /dev/null +++ b/src/Security/Voter/ProjectVoter.php @@ -0,0 +1,38 @@ +getUser(); + + switch ($attribute) { + case self::EDIT: + return $this->isOwnerOf($user, $subject); + case self::VIEW: + return true; + } + + return false; + } +} From c3a813e07d3f71547f3f36e8380c6cfba4eaa0f7 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:28:19 +0100 Subject: [PATCH 3/8] Generic user ownership trait for voters --- src/Security/Voter/UserOwnedVoterTrait.php | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 src/Security/Voter/UserOwnedVoterTrait.php diff --git a/src/Security/Voter/UserOwnedVoterTrait.php b/src/Security/Voter/UserOwnedVoterTrait.php new file mode 100644 index 0000000..4b62b72 --- /dev/null +++ b/src/Security/Voter/UserOwnedVoterTrait.php @@ -0,0 +1,35 @@ +isOwnedBy($user); + } + + if (!\property_exists($subject, 'owner')) { + return false; + } + + if ($subject->owner instanceof UserApiResource) { + return $subject->owner->id === $user->getId(); + } + + return false; + } +} From ab126dc7ba5154cd8e7d81707c1118bcda3a4348 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:33:16 +0100 Subject: [PATCH 4/8] Allow to check for resource ownership on null users --- src/Security/Voter/ProjectVoter.php | 2 +- src/Security/Voter/UserOwnedVoterTrait.php | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Security/Voter/ProjectVoter.php b/src/Security/Voter/ProjectVoter.php index b19abf6..ae52a8a 100644 --- a/src/Security/Voter/ProjectVoter.php +++ b/src/Security/Voter/ProjectVoter.php @@ -28,7 +28,7 @@ final class ProjectVoter extends Voter switch ($attribute) { case self::EDIT: - return $this->isOwnerOf($user, $subject); + return $this->isOwnerOf($subject, $user); case self::VIEW: return true; } diff --git a/src/Security/Voter/UserOwnedVoterTrait.php b/src/Security/Voter/UserOwnedVoterTrait.php index 4b62b72..2d9abe7 100644 --- a/src/Security/Voter/UserOwnedVoterTrait.php +++ b/src/Security/Voter/UserOwnedVoterTrait.php @@ -11,13 +11,17 @@ trait UserOwnedVoterTrait /** * Determines if the given User is the owner of the resource. * - * @param User $user The User to check ownership against * @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(User $user, object $subject): bool + public function isOwnerOf(object $subject, ?User $user): bool { + if ($user === null) { + return false; + } + if ($subject instanceof UserOwnedInterface) { return $subject->isOwnedBy($user); } From 2e6d4fc8da299c517f8ac26a3a9eaee8c63c6577 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:42:17 +0100 Subject: [PATCH 5/8] Update old security voters --- src/Security/Voter/AccountingVoter.php | 17 ++++++++++------- src/Security/Voter/UserOwnedVoter.php | 21 ++++++++++----------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/Security/Voter/AccountingVoter.php b/src/Security/Voter/AccountingVoter.php index eccd006..73d2956 100644 --- a/src/Security/Voter/AccountingVoter.php +++ b/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; } diff --git a/src/Security/Voter/UserOwnedVoter.php b/src/Security/Voter/UserOwnedVoter.php index f8f6225..095d436 100644 --- a/src/Security/Voter/UserOwnedVoter.php +++ b/src/Security/Voter/UserOwnedVoter.php @@ -3,13 +3,19 @@ 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 @@ -22,17 +28,10 @@ class UserOwnedVoter extends Voter } /** - * @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()); } } From 7690ac441c4b26485d6b1a9e241063a765b041b8 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:44:12 +0100 Subject: [PATCH 6/8] Fix missing entity class for user token api resource --- src/ApiResource/User/UserTokenApiResource.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ApiResource/User/UserTokenApiResource.php b/src/ApiResource/User/UserTokenApiResource.php index bdc31bd..bbb0560 100644 --- a/src/ApiResource/User/UserTokenApiResource.php +++ b/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)')] From 04c18983d4adab2cd294b9469dfa4061e0bb4e13 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:53:01 +0100 Subject: [PATCH 7/8] Increase leniency of USER_OWNED voter support --- src/Security/Voter/UserOwnedVoter.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Security/Voter/UserOwnedVoter.php b/src/Security/Voter/UserOwnedVoter.php index 095d436..c1d02a4 100644 --- a/src/Security/Voter/UserOwnedVoter.php +++ b/src/Security/Voter/UserOwnedVoter.php @@ -20,11 +20,11 @@ class UserOwnedVoter extends Voter 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; } /** From 063e260b39fa5fe0809661dd76ede165cd188eb8 Mon Sep 17 00:00:00 2001 From: Daniel Subiabre Date: Fri, 24 Jan 2025 11:54:24 +0100 Subject: [PATCH 8/8] Updated user voter --- src/Security/Voter/UserVoter.php | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/Security/Voter/UserVoter.php b/src/Security/Voter/UserVoter.php index 5757ced..54f233c 100644 --- a/src/Security/Voter/UserVoter.php +++ b/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; + } }