Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the page permission voters #6675

Merged
merged 23 commits into from Jan 10, 2024
Merged

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Dec 27, 2023

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:

@aschempp aschempp added this to the 5.3 milestone Dec 27, 2023
@aschempp aschempp requested a review from a team December 27, 2023 17:13
@aschempp aschempp self-assigned this Dec 27, 2023
Toflar
Toflar previously approved these changes Jan 2, 2024
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!! I haven't tested the permissions manually yet (because tbh I never really understood them anyway) but the code looks correct to me.
However, the new voters will require tests. Otherwise the codecov check will never pass.

@leofeyer
Copy link
Member

leofeyer commented Jan 4, 2024

There is a behavior change in how article permissions are applied:

Now if I log in as j.wilson, the copy icon is active (it was not before):

But when I try to copy an article, all the paste buttons are inactive:

@aschempp
Copy link
Member Author

aschempp commented Jan 5, 2024

That is exactly what I explained in my initial description, section 3 point 4 😉

@leofeyer
Copy link
Member

leofeyer commented Jan 5, 2024

If we already know that a user does not have permission to paste a duplicated article, what is the point of enabling the "copy" icon? It is completely useless for them and should therefore remain disabled.

@aschempp
Copy link
Member Author

aschempp commented Jan 5, 2024

If we already know that a user does not have permission to paste a duplicated article, what is the point of enabling the "copy" icon? It is completely useless for them and should therefore remain disabled.

That's correct, fixed in 9ebbfb8

@aschempp
Copy link
Member Author

aschempp commented Jan 8, 2024

Tests done with ~300 assertions! 🙈 This should be RTM now 🙃

Toflar
Toflar previously approved these changes Jan 8, 2024
@aschempp
Copy link
Member Author

aschempp commented Jan 8, 2024

Btw, @zoglo volunteered (without a timing) to have a look at frontend tests (Cypress) for the page and article permissions 🎉

@aschempp aschempp requested review from leofeyer and removed request for leofeyer January 9, 2024 14:12
@leofeyer leofeyer changed the title Page permissions Add page permission voters Jan 10, 2024
@leofeyer leofeyer changed the title Add page permission voters Add the page permission voters Jan 10, 2024
@leofeyer leofeyer merged commit e14f401 into contao:5.x Jan 10, 2024
17 checks passed
@leofeyer
Copy link
Member

Thank you @aschempp.

@aschempp aschempp deleted the feature/page-permissions branch January 11, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants