Bug #71461
closed
CategoryPermissionsAspect does not check changed rootUid of tree
Added by Marc Bastian Heinrichs over 8 years ago. Updated almost 8 years ago.
0%
Description
Using
'treeConfig' => array(
'rootUid' => 42,
),
you can change the root category of a tree, but CategoryPermissionsAspect::addUserPermissionsToCategoryTreeData does not taking care of checking, if this root is in the category mount points.
Files
| 71461-wip-01.diff (6.65 KB) 71461-wip-01.diff | Christian Kuhn, 2016-01-28 17:35 |
Updated by Oliver Hader over 8 years ago
What would be the expected behavior if the rootUid is not part of the category-mountpoints of a non-privileged backend user? Error message and fail or show the allowed ones (if any) instead?
Updated by Christian Kuhn about 8 years ago
i'm not very deep in all the category stuff, but i'll try to get an idea of whats going on there ...
Updated by Christian Kuhn about 8 years ago
understood. i'll try to come up with something.
Updated by Christian Kuhn about 8 years ago
Reproduce, this example will show behavior with tt_content categories:
Set up a category tree on root "0" (!) page where user and groups are located
[1] parent 1
[2] child 1 1
[3] child 1 1 1
[4] child 1 1 2
[5] child 1 2
[6] child 1 2 1
[7] child 1 2 2
Create a BE user/group that has access to pages, tt_content, gets a page mountpoint and some content elements.
Add tt_content category field to access list.
- [OK] User has no category mounts -> user can see all categories for a content element
- [OK] Set category mounts of user to "child 1 1" -> user can see only sub-tree "child 1 1"
sysext/css_styled_content/Configuration/TCA/tt_content.php, change "makeCatogorizable" line to: \TYPO3\CMS\Core\Utility\ExtensionManagementUtility::makeCategorizable('css_styled_content', 'tt_content', 'categories', ['fieldConfiguration' => ['treeConfig' => ['rootUid' => 5]]], true);
Where "5" is the uid of "child 1 2", or otherwise set this rootUid in TCA. This will define "child 1 2" as "entry point" to the tree that should be shown.
- [OK] User has no category mounts -> user can see node and sub tree "child 1 2"
- [FAIL] Set category mounts of user to "child 1 1" -> user still can see node "child 1 2", but not its sub tree. Expected: User should see no categories at all.
- [FAIL] Set category mounts of user to "child 1 2 1" (a sub node of rootUid node "child 1 2") -> user sees node "child 1 2" and "child 1 2 1". Expected: User should only see "child 1 2 1".
Updated by Christian Kuhn about 8 years ago
- File 71461-wip-01.diff 71461-wip-01.diff added
Ok, this is really hard to fix. I attached a patch that fetches the rootline of the $treeData TreeNode "rootNode" and checks if a category mount point is within this root line. If not, the root note is marked as "invalid" and thus the whole tree vanishes.
Issues:- If there is a category mount of a user to a sub tree of given root node, the whole tree will still vanish.
- There are additional not-yet-clear issues with icons not rendered and having a spinner icon all the time.
- If the root node itself is allowed, the existing lookUpCategoryMountPointInTreeNodes() recursion still fails, so a sub tree of a root note is still not correctly displayed.
General code architectur fails:
The signal that is used for the "aspect" gets the $treeData "rootNode" as object by reference. Just deleting or overriding the object with a different root node instance within the slot is not possible. The code expects the root node to always exist and the render part fails if root node is not an instance of RootNode but null. A "empty" tree without knots is not possible. The only way around that based on current code would probably be substituting the RootNode with a collection, which in turn then might be an empty collection, too. This change would however break API big time and it is just an idea.
Another funny thing is the transition from the tree object hierarchy to a different object hierarchy in DatabaseTreeDataProvider buildRepresentationForNode() which is then finally again transitioned to an array. In general, the whole tree class structure is ... well ... at least hard do understand, hard to follow and hard to change.
But here is the real bummer:
This category visibility restriction to sub-trees and category mount points was only implemented within FormEngine in 6.2. see #52634 and #52718 . This means there is no server side check whatsoever! A non admin user can easily set any category he wants by just tampering FormEngine POST data and DataHandler would happily accept that, since all this shit was only implemented half way through and just looks like a security feature, but it isn't. Also the list module shows all categories if they are not located on root page "0" and the whole category mount stuff only works if those categories are located on root page "0".
So ... how to proceed here? I basically see two options: Treat the "root node can be seen by user" as security issue. This means we MUST fix the DataHandler part, too and we may need to have a look at list module. Dealing with the code will be the next problem, i now hacked about a day in this area and continuously run into not-funny-hard-to-change programmatic issues. Or, tell people "displaying sub parts of categories is just a display thing and users can still set something else". For the latter, we could say "ok, then it is ok if the parent root node is shown even if user has no category mount on it, too".
Updated by Christian Kuhn about 8 years ago
proposal:
make issue public, declare 'category mount points for non-admin be_users' as display-only restriction and not as a security feature.
Updated by Helmut Hummel about 8 years ago
- Project changed from 1716 to TYPO3 Core
- Is Regression set to No
Updated by Christian Kuhn almost 8 years ago
- Status changed from New to Rejected
closed for now. this will not be done in the nearer future.