Feat: restrict the read restricted role

[pieterlukasse]

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

• restrict role 15 (the "read restricted role") to a smaller set of permissions. Compare this to V2.14.0.20231025170057__altas_read_restricted_role_and_permissions.sql or to https://github.com/OHDSI/WebAPI/pull/2342/files
• enable the system to work for users that ONLY have role 15. This means "Atlas user" (role 10) and "Source user (omop)" roles are also not required anymore, which is important, since close review shows that they are too broadly defined as well (lots of * permissions).

Bug fixes

• fixes an issue where users would be able to copy cohortdefinitions for which they would not be expected to have permissions. This happened because of a * permission, and now fixed in src/main/java/org/ohdsi/webapi/security/model/CohortDefinitionPermissionSchema.java so a more specific permission is stored. Comes with respective migration script (at the end of src/main/resources/db/migration/postgresql/V2.15.0.20240617204600__custom_ctds_more_restricted_read_restricted_role.sql)
• fixes incorrect use of sec_permission_id_seq in src/main/java/org/ohdsi/webapi/shiro/Entities/PermissionEntity.java. Now using sec_permission_sequence
• fixes the issue where only one permission is expected from Arborist in src/main/java/org/ohdsi/webapi/shiro/filters/TeamProjectBasedAuthorizingFilter.java. Now updated to two.

[m0nhawk]

👍

[chrisknoll]

fixes incorrect use of sec_permission_id_seq in src/main/java/org/ohdsi/webapi/shiro/Entities/PermissionEntity.java. Now using sec_permission_sequence\

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.

[pieterlukasse]

@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.

[chrisknoll]

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:

1. update the JPA Entity to use the sequence sec_permission_sequence (this was done here)
2. Create a migration script that re-sets the sec_permission_sequence to the max of sec_permission.id and drop the old one
   You provided what that migration would look like in the othe thread:

SELECT setval('${ohdsiSchema}.sec_permission_sequence', (select max(id)+1 from sec_permission), false);
DROP SEQUENCE ${ohdsiSchema}.sec_permission_id_seq;

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.