Inconsistent behaviour for Assets inside restricted-assets folder with no (inherited) MembersGroup

[kathangeorg]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no

Assets with no (inherited) MembersGroup in an Asset listing ARE ALLOWED (via addRestrictionInjection) for any user.

But Assets with no (inherited) MembersGroup ARE NOT ALLOWED for any user (in getElementRestrictionStatus) e.g. if the user tries to download the asset (link generated via members_generate_asset_url).

In my opinion this behaviour should be consistent.

Files with no (inherited) MembersGroup should be either

• ALLOWED (my favorite) or
• NOT ALLOWED

for any user everywhere.

This would be a breaking change for existing installations.

[solverat]

@kathangeorg thanks for your report. I just tested it and you're right.

Initial State

<?php

 /** @var Asset\Listing $listing */
$listing = new Asset\Listing();
$listing->onCreateQuery(function (QueryBuilder $query) use ($listing) {
    $this->container->get(RestrictionQuery::class)
        ->addRestrictionInjection($query, $listing, 'assets.id');
});

$listing->addConditionParam('assets.path LIKE "/restricted-assets/assets/%"');
$availableObjects = $listing->getAssets();

dump(count($availableObjects)); 
// => returns 3 ("sub-folder" + two images)

$download1 = Asset::getById(15);
dump($this->container->get(RestrictionUri::class)->generateAssetUrl($download1, false, true));
// => returns empty

$download2 = Asset::getById(16);
dump($this->container->get(RestrictionUri::class)->generateAssetUrl($download2, false, true));
// => return empty

That's actually your situation, right?

---

The reason for this inconsistent behavior comes with this:

https://github.com/dachcom-digital/pimcore-members/blob/de2caf4e7ac63ec1396778fe69f9404e708aed10/src/MembersBundle/Manager/RestrictionManager.php#L110-L112

Every asset within the restricted-assets folder is restricted by default. Because pimcore allows a direct asset call via .htaccess, we can't fetch asset requests in PHP.

As you mentioned, changing this behavior would be a breaking change.

However, a BC break could be bearable, if we keep some security aspects. For example, we could improve the addRestrictionInjection() method with an additional join and also restrict assets per default.

If we're going to change the getElementRestrictionStatus method, maybe thousands of assets are available after an upgrade.

It's a quite tough decision and I think we should go with the injection change.

[solverat]

@kathangeorg I just created an PR for that.

[kathangeorg]

Thanks for testing :).

Yes, that's actually my situation.

I think maybe it is the right choice to improve addRestrictionInjection() instead of not restricting assets per default.

But what do you mean when you say "Because pimcore allows a direct asset call via .htaccess, we can't fetch asset requests in PHP."? As far as I can see, the restricted-assets folder is secured via an .htaccess file generated in the installation routine of the bundle. Is there another way of directly accessing the files inside?

If the new default behaviour is restricting access for all files without an MembersGroup I think an option to change that behaviour to the opposite would be great.

I also commented on your pull request, because I think it doesn't solve the problem right now but I will have a closer look tomorrow.

[solverat]

But what do you mean when you say "Because pimcore allows a direct asset call via .htaccess, we can't fetch asset requests in PHP."? As far as I can see, the restricted-assets folder is secured via an .htaccess file generated in the installation routine of the bundle.

That's right. And currently the only way to protect them properly.

Is there another way of directly accessing the files inside?

Nope, only via data stream (e.g. file_get_contents)

If the new default behaviour is restricting access for all files without an MembersGroup I think an option to change that behaviour to the opposite would be great.

Yes, that's what i thought too.