SubResource and security

[akazik]

I have entities A with one-to-many B, B is defined as a SubResource in A.
Both A and B have security checks which ensure that:

• the collection can only be accessed by a admin
• you can only access it if (the associated) A's id is your own

The following routs work as excepted (regular user):
/A (no access)
/A/id (only possible for own id)
/B (no access)
/B/id (only possible if A has your own id)

But the collection built by the SubResource is not restricted:
/A/id/B (can be accessed by everyone)

How can it be secured?

Code for security so far:

 * @ApiResource(
 *     attributes={
 *       "access_control"="is_granted('ROLE_USER')"
 *     },
 *     collectionOperations={
 *         "get"={"method"="GET", "access_control"="is_granted('ROLE_ADMIN')"},
 *         "post"={"method"="POST", "access_control"="is_granted('ROLE_ADMIN')"},
 *     },
 *     itemOperations={
 *         "get"={"method"="GET", "access_control"="is_granted('ROLE_USER') and object.getId() == user.getAccount().getId()"}
 *     }
 * )

[soyuka]

Something might be missing here as the DenyAccessListener does take subresources into consideration:

core/src/Security/EventListener/DenyAccessListener.php

Line 72 in 337ffdf

$isGranted = $resourceMetadata->getCollectionOperationAttribute($attributes['subresource_operation_name'], 'access_control', null, true);

I think that you need to add a context for the subresource on the subresource operation name.
Might be linked to #1617 (where to declare subresource context).

[akazik]

I added
* "api_accounts_emails_get_subresource"={"method"="GET", "access_control"="is_granted('ROLE_USER') and object.getA().getId() == user.getAccount().getId()"},
to B's collectionOperations and it's executed, the problem is though that object is neither A nor B (but the Paginator) and thus I can't check if the objects belong to the user.

I would have preferred an check with A being the object since it will load something related to A.

Thanks for the reply, and a documentation about this would be also helpful.

Edit: I think I can work around it by creating a new expression language function which checks every item of the paginator.

[soyuka]

Edit: I think I can work around it by creating a new expression language function which checks every item of the paginator.

Won't performances be slightly decreased by that? I think we had a recent issue about this and the paginator. Anyway I'd like this to be consistent with how the grant on Collection (that aren't part of a subresource) works.

/edit found #1481 ping @takeit

[dunglas]

Won't performances be slightly decreased by that?

It will, and it's why it has been done this way initially. It may also break the pagination. If you want to check something for all rows, better use an extension to tweak the SQL query.

[dunglas]

We may add a foreach helper function or something like this in ExpressionLanguage (with a big warning in the docs explaining when it should not be used).

[soyuka]

We may add a foreach helper function or something like this in ExpressionLanguage (with a big warning in the docs explaining when it should not be used).

Meh shouldn't we enforce users to do proper querying instead? Even an additionnal query to check if the user has the given rights seems better then checking a whole collection to me.

[BatsaxIV]

Any news on this?
I have the same problem, it would nice to be able to access the subresource if the access is granted on the main ressource :

/A/id => Access granted
/A/id/B => also access granted (to avoid checking all subresources)

[matlar83]

Really can't understand why "object" is a Paginator instance.
This way is not possible to fine secure subresources in annotation like this:
"api_students_marks_get_subresource"={"method"="GET", "access_control"="is_granted('VIEW', object)", "path"="/students/{id}/all-marks"}

I think "object" should be a Student instance, instead.

How can I secure the subresource with a Paginator object?

[soyuka]

For now, and because it is a complex issue if we don't want to introduce bad design I really suggest that you add an event listener (PRE_READ for example) that checks if the user has access to the request.

Yes, it's convenient to directly use the access_control but it's not that easy to implement on subresources and will definitely take time.

[matlar83]

Could you please provide a short example on how to perform the security check in the event listener? Thank you Il giorno mar 21 ago 2018 alle ore 14:09 Antoine Bluchet < [email protected]> ha scritto:

…

For now, and because it is a complex issue if we don't want to introduce bad design I really suggest that you add an event listener (PRE_READ for example) that checks if the user has access to the request. Yes, it's convenient to directly use the access_control but it's not that easy to implement on subresources and will definitely take time. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1649 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aomk7KbgKIR7prPyXlyApLrUTYjL3u1Oks5uS_iOgaJpZM4Rebtl> .

[Lobosque]

@matlar83 I am going through the same process and I wonder if you were able to write your access rules in the PRE_READ event listener?
I wonder in cases where I get something like /api/albums/4/songs, how do I get a hold of the actual Album Entity with id 4 to do my security checks.

[Lobosque]

@soyuka also, a weird thing to note is that using PRE_READ, POST_READ, PRE_DESERIALIZE, POST_DESERIALIZE never call my subscriber function. only PRE_VALIDATE and POST_VALIDATE works. I thought these were supposed to be used on POST/PUT routes?

[mvkirk]

PRE_READ and POST_READ work. The subscribe event is on the KernelEvents::REQUEST and not the KernelEvents::VIEW for those.

https://api-platform.com/docs/core/events

[mvkirk]

Tell me if I did a mistake. In the access_control in addition of the object you also have access to variables in your path. For example /A/{id}/B. It's useful if the id variable depends on your user.

 *     attributes={
 *       "access_control"="is_granted('ROLE_USER')"
 *     },
 *     collectionOperations={
 *         "get"={"method"="GET", "access_control"="is_granted('ROLE_ADMIN')"},
 *         "post"={"method"="POST", "access_control"="is_granted('ROLE_ADMIN')"},
 *     },
 *     itemOperations={
 *         "get"={"method"="GET", "path"="/A/{id}/B", "access_control"="is_granted('ROLE_USER') and user.getAccount().getId() == id"}
 *     }
 * )

Otherwise if we need more complicated access control, we should use some events.

[juanwilde]

Maybe is too late but I did it by passing the id to the voter with this

"get"={"method"="GET", "path"="/users/{id}/expense_types", "access_control"="is_granted('USER_EXPENSE_TYPES_READ', id)"},

And then in the Voter

if (self::USER_EXPENSE_TYPES_READ === $attribute) {
      return $user->getId()->__toString() === $subject;
}

This is useful if the ID is a UUID object instead an integer, for example

[soyuka]

Closing in favor of #2706