Menu Manager ACL by rmcdaniel · Pull Request #212 · joomla/joomla-cms

rmcdaniel · 2012-05-21T16:24:32Z

This adds ACL to the menu manager. Most of the code was copied from the content manager. I tried to follow all the conventions in the original code. I hope this helps!

rmcdaniel · 2012-05-21T16:30:38Z

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=28552

realityking · 2012-05-21T16:56:26Z

Haven't tested it yet but the idea is awesome!

Note: We'll have to update the sample data after this has landed.

rmcdaniel · 2012-05-21T20:09:08Z

I forgot something else. In order to install properly, in installation/sql/mysql/joomla.sql this line:

INSERT INTO #__menu_types VALUES (1, 'mainmenu', 'Main Menu', 'The main menu for the site');

should be:

INSERT INTO #__menu_types VALUES (1, 0, 'mainmenu', 'Main Menu', 'The main menu for the site');

How do I add this to the pull request? Or should I make a new one?

realityking · 2012-05-21T20:13:48Z

You can just add another commit to your branch, it will automatically be part of the pull request.

Another note that just occured to me: We probably need a migration script for this. I'm pretty sure something weird will happen if there isn't an entry in the asset table for a menu or menu item.

rmcdaniel · 2012-05-21T20:21:56Z

Thanks for the help.

Regarding the migration script, when I was working on this, I started by just doing ALTER TABLE to add the asset_id column. Everything defaulted to 0 asset_id and nothing blew up. That might be enough.

infograf768 · 2012-05-22T07:57:18Z

Created an update sql file containing
ALTER TABLE #__menu_types ADD COLUMN asset_id INTEGER UNSIGNED NOT NULL DEFAULT '0' COMMENT 'FK to the #__assets table.' AFTER id;
ALTER TABLE #__menu ADD COLUMN asset_id INTEGER UNSIGNED NOT NULL DEFAULT '0' COMMENT 'FK to the #__assets table.' AFTER id;

Ran it and then tested on existing site

saw some errors preventing the tmpl from displaying wen editing a menu item

$canDo = $this->canDo; // error
$canDoAlso = $this->canDoAlso; //error

Also some errors with divs in views/item/tmpl/edit.php

rmcdaniel · 2012-05-22T15:49:10Z

Alright, this latest commit should fix the problems that infograf768 saw.

infograf768 · 2012-05-23T06:28:14Z

The above errors are now corrected.
@test
Default Administrator has no access to menus/menu items permissions.

rmcdaniel · 2012-05-23T19:34:39Z

I fixed the permissions for new installations. For current installations you need to execute:

UPDATE #__assets SET rules = '{"core.admin":{"7":1},"core.manage":{"6":1},"core.create":{"3":1},"core.delete":[],"core.edit":{"4":1},"core.edit.state":{"5":1},"core.edit.own":[]}' WHERE id = 16;

infograf768 · 2012-05-24T05:24:17Z

@ test
This does not work here.

rmcdaniel · 2012-05-24T19:15:11Z

The only thing I can think of is that maybe the group and/or asset ids are different on your dataset.

infograf768 · 2012-05-25T06:54:07Z

I am using default trunk install with sampledata, patched with your diff and both queries.
I suggest you test this environment

sanderpotjer · 2012-05-31T13:54:52Z

@rmcdaniel great idea to add ACL support to menu's! Much requested feature among the users of Joomla ACL.

Just started testing this new feature, and during the installation of fresh site with sampledata errors are visible while installing sampledata. The reason for this is that for example "installation/sql/mysql/sample_data.sq" should be updates as well with asset_id values for the #__menu and #__menu_types. Same for any other sample data file. In addition to this there should be references for all menu items and menus in the #__assets table as well.

infograf768 · 2012-05-31T14:12:25Z

@sanderpotjer
Indeed. This is what I tried to convey above. If we want to implemnt this for 3.x, we will not only have to change sampledatas, but also provide a migration for existing sites.

rmcdaniel · 2012-05-31T14:31:10Z

Thanks for the information. I'll try to get the sample data and the assets done today. As for the migration script, can you give me any ideas on how to start? Is there an example in the installation folder already? Any documentation? Another thing that someone with more experience could look at is how I implemented _getAssetParentId(). I think it might be supposed to return the menutype id, or maybe it should return the parent menu item if that exists?

infograf768 · 2012-05-31T14:39:26Z

We do not have any migration script in present trunk. Just some simple updates to the db in com_admin.

sanderpotjer · 2012-05-31T16:15:22Z

administrator/components/com_menus/access.xml

The core.edit.own action is missing in the component section. So next line after core.edit.state should be core.edit.own

That comes from upstream.

See: https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_menus/access.xml

That's because it doesn't make sense for the component wide access control we have now. However with the item level access you're adding it does make sense.

Each action you add in additional access sections in an access.xml file should be available in the component access section. So the core.edit.own action is available in the menu section, and should be added to the component section as well. Otherwise you can't set an action component wide.

Thanks, I understand now. Thanks for your help. I have added the needed line.

Actually in content for example edit.own only applies at the category adn compoent level, you can't further specify it at the item level for performance and other reasons. You give edit own in a category and the user can edit everything belonging to her in the category.

sanderpotjer · 2012-05-31T18:00:28Z

Actually, the core.edit.own action is not needed in this component at the moment. There is no information stored in the menu tables about the creators of menutypes/menus. So it is better to remove the entire core.edit.own action for this component or we need to extend the menu tables with the creator information.

rmcdaniel · 2012-05-31T18:11:32Z

Does that mean I should remove it from the menu section as well?

sanderpotjer · 2012-05-31T18:11:55Z

libraries/joomla/database/table/menutype.php

At the moment the menu types are stored in the assets table with a wrong parent id and level, to fix this add the following code over here to retrieve the correct information:

// This is a menutype that needs to parent with the extension. if ($assetId === null) { // Build the query to get the asset id of the parent component. $query = $this->_db->getQuery(true); $query->select($this->_db->quoteName('id')); $query->from($this->_db->quoteName('#__assets')); $query->where($this->_db->quoteName('name') . ' = ' . $this->_db->quote('com_menus')); // Get the asset id from the database. $this->_db->setQuery($query); if ($result = $this->_db->loadResult()) { $assetId = (int) $result; } }

sanderpotjer · 2012-05-31T18:15:18Z

@rmcdaniel yes that is correct to remove core.edit.own from the menu section as well. Sorry for the confusing btw, just noticed edit.own isn't possible at the moment, so better to remove it for now.

sanderpotjer · 2012-05-31T18:25:31Z

Another thing I noticed, when installing new components an entry in the #__menu table is created for each of the backend menu items of the component (this is currently default in Joomla). Now with this new menu ACL an entry in the assets table is created as well, but missing the correct parent and level id's. This is more difficult to fix because there is no 'menutype' for the backend menu items.

This might be solved if the backend menu will be transformed in a similar system as the frontend menus. I know there are plans for that, but don't know the status of that. So need to look into this (but want to share it already).

sanderpotjer · 2012-05-31T18:38:33Z

administrator/components/com_menus/views/item/tmpl/edit.php

Add

<div class="clr"></div>

elinw · 2012-06-01T23:29:44Z

In what sense is there ownership in menus?
the edit.own management is very messy to start out with since it violates a number of principles of ACL. There were very good reasons it was decided not to implement it below the category level for anything but articles in 1.6+.

You need to look at com_categories as that is the only core component that has a table extending jtablenested. That should show you how to work with nesting correctly.

sanderpotjer · 2012-06-02T08:53:47Z

administrator/components/com_menus/views/menus/view.html.php

The Rebuild button in the toolbar is now visible for users with access to the component but no edit/create/etc permissions. I suggest to show the button to users with core.admin permission, so it is in line with the com_categories for example. Line 68-72 of this file will become:

if ($canDo->get('core.admin')) { JToolBarHelper::divider(); JToolBarHelper::custom('menus.rebuild', 'refresh.png', 'refresh_f2.png', 'JTOOLBAR_REBUILD', false); JToolBarHelper::preferences('com_menus'); }

Agree, we need to look at all of those buttons if we are letting more
people into that ui.

On Sat, Jun 2, 2012 at 4:53 AM, Sander Potjer <
reply@reply.github.com

wrote:

@@ -51,7 +51,7 @@ protected function addToolbar()
{
require_once JPATH_COMPONENT.'/helpers/menus.php';

$canDo =
MenusHelper::getActions($this->state->get('filter.parent_id'));

$canDo = MenusHelper::getActions();

JToolBarHelper::title(JText::_('COM_MENUS_VIEW_MENUS_TITLE'),
'menumgr.png');

The Rebuild button in the toolbar is now visible for users with access to
the component but no edit/create/etc permissions. I suggest to show the
button to users with core.admin permission, so it is in line with the
com_categories for example. Line 68-72 of this file will become:

if ($canDo->get('core.admin')) { JToolBarHelper::divider(); JToolBarHelper::custom('menus.rebuild', 'refresh.png', 'refresh_f2.png', 'JTOOLBAR_REBUILD', false); JToolBarHelper::preferences('com_menus'); }

Reply to this email directly or view it on GitHub:
https://github.com/joomla/joomla-cms/pull/212/files#r918892

rmcdaniel · 2012-07-11T19:28:09Z

We'd really like to see this in Joomla 3.0 if not sooner. Please advise on what needs to be done in order to keep this alive. I've seen a lot of support for the idea so far.

klah · 2012-10-02T09:35:45Z

Is there any news on whether this will be included in a future Joomla version?

Also (to me) the menu item management option doesn't seem to be working. I set the permissions of a menu item for Manager to Denied for all the actions. But a manager can still happily open, edit and save the item. He can even change his own permissions.

elinw · 2012-10-03T15:53:31Z

1,2,3,4,5,6,7,8,9,10

As always the only "news" that matters is that from people who have tested
and done code review and placed those comments in the feature tracker. So
I am assuming you have done that?

Elin

On Tue, Oct 2, 2012 at 5:35 AM, klah notifications@github.com wrote:

Is there any news on whether this will be included in a future Joomla
version?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/212#issuecomment-9065336.

Let RIPS only run on the main repo

Add ACL to menu manager

2e53bff

Add default asset id for main menu

99421c7

Fix missing objects and misplaced div tag

9d568b7

Richard McDaniel added 2 commits May 22, 2012 10:50

Merge remote-tracking branch 'upstream/master'

5ee4ab5

Remove debug code

4680a08

Fix default permissions for new installations

e2af084

Set better default permissions

c2776d9

Richard McDaniel added 3 commits May 24, 2012 16:59

Fix merge conflict with new access.xml

11774a9

Remove no longer required changes to config.xml

c87d4a0

Fix conflict with new JAccess

fb179bf

sanderpotjer reviewed May 31, 2012
View reviewed changes

Richard McDaniel added 2 commits May 31, 2012 12:16

Merge remote-tracking branch 'upstream/master'

62b1c0f

Add core.edit.own to component

321ca6c

sanderpotjer reviewed May 31, 2012
View reviewed changes

administrator/components/com_menus/views/item/tmpl/edit.php

Copy link
Copy Markdown

Member

sanderpotjer May 31, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add

<div class="clr"></div>

sanderpotjer reviewed Jun 2, 2012
View reviewed changes

Richard McDaniel added 2 commits June 4, 2012 15:13

Merge remote-tracking branch 'upstream/master'

178476f

Add changes suggested by sanderpotjer

6ac9bdf

rmcdaniel closed this Jul 19, 2013

wilsonge pushed a commit that referenced this pull request Jun 4, 2017

Add select box to folder browser item (#212)

88641d2

wilsonge pushed a commit to wilsonge/joomla-cms that referenced this pull request May 26, 2019

Merge pull request joomla#212 from rdeutz/4.0-dev

63d8d4d

Let RIPS only run on the main repo

Uh oh!

Conversation

rmcdaniel commented May 21, 2012

Uh oh!

rmcdaniel commented May 21, 2012

Uh oh!

realityking commented May 21, 2012

Uh oh!

rmcdaniel commented May 21, 2012

Uh oh!

realityking commented May 21, 2012

Uh oh!

rmcdaniel commented May 21, 2012

Uh oh!

infograf768 commented May 22, 2012

Uh oh!

rmcdaniel commented May 22, 2012

Uh oh!

infograf768 commented May 23, 2012

Uh oh!

rmcdaniel commented May 23, 2012

Uh oh!

infograf768 commented May 24, 2012

Uh oh!

rmcdaniel commented May 24, 2012

Uh oh!

infograf768 commented May 25, 2012

Uh oh!

sanderpotjer commented May 31, 2012

Uh oh!

infograf768 commented May 31, 2012

Uh oh!

rmcdaniel commented May 31, 2012

Uh oh!

infograf768 commented May 31, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderpotjer commented May 31, 2012

Uh oh!

rmcdaniel commented May 31, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderpotjer commented May 31, 2012

Uh oh!

sanderpotjer commented May 31, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elinw commented Jun 1, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmcdaniel commented Jul 11, 2012

Uh oh!

klah commented Oct 2, 2012

Uh oh!

elinw commented Oct 3, 2012

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants