-
Notifications
You must be signed in to change notification settings - Fork 175
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,49 @@ 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 | ||
$hasAccessToAllProfiles = $user->hasAllPermissions( | ||
array( | ||
'access_all_profiles', | ||
'data_entry', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
) | ||
); | ||
|
||
$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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -39,9 +79,42 @@ 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 = (array) null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting way to make an empty array. I've never seen this way until today. 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @maltheism that this is weird. Why not just set it to |
||
$currentUser = \User::singleton(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should come from the factory |
||
if ($currentUser->hasPermission('access_all_profiles')) { | ||
$list_of_permitted_sites = array_keys(\Utility::getSiteList()); | ||
} else { | ||
foreach ($currentUser->getCenterIDs() as $centerID) { | ||
if ($currentUser->hasCenterPermission( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the suggestions make sense. I can't comment on the code as I didn't write it. Maybe @racostas can comment. |
||
'data_entry', | ||
intval($centerID) | ||
) | ||
) { | ||
array_push($list_of_permitted_sites, $centerID); | ||
} | ||
} | ||
} | ||
$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 "; | ||
|
There was a problem hiding this comment.
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.