[#17754] - Pagebreak editor button Authorization problem
- Closed
- 4 Sep 2017
- Medium
- Build: staging
- # 17754
Steps to reproduce the issue
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 button is NOT be displayed.
System information (as much as possible)
J! 3.7.5
Comments and proposed solution
The problem is that the plugin button code does authorisation check at component level, not at asset level.
We could solve adding proper checks in pagebreak plugin button onDisplay function this way:
public function onDisplay($name, $asset, $author)
{
$user = JFactory::getUser();
// Original code was simply:
/* if ($user->authorise('core.create', 'com_content')
|| $user->authorise('core.edit', 'com_content')
|| $user->authorise('core.edit.own', 'com_content'))
*/
// New code to authorise on asset - BEGIN
$app = JFactory::getApplication();
$currView = $app->input->get('view');
// If a_id is missing, $articleId is forced to 0, so supposing a malformed url, lacking a_id=0, to create a new article
$articleId = (int) $app->input->get('a_id');
if ($currView === 'form' && $articleId === 0)
{
// Retrieve current menuitem
$currMenuItem = empty($menu = $app->getMenu()) ? null : $menu->getActive();
// Retrieve and sanitize 'enable_category' param
$enableCategory = empty($currMenuItem) ? 0 : $currMenuItem->params->get('enable_category');
$enableCategory = empty($enableCategory) ? 0 : $enableCategory;
// If we have a valid 'enable_category' we can retrieve 'catid' from menuitem
$catId = $enableCategory ? $currMenuItem->params->get('catid') : 0;
// If the menuitem does not specify a predefined category, then use 0
$catId = empty($catId) ? 0 : $catId;
if ($enableCategory && $catId)
{
$authorised = $user->authorise('core.create', 'com_content.category.' . $catId);
}
else
{
/* User is authorised if it has component permission OR if it has permissions at least on a category.
Strictly speaking we should authorise just the exact category in which user has permissions to create
but if user has not permissions to create the the form view is forbidden too and we never reach this code row.
Moreover doing so is a bit faster */
// TODO: authorise on the category selected by user in editor, if it's not too time-expensive
$authorised = $user->authorise('core.create', 'com_content') || count($user->getAuthorisedCategories('com_content', 'core.create'));
}
}
else
{
$authorised = $user->authorise('core.edit', $asset) || ($user->authorise('core.edit.own', $asset) && $author === $user->id);
}
if ($authorised)
// New code to authorise on asset - END
{
JFactory::getDocument()->addScriptOptions('xtd-pagebreak', array('editor' => $name));
$link = 'index.php?option=com_content&view=article&layout=pagebreak&tmpl=component&e_name=' . $name;
$button = new JObject;
$button->modal = true;
$button->class = 'btn';
$button->link = $link;
$button->text = JText::_('PLG_EDITORSXTD_PAGEBREAK_BUTTON_PAGEBREAK');
$button->name = 'copy';
$button->options = "{handler: 'iframe', size: {x: 500, y: 300}}";
return $button;
}
}
So we'll have a fine-grained authorisation.
Additional comments
Similar issues are in other buttons.
But do we really need authorisation checks here?
Or could we just do as Readmore plugin does: NO authorisation check at all ?
| Labels |
Added:
?
|
||
| Category | ⇒ | com_plugins |
Yes, I saw components/com_content/content.php but before touching it I wanted to know someone else thoughts about this ACL checks. Thanks for sharing your arguments.
Good, so we agree.
I hope someone will get a PR ready for components/com_content/content.php and a PR to remove ACL checks from this (and probably other) XTD editor plugin.
I hope to have time to do it.
The only argument for the ACL check there is not to show the button if user can not use the layout because user will click it and then user will be blocked by ACL check at
components/com_content/content.php
so it would be bad UX to have the above happen,
but if the user is already viewing the edit form this bad UX case can not happen
in any case if there is an ACL check for the button,
then this ACL check should be same as with the ACL check at
components/com_content/content.php
which is blocking access to article 'pagebreak' and articles 'modal' layouts
now to add button to 'pagebreak' / 'articles' modal layout you have at
plugins/editors-xtd/pagebreak/pagebreak.php
plugins/editors-xtd/article/article.php
if ($user->authorise('core.create', 'com_content')
|| $user->authorise('core.edit', 'com_content')
|| $user->authorise('core.edit.own', 'com_content'))
but when viewing these layouts you have at
components/com_content/content.php
(which more restrictive)
if (!$user->authorise('core.create', 'com_content'))
{
JFactory::getApplication()->enqueueMessage(JText::_('JERROR_ALERTNOAUTHOR'), 'warning');
return;
}
To summarize
even if you make the ACL changes that you suggest for adding the buttons
- still you will not fix the issue of legitimate users being blocked from the article-pagebreak, articles-modal layouts because of the different ACL check when the layouts are actually viewed:
components/com_content/content.php
if (!$user->authorise('core.create', 'com_content'))
{
JFactory::getApplication()->enqueueMessage(JText::_('JERROR_ALERTNOAUTHOR'), 'warning');
return;
}
Evidently I did not explain well my point of view: I do not propose additional ACL checks. I propose not to check in button code at all!
I expressed this point of view with my last question in the 'Additional comments' in the description of this issue: "But do we really need authorisation checks here? Or could we just do as Readmore plugin does: NO authorisation check at all ?"
The code I wrote was to show that the current direction of the button code will lead to complex checks (that code, strictly speaking, is not enought) and, as @ggppdk explained well, that would not be enought, there is components/com_content/content.php to consider.
| Status | New | ⇒ | Discussion |
I had some time to make a PR, it is #17854.
Do you think that we should have some checks in component.php? After your arguments in #10653 (comment) I cannot see any reason.
| Status | Discussion | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-09-04 04:53:27 |
| Closed_By | ⇒ | franz-wohlkoenig |
closed as having Pull Request #17854
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17754.
Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/17754
Or in order to fix this
I had said that this ACL check is bogus (and it is still bogus blocking the button from showing in some legitimate cases)
and suggested its complete removal here:
#10653 (comment)
what is this wrong ACL check protecting here ?
it is protecting creating a link to the layout ! LOL
but you can type the link to the layout in your browser too
you can edit HTML of the form and add the button manually
you can inject some custom JS to add the button
The onDisplay method just creates an object
it does not reveal any record data nor it modifies any record data
Then even if it revealed data or modified data, the question is could it be called directly ?
like e.g. a controller task ?
no it can not
with same logic we need to add ACL checks everywhere
NOTE: i do not say that ALL editor XTD buttons do not need ACL check,
it depends on what the button does is going to show
but usually the button is
-- a label
-- and a link
which any way is not directly createable, instead the editor creating code is calling this code
Finally even if you fix this, it will not work
did you see the ACL checks for showing the 'pagebreak' and 'articles' layouts at
components/com_content/content.php
i suggest remove these ACL checks and only fix the ACL checks that control viewing the layouts
at
components/com_content/content.php