OHDSI / WebAPI

OHDSI WebAPI contains all OHDSI services that can be called from OHDSI applications
Apache License 2.0
126 stars 156 forks source link

Users can see all objects via direct link despite having no READ permissions #2321

Closed ttemnikova closed 1 week ago

ttemnikova commented 7 months ago

Expected behavior

With the feature https://github.com/OHDSI/WebAPI/issues/2300 users have no access via a direct link to the objects for which they do not have READ or WRITE permissions. The user should get an appropriate error message (e.g. "You have no permissions to this object") instead

Actual behavior

The object is not in the list, but it is accessible via the direct link to the object

Steps to reproduce behavior

  1. user1 creates an object (concept set, cohort definition, characterization, pathways, incidence rates, PLE or PLP)
  2. user1 does not grant access to the object for any other user
  3. user2 try opening the object from the step 1 via a URL to the object
anthonysena commented 7 months ago

From discussion today on the Atlas/WebAPI developer subteam call, this should be addressed by the work done by @rkboyce on #2316 which adds a new Atlas "read only" user role which should prevent this behavior.

chrisknoll commented 7 months ago

In addition, there's a disconnect on how permissions are checked on an entity and how the check is done via URLs:

For an entity check, we have a permission template (ie: cohortdefinition:%s:get) where when we check for a read permission to the asset, we substitute the entity id (yields: cohortdefinition:{id}:get) and we look for a verbatim permission in the user permissions that matches. This means that the user that has the permission cohortdefinition:*:get does not pass this check, even tho, semantically, they have that permission to any ID.

The core of this is that there are two different implementations for checking permissions: one is via the SHIRO framework that tests a URL to a permission, the other is the 'Read/Write' role check that does the above 'template test'. It could have been the case that the SHRIO api was too slow to check a permission (tho I would imagine SHIRO is designed to handle thousands of permission checks per second in a real wourld/heavy load environment). However, it's not clear in the implementation why they did a custom approach for permission checks.

rkboyce commented 7 months ago

As is discussed above, this is not an issue that will affect organizations who want to use the read-restricted role because we have now documented on the wiki (which we will point to in the release note) the use case and preparation required for using the new feature. However, the issue is noted in the wiki with discussion.

anthonysena commented 1 week ago

Closing since this is by design; if you want to provide read permissions, you have to include the explicit get permission as mentioned by @chrisknoll.