Implement ApiProperty security attribute (#3503)

GregoireHebert · fredericbarthelet · web-flow · commit 6e3a259eeaf6 · 2020-04-21T11:33:31.000+02:00
* Implement ApiProperty security attribute

* add tests

* fix test

Co-authored-by: Frédéric Barthelet &lt;fredericb@theodo.fr&gt;
diff --git a/features/authorization/deny.feature b/features/authorization/deny.feature
@@ -59,7 +59,8 @@ Feature: Authorization checking
     {
         "title": "Special Title",
         "description": "Description",
-        "owner": "dunglas"
+        "owner": "dunglas",
+        "adminOnlyProperty": "secret"
     }
     """
     Then the response status code should be 201
@@ -100,3 +101,53 @@ Feature: Authorization checking
     }
     """
     Then the response status code should be 200
+
+  Scenario: An admin retrieves a resource with an admin only viewable property
+    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 YWRtaW46a2l0dGVu"
+    And I send a "GET" request to "/secured_dummies"
+    Then the response status code should be 200
+    And the response should contain "adminOnlyProperty"
+
+  Scenario: A user retrieves a resource with an admin only viewable property
+    When I add "Accept" header equal to "application/ld+json"
+    And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg=="
+    And I send a "GET" request to "/secured_dummies"
+    Then the response status code should be 200
+    And the response should not contain "adminOnlyProperty"
+
+  Scenario: An admin can create a secured resource with a secured Property
+    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 YWRtaW46a2l0dGVu"
+    And I send a "POST" request to "/secured_dummies" with body:
+    """
+    {
+        "title": "Common Title",
+        "description": "Description",
+        "owner": "dunglas",
+        "adminOnlyProperty": "Is it safe?"
+    }
+    """
+    Then the response status code should be 201
+    And the response should contain "adminOnlyProperty"
+    And the JSON node "adminOnlyProperty" should be equal to the string "Is it safe?"
+
+  Scenario: A user cannot update a secured property
+    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:
+    """
+    {
+        "adminOnlyProperty": "Yes it is!"
+    }
+    """
+    Then the response status code should be 200
+    And the response should not contain "adminOnlyProperty"
+    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 contain "adminOnlyProperty"
+    And the JSON node "hydra:member[2].adminOnlyProperty" should be equal to the string "Is it safe?"
diff --git a/features/graphql/authorization.feature b/features/graphql/authorization.feature
@@ -91,7 +91,7 @@ Feature: Authorization checking
     When I send the following GraphQL request:
     """
     mutation {
-      createSecuredDummy(input: {owner: "me", title: "Hi", description: "Desc", clientMutationId: "auth"}) {
+      createSecuredDummy(input: {owner: "me", title: "Hi", description: "Desc", adminOnlyProperty: "secret", clientMutationId: "auth"}) {
         securedDummy {
           title
           owner
@@ -112,7 +112,7 @@ Feature: Authorization checking
     And I send the following GraphQL request:
     """
     mutation {
-      createSecuredDummy(input: {owner: "someone", title: "Hi", description: "Desc"}) {
+      createSecuredDummy(input: {owner: "someone", title: "Hi", description: "Desc", adminOnlyProperty: "secret"}) {
         securedDummy {
           id
           title
@@ -131,7 +131,7 @@ Feature: Authorization checking
     And I send the following GraphQL request:
     """
     mutation {
-      createSecuredDummy(input: {owner: "dunglas", title: "Hi", description: "Desc"}) {
+      createSecuredDummy(input: {owner: "dunglas", title: "Hi", description: "Desc", adminOnlyProperty: "secret"}) {
         securedDummy {
           id
           title
diff --git a/src/Annotation/ApiProperty.php b/src/Annotation/ApiProperty.php
@@ -29,6 +29,7 @@
  *     @Attribute("openapiContext", type="array"),
  *     @Attribute("jsonldContext", type="array"),
  *     @Attribute("push", type="bool"),
+ *     @Attribute("security", type="string"),
  *     @Attribute("swaggerContext", type="array")
  * )
  */
@@ -128,6 +129,13 @@ final class ApiProperty
      */
     private $push;
 
+    /**
+     * @see https://github.com/Haehnchen/idea-php-annotation-plugin/issues/112
+     *
+     * @var string
+     */
+    private $security;
+
     /**
      * @see https://github.com/Haehnchen/idea-php-annotation-plugin/issues/112
      *
diff --git a/src/Bridge/Symfony/Bundle/Resources/config/api.xml b/src/Bridge/Symfony/Bundle/Resources/config/api.xml
@@ -115,7 +115,7 @@
             <argument>null</argument>
             <argument type="tagged" tag="api_platform.data_transformer" on-invalid="ignore" />
             <argument type="service" id="api_platform.metadata.resource.metadata_factory" on-invalid="ignore" />
-            <argument>false</argument>
+            <argument type="service" id="api_platform.security.resource_access_checker" />
 
             <!-- Run before serializer.normalizer.json_serializable -->
             <tag name="serializer.normalizer" priority="-895" />
diff --git a/src/Bridge/Symfony/Bundle/Resources/config/hal.xml b/src/Bridge/Symfony/Bundle/Resources/config/hal.xml
@@ -40,7 +40,7 @@
             <argument type="collection" />
             <argument type="tagged" tag="api_platform.data_transformer" on-invalid="ignore" />
             <argument type="service" id="api_platform.metadata.resource.metadata_factory" on-invalid="ignore" />
-            <argument>false</argument>
+            <argument type="service" id="api_platform.security.resource_access_checker" />
 
             <!-- Run before serializer.normalizer.json_serializable -->
             <tag name="serializer.normalizer" priority="-890" />
diff --git a/src/Bridge/Symfony/Bundle/Resources/config/jsonapi.xml b/src/Bridge/Symfony/Bundle/Resources/config/jsonapi.xml
@@ -41,7 +41,7 @@
             <argument type="service" id="api_platform.metadata.resource.metadata_factory" />
             <argument type="collection" />
             <argument type="tagged" tag="api_platform.data_transformer" on-invalid="ignore" />
-            <argument>false</argument>
+            <argument type="service" id="api_platform.security.resource_access_checker" />
 
             <!-- Run before serializer.normalizer.json_serializable -->
             <tag name="serializer.normalizer" priority="-890" />
diff --git a/src/Bridge/Symfony/Bundle/Resources/config/jsonld.xml b/src/Bridge/Symfony/Bundle/Resources/config/jsonld.xml
@@ -27,7 +27,7 @@
             <argument type="service" id="serializer.mapping.class_metadata_factory" on-invalid="ignore" />
             <argument type="collection" />
             <argument type="tagged" tag="api_platform.data_transformer" on-invalid="ignore" />
-            <argument>false</argument>
+            <argument type="service" id="api_platform.security.resource_access_checker" />
 
             <!-- Run before serializer.normalizer.json_serializable -->
             <tag name="serializer.normalizer" priority="-890" />
diff --git a/src/JsonApi/Serializer/ItemNormalizer.php b/src/JsonApi/Serializer/ItemNormalizer.php
@@ -20,6 +20,7 @@
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\PropertyMetadata;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
+use ApiPlatform\Core\Security\ResourceAccessCheckerInterface;
 use ApiPlatform\Core\Serializer\AbstractItemNormalizer;
 use ApiPlatform\Core\Serializer\CacheKeyTrait;
 use ApiPlatform\Core\Serializer\ContextTrait;
@@ -49,9 +50,9 @@ final class ItemNormalizer extends AbstractItemNormalizer
 
     private $componentsCache = [];
 
-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ?PropertyAccessorInterface $propertyAccessor, ?NameConverterInterface $nameConverter, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = [], iterable $dataTransformers = [])
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ?PropertyAccessorInterface $propertyAccessor, ?NameConverterInterface $nameConverter, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = [], iterable $dataTransformers = [], ResourceAccessCheckerInterface $resourceAccessChecker = null)
     {
-        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext, $dataTransformers, $resourceMetadataFactory);
+        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext, $dataTransformers, $resourceMetadataFactory, $resourceAccessChecker);
     }
 
     /**
diff --git a/src/JsonLd/Serializer/ItemNormalizer.php b/src/JsonLd/Serializer/ItemNormalizer.php
@@ -19,6 +19,7 @@
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
+use ApiPlatform\Core\Security\ResourceAccessCheckerInterface;
 use ApiPlatform\Core\Serializer\AbstractItemNormalizer;
 use ApiPlatform\Core\Serializer\ContextTrait;
 use ApiPlatform\Core\Util\ClassInfoTrait;
@@ -43,9 +44,9 @@ final class ItemNormalizer extends AbstractItemNormalizer
 
     private $contextBuilder;
 
-    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], iterable $dataTransformers = [])
+    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], iterable $dataTransformers = [], ResourceAccessCheckerInterface $resourceAccessChecker = null)
     {
-        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext, $dataTransformers, $resourceMetadataFactory);
+        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext, $dataTransformers, $resourceMetadataFactory, $resourceAccessChecker);
 
         $this->contextBuilder = $contextBuilder;
     }
diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php
@@ -24,6 +24,7 @@
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\PropertyMetadata;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
+use ApiPlatform\Core\Security\ResourceAccessCheckerInterface;
 use ApiPlatform\Core\Util\ClassInfoTrait;
 use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
 use Symfony\Component\PropertyAccess\PropertyAccess;
@@ -55,13 +56,14 @@ abstract class AbstractItemNormalizer extends AbstractObjectNormalizer
     protected $propertyMetadataFactory;
     protected $iriConverter;
     protected $resourceClassResolver;
+    protected $resourceAccessChecker;
     protected $propertyAccessor;
     protected $itemDataProvider;
     protected $allowPlainIdentifiers;
     protected $dataTransformers = [];
     protected $localCache = [];
 
-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, bool $allowPlainIdentifiers = false, array $defaultContext = [], iterable $dataTransformers = [], ResourceMetadataFactoryInterface $resourceMetadataFactory = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, bool $allowPlainIdentifiers = false, array $defaultContext = [], iterable $dataTransformers = [], ResourceMetadataFactoryInterface $resourceMetadataFactory = null, ResourceAccessCheckerInterface $resourceAccessChecker = null)
     {
         if (!isset($defaultContext['circular_reference_handler'])) {
             $defaultContext['circular_reference_handler'] = function ($object) {
@@ -83,6 +85,7 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
         $this->allowPlainIdentifiers = $allowPlainIdentifiers;
         $this->dataTransformers = $dataTransformers;
         $this->resourceMetadataFactory = $resourceMetadataFactory;
+        $this->resourceAccessChecker = $resourceAccessChecker;
     }
 
     /**
@@ -349,6 +352,25 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
         return $allowedAttributes;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    protected function isAllowedAttribute($classOrObject, $attribute, $format = null, array $context = [])
+    {
+        if (!parent::isAllowedAttribute($classOrObject, $attribute, $format, $context)) {
+            return false;
+        }
+
+        $options = $this->getFactoryOptions($context);
+        $propertyMetadata = $this->propertyMetadataFactory->create($context['resource_class'], $attribute, $options);
+        $security = $propertyMetadata->getAttribute('security');
+        if ($this->resourceAccessChecker && $security) {
+            return $this->resourceAccessChecker->isGranted($attribute, $security);
+        }
+
+        return true;
+    }
+
     /**
      * {@inheritdoc}
      */
diff --git a/src/Serializer/ItemNormalizer.php b/src/Serializer/ItemNormalizer.php
@@ -20,6 +20,7 @@
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
+use ApiPlatform\Core\Security\ResourceAccessCheckerInterface;
 use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
 use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
@@ -36,9 +37,9 @@ class ItemNormalizer extends AbstractItemNormalizer
 {
     private $logger;
 
-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, bool $allowPlainIdentifiers = false, LoggerInterface $logger = null, iterable $dataTransformers = [], ResourceMetadataFactoryInterface $resourceMetadataFactory = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, bool $allowPlainIdentifiers = false, LoggerInterface $logger = null, iterable $dataTransformers = [], ResourceMetadataFactoryInterface $resourceMetadataFactory = null, ResourceAccessCheckerInterface $resourceAccessChecker = null)
     {
-        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, $itemDataProvider, $allowPlainIdentifiers, [], $dataTransformers, $resourceMetadataFactory);
+        parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, $itemDataProvider, $allowPlainIdentifiers, [], $dataTransformers, $resourceMetadataFactory, $resourceAccessChecker);
 
         $this->logger = $logger ?: new NullLogger();
     }
diff --git a/tests/Annotation/ApiPropertyTest.php b/tests/Annotation/ApiPropertyTest.php
@@ -52,6 +52,7 @@ public function testConstruct()
             'fetchable' => true,
             'fetchEager' => false,
             'jsonldContext' => ['foo' => 'bar'],
+            'security' => 'is_granted(\'ROLE_ADMIN\')',
             'swaggerContext' => ['foo' => 'baz'],
             'openapiContext' => ['foo' => 'baz'],
             'push' => true,
@@ -62,6 +63,7 @@ public function testConstruct()
             'fetchable' => false,
             'fetch_eager' => false,
             'jsonld_context' => ['foo' => 'bar'],
+            'security' => 'is_granted(\'ROLE_ADMIN\')',
             'swagger_context' => ['foo' => 'baz'],
             'openapi_context' => ['foo' => 'baz'],
             'push' => true,
diff --git a/tests/Fixtures/TestBundle/Document/SecuredDummy.php b/tests/Fixtures/TestBundle/Document/SecuredDummy.php
@@ -13,6 +13,7 @@
 
 namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Document;
 
+use ApiPlatform\Core\Annotation\ApiProperty;
 use ApiPlatform\Core\Annotation\ApiResource;
 use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
 use Symfony\Component\Validator\Constraints as Assert;
@@ -26,7 +27,7 @@
  * @ApiResource(
  *     attributes={"security"="is_granted('ROLE_USER')"},
  *     collectionOperations={
- *         "get",
+ *         "get"={"security"="is_granted('ROLE_USER') or is_granted('ROLE_ADMIN')"},
  *         "get_from_data_provider_generator"={
  *             "method"="GET",
  *             "path"="custom_data_provider_generator",
@@ -72,6 +73,14 @@ class SecuredDummy
      */
     private $description = '';
 
+    /**
+     * @var string The dummy secret property, only readable/writable by specific users
+     *
+     * @ODM\Field
+     * @ApiProperty(security="is_granted('ROLE_ADMIN')")
+     */
+    private $adminOnlyProperty = '';
+
     /**
      * @var string The owner
      *
@@ -105,6 +114,16 @@ public function setDescription(string $description)
         $this->description = $description;
     }
 
+    public function getAdminOnlyProperty(): ?string
+    {
+        return $this->adminOnlyProperty;
+    }
+
+    public function setAdminOnlyProperty(?string $adminOnlyProperty)
+    {
+        $this->adminOnlyProperty = $adminOnlyProperty;
+    }
+
     public function getOwner(): string
     {
         return $this->owner;
diff --git a/tests/Fixtures/TestBundle/Entity/SecuredDummy.php b/tests/Fixtures/TestBundle/Entity/SecuredDummy.php
diff --git a/tests/Serializer/AbstractItemNormalizerTest.php b/tests/Serializer/AbstractItemNormalizerTest.php
