TASK-4389 - OpenCGA Catalog - Organizations #2375
Conversation
opencga-analysis/src/main/java/org/opencb/opencga/analysis/cohort/CohortIndexTask.java
Outdated
Show resolved
Hide resolved
...atalog/src/main/java/org/opencb/opencga/catalog/auth/authorization/AuthorizationManager.java
Outdated
Show resolved
Hide resolved
opencga-master/src/main/java/org/opencb/opencga/master/monitor/daemons/AuthorizationDaemon.java
Show resolved
Hide resolved
opencga-master/src/main/java/org/opencb/opencga/master/monitor/daemons/ExecutionDaemon.java
Outdated
Show resolved
Hide resolved
...alog/src/main/java/org/opencb/opencga/catalog/auth/authentication/AuthenticationManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (!isInstallationAdministrator(userId) && !isAdministrativeUser(studyId, userId)) { | ||
| throw new CatalogAuthorizationException("Only administrative users are allowed to assign/remove users to groups."); | ||
| if (ADMINS_GROUP.equals(group) && !isOrganizationOwnerOrAdmin(organizationId, userId)) { |
There was a problem hiding this comment.
Shouldn't this also allow "opencgaAdministrator" ? 99% of the calls to the method "isOrganizatoinOwnerOrAdmin" include a call to "isOpencgaAdministrator". Shouldn't we have a method that checks if is "at least" organizationAdmin? This would include:
- opencgaAdministrator
- organizationOwner
- organizationAdministrator
Some similar methods might be helpful
There was a problem hiding this comment.
Agree. To be changed in the next release.
| /** | ||
| * Check if the user is part of the {@link ParamConstants#ADMINS_GROUP} group of the study. | ||
| * Keep in mind that all organization admins and the organization owner are also study admins. | ||
| * It does not include the opencga admins. |
There was a problem hiding this comment.
isStudyAdmin includes
- studyAdmin
- organizationAdmin
- organizationOwner
but excludes "opencgaAdmin" .. why? Same as before, we have a lot of queries to both methods:
!isOpencgaAdministrator(organizationId, userId) && !isStudyAdministrator(organizationId, studyId, userId) There was a problem hiding this comment.
I understand that there is nothing that an opencgaAdministrator shouldn't be allowed to do
There was a problem hiding this comment.
Agree. To be changed in the next release.
| @@ -131,13 +130,6 @@ enum QueryParams implements QueryParam { | |||
| BATTRIBUTES("battributes", BOOLEAN, ""), // "Format: <key><operation><true|false> where <operation> is [==|!=]" | |||
|
|
|||
| PROJECTS("projects", TEXT_ARRAY, ""), | |||
There was a problem hiding this comment.
No, it will be removed in the next release.
| @@ -117,6 +117,7 @@ | |||
| <dependency> | |||
| <groupId>org.apache.solr</groupId> | |||
| <artifactId>solr-solrj</artifactId> | |||
| <scope>test</scope> | |||
There was a problem hiding this comment.
This and the other solr-related dependencies are not needed.
I can only see one usage at org.opencb.opencga.catalog.managers.StudyManagerTest , and it should be replaced with comons-lang3
import org.apache.solr.common.StringUtils;import org.apache.commons.lang3.StringUtils;There was a problem hiding this comment.
Agree. To be changed in the next release.
|
|
||
| public class JwtPayload { | ||
|
|
||
| private String userId; |
There was a problem hiding this comment.
Shouldn't these fields be "final"?
There was a problem hiding this comment.
True, to be changed in the next release.
TASK-4389