Cleanup of canDelete, canEdit and canEditState in Component Models #13500
Conversation
| $menuTypeId = $this->getMenuTypeId($record->menutype); | ||
| } | ||
|
|
||
| return $user->authorise('core.delete', 'com_menus.menu.' . (int) $menuTypeId); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
|
@wilsonge please can you look at resolving the conflicts here |
wilsonge
changed the title
|
Done :) |
| else | ||
| { | ||
| return parent::canEditState('com_modules'); | ||
| return parent::canEditState($record); |
There was a problem hiding this comment.
Remove the else statement and this line can be on its own.
|
It seems to me that the Try to delete a client from the banners component or any other item in an affected component. It fails with this PR. |
|
@wilsonge can you look at the comments from @Harmageddon please |
|
Fixed :) - nice spot @Harmageddon |
| return $user->authorise('core.delete', 'com_banners'); | ||
| if (!empty($record->catid)) | ||
| { | ||
| return $user->authorise('core.delete', 'com_banners.category.' . (int) $record->catid); |
There was a problem hiding this comment.
Delete line 43 and change to:
return JFactory::getUser()->authorise('core.delete', 'com_banners.category.' . (int) $record->catid);
ghost
added
the
|
I have tested this item ✅ successfully on b702f80 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13500. |
|
I have tested this item ✅ successfully on f29ab48 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13500. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13500. |
|
thx |
10 participants
This cleans up
canDelete,canEditandcanEditStatefunctions so they always return a boolean and are consistent in how they operate (they can returnnullin many cases instead of the documented boolean). There are no actual changes to functionality here so theoretically this is code review.But feel free to test that the authorisation checks on ability to edit, publish/unpublish and delete are unaffected