aces / Loris

LORIS is a web-accessible database solution for longitudinal multi-site studies.
GNU General Public License v3.0
145 stars 173 forks source link

the "UserProjectMatch filter" possibly broke things #6014

Closed maltheism closed 4 years ago

maltheism commented 4 years ago

I believe the PR https://github.com/aces/Loris/pull/5398 possibly broke the logic of handling "genomic_browser_view_site" & "genomic_browser_view_allsites" permissions when accessing the genomic_browser for my PR https://github.com/aces/Loris/pull/5835 (as a regular user with the foregoing permissions).

The genomic_browser module works when just having "genomic_browser_view_allsites" permission without genomic_browser_view_site but breaks when genomic_browser_view_site is added.

I debugged the problem and the error seems to be from the LORIS/src/Data/Filters/UserSiteMatch.php at line 64 an exception is thrown of "resource type that has no sites" but ignores the "genomic_browser_view_allsites" permissions.

To Reproduce Steps to reproduce the behavior (attach screenshots if applicable):

  1. Create a user with permissions "genomic_browser_view_site" & "genomic_browser_view_allsites"
  2. Go to the genomic_browser module with the PR https://github.com/aces/Loris/pull/5835.
  3. See error

@ridz1208 I see you were last one to modify that file.

ridz1208 commented 4 years ago

@maltheism just to be clear.

You created this issue about a change from one PR that affected another PR ? what about the current code ? is it also broken ? is this issue on master ?

maltheism commented 4 years ago

@ridz1208 the current code of the genomic_browser seems to not throw the error because the UserSiteMatch class with the filter function doesn't get called. My PR does have that class get called because it's using the new dataframework (I believe).

edit: Also I checked the permissions in my PR which are returning true for accessing the module.

ridz1208 commented 4 years ago

Unless you can find somewhere on master where this error is thrown you can keep the discussion on the PR i think.

maltheism commented 4 years ago

@ridz1208 I don't necessarily think it's an issue with my PR but instead with the UserSiteMatch class filter function. That's why I think it should be a separate issue. The old genomic_browser module doesn't use the class. The UserSiteMatch class introduces a bug with the permissions of the genomic_browser module.

ridz1208 commented 4 years ago

UserSiteMatch or UserProjectMatch ?

maltheism commented 4 years ago

@ridz1208 UserSiteMatch and in my PR it gets called in this section of the code. https://github.com/aces/Loris/blob/f1164e2b9ef8bddd2e7b3a65c5837cdc59ed8c15/modules/genomic_browser/php/provisioners/cnvprovisioner.class.inc#L106

Edit: now that I'm looking at that code, should access_all_profiles be changed to genomic_browser_view_allsites?

ridz1208 commented 4 years ago

@driusan any thoughts ?

maltheism commented 4 years ago

@ridz1208 see the edit i made in last comment. Edit: If I change access_all_profiles to genomic_browser_view_allsites it fixes the problem. I'm just not sure if that's supposed to be done.

driusan commented 4 years ago

The class linked to doesn't have any of the methods that UserSiteMatch uses to determine whether the site matches. How would the filter work?

maltheism commented 4 years ago

@driusan that makes sense. I'm not sure what's supposed to be supplied in that class with the DataFramework for making just genomic_browser_view_site permission function correctly.

driusan commented 4 years ago

What do the lines above line 64 in UserSiteMatch say?

maltheism commented 4 years ago

@driusan I see that the code is checking if the user has CenterID permission but that seems to override the genomic_browser_view_allsites permission? Such as the case of providing both permissions and the user doesn't have any sites.

maltheism commented 4 years ago

I think I'm suppose to use the HasAnyPermissionOrUserSiteMatch that @xlecours mentioned in another PR.

driusan commented 4 years ago

It doesn't do anything with permissions, it tries various methods on the \Loris\Data\DataInstance to get a list of centerIDs that the data belongs to before throwing the exception about the resource not having sites.