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
Fix ACL checks for pagebreak, articles com_content layouts (and their editor XTD buttons) blocking legitimate editors #17854
Conversation
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:
|
Thought so. |
Sorry for making @brianteeman nervous... I thought I had read enought about that checks in component.php but I missed that infos. 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: Articles modal: 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. |
The ACL checks inside the creation of the buttons protect nothing
Now about protecting the viewing of the layouts
users that should have access are
does the current ACL check do the above well no ? This PR fixes 1, but 2 is still not done $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 |
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). |
i understand your point, you are right, Can you also comment on using what the view is using
https://github.com/joomla/joomla-cms/blob/staging/components/com_content/controller.php#L98 The difference is that we do not check if user was allowed edit a specific article
|
I miss the point: what's the need in protecting from something not dangerous? In case of pagebreak:
|
anyway i have suggested code above that can be used both 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 ? |
i have updated the code above, it had some missing negations operator ! |
@ggppdk: sure we can update this PR, I'll do. Please, @mbabker or @brianteeman could you explain? |
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). |
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, |
@ggppdk: i fixed and cleaned your code in content.php. Tks for pointing out the lacking edit permission check. @mbabker: Your point was clear in your first post, tks for repeating and adding details. I do not want to waste your time. |
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. |
{ | ||
$link = 'index.php?option=com_content&view=articles&layout=modal&tmpl=component&' | ||
. JSession::getFormToken() . '=1&editor=' . $name; | ||
JFactory::getApplication()->enqueueMessage(JText::_('JERROR_ALERTNOAUTHOR'), 'warning'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not enqueue a messsage here, only execute return
Enqueueing a no access message should only be added when viewing the layout
$button->options = "{handler: 'iframe', size: {x: 500, y: 300}}"; | ||
|
||
return $button; | ||
JFactory::getApplication()->enqueueMessage(JText::_('JERROR_ALERTNOAUTHOR'), 'warning'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this message should be removed, only execute return
I have tested this item ✅ successfully on 9b44db6 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17854. |
Title of this PR is no longer valid please change title to |
I have tested this item ✅ successfully on 9b44db6 Without PR:With PR:This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17854. |
RTC after two successful tests. |
Pull Request for Issue #17754
Summary of Changes
Removed unnecessary ACL checks from pagebreak and article editor buttons and in content component main file.
Testing Instructions
Create new category ONE and set permissions to registered users group to 'create', 'edit' or 'edit.own'.
Create a menuitem to Create (or edit, or edit.own) a new article in that category.
Create a 'Registered' user, login and go to that menuitem.
Expected result
Pagebreak button should be displayed.
Actual result
Pagebreak and article buttons are NOT displayed.
Documentation Changes Required
none
Notes
Interesting arguments by @ggppdk are here #17754 (comment) and here #10653 (comment)