Skip to content

Commit

Permalink
Impl. unauthorized studies returned by /api/studies endpoint
Browse files Browse the repository at this point in the history
  • Loading branch information
pvannierop committed Sep 28, 2021
1 parent 7dbb90e commit 71c535d
Show file tree
Hide file tree
Showing 61 changed files with 385 additions and 135 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public interface AccessControl {
* @throws DaoException Database Error.
* @throws ProtocolException Protocol Error.
*/
@PostFilter("hasPermission(filterObject.getCancerStudyStableId(), 'CancerStudyId', 'read')")
@PostFilter("hasPermission(filterObject.getCancerStudyStableId(), 'CancerStudyId', T(org.cbioportal.utils.security.AccessLevel).READ)")
List<CancerStudy> getCancerStudies() throws DaoException, ProtocolException;

/**
Expand All @@ -71,7 +71,7 @@ public interface AccessControl {
* @return ListCancerStudy
* @throws DaoException
*/
@PostFilter("hasPermission(#stableStudyId, 'CancerStudyId', 'read')")
@PostFilter("hasPermission(#stableStudyId, 'CancerStudyId', T(org.cbioportal.utils.security.AccessLevel).READ)")
List<CancerStudy> isAccessibleCancerStudy(String stableStudyId) throws DaoException;

UserDetails getUserDetails();
Expand Down
9 changes: 9 additions & 0 deletions docs/portal.properties-Reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,15 @@ Different samples of a patient may have been analyzed with different gene panels
skin.patientview.filter_genes_profiled_all_samples=
```

## Control unauthorized studies to be displayed on the home page
By default, on an authenticated portal the home page will only show studies for which the current user is authorized.
By setting the _skin.home_page.show_unauthorized_studies_ property to _true_ the home page will also show unauthorized
studies. The unauthorized studies will appear greyed out and cannot be selected for downstream analysis in Results View or
Study View.
```
skin.home_page.show_unauthorized_studies=
```

## Control the appearance of the settings menu in study view and group comparison that controls custom annotation-based filtering
A settings menu that allows the user to filter alterations in study view and group comparison may be used
when [custom driver annotations](File-Formats.md#custom-driver-annotations) were loaded for the study or studies displayed
Expand Down
15 changes: 13 additions & 2 deletions model/src/main/java/org/cbioportal/model/CancerStudy.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import java.util.Date;
import javax.validation.constraints.NotNull;

public class CancerStudy implements Serializable {
public class CancerStudy implements ReadPermission, Serializable {

private Integer cancerStudyId;
@NotNull
Expand All @@ -31,7 +31,8 @@ public class CancerStudy implements Serializable {
private Integer massSpectrometrySampleCount;
private Integer completeSampleCount;
private String referenceGenome;

private Boolean readPermission = true;

public Integer getCancerStudyId() {
return cancerStudyId;
}
Expand Down Expand Up @@ -219,4 +220,14 @@ public void setMassSpectrometrySampleCount(Integer massSpectrometrySampleCount)
public String getReferenceGenome() { return referenceGenome; }

public void setReferenceGenome(String referenceGenome) { this.referenceGenome = referenceGenome; }

@Override
public void setReadPermission(Boolean permission) {
this.readPermission = permission;
}

@Override
public Boolean getReadPermission() {
return readPermission;
}
}
6 changes: 6 additions & 0 deletions model/src/main/java/org/cbioportal/model/ReadPermission.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package org.cbioportal.model;

public interface ReadPermission {
public void setReadPermission(Boolean permission);
public Boolean getReadPermission();
}
5 changes: 5 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,11 @@
<artifactId>security-spring</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.mskcc.cbio</groupId>
<artifactId>permission-evaluator</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.mongodb</groupId>
<artifactId>mongo-java-driver</artifactId>
Expand Down
1 change: 1 addition & 0 deletions portal/src/main/webapp/config_service.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
"skin.show_tweet_button",
"skin.patientview.filter_genes_profiled_all_samples",
"skin.patientview.show_mskcc_slide_viewer",
"skin.home_page.show_unauthorized_studies",
"skin.show_settings_menu",
"skin.hide_logout_button",
"quick_search.enabled",
Expand Down
24 changes: 24 additions & 0 deletions security/permission-evaluator/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<parent>
<artifactId>security</artifactId>
<groupId>org.mskcc.cbio</groupId>
<version>0-unknown-version-SNAPSHOT</version>
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>permission-evaluator</artifactId>
<dependencies>
<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-core</artifactId>
</dependency>
<dependency>
<groupId>org.mskcc.cbio</groupId>
<artifactId>persistence-mybatis</artifactId>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package org.cbioportal.security.spring;
package org.cbioportal.security;

import java.io.Serializable;
import java.util.*;
import org.apache.commons.logging.*;
import org.cbioportal.model.*;
import org.cbioportal.persistence.cachemaputil.CacheMapUtil;
import org.springframework.beans.factory.annotation.*;
import org.cbioportal.utils.security.AccessLevel;
import org.springframework.security.access.PermissionEvaluator;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.authority.AuthorityUtils;
Expand All @@ -50,7 +51,7 @@
*
* @author Benjamin Gross
*/
class CancerStudyPermissionEvaluator implements PermissionEvaluator {
public class CancerStudyPermissionEvaluator implements PermissionEvaluator {

@Autowired
private CacheMapUtil cacheMapUtil;
Expand Down Expand Up @@ -125,7 +126,7 @@ public boolean hasPermission(Authentication authentication, Object targetDomainO
// authentication will always have authorities.
Object user = authentication.getPrincipal();
if (user != null) {
return hasAccessToCancerStudy(authentication, cancerStudy);
return hasAccessToCancerStudy(authentication, cancerStudy, (AccessLevel) permission);
} else {
return false;
}
Expand Down Expand Up @@ -219,11 +220,21 @@ private CancerStudy getRelevantCancerStudyFromTarget(Object targetDomainObject)
/**
* Helper function to determine if given user has access to given cancer study.
*
* @param authentication Spring Authentication object of the logged-in user.
* @param cancerStudy cancer study to check for
* @param authentication Spring Authentication of the logged-in user.
* @param permission requested permission level (can be org.cbioportal.utils.security.AccessLevel.READ or org.cbioportal.utils.security.AccessLevel.LIST)
* @return boolean
*/
private boolean hasAccessToCancerStudy(Authentication authentication, CancerStudy cancerStudy) {
private boolean hasAccessToCancerStudy(Authentication authentication, CancerStudy cancerStudy, AccessLevel permission) {

// The 'list' permission is only requested by the /api/studies endpoint of StudyController. This permission is
// requested by the Study Overview page when the portal instance is configured to show all studies (with non-
// authorized study options greyed out), instead of only showing authorized studies.
// When the 'list' permission is requested, CancerPermissionEvaluator returns true always.
if (AccessLevel.LIST == permission) {
return true;
}

Set<String> grantedAuthorities = getGrantedAuthorities(authentication);
String stableStudyID = cancerStudy.getCancerStudyIdentifier();
if (log.isDebugEnabled()) {
Expand Down
1 change: 1 addition & 0 deletions security/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<packaging>pom</packaging>
<modules>
<module>security-spring</module>
<module>permission-evaluator</module>
</modules>

<dependencies>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
<b:bean id="expressionHandler" class="org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler">
<b:property name="permissionEvaluator" ref="customPermissionEvaluator"/>
</b:bean>
<b:bean id="customPermissionEvaluator" class="org.cbioportal.security.spring.CancerStudyPermissionEvaluator"/>
<b:bean id="customPermissionEvaluator" class="org.cbioportal.security.CancerStudyPermissionEvaluator"/>

<!-- fstatic resources not processed by spring security filters -->
<http pattern="/css/**" security="none"/>
Expand Down
3 changes: 1 addition & 2 deletions service/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
</dependency>
<dependency>
<groupId>org.mskcc.cbio</groupId>
<artifactId>utils</artifactId>
<artifactId>permission-evaluator</artifactId>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
Expand All @@ -41,7 +41,6 @@
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-jackson</artifactId>
</dependency>

<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.cbioportal.service;

import org.cbioportal.model.ReadPermission;
import org.springframework.security.core.Authentication;

import java.util.Collection;

public interface ReadPermissionService {
public void setReadPermission(Collection<? extends ReadPermission> entities, Authentication authentication);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
import org.cbioportal.model.CancerStudyTags;
import org.cbioportal.model.meta.BaseMeta;
import org.cbioportal.service.exception.StudyNotFoundException;
import org.cbioportal.utils.security.AccessLevel;
import org.springframework.security.core.Authentication;

import java.util.List;

public interface StudyService {

List<CancerStudy> getAllStudies(String keyword, String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction);
List<CancerStudy> getAllStudies(String keyword, String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction, Authentication authentication, AccessLevel accessLevel);

BaseMeta getMetaStudies(String keyword);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class ClinicalAttributeServiceImpl implements ClinicalAttributeService {
private String AUTHENTICATE;

@Override
@PostFilter("hasPermission(filterObject.cancerStudyIdentifier, 'CancerStudyId', 'read')")
@PostFilter("hasPermission(filterObject.cancerStudyIdentifier, 'CancerStudyId', T(org.cbioportal.utils.security.AccessLevel).READ)")
public List<ClinicalAttribute> getAllClinicalAttributes(String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ public List<GeneMolecularData> getMolecularDataInMultipleMolecularProfilesByGene
}

@Override
@PreAuthorize("hasPermission(#molecularProfileIds, 'Collection<MolecularProfileId>', 'read')")
@PreAuthorize("hasPermission(#molecularProfileIds, 'Collection<MolecularProfileId>', T(org.cbioportal.utils.security.AccessLevel).READ)")
public BaseMeta getMetaMolecularDataInMultipleMolecularProfiles(List<String> molecularProfileIds,
List<String> sampleIds, List<Integer> entrezGeneIds) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class MolecularProfileServiceImpl implements MolecularProfileService {
private String AUTHENTICATE;

@Override
@PostFilter("hasPermission(filterObject, 'read')")
@PostFilter("hasPermission(filterObject, T(org.cbioportal.utils.security.AccessLevel).READ)")
public List<MolecularProfile> getAllMolecularProfiles(String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class PatientServiceImpl implements PatientService {
private String AUTHENTICATE;

@Override
@PostFilter("hasPermission(filterObject, 'read')")
@PostFilter("hasPermission(filterObject, T(org.cbioportal.utils.security.AccessLevel).READ)")
public List<Patient> getAllPatients(String keyword, String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction) {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.cbioportal.service.impl;

import org.cbioportal.service.ReadPermissionService;
import org.cbioportal.model.ReadPermission;
import org.cbioportal.security.CancerStudyPermissionEvaluator;
import org.cbioportal.utils.security.AccessLevel;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.Authentication;
import org.springframework.stereotype.Service;

import java.util.Collection;

@Service
public class ReadPermissionServiceImpl implements ReadPermissionService {

// The CancerStudyPermissionEvaluator bean does not exist on portals w/o user-authentication
@Autowired(required = false)
private CancerStudyPermissionEvaluator cancerStudyPermissionEvaluator;

@Override
public void setReadPermission(Collection<? extends ReadPermission> entities, Authentication authentication) {
entities.forEach(s -> {
// Add user read-permission to each entity when authentication is used (defaults
// to 'true' on portals w/o user-authentication).
boolean hasReadPermission = authentication == null || cancerStudyPermissionEvaluator == null ||
cancerStudyPermissionEvaluator.hasPermission(authentication, s, AccessLevel.READ);
s.setReadPermission(hasReadPermission);
});
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class SampleListServiceImpl implements SampleListService {
private String AUTHENTICATE;

@Override
@PostFilter("hasPermission(filterObject, 'read')")
@PostFilter("hasPermission(filterObject, T(org.cbioportal.utils.security.AccessLevel).READ)")
public List<SampleList> getAllSampleLists(String projection, Integer pageSize, Integer pageNumber, String sortBy,
String direction) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,33 +6,37 @@
import org.cbioportal.model.meta.BaseMeta;
import org.cbioportal.persistence.StudyRepository;
import org.cbioportal.service.CancerTypeService;
import org.cbioportal.service.ReadPermissionService;
import org.cbioportal.service.StudyService;
import org.cbioportal.service.exception.StudyNotFoundException;
import org.cbioportal.utils.security.AccessLevel;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.security.access.prepost.PostFilter;
import org.springframework.security.core.Authentication;
import org.springframework.stereotype.Service;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.ArrayList;
import java.util.LinkedHashMap;

@Service
public class StudyServiceImpl implements StudyService {

@Autowired
private StudyRepository studyRepository;

@Autowired
private CancerTypeService cancerTypeService;
@Value("${authenticate:false}")
private String AUTHENTICATE;

@Autowired
private ReadPermissionService readPermissionService;

@Override
@PostFilter("hasPermission(filterObject, 'read')")
@PostFilter("hasPermission(filterObject,#accessLevel)")
public List<CancerStudy> getAllStudies(String keyword, String projection, Integer pageSize, Integer pageNumber,
String sortBy, String direction) {
String sortBy, String direction, Authentication authentication, AccessLevel accessLevel) {

List<CancerStudy> allStudies = studyRepository.getAllStudies(keyword, projection, pageSize, pageNumber, sortBy, direction);
Map<String,CancerStudy> sortedAllStudiesByCancerStudyIdentifier = allStudies.stream().collect(Collectors.toMap(c -> c.getCancerStudyIdentifier(), c -> c, (e1, e2) -> e2, LinkedHashMap::new));
Expand All @@ -48,11 +52,18 @@ public List<CancerStudy> getAllStudies(String keyword, String projection, Intege
}
}
}

// For authenticated portals it is essential to make a new list, such
// that @PostFilter does not taint the list stored in the mybatis
// second-level cache. When making changes to this make sure to copy the
// allStudies list at least for the AUTHENTICATE.equals("true") case
return sortedAllStudiesByCancerStudyIdentifier.values().stream().collect(Collectors.toList());
List<CancerStudy> returnedStudyObjects = sortedAllStudiesByCancerStudyIdentifier.values().stream().collect(Collectors.toList());

// When using prop. 'skin.home_page.show_unauthorized_studies' this endpoint
// returns the full list of studies, some of which can be accessed by the user.
readPermissionService.setReadPermission(returnedStudyObjects, authentication);

return returnedStudyObjects;
}

@Override
Expand All @@ -62,7 +73,7 @@ public BaseMeta getMetaStudies(String keyword) {
}
else {
BaseMeta baseMeta = new BaseMeta();
baseMeta.setTotalCount(getAllStudies(keyword, "SUMMARY", null, null, null, null).size());
baseMeta.setTotalCount(getAllStudies(keyword, "SUMMARY", null, null, null, null, null, AccessLevel.READ).size());
return baseMeta;
}
}
Expand Down
Loading

0 comments on commit 71c535d

Please sign in to comment.