Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/flyway atlas read only user #2342

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/flyway atlas read only user #2342

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3e01d1
fix to the Atlas resd-restricted role to add permissions for all appr…
rkboyce Feb 2, 2024
700ee82
file path name fix
rkboyce Feb 2, 2024
939ef57
fix: remove all * permissions from role 15
pieterlukasse Aug 23, 2024
fc7b296
fix: add missing read permissions to PermissionSchema classes
pieterlukasse Aug 23, 2024
80c9780
Merge pull request #8 from pieterlukasse/patch-4
rkboyce Sep 2, 2024
5b91043
Merge pull request #7 from pieterlukasse/patch-3
rkboyce Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
226 changes: 226 additions & 0 deletions ...ion/postgresql/V2.14.1.20240130082100__fix_atlas_read_restricted_role_and_permissions.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,226 @@
delete from ${ohdsiSchema}.sec_role_permission where role_id = 15;

INSERT INTO ${ohdsiSchema}.sec_role_permission (role_id, permission_id)
with vocab_source as (
select source_key
from ${ohdsiSchema}.source s
inner join ${ohdsiSchema}.source_daimon sd on s.source_id = sd.source_id
where sd.daimon_type = 1
), vocab_perms as (
select distinct concat(l,m,r) perm
from (
select *
from (values
('vocabulary:')
) t1 (l)
cross join
( select source_key
from vocab_source
) t2 (m)
cross join
Comment on lines +16 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 ^ ?

Copy link
Contributor

@pieterlukasse pieterlukasse Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, and that would be needed otherwise it won't find any permission IDs to assign to the role.

(values
(':*:get'),
(':compare:post'),
(':concept:*:ancestorAndDescendant:get'),
(':concept:*:get'),
(':concept:*:related:get'),
(':included-concepts:count:post'),
(':lookup:identifiers:ancestors:post'),
(':lookup:identifiers:post'),
(':lookup:mapped:post'),
(':lookup:recommended:post'),
(':lookup:sourcecodes:post'),
(':optimize:post'),
(':resolveConceptSetExpression:post'),
(':search:*:get'),
(':search:post')
) t3 (r)
) combined
)
SELECT DISTINCT 15 role_id, permission_id
FROM ${ohdsiSchema}.sec_role_permission srp
INNER JOIN ${ohdsiSchema}.sec_permission sp ON srp.permission_id = sp.id
WHERE
sp.value IN (select perm from vocab_perms)
or
sp.value IN
(
'*:cohortresults:*:breakdown:get',
'*:person:*:get:dates',
'*:vocabulary:lookup:identifiers:post',
'cdmresults:*:get',
'cohort-characterization:*:download:get',
'cohort-characterization:*:exists:get',
'cohort-characterization:*:export:conceptset:get',
'cohort-characterization:*:export:get',
'cohort-characterization:*:post',
'cohort-characterization:*:tag:*:delete',
'cohort-characterization:*:tag:post',
'cohort-characterization:*:version:*:createAsset:put',
'cohort-characterization:*:version:*:delete',
'cohort-characterization:*:version:*:put',
'cohort-characterization:byTags:post',
'cohort-characterization:check:post',
'cohort-characterization:generation:*:delete',
'cohort-characterization:generation:*:design:get',
'cohort-characterization:generation:*:explore:prevalence:*:*:*:get',
'cohort-characterization:generation:*:result:count:get',
'cohort-characterization:generation:*:result:export:post',
'cohort-characterization:generation:*:result:get',
'cohort-characterization:generation:*:result:post',
'cohort-characterization:get',
'cohort-characterization:import:post',
'cohort-characterization:post',
'cohortanalysis:*:get',
'cohortanalysis:get',
'cohortanalysis:post',
'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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'cohortdefinition:byTags:post',
'cohortdefinition:check:post',
'cohortdefinition:checkv2:post',
'cohortdefinition:get',
'cohortdefinition:post',
'cohortdefinition:printfriendly:cohort:post',
'cohortdefinition:printfriendly:conceptsets:post',
'cohortdefinition:sql:post',
'cohortresults:*:get',
'cohortsample:*:*:*:delete',
'cohortsample:*:*:*:get',
'cohortsample:*:*:*:refresh:post',
'cohortsample:*:*:delete',
'cohortsample:*:*:get',
'cohortsample:*:*:post',
'comparativecohortanalysis:*:copy:get',
'comparativecohortanalysis:*:delete',
'comparativecohortanalysis:*:put',
'comparativecohortanalysis:get',
'comparativecohortanalysis:post',
'conceptset:*:copy-name:get',
'conceptset:*:exists:get',
'conceptset:*:export:get',
'conceptset:*:expression:*:get',
'conceptset:*:generationinfo:get',
'conceptset:*:tag:*:delete',
'conceptset:*:tag:post',
'conceptset:*:version:*:createAsset:put',
'conceptset:*:version:*:delete',
'conceptset:*:version:*:expression:*:get',
'conceptset:*:version:*:get',
'conceptset:*:version:*:put',
'conceptset:*:version:get',
'conceptset:byTags:post',
'conceptset:check:post',
'conceptset:get',
'conceptset:post',
'configuration:edit:ui',
'estimation:*:exists:get',
'estimation:*:generation:*:post',
'estimation:check:post',
'estimation:generation:*:result:get',
'estimation:get',
'estimation:import:post',
'estimation:post',
'evidence:*:drugconditionpairs:post',
'evidence:*:druglabel:post',
'evidence:*:get',
'evidence:*:negativecontrols:*:get',
'evidence:*:negativecontrols:post',
'executionservice:*:get',
'executionservice:execution:run:post',
'feasibility:*:delete',
'feasibility:*:get',
'feasibility:*:put',
'feasibility:get',
'feature-analysis:*:copy:get',
'feature-analysis:*:exists:get',
'feature-analysis:*:export:conceptset:get',
'feature-analysis:*:get',
'feature-analysis:aggregates:get',
'feature-analysis:get',
'feature-analysis:post',
'featureextraction:*:get',
'gis:cohort:*:bounds:*:get',
'gis:cohort:*:clusters:*:get',
'gis:cohort:*:density:*:get',
'gis:person:*:bounds:*:get',
'gis:source:check:*:get',
'ir:*:exists:get',
'ir:*:tag:*:delete',
'ir:*:tag:post',
'ir:*:version:*:createAsset:put',
'ir:*:version:*:delete',
'ir:*:version:*:put',
'ir:byTags:post',
'ir:check:post',
'ir:design:post',
'ir:get',
'ir:post',
'ir:sql:post',
'job:execution:get',
'job:get',
'job:type:*:name:*:get',
'notifications:get',
'notifications:viewed:get',
'notifications:viewed:post',
'pathway-analysis:*:exists:get',
'pathway-analysis:*:export:get',
'pathway-analysis:*:post',
'pathway-analysis:*:tag:*:delete',
'pathway-analysis:*:tag:post',
'pathway-analysis:*:version:*:createAsset:put',
'pathway-analysis:*:version:*:delete',
'pathway-analysis:*:version:*:put',
'pathway-analysis:byTags:post',
'pathway-analysis:check:post',
'pathway-analysis:get',
'pathway-analysis:import:post',
'pathway-analysis:post',
'plp:*:copy:get',
'plp:*:delete',
'plp:*:put',
'plp:get',
'plp:post',
'prediction:*:generation:*:post',
'prediction:check:post',
'prediction:generation:*:result:get',
'prediction:get',
'prediction:import:post',
'prediction:post',
'reusable:*:exists:get',
'reusable:*:get',
'reusable:*:post',
'reusable:*:tag:*:delete',
'reusable:*:tag:post',
'reusable:*:version:*:createAsset:put',
'reusable:*:version:*:delete',
'reusable:*:version:*:get',
'reusable:*:version:*:put',
'reusable:*:version:get',
'reusable:byTags:post',
'reusable:get',
'reusable:post',
'source:*:get',
'source:daimon:priority:get',
'source:priorityVocabulary:get',
'sqlrender:translate:post',
'tag:*:delete',
'tag:*:get',
'tag:*:put',
'tag:get',
'tag:multiAssign:post',
'tag:multiUnassign:post',
'tag:post',
'tag:search:get',
'vocabulary:*:compare-arbitrary',
'vocabulary:*:post'
)
;
Loading