Skip to content

feat: improve ApiProperty::security using property name #5853

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 10 additions & 4 deletions features/authorization/deny.feature
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,16 @@ Feature: Authorization checking
And I send a "GET" request to "/secured_dummies/2"
Then the response status code should be 200

Scenario: A user can see a secured owner-only property on an object they own
Scenario: A user can see a secured owner-only property, or accessible property based on voter, on an object they own
When I add "Accept" header equal to "application/ld+json"
And I add "Content-Type" header equal to "application/ld+json"
And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg=="
And I send a "GET" request to "/secured_dummies/2"
Then the response status code should be 200
And the JSON node "ownerOnlyProperty" should exist
And the JSON node "ownerOnlyProperty" should not be null
And the JSON node "attributeBasedProperty" should exist
And the JSON node "attributeBasedProperty" should not be null

@!mongodb
Scenario: An admin can create a secured resource with properties depending on themselves
Expand Down Expand Up @@ -116,12 +118,13 @@ Feature: Authorization checking
And the JSON node "canUpdateProperty" should be true
And the JSON node "property" should be false

Scenario: An admin can't see a secured owner-only property on objects they don't own
Scenario: An admin can't see a secured owner-only property, or non-accessible property based on voter, on objects they don't own
When I add "Accept" header equal to "application/ld+json"
And I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu"
And I send a "GET" request to "/secured_dummies"
Then the response status code should be 200
And the response should not contain "ownerOnlyProperty"
And the response should not contain "attributeBasedProperty"

Scenario: A user can't assign to themself an item they doesn't own
When I add "Accept" header equal to "application/ld+json"
Expand Down Expand Up @@ -197,19 +200,21 @@ Feature: Authorization checking
And the response should contain "adminOnlyProperty"
And the JSON node "hydra:member[2].adminOnlyProperty" should be equal to the string "Is it safe?"

Scenario: An user can update an owner-only secured property on an object they own
Scenario: An user can update owner-only secured or accessible properties on an object they own
When I add "Accept" header equal to "application/ld+json"
And I add "Content-Type" header equal to "application/ld+json"
And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg=="
And I send a "PUT" request to "/secured_dummies/3" with body:
"""
{
"ownerOnlyProperty": "updated"
"ownerOnlyProperty": "updated",
"attributeBasedProperty": "updated"
}
"""
Then the response status code should be 200
And the response should contain "ownerOnlyProperty"
And the JSON node "ownerOnlyProperty" should be equal to the string "updated"
And the JSON node "attributeBasedProperty" should be equal to the string "updated"

@link_security
Scenario: An non existing entity should return Not found
Expand Down Expand Up @@ -299,3 +304,4 @@ Feature: Authorization checking
And I send a "GET" request to "/secured_dummies"
Then the response status code should be 200
And the response should contain "ownerOnlyProperty"
And the response should contain "attributeBasedProperty"
6 changes: 4 additions & 2 deletions src/Doctrine/Orm/Filter/OrderFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@
*
* By default, whenever the query does not specify the direction explicitly (e.g.: `/books?order[title]&order[id]`), filters will not be applied unless you configure a default order direction to use:
*
* [codeSelector]
* <div data-code-selector>
*
* ```php
* <?php
* // api/src/Entity/Book.php
Expand Down Expand Up @@ -181,7 +182,8 @@
* </resource>
* </resources>
* ```
* [/codeSelector]
*
* </div>
*
* When the property used for ordering can contain `null` values, you may want to specify how `null` values are treated in the comparison:
* - Use the default behavior of the DBMS: use `null` strategy
Expand Down
86 changes: 86 additions & 0 deletions src/Metadata/ApiProperty.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,93 @@ public function __construct(
private ?array $openapiContext = null,
private ?array $jsonSchemaContext = null,
private ?bool $push = null,
/**
* The `security` option defines the access to the current property, on normalization process, based on Symfony Security.
* It receives an `object` variable related to the current object, and a `property` variable related to the current property.
*
* <div data-code-selector>
*
* ```php
* <?php
* // api/src/Entity/Review.php
* use ApiPlatform\Metadata\ApiProperty;
* use ApiPlatform\Metadata\ApiResource;
*
* #[ApiResource]
* class Review
* {
* #[ApiProperty(security: 'is_granted("ROLE_ADMIN")')]
* public string $letter;
* }
* ```
*
* ```yaml
* # api/config/api_platform/properties.yaml
* properties:
* App\Entity\Review:
* letter:
* security: 'is_granted("ROLE_ADMIN")'
* ```
*
* ```xml
* <?xml version="1.0" encoding="UTF-8" ?>
* <!-- api/config/api_platform/properties.xml -->
*
* <properties
* xmlns="https://api-platform.com/schema/metadata/properties-3.0"
* xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
* xsi:schemaLocation="https://api-platform.com/schema/metadata/properties-3.0
* https://api-platform.com/schema/metadata/properties-3.0.xsd">
* <property resource="App\Entity\Review" name="letter" security="is_granted('ROLE_ADMIN')" />
* </properties>
* ```
*
* </div>
*/
private ?string $security = null,
/**
* The `securityPostDenormalize` option defines access to the current property after the denormalization process, based on Symfony Security.
* It receives an `object` variable related to the current object, and a `property` variable related to the current property.
*
* <div data-code-selector>
*
* ```php
* <?php
* // api/src/Entity/Review.php
* use ApiPlatform\Metadata\ApiProperty;
* use ApiPlatform\Metadata\ApiResource;
*
* #[ApiResource]
* class Review
* {
* #[ApiProperty(securityPostDenormalize: 'is_granted("ROLE_ADMIN")')]
* public string $letter;
* }
* ```
*
* ```yaml
* # api/config/api_platform/properties.yaml
* properties:
* App\Entity\Review:
* letter:
* securityPostDenormalize: 'is_granted("ROLE_ADMIN")'
* ```
*
* ```xml
* <?xml version="1.0" encoding="UTF-8" ?>
* <!-- api/config/api_platform/properties.xml -->
*
* <properties
* xmlns="https://api-platform.com/schema/metadata/properties-3.0"
* xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
* xsi:schemaLocation="https://api-platform.com/schema/metadata/properties-3.0
* https://api-platform.com/schema/metadata/properties-3.0.xsd">
* <property resource="App\Entity\Review" name="letter" securityPostDenormalize="is_granted('ROLE_ADMIN')" />
* </properties>
* ```
*
* </div>
*/
private ?string $securityPostDenormalize = null,
private array|string|null $types = null,
/*
Expand Down
2 changes: 2 additions & 0 deletions src/Serializer/AbstractItemNormalizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ protected function canAccessAttribute(?object $object, string $attribute, array
if (null !== $this->resourceAccessChecker && $security) {
return $this->resourceAccessChecker->isGranted($context['resource_class'], $security, [
'object' => $object,
'property' => $attribute,
]);
}

Expand All @@ -482,6 +483,7 @@ protected function canAccessAttributePostDenormalize(?object $object, ?object $p
return $this->resourceAccessChecker->isGranted($context['resource_class'], $security, [
'object' => $object,
'previous_object' => $previousObject,
'property' => $attribute,
]);
}

Expand Down
14 changes: 7 additions & 7 deletions src/Serializer/Tests/AbstractItemNormalizerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public function testNormalizeWithSecuredProperty(): void
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'is_granted(\'ROLE_ADMIN\')',
['object' => $dummy]
['object' => $dummy, 'property' => 'adminOnlyProperty']
)->willReturn(false);

$serializerProphecy = $this->prophesize(SerializerInterface::class);
Expand Down Expand Up @@ -352,7 +352,7 @@ public function testDenormalizeWithSecuredProperty(): void
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'is_granted(\'ROLE_ADMIN\')',
['object' => null]
['object' => null, 'property' => 'adminOnlyProperty']
)->willReturn(false);

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
Expand Down Expand Up @@ -464,12 +464,12 @@ public function testDenormalizeUpdateWithSecuredProperty(): void
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'true',
['object' => null]
['object' => null, 'property' => 'ownerOnlyProperty']
)->willReturn(true);
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'true',
['object' => $dummy]
['object' => $dummy, 'property' => 'ownerOnlyProperty']
)->willReturn(true);

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
Expand Down Expand Up @@ -528,12 +528,12 @@ public function testDenormalizeUpdateWithDeniedSecuredProperty(): void
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'false',
['object' => null]
['object' => null, 'property' => 'ownerOnlyProperty']
)->willReturn(false);
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'false',
['object' => $dummy]
['object' => $dummy, 'property' => 'ownerOnlyProperty']
)->willReturn(false);

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
Expand Down Expand Up @@ -593,7 +593,7 @@ public function testDenormalizeUpdateWithDeniedPostDenormalizeSecuredProperty():
$resourceAccessChecker->isGranted(
SecuredDummy::class,
'false',
['object' => $dummy, 'previous_object' => $dummy]
['object' => $dummy, 'previous_object' => $dummy, 'property' => 'ownerOnlyProperty']
)->willReturn(false);

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
Expand Down
18 changes: 18 additions & 0 deletions tests/Fixtures/TestBundle/Document/SecuredDummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use ApiPlatform\Metadata\Link;
use ApiPlatform\Metadata\Post;
use ApiPlatform\Metadata\Put;
use ApiPlatform\Tests\Fixtures\TestBundle\Security\SecuredDummyAttributeBasedVoter;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
Expand Down Expand Up @@ -75,6 +76,13 @@ class SecuredDummy
#[ODM\Field]
private ?string $ownerOnlyProperty = '';

/**
* @var string Secret property, only readable/writable through voters using "property" attribute
*/
#[ApiProperty(security: 'is_granted("'.SecuredDummyAttributeBasedVoter::ROLE.'", property)', securityPostDenormalize: 'is_granted("'.SecuredDummyAttributeBasedVoter::ROLE.'", property)')]
#[ODM\Field]
private string $attributeBasedProperty = '';

/**
* @var string|null The owner
*/
Expand Down Expand Up @@ -173,6 +181,16 @@ public function setOwnerOnlyProperty(?string $ownerOnlyProperty): void
$this->ownerOnlyProperty = $ownerOnlyProperty;
}

public function getAttributeBasedProperty(): string
{
return $this->attributeBasedProperty;
}

public function setAttributeBasedProperty(string $attributeBasedProperty): void
{
$this->attributeBasedProperty = $attributeBasedProperty;
}

public function getOwner(): ?string
{
return $this->owner;
Expand Down
18 changes: 18 additions & 0 deletions tests/Fixtures/TestBundle/Entity/SecuredDummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use ApiPlatform\Metadata\Link;
use ApiPlatform\Metadata\Post;
use ApiPlatform\Metadata\Put;
use ApiPlatform\Tests\Fixtures\TestBundle\Security\SecuredDummyAttributeBasedVoter;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
Expand Down Expand Up @@ -91,6 +92,13 @@ class SecuredDummy
#[ORM\Column]
private string $ownerOnlyProperty = '';

/**
* @var string Secret property, only readable/writable through voters using "property" attribute
*/
#[ApiProperty(security: 'is_granted("'.SecuredDummyAttributeBasedVoter::ROLE.'", property)', securityPostDenormalize: 'is_granted("'.SecuredDummyAttributeBasedVoter::ROLE.'", property)')]
#[ORM\Column]
private string $attributeBasedProperty = '';

/**
* @var string The owner
*/
Expand Down Expand Up @@ -202,6 +210,16 @@ public function setOwnerOnlyProperty(?string $ownerOnlyProperty): void
$this->ownerOnlyProperty = $ownerOnlyProperty;
}

public function getAttributeBasedProperty(): string
{
return $this->attributeBasedProperty;
}

public function setAttributeBasedProperty(string $attributeBasedProperty): void
{
$this->attributeBasedProperty = $attributeBasedProperty;
}

public function getOwner(): ?string
{
return $this->owner;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ApiPlatform\Tests\Fixtures\TestBundle\Security;

use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Symfony\Component\Security\Core\User\UserInterface;

final class SecuredDummyAttributeBasedVoter extends Voter
{
public const ROLE = 'SECURED_DUMMY_ATTRIBUTE_BASED';
private const RBAC = [
// property => array of users with access
'attributeBasedProperty' => ['dunglas'],
];

protected function supports(string $attribute, mixed $subject): bool
{
if (self::ROLE !== $attribute) {
return false;
}

if (!\is_string($subject)) {
return false;
}

return \in_array($subject, array_keys(self::RBAC), true);
}

protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
{
$user = $token->getUser();
if (!$user instanceof UserInterface || !\is_string($subject)) {
return false;
}

if (!\in_array($subject, array_keys(self::RBAC), true) || !\is_array(self::RBAC[$subject])) {
return false;
}

return \in_array($user->getUserIdentifier(), self::RBAC[$subject], true);
}
}
5 changes: 5 additions & 0 deletions tests/Fixtures/app/config/config_common.yml
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,11 @@ services:
arguments:
$router: '@router'

app.security.voter.secured_dummy_attribute_based:
class: 'ApiPlatform\Tests\Fixtures\TestBundle\Security\SecuredDummyAttributeBasedVoter'
tags:
- { name: 'security.voter' }

ApiPlatform\Tests\Fixtures\TestBundle\Doctrine\Orm\EntityManager:
decorates: 'doctrine.orm.default_entity_manager'
arguments:
Expand Down