Security Model Modification - Deleting all pages with 'Modify Page Structure'

[greenmile]

I noticed a "problem" with the security model in version 1.2.3 "Black Rock" earlier today. Here's the issue:

I want users belonging to the "Editors" group to be able to edit, re-arrange, and delete pages that they've been assigned to based on "Additional Editors" setting on each page.

So, I granted the "Editors" group the following permissions:

- Add Pages
- Remove Pages
- Modify Page Structure

I didn't want to grant "Modify all Pages" because there are certain pages that only I want to be able to edit or delete.

The problem with the above rights is that by granting 'Modify Page Structure', all users were able to delete any page, even if they weren't assigned access through the "Additional Editors" setting on that particular page. Granted, the CMSMS was smart enough to not display the "Edit" icon on those pages, but the "Delete" icon was still there and functional.

I saw this as dangerous, but yet I didn't want to sacrifice the ability of my editors to reorder pages, so I went ahead and patched the code for my specific purpose. I'm sharing this with you, but note that it's hacky and not thoroughly tested, so back your stuff up first if you want to use it. This will allow you to grant 'Modify Page Structure' permissions while not always granting the ability to delete all pages (they'll only be able to delete pages that they can also edit).

---------------------------------------------------

Core changes for Security. This will allow users with 'Remove Pages' and 'Modify Page Structure' permissions to be able to delete pages that only they have 'Edit' rights to. The original functionality allowed users with 'Modify Page Structure' to delete all pages, even if they were not listed on that item's security.

File: /admin/listcontent.php

Change method "deleteContent" (line 338). Change the start of the function down to the following:

Code: Select all

function deletecontent($contentid)
{
	$userid = get_userid();
	global $gCms;
	$hierManager =& $gCms->GetHierarchyManager();
	$node = &$hierManager->getNodeById($contentid);
		
	$access = ( check_permission($userid, 'Remove Pages') || check_permission($userid, 'Modify Page Structure') 
) || ( check_modify_all($userid) || check_ownership($userid, $node->Id()) || check_authorship($userid, $node->Id()) );
	
	if ($access)
	{
		if ($node)
...

Add the following to line 776 to display an empty table cell if no "edit" icon is displayed:

Code: Select all

else
{
	$thelist .= '<td> </td>' . "\n";
}

change line 780 from :

Code: Select all

if ($root->getChildrenCount() == 0 && (check_permission($userid, 'Modify Page Structure') || check_permission($userid, 'Remove Pages')))

to:

Code: Select all

if ($root->getChildrenCount() == 0 && ($display == 'edit' && check_permission($userid, 'Remove Pages')))

Comment-out lines 949, 950, and 953 so that you get some extra tags displayed and the table format isn't goofed. It should look like:

Code: Select all

//	if (check_modify_all($userid) && check_permission($userid, 'Modify Page Structure'))
//	{
		$headoflist .= "<th class=\"move\">".lang('move')."</th>\n";
		$headoflist .= "<th class=\"pagepos invisible\">".lang('order')."</th>\n";
//	}

File: /admin/multicontent.php

Change the following from like 221 down:

Code: Select all

foreach ($nodelist as $node)
{
	$contentid = $node->Id();
	if( !(check_permission($userid, 'Modify Any Page') || check_ownership($userid, $contentid) || check_authorship($userid, $contentid)) ) 
	{
      		redirect('listcontent.php?error=error_delete_no_access');
    	}	
	if ($node->DefaultContent())
...

File: /admin/lang/en_US/admin.inc.php

Add the following to line 41 (or really anywhere in the file):

Code: Select all

$lang['admin']['error_delete_no_access'] = 'You cannot delete a page unless you have a minimum of edit rights on that page.';

[calguy1000]

if you told us which version of CMS you were modifying

and sent us a 'diff' (looks like you know a bit about programming so you should know about diff)

We would be able to deal with this a little easier.... this sounds like a bug, and since there's a 1.2.4 probably coming soon we may be able to put this in.....

However, the permissions model is currently fugged up in CMS 1.x, there's overlap and some weird things happening, primarily because  the definition of the permissions 'Modify Any Page' for example isn't exactly clear to everybody. So problems like this will probably popup until we manage to get 2.0 out.

[greenmile]

Good call, I forgot to mention it's version 1.2.3 "Black Rock" (I believe that's the latest version).

I use BeyondCompare as a diff tool, but I've never "generated" a diff report for public consumption. How do you normally go about doing that?

[Pierre M.]

man diff

Code: Select all

diff originalfile yourfile > yourdiff.diff

Pierre M.

[greenmile]

Ok, I've attached the diffs. If I could attach more than 4 tiles, I'd go ahead and just attach the full files too. Feel free to contact me for the complete files if you'd like.

[Pierre M.]

Hello,

I don't see any attachment in your post ??
If you have several diff files and want to group them in a zip/tar may be the forum needs you add a ".txt" suffix to the archive's name.

Pierre M.

[greenmile]

Hello,

I don't see any attachment in your post ??
If you have several diff files and want to group them in a zip/tar may be the forum needs you add a ".txt" suffix to the archive's name.

Pierre M.

The attachments are there from what I see. All 3 of the diffs. I can send them to you directly if you're having problems.

[Pierre M.]

Silly me ! I hadn't seen you have put them in your first post

Pierre M.