Skip to content

Cleanup of canDelete, canEdit and canEditState in Component Models #13500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Jul 1, 2019
Merged

Cleanup of canDelete, canEdit and canEditState in Component Models #13500

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ce3a621
Cleanup of canDelete function to always return a bool and be more con…
wilsonge Jan 5, 2017
ebe5379
Cleanup canEdit and canEditState
wilsonge Jan 5, 2017
c5eedbc
Merge branch 'staging' into canDelete
wilsonge Mar 18, 2017
97c1d9a
Merge branch 'staging' into canDelete
wilsonge May 21, 2017
76087c1
Codestyle
wilsonge May 21, 2017
99cc8a4
Merge branch 'staging' into canDelete
rdeutz May 27, 2017
c3bebdf
Fix
wilsonge Aug 3, 2018
7ebd7f4
Fix
wilsonge Aug 3, 2018
6ac70f3
Fix
wilsonge Aug 3, 2018
638a436
Fix
wilsonge Aug 3, 2018
893e81d
Fix
wilsonge Aug 3, 2018
8626190
Fix
wilsonge Aug 3, 2018
31de08e
Fix
wilsonge Aug 3, 2018
1491c37
Fix
wilsonge Aug 3, 2018
b865fbb
Code cleanup. Thanks @Quy
wilsonge Aug 5, 2018
b702f80
Merge branch 'staging' into canDelete
HLeithner Jun 30, 2019
1d872e4
Cleanup code
Jul 1, 2019
fed55e1
Cleanup code
Jul 1, 2019
ff09ef4
Cleanup code
Jul 1, 2019
f29ab48
Merge branch 'staging' into canDelete
infograf768 Jul 1, 2019
350ec12
Remove canEditState override
HLeithner Jul 1, 2019
fa64188
Revert canEditState removal commit
HLeithner Jul 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions administrator/components/com_banners/models/banner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,20 +231,17 @@ protected function batchCopy($value, $pks, $contexts)
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->state != -2)
{
if ($record->state != -2)
{
return false;
}

if (!empty($record->catid))
{
return JFactory::getUser()->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);
}
return false;
}

return parent::canDelete($record);
if (!empty($record->catid))
{
return JFactory::getUser()->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);
}

return parent::canDelete($record);
}

/**
Expand Down
21 changes: 9 additions & 12 deletions administrator/components/com_banners/models/client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,19 @@ class BannersModelClient extends JModelAdmin
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->state != -2)
{
if ($record->state != -2)
{
return false;
}

$user = JFactory::getUser();
return false;
}

if (!empty($record->catid))
{
return $user->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);
}
$user = JFactory::getUser();

return $user->authorise('core.delete', 'com_banners');
if (!empty($record->catid))
{
return $user->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);
Copy link
Contributor

@Quy Quy Aug 5, 2018

Choose a reason for hiding this comment

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

Delete line 43 and change to:
return JFactory::getUser()->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);

Copy link
Contributor Author

@wilsonge wilsonge Aug 5, 2018

Choose a reason for hiding this comment

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

Nice spot!

}

return parent::canDelete($record);
}

/**
Expand Down
11 changes: 4 additions & 7 deletions administrator/components/com_contact/models/contact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,12 @@ protected function batchUser($value, $pks, $contexts)
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->published != -2)
{
if ($record->published != -2)
{
return false;
}

return JFactory::getUser()->authorise('core.delete', 'com_contact.category.' . (int) $record->catid);
return false;
}

return JFactory::getUser()->authorise('core.delete', 'com_contact.category.' . (int) $record->catid);
}

/**
Expand Down
13 changes: 4 additions & 9 deletions administrator/components/com_content/models/article.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,17 +169,12 @@ protected function batchCopy($value, $pks, $contexts)
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->state != -2)
{
if ($record->state != -2)
{
return false;
}

return JFactory::getUser()->authorise('core.delete', 'com_content.article.' . (int) $record->id);
return false;
}

return false;
return JFactory::getUser()->authorise('core.delete', 'com_content.article.' . (int) $record->id);
}

/**
Expand Down Expand Up @@ -208,7 +203,7 @@ protected function canEditState($record)
}

// Default to component settings if neither article nor category known.
return parent::canEditState();
return parent::canEditState($record);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion administrator/components/com_contenthistory/models/history.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ protected function canEdit($record)
*/
protected function canDelete($record)
{
return canEdit($record);
return $this->canEdit($record);
}

/**
Expand Down
15 changes: 5 additions & 10 deletions administrator/components/com_fields/models/field.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,19 +773,14 @@ public function cleanupValues($context, $itemId)
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->state != -2)
{
if ($record->state != -2)
{
return false;
}

$parts = FieldsHelper::extract($record->context);

return JFactory::getUser()->authorise('core.delete', $parts[0] . '.field.' . (int) $record->id);
return false;
}

return false;
$parts = FieldsHelper::extract($record->context);

return JFactory::getUser()->authorise('core.delete', $parts[0] . '.field.' . (int) $record->id);
}

/**
Expand Down
22 changes: 8 additions & 14 deletions administrator/components/com_menus/models/item.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,25 +94,19 @@ protected function canDelete($record)
{
$user = JFactory::getUser();

if (!empty($record->id))
if (empty($record->id) || $record->published != -2)
{
// Only delete trashed items
if ($record->published != -2)
{
return false;
}

$menuTypeId = 0;
return false;
}

if (!empty($record->menutype))
{
$menuTypeId = $this->getMenuTypeId($record->menutype);
}
$menuTypeId = 0;

return $user->authorise('core.delete', 'com_menus.menu.' . (int) $menuTypeId);
if (!empty($record->menutype))
{
$menuTypeId = $this->getMenuTypeId($record->menutype);
}

return false;
return $user->authorise('core.delete', 'com_menus.menu.' . (int) $menuTypeId);
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva Jan 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

what if the (int) $menuTypeId is 0?
This will make a ACL check to com_menus.menu.0 - which is an asset name that doesn't exist.
IIRC, will not really be an issue because ACL checks will make a second ACL check to fallback to component.

But still this is extra ACL processing that IMHO could and should be avoided. The asset name to check when 0 should be the component itself, ie, in this case com_menus.

It seems the same issue applies to several of the ACL checks in this PR. maybe that should be reviewed too.

Anyway this one in particular already existed before this PR.

Copy link
Contributor

@andrepereiradasilva andrepereiradasilva Jan 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Update: In reality, thinking this better, there shouldn't even exist menu items without associated menutype, so ... don't even know why the if (!empty($record->menutype)) check exists here.

Update 2: Maybe the check is for when the menu item is not created yet, if that's the case, and in that case, the ACL check should be to com_menus when the menutype is 0 as commented above

Copy link
Contributor Author

@wilsonge wilsonge Jan 7, 2017

Choose a reason for hiding this comment

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

there should be no items without id's in the record. I think it's just a basic sanity check to try and ensure that we have something that seems record like. At this point if someone's managed to inject dodgy details then we have a massive problem anyhow!

}

/**
Expand Down
4 changes: 1 addition & 3 deletions administrator/components/com_menus/models/menu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ class MenusModelMenu extends JModelForm
*/
protected function canDelete($record)
{
$user = JFactory::getUser();

return $user->authorise('core.delete', 'com_menus.menu.' . (int) $record->id);
return JFactory::getUser()->authorise('core.delete', 'com_menus.menu.' . (int) $record->id);
}

/**
Expand Down
6 changes: 2 additions & 4 deletions administrator/components/com_modules/models/module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,9 @@ protected function canEditState($record)
{
return $user->authorise('core.edit.state', 'com_modules.module.' . (int) $record->id);
}

// Default to component settings if module not known.
else
{
return parent::canEditState('com_modules');
}
return parent::canEditState($record);
}

/**
Expand Down
29 changes: 9 additions & 20 deletions administrator/components/com_newsfeeds/models/newsfeed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,26 +148,17 @@ protected function batchCopy($value, $pks, $contexts)
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->published != -2)
{
if ($record->published != -2)
{
return false;
}

$user = JFactory::getUser();
return false;
}

if (!empty($record->catid))
{
return $user->authorise('core.delete', 'com_newsfeed.category.' . (int) $record->catid);
}
else
{
return parent::canDelete($record);
}
if (!empty($record->catid))
{
return JFactory::getUser()->authorise('core.delete', 'com_newsfeed.category.' . (int) $record->catid);
}

return false;
return parent::canDelete($record);
}

/**
Expand All @@ -187,10 +178,8 @@ protected function canEditState($record)
{
return $user->authorise('core.edit.state', 'com_newsfeeds.category.' . (int) $record->catid);
}
else
{
return parent::canEditState($record);
}

return parent::canEditState($record);
}

/**
Expand Down
21 changes: 1 addition & 20 deletions administrator/components/com_redirect/models/link.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,26 +40,7 @@ protected function canDelete($record)
return false;
}

$user = JFactory::getUser();

return $user->authorise('core.delete', 'com_redirect');
}

/**
* Method to test whether a record can have its state edited.
*
* @param object $record A record object.
*
* @return boolean True if allowed to change the state of the record. Defaults to the permission set in the component.
*
* @since 1.6
*/
protected function canEditState($record)
{
$user = JFactory::getUser();

// Check the component since there are no categories or other assets.
return $user->authorise('core.edit.state', 'com_redirect');
return parent::canDelete($record);
}

/**
Expand Down
23 changes: 3 additions & 20 deletions administrator/components/com_tags/models/tag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,29 +53,12 @@ class TagsModelTag extends JModelAdmin
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->published != -2)
{
if ($record->published != -2)
{
return false;
}

return parent::canDelete($record);
return false;
}
}

/**
* Method to test whether a record can have its state changed.
*
* @param object $record A record object.
*
* @return boolean True if allowed to change the state of the record. Defaults to the permission set in the component.
*
* @since 3.1
*/
protected function canEditState($record)
{
return parent::canEditState($record);
return parent::canDelete($record);
}

/**
Expand Down
17 changes: 5 additions & 12 deletions components/com_config/model/cms.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,17 +256,12 @@ protected function populateState()
*/
protected function canDelete($record)
{
if (!empty($record->id))
if (empty($record->id) || $record->published != -2)
{
if ($record->published != -2)
{
return false;
}

$user = JFactory::getUser();

return $user->authorise('core.delete', $this->option);
return false;
}

return JFactory::getUser()->authorise('core.delete', $this->option);
}

/**
Expand All @@ -280,8 +275,6 @@ protected function canDelete($record)
*/
protected function canEditState($record)
{
$user = JFactory::getUser();

return $user->authorise('core.edit.state', $this->option);
return JFactory::getUser()->authorise('core.edit.state', $this->option);
}
}