Feat: restrict the read restricted role#142
Conversation
…ma for cohortdefinition
...as this would cause the current code to filter out all cohorts. Current code requires the user to have ALL read permissions listed in the schema to see a cohort definition...
96ee898 to
0b23102
Compare
614a918 to
bde7f91
Compare
28bc22d to
2d66677
Compare
…l need review ...and fixing in ConceptSetPermissionSchema.java
…od and service expected
... as we have now moved the most relevant permissions into role 15
Careful of this one, there was a permission PR in WebAPI where the duplicate sequence was identified for that table, and the one that was specified in the PermissionEntity was deemed the one 'in use' but this PR indicates that the entity's sequence was switched. The reason why the choice was made on the WebAPI side was that we expected that permissions were created under this entity-specified sequence and we didn't want to deal with updating the unused sequence by renaming the one in the Entity. I think this PR took the approach of choosing the one that follows the correct naming conventions. Either choice has merit, but I wanted to call out here the consequence. |
|
@chrisknoll thanks for the heads up. So does this mean that the solution discussed in the ticket OHDSI#2352 is outdated? Can you please add a link to the PR where this was done? I'll close the outdated ticket in this case and will probably merge in your solution. |
|
I had to check to see what the current state is. Looks like master is using sec_permissin_id_seq. From the discussion in the above ticket, we noted that the sec_permission_sequence wasn't used and could be dropped, but in this PR the JPA entity was changed to use the unused sequence sec_permission_sequence. So, if you want to use the sec_permission_sequence as it's done in this PR, you need the following:
The other Issue doesn't appear to have a PR associated with it, so the Issue just raised the concern, but there wasn't any change to address it (I thought there was, that's why I raised the flag above). I probably would have avoided making the change to the JPA sequence in this PR, but I understand that it looked reasonable and the change was made as part of the 'read restricted role' PR. I'm not sure how the changes in this fork are going to propogate over to the WebAPI codebase, but I feel like if we want to fix a sequence, we should make the change up on the WebAPI and propogate that change down to your fork. But, considering the sequence was changed in your fork, I'm unsure what the easiest path is to get the sequence in the upstream to agree with this fork....unless ultimately we will make a PR from your fork into WebAPI...if that's the case, we should probably introduce another migration script to re-set the sec_permission_sequence and then open a PR over on WebAPI to pull in the changes on your fork. |
* feat: remove the * permissions * fix: remove extra item from concat(l,m,r) * tmp: temporarily disable conflicting check * fix: put back the regular vocabulary: permissions * tmp: disable "source user" role assignment * tmp: rename flyway script * fix: ensure source:omop:access becomes part of role 15 * tmp: rename sql migration script * fix: make sure copy permission is part of the default permission schema for cohortdefinition * fix: add cohortdefinition:*:exists:get permission to role 15 * fix: revert copy permission part ...as this would cause the current code to filter out all cohorts. Current code requires the user to have ALL read permissions listed in the schema to see a cohort definition... * fix: add cohortdefinition:*:copy:get permisstion to role 15 * Revert "fix: revert copy permission part" This reverts commit 8c9caf9. * feat: migration script to add copy:get permission to teamproject cohorts * fix: set permissionEntity to use right sequence * fix: fix the migration script / schema name part for setval * feat: migration script to add generate:SOURCE:get permission to role 15 * fix: added extra conceptset permissions to role 15, some of which will need review ...and fixing in ConceptSetPermissionSchema.java * fix: support two authorization rules, where one should match the method and service expected * fix: remove temporary solution for "Source user" ... as we have now moved the most relevant permissions into role 15 * fix: format in CohortDefinitionPermissionSchema.java
Link to JIRA ticket if there is one:
https://ctds-planx.atlassian.net/browse/VADC-1071,
https://ctds-planx.atlassian.net/browse/VADC-1209
Improvements
*permissions).Bug fixes
*permission, and now fixed insrc/main/java/org/ohdsi/webapi/security/model/CohortDefinitionPermissionSchema.javaso a more specific permission is stored. Comes with respective migration script (at the end ofsrc/main/resources/db/migration/postgresql/V2.15.0.20240617204600__custom_ctds_more_restricted_read_restricted_role.sql)sec_permission_id_seqinsrc/main/java/org/ohdsi/webapi/shiro/Entities/PermissionEntity.java. Now usingsec_permission_sequencesrc/main/java/org/ohdsi/webapi/shiro/filters/TeamProjectBasedAuthorizingFilter.java. Now updated to two.