Skip to content

Commit

Permalink
Fixed issue #18356: [security] User with only user update allowed can…
Browse files Browse the repository at this point in the history 
… set/remove any role to any user (#2625)

* Fix part of #18355: make sure a user can only assign permissions to it's own child users

---------

Co-authored-by: encuestabizdevgit <devgit@encuesta.biz>
Co-authored-by: lapiudevgit <devgit@lapiu.biz>
  • Loading branch information
@gabrieljenik @encuestabizdevgit @lapiudevgit
3 people committed Jun 26, 2023
1 parent 5ff8650 commit a2eece7
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 26 deletions.
64 changes: 38 additions & 26 deletions application/controllers/UserManagementController.php
View file
@@ -1,5 +1,7 @@
<?php

use LimeSurvey\Models\Services\UserManager;

//LSYii_Controller
/**
* Class UserManagementController
Expand Down Expand Up @@ -417,17 +419,18 @@ public function actionViewUser(int $userid): ?string
*/
public function actionUserPermissions()
{
if (!Permission::model()->hasGlobalPermission('users', 'update')) {
$userId = Yii::app()->request->getParam('userid');
$userId = sanitize_int($userId);
$oUser = User::model()->findByPk($userId);

$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canAssignPermissions()) {
return $this->renderPartial(
'partial/error',
['errors' => [gT("You do not have permission to access this page.")], 'noButton' => true]
);
}

$userId = Yii::app()->request->getParam('userid');
$userId = sanitize_int($userId);
$oUser = User::model()->findByPk($userId);

// Check permissions
$aBasePermissions = Permission::model()->getGlobalBasePermissions();
if (!Permission::model()->hasGlobalPermission('superadmin', 'read')) {
Expand Down Expand Up @@ -471,20 +474,25 @@ public function actionUserPermissions()
*/
public function actionSaveUserPermissions(): string
{
if (!Permission::model()->hasGlobalPermission('users', 'update')) {
return $this->renderPartial(
'partial/error',
['errors' => [gT("You do not have permission to access this page.")], 'noButton' => true]
);
}
$userId = Yii::app()->request->getPost('userid');
$oUser = User::model()->findByPk($userId);

$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canAssignPermissions()) {
return Yii::app()->getController()->renderPartial('/admin/super/_renderJson', [
"data" => [
'success' => false,
'errors' => [gT("You do not have permission to access this page.")],
]
]);
}

$aPermissions = Yii::app()->request->getPost('Permission', []);

Permissiontemplates::model()->clearUser($userId);

$results = $this->applyPermissionFromArray($userId, $aPermissions);

$oUser = User::model()->findByPk($userId);
$oUser->modified = date('Y-m-d H:i:s');
$oUser->save();

Expand Down Expand Up @@ -569,19 +577,17 @@ public function actionSaveThemePermissions(): string
*/
public function actionAddRole(): ?string
{
//Permission check user should have permission to add/edit new user ('create' or 'update')
if (
!(Permission::model()->hasGlobalPermission('users', 'create') ||
Permission::model()->hasGlobalPermission('users', 'update'))
) {
$userId = Yii::app()->request->getParam('userid');
$oUser = User::model()->findByPk($userId);

$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canAssignRole()) {
return $this->renderPartial(
'partial/error',
['errors' => [gT("You do not have permission to access this page.")], 'noButton' => true]
);
}

$userId = Yii::app()->request->getParam('userid');
$oUser = User::model()->findByPk($userId);
$aPermissionTemplates = Permissiontemplates::model()->findAll();
$aPossibleRoles = [];
array_walk(
Expand Down Expand Up @@ -612,13 +618,18 @@ function ($oPermissionRole) use (&$aPossibleRoles) {
*/
public function actionSaveRole(): ?string
{
if (!Permission::model()->hasGlobalPermission('users', 'update')) {
return $this->renderPartial(
'partial/error',
['errors' => [gT("You do not have permission to access this page.")], 'noButton' => true]
);
}
$iUserId = Yii::app()->request->getPost('userid');
$oUser = User::model()->findByPk($iUserId);

$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canAssignRole()) {
return Yii::app()->getController()->renderPartial('/admin/super/_renderJson', [
"data" => [
'success' => false,
'errors' => [gT("You do not have permission to access this page.")],
]
]);
}
$aUserRoleIds = Yii::app()->request->getPost('roleselector', []);
$results = [];

Expand Down Expand Up @@ -1212,7 +1223,8 @@ public function updateAdminUser(array $aUser): User

// Abort if logged in user has no access to this user.
// Using same logic as User::getButtons().
if (!$oUser->canEdit(Yii::app()->session['loginID'])) {
$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canEdit()) {
throw new CHttpException(403, gT("You do not have permission to access this page."));
}

Expand Down
2 changes: 2 additions & 0 deletions application/models/User.php
View file
Expand Up @@ -13,6 +13,8 @@
*
*/

use LimeSurvey\Models\Services\UserManager;

/**
* Class User
*
Expand Down
86 changes: 86 additions & 0 deletions application/models/services/UserManager.php
View file
@@ -0,0 +1,86 @@
<?php

namespace LimeSurvey\Models\Services;

use LSWebUser;
use Permission;
use User;

/**
* Service class for managing users and their permissions
*/
class UserManager
{
/** @var LSWebUser the user managing other users */
private $managingUser;

/** @var User the user being handled */
private $targetUser;

/**
* @param LSWebUser $managingUser
* @param User|null $targetUser
*/
public function __construct(
LSWebUser $managingUser,
User $targetUser = null
) {
$this->managingUser = $managingUser;
$this->targetUser = $targetUser;
}

/**
* Returns true if the managing user can assign permissions to the target user.
* @return boolean
*/
public function canAssignPermissions()
{
if (empty($this->managingUser) || empty($this->targetUser)) {
return false;
}

if (
Permission::model()->hasGlobalPermission('superadmin', 'read', $this->managingUser->id)
|| (
Permission::model()->hasGlobalPermission('users', 'update', $this->managingUser->id)
&& $this->targetUser->parent_id == $this->managingUser->id
)
) {
return true;
}

return false;
}

/**
* Returns true if the managing user can assign roles to the target user.
* @return boolean
*/
public function canAssignRole()
{
if (empty($this->managingUser)) {
return false;
}

return Permission::model()->hasGlobalPermission('superadmin', 'read', $this->managingUser->id);
}

/**
* Returns true if the managing user can edit the target user
* @return bool
*/
public function canEdit()
{
if (empty($this->managingUser) || empty($this->targetUser)) {
return false;
}

return
Permission::model()->hasGlobalPermission('superadmin', 'read', $this->managingUser->id)
|| $this->targetUser->uid == $this->managingUser->id
|| (
Permission::model()->hasGlobalPermission('users', 'update', $this->managingUser->id)
&& $this->targetUser->parent_id == $this->managingUser->id
);
}
}
2 changes: 2 additions & 0 deletions application/views/userManagement/partial/addrole.php
View file
Expand Up @@ -15,6 +15,8 @@

<div class="modal-body selector--edit-role-container">
<div class="form">
<div class="row ls-space margin top-5 bottom-5 hidden" id="UserManagement--errors">
</div>
<input type="hidden" name="userid" value="<?= $oUser->uid ?>"/>
<?php
$this->widget('ext.AlertWidget.AlertWidget', [
Expand Down
2 changes: 2 additions & 0 deletions application/views/userManagement/partial/editpermissions.php
View file
Expand Up @@ -7,6 +7,8 @@

<?= TbHtml::formTb(null, App()->createUrl('userManagement/saveUserPermissions'), 'post', ["id" => "UserManagement--modalform"]) ?>
<div class="modal-body overflow-scroll selector--edit-permissions-container">
<div class="row ls-space margin top-5 bottom-5 hidden" id="UserManagement--errors">
</div>
<input type='hidden' name='userid' value='<?php echo (isset($oUser) ? $oUser->uid : ''); ?>' />
<table id='UserManagement--userpermissions-table' class='activecell table table-striped'>
<thead>
Expand Down

0 comments on commit a2eece7

Please sign in to comment.