Skip to content
Permalink
Browse files Browse the repository at this point in the history
fix: [security] Sharing group ACL fixes
- added indirect object reference protection
- added correct ACL functionalities to delete, addOrg, removeOrg

- as reported by Dawid Czarnecki from Zigrin Security
  • Loading branch information
iglocska committed Feb 3, 2022
1 parent 4a7183d commit 15190b9
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 2 deletions.
6 changes: 6 additions & 0 deletions src/Controller/Component/CRUDComponent.php
Expand Up @@ -420,10 +420,16 @@ public function view(int $id, array $params = []): void
}

$data = $this->Table->get($id, $params);
if (empty($data)) {
throw new NotFoundException(__('Invalid {0}.', $this->ObjectAlias));
}
$data = $this->attachMetaData($id, $data);
if (isset($params['afterFind'])) {
$data = $params['afterFind']($data);
}
if (empty($data)) {
throw new NotFoundException(__('Invalid {0}.', $this->ObjectAlias));
}
if ($this->Controller->ParamHandler->isRest()) {
$this->Controller->restResponsePayload = $this->RestResponse->viewData($data, 'json');
}
Expand Down
44 changes: 42 additions & 2 deletions src/Controller/SharingGroupsController.php
Expand Up @@ -7,6 +7,7 @@
use Cake\Utility\Text;
use \Cake\Database\Expression\QueryExpression;
use Cake\Error\Debugger;
use Cake\Http\Exception\NotFoundException;

class SharingGroupsController extends AppController
{
Expand Down Expand Up @@ -54,8 +55,25 @@ public function add()

public function view($id)
{
$currentUser = $this->ACL->getUser();
$this->CRUD->view($id, [
'contain' => ['SharingGroupOrgs', 'Organisations', 'Users' => ['fields' => ['id', 'username']]]
'contain' => ['SharingGroupOrgs', 'Organisations', 'Users' => ['fields' => ['id', 'username']]],
'afterFind' => function($data) use ($currentUser) {
if (empty($currentUser['role']['perm_admin'])) {
$orgFround = false;
if (!empty($data['sharing_group_orgs'])) {
foreach ($data['sharing_group_orgs'] as $org) {
if ($org['id'] === $currentUser['organisation_id']) {
$orgFound = true;
}
}
}
if ($data['organisation_id'] !== $currentUser['organisation_id'] && !$orgFround) {
return null;
}
}
return $data;
}
]);
$responsePayload = $this->CRUD->getResponsePayload();
if (!empty($responsePayload)) {
Expand Down Expand Up @@ -87,7 +105,11 @@ public function edit($id = false)

public function delete($id)
{
$this->CRUD->delete($id);
$currentUser = $this->ACL->getUser();
if (empty($currentUser['role']['perm_admin'])) {
$params['conditions'] = ['organisation_id' => $currentUser['organisation_id']];
}
$this->CRUD->delete($id, $params);
$responsePayload = $this->CRUD->getResponsePayload();
if (!empty($responsePayload)) {
return $responsePayload;
Expand All @@ -97,9 +119,18 @@ public function delete($id)

public function addOrg($id)
{
$currentUser = $this->ACL->getUser();
$sharingGroup = $this->SharingGroups->get($id, [
'contain' => 'SharingGroupOrgs'
]);
if (empty($currentUser['role']['perm_admin'])) {
if ($sharingGroup['organisation_id'] !== $currentUser['organisation_id']) {
$sharingGroup = null;
}
}
if (empty($sharingGroup)) {
throw new NotFoundException(__('Invalid SharingGroup.'));
}
$conditions = [];
$containedOrgIds = array_values(\Cake\Utility\Hash::extract($sharingGroup, 'sharing_group_orgs.{n}.id'));
if (!empty($containedOrgIds)) {
Expand Down Expand Up @@ -156,9 +187,18 @@ public function addOrg($id)

public function removeOrg($id, $org_id)
{
$currentUser = $this->ACL->getUser();
$sharingGroup = $this->SharingGroups->get($id, [
'contain' => 'SharingGroupOrgs'
]);
if (empty($currentUser['role']['perm_admin'])) {
if ($sharingGroup['organisation_id'] !== $currentUser['organisation_id']) {
$sharingGroup = null;
}
}
if (empty($sharingGroup)) {
throw new NotFoundException(__('Invalid SharingGroup.'));
}
if ($this->request->is('post')) {
$org = $this->SharingGroups->SharingGroupOrgs->get($org_id);
$result = (bool)$this->SharingGroups->SharingGroupOrgs->unlink($sharingGroup, [$org]);
Expand Down

0 comments on commit 15190b9

Please sign in to comment.