Closed rkboyce closed 11 months ago
In response to the last comment about "have we thought about considering 'read access' if you have write" -- yes, it is implemented that way and seems to function correctly based on my tests.
This updated draft pull request implements the READ permissions filtering across all the following applications in Atlas:
All have functioned as expected : things are not accessible to a user unless 1) they create the entity (which sets READ and WRITE), or 2) they are given READ permission to the entity
NOTE: These changes will need to come with a new system role that the admins could select which I am calling the 'Read Restricted Atlas User' role. I think the following query could be the basis for adding SQL to flyway to create this role:
select distinct sp.*
from ohdsi.sec_role_permission srp
inner join ohdsi.sec_permission sp on srp.permission_id = sp.id
where srp.role_id in (6,10) -- 'cohort reader', 'Atlas Users'
and sp.value not in
(
'conceptset:*:get',
'conceptset:*:expression:get',
'conceptset:*:version:*:expression:get',
--
'cohortdefinition:*:get',
'cohortdefinition:*:info:get',
'cohortdefinition:*:version:get',
'cohortdefinition:*:version:*:get',
--
'cohort-characterization:*:get',
'cohort-characterization:*:generation:get',
'cohort-characterization:generation:*:get',
'cohort-characterization:design:get',
'cohort-characterization:*:design:get',
'cohort-characterization:design:*:get',
'cohort-characterization:*:version:get',
'cohort-characterization:*:version:*:get',
--
'pathway-analysis:*:get',
'pathway-analysis:*:generation:get',
'pathway-analysis:generation:*:get',
'pathway-analysis:generation:*:result:get',
'pathway-analysis:generation:*:design:get',
'pathway-analysis:*:version:get',
'pathway-analysis:*:version:*:get'
--
'ir:*:get',
'ir:*:copy:get',
'ir:*:info:get',
'ir:*:design:get',
'ir:*:version:get',
'ir:*:version:*:get'
--
'estimation:*:get',
'estimation:*:copy:get',
'estimation:*:download:get',
'estimation:*:export:get',
'estimation:*:generation:get'
'comparativecohortanalysis:*:get',
--
'prediction:*:get',
'prediction:*:copy:get',
'prediction:*:download:get',
'prediction:*:export:get',
'prediction:*:generation:get',
'prediction:*:exists:get',
'plp:*:get'
)
order by sp.value
;
A detail worth mentioning - were were curious what would happen if a person without read access knew the URL to an entity and tried a GET. I tested as follows:
1) logged in as an admin, created a brand new entity (did it for a few types e.g., cohort definition, estimation, etc)
2) copied the URL
3) logged out from the admin account and logged in to a user account without access to the entity
4) put the URL in the browser
The result is an empty page with no message at all. The server simply returns a 403 with an empty body in the response. Atlas renders nothing. So, I think it is Ok for now.
Yes, that's fine. Ok, if you're done with your part, I will merge this into a new feature branch under WebAPI so I can put in the final changes. Let me know if you are ready for me to do that.
All good here!
The new PRs are ready, if you could pull both the Atlas and WebAPI branches, @rkboyce , and give it a shot in your local env, and report in the other PR threads if it is good or if you found issues. Thanks.
@rkboyce , I noticed when we did the walkthrough and we enabled the filter on the server, the asset list returned nothing until I created a new asset and it got the read perm assigned to it. I wanted to ask: have we thought about considering 'read access' if you have write? This way, if we enable it, anything I've been granted write access will appear, plus anything that someone grants me read access to. Basically the filter becomes
if (canRead || canWrite| then show item
. Is that possible or was there a reason why we had to explicitly look for the granted permission? The only reason why I bring this is that it seems if you can write to something then you can also read it...