User tests: Successful: Unsuccessful:
When logged in as a user with sufficient resources on articles and categories, that user should be able to reorder categories. Note that ACLs are stored under com_content and there is no separate entry for com_articles.
Labels |
Added:
?
?
|
There is no such thing as com_articles
There is no such thing as com_articles
I think he meant com_categories there.
I think he meant com_categories there.
Yes, sorry that (com_categories) is what I meant. I think not many people are testing with limited access, i.e. non superuser users, Since I have a couple more ACL related issues on my list which I would like to share with you. After the Joomla Days in Zeist I decided to make an effort to get a couple of my patches integrated into Joomla, since I become a bit tired of patching all my Joomla instances after every Joomla update.
However, with your information I'll do another check on this and share the results here. Here a bit more detail how I run into this issue:
My setup is a clean up to date 2.5.x Joomla. I have a group "Beheerder" and this group has limited access but is allowed to create com_content (see added png). Next I log in as a user which is a member of Beheerder and I select Category Manger. The error is directly triggered when I click on a blue arrow to reorder a category item.
Something is wrong with JRequest::getCmd('extension'). The returned value is not equal to 'com_content'. Something is dangling in the delivered variable. If you do something like
$ext=JRequest::getCmd('extension');
JError::raiseNotice(200, serialize($ext));
I get:
s:11:"com_content";
s:0:"";
Conclusion:
JRequest::getCmd('extension') does not return a clean 'com_content' string. That is why
if (!JFactory::getUser()->authorise('core.manage', 'com_content')), does fix my problem.
I get s:11:"com_content";
as expected.
Ah sorry I was wrong, I get the same when trying to reorder. Interesting.
Looks like the reorder function doesn't set the extension
and thus JRequest returns an empty string. That's the real issue. Using (!JFactory::getUser()->authorise('core.manage', 'com_content'))
fixes it for com_content, but breaks categories ACL for each other extension. So it's the wrong fix for an existing issue.
Ok, let's fix it then in the right place. The POST issued by the reorder doesn't provide the variable extension in the URL. I am not an expert here and I was thinking that it may have been read from the data provided by POST. In that data extension='com_content' is defined.
Can you confirm that it must be passed in the URL?
I think I found the problem. The reorder function in JControllerAdmin does a redirect at the end, and this redirect doesn't contain the extension
parameter. That's actually correct since this function is also used by other extensions than com_categories where that parameter isn't needed at all.
See https://github.com/joomla/joomla-cms/blob/2.5.x/libraries/joomla/application/component/controlleradmin.php#L242 and https://github.com/joomla/joomla-cms/blob/2.5.x/libraries/joomla/application/component/controlleradmin.php#L249
The easiest solution would like be to override this function in the CategoriesControllerCategories class and generate a proper redirect there.
I reverted the other modifications and added the function. It looks like it works. Can you have a look at it?
Bakual,
Thinking a bit further about your comment not to hardcode com_content, why not shift the extension stuff in the URL up to the JControllerAdmin class iso defining an override?
Can I conclude that in the current case, everything which uses JControllerAdmin::reorder breakes ACL processing, since most of the time authorization is checked using the extension passed?
I'll test this and see if it fixes the same problem when reordering of menu items.
Only com_categories uses the extension
parameter because as said, it's a "service" component which manages the categories for other extensions (like com_content, com_weblinks, ..., 3rd parties). The extension parameter tells com_categories which set of categories it should manage. And of course which ACL to use.
All other extensions using JControllerAdmin don't use that parameter and it would break them if you did change that there. It's a unique thing to com_categories.
Ok, so you say the previous patch is a no go. But if you take a look at https://github.com/gwkant/joomla-cms/blob/2.5.x/libraries/joomla/application/component/controlleradmin.php#L215 , you see that JControllerAdmin::publish also uses extension and if not defined, it is ignored. I copied this behavior into reorder as well. I don't like overriding the reorder function and recode almost 100% of the functionality which is already available in JControllerAdmin::reorder.
Do you think that this way it will still break other extensions?
I wasn't aware that the publish method already does this. Then it of course makes sense to do it similar in the reorder method.
I still think it's a wrong approach, but it's better having consistency
On a sidenote: This bug needs likely fixing in 3.x as well.
I can confirm that indeed this is an issue in 3.x as well.
I still think it's a wrong approach, but it's better having consistency
I tend to agree with you. My reasoning would be that JControllerAdmin does not need to know about extension so it should not be there. Somehow it is the processing at the end leading up to setRedirect which prevents a more elegant solution I would say. I need to think a bit further on this. Do you have some suggestions?
(I was thinking along the line of moving the redirect processing, a kind of common pattern at the end of a lot of methods, to a separate (generic) method. Then a subclass only need to override that redirect method.)
If we follow the override approach, public should be overridden as well to maintain consistency
It should probably be possible to set a new redirect in the child class after the parent method was run. Something along the line
public function reorder()
{
$return = parent::reorder();
if ($parent === false)
{
$this->setRedirect(JRoute::_('index.php?option=' . $this->option . '&view=' . $this->view_list . $extensionURL, false), $message, 'error');
}
else
{
$this->setRedirect(JRoute::_('index.php?option=' . $this->option . '&view=' . $this->view_list . $extensionURL, false), $message);
}
return $return;
}
The problem may be to set the correct message here as well. Or maybe we can redirect and keep the old message? I never tested that
I tested your idea and indeed if message is not provided the corresponding variable in the class instance is left untouched. The same holds for the type ('error').
Status | New | ⇒ | Pending |
Labels |
Removed:
?
|
Category | ⇒ | ACL |
Thanks for working on this. Unfortunately this did not make it into the final release of Joomla 2.5, or it was handled elsewhere, so this is being closed. If you feel this is still a valid issue in Joomla 3 please create a new issue.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-01-01 13:05:45 |
Closed_By | ⇒ | brianteeman |
The current
if (!JFactory::getUser()->authorise('core.manage', JRequest::getCmd('extension'))) {
should actually do exactly what you want.Please note that com_categories is not only used by com_content but any extension using categories. If you look at the URL when managing categories, you see something like this:
administrator/index.php?option=com_categories&extension=com_content
. Note theextensions=com_content
part which tells com_categories for which extension it should manage the categories. The ACL is taken from this extension then.Can you doublecheck that this indeed doesn't work as it should? Otherwise please close this issue.