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

Hotfix/fix admin check #39

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Hotfix/fix admin check #39

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f157b9
fix method to determine if user is admin, add two new convenience met…
wow-such-code Dec 21, 2020
4d48850
update interface
wow-such-code Dec 21, 2020
9d17f78
prepare hotfix version 1.5.1
wow-such-code Dec 21, 2020
40aa0d1
add interface methods to mock
wow-such-code Dec 21, 2020
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
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
</parent>
<artifactId>openbis-client-lib</artifactId>
<packaging>jar</packaging>
<version>1.5.0</version>
<version>1.5.1</version>
<name>openBIS client library</name>
<!-- we only need to tell maven where to find our parent pom and other QBiC
dependencies -->
Expand Down
28 changes: 27 additions & 1 deletion src/main/java/life/qbic/openbis/openbisclient/IOpenBisClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import ch.ethz.sis.openbis.generic.asapi.v3.dto.experiment.ExperimentType;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.project.Project;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.property.PropertyType;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.roleassignment.Role;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.roleassignment.RoleLevel;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.sample.Sample;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.sample.SampleType;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.vocabulary.Vocabulary;
Expand Down Expand Up @@ -295,12 +297,36 @@ public interface IOpenBisClient {
public List<String> getUserSpaces(String userID);

/**
* Returns wether a user is instance admin in openBIS
* Returns whether a user is instance admin in openBIS. Checks both a user's direct role assignments as well as their groups' assignments
*
* @param userID the user's id
* @return true, if user is instance admin, false otherwise
*/
public boolean isUserAdmin(String userID);

/**
* Returns whether a user with a given user Id is assigned a given role at a given role level.
* Does not check user groups of that user!
*
* @param userID the user's id
* @param role the openBIS role
* @param level the openBIS role level, denoting if the user has that role for the instance or
* just one or more spaces or projects
* @return true, if user has that role, false otherwise
*/
public boolean userHasRole(String userID, Role role, RoleLevel level);

/**
* Returns whether a user's user group is assigned a given role at a given role level
*
* @param userID the user's id
* @param role the openBIS role
* @param level the openBIS role level, denoting if the user and their group has that role for the
* instance or just one or more spaces or projects
* @return true, if user has that role through their user group, false otherwise
*/
public boolean usersGroupHasRole(String userID, Role role, RoleLevel level);

/**
* Function to retrieve a project from openBIS by the identifier of the project.
*
Expand Down
60 changes: 58 additions & 2 deletions src/main/java/life/qbic/openbis/openbisclient/OpenBisClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
import static life.qbic.openbis.openbisclient.helper.OpenBisClientHelper.fetchSamplesCompletely;
import ch.ethz.sis.openbis.generic.asapi.v3.IApplicationServerApi;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.attachment.Attachment;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.authorizationgroup.AuthorizationGroup;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.authorizationgroup.fetchoptions.AuthorizationGroupFetchOptions;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.authorizationgroup.search.AuthorizationGroupSearchCriteria;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.common.interfaces.IEntityType;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.common.search.SearchResult;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.dataset.DataSet;
Expand Down Expand Up @@ -35,6 +38,7 @@
import ch.ethz.sis.openbis.generic.asapi.v3.dto.property.fetchoptions.PropertyAssignmentFetchOptions;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.roleassignment.Role;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.roleassignment.RoleAssignment;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.roleassignment.RoleLevel;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.sample.Sample;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.sample.SampleType;
import ch.ethz.sis.openbis.generic.asapi.v3.dto.sample.fetchoptions.SampleFetchOptions;
Expand Down Expand Up @@ -570,12 +574,31 @@ public List<String> getUserSpaces(String userID) {
}

/**
* Returns wether a user is instance admin in openBIS
* Returns whether a user is instance admin in openBIS. Checks both a user's direct role
* assignments as well as their groups' assignments
*
* @param userID the user's id
* @return true, if user is instance admin, false otherwise
*/
@Override
public boolean isUserAdmin(String userID) {
Role role = Role.ADMIN;
RoleLevel level = RoleLevel.INSTANCE;
return userHasRole(userID, role, level) || usersGroupHasRole(userID, role, level);
}

/**
* Returns whether a user with a given user Id is assigned a given role at a given role level.
* Does not check user groups of that user!
*
* @param userID the user's id
* @param role the openBIS role
* @param level the openBIS role level, denoting if the user has that role for the instance or
* just one or more spaces or projects
* @return true, if user has that role, false otherwise
*/
@Override
public boolean userHasRole(String userID, Role role, RoleLevel level) {
ensureLoggedIn();
PersonSearchCriteria criteria = new PersonSearchCriteria();
criteria.withUserId().thatEquals(userID);
Expand All @@ -584,14 +607,47 @@ public boolean isUserAdmin(String userID) {
SearchResult<Person> res = v3.searchPersons(sessionToken, criteria, options);
for (Person p : res.getObjects()) {
for (RoleAssignment r : p.getRoleAssignments()) {
if (r.getRole().equals(Role.ADMIN)) {
if (r.getRole().equals(role) && r.getRoleLevel().equals(level)) {
return true;
}
}
}
return false;
}

/**
* Returns whether a user's user group is assigned a given role at a given role level
*
* @param userID the user's id
* @param role the openBIS role
* @param level the openBIS role level, denoting if the user and their group has that role for the
* instance or just one or more spaces or projects
* @return true, if user has that role through their user group, false otherwise
*/
@Override
public boolean usersGroupHasRole(String userID, Role role, RoleLevel level) {
ensureLoggedIn();
AuthorizationGroupSearchCriteria criteria = new AuthorizationGroupSearchCriteria();
AuthorizationGroupFetchOptions options = new AuthorizationGroupFetchOptions();
options.withRoleAssignments().withAuthorizationGroup().withRoleAssignments();
options.withUsers();
SearchResult<AuthorizationGroup> searchResult =
v3.searchAuthorizationGroups(sessionToken, criteria, options);

for (AuthorizationGroup group : searchResult.getObjects()) {
for (Person person : group.getUsers()) {
if (person.getUserId().equals(userID)) {
for (RoleAssignment r : group.getRoleAssignments()) {
if (r.getRole().equals(role) && r.getRoleLevel().equals(level)) {
return true;
}
}
}
}
}
return false;
Comment on lines +637 to +648
Copy link
Contributor

Choose a reason for hiding this comment

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

three nested loops o.0. I find this hard to read and difficult to debug.

My suggestions:

  1. Maybe introduce some filter methods that make the code more readable
  2. Search for the groups where the users belongs to in the first place (search criteria, if API allows)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I could not fetch the groups directly via the role assignments (which should be possible according to the API), but I can certainly try to add a search subcriterium to find only groups of a specific user.

Copy link
Member Author

Choose a reason for hiding this comment

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

there does not seem to be an easy way to search for groups of a specific user. will look into this more tomorrow

}

@Override
public Project getProjectByIdentifier(String projectIdentifier) {
ensureLoggedIn();
Expand Down