Harmonize Restriction Query

[solverat]

Q	A
Branch?	dev-master
Bug fix?	sort of
New feature?	no
BC breaks?	yes
Deprecations?	no
Fixed tickets	#118

[kathangeorg]

I'm not sure your change does solve the problem. I replaced the old function with your new one, and nothing changed. Files without any MembersGroup are listed anyway.

In my opinion the problem is line 72 $query->where('members_restrictions.targetId IS NULL ' . $additionalQuery);

If i change it to $query->where($additionalQuery); and of course remove the OR in line 67 the behaviour changes as I would expect.

If i understand correctly 'members_restrictions.targetId IS NULL keeps results that would be correctly removed via the LEFT JOIN?

[solverat]

Well it should change something 😄. I just modified the query.

Basically for assets:

If allowed groups are available: show assets with assigned groups OR unprotected assets If allowed groups are not available: only show unprotected assets

[kathangeorg]

Ok. Now it seems to work :). I will have a closer look tomorrow.

[kathangeorg]

I know we only talked about assets so far.

But if an object or a page has no MembersGroup assigned, currently any user (with or without groups) can see the objects or pages via addRestrictionInjection(...). Is this intended?

Wouldn't it be more consistent if all objects (assets, objects, pages) with no MembersGroup are not available to any user? Or should assets consciously be the exception here?

Perhaps it is more reasonable to keep the defaults (of this PR) and think about options for every object type (asset, document, object? I don't know, but I'm curious what's your opinion. @solverat, what do you think in this regard?

Just for testing purposes I temporary changed this part in my current project:

$assetQuery = '';
if ($listing instanceof \Pimcore\Model\Asset\Listing) {
    $assetQuery = sprintf('AND assets.path NOT LIKE "/%s%%"', RestrictionUri::PROTECTED_ASSET_FOLDER);
}
if (count($allowedGroups) > 0) {
    $subQuery = sprintf('(members_restrictions.targetId IS NULL %s)', $assetQuery);
    $queryStr = sprintf('%s OR (members_restrictions.ctype = "%s" AND members_group_relations.groupId IN (%s))', $subQuery, $cType, implode(',', $allowedGroups));
} else {
    $queryStr = sprintf('members_restrictions.targetId IS NULL %s', $assetQuery);
}

to that:

$unprotectedAssetsSubQuery = sprintf('AND assets.path NOT LIKE "/%s%%"', RestrictionUri::PROTECTED_ASSET_FOLDER);
$unprotectedAssetsQuery = sprintf('(members_restrictions.targetId IS NULL %s)', $unprotectedAssetsSubQuery);

if (count($allowedGroups) > 0) {
    // List protected assets / documents / pages ...
    $queryStr = sprintf(
        '%s (members_restrictions.ctype = "%s" AND members_group_relations.groupId IN (%s))',
        ($listing instanceof \Pimcore\Model\Asset\Listing ? $unprotectedAssetsQuery . ' OR' : ''), // ... or unprotected assets
        $cType,
        implode(',', $allowedGroups)
    );
} else {
    // Do not list any object / page
    // Not very elegant, but does the job
    $queryStr = ('1 = 0');
    if ($listing instanceof \Pimcore\Model\Asset\Listing) {
        // Only list unprotected assets
        $queryStr = $unprotectedAssetsQuery;
    }
}

[solverat]

@kathangeorg: Yes, that's totally intended. Why should we restrict all objects/documents by default? We have to tread assets differently, because they can be accessed from the outside, before symfony kicks in. That's why we only allow restricted assets located under restricted-assets.

I think, this PR harmonizes the "query injector" <=> "asset url generator" behavior, which should be enough. To restrict everything by default was never the idea and would be a BC break from hell anyway.

[kathangeorg]

@solverat thanks for the clarification :). I was just curious. I think sometimes it can be useful to restrict (some or all) objects or pages by default.

To restrict everything by default was never the idea and would be a BC break from hell anyway.

There you are right. Maybe this could be optional in the future but of course that would be an issue for a separate feature request. And of course, if necessary, I can simply overwrite the corresponding classes, which I may do in my current project in the opposite direction (not restricted by default).

Thanks for your time and the harmonization/fix in this PR.