[electrophysiology_browser] Session page: Add Project Permissions check #6640

AlexandraLivadas · 2020-05-28T19:55:34Z

Brief summary of changes

When a user tries to access the individual session page for a project they are not affiliated with (by altering the URL), a "permission denied" message should appear.

Testing instructions (if applicable)

Create a user and give them View all-sites Electrophysiology Browser pages permission.
Assign the user project "Pumpernickel" and site "Ottawa"
Navigate to the EEG browser page as that user.
Copy either the "all-types" or "raw" link from one of the rows.
As an admin, change the user to a different project (anything other than Pumpernickel)
Go back to the user account and to the EEG Browser page.
Note that there are no sessions listed (on the test VM)
Paste the copied URL.
There should be a "Permission denied" message and no data should be displayed.

Links to related issues

christinerogers

looks good to me. I will leave you and Karim to each pull and test it.

@ridz1208 or @johnsaigle and eventually dave can review

ridz1208 · 2020-05-29T20:47:32Z

modules/electrophysiology_browser/php/sessions.class.inc

+        $candID    = $this->timepoint->getCandID();
+        $candidate = \Candidate::singleton($candID);
+        if (!$this->_hasAccess($user)
+            || !$user->hasProject($candidate->getProjectID())


@AlexandraLivadas I think it's better to put the project access check inside the hasAccess function

@ridz1208 This wouldn't work because the timepoint variable is not properly defined when the _hasAccess method is first called. This is also what caused the issue #6557. The _hasAccess method is called first by the Module class here, and it is called before the handle method is called where the timepoint variable is properly defined. So, I couldn't include it in the method because it would give a null pointer error. Let me know if this explanation makes sense!

@AlexandraLivadas it makes sense. I think I've seen this use case before

@HenriRabalais isn't that a similar problem to the one you had with issue tracker? do you remember what Dave suggested to add to the class in order to instantiate the object before getting to the has access function ?

https://github.com/aces/Loris/pull/6471/files

this might be helpful. look at the comment of the process function.

I tried this approach and it did not work. I think it is because this class (Sessions) is of type NDB_Page, while the one you linked is of type NDB_Form. If you look at the Module class, the method process is never called for the actual page. So, when I created the process method and defined the timepoint variable in there and then moved the project permission line into the _hasAccess method, it gave me a null pointer error again.

@driusan I sugested that above. @AlexandraLivadas is saying it didnt work

All page types should have the process middleware called on it, there shouldn't be anything specific to forms in that regard. (Forms are special in that they have a _process, but that's different than and unrelated to the PSR15 middleware process)

@driusan I tried again following a very similar format to modules/instrument_list/php/instrument_list.class.inc and it still gives me the following error when I move the project permission to the _hasAccess method:

PHP Fatal error: Uncaught Error: Call to a member function getProjectID() on null in /var/www/loris2020/modules/electrophysiology_browser/php/sessions.class.inc:56\nStack trace:\n#0 /var/www/loris2020/php/libraries/Module.class.inc(351): LORIS\\electrophysiology_browser\\Sessions->_hasAccess(Object(User))\n#1 /var/www/loris2020/src/Middleware/ResponseGenerator.php(50): Module->handle(Object(Laminas\\Diactoros\\ServerRequest))\n#2 /var/www/loris2020/src/Middleware/AuthMiddleware.php(63): LORIS\\Middleware\\ResponseGenerator->process(Object(Laminas\\Diactoros\\ServerRequest), Object(LORIS\\electrophysiology_browser\\Module))\n#3 /var/www/loris2020/src/Router/ModuleRouter.php(75): LORIS\\Middleware\\AuthMiddleware->process(Object(Laminas\\Diactoros\\ServerRequest), Object(LORIS\\electrophysiology_browser\\Module))\n#4 /var/www/loris2020/src/Router/BaseRouter.php(105): LORIS\\Router\\ModuleRouter->handle(Object(Laminas\\Diactoros\\ServerRequest))\n#5 /var/www/loris2020/src/Middleware/ResponseGenerator.php(50): LORIS\\Router\\BaseRouter->handle(Object(Laminas in /var/www/loris2020/modules/electrophysiology_browser/php/sessions.class.inc on line 56, referer: https://alivadas-dev.loris.ca/electrophysiology_browser

I included the if-statements from the instrument_list.class.inc both in the _hasAccess and process methods to check if the timepoint and candidate variables are null. The \NotFound error (from _hasAccess) appeared but the \LorisException error (from process) did not.

[Added] I have found multiple cases of using this process method this way, so I'm not sure why it's not working. I will continue to look into why this is.

@AlexandraLivadas any news from you investigation ?

@h-karim and I have been running into the same problem when trying to implement the process method (check out his PR #6639). After some digging, we both found that the process method is called after the _hasAccess method, so this fix would not work.

h-karim

From a testing POV, it's behaving as intended.

christinerogers · 2020-06-09T22:22:40Z

pinging @ridz1208 for re-review, since @AlexandraLivadas should have addressed your changes already

@driusan - should be ready for final review

Stale. Rida please re-review

AlexandraLivadas · 2020-06-12T20:50:26Z

The changes requested by @ridz1208 and @driusan were attempted but did not work. A similar change was requested for PR #6639 and @h-karim also had trouble implementing it.

incorrect method.

ridz1208 · 2020-06-26T21:55:46Z

@AlexandraLivadas

@driusan and I have discussed this issue at length yesterday.

explanation : the reason @HenriRabalais 's fix on #6471 worked is because he added a null check for $this->issueID in the _hasAccess() function causing it to return true when it is first checked. After which the resources were being instanciated and finally the _hasAccess was called again and only then proper permission checking was done.

We want to change this behaviour because it's not efficient and slightly hacky.

What to do: We decided that a short term stable fix would be the following. In the php/libraries/Module.class.inc, the $page->_hasAccess() function is called on line 351. We are proposing to add a function call on line 350 $page->loadResources($user,$request) this function would be defined in NDB_Page class and does absolutely nothing at that level, but in each module's pages you can override this function and instanciate all the resources you need (timepoint, issue, users,...)

So in this PR what we would expect to see is the addition of the NDB_Page->loadResources($user,$request) function, addition on line 350 of the Module.class.inc class of a call to the new NDB_Page function and, in modules/electrophysiology_browser/php/sessions.class.inc addition of an override for loadResources() function (the content of this function would instanciate the necessary timepoit to allow _hasAccess() to check for permissions)

h-karim

LGTM, passes manual test, and also resolves #6557 .

modules/electrophysiology_browser/php/sessions.class.inc

AlexandraLivadas · 2020-07-09T18:19:33Z

modules/electrophysiology_browser/php/sessions.class.inc

+        }
+        $session_id = intval($matches[1]);
+
+        $this->timepoint = \NDB_Factory::singleton()->timepoint($session_id);


@driusan This section returns a 500 Internal Server Error if the session/timepoint does not exist. Is this okay or should it return some other type of exception?

I think it should be converted to a 404 not found.

modules/electrophysiology_browser/php/sessions.class.inc

driusan

approved pending travis..

…ck (aces#6640) When a user tries to access the individual session page for a project they are not affiliated with (by altering the URL), a "permission denied" message should appear. Resolves aces#6558 Related to issue aces#6557 and associated PR aces#6639

AlexandraLivadas requested a review from christinerogers May 28, 2020 19:55

AlexandraLivadas added 23.0.0-testing Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security labels May 28, 2020

AlexandraLivadas requested a review from h-karim May 29, 2020 15:27

christinerogers previously approved these changes May 29, 2020

View reviewed changes

ridz1208 previously requested changes May 29, 2020

View reviewed changes

ridz1208 assigned AlexandraLivadas May 29, 2020

AlexandraLivadas mentioned this pull request Jun 2, 2020

[imaging_browser] ViewSession subpage: Add Project Permission checks #6638

Merged

christinerogers mentioned this pull request Jun 2, 2020

[DicomArchive] Add project permissions to Subpage #6658

Open

h-karim approved these changes Jun 2, 2020

View reviewed changes

h-karim added the Passed manual tests PR has been successfully tested by at least one peer label Jun 2, 2020

ridz1208 added this to the 23.0.0 milestone Jun 8, 2020

johnsaigle assigned ridz1208 Jun 9, 2020

johnsaigle requested a review from ridz1208 June 9, 2020 14:47

ridz1208 removed this from the 23.0.0 milestone Jun 9, 2020

h-karim mentioned this pull request Jun 11, 2020

[electrophysiology browser] Check if user is affiliated with site when opening individual scan sessions #6639

Closed

ridz1208 added this to the 23.0.1 milestone Jun 12, 2020

ridz1208 added State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed and removed Passed manual tests PR has been successfully tested by at least one peer labels Jun 16, 2020

ridz1208 modified the milestones: 23.0.1, 23.0.2 Jun 17, 2020

AlexandraLivadas requested a review from h-karim June 29, 2020 17:33

h-karim approved these changes Jun 29, 2020

View reviewed changes

AlexandraLivadas requested a review from h-karim June 29, 2020 18:25

AlexandraLivadas force-pushed the 2020-05-28-eegBrowserProjectPermissions branch 3 times, most recently from c302982 to 9ed7db6 Compare July 8, 2020 16:22

AlexandraLivadas added the State: Needs work PR awaiting additional work by the author to proceed label Jul 8, 2020

AlexandraLivadas added 6 commits July 9, 2020 13:27

Added changes

18a3351

Load resources added to NDB_Page

8a4916f

Fixed formatting in hasAccess

2d63296

LoadResources returns void

3a7aa7d

CHanged exception handling

195544c

Removing try-catch

628e247

AlexandraLivadas force-pushed the 2020-05-28-eegBrowserProjectPermissions branch from aea5614 to 628e247 Compare July 9, 2020 17:40

AlexandraLivadas removed the State: Needs work PR awaiting additional work by the author to proceed label Jul 9, 2020

Changed exception to NotFound

e98ffdd

AlexandraLivadas commented Jul 9, 2020

View reviewed changes

modules/electrophysiology_browser/php/sessions.class.inc Show resolved Hide resolved

AlexandraLivadas commented Jul 9, 2020

View reviewed changes

AlexandraLivadas requested review from driusan and h-karim July 9, 2020 18:58

h-karim approved these changes Jul 14, 2020

View reviewed changes

Changed to 404 Not Found

bae5954

AlexandraLivadas requested review from driusan and removed request for driusan July 15, 2020 17:04

driusan reviewed Jul 15, 2020

View reviewed changes

modules/electrophysiology_browser/php/sessions.class.inc Outdated Show resolved Hide resolved

Checks timepoint project ID

bf35747

AlexandraLivadas requested a review from driusan July 15, 2020 18:00

driusan approved these changes Jul 15, 2020

View reviewed changes

driusan merged commit 406d0ad into aces:23.0-release Jul 15, 2020

AlexandraLivadas mentioned this pull request Jul 17, 2020

[EEG Browser] User can access individual sessions for projects they are not involed in #6558

Closed

ridz1208 mentioned this pull request Aug 5, 2020

[battery_manager] Resolved Permission Issues #6858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[electrophysiology_browser] Session page: Add Project Permissions check #6640

[electrophysiology_browser] Session page: Add Project Permissions check #6640

AlexandraLivadas commented May 28, 2020 •

edited

Loading

christinerogers left a comment

ridz1208 May 29, 2020

AlexandraLivadas May 29, 2020

ridz1208 May 30, 2020 •

edited

Loading

ridz1208 May 30, 2020

AlexandraLivadas Jun 1, 2020

ridz1208 Jun 1, 2020

driusan Jun 1, 2020

AlexandraLivadas Jun 1, 2020 •

edited

Loading

ridz1208 Jun 12, 2020

AlexandraLivadas Jun 15, 2020

h-karim left a comment

christinerogers commented Jun 9, 2020

AlexandraLivadas commented Jun 12, 2020

ridz1208 commented Jun 26, 2020

h-karim left a comment

AlexandraLivadas Jul 9, 2020

driusan Jul 15, 2020

driusan left a comment

[electrophysiology_browser] Session page: Add Project Permissions check #6640

[electrophysiology_browser] Session page: Add Project Permissions check #6640

Conversation

AlexandraLivadas commented May 28, 2020 • edited Loading

Brief summary of changes

Testing instructions (if applicable)

Links to related issues

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

ridz1208 May 30, 2020 • edited Loading

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

AlexandraLivadas Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-karim left a comment

Choose a reason for hiding this comment

christinerogers commented Jun 9, 2020

AlexandraLivadas commented Jun 12, 2020

ridz1208 commented Jun 26, 2020

h-karim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

AlexandraLivadas commented May 28, 2020 •

edited

Loading

ridz1208 May 30, 2020 •

edited

Loading

AlexandraLivadas Jun 1, 2020 •

edited

Loading