Fix/flyway atlas read only user #2342
Conversation
|
With the update for WildCard permissions in #2355, we should be able to just use * permissions to grant permission to any source. ie: @rkboyce: would you try to perform a test on your env with the branch from #2355, and possibly will need to make a new source for testing because if this PR was deployed to your WebAPI DB, then you have all the source-specific permissions granted already and you want to test it on a new source (so the * takes effect). |
| from (values | ||
| ('vocabulary:') | ||
| ) t1 (l) | ||
| cross join | ||
| ( select source_key | ||
| from vocab_source | ||
| ) t2 (m) | ||
| cross join |
There was a problem hiding this comment.
| from (values | |
| ('vocabulary:') | |
| ) t1 (l) | |
| cross join | |
| ( select source_key | |
| from vocab_source | |
| ) t2 (m) | |
| cross join | |
| from (values | |
| ('vocabulary:*') | |
| ) t1 (l) | |
| cross join |
feedback from Chris ^ ?
There was a problem hiding this comment.
one issue with this is that permissions like vocabulary:*:search:*:get do not yet exist in the sec_permission table, so these would first have to be added as well (maybe as a first step of this migration script).
There was a problem hiding this comment.
good point, and that would be needed otherwise it won't find any permission IDs to assign to the role.
| 'cohortdefinition:*:check:get', | ||
| 'cohortdefinition:*:check:post', | ||
| 'cohortdefinition:*:copy:get', | ||
| 'cohortdefinition:*:exists:get', | ||
| 'cohortdefinition:*:export:conceptset:get', | ||
| 'cohortdefinition:*:tag:*:delete', | ||
| 'cohortdefinition:*:tag:post', | ||
| 'cohortdefinition:*:version:*:createAsset:put', | ||
| 'cohortdefinition:*:version:*:delete', | ||
| 'cohortdefinition:*:version:*:put', |
There was a problem hiding this comment.
these * permissions should not be part of role 15, as they enable users to access cohortdefinition data that should not be accessible to them. See also uc-cdis#142
There was a problem hiding this comment.
Hi @pieterlukasse ,
I skimmed through the referenced issue you linked, but didn't digest all of it (I did leave a comment tho), but in the context of your concern, the role is supposed to grant global read permission to anyone who has the role, so, in your context, wouldn't hte solution be to NOT assign role 15 to any users who you want to restrict those actions on cohort definitions? I don't think the role is wrong, it's just who you would apply the role permissions to (which in your case, sounds like no one).
There was a problem hiding this comment.
@chrisknoll thanks for reviewing and adding the comment here and in the linked ticket!
Please note that the role 15 is supposed to replace the "Atlas user" role (role 10) and ensure all artifacts are visible only to their creator (see also #2222 and https://github.com/OHDSI/WebAPI/wiki/Read-restricted-Configuration). This means that permissions like cohortdefinition:*:get should not be part of role 15 (as correctly done in #2316), as well as any other permission that enables the user to get access to a cohortdefinition by calling other endpoints (meaning permissions such as cohortdefinition:*:copy:get should also not be part of role 15).
@rkboyce please correct above if anything is wrong!
There was a problem hiding this comment.
Ok, thank you for the clarification. Yes, in the case where users can only see the things they have access to, then you'd have a user role configured that only grants the creation permissions of things (ie: :post permissions) and during creation of an asset, the individual user's role is granted direct read permission to the specific element (id: cohort-definition:{id}:get). They still need to have access to the 'list' functions so those permissions should be assigned, but it sounds like the * permissions should not be part of this role (If I am understanding the context properly).
As an aside, if there was a need for a 'read access role', that role would be the one where you would have wild-card permissions for all the :get permissions. The read access role would grant you read permission to everything.
Finally, the permissions like 'copy' and 'view versions' are something that we should check are assigned when you create the asset (like a cohort definition). This is defined in the PermissionSchema objects, like this example. In this example (CohortDefinitions) it seems to create permissions for put/delete/check endpionts for write, and get/info/versions for read. However, that's not all the things that a reader would do (where's copy?) and I'm wondering if certain permissions were granted at the 'global level' assuming that item-specific permissions would restrict access to the element. This might be the case because permission management is very awkward under the current system (write permission means you are granted these specific permissions to specific endpoints). In the new system, we can introduce permissions like 'read, write, administration, metna, etc' and just declare which functions require which type of permisson on the entity, instead of this many endpoints to one permission map we have to maintain in the current state. But this change is beyond the scope of this PR, but wanted to just bring it up again so that we can remember why we need to change it in the 3.X line.
There was a problem hiding this comment.
Thanks @chrisknoll, I think we are aligned.
one where you would have wild-card permissions for all the :get permissions. The read access role would grant you read permission to everything.
correct.
where's copy?
I've been asking that myself. That is why I fixed it here as well: https://github.com/uc-cdis/WebAPI/pull/142/files#diff-b0ddc2ab44ac01c9f5a2769cbfc9b8e54224dc0ea8a6c03b6113bc89b45487e4R23, uc-cdis@fa2eafe
we need to change it in the 3.X line
I agree. It would be great to simplify this part. As this discussion exemplifies, it is al too easy to get lost in the many permissions, and wrong permissions can slip into the configuration more easily.
This risks being a bit too blunt and some functionality might stop working for some of the removed parts. In these cases, we'll have to add these parts back one by one without a * permission, and test. So far, this has been mainly tested for cohortdefinition and conceptsets. Anyway, I judged this better than having too broad permissions and resulting security incidents.
|
@rkboyce I made 2 PRs above that address the issues for this PR. |
Fix: add missing read permissions to PermissionSchema classes
Fix: remove all * permissions from role 15
|
Though not well documented, this pull request actually tried to address two issues: 1) adding read permissions for the 'atlas read restricted' role to existing sources, and 2) addressing some missing permissions in the WebAPI permissions templates needed for read permissions to work properly.
The action decided for now was to create an issue ticket that enumerates the issues above and references this pull request. We might remove the flyway parts of the pull request and retain the WebAPI permission templates but this will require further review and testing. The issue created is: #2398 |
|
Thanks for the summary @rkboyce , I think that makes sense. An extra point to mention is that the flyway script, as proposed here, does remove all * permissions from role 15, and this potentially breaks some of the functionality for some of the features in the Atlas UI. These * permissions need to be translated into their respective artifact specific versions of the permission first, before they can be fully dropped from role 15. |
This is a fix to the Atlas read-restricted role permissions SQL. The prior SQL permissions select statement did not adequately address multiple schemas for vocabulary and sources. As a result, some the intended read access would not work as expected for some WebAPI configurations. This script does a better job of accounting for all sources and schemas relevant to the user.