?

User tests: Successful: Unsuccessful:

avatar gwkant
gwkant
27 Mar 2014

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.

avatar gwkant gwkant - open - 27 Mar 2014
avatar gwkant gwkant - change - 27 Mar 2014
Labels Added: ? ?
avatar Bakual
Bakual - comment - 27 Mar 2014

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 the extensions=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.

avatar brianteeman
brianteeman - comment - 27 Mar 2014

There is no such thing as com_articles

avatar Bakual
Bakual - comment - 27 Mar 2014

There is no such thing as com_articles

I think he meant com_categories there.

avatar gwkant
gwkant - comment - 27 Mar 2014

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.
acldebugreport

avatar gwkant
gwkant - comment - 27 Mar 2014

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.

avatar Bakual
Bakual - comment - 27 Mar 2014

I get s:11:"com_content"; as expected.

avatar Bakual
Bakual - comment - 27 Mar 2014

Ah sorry I was wrong, I get the same when trying to reorder. Interesting.

avatar Bakual
Bakual - comment - 27 Mar 2014

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.

avatar gwkant
gwkant - comment - 28 Mar 2014

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?

avatar Bakual
Bakual - comment - 28 Mar 2014

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.

avatar gwkant
gwkant - comment - 30 Mar 2014

I reverted the other modifications and added the function. It looks like it works. Can you have a look at it?

avatar gwkant
gwkant - comment - 30 Mar 2014

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.

avatar Bakual
Bakual - comment - 30 Mar 2014

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.

avatar gwkant
gwkant - comment - 30 Mar 2014

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?

avatar Bakual
Bakual - comment - 30 Mar 2014

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 :smile:

avatar Bakual
Bakual - comment - 30 Mar 2014

On a sidenote: This bug needs likely fixing in 3.x as well.

avatar gwkant
gwkant - comment - 30 Mar 2014

I can confirm that indeed this is an issue in 3.x as well.

avatar gwkant
gwkant - comment - 2 Apr 2014

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

avatar Bakual
Bakual - comment - 2 Apr 2014

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 :smile:

avatar gwkant
gwkant - comment - 4 Apr 2014

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').

avatar brianteeman brianteeman - change - 21 Aug 2014
Status New Pending
avatar nicksavov nicksavov - change - 21 Aug 2014
Labels Removed: ?
avatar brianteeman brianteeman - change - 1 Sep 2014
Category ACL
avatar brianteeman
brianteeman - comment - 1 Jan 2015

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.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3377.
avatar brianteeman brianteeman - change - 1 Jan 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-01-01 13:05:45
Closed_By brianteeman
avatar brianteeman brianteeman - close - 1 Jan 2015

Add a Comment

Login with GitHub to post a comment