Fetch visible root trail record from database #6670
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you please add your explanation as a comment there too because again, this is something that looks irrational without comment if you just look at the code. |
aschempp
force-pushed
the
fix/visible-trail-permission
branch
from
b6ef7ed
to
dc4bd1a
Compare
Toflar
previously approved these changes
Dec 27, 2023
I don't think so. You grant access to a specific part of the tree in the user permissions. A user should not have access to nodes not visible in that tree. The trail view is actually a special case that should not exist and could not be implemented in a REST view (but is helpful to users) 🤷 |
|
Well in REST it could be implemented by having an extra resource that does a different check so I guess we're good here 👍 |
leofeyer
reviewed
Dec 27, 2023
That's true, something like a resource that only returns the title and ID or so. |
Toflar
approved these changes
Dec 27, 2023
|
Thank you @aschempp. |
leofeyer
added a commit
that referenced
this pull request
Jan 10, 2024
Description ----------- This was hell of a ride… which resulted in #6670, #6669, #6668, #6659, #6658 but I hope this is now working as intended. **I'd love to get someone to test this manually, maybe by comparing demo.contao.org settings to a local setup.** I did that for several days now but my brain feels muddy. This now essentially does the following: 1. Add a voter on page permissions (`chmod`) for `tl_article` and `tl_page`. Since the permissions are identical and all managed in `tl_page`, one voter can handle both cases. 2. Add a voter to limit access to page types 3. Add a voter to deny articles on pages without content composition or the article module in layout 4. Add a voter to deny access to `tl_content` if the parent article is not editable Since we now have correct permissions - we don't need a paste button callback on `tl_article` and `tl_page` - also don't need any button callbacks and can therefore use automatically generated operations - Voter no. 2 will make sure root pages are only added/pasted to the root node - Voter no. 2 will also make sure only one of the error pages exist per root page. This will e.g. automatically disallow moving a 404 page type from one root to another, if the other root already has a 404 page 😎 - Because we have voters for page types, we don't need the `FilterPageTypeEvent` anymore (it is now deprecated). There are some (intentional) behaviour changes and bug fixes: - #6666 is fixed automatically - #6667 is fixed automatically - #6674 is fixed automatically - A user can now copy articles if they have either `hierarchy` or `edit` permission. New or copies can only be pasted if a user has both permissions (see #6674). - The `new` global operation will automatically disappear if the user does not have access to create a page/article anywhere in the tree Commits ------- 9403bfd Page permissions f54d639 Fixed tests 7737273 Added tests 8c47c7b Tests 2dc5521 Tests 875fc2b Cannot change type to root outside root 9ebbfb8 Check if any record can be created on copy 9c0a3e9 Update core-bundle/src/Security/Voter/DataContainer/PagePermissionVot… e03853a Added tests for CreateAction in PagePermissionVoter d44fefb Finalized page permission tests a5c7b02 ¯\_(ツ)_/¯ cf4f9cc Reduce the complexity of the voters a046814 Add comments Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer
added a commit
to contao/core-bundle
that referenced
this pull request
Jan 10, 2024
Description ----------- This was hell of a ride… which resulted in contao/contao#6670, contao/contao#6669, contao/contao#6668, contao/contao#6659, contao/contao#6658 but I hope this is now working as intended. **I'd love to get someone to test this manually, maybe by comparing demo.contao.org settings to a local setup.** I did that for several days now but my brain feels muddy. This now essentially does the following: 1. Add a voter on page permissions (`chmod`) for `tl_article` and `tl_page`. Since the permissions are identical and all managed in `tl_page`, one voter can handle both cases. 2. Add a voter to limit access to page types 3. Add a voter to deny articles on pages without content composition or the article module in layout 4. Add a voter to deny access to `tl_content` if the parent article is not editable Since we now have correct permissions - we don't need a paste button callback on `tl_article` and `tl_page` - also don't need any button callbacks and can therefore use automatically generated operations - Voter no. 2 will make sure root pages are only added/pasted to the root node - Voter no. 2 will also make sure only one of the error pages exist per root page. This will e.g. automatically disallow moving a 404 page type from one root to another, if the other root already has a 404 page 😎 - Because we have voters for page types, we don't need the `FilterPageTypeEvent` anymore (it is now deprecated). There are some (intentional) behaviour changes and bug fixes: - contao/contao#6666 is fixed automatically - contao/contao#6667 is fixed automatically - contao/contao#6674 is fixed automatically - A user can now copy articles if they have either `hierarchy` or `edit` permission. New or copies can only be pasted if a user has both permissions (see #6674). - The `new` global operation will automatically disappear if the user does not have access to create a page/article anywhere in the tree Commits ------- 9403bfda Page permissions f54d6391 Fixed tests 77372738 Added tests 8c47c7b1 Tests 2dc5521e Tests 875fc2b5 Cannot change type to root outside root 9ebbfb89 Check if any record can be created on copy 9c0a3e9c Update core-bundle/src/Security/Voter/DataContainer/PagePermissionVot… e03853ab Added tests for CreateAction in PagePermissionVoter d44fefb6 Finalized page permission tests a5c7b024 ¯\_(ツ)_/¯ cf4f9ccf Reduce the complexity of the voters a046814d Add comments Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rendering visible root trails in a tree is a very special case. Because the user actually does not have read permissions on the given record.
We currently use
DataContainer::getCurrentRecordto fetch visible trail pages. However, if correct permissions are implemented (e.g. on tl_page), the user would not have access to that page. It would therefore not render this node, and therefore not render any nodes of any trees (because the root records are not readable).I first tried to add a
$checkPermissionsproperty togetCurrentRecord, but that does not really work with our cache of either records or exception.