Invalidate the pagemounts cache in the back end access voter when duplicating a page #7102
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
aschempp
requested changes
Apr 16, 2024
There was a problem hiding this comment.
I don't think using the session is a good idea, because it is very backend-specific. The best option @Toflar and I came up with, is to change the pagemountsCache. It should contain all page IDs with true or false value. That means
- SELECT all page IDs and set to false
- SELECT all allowed and set them to true
If a page ID is now not in the cache, we know the cache is outdated and needs to be updated/rebuilt.
|
Well, that was not what i wanted to do.... 🙄 |
|
There we go. |
lukasbableck
changed the title
can you explain that? 😅 I did a Pull Request on your repository (see lukasbableck#1), maybe that would simplify the code a bit? |
Store true/false in the pagemountsCache
Merge pull request #1 from aschempp/fix/page-access
Nevermind that, I messed up the commit history a little, but everything is good again 😅
👍🏼 |
aschempp
previously approved these changes
Apr 16, 2024
|
Yes, works as expected with your changes 👍 |
leofeyer
reviewed
Apr 17, 2024
leofeyer
changed the title
leofeyer
added
the
up for discussion
fritzmg
previously approved these changes
May 2, 2024
leofeyer
removed
the
up for discussion
leofeyer
requested changes
May 3, 2024
leofeyer
pushed a commit
that referenced
this pull request
May 7, 2024
Description ----------- This restores the permission handling we had in all previous Contao versions. After discussing with @Toflar, this is the only viable solution to fix our permission issues. Here are two examples that are imho impossible to fix otherwise. They are not a technical issue, but a logical one. 1. If you duplicate a page, its articles will be duplicated as well as its content elements. If you do not have access to some elements, the whole page and articles are duplicated, **but "random" content elements will be missing**. 2. even worse, when deleting a page, DC_Table tries to delete all child records and create an undo record for them. The ones that the user does not have access will not be deleted, **and will not be added to the undo record**. They are however still removed by our reviseTable, and will therefore be lost without the option to undo. In the same sense we could discuss to adjust the `undo` method instead of what we discussed in #7056 FYI @Toflar and me discussed that the same handling will not be possible in a regular REST/CRUD API. It would require special APIs for that, which can always be added if necessary. - Fixes #7101 - Probably also makes #7102 obsolete but correctly checking the IDs/updating the cache might still be correct. Commits ------- eb388c6 Skip permissions checks for child records 961033f Also remove permission check in undo operation 70d7577 Fixed duplicate variable name
leofeyer
pushed a commit
to contao/core-bundle
that referenced
this pull request
May 7, 2024
Description ----------- This restores the permission handling we had in all previous Contao versions. After discussing with @Toflar, this is the only viable solution to fix our permission issues. Here are two examples that are imho impossible to fix otherwise. They are not a technical issue, but a logical one. 1. If you duplicate a page, its articles will be duplicated as well as its content elements. If you do not have access to some elements, the whole page and articles are duplicated, **but "random" content elements will be missing**. 2. even worse, when deleting a page, DC_Table tries to delete all child records and create an undo record for them. The ones that the user does not have access will not be deleted, **and will not be added to the undo record**. They are however still removed by our reviseTable, and will therefore be lost without the option to undo. In the same sense we could discuss to adjust the `undo` method instead of what we discussed in contao/contao#7056 FYI @Toflar and me discussed that the same handling will not be possible in a regular REST/CRUD API. It would require special APIs for that, which can always be added if necessary. - Fixes contao/contao#7101 - Probably also makes contao/contao#7102 obsolete but correctly checking the IDs/updating the cache might still be correct. Commits ------- eb388c66 Skip permissions checks for child records 961033f3 Also remove permission check in undo operation 70d75771 Fixed duplicate variable name
aschempp
approved these changes
May 21, 2024
There was a problem hiding this comment.
This makes the assumption that the user will never vote something they are not allowed to see, simply because the back end should not render that. Otherwise that cache approach would not make sense, but I think we can agree that DC_Table would not render items that are not visible.
ausi
approved these changes
May 23, 2024
leofeyer
approved these changes
May 23, 2024
|
Thank you @lukasbableck. |
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.
If a user who is not an admin copies a page, the articles (and thus all content) are no longer copied, as it was the case in Contao 4. Instead a new empty article is created.
This is because the check whether the user is allowed to create the articles depends on the pagemountCache in the BackendAccessVoter, which is only set once per request and user.
As the page does not yet exist when the cache is first set and has not been reset after the page has been created, the check as to whether the user is allowed to access the newly created page fails.
This PR adds a check if a new page has been created before accessing the pagemountCache in the BackendAccessVoter and clears the cache if this is the case.
Steps to reproduce: