This makes me very nervous without knowing why/when these acl checks were placed in the code originally

At least one of those cases prevents potential read ACL violations. So no, they aren't 100% unnecessary.

Additional context:

• 3.4.5 security release
• LivioCavallo/joomla-cms@8acabc8 adding ACL checks to plugins
• LivioCavallo/joomla-cms@00a011f restoring the removed check from the previous commit which reverted part of the 3.4.5 security fixes

Thought so.

Sorry for making @brianteeman nervous... I thought I had read enought about that checks in component.php but I missed that infos.
Thanks @mbabker for all that useful context info!

I tried to make it better: now code checks if user has permissions to create at least in a category or he has component create permission.

If enable_category in article creation params is set, it would be preferable to check against the default/specific category but how to pass category info to modal view? How to retrieve it?

This PR still is no good in my view, you are removing ACL checks. You need to ensure that you are not restoring a read ACL violation; until then at best this PR is WIP.

As @ggppdk noted in #10653 (comment), pagebreak and modal layouts are "proxied" to the backend models/views: https://github.com/joomla/joomla-cms/blob/staging/components/com_content/controller.php#L32-L42

Pagebreak:
There is no need for pagebreak button to make any ACL check, since this button must be usable when editing any article.
The current staging check just protects creating a link to the pagebreak layout; that's not so useful as you can anyway type the link to the layout in your browser!
Something like: ?option=com_content&view=article&layout=pagebreak&tmpl=component&e_name=jform_articletext and this will not open any security risk as it will just display a form that won't show any information and cannot modify any record data.

Articles modal:
for this layout currently the session token is checked for frontend; you can steal the token from the login form and use a link like: ?option=com_content&view=articles&layout=modal&tmpl=component&{formtoken}=1
but an unauthorised user will be stopped in content.php (by the checks I firsty erroneously removed then I fixed and re-introduced with last commit).

Moreover since user's view access levels are used by the backend model (https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_content/models/articles.php#L236-L255), the user is shown a list that only includes its viewable articles.

This view cannot modify in no way any record data.

@brianteeman
@mbabker

The ACL checks inside the creation of the buttons protect nothing
they protect a link that the user can anyway type in the browser address bar

• so we can remove the ACL checks from them (or apply the ACL fix i will suggest below) Note: i do not say that this would apply for all XTD buttons, e.g. someone could author a button to add a link that has some secret information or do some extra actions on the side, so i was only speaking of the pagebreak and articles-modal links !

Now about protecting the viewing of the layouts

• why protect the page break layout, what does it shows that needs protection ? but again you can keep the ACL check here despite not need (again see my suggested fix below)
• about the articles modal layout, instead of removing ACL check, i said that it is best to fix this somehow , the listing is already protected by viewing access levels but i understand that this is not enough, still the current ACL check is broken !!, it fails to give access to people that should be granted access

users that should have access are

1. users that can create 1 article (in any category)
2. users that can are edit 1 article (any)

does the current ACL check do the above well no ?

This PR fixes 1, but 2 is still not done
here is the full fix:

$check_create_edit = ($input->get('view') === 'articles' && $input->get('layout') === 'modal')
 || ($input->get('view') === 'articles' && $input->get('layout') === 'modal');

if ($check_create_edit)
{
	// *** The following should be in XTD buttons too

	// Can create in any category
	$canCreateRecords = $user->authorise('core.create', 'com_content')
	 && count($user->getAuthorisedCategories('com_content', 'core.create')) > 0;

	// Instead of checking edit on all records, we can use **same** check as the form editing view
	$values = (array) \JFactory::getApplication()->getUserState('com_content.edit.article.id');
	$isEditingRecords = count($values);

	$hasAccess = $canCreateRecords || $isEditingRecords;
	if (!$hasAccess)
	{
		JFactory::getApplication()->enqueueMessage(JText::_('JERROR_ALERTNOAUTHOR'), 'warning');
	}
}

@LivioCavallo
please try the in both the XTD button creation and to content.php and it should work
buttons should show to authorized users
and everybody will be happy

The ACL checks in the button are intended to contextually hide a button that the user does not have authorization to use. Yes, a user may be able to manually craft the link and enter it in their browser, which means the button's ACL check can't be relied on as the only check in the system, but that does not mean we should show a button a user doesn't have permission to use (same logic as when a user lacks edit permissions the item titles aren't links but plain text).

@mbabker

i understand your point, you are right,

Can you also comment on using what the view is using

• for checking edit on existing records

https://github.com/joomla/joomla-cms/blob/staging/components/com_content/controller.php#L98

https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Controller/BaseController.php#L502-L506

The difference is that we do not check if user was allowed edit a specific article
instead we check if user is currently allowed edit for at least 1 article

	// Instead of checking edit on all records, we can use **same** check as the form editing view
	$values = (array) \JFactory::getApplication()->getUserState('com_content.edit.article.id');
	$isEditingRecords = count($values);

I miss the point: what's the need in protecting from something not dangerous?

In case of pagebreak:

• the check inside the button cannot protect anything sensitive: it just inhibits inserting a link in the editor, but the same link can be typed directly in browser and anyway that link to the layout is not dangerous (see here below)
• as far as I can see, the pagebreak layout won't be dangerous even if displayed to 'public' as it is not revealing any information and cannot modify anything if called outside the editor!
  In fact, clicking the Insert Pagebreak button in the pagebreak layout, calls insertPagebreak function in media\com_content_js\admin-article-pagebreak.js that, if no editor is present (=pagebreak layout called directly, outside editor), will just close the form and redirect to the home page.
• if user is in the editor he has permissions to do so (to access pagebreak button and layout) as the form view already has performed ACL checks.

@LivioCavallo

anyway i have suggested code above that can be used both
in the buttons and in the component entry point (content.php)

it fixes the wrong access blocking and it addresses everybody concerns, we can check it (e.g. for any syntax error) and update your PR ?

@LivioCavallo

i have updated the code above, it had some missing negations operator !

@ggppdk: sure we can update this PR, I'll do.
But I'd like to know a reason to keep that checks in pagebreak and article buttons even if they are unnecessary.
I cannot find a reason for their necessity (analogy with item title does not apply, as no additional info is shown nor any data can be modified); can you?

Please, @mbabker or @brianteeman could you explain?
I'd appreciate very much. Thanks.

Again, the logic is if the user is not authorized to perform an action, then the button related to that action is hidden as well. At least one of the buttons provides the possibility to access a layout where a user may be able to read view they should not otherwise have access to view. In addition to the ACL checks in place when that request to render the view is processed, the button itself should check to determine that the user is authorized to access that view in the present context and hide itself if the user is not authorized (this prevents a user from clicking a button and seeing a "not authorized" error message). Yes, a user may directly input any data into the editor and that is not stripped just because the user inputting it doesn't have ACL read access to that function or content item, but that doesn't mean that just because a user may be able to craft links to stuff doesn't mean we should try to give the user a reasonable experience and not show them actions they can't perform. Again, same example as before, we disable the link to the edit form for content a user doesn't have edit access for, but that doesn't stop a user from being able to build the edit link manually (necessitating that there also be an ACL check when the edit view is actually requested).

... In addition to the ACL checks in place when that request to render the view is processed, the button itself should check to determine that the user is authorized to access that view in the present context and hide itself if the user is not authorized (this prevents a user from clicking a button and seeing a "not authorized" error message).

Correct ! it is quite bad to show the buttons and user to click them and get a Not authorized message

just in this case if we reached the point at which the editor buttons are request to be created,
then the create / edit ACL checks on the article must have be done already, so these seem to be redudant

@ggppdk: i fixed and cleaned your code in content.php. Tks for pointing out the lacking edit permission check.
I still think that we could skip ACL checks in pagebreak button, but anyway I put that checks in the last commit, so I hope the PR will be accepted.

@mbabker: Your point was clear in your first post, tks for repeating and adding details.
I think that when we are to display the pagebreak and article buttons all needed ACL checks have already been done.
So in short I do not agree completely.

I do not want to waste your time.
So I modified this PR to make checks in article and pagebreak button (event if I think it's unnecessary), to verify permissions to create or edit, following your indications and using @ggppdk suggested code.

With the last commit we have checks in buttons and in content.php so that a registered user should now be able to display pagebreak and article buttons for categories he has permissions.
Can you please confirm?

I have tested this item ✅ successfully on 9b44db6

@LivioCavallo

Title of this PR is no longer valid

please change title to
Fix ACL checks for pagebreak , articles com_content layouts (and their editor XTD buttons) blocking legitimate editors

I have tested this item ✅ successfully on 9b44db6

Without PR:

With PR:

RTC after two successful tests.