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

Jump to bottom

TASK-5659 - Fix Organizations small issues found during testing #2403

Merged
merged 7 commits into from
Mar 28, 2024
Merged

TASK-5659 - Fix Organizations small issues found during testing #2403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2aa4131
catalog: remove unused NATTRIBUTES, BATTRIBUTES, #TASK-5659
pfurio Feb 20, 2024
19798ee
core: make JwtPayload fields final, #TASK-5659
pfurio Feb 20, 2024
c3835c4
catalog: remove solr dependencies, #TASK-5659
pfurio Feb 20, 2024
c4ea7bc
catalog: authorization check methods include implicit permissions,
pfurio Feb 20, 2024
ece751b
Merge branch 'develop' into TASK-5659
pfurio Mar 21, 2024
8ec6cf6
Merge branch 'develop' into TASK-5659
pfurio Mar 27, 2024
db7fd56
master: fix daemon execution for jobs of scope PROJECT, #TASK-5659
pfurio Mar 28, 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
14 changes: 7 additions & 7 deletions opencga-analysis/src/main/java/org/opencb/opencga/analysis/rga/RgaManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public void index(String study, Path file, String token) throws CatalogException
try {
AuthorizationManager authorizationManager = catalogManager.getAuthorizationManager();
long studyId = studyObject.getUid();
authorizationManager.isStudyAdministrator(organizationId, studyId, userId);
authorizationManager.checkIsAtLeastStudyAdministrator(organizationId, studyId, userId);
} catch (CatalogException e) {
logger.error(e.getMessage(), e);
throw new CatalogException("Only owners or admins can index", e.getCause());
Expand Down Expand Up @@ -244,7 +244,7 @@ public void generateAuxiliarCollection(String studyStr, String token) throws Cat
try {
AuthorizationManager authorizationManager = catalogManager.getAuthorizationManager();
long studyId = study.getUid();
authorizationManager.isStudyAdministrator(organizationId, studyId, userId);
authorizationManager.checkIsAtLeastStudyAdministrator(organizationId, studyId, userId);
} catch (CatalogException e) {
logger.error(e.getMessage(), e);
throw new CatalogException("Only owners or admins can generate the auxiliary RGA collection", e.getCause());
Expand Down Expand Up @@ -456,7 +456,7 @@ private OpenCGAResult<Long> updateRgaInternalIndexStatus(String studyStr, List<S

String collection = getMainCollectionName(study.getFqn());

catalogManager.getAuthorizationManager().checkIsStudyAdministrator(organizationId, study.getUid(), userId);
catalogManager.getAuthorizationManager().checkIsAtLeastStudyAdministrator(organizationId, study.getUid(), userId);

if (!rgaEngine.isAlive(collection)) {
throw new RgaException("Missing RGA indexes for study '" + study.getFqn() + "' or solr server not alive");
Expand Down Expand Up @@ -503,7 +503,7 @@ private OpenCGAResult<Long> updateRgaInternalIndexStatus(String studyStr, String

String collection = getMainCollectionName(study.getFqn());

catalogManager.getAuthorizationManager().checkIsStudyAdministrator(organizationId, study.getUid(), userId);
catalogManager.getAuthorizationManager().checkIsAtLeastStudyAdministrator(organizationId, study.getUid(), userId);

if (!rgaEngine.isAlive(collection)) {
throw new RgaException("Missing RGA indexes for study '" + study.getFqn() + "' or solr server not alive");
Expand Down Expand Up @@ -666,7 +666,7 @@ public OpenCGAResult<RgaKnockoutByGene> geneQuery(String studyStr, Query query,

AuthorizationManager authorizationManager = catalogManager.getAuthorizationManager();
long studyId = study.getUid();
Boolean isOwnerOrAdmin = authorizationManager.isStudyAdministrator(organizationId, studyId, userId);
boolean isOwnerOrAdmin = authorizationManager.isAtLeastStudyAdministrator(organizationId, studyId, userId);
Query auxQuery = query != null ? new Query(query) : new Query();

// Get number of matches
Expand Down Expand Up @@ -809,7 +809,7 @@ public OpenCGAResult<KnockoutByVariant> variantQuery(String studyStr, Query quer

AuthorizationManager authorizationManager = catalogManager.getAuthorizationManager();
long studyId = study.getUid();
Boolean isOwnerOrAdmin = authorizationManager.isStudyAdministrator(organizationId, studyId, userId);
boolean isOwnerOrAdmin = authorizationManager.isAtLeastStudyAdministrator(organizationId, studyId, userId);
Query auxQuery = query != null ? new Query(query) : new Query();

ResourceIds resourceIds;
Expand Down Expand Up @@ -1787,7 +1787,7 @@ private Preprocess individualQueryPreprocess(String organizationId, Study study,

AuthorizationManager authorizationManager = catalogManager.getAuthorizationManager();
long studyId = study.getUid();
boolean isOwnerOrAdmin = authorizationManager.isStudyAdministrator(organizationId, studyId, userId);
boolean isOwnerOrAdmin = authorizationManager.isAtLeastStudyAdministrator(organizationId, studyId, userId);

Preprocess preprocessResult = new Preprocess();
preprocessResult.setUserId(userId);
Expand Down
19 changes: 0 additions & 19 deletions opencga-catalog/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@
<groupId>org.opencb.commons</groupId>
<artifactId>commons-datastore-mongodb</artifactId>
</dependency>
<dependency>
<groupId>org.opencb.commons</groupId>
<artifactId>commons-datastore-solr</artifactId>
</dependency>
<dependency>
<groupId>org.opencb.biodata</groupId>
<artifactId>biodata-formats</artifactId>
Expand Down Expand Up @@ -114,11 +110,6 @@
<artifactId>jjwt-impl</artifactId>
<scope>runtime</scope>
</dependency>
<dependency>
<groupId>org.apache.solr</groupId>
<artifactId>solr-solrj</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>adal4j</artifactId>
Expand Down Expand Up @@ -193,11 +184,6 @@
<groupId>com.github.samtools</groupId>
<artifactId>htsjdk</artifactId>
</dependency>
<dependency>
<groupId>org.apache.solr</groupId>
<artifactId>solr-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-core</artifactId>
Expand Down Expand Up @@ -231,11 +217,6 @@
<artifactId>jjwt-jackson</artifactId>
<scope>runtime</scope>
</dependency>
<dependency>
<groupId>org.apache.solr</groupId>
<artifactId>solr-test-framework</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
16 changes: 7 additions & 9 deletions ...log/src/main/java/org/opencb/opencga/catalog/auth/authorization/AuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package org.opencb.opencga.catalog.auth.authorization;

import org.opencb.opencga.catalog.exceptions.CatalogDBException;
import org.opencb.opencga.catalog.exceptions.CatalogException;
import org.opencb.opencga.catalog.utils.ParamUtils;
import org.opencb.opencga.core.api.ParamConstants;
Expand Down Expand Up @@ -142,18 +141,17 @@ default void checkIsOpencgaAdministrator(String organization, String userId) thr
* @param userId User id
* @throws CatalogException CatalogException
*/
void checkIsOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogException;
void checkIsAtLeastOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogException;

/**
* Check if the given user is the owner of the organization or if it is an admin.
* It does not include opencga admins.
* Check if the given user is the owner or one of the admins of the organization or or one of the Opencga administrators.
*
* @param organization Organization
* @param userId User id
* @return true if the user is the owner or an admin of the organization
* @throws CatalogDBException CatalogDBException
* @return true if the user is the owner or an admin of the organization or one of the Opencga administrators.
* @throws CatalogException CatalogDBException
*/
boolean isOrganizationOwnerOrAdmin(String organization, String userId) throws CatalogDBException;
boolean isAtLeastOrganizationOwnerOrAdmin(String organization, String userId) throws CatalogException;

/**
* Check if the user is part of the {@link ParamConstants#ADMINS_GROUP} group of the study.
Expand All @@ -166,7 +164,7 @@ default void checkIsOpencgaAdministrator(String organization, String userId) thr
* @return true if the user is part of the admins group of the study
* @throws CatalogException CatalogException
*/
boolean isStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException;
boolean isAtLeastStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException;

/**
* Check if the user is part of the {@link ParamConstants#ADMINS_GROUP} group of the study.
Expand All @@ -179,7 +177,7 @@ default void checkIsOpencgaAdministrator(String organization, String userId) thr
* @param userId User id
* @throws CatalogException CatalogException
*/
void checkIsStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException;
void checkIsAtLeastStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException;

void checkFilePermission(String organizationId, long studyId, long fileId, String userId, FilePermissions permission)
throws CatalogException;
Expand Down
40 changes: 23 additions & 17 deletions .../main/java/org/opencb/opencga/catalog/auth/authorization/CatalogAuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ public CatalogAuthorizationManager(DBAdaptorFactory dbFactory, AuthorizationDBAd

@Override
public void checkCanEditProject(String organizationId, long projectId, String userId) throws CatalogException {
if (isOpencgaAdministrator(organizationId, userId) || isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
return;
}
throw new CatalogAuthorizationException("Permission denied: Only the organization owner or administrators can update the project.");
}

@Override
public void checkCanViewProject(String organizationId, long projectId, String userId) throws CatalogException {
if (isOpencgaAdministrator(organizationId, userId) || isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
return;
}

Expand Down Expand Up @@ -117,7 +117,7 @@ public void checkStudyPermission(String organizationId, long studyUid, JwtPayloa
@Override
public void checkStudyPermission(String organizationId, long studyId, String userId, StudyPermissions.Permissions permission)
throws CatalogException {
if (isOpencgaAdministrator(organizationId, userId) || isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
return;
} else {
if (dbAdaptorFactory.getCatalogStudyDBAdaptor(organizationId).hasStudyPermission(studyId, userId, permission)) {
Expand All @@ -129,7 +129,7 @@ public void checkStudyPermission(String organizationId, long studyId, String use

@Override
public void checkCanEditStudy(String organizationId, long studyId, String userId) throws CatalogException {
if (!isOpencgaAdministrator(organizationId, userId) && !isStudyAdministrator(organizationId, studyId, userId)) {
if (!this.isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("modify a study");
}
}
Expand All @@ -148,7 +148,7 @@ public void checkCanViewStudy(String organizationId, long studyId, String userId

@Override
public void checkCanUpdatePermissionRules(String organizationId, long studyId, String userId) throws CatalogException {
if (!isOpencgaAdministrator(organizationId, userId) && !isStudyAdministrator(organizationId, studyId, userId)) {
if (!isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("update the permission rules");
}
}
Expand All @@ -160,7 +160,7 @@ public void checkCreateDeleteGroupPermissions(String organizationId, long studyI
throw new CatalogAuthorizationException(group + " is a protected group that cannot be created or deleted.");
}

if (!isOpencgaAdministrator(organizationId, userId) && !isStudyAdministrator(organizationId, studyId, userId)) {
if (!isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("create or remove groups.");
}
}
Expand All @@ -177,10 +177,10 @@ public void checkUpdateGroupPermissions(String organizationId, long studyId, Str
&& (action != ParamUtils.BasicUpdateAction.ADD && action != ParamUtils.BasicUpdateAction.REMOVE)) {
throw new CatalogAuthorizationException("Only ADD or REMOVE actions are accepted for " + MEMBERS_GROUP + " group.");
}
if (ADMINS_GROUP.equals(group) && !isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (ADMINS_GROUP.equals(group) && !isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
throw CatalogAuthorizationException.notOrganizationOwnerOrAdmin("assign or remove users to the " + ADMINS_GROUP + " group.");
}
if (!isStudyAdministrator(organizationId, studyId, userId)) {
if (!isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("assign or remove users to groups.");
}
}
Expand All @@ -196,14 +196,14 @@ public void checkNotAssigningPermissionsToAdminsGroup(List<String> members) thro

@Override
public void checkCanAssignOrSeePermissions(String organizationId, long studyId, String userId) throws CatalogException {
if (!isOpencgaAdministrator(organizationId, userId) && !isStudyAdministrator(organizationId, studyId, userId)) {
if (!isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("assign or see all permissions");
}
}

@Override
public void checkCanCreateUpdateDeleteVariableSets(String organizationId, long studyId, String userId) throws CatalogException {
if (!isOpencgaAdministrator(organizationId, userId) && !isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (!isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
throw CatalogAuthorizationException.notOrganizationOwnerOrAdmin("create, update or delete variable sets.");
}
}
Expand All @@ -227,28 +227,34 @@ public void checkIsOpencgaAdministrator(String organizationId, String userId, St
}

@Override
public void checkIsOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogException {
if (!isOrganizationOwnerOrAdmin(organizationId, userId)) {
public void checkIsAtLeastOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogException {
if (!isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
throw CatalogAuthorizationException.notOrganizationOwnerOrAdmin();
}
}

@Override
public boolean isOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogDBException {
public boolean isAtLeastOrganizationOwnerOrAdmin(String organizationId, String userId) throws CatalogException {
if (isOpencgaAdministrator(organizationId, userId)) {
return true;
}
OrganizationDBAdaptor organizationDBAdaptor = dbAdaptorFactory.getCatalogOrganizationDBAdaptor(organizationId);
Organization organization = organizationDBAdaptor.get(OrganizationManager.INCLUDE_ORGANIZATION_ADMINS).first();
return organization.getOwner().equals(userId) || organization.getAdmins().contains(userId);
}

@Override
public void checkIsStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException {
if (!isStudyAdministrator(organizationId, studyId, userId)) {
public void checkIsAtLeastStudyAdministrator(String organizationId, long studyId, String userId) throws CatalogException {
if (!isAtLeastStudyAdministrator(organizationId, studyId, userId)) {
throw CatalogAuthorizationException.notStudyAdmin("perform this action");
}
}

@Override
public boolean isStudyAdministrator(String organizationId, long studyId, String user) throws CatalogException {
public boolean isAtLeastStudyAdministrator(String organizationId, long studyId, String user) throws CatalogException {
if (isAtLeastOrganizationOwnerOrAdmin(organizationId, user)) {
return true;
}
OpenCGAResult<Group> groupBelonging = getGroupBelonging(organizationId, studyId, user);
for (Group group : groupBelonging.getResults()) {
if (group.getId().equals(ADMINS_GROUP)) {
Expand All @@ -274,7 +280,7 @@ public void checkFilePermission(String organizationId, long studyId, long fileId

private boolean checkUserPermission(String organizationId, String userId, Query query, CoreDBAdaptor dbAdaptor)
throws CatalogException {
if (isOpencgaAdministrator(organizationId, userId) || isOrganizationOwnerOrAdmin(organizationId, userId)) {
if (isAtLeastOrganizationOwnerOrAdmin(organizationId, userId)) {
return true;
} else {
if (dbAdaptor.count(query, userId).getNumMatches() == 1) {
Expand Down
2 changes: 0 additions & 2 deletions ...ga-catalog/src/main/java/org/opencb/opencga/catalog/db/api/ClinicalAnalysisDBAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ enum QueryParams implements QueryParam {
DISORDER_NAME("disorder.name", TEXT, ""),
TYPE("type", TEXT, ""),
ATTRIBUTES("attributes", TEXT, ""), // "Format: <key><operation><stringValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
NATTRIBUTES("nattributes", DECIMAL, ""), // "Format: <key><operation><numericalValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
BATTRIBUTES("battributes", BOOLEAN, ""), // "Format: <key><operation><true|false> where <operation> is [==|!=]"
STATUS("status", OBJECT, ""),
STATUS_ID("status.id", TEXT, ""),
STATUS_DATE("status.date", TEXT, ""),
Expand Down
2 changes: 0 additions & 2 deletions opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/api/CohortDBAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,6 @@ enum QueryParams implements QueryParam {
ANNOTATION("annotation", TEXT_ARRAY, ""),

ATTRIBUTES("attributes", TEXT, "Format: <key><operation><stringValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"),
NATTRIBUTES("nattributes", DECIMAL, "Format: <key><operation><numericalValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"),
BATTRIBUTES("battributes", BOOLEAN, "Format: <key><operation><true|false> where <operation> is [==|!=]"),

STUDY_UID("studyUid", INTEGER_ARRAY, ""),
STUDY("study", INTEGER_ARRAY, ""); // Alias to studyId in the database. Only for the webservices.
Expand Down
2 changes: 0 additions & 2 deletions opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/api/FamilyDBAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ enum QueryParams implements QueryParam {
DESCRIPTION("description", TEXT, ""),
EXPECTED_SIZE("expectedSize", INTEGER, ""),
ATTRIBUTES("attributes", TEXT, ""), // "Format: <key><operation><stringValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
NATTRIBUTES("nattributes", DECIMAL, ""), // "Format: <key><operation><numericalValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
BATTRIBUTES("battributes", BOOLEAN, ""), // "Format: <key><operation><true|false> where <operation> is [==|!=]"
STATUS("status", TEXT_ARRAY, ""),
STATUS_ID("status.id", TEXT, ""),
STATUS_DATE("status.date", TEXT, ""),
Expand Down
2 changes: 0 additions & 2 deletions opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/api/FileDBAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,6 @@ enum QueryParams implements QueryParam {
INTERNAL_MISSING_SAMPLES_NON_EXISTING("internal.missingSamples.nonExisting", TEXT_ARRAY, ""),

ATTRIBUTES("attributes", TEXT, ""), // "Format: <key><operation><stringValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
NATTRIBUTES("nattributes", DECIMAL, ""), // "Format: <key><operation><numericalValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
BATTRIBUTES("battributes", BOOLEAN, ""), // "Format: <key><operation><true|false> where <operation> is [==|!=]"
STATS("stats", TEXT, ""),
NSTATS("nstats", DECIMAL, ""),

Expand Down
2 changes: 0 additions & 2 deletions opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/api/IndividualDBAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@ enum QueryParams implements QueryParam {
KARYOTYPIC_SEX("karyotypicSex", TEXT, ""),
LIFE_STATUS("lifeStatus", TEXT, ""),
ATTRIBUTES("attributes", TEXT, ""), // "Format: <key><operation><stringValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
NATTRIBUTES("nattributes", DECIMAL, ""), // "Format: <key><operation><numericalValue> where <operation> is [<|<=|>|>=|==|!=|~|!~]"
BATTRIBUTES("battributes", BOOLEAN, ""), // "Format: <key><operation><true|false> where <operation> is [==|!=]"

DELETED(ParamConstants.DELETED_PARAM, BOOLEAN, ""),

Expand Down
Loading
Loading