Skip to content

Commit

Permalink
[ACL] Don't explicit add the parent asset permissions on creating a n…
Browse files Browse the repository at this point in the history 
…ew item + other issues (#10894)

* try to solve remaining acl problems - initial commit

* some corrections

* code improvements

* ups

* more improvements

* simplify

* better comments

* ups

* Update application.php

* Update rules.php

* more problems solved

* only chekc parent asset if not global config

* call things by it's name

* further simplify code

* further improvements

* revert something and make comment because of new

* add some temporary tooltip debug information on calculated permissions

* improve tooltip debugging information

* even better debug

* name, not title

* minor improvements

* minor debug improvements

* add known issues as to do comments

* remove debug information

* use clear as $query is already instantiated. (thanks roland)

* cs

* cs 1: wrap long lines

* cs 2: multiline comments

* cs 3: one final multiline comment

* minor code ordering and comments
  • Loading branch information
@andrepereiradasilva @wilsonge
andrepereiradasilva authored and wilsonge committed Jun 27, 2016
1 parent 195e573 commit 18875ad
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 61 deletions.
95 changes: 67 additions & 28 deletions administrator/components/com_config/model/application.php
View file
Expand Up @@ -403,6 +403,11 @@ public function storePermissions($permission = null)
return false;
}

$permission['component'] = empty($permission['component']) ? 'root.1' : $permission['component'];

// Current view is global config?
$isGlobalConfig = $permission['component'] === 'root.1';

// Check if changed group has Super User permissions.
$isSuperUserGroupBefore = JAccess::checkGroup($permission['rule'], 'core.admin');

Expand Down Expand Up @@ -493,6 +498,15 @@ public function storePermissions($permission = null)
$parentAssetId = $parentAsset->getRootId();
}

/**
* @to do: incorrect ACL stored
* When changing a permission of an item that doesn't have a row in the asset table the row a new row is created.
* This works fine for item <-> component <-> global config scenario and component <-> global config scenario.
* But doesn't work properly for item <-> section(s) <-> component <-> global config scenario,
* because a wrong parent asset id (the component) is stored.
* Happens when there is no row in the asset table (ex: deleted or not created on update).
*/

$asset->setLocation($parentAssetId, 'last-child');

if (!$asset->check() || !$asset->store())
Expand Down Expand Up @@ -541,7 +555,7 @@ public function storePermissions($permission = null)
// Store the new permissions.
try
{
$query = $this->db->getQuery(true)
$query->clear()
->update($this->db->quoteName('#__assets'))
->set($this->db->quoteName('rules') . ' = ' . $this->db->quote(json_encode($temp)))
->where($this->db->quoteName('name') . ' = ' . $this->db->quote($permission['component']));
Expand All @@ -568,30 +582,54 @@ public function storePermissions($permission = null)
try
{
// Get the asset id by the name of the component.
$query = $this->db->getQuery(true)
->select($this->db->quoteName('id'))
->from($this->db->quoteName('#__assets'))
->where($this->db->quoteName('name') . ' = ' . $this->db->quote($permission['component']));
$query->clear()
->select($this->db->quoteName('id'))
->from($this->db->quoteName('#__assets'))
->where($this->db->quoteName('name') . ' = ' . $this->db->quote($permission['component']));

$this->db->setQuery($query);

$assetId = (int) $this->db->loadResult();

// Get the group parent id of the current group.
$query = $this->db->getQuery(true)
// Fetch the parent asset id.
$parentAssetId = null;

/**
* @to do: incorrect info
* When creating a new item (not saving) it uses the calculated permissions from the component (item <-> component <-> global config).
* But if we have a section too (item <-> section(s) <-> component <-> global config) this is not correct.
* Also, currently it uses the component permission, but should use the calculated permissions for achild of the component/section.
*/

// If not in global config we need the parent_id asset to calculate permissions.
if (!$isGlobalConfig)
{
// In this case we need to get the component rules too.
$query->clear()
->select($this->db->quoteName('parent_id'))
->from($this->db->quoteName('#__usergroups'))
->where($this->db->quoteName('id') . ' = ' . (int) $permission['rule']);
->from($this->db->quoteName('#__assets'))
->where($this->db->quoteName('id') . ' = ' . $assetId);

$this->db->setQuery($query);

$parentAssetId = (int) $this->db->loadResult();
}

// Get the group parent id of the current group.
$query->clear()
->select($this->db->quoteName('parent_id'))
->from($this->db->quoteName('#__usergroups'))
->where($this->db->quoteName('id') . ' = ' . (int) $permission['rule']);

$this->db->setQuery($query);

$parentGroupId = (int) $this->db->loadResult();

// Count the number of child groups of the current group.
$query = $this->db->getQuery(true)
->select('COUNT(' . $this->db->quoteName('id') . ')')
->from($this->db->quoteName('#__usergroups'))
->where($this->db->quoteName('parent_id') . ' = ' . (int) $permission['rule']);
$query->clear()
->select('COUNT(' . $this->db->quoteName('id') . ')')
->from($this->db->quoteName('#__usergroups'))
->where($this->db->quoteName('parent_id') . ' = ' . (int) $permission['rule']);

$this->db->setQuery($query);

Expand All @@ -611,12 +649,12 @@ public function storePermissions($permission = null)
$isSuperUserGroupAfter = JAccess::checkGroup($permission['rule'], 'core.admin');

// Get the rule for just this asset (non-recursive) and get the actual setting for the action for this group.
$assetRule = JAccess::getAssetRules($assetId)->allow($permission['action'], $permission['rule']);
$assetRule = JAccess::getAssetRules($assetId, false, false)->allow($permission['action'], $permission['rule']);

// Get the group, group parent id, and group global config recursive calculated permission for the chosen action.
$inheritedGroupRule = JAccess::checkGroup($permission['rule'], $permission['action'], $assetId);
$inheritedGroupGlobalRule = JAccess::checkGroup($permission['rule'], $permission['action']);
$inheritedParentGroupRule = JAccess::checkGroup($parentGroupId, $permission['action'], $assetId);
$inheritedGroupRule = JAccess::checkGroup($permission['rule'], $permission['action'], $assetId);
$inheritedGroupParentAssetRule = !empty($parentAssetId) ? JAccess::checkGroup($permission['rule'], $permission['action'], $parentAssetId) : null;
$inheritedParentGroupRule = !empty($parentGroupId) ? JAccess::checkGroup($parentGroupId, $permission['action'], $assetId) : null;

// Current group is a Super User group, so calculated setting is "Allowed (Super User)".
if ($isSuperUserGroupAfter)
Expand Down Expand Up @@ -644,6 +682,12 @@ public function storePermissions($permission = null)

// Second part: Overwrite the calculated permissions labels if there is an explicity permission in the current group.

/**
* @to do: incorect info
* If a component as a permission that doesn't exists in global config (ex: frontend editing in com_modules) by default
* we get "Not Allowed (Inherited)" when we should get "Not Allowed (Default)".
*/

// If there is an explicity permission "Not Allowed". Calculated permission is "Not Allowed".
if ($assetRule === false)
{
Expand All @@ -659,23 +703,18 @@ public function storePermissions($permission = null)

// Third part: Overwrite the calculated permissions labels for special cases.

// User in in global config Root (Public)?
$isGlobalConfig = (empty($permission['component']) || $permission['component'] === 'root.1') ? true : false;

// Global configuration with "Not Set" permission. Calculated permission is "Not Allowed (Default)".
if (empty($parentGroupId) && $isGlobalConfig === true && $assetRule === null)
{
$result['class'] = 'label label-important';
$result['text'] = JText::_('JLIB_RULES_NOT_ALLOWED_DEFAULT');
}
// Component/item root level with explicit "Denied" permission at Global configuration. Calculated permission is "Not Allowed (Locked)".
elseif (empty($parentGroupId) && $isGlobalConfig === false && $inheritedParentGroupRule === null && $inheritedGroupGlobalRule === false)
{
$result['class'] = 'label label-important';
$result['text'] = '<span class="icon-lock icon-white"></span>' . JText::_('JLIB_RULES_NOT_ALLOWED_LOCKED');
}
// Some parent group has an explicit "Denied". Calculated permission is "Not Allowed (Locked)".
elseif ($inheritedParentGroupRule === false)
/**
* Component/Item with explicit "Denied" permission at parent Asset (Category, Component or Global config) configuration.
* Or some parent group has an explicit "Denied".
* Calculated permission is "Not Allowed (Locked)".
*/
elseif ($inheritedGroupParentAssetRule === false || $inheritedParentGroupRule === false)
{
$result['class'] = 'label label-important';
$result['text'] = '<span class="icon-lock icon-white"></span>' . JText::_('JLIB_RULES_NOT_ALLOWED_LOCKED');
Expand Down
13 changes: 8 additions & 5 deletions libraries/joomla/access/access.php
View file
Expand Up @@ -560,14 +560,15 @@ protected static function getGroupPath($groupId)
* only the rules explicitly set for the asset or the summation of all inherited rules from
* parent assets and explicit rules.
*
* @param mixed $asset Integer asset id or the name of the asset as a string.
* @param boolean $recursive True to return the rules object with inherited rules.
* @param mixed $asset Integer asset id or the name of the asset as a string.
* @param boolean $recursive True to return the rules object with inherited rules.
* @param boolean $recursiveParentAsset True to calculate the rule also based on inherited component/extension rules.
*
* @return JAccessRules JAccessRules object for the asset.
*
* @since 11.1
*/
public static function getAssetRules($asset, $recursive = false)
public static function getAssetRules($asset, $recursive = false, $recursiveParentAsset = true)
{
// Get instance of the Profiler:
$_PROFILER = JProfiler::getInstance('Application');
Expand All @@ -576,7 +577,8 @@ public static function getAssetRules($asset, $recursive = false)

// Almost all calls should have recursive set to true
// so we'll get to take advantage of preloading:
if ($recursive && isset(self::$assetPermissionsByName[$extensionName]) && isset(self::$assetPermissionsByName[$extensionName][$asset]))
if ($recursive && $recursiveParentAsset && isset(self::$assetPermissionsByName[$extensionName])
&& isset(self::$assetPermissionsByName[$extensionName][$asset]))
{
// Mark in the profiler.
JDEBUG ? $_PROFILER->mark('Start JAccess::getAssetRules New (' . $asset . ')') : null;
Expand Down Expand Up @@ -635,7 +637,7 @@ public static function getAssetRules($asset, $recursive = false)
->from('#__assets AS a');

$extensionString = '';
if ($extensionName !== $asset || is_numeric($asset))
if ($recursiveParentAsset && ($extensionName !== $asset || is_numeric($asset)))
{
$extensionString = ' OR a.name = ' . $db->quote($extensionName);
}
Expand Down Expand Up @@ -681,6 +683,7 @@ public static function getAssetRules($asset, $recursive = false)
$result = $db->loadResult();
$result = array($result);
}

// Instantiate and return the JAccessRules object for the asset rules.
$rules = new JAccessRules;
$rules->mergeCollection($result);
Expand Down
85 changes: 57 additions & 28 deletions libraries/joomla/form/fields/rules.php
View file
Expand Up @@ -156,8 +156,11 @@ protected function getInput()

// Initialise some field attributes.
$section = $this->section;
$component = $this->component;
$assetField = $this->assetField;
$component = empty($this->component) ? 'root.1' : $this->component;

// Current view is global config?
$isGlobalConfig = $component === 'root.1';

// Get the actions for the asset.
$actions = JAccess::getActions($component, $section);
Expand All @@ -172,29 +175,54 @@ protected function getInput()
}
}

// Get the explicit rules for this asset.
if ($section == 'component')
// Get the asset id.
// Note that for global configuration, com_config injects asset_id = 1 into the form.
$assetId = $this->form->getValue($assetField);
$newItem = empty($assetId) && $isGlobalConfig === false && $section !== 'component';
$parentAssetId = null;

// If the asset id is empty (component or new item).
if (empty($assetId))
{
// Need to find the asset id by the name of the component.
// Get the component asset id as fallback.
$db = JFactory::getDbo();
$query = $db->getQuery(true)
->select($db->quoteName('id'))
->from($db->quoteName('#__assets'))
->where($db->quoteName('name') . ' = ' . $db->quote($component));

$db->setQuery($query);

$assetId = (int) $db->loadResult();

/**
* @to do: incorrect info
* When creating a new item (not saving) it uses the calculated permissions from the component (item <-> component <-> global config).
* But if we have a section too (item <-> section(s) <-> component <-> global config) this is not correct.
* Also, currently it uses the component permission, but should use the calculated permissions for achild of the component/section.
*/
}
else

// If not in global config we need the parent_id asset to calculate permissions.
if (!$isGlobalConfig)
{
// Find the asset id of the content.
// Note that for global configuration, com_config injects asset_id = 1 into the form.
$assetId = $this->form->getValue($assetField);
// In this case we need to get the component rules too.
$db = JFactory::getDbo();

$query = $db->getQuery(true)
->select($db->quoteName('parent_id'))
->from($db->quoteName('#__assets'))
->where($db->quoteName('id') . ' = ' . $assetId);

$db->setQuery($query);

$parentAssetId = (int) $db->loadResult();
}

// Full width format.

// Get the rules for just this asset (non-recursive).
$assetRules = JAccess::getAssetRules($assetId);
$assetRules = JAccess::getAssetRules($assetId, false, false);

// Get the available user groups.
$groups = $this->getUserGroups();
Expand Down Expand Up @@ -295,13 +323,13 @@ protected function getInput()
*/

// Get the actual setting for the action for this group.
$assetRule = $assetRules->allow($action->name, $group->value);
$assetRule = $newItem === false ? $assetRules->allow($action->name, $group->value) : null;

// Build the dropdowns for the permissions sliders

// The parent group has "Not Set", all children can rightly "Inherit" from that.
$html[] = '<option value=""' . ($assetRule === null ? ' selected="selected"' : '') . '>'
. JText::_(empty($group->parent_id) && empty($component) ? 'JLIB_RULES_NOT_SET' : 'JLIB_RULES_INHERITED') . '</option>';
. JText::_(empty($group->parent_id) && $isGlobalConfig ? 'JLIB_RULES_NOT_SET' : 'JLIB_RULES_INHERITED') . '</option>';
$html[] = '<option value="1"' . ($assetRule === true ? ' selected="selected"' : '') . '>' . JText::_('JLIB_RULES_ALLOWED')
. '</option>';
$html[] = '<option value="0"' . ($assetRule === false ? ' selected="selected"' : '') . '>' . JText::_('JLIB_RULES_DENIED')
Expand All @@ -318,9 +346,9 @@ protected function getInput()
$result = array();

// Get the group, group parent id, and group global config recursive calculated permission for the chosen action.
$inheritedGroupRule = JAccess::checkGroup((int) $group->value, $action->name, $assetId);
$inheritedGroupGlobalRule = JAccess::checkGroup((int) $group->value, $action->name);
$inheritedParentGroupRule = JAccess::checkGroup((int) $group->parent_id, $action->name, $assetId);
$inheritedGroupRule = JAccess::checkGroup((int) $group->value, $action->name, $assetId);
$inheritedGroupParentAssetRule = !empty($parentAssetId) ? JAccess::checkGroup($group->value, $action->name, $parentAssetId) : null;
$inheritedParentGroupRule = !empty($group->parent_id) ? JAccess::checkGroup($group->parent_id, $action->name, $assetId) : null;

// Current group is a Super User group, so calculated setting is "Allowed (Super User)".
if ($isSuperUserGroup)
Expand All @@ -346,15 +374,21 @@ protected function getInput()
$result['text'] = JText::_('JLIB_RULES_ALLOWED_INHERITED');
}

// Second part: Overwrite the calculated permissions labels if there is an explicity permission in the current group.
// Second part: Overwrite the calculated permissions labels if there is an explicit permission in the current group.

/**
* @to do: incorrect info
* If a component as a permission that doesn't exists in global config (ex: frontend editing in com_modules) by default
* we get "Not Allowed (Inherited)" when we should get "Not Allowed (Default)".
*/

// If there is an explicity permission "Not Allowed". Calculated permission is "Not Allowed".
// If there is an explicit permission "Not Allowed". Calculated permission is "Not Allowed".
if ($assetRule === false)
{
$result['class'] = 'label label-important';
$result['text'] = JText::_('JLIB_RULES_NOT_ALLOWED');
}
// If there is an explicity permission is "Allowed". Calculated permission is "Allowed".
// If there is an explicit permission is "Allowed". Calculated permission is "Allowed".
elseif ($assetRule === true)
{
$result['class'] = 'label label-success';
Expand All @@ -363,23 +397,18 @@ protected function getInput()

// Third part: Overwrite the calculated permissions labels for special cases.

// Changing global config?
$isGlobalConfig = (empty($component) || $component === 'root.1') ? true : false;

// Global configuration with "Not Set" permission. Calculated permission is "Not Allowed (Default)".
if (empty($group->parent_id) && $isGlobalConfig === true && $assetRule === null)
{
$result['class'] = 'label label-important';
$result['text'] = JText::_('JLIB_RULES_NOT_ALLOWED_DEFAULT');
}
// Component/item root level with explicit "Denied" permission at Global configuration. Calculated permission is "Not Allowed (Locked)".
elseif (empty($group->parent_id) && $isGlobalConfig === false && $inheritedParentGroupRule === null && $inheritedGroupGlobalRule === false)
{
$result['class'] = 'label label-important';
$result['text'] = '<span class="icon-lock icon-white"></span>' . JText::_('JLIB_RULES_NOT_ALLOWED_LOCKED');
}
// Some parent group has an explicit "Denied". Calculated permission is "Not Allowed (Locked)".
elseif ($inheritedParentGroupRule === false)
/**
* Component/Item with explicit "Denied" permission at parent Asset (Category, Component or Global config) configuration.
* Or some parent group has an explicit "Denied".
* Calculated permission is "Not Allowed (Locked)".
*/
elseif ($inheritedGroupParentAssetRule === false || $inheritedParentGroupRule === false)
{
$result['class'] = 'label label-important';
$result['text'] = '<span class="icon-lock icon-white"></span>' . JText::_('JLIB_RULES_NOT_ALLOWED_LOCKED');
Expand Down

0 comments on commit 18875ad

Please sign in to comment.