feat(security): introduce voter on pre_read

[thomasbeaujean]

Q	A
Branch?	main
Tickets	N/A
License	MIT
Doc PR	https://github.com/api-platform/docs/pull/1541/files

First, thanks a lot for the work done with API Platform.

While working with api platform, i noticed that the security on operation are applied after the "read" of the input, (done in ReadListener), so my dataCollectionProvider is processed even if the user is not allowed.

I understand and agree with this architectural choice. In order to have a voter that can handle all use case, the subject must potentially be given, and then the voter can do the right choice.

However, in some use cases, I do not even need the subject to vote:

• GetCollection / Get / Post / etc.: using RBAC, the voter only use the ROLE of the user
• GetCollection: the provider of the data can not handle the current user and throw an exception, it could be avoided using a voter before the dataProvider
• GetCollection: the dataProvider requires some "heavy" computation process, and at the end, the voter says no, this computation could be avoided (I could handle this kind of user in the dataProvider, but it should not here in the first place, and I do not want to write code to check unwanted users)

In summary, I wanted to avoid to consume any computation process (dataProvider or persister) if the voter says no in the first place.

What I found looking in the events :

• the ReadListener has the priority kernel.request:4
• the DenyAccessListener has the priority kernel.request:3 (onSecurity)

My proposal:

• add a new "security_pre_read" called before the ReadListener (with event kernel.request:5 => onSecurityPreRead)
• it can be enabled at the ressource level only
• the voter will be called before doing anything (the use cases described before)
• the voter will have a subject that is null (not a problem in the use cases described before)
• if the voter need the subject, just use the standard "security"
• the "security_pre_read" and the others "security" can be used complementary

[GregoireHebert]

I think this could be part of 2.7/3.0 ping @vincentchalamon @dunglas WDYT?

[alanpoulain]

I think it can be done easily with an event listener. I don't think this feature is asked a lot since it only matters for performance reasons, so I'm against it. We already have too much security attributes.

[vincentchalamon]

We already have too much security attributes.

I also think those security attributes need a refacto, maybe on 3.1, WDYT @alanpoulain?

[alanpoulain]

Why not, to avoid having 15 security attributes on the Operation class. Only if we find a way to have a general implementation, working automatically for all stages (read, validate, write...).

[GregoireHebert]

Performance and avoid queries that should be filtered for the current user to be executed without the where clause. I agree on the "too many security arguments" part though.

Like If I do a getCollection for orders on a e-commerce with large users base, then I potentially retrieved every orders. I could skip the query execution when I don't have a user, but then it's biased, because security should prevent me tot even get there in the first place.

Maybe the security part coud be improved by an attribute of its own. As mentioned IRL by @dunglas One could use security.yml in the meantime.

[alanpoulain]

Performance and avoid queries that should be filtered for the current user to be executed without the where clause.

I think it's doable with a Doctrine extension though: if there is no user, you can add a where returning no result.

[thomasbeaujean]

Hi, just to specify one of my other use case:

• i have a route that is a GET, this route requires some processing (1 to 2 seconds)
• the data is stored in a redis
• the user is not allowed to use this route, because one of its attribute is false (not using RBAC here)
• my api receives lot of queries per second, so I need to optimize everything

So:

• I can not use doctrine filter (redis used here)
• I can not use security.yml (no RBAC)
• The data can not be filtered easily by current user (it would kill performance for a legitimate user)

I know this use case is not common, I just wanted to explain why I did not used the techniques mentioned earlier.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[thomasbeaujean]

This PR is stalled, so I close it.

[maks-rafalko]

My 2 cents to bring attention to this issue once again.

We are now using APIP 3.0, and were completely surprised our Provider is executed when API ultimately returns 403.

Why it hurts, because our custom provider goes to separate API by doing HTTP request, and all of this is being done without a reason - just to return 403 status a bit later and throw away results of HTTP queries.

What we are going to do is to somehow implement our custom logic and run Security checks before ReadListener, but this will be a workaround because of lack of native APIP support for this feature.

I understand that introducing new security attributes might be not the best way, however I still think the issue is here and needs an attention.

[maks-rafalko]

Ok, here is the code that we wrote for our project. It rejects the Request as early as possible, not calling State Provider.

If the security contains object and relies on ReadListener to be executed, this custom listener passes the job farther to original DenyAccessListener.

Note: it works only for Get, GetCollection operations, so please modify for your needs:

HighPriorityDenyAccessListener.php

<?php

declare(strict_types=1);

namespace App\EventListener;

use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use ApiPlatform\Symfony\EventListener\DenyAccessListener;
use ApiPlatform\Symfony\EventListener\ReadListener;
use ApiPlatform\Symfony\Security\ResourceAccessCheckerInterface;
use ApiPlatform\Util\OperationRequestInitiatorTrait;
use ApiPlatform\Util\RequestAttributesExtractor;
use Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener;
use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\EventListener\RouterListener;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;

/**
 * Because of the issue explained here {@see https://github.com/api-platform/core/pull/4735},
 * the current behavior of APIP is to first call the provider, and next check the security.
 *
 * In majority of the cases, this is not what we want to do - executing complex DB queries and HTTP requests
 * to 3rd-party APIs to just return 403.
 *
 * This listener has a higher priority to be executed **before** {@see ReadListener} to fail fast.
 */
#[AsEventListener(event: RequestEvent::class, priority: self::PRIORITY)]
class HighPriorityDenyAccessListener
{
    use OperationRequestInitiatorTrait;

    /**
     * Priority 7 is because to run **after** {@link RouterListener} (priority 32), {@link TraceableFirewallListener} (priority 8)
     */
    public const PRIORITY = 7;

    public function __construct(
        private readonly ResourceAccessCheckerInterface $resourceAccessChecker,
        ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory,
    ) {
        $this->resourceMetadataCollectionFactory = $resourceMetadataCollectionFactory;
    }

    public function __invoke(RequestEvent $event): void
    {
        $request = $event->getRequest();

        if (!$attributes = RequestAttributesExtractor::extractAttributes($request)) {
            return;
        }

        $operation = $this->initializeOperation($request);

        if (!$operation instanceof Get && !$operation instanceof GetCollection) {
            return;
        }

        $isGranted = $operation->getSecurity();
        $message = $operation->getSecurityMessage();

        if ($isGranted === null) {
            return;
        }

        $extraVariables = [];
        $extraVariables += $request->attributes->all();
        $extraVariables['request'] = $request;
        // left for BC to be present as variables on Expression Language, but expected to be null at this point
        $extraVariables['object'] = $request->attributes->get('data');
        $extraVariables['previous_object'] = $request->attributes->get('previous_data');

        /**
         * if `security` expression contains usage of `object`, we should pass the logic to original {@see DenyAccessListener}
         */
        if (str_contains($isGranted, 'object')) {
            return;
        }

        if (!$this->resourceAccessChecker->isGranted($attributes['resource_class'], $isGranted, $extraVariables)) {
            throw new AccessDeniedException($message ?? 'Access Denied.');
        }
    }
}