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

[h-karim]

Brief summary of changes

The _hasAccess method is being called by Module.class.inc here. This is causing an error because because $this.timepoint is not initialised before the method call in the module class, which leads to calling getCenterID on null:

Loris/modules/electrophysiology_browser/php/sessions.class.inc

Line 51 in a5304c7

|| ($user->hasCenter($this->timepoint->getCenterID())

I added hasStudySite as part of the _hasAccess method to verify the user is affiliated with the study site of the scan session, and removed $user->hasCenter($this->timepoint->getCenterID(), to follow the same logic check as some of the other mdules.

Testing instructions (if applicable)

1. Assign your user "view own site EEG pages" only
2. Go to the EEG front page, click on one of the hyperlinks under the "Links" column
3. Observe session loading successfully

Link(s) to related issue(s)

• Resolves [Electrophysiology_Browser] 500 error on view session when user only has view own site permission #6557

[christinerogers]

Hey @h-karim hasStudySite has a specific meaning and think we may be in the process of removing it. Did you check its history/usage ?
Can you specify -- is it supposed to be used this way? Where else in the codebase is it used this way?

[christinerogers]

also, is the PR title accurate? maybe site instead of sessions?

[h-karim]

is it supposed to be used this way? Where else in the codebase is it used this way?

@christinerogers hasStudySite has been used similarly in the _hasAccess method in the brain browser:

Loris/modules/brainbrowser/php/brainbrowser.class.inc

Lines 34 to 47 in a5304c7

	function _hasAccess(\User $user) : bool
	{
	/* User has access if they have an 'all site' permission or if they are
	* part of a study site and are permitted to view their own site.
	*/
	return $user->hasAnyPermission(
	array(
	'imaging_browser_view_allsites',
	'imaging_browser_phantom_allsites',
	'imaging_browser_phantom_ownsite',
	)
	)
	|| (
	$user->hasStudySite()

and the candidate list module:

Loris/modules/candidate_list/php/validateids.class.inc

Lines 40 to 46 in a5304c7

	* @return bool true iff the user has access to this page.
	*/
	function _hasAccess(\User $user) : bool
	{
	return (
	$user->hasPermission('access_all_profiles')
	|| ($user->hasStudySite() && $user->hasPermission('data_entry'))

[christinerogers]

i think this looks good to me.

@h-karim if you're happy with this solution -- let's keep this moving through the review process.
The next step is the find a way to take this out of draft mode,
I think this means marking this draft PR as Ready for review. If you find you can't do this, (due to no write privilege), ask Rida if he can do this for you or advise on who can.

Then, request testing and review from @laemtl (if you don't mind, Laetitia, merci)

[johnsaigle]

The permissions of this module are confusing to me, both the old and new implementation.

Old way

A user can access this session ONLY if they have the electrophysiology_browser_view_site AND at least one of the following:

• electrophysiology_browser_view_allsites
• $user->hasCenter($this->timepoint->getCenterID())

New way

A user can access this session ONLY if they have the electrophysiology_browser_view_site AND at least one of the following:

• electrophysiology_browser_view_allsites
• access to any 'study site'
• $user->hasCenter($this->timepoint->getCenterID())

---

I don't think either of these is correct. Why would someone need both the allsites and the site permission? allsites should override the other permission.

I think we need permissions like this:

Ideal way

A user can access this session if they have the electrophysiology_browser_view_allsites permission.

Alternatively, a user can access a session if they have electrophysiology_browser_view_site AND a center matching the centerID of this timepoint.

This would be

return $user->hasPermission('electrophysiology_browser_view_allsites')
    || ($user->hasPermission('electrophysiology_browser_view_site')
        && $user->hasCenter($this->timepoint->getCenterID());

I know this doesn't solve the "undefined" issue but it's important that we understand the permissions we're trying to go for. In any case I don't think that a user being able to access any study site has anything to do with whether they are viewing some particualr EEG session.

@cmadjar I think you wrote the module, maybe you can clarify?

[christinerogers]

@johnsaigle - How about spinning your Proposal off into a new issue for discussion?
At a minimum this should be checked for consistency across modules.

[johnsaigle]

I don't consider this a proposal, I think it's an issue with the permissions logic in this code.

[h-karim]

@johnsaigle Considering the brain browser and candidate list modules (among possible others) have a similar line of logic for permissions:

Loris/modules/brainbrowser/php/brainbrowser.class.inc

Lines 34 to 47 in a5304c7

	function _hasAccess(\User $user) : bool
	{
	/* User has access if they have an 'all site' permission or if they are
	* part of a study site and are permitted to view their own site.
	*/
	return $user->hasAnyPermission(
	array(
	'imaging_browser_view_allsites',
	'imaging_browser_phantom_allsites',
	'imaging_browser_phantom_ownsite',
	)
	)
	|| (
	$user->hasStudySite()

Loris/modules/candidate_list/php/validateids.class.inc

Lines 40 to 46 in a5304c7

	* @return bool true iff the user has access to this page.
	*/
	function _hasAccess(\User $user) : bool
	{
	return (
	$user->hasPermission('access_all_profiles')
	|| ($user->hasStudySite() && $user->hasPermission('data_entry'))

Perhaps an umbrella issue/proposal should be created with the eventual goal to centralize all the permissions in one place? The way it's being handled now has the potential to create significant technical debt.

I don't think that a user being able to access any study site has anything to do with whether they are viewing some particualr EEG session.

I think the logic was going on the premises that the module table is displaying only the site data which the user has access to, and that during user creation a user must be assigned a site, therefore the user would be able to access the individual sessions of the table data.

The ideal way you quoted is what is currently implemented : https://github.com/aces/Loris/blob/23.0-release/modules/electrophysiology_browser/php/sessions.class.inc#L50-L54 , it's checking whether the user has the electrophysiology_browser_view_allsites permission or the alternative you proposed, which is why #6557 only comes up when the user only has the electrophysiology_browser_view_site permission.

[johnsaigle]

The ideal way you quoted is what is currently implemented

Yeah I took another look and I think you're right. The logical operators and the brackets are confusing to me in this module. The candidate_list example you quoted is much more clear to me but I think these are logically equivalent.

Sorry for the tangent

[laemtl]

Tested and works fine.

[christinerogers]

Sorry for the tangent

np.

Karim's right -- yes, we are moving towards centralizing / coordinating common definitions of permissions, creating less risk of inconsistencies across modules and technical debt.
So far this has happened via the new data framework -- which is not applying / designed to apply to module subpages.

out of date/i was wrong

[christinerogers]

@AlexandraLivadas or @johnsaigle could either of you re/review this permissions fix? thanks

[ridz1208]

This is the wrong fix for the issue.

fix should look more similar to #6471

As a workaround, hasStudySite is added as part of the _hasAccess method to verify the user is affiliated with the study site of the scan session, short-circuiting $user->hasCenter($this->timepoint->getCenterID().

words like short-circuiting and workaround means that it's probably not the correct fix.

[johnsaigle]

I agree with @ridz1208 that this should probably be done a bit differently. I know Dave commented on a similar issue on Alex was working on about a possible approach using the module's process method. That would be a better way to go then to deliberately include (or retain) broken code.

stale

[christinerogers]

@h-karim can you move forward with a solution to this today? a new PR is fine if that seems best.
(cc rope in @laemtl if you like)

[h-karim]

@ridz1208 Echoing @AlexandraLivadas 's comment overriding the process method inside the Sessions class does not seem to solve the issue:

public function process(
        ServerRequestInterface $request,
        RequestHandlerInterface $handler ) : ResponseInterface {

        $this->timepoint = $request->getAttribute("timePoint");
        if ($this->timepoint === null) {
            // This shouldn't happen, the module level router verifies that
            // the candidate exists before getting here.
            throw new \LorisException("Timepoint list requires a timepoint.");
        }
        return parent::process($request, $handler);
    }

Inserting print functions to test it out, the process method is never only called after the hasAccess method.
Also, if the current PR is the wrong fix, there are some other modules (perhaps create a separate issue for them?) that implement this same approach that should be addressed. For example:

• Brainbrowser
  

  Loris/modules/brainbrowser/php/brainbrowser.class.inc

  Lines 39 to 49 in d6c3fd7

  	return $user->hasAnyPermission(
  	array(
  	'imaging_browser_view_allsites',
  	'imaging_browser_phantom_allsites',
  	'imaging_browser_phantom_ownsite',
  	)
  	)
  	|| (
  	$user->hasStudySite()
  	&& $user->hasPermission('imaging_browser_view_site')
  	);

• imagingbrowser:

  Loris/modules/imaging_browser/php/imaging_browser.class.inc

  Lines 45 to 55 in 938c11c

  	return $user->hasAnyPermission(
  	array(
  	'imaging_browser_view_allsites',
  	'imaging_browser_phantom_allsites',
  	'imaging_browser_phantom_ownsite',
  	)
  	)
  	|| (
  	$user->hasStudySite()
  	&& $user->hasPermission('imaging_browser_view_site')
  	);

• Candidate_list:

  Loris/modules/candidate_list/php/candidate_list.class.inc

  Lines 33 to 39 in 938c11c

  	function _hasAccess(\User $user) : bool
  	{
  	return (
  	$user->hasPermission('access_all_profiles')
  	|| ($user->hasStudySite() && $user->hasPermission('data_entry'))
  	);

[ridz1208]

@driusan can you look into these issues ?

[driusan]

None of those pieces of code are being used to try and short-circuit out of other checks. They're being used to verify that if the user only has the "site" permission for the module, the user has a study site.

[h-karim]

I'll remove $user->hasCenter($this->timepoint->getCenterID()) from the check which will become :

 function _hasAccess(\User $user) : bool
    {
        return ($user->hasPermission('electrophysiology_browser_view_allsites')
                || ($user->hasStudySite()
                    && $user->hasPermission('electrophysiology_browser_view_site') ) );
    }

following the same logic as the other pieces of code.

Edit: Doing this confirmed that the process method is only called after hasAccess.

stale. Rida please re-review

[christinerogers]

For LORIS Magic Monday @h-karim : See Rida's comments for Alex in #6640 and then follow her lead...

[h-karim]

#6640 takes care of the linked issue. Closing this PR.