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

laemtl · 2020-06-02T15:00:45Z

Description of the issue

In Statistics / Behavioural Statistics / Double Data Entry Statistics, users were allowed to see the double data entry breakdown per participant from sites they don't have access to.

Brief summary of changes

The _hasAccess function was incorpoerate to this class (same functionality that the one in #5591)

The function _checkCriteria was modified for only retrieve data from the sites(centers) the user have access to.

Note: In future developments the capacity of setting permissions per site per user could be desirable. A note in rapport was included in the codeset. In this case the proposed function _hasAccess should be updated accordingly .

Testing instructions (if applicable)

A user with permission 'data_entry' should be now able to access the 'breakdown per participant' for the double data entry page only for the sites it have access to.

Links to related PRs

#5950
#5591
Duplicates of #5966 (rebased on 23.0)

laemtl · 2020-06-02T15:17:21Z

@ridz1208 I retested this rebase and it works as expected.

maltheism · 2020-06-08T14:37:27Z

modules/statistics/php/statistics_dd_site.class.inc

        if (!empty($centerID)) {
            $this->query_criteria   .= " AND s.CenterID =:cid ";
            $this->query_vars['cid'] = $centerID;
+        } else {
+            $list_of_permitted_sites = (array) null;


Interesting way to make an empty array. I've never seen this way until today. 🙂

maltheism · 2020-06-08T14:38:52Z

I'm going to manually test. The code looks good!

maltheism · 2020-06-08T15:16:40Z

Hi @laemtl here are my results from what my test user could see visible. The General Demographics Statistics seems to be correct with only showing the two projects the user has access. The other statistics tabs in the module show all the projects for the select field.

User test permissions:

laemtl · 2020-06-08T15:30:08Z

Thank you @maltheism. I will apply the same fix to the other sections as well!

maltheism · 2020-06-08T15:31:41Z

I think this PR will fix #6675 @kongtiaowang

kongtiaowang · 2020-06-08T17:33:16Z

@maltheism This PR will fix Behavioural Statistics. But Imaging Statistics has the same issue.

Only display projects to which a user has access in the projects dropdown in statistics. Resolves #6675 Resolves comments made in #6659

christinerogers · 2020-06-11T18:00:34Z

@laemtl What's the status? Is this ready for @maltheism's re-review? If so, please also ask Karim and/or Alex to help you test and review this today.

laemtl · 2020-06-11T18:05:31Z

@christinerogers All the comments were addressed and merged with #6706

laemtl · 2020-06-11T18:07:51Z

@AlexandraLivadas @h-karim Does any of you have time to test this as well?

spell00

I manually tested and the fix works as expected!

ridz1208 · 2020-07-02T18:17:44Z

@pierre-p-s please review and test

pierre-p-s · 2020-07-03T16:27:41Z

The behavioural statistics tab seems to be working for all sites except for 'Data Coordinating Center' (DCC). When you have the permission to access only DCC data, nothing appears in the behavioural statistics tab (see screenshot below). I am not getting any error in the console or error log. Is this intended?

When clicking on breakdown per participant with only DCC permission this is what I am getting:

ridz1208 · 2020-07-07T18:44:16Z

@pierre-p-s DCC is not included in stats by design

christinerogers

Glad you linked new issues to this.
I highlighted the typos and polished it in the suggestions.
Not sure how i feel about issue #s appearing in the text but can't think of a better way to do it right now.

modules/statistics/php/statistics_dd_site.class.inc

christinerogers · 2020-07-08T20:47:56Z

modules/statistics/php/statistics_dd_site.class.inc

-        // in near versions when the permission framework allow it
-        // The filter _checkCriteria() takes care of restricting
-        // the user access only to the sites it belongs to.
+        // TODO (#6743): _checkCriteria takes care of restricting access


Suggested change

// TODO (#6743): _checkCriteria takes care of restricting access

// TODO (#6743): _checkCriteria takes care of restricting data access

clarifying that it's data access not e.g. page-loading access

modules/statistics/php/statistics_dd_site.class.inc

pierre-p-s

Travis failed for the PR.

I tested, the breakdown per participant, it only outputs data for sites that the user has access to as intended.

I am also linking another issue here that I encountered when testing the behavioural statistics tab: #6573

pierre-p-s · 2020-07-09T20:26:06Z

modules/statistics/php/statistics_dd_site.class.inc

+     */
+    function _hasAccess(\User $user) : bool
+    {
+        // TODO (#6742): Create a permission specific to statistics


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.

laemtl · 2020-07-09T21:29:37Z

@pierre-p-s Restarted the build and it succeeded.

christinerogers

LGTM

driusan · 2020-07-15T17:03:50Z

modules/statistics/php/statistics_dd_site.class.inc

+        $hasAccessToAllProfiles = $user->hasAllPermissions(
+            array(
+                'access_all_profiles',
+                'data_entry',


access_all_profiles should be sufficient. There's no reason to check for both that and data_entry. And you can just immediately return true if the user has it, there's no reason to check for center permissions once you've already determine that they should have access.

driusan · 2020-07-15T17:05:16Z

modules/statistics/php/statistics_dd_site.class.inc

        if (!empty($centerID)) {
            $this->query_criteria   .= " AND s.CenterID =:cid ";
            $this->query_vars['cid'] = $centerID;
+        } else {
+            $list_of_permitted_sites = (array) null;


I agree with @maltheism that this is weird. Why not just set it to []?

driusan · 2020-07-15T17:05:39Z

modules/statistics/php/statistics_dd_site.class.inc

        if (!empty($centerID)) {
            $this->query_criteria   .= " AND s.CenterID =:cid ";
            $this->query_vars['cid'] = $centerID;
+        } else {
+            $list_of_permitted_sites = (array) null;
+            $currentUser = \User::singleton();


This should come from the factory

driusan · 2020-07-15T17:06:30Z

modules/statistics/php/statistics_dd_site.class.inc

+                $list_of_permitted_sites = array_keys(\Utility::getSiteList());
+            } else {
+                foreach ($currentUser->getCenterIDs() as $centerID) {
+                    if ($currentUser->hasCenterPermission(


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?

All the suggestions make sense. I can't comment on the code as I didn't write it. Maybe @racostas can comment.

driusan · 2020-07-21T14:19:06Z

modules/statistics/php/statistics_dd_site.class.inc

+        } 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) {


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?

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

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

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.

racostas · 2020-07-23T20:26:00Z

#6861 refactors the code in this PR. Also includes permission restrictions by project . Please refer to this new version.

racostas · 2020-07-29T19:35:17Z

closed by #6861

…ermission fix (#6861) Code refactorization of the functions _hasAccess and _checkCriteria. Adds per projects permissions restrictions. A user with permission data_entry should be now able to access the 'breakdown per participant' only for the sites and projects it have access to. Resolves #6659

…ermission fix (aces#6861) Code refactorization of the functions _hasAccess and _checkCriteria. Adds per projects permissions restrictions. A user with permission data_entry should be now able to access the 'breakdown per participant' only for the sites and projects it have access to. Resolves aces#6659

laemtl added the Category: Bug PR or issue that aims to report or fix a bug label Jun 2, 2020

laemtl mentioned this pull request Jun 2, 2020

[Statistics / Behavioural Statistics / Double Data Entry Statistics] Fix permission issue where users were allowed to see the double data entry breakdown per participat from sites they don't have access to. #5966

Closed

laemtl added Category: Security PR or issue that aims to improve security Passed manual tests PR has been successfully tested by at least one peer labels Jun 2, 2020

laemtl force-pushed the 2020-06-02-statistics-permission-fix branch from 977bc1a to 689b824 Compare June 2, 2020 15:31

laemtl requested a review from christinerogers June 3, 2020 17:36

maltheism reviewed Jun 8, 2020

View reviewed changes

ridz1208 added this to the 23.0.0 milestone Jun 8, 2020

laemtl self-assigned this Jun 8, 2020

laemtl removed the Passed manual tests PR has been successfully tested by at least one peer label Jun 8, 2020

christinerogers added the 23.0.0-testing label Jun 9, 2020

laemtl changed the title ~~[Statistics] Behavioural Statistics > Double Data Entry permission fix~~ [Statistics] Behavioural Statistics > Double Data Entry Site permission fix Jun 9, 2020

laemtl changed the title ~~[Statistics] Behavioural Statistics > Double Data Entry Site permission fix~~ [Statistics] Behavioural Statistics > Double Data Entry - Site permission fix Jun 9, 2020

laemtl mentioned this pull request Jun 9, 2020

[Statistics] Project Permission #6706

Merged

driusan pushed a commit that referenced this pull request Jun 9, 2020

[Statistics] Project Permission (#6706)

d3c7568

Only display projects to which a user has access in the projects dropdown in statistics. Resolves #6675 Resolves comments made in #6659

ridz1208 removed this from the 23.0.0 milestone Jun 9, 2020

laemtl requested a review from maltheism June 11, 2020 18:04

spell00 self-requested a review June 11, 2020 20:48

spell00 approved these changes Jun 11, 2020

View reviewed changes

laemtl requested a review from driusan June 12, 2020 15:59

christinerogers added the Passed manual tests PR has been successfully tested by at least one peer label Jun 12, 2020

laemtl force-pushed the 2020-06-02-statistics-permission-fix branch 2 times, most recently from 0c6793b to d8fad7f Compare June 17, 2020 16:59

christinerogers requested changes Jul 8, 2020

View reviewed changes

Clean comments (shortened and reference to created issue)

1342585

laemtl force-pushed the 2020-06-02-statistics-permission-fix branch from d8fad7f to 1342585 Compare July 8, 2020 21:04

laemtl requested a review from christinerogers July 8, 2020 21:04

pierre-p-s reviewed Jul 9, 2020

View reviewed changes

laemtl requested a review from pierre-p-s July 9, 2020 21:30

pierre-p-s approved these changes Jul 13, 2020

View reviewed changes

christinerogers approved these changes Jul 13, 2020

View reviewed changes

maltheism approved these changes Jul 14, 2020

View reviewed changes

driusan requested changes Jul 15, 2020

View reviewed changes

laemtl requested a review from driusan July 15, 2020 22:39

Dave's review comments

3426070

laemtl force-pushed the 2020-06-02-statistics-permission-fix branch from 1b616c3 to 3426070 Compare July 15, 2020 23:47

driusan reviewed Jul 21, 2020

View reviewed changes

racostas mentioned this pull request Jul 22, 2020

[Statistics/Behavioural] Detailed View and Double Data Entry - Site permission fix #6861

Merged

racostas closed this Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laemtl commented Jun 2, 2020 •

edited

Loading

laemtl commented Jun 2, 2020 •

edited

Loading

maltheism Jun 8, 2020

maltheism commented Jun 8, 2020

maltheism commented Jun 8, 2020

laemtl commented Jun 8, 2020

maltheism commented Jun 8, 2020

kongtiaowang commented Jun 8, 2020 •

edited

Loading

christinerogers commented Jun 11, 2020

laemtl commented Jun 11, 2020 •

edited

Loading

laemtl commented Jun 11, 2020

spell00 left a comment

ridz1208 commented Jul 2, 2020

pierre-p-s commented Jul 3, 2020

ridz1208 commented Jul 7, 2020

christinerogers left a comment

christinerogers Jul 8, 2020

pierre-p-s left a comment •

edited

Loading

pierre-p-s Jul 9, 2020

laemtl commented Jul 9, 2020 •

edited

Loading

christinerogers left a comment

driusan Jul 15, 2020

driusan Jul 15, 2020

driusan Jul 15, 2020

driusan Jul 15, 2020

laemtl Jul 15, 2020

driusan Jul 21, 2020

ridz1208 Jul 21, 2020

laemtl Jul 21, 2020 •

edited

Loading

racostas Jul 21, 2020

racostas commented Jul 23, 2020

racostas commented Jul 29, 2020

	// TODO (#6743): _checkCriteria takes care of restricting access
	// TODO (#6743): _checkCriteria takes care of restricting data access

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

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

Conversation

laemtl commented Jun 2, 2020 • edited Loading

Description of the issue

Brief summary of changes

Testing instructions (if applicable)

Links to related PRs

laemtl commented Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

maltheism commented Jun 8, 2020

maltheism commented Jun 8, 2020

laemtl commented Jun 8, 2020

maltheism commented Jun 8, 2020

kongtiaowang commented Jun 8, 2020 • edited Loading

christinerogers commented Jun 11, 2020

laemtl commented Jun 11, 2020 • edited Loading

laemtl commented Jun 11, 2020

spell00 left a comment

Choose a reason for hiding this comment

ridz1208 commented Jul 2, 2020

pierre-p-s commented Jul 3, 2020

ridz1208 commented Jul 7, 2020

christinerogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierre-p-s left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl commented Jul 9, 2020 • edited Loading

christinerogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

racostas commented Jul 23, 2020

racostas commented Jul 29, 2020

laemtl commented Jun 2, 2020 •

edited

Loading

laemtl commented Jun 2, 2020 •

edited

Loading

kongtiaowang commented Jun 8, 2020 •

edited

Loading

laemtl commented Jun 11, 2020 •

edited

Loading

pierre-p-s left a comment •

edited

Loading

laemtl commented Jul 9, 2020 •

edited

Loading

laemtl Jul 21, 2020 •

edited

Loading