Skip to content
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

[Statistics] Behavioural Statistics > Double Data Entry - Site permission fix #6659

Closed
wants to merge 3 commits into from
Closed

[Statistics] Behavioural Statistics > Double Data Entry - Site permission fix #6659

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
790f4b0
[Statistics] Fix permission
laemtl Jun 2, 2020
1342585
Clean comments (shortened and reference to created issue)
laemtl Jun 17, 2020
3426070
Dave's review comments
laemtl Jul 15, 2020
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
79 changes: 71 additions & 8 deletions modules/statistics/php/statistics_dd_site.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,47 @@ namespace LORIS\statistics;
*/
class Statistics_DD_Site extends statistics_site
{

var $query_criteria = '';
var $query_vars = array();
/**
* Checking user's permission
*
* @param \User $user The user whose access is being checked
*
* @return bool
*/
function _hasAccess(\User $user) : bool
{
// TODO (#6742): Create a permission specific to statistics
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should not be in the code but rather should be documented in the PR. Same thing for all the other mentions of issues.

$hasAccessToAllProfiles = $user->hasPermission('access_all_profiles');
if ($hasAccessToAllProfiles) {
return true;
}

$hasCenterPermission = false;

// TODO (#6742): There are no means of setting permissions per site
// for a given user right now (see #6742 for more info)

// If a CenterID is passed in the request, check if the user has the
// data_entry permission at the site/center specified by CenterID.
if (!empty($_REQUEST['CenterID'])) {
$hasCenterPermission = $user->hasCenterPermission(
'data_entry',
intval($_REQUEST['CenterID'])
);
} else {
// FIXME (#6742): For the short term the user will be granted access
// if they have permission for a minimum of one of the centers
foreach ($user->getCenterIDs() as $centerID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment from 6 days ago.. not sure why GitHub removed it:

I don't understand this check.. under what situations would the user have the centerID returned from getCenterIDs but not have permission to the centerID?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@racostas can you comment on @driusan 's question ?

Copy link
Contributor Author

@laemtl laemtl Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@driusan @ridz1208 Additional information can be found here: #6742.
Part of the comment has been migrated to an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for bring this back. This code is a bit old now I think. I will take a look but sure there are lot of things could be refactored/fixed.

if ($user->hasCenterPermission('data_entry', intval($centerID))) {
$hasCenterPermission = true;
break;
}
}
}
return $hasAccessToAllProfiles || $hasCenterPermission;
}

/**
* CheckCriteria function
Expand All @@ -39,9 +77,34 @@ class Statistics_DD_Site extends statistics_site
*/
function _checkCriteria($centerID, $projectID)
{
// TODO (#6743): _checkCriteria takes care of restricting data access
// to sites the user belongs to.
// When logic reimplemented on hasCenterPermission(),
// _checkCriteria() will take care of retrieving information
// only for those centers the user has the specific permission.

// TODO (#6743): There are no means of setting permissions per site
// for a given user right now (see #6743 for more info)
if (!empty($centerID)) {
$this->query_criteria .= " AND s.CenterID =:cid ";
$this->query_vars['cid'] = $centerID;
} else {
$list_of_permitted_sites = [];
$currentUser = \NDB_Factory::singleton()->user();
if ($currentUser->hasPermission('access_all_profiles')) {
$list_of_permitted_sites = array_keys(\Utility::getSiteList());
} else {
$list_of_permitted_sites = $currentUser->getCenterIDs();
}
$params = array();
$centerIDs = array();
foreach ($list_of_permitted_sites as $key => $siteID) {
$params[] = ":id$key";
$centerIDs["id$key"] = $siteID;
}
$this->query_criteria .=
" AND s.CenterID IN (" . implode(',', $params) . ")";
$this->query_vars += $centerIDs;
}
if (!empty($projectID)) {
$this->query_criteria .= " AND s.ProjectID =:pid ";
Expand Down Expand Up @@ -105,13 +168,13 @@ class Statistics_DD_Site extends statistics_site
"SELECT count(s.CandID) FROM session s, candidate c,
flag f, $safe_instrument i
WHERE
s.ID=f.SessionID AND f.CommentID=i.CommentID
AND s.CandID=c.CandID
AND s.Active='Y'
s.ID=f.SessionID AND f.CommentID=i.CommentID
AND s.CandID=c.CandID
AND s.Active='Y'
AND s.CenterID <> '1'
AND s.Current_stage <> 'Recycling Bin'
$this->query_criteria
AND f.Data_entry='Complete' AND f.Administration='All'
AND f.Data_entry='Complete' AND f.Administration='All'
AND i.CommentID LIKE 'DDE%' ORDER BY c.PSCID",
$this->query_vars
);
Expand All @@ -132,10 +195,10 @@ class Statistics_DD_Site extends statistics_site
$safe_instrument = $DB->escape($instrument);
$result = $DB->pselect(
"SELECT s.CandID, f.SessionID, i.CommentID, c.PSCID,
lower(s.Visit_label) as Visit_label
lower(s.Visit_label) as Visit_label
FROM session s, candidate c, flag f, $safe_instrument i
WHERE s.ID=f.SessionID AND f.CommentID=i.CommentID AND
s.CandID=c.CandID AND s.Active='Y'
WHERE s.ID=f.SessionID AND f.CommentID=i.CommentID AND
s.CandID=c.CandID AND s.Active='Y'
AND s.CenterID <> '1'
$this->query_criteria
AND s.Current_stage <> 'Recycling Bin'
Expand Down