Filter cohorts and concepts by permission #2245

rkboyce · 2023-04-04T13:31:50Z

No description provided.

…a user only sees what their role has permission to read. This initial commit has a big issue in that a person who authors a conceptset cannot see the concept set unless a new permission is added. To be fixed.

…i-ohdsi/WebAPI into HEAD

…AD access

…i-ohdsi/WebAPI into filter_cohorts_and_concepts

chrisknoll · 2023-04-20T17:31:44Z

src/main/java/org/ohdsi/webapi/security/PermissionController.java

-    @Path("/access/{entityType}/{entityId}")
+    @Path("/access/{entityType}/{entityId}/{role}")
    @Consumes(MediaType.APPLICATION_JSON)
    @Produces(MediaType.APPLICATION_JSON)
    public List<RoleDTO> listAccessesForEntity(
            @PathParam("entityType") EntityType entityType,
-            @PathParam("entityId") Integer entityId
+            @PathParam("entityId") Integer entityId,
+	    @PathParam("role") String role
    ) throws Exception {


This method has changed in a backwards breaking way, I think we should leave the existing method alone and add a new one that takes the /role as a param. It looks like the default action is write, so we could include a new route that takes role, and call a common method that takes role as params:

listAccessForEntity(...) { listAccessForEntityRole(..., AccessType.Write) } listAccessForEntityRole(...) { /// this method takes the implemnetation for listAccessesForEntity but now handles the role type }

Also, the param type for role param could be a AccessType enum, but would have to experiment if the /{role} as /WRITE would map properly to the enum.

Also, there's some overloading of terms 'role' where in this context, it is 'READ', 'WRITE' for 'role' but in other places we have 'Admin' as a role and 'Atlas User' as a role...but the latter roles maps to a shiro 'group' (which you assign permissions to'....we could probably interchange groups and roles, however, the AccessType is neither a group or a role...so...I don't have a solution here, just pointing out the differences in context.

It's almost like we're talking about a 'permission type' .. permission to read, permission to write.

I addressed this the overloading of the 'role' string by using 'permType' as the parameter. The function name is now listAccessesForEntityByPermType used for the /access/{entityType}/{entityId}/{permType} endpoint with WRITE as the default permission type sent from the client. This distinguishes it from listAccessesForEntity which is used for the /access/suggest endpoint. I did not get a chance to test if the AccessType enum would work instead of string "READ" and "WRITE".

src/main/java/org/ohdsi/webapi/service/ConceptSetService.java

chrisknoll · 2023-05-18T13:14:41Z

@rkboyce , I noticed when we did the walkthrough and we enabled the filter on the server, the asset list returned nothing until I created a new asset and it got the read perm assigned to it. I wanted to ask: have we thought about considering 'read access' if you have write? This way, if we enable it, anything I've been granted write access will appear, plus anything that someone grants me read access to. Basically the filter becomes if (canRead || canWrite| then show item. Is that possible or was there a reason why we had to explicitly look for the granted permission? The only reason why I bring this is that it seems if you can write to something then you can also read it...

merging upstream

…n used to tell the WebAPI to do filtering based on READ permissions. The new property is called security.defaultGlobalReadPermissions

… on getting the cohortdefinition filtering to work. Still need to fix the case where another user grants read permission to a single cohort definition to another user and then that definition is visible. Currently, somehow, all definitions authored by the granting user are showing in the grantee's Atlas. Once worked out, the task will be to repeat the steps for every one of the following files: src/main/java/org/ohdsi/webapi/security/model/CohortCharacterizationPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/CohortDefinitionPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/ConceptSetPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/EstimationPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/FeatureAnalysisPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/IncidenceRatePermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/PathwayAnalysisPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/PredictionPermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/ReusablePermissionSchema.java src/main/java/org/ohdsi/webapi/security/model/TagPermissionSchema.java

… apply the concept filtering to other artifact types

…efinitions. Tests OK

… permissions

…issions

…D permissions - tested and not working completely. Strange permissions issue that appears to be entirely client-side.

…timation but that were not obvious previously

…' string to a more clear 'permType

rkboyce · 2023-07-23T02:26:06Z

In response to the last comment about "have we thought about considering 'read access' if you have write" -- yes, it is implemented that way and seems to function correctly based on my tests.

This updated draft pull request implements the READ permissions filtering across all the following applications in Atlas:

concept sets
Cohort definition
Cohort characterization
Incident Rates
Estimation
Prediction

All have functioned as expected : things are not accessible to a user unless 1) they create the entity (which sets READ and WRITE), or 2) they are given READ permission to the entity

NOTE: These changes will need to come with a new system role that the admins could select which I am calling the 'Read Restricted Atlas User' role. I think the following query could be the basis for adding SQL to flyway to create this role:

select distinct sp.*
from ohdsi.sec_role_permission srp 
  inner join ohdsi.sec_permission sp on srp.permission_id = sp.id 
where srp.role_id in (6,10) -- 'cohort reader', 'Atlas Users'
 and sp.value not in 
    (  
    	'conceptset:*:get',
    	'conceptset:*:expression:get',
    	'conceptset:*:version:*:expression:get',    	       
    	--
        'cohortdefinition:*:get',
        'cohortdefinition:*:info:get',
        'cohortdefinition:*:version:get',
        'cohortdefinition:*:version:*:get',        
        --        
	        'cohort-characterization:*:get',
                'cohort-characterization:*:generation:get',
		'cohort-characterization:generation:*:get',
		'cohort-characterization:design:get',
		'cohort-characterization:*:design:get',
		'cohort-characterization:design:*:get',
		'cohort-characterization:*:version:get',
		'cohort-characterization:*:version:*:get',
		--
		'pathway-analysis:*:get',
		'pathway-analysis:*:generation:get',
		'pathway-analysis:generation:*:get',
		'pathway-analysis:generation:*:result:get',
		'pathway-analysis:generation:*:design:get',
		'pathway-analysis:*:version:get',
		'pathway-analysis:*:version:*:get'
		--
		'ir:*:get',
		'ir:*:copy:get',
		'ir:*:info:get',
		'ir:*:design:get',
		'ir:*:version:get',
		'ir:*:version:*:get'
		--		
		'estimation:*:get',
		'estimation:*:copy:get',
		'estimation:*:download:get',
		'estimation:*:export:get',
		'estimation:*:generation:get'
               'comparativecohortanalysis:*:get',
		--
		'prediction:*:get',
		'prediction:*:copy:get',
		'prediction:*:download:get',
		'prediction:*:export:get',
		'prediction:*:generation:get',
		'prediction:*:exists:get',
                'plp:*:get'
    )
order by sp.value 
;

chrisknoll · 2023-07-24T18:00:04Z

pom.xml

+    <!-- If defaultGlobalReadPermissions is set to true (default), then all users can see every artifact.  -->
+    <!-- If it is set to false, WebAPI will filter out the artifacts that a user does not explicitly have -->
+    <!-- read permissions to -->
+    <security.defaultGlobalReadPermissions>false</security.defaultGlobalReadPermissions>	


We should default this to true, and rely on settings.xml to override this setting. This will maintain current behavior between versions.

corrected now and tested that setting the variable false in the WebAPIConfig/settings.xml works

src/main/java/org/ohdsi/webapi/cohortcharacterization/CcController.java

chrisknoll · 2023-07-24T18:01:35Z

src/main/java/org/ohdsi/webapi/cohortcharacterization/CcController.java

-            permissionService.fillWriteAccess(entity, dto);
-            return dto;
-        });
+			if (defaultglobalreadpermissions == true) { // don't filter based on read permissions 


This is a place wehre we want to have proper stayle: defaultGlobalReadPermissions

src/main/java/org/ohdsi/webapi/estimation/EstimationController.java

chrisknoll · 2023-07-24T18:06:23Z

src/main/java/org/ohdsi/webapi/estimation/EstimationController.java

+    if (defaultglobalreadpermissions == true) { // don't filter based on read permissions 
+      return StreamSupport.stream(service.getAnalysisList().spliterator(), false)
+	.map(analysis -> {
+	    EstimationShortDTO dto = conversionService.convert(analysis, EstimationShortDTO.class);
+	    permissionService.fillWriteAccess(analysis, dto);
+	    permissionService.fillReadAccess(analysis, dto);
+	    return dto;
+	  })
+	.collect(Collectors.toList());
+    } else {
+      return StreamSupport.stream(service.getAnalysisList().spliterator(), false)
+	.filter(candidateEstimation -> permissionService.hasReadAccess(candidateEstimation))
+	.map(analysis -> {
+	    EstimationShortDTO dto = conversionService.convert(analysis, EstimationShortDTO.class);
+	    permissionService.fillWriteAccess(analysis, dto);
+	    permissionService.fillReadAccess(analysis, dto);
+	    return dto;
+	  })
+	.collect(Collectors.toList());
+    }


I believe this was mentioned before but you can put the filter conditional in the stream expression:

return StreamSupport.stream(service.getAnalysisList().spliterator(), false) .filter(!defaultglobalreadpermissions ? candidateEstimation -> permissionService.hasReadAccess(candidateEstimation) : candidateEstimation -> true) .map(analysis -> { EstimationShortDTO dto = conversionService.convert(analysis, EstimationShortDTO.class); permissionService.fillWriteAccess(analysis, dto); permissionService.fillReadAccess(analysis, dto); return dto; }) .collect(Collectors.toList());

chrisknoll · 2023-07-24T18:15:37Z

src/main/java/org/ohdsi/webapi/security/PermissionController.java

-    @Path("/access/{entityType}/{entityId}")
+    @Path("/access/{entityType}/{entityId}/{permType}")
    @Consumes(MediaType.APPLICATION_JSON)
    @Produces(MediaType.APPLICATION_JSON)
-    public List<RoleDTO> listAccessesForEntity(
+    public List<RoleDTO> listAccessesForEntityByPermType(
            @PathParam("entityType") EntityType entityType,
-            @PathParam("entityId") Integer entityId
+            @PathParam("entityId") Integer entityId,
+	    @PathParam("permType") String permType
    ) throws Exception {

        permissionService.checkCommonEntityOwnership(entityType, entityId);
-
-        Set<String> permissionTemplates = permissionService.getTemplatesForType(entityType, AccessType.WRITE).keySet();
+	Set<String> permissionTemplates = null;
+	if (permType == "WRITE") {
+	  permissionTemplates = permissionService.getTemplatesForType(entityType, AccessType.WRITE).keySet();
+	} else {
+	  permissionTemplates = permissionService.getTemplatesForType(entityType, AccessType.READ).keySet();
+	}


For backwards compatibility, you should maintain the endpoint /access/entityType/entityId, tho this will default to the perm type of 'WRITE'. So, revert this change to leave listAccessForEntity, but have this function call over to 'listAccessForEntityByPermType) with the perType paramater as AccessType.WRITE. In addition, i believe permType should be the enum, not the string, but will need to do some tests if the Java REST api will automatically serialize the ENUM input. The issue is that you could pass a perm type of 'BlahBlah' and it will assume that is 'READ' via the else statement.

chrisknoll · 2023-07-24T18:16:39Z

src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java

-
-		return definitions.stream()
-						.map(def -> {
-							CohortMetadataDTO dto = conversionService.convert(def, CohortMetadataImplDTO.class);
-							permissionService.fillWriteAccess(def, dto);
-							return dto;
-						})
-						.collect(Collectors.toList());
+		if (defaultglobalreadpermissions == true) { // don't filter based on read permissions 
+			return definitions.stream()
+					.map(def -> {
+						CohortMetadataDTO dto = conversionService.convert(def, CohortMetadataImplDTO.class);
+						permissionService.fillWriteAccess(def, dto);
+						permissionService.fillReadAccess(def, dto);
+						return dto;
+					})
+					.collect(Collectors.toList());
+		} else { // filter out cohortdefinitions that the user does not have read access to
+			return definitions.stream()
+					.filter(candidateCohortDef -> permissionService.hasReadAccess(candidateCohortDef))
+					.map(def -> {
+						CohortMetadataDTO dto = conversionService.convert(def, CohortMetadataImplDTO.class);
+						permissionService.fillWriteAccess(def, dto);
+						permissionService.fillReadAccess(def, dto);
+						return dto;
+					})
+					.collect(Collectors.toList());
+		}


use conditional filter here.

chrisknoll · 2023-07-24T18:16:55Z

src/main/java/org/ohdsi/webapi/service/ConceptSetService.java

+   	       if (defaultglobalreadpermissions == true) { // don't filter based on read permissions 
+			return getTransactionTemplate().execute(
+					transactionStatus -> StreamSupport.stream(getConceptSetRepository().findAll().spliterator(), false)
+							.map(conceptSet -> {
+								ConceptSetDTO dto = conversionService.convert(conceptSet, ConceptSetDTO.class);
+								permissionService.fillWriteAccess(conceptSet, dto);
+								permissionService.fillReadAccess(conceptSet, dto);
+								return dto;
+							})
+							.collect(Collectors.toList()));
+		} else { // filter out conceptsets that the user does not have read access to 
+		    return getTransactionTemplate().execute(
+					transactionStatus -> StreamSupport.stream(getConceptSetRepository().findAll().spliterator(), false)
+					                .filter(candidateConceptSet -> permissionService.hasReadAccess(candidateConceptSet))
+							.map(conceptSet -> {
+								ConceptSetDTO dto = conversionService.convert(conceptSet, ConceptSetDTO.class);
+								permissionService.fillWriteAccess(conceptSet, dto);
+								permissionService.fillReadAccess(conceptSet, dto);
+								return dto;
+							})
+					                .collect(Collectors.toList()));


use conditional filter here.

chrisknoll · 2023-07-24T18:18:05Z

src/main/java/org/ohdsi/webapi/service/IRAnalysisService.java

+    if (defaultglobalreadpermissions == true) { // don't filter based on read permissions 
+      return getTransactionTemplate().execute(transactionStatus -> {
+	  Iterable<IncidenceRateAnalysis> analysisList = this.irAnalysisRepository.findAll();
+	  return StreamSupport.stream(analysisList.spliterator(), false)
+	    .map(analysis -> {
                IRAnalysisShortDTO dto = conversionService.convert(analysis, IRAnalysisShortDTO.class);
                permissionService.fillWriteAccess(analysis, dto);
+		permissionService.fillReadAccess(analysis, dto);
                return dto;
              })
-              .collect(Collectors.toList());
-    });
+	    .collect(Collectors.toList());
+	});
+    } else { // filter out entities that the user does not have read permissions to view
+      return getTransactionTemplate().execute(transactionStatus -> {
+	  Iterable<IncidenceRateAnalysis> analysisList = this.irAnalysisRepository.findAll();
+	  return StreamSupport.stream(analysisList.spliterator(), false)
+	    .filter(candidateIRAnalysis -> permissionService.hasReadAccess(candidateIRAnalysis))
+	    .map(analysis -> {
+                IRAnalysisShortDTO dto = conversionService.convert(analysis, IRAnalysisShortDTO.class);
+                permissionService.fillWriteAccess(analysis, dto);
+		permissionService.fillReadAccess(analysis, dto);
+                return dto;
+              })
+	    .collect(Collectors.toList());
+	});


use conditional filter here, will simplify code so that you only have the one getTransactionTemplarte() call, but within the stream you filter conditionally before mapping.

src/main/resources/application.properties

Fixed the default value of DefaultGlobalReadPermissions to be true in pom.xm and to be camel case formatted throughout the code

rkboyce · 2023-07-25T23:41:40Z

A detail worth mentioning - were were curious what would happen if a person without read access knew the URL to an entity and tried a GET. I tested as follows:

logged in as an admin, created a brand new entity (did it for a few types e.g., cohort definition, estimation, etc)
copied the URL
logged out from the admin account and logged in to a user account without access to the entity
put the URL in the browser

The result is an empty page with no message at all. The server simply returns a 403 with an empty body in the response. Atlas renders nothing. So, I think it is Ok for now.

chrisknoll · 2023-07-25T23:47:56Z

Yes, that's fine. Ok, if you're done with your part, I will merge this into a new feature branch under WebAPI so I can put in the final changes. Let me know if you are ready for me to do that.

Merging into feature branch in WebAPI.

rkboyce · 2023-07-28T21:51:31Z

All good here!

chrisknoll · 2023-08-01T12:36:43Z

The new PRs are ready, if you could pull both the Atlas and WebAPI branches, @rkboyce , and give it a shot in your local env, and report in the other PR threads if it is good or if you found issues. Thanks.

The new property is called security.defaultGlobalReadPermissions which is true by default. Co-authored-by: Richard D Boyce, PhD <rdb20@pitt.edu>

rkboyce and others added 8 commits February 13, 2023 14:44

helpful build steps

610af40

Merge branch 'OHDSI:master' into filter_cohorts_and_concepts

20e985d

This is the first working filtration of the conceptset lists so that …

b792094

…a user only sees what their role has permission to read. This initial commit has a big issue in that a person who authors a conceptset cannot see the concept set unless a new permission is added. To be fixed.

Merge branch 'filter_cohorts_and_concepts' of https://github.com/vinc…

db76dfe

…i-ohdsi/WebAPI into HEAD

Merge branch 'OHDSI:master' into filter_cohorts_and_concepts

feab759

fixed issue where the creator of an artifact was skipped as having RE…

1762c70

…AD access

Merge branch 'filter_cohorts_and_concepts' of https://github.com/vinc…

61f30fb

…i-ohdsi/WebAPI into filter_cohorts_and_concepts

Merge branch 'OHDSI:master' into filter_cohorts_and_concepts

e41524a

chrisknoll reviewed Apr 20, 2023

View reviewed changes

chrisknoll changed the title ~~Filter cohorts and concepts~~ Filter cohorts and concepts by permission Apr 25, 2023

rkboyce and others added 12 commits June 19, 2023 15:09

Merge pull request #1 from OHDSI/master

f001a2d

merging upstream

Merge branch 'master' into filter_cohorts_and_concepts

bcaa822

changed the name, data type, and location for the configuration optio…

b8f5220

…n used to tell the WebAPI to do filtering based on READ permissions. The new property is called security.defaultGlobalReadPermissions

changed the name, data type, and location for the configuration optio…

c9b63ea

…n used to tell the WebAPI to do filtering based on READ permissions. The new property is called security.defaultGlobalReadPermissions

partial progress through the list of schema files that need edited to…

317cf3e

… apply the concept filtering to other artifact types

what appears to be the appropriate READ permission schema for cohortd…

e26efba

…efinitions. Tests OK

the appropriate READ permission schema for cohortdefinitions. Tests OK

83befff

added necessary code to filter cohort-characterizations based on read…

3d80d5c

… permissions

added filtering of cohort pathway analyses based on READ permissions

0d0690f

added filtering of IR analyses returned from Atlas based on READ perm…

247effd

…issions

Implemented filtering of prediction entities depending on a users REA…

efa3557

…D permissions - tested and not working completely. Strange permissions issue that appears to be entirely client-side.

rkboyce mentioned this pull request Jul 19, 2023

Filter cohorts and concepts but read permissions - penultimate draft pull request #2297

Closed

rkboyce added 3 commits July 23, 2023 01:47

slight change to add permissions that Atlas checks for for PLP and Es…

28d6480

…timation but that were not obvious previously

changed parameter for listAccessesForEntity from the overloaded 'role…

23a86bc

…' string to a more clear 'permType

changed parameter for listAccessesForEntity from the overloaded 'role…

8c7a54f

…' string to a more clear 'permType

chrisknoll previously requested changes Jul 24, 2023

View reviewed changes

Added the Makefile to the .gitignore

04ac2c8

Fixed the default value of DefaultGlobalReadPermissions to be true in pom.xm and to be camel case formatted throughout the code

chrisknoll changed the base branch from master to issue-2300 July 28, 2023 18:40

chrisknoll marked this pull request as ready for review July 28, 2023 18:41

chrisknoll merged commit 550c088 into OHDSI:issue-2300 Jul 28, 2023

chrisknoll added a commit that referenced this pull request Aug 14, 2023

Filter cohorts and concepts by permission (#2245) (#2301)

5393ed2

The new property is called security.defaultGlobalReadPermissions which is true by default. Co-authored-by: Richard D Boyce, PhD <rdb20@pitt.edu>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter cohorts and concepts by permission #2245

Filter cohorts and concepts by permission #2245

rkboyce commented Apr 4, 2023

chrisknoll Apr 20, 2023

chrisknoll Apr 20, 2023

rkboyce Jul 23, 2023

chrisknoll commented May 18, 2023

rkboyce commented Jul 23, 2023

chrisknoll Jul 24, 2023

rkboyce Jul 25, 2023

chrisknoll Jul 24, 2023

rkboyce Jul 25, 2023

chrisknoll Jul 24, 2023

chrisknoll Jul 24, 2023

chrisknoll Jul 24, 2023

chrisknoll Jul 24, 2023

chrisknoll Jul 24, 2023

rkboyce commented Jul 25, 2023

chrisknoll commented Jul 25, 2023

rkboyce commented Jul 28, 2023

chrisknoll commented Aug 1, 2023

Filter cohorts and concepts by permission #2245

Filter cohorts and concepts by permission #2245

Conversation

rkboyce commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisknoll commented May 18, 2023

rkboyce commented Jul 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkboyce commented Jul 25, 2023

chrisknoll commented Jul 25, 2023

rkboyce commented Jul 28, 2023

chrisknoll commented Aug 1, 2023