Implement ApiProperty security attribute by fredericbarthelet · Pull Request #3227 · api-platform/core

fredericbarthelet · 2019-10-31T16:00:55Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tickets	fixes #773
License	MIT
Doc PR	api-platform/docs#1000

Implement new security attribute on ApiProperty annotations.
Allows built-in support for property removal from resource using acl expression language.

This initial PR is a POC to explain my intention :

adding new ApiProperty attribute security
injecting ResourceAccessChecker within AbstractItemNormalizer
supercharge isAllowedAttribute method inherited from symfony AbstractNormalizer service
early return based on acl resolution if resolved to false

WDYT ?

soyuka · 2019-11-01T10:02:17Z

I find this interesting, @dunglas is this what you had in mind?

antograssiot

doesn't the api_platform.jsonld.normalizer.item, api_platform.jsonapi.normalizer.item and api_platform.hal.normalizer.item services declaration needs to be updated as well for this to work in real apps ?

antograssiot · 2019-11-01T15:42:32Z

    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, ResourceAccessCheckerInterface $resourceAccessChecker, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, bool $allowPlainIdentifiers = false, array $defaultContext = [], iterable $dataTransformers = [], ResourceMetadataFactoryInterface $resourceMetadataFactory = null)


Additional arguments must be added at the end with and with a default value or it is a BC break

teohhanhui · 2019-11-01T23:13:52Z

+            return false;
+        }
+
+        return parent::isAllowedAttribute($classOrObject, $attribute, $format, $context);


The parent method should be called first. No need to do extra work when we can return the value directly if it's already false. We only need to check against the security attribute when the parent method returns true, since we're taking a blacklist approach here.

teohhanhui · 2019-11-01T23:15:21Z

     * @ORM\Column
     * @Assert\NotBlank
-     * @ApiProperty(iri="http://schema.org/name")
+     * @ApiProperty(iri="http://schema.org/name", security="is_granted('ROLE_USER')")


Please don't make this change. I suspect this will affect many existing tests. The new property you've added ($adminOnlyProperty) is enough.

teohhanhui · 2019-11-01T23:18:39Z

Just to provide some background: @fredericbarthelet discussed this with me during the Hackday in Paris.

Thanks for following up on this!

antograssiot · 2019-11-04T20:12:54Z

+     * @ORM\Column(nullable=true)
+     * @ApiProperty(security="is_granted('ROLE_ADMIN')")
+     */
+    private $adminOnlyProperty;


the same changes must be done in tests/Fixtures/TestBundle/Document/Dummy.php so the mongoDB suite will pass too

Just a suggestion, but using another Entity/Document will avoid to have to modify filters tests

Maybe this could be added to the SecuredDummy instead, it will have less side impacts and will be at the same place than the security attribute from ApiResource

@antograssiot that what I noticed as well when I started implementing behat tests. I will move it to SecuredDummy instead :)

fredericbarthelet · 2019-11-07T11:25:00Z

@soyuka, @antograssiot, @teohhanhui I updated my PR to take into account your comments. Thank you for your feedbacks :) !

I encountered an issue when integrated security property on graphQL API.
When a user makes a graphQL request with a field he cannot access based on this new security property, ie :

Resource

    /**
     * @ApiProperty(security="is_granted('ROLE_ADMIN')")
     */
    private $adminOnlyProperty = '';

Request

    {
      securedDummy(id: "/secured_dummies/2") {
        owner
        title
        adminOnlyProperty
      }
    }

I get a 200 HTTP answer with an Internal Server Error message because Cannot return null for non-nullable field SecuredDummy.adminOnlyProperty. IMHO, the ideal feedback message should be similar to the one returned when requesting non-existing properties, aka Cannot query field \"adminOnlyProperty\" on type \"SecuredDummy\".

My idea would be to inject as well ResourceAccessCheckerInterface service in src/GraphQl/Type/FieldsBuilder.php and modify generated fields on graphQL allowed operations to remove those removed by the same isAllowedAttribute method of the ItemNormalizer. This is done in the getResourceObjectTypeFields method. Do you agree with such solution ?

alanpoulain · 2019-11-16T21:52:00Z

@fredericbarthelet I'm not sure it's a good idea to check the security inside the FieldsBuilder. The GraphQL system has two steps:

building the schema
resolving a query

The built schema should not be dynamic and dependent on the query. That's why you can remove fields for instance by using non-dynamic normalization context but not in your case IMO.

I'm not sure it's the best way, but my proposal would be to make the field nullable (or to throw an exception to force the user to make it nullable?) if it has a security attribute and set it to null if the user doesn't have the rights to see the value of this field.

teohhanhui · 2019-11-18T08:12:17Z

my proposal would be to make the field nullable (or to throw an exception to force the user to make it nullable?) if it has a security attribute and set it to null if the user doesn't have the rights to see the value of this field

That does not make sense... :x We should find a better solution.

alanpoulain · 2019-11-18T09:00:25Z

From what I read in the GraphQL examples, it seems to be the current practice: https://graphql.org/learn/authorization/

teohhanhui · 2019-11-18T09:44:25Z

I don't see the relevance of the link you've shared. I'm not advocating doing it at the GraphQL layer. Making fields nullable is propagating this authorization logic to the GraphQL layer, which is exactly what they've warned against.

alanpoulain · 2019-11-18T12:10:01Z

In most GraphQL systems, the schema cannot change dynamically depending on the context (very often it's a static file). So you have to make sure the fields can be nullable if you want some users to not see the corresponding value.

That's also why in the type system, the types are nullable by default.

If you take the GitHub API (https://developer.github.com/v4), 95% of the fields are nullable.

alanpoulain · 2019-11-18T12:15:39Z

See also: graphql/graphql-js#113

alanpoulain · 2019-11-18T12:20:29Z

GraphQL Ruby seems to be the only one doing it. I'm not sure we should too. https://graphql-ruby.org/authorization/visibility

teohhanhui · 2019-11-20T13:22:27Z

That sounds like a good strategy, yes. Let's get this feature for REST first.

alanpoulain · 2019-11-20T13:55:06Z

I think it will be good to be coherent with GraphQL too...
For me there are two choices:

force the field to be nullable in the GraphQL schema if it has the security attribute
throw an exception if the field is not nullable if it has a security attribute

The @api-platform/core-team should make a choice and not just leave the problem because it's GraphQL.

alanpoulain · 2019-11-20T14:01:04Z

I've added an issue for the visibility attribute too: #3279.

teohhanhui · 2019-11-20T14:04:51Z

I agree that it should be supported for GraphQL too, but it needs more work in the GraphQL system of API Platform before that's possible. We don't need to block this PR because of that.

The nullable proposal is not feasible, per discussion above.

alanpoulain · 2019-11-20T14:13:56Z

The nullable property is totally feasible.
If you refer to:

Making fields nullable is propagating this authorization logic to the GraphQL layer, which is exactly what they've warned against.

I think you misunderstood what they say. First of all, they are talking about the resolver part of the GraphQL system. We are talking about the building part here. Secondly, it's only a recommendation to avoid duplication. Here we can do this with no issue like this. And finally the "problem" would be the same for the visibility attribute.

Now that the GraphQL system is introduced and is fully working, we should make sure new features are working for it too.

teohhanhui · 2019-11-20T14:29:22Z

#3227 (comment)

alanpoulain · 2019-11-20T14:41:13Z

I've already responded to this comment.
Yes you cannot distinguish between a "true" null value and a null value because it's not available for you.
But that's how GraphQL has been thought. Even if you don't agree with this (which I can understand), it's working like this today in most GraphQL systems. We are not here to question the GraphQL specification.
And if the user has a problem with it, the visibility attribute could solve it.
That's how GraphQL Ruby is doing it for instance: https://graphql-ruby.org/authorization/authorization#handling-unauthorized-objects

teohhanhui · 2019-11-20T15:12:50Z

So yes, if you insist that GraphQL support needs to be implemented in this PR (which I disagree with), then we're now blocked by #3279

What I'm saying is, we can add GraphQL support for this feature once #3279 is implemented. There's no need to block this contribution now.

alanpoulain · 2019-11-20T15:16:32Z

Blocked? No we're not. This visibility attribute can be done in a second time (or even never).
I've proposed two solutions above to solve the GraphQL part. Both have advantages and drawbacks but we just need to make a choice.

teohhanhui · 2019-11-20T15:19:15Z

Do you have examples of other GraphQL implementations which return null on unauthorized access? I'm not convinced that it's a feasible alternative. From the discussions I've seen, it's either no authorization on property level, or implement the visibility feature.

alanpoulain · 2019-11-20T15:30:36Z

Sure...
GraphQLite: https://graphqlite.thecodingmachine.io/docs/authentication_authorization#constant-schemas
GraphQL Java: graphql-java/graphql-java#290 (comment)
Graphene: http://docs.graphene-python.org/projects/django/en/latest/authorization/#filtering-id-based-node-access

I'm returning your question: please find a GraphQL system which doesn't work like this.

theofidry · 2019-11-20T15:45:28Z

Can also confirm it’s the case for OnyxGraphQL as well. Note that it’s the same on error: but in both cases you may have an error entry besides the data one in the returned payload

…

On Wed 20 Nov 2019 at 16:30, Alan Poulain ***@***.***> wrote: Sure... GraphQLite: https://graphqlite.thecodingmachine.io/docs/authentication_authorization#constant-schemas GraphQL Java: graphql-java/graphql-java#290 (comment) <graphql-java/graphql-java#290 (comment)> Graphene: http://docs.graphene-python.org/projects/django/en/latest/authorization/#filtering-id-based-node-access I'm returning your question: please find a GraphQL system which *doesn't* work like this. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#3227?email_source=notifications&email_token=ABHPVAKG6NWE35E5VL7N6OLQUVJZ3A5CNFSM4JHMSQR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESLZ3I#issuecomment-556055789>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHPVAJPEYOIOBKE2M7A4JTQUVJZ3ANCNFSM4JHMSQRQ> .

alanpoulain · 2019-11-20T15:54:27Z

Some GraphQL systems have not implemented the visibility feature yet.
@theofidry having an error entry could be a third solution. I don't really like it because it's not really an error for me but we can discuss it.

theofidry · 2019-11-20T15:57:18Z

It is, because you requested access to a field you don’t have access to. That said I would recommend to stick to the standards and not going for a new exotic solution

…

On Wed 20 Nov 2019 at 16:54, Alan Poulain ***@***.***> wrote: Some GraphQL systems have not implemented the visibility feature yet. @theofidry <https://github.com/theofidry> having an error entry could be a third solution. I don't really like it because it's not really an error for me but we can discuss it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3227?email_source=notifications&email_token=ABHPVAPDGUK6PKY4OUIUKO3QUVMTLA5CNFSM4JHMSQR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESPGAA#issuecomment-556069632>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHPVAKJFCF7OLQXBMW2XOLQUVMTLANCNFSM4JHMSQRQ> .

theofidry · 2019-11-20T15:58:48Z

Note however that I’m no GraphQL expert. I’m merely using one PHP implementation and was pointing out how it behaves as a reference

…

On Wed 20 Nov 2019 at 16:57, Théo FIDRY ***@***.***> wrote: It is, because you requested access to a field you don’t have access to. That said I would recommend to stick to the standards and not going for a new exotic solution On Wed 20 Nov 2019 at 16:54, Alan Poulain ***@***.***> wrote: > Some GraphQL systems have not implemented the visibility feature yet. > @theofidry <https://github.com/theofidry> having an error entry could be > a third solution. I don't really like it because it's not really an error > for me but we can discuss it. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#3227?email_source=notifications&email_token=ABHPVAPDGUK6PKY4OUIUKO3QUVMTLA5CNFSM4JHMSQR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESPGAA#issuecomment-556069632>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABHPVAKJFCF7OLQXBMW2XOLQUVMTLANCNFSM4JHMSQRQ> > . >

fredericbarthelet · 2019-11-20T17:38:41Z

I updated my PR, removing the implementation I had for GraphQL part for now.

@alanpoulain I would be happy to help implement the desired strategy on graphQL in a seperate PR related to the newly opened issue #3279 if that's OK for you :)

Any remaining comments on rest part on this one @teohhanhui ?

GregoireHebert · 2020-04-05T08:51:31Z

I like this, could you also add tests for writing operations?

dunglas

Sorry for the late review, but it's awesome!

chalasr · 2020-04-21T10:01:39Z

Finished in #3503, can be closed.

soyuka · 2020-04-27T14:04:44Z

thanks @fredericbarthelet

dunglas · 2020-11-09T16:53:53Z

For the record, I'm also in favor of returning null instead of throwing an error when a property cannot be accessed because of a security rule in the GraphQL subsystem.

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch from f53b219 to 071b71b Compare October 31, 2019 16:03

fredericbarthelet changed the title ~~WIP~~ Implement ApiProperty security attribute Oct 31, 2019

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch from 071b71b to 65cbde8 Compare October 31, 2019 16:05

antograssiot suggested changes Nov 1, 2019

View reviewed changes

teohhanhui reviewed Nov 1, 2019

View reviewed changes

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch 5 times, most recently from 67cf534 to c56fe21 Compare November 4, 2019 19:57

antograssiot reviewed Nov 4, 2019

View reviewed changes

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch 5 times, most recently from 7bffc91 to 4bcb395 Compare November 7, 2019 11:03

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch 2 times, most recently from 49ad651 to 28b4d38 Compare November 7, 2019 16:14

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch from 28b4d38 to 2aa51f9 Compare November 16, 2019 19:18

alanpoulain mentioned this pull request Nov 20, 2019

[GraphQL] Add a visibily attribute in ApiProperty to hide fields depending on the context #3279

Open

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch 2 times, most recently from b1f159a to 399ab49 Compare November 20, 2019 17:29

fredericbarthelet mentioned this pull request Nov 27, 2019

[Serializer] Add documentation on ApiProperty security attribute api-platform/docs#1000

Merged

Implement ApiProperty security attribute

5f1bbf1

fredericbarthelet force-pushed the use-api-property-for-attribute-acl-computation branch from 399ab49 to 5f1bbf1 Compare November 27, 2019 14:16

dunglas approved these changes Apr 5, 2020

View reviewed changes

GregoireHebert mentioned this pull request Apr 20, 2020

Implement ApiProperty security attribute #3503

Merged

soyuka closed this Apr 27, 2020

alanpoulain mentioned this pull request Mar 18, 2021

GraphQL association field security fixes + @ApiProperty security support #4143

Merged

Uh oh!

Uh oh!

Conversation

fredericbarthelet commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Nov 1, 2019

Uh oh!

antograssiot left a comment

Choose a reason for hiding this comment

Uh oh!

antograssiot Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

teohhanhui Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Nov 1, 2019

Uh oh!

antograssiot Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

antograssiot Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

fredericbarthelet Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

fredericbarthelet commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Nov 16, 2019

Uh oh!

teohhanhui commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Nov 18, 2019

Uh oh!

teohhanhui commented Nov 18, 2019

Uh oh!

alanpoulain commented Nov 18, 2019

Uh oh!

alanpoulain commented Nov 18, 2019

Uh oh!

alanpoulain commented Nov 18, 2019

Uh oh!

teohhanhui commented Nov 20, 2019

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

teohhanhui commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

teohhanhui commented Nov 20, 2019

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

teohhanhui commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

teohhanhui commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

theofidry commented Nov 20, 2019 via email

Uh oh!

alanpoulain commented Nov 20, 2019

Uh oh!

fredericbarthelet commented Oct 31, 2019 •

edited

Loading

teohhanhui Nov 1, 2019 •

edited

Loading

teohhanhui Nov 1, 2019 •

edited

Loading

fredericbarthelet commented Nov 7, 2019 •

edited

Loading

teohhanhui commented Nov 18, 2019 •

edited

Loading

teohhanhui commented Nov 20, 2019 •

edited

Loading

teohhanhui commented Nov 20, 2019 •

edited

Loading

teohhanhui commented Nov 20, 2019 •

edited

Loading