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.

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

Optimize permission lookups for a user #10906

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
The following API have been added:
Copy link
Member

Choose a reason for hiding this comment

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

Once we've finalized the name of the API endpoint, please add it to the API Guide.


/api/users/{identifier}/allowedcollections/{permission}
Copy link
Member

Choose a reason for hiding this comment

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

I can live with this name. I would probably camel case it as allowedCollections.

How would we extend this API to datasets or files, if we needed to? Like below?

/api/users/{identifier}/allowedDatasets/{permission}
/api/users/{identifier}/allowedFiles/{permission}

Just playing around below, maybe instead we could have...

/api/users/{identifier}/permissions/collections/{permission}
/api/users/{identifier}/permissions/datasets/{permission}
/api/users/{identifier}/permissions/files/{permission}

? You might want to get some other opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the top APIs better. I think the allowedCollections etc. is more descriptive of what is being returned.

@qqmyers @scolapasta Any comment :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either set of names.


This API lists the dataverses/collections that the user has access to via the permission passed.
Copy link
Member

Choose a reason for hiding this comment

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

What about cases where the user is granted access at runtime based on their membership in Shibboleth groups or IP groups?

We have this related comment in the code:

/**
 * We don't expect this to support Shibboleth groups because even though
 * a Shibboleth user can have an API token the transient
 * shibIdentityProvider String on AuthenticatedUser is only set when a
 * SAML assertion is made at runtime via the browser.
 */

On a related note, that comment is above the call to this method on ServiceDocumentManagerImpl

public List<Dataverse> getDataversesUserHasPermissionOn(AuthenticatedUser user, Permission permission)

Should this method be replaced by the new one in this PR:

public List<Dataverse> findPermittedCollections(AuthenticatedUser user, int permissionBit)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll swap it out and test it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pdurbin I added IP Group support but unfortunately ServiceDocumentManagerImpl doesn't have access to the request and therefore the ip address of the caller. Unless you know of a way to get it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see an obvious way. However, the SWORD API has never supported IP Groups and to my knowledge nobody has asked for it, so I think it's ok.

The main use case for IP Groups is read-only access, such as walking into a library and having access to data because you are on the library's IP range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a way to pass the IPAddress from the request made to Sword.

By passing "any" as the permission the list will return all dataverse/collections that the user can access regardless of which permission is used.
This API can be executed only by administrators.
stevenwinship marked this conversation as resolved.
Show resolved Hide resolved
Valid Permissions are: AddDataverse, AddDataset, ViewUnpublishedDataverse, ViewUnpublishedDataset, DownloadFile, EditDataverse, EditDataset, ManageDataversePermissions,
ManageDatasetPermissions, ManageFilePermissions, PublishDataverse, PublishDataset, DeleteDataverse, DeleteDatasetDraft, and "any" as a wildcard option.
Comment on lines +8 to +9
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to list these here but it might be nice to have an API to list all these permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't have bother listing them in the release note but I wanted to point out the "any" permission for a wildcard. But since this wasn't asked for maybe we just leave it out of the release notes?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, but do you think it's potentially useful to list these permissions via API? Maybe not for this PR but in the future.

The permissions should probably be listed in the guides for now. That way, the release note could link to them.

Copy link
Contributor Author

@stevenwinship stevenwinship Oct 4, 2024

Choose a reason for hiding this comment

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

An API would be nice. There is already a way to list the permissions in a role for the UI so adding a simple list of all permissions should be a quick add.

45 changes: 45 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,41 @@ public class PermissionServiceBean {
@Inject
DatasetVersionFilesServiceBean datasetVersionFilesServiceBean;

private static final String LIST_ALL_DATAVERSES_USER_HAS_PERMISSION = "WITH grouplist AS (\n" +
stevenwinship marked this conversation as resolved.
Show resolved Hide resolved
" SELECT explicitgroup_authenticateduser.explicitgroup_id as id FROM explicitgroup_authenticateduser\n" +
" WHERE explicitgroup_authenticateduser.containedauthenticatedusers_id = @USERID\n" +
")\n" +
"\n" +
"SELECT * FROM DATAVERSE WHERE id IN (\n" +
" SELECT definitionpoint_id\n" +
" FROM roleassignment\n" +
" WHERE roleassignment.assigneeidentifier IN (\n" +
" SELECT CONCAT('&explicit/', explicitgroup.groupalias) as assignee\n" +
" FROM explicitgroup\n" +
" WHERE explicitgroup.id IN (\n" +
" (\n" +
" SELECT explicitgroup.id id\n" +
" FROM explicitgroup\n" +
" WHERE EXISTS (SELECT id FROM grouplist WHERE id = explicitgroup.id)\n" +
" ) UNION (\n" +
" SELECT explicitgroup_explicitgroup.containedexplicitgroups_id id\n" +
" FROM explicitgroup_explicitgroup\n" +
" WHERE EXISTS (SELECT id FROM grouplist WHERE id = explicitgroup_explicitgroup.explicitgroup_id)\n" +
" AND EXISTS (SELECT id FROM dataverserole\n" +
" WHERE dataverserole.id = roleassignment.role_id and (dataverserole.permissionbits & @PERMISSIONBIT !=0))\n" +
" )\n" +
" )\n" +
" ) UNION (\n" +
" SELECT definitionpoint_id\n" +
" FROM roleassignment\n" +
" WHERE roleassignment.assigneeidentifier = (\n" +
" SELECT CONCAT('@', authenticateduser.useridentifier)\n" +
" FROM authenticateduser\n" +
" WHERE authenticateduser.id = @USERID)\n" +
" AND EXISTS (SELECT id FROM dataverserole\n" +
" WHERE dataverserole.id = roleassignment.role_id and (dataverserole.permissionbits & @PERMISSIONBIT !=0))\n" +
" )\n" +
")";
/**
* A request-level permission query (e.g includes IP ras).
*/
Expand Down Expand Up @@ -888,4 +923,14 @@ private boolean hasUnrestrictedReleasedFiles(DatasetVersion targetDatasetVersion
Long result = em.createQuery(criteriaQuery).getSingleResult();
return result > 0;
}

public List<Dataverse> findPermittedCollections(AuthenticatedUser user, int permissionBit) {
if (user != null) {
String sqlCode = LIST_ALL_DATAVERSES_USER_HAS_PERMISSION
.replace("@USERID", String.valueOf(user.getId()))
.replace("@PERMISSIONBIT", String.valueOf(permissionBit));
return em.createNativeQuery(sqlCode, Dataverse.class).getResultList();
}
return null;
}
}
24 changes: 23 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/api/Users.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser;
import edu.harvard.iq.dataverse.authorization.users.User;
import edu.harvard.iq.dataverse.engine.command.impl.ChangeUserIdentifierCommand;
import edu.harvard.iq.dataverse.engine.command.impl.GetUserPermittedCollectionsCommand;
import edu.harvard.iq.dataverse.engine.command.impl.GetUserTracesCommand;
import edu.harvard.iq.dataverse.engine.command.impl.MergeInAccountCommand;
import edu.harvard.iq.dataverse.engine.command.impl.RevokeAllRolesCommand;
Expand Down Expand Up @@ -260,5 +261,26 @@ public Response getTracesElement(@Context ContainerRequestContext crc, @Context
return ex.getResponse();
}
}

@GET
@AuthRequired
@Path("{identifier}/allowedcollections/{permission}")
@Produces("text/csv, application/json")
stevenwinship marked this conversation as resolved.
Show resolved Hide resolved
public Response getUserPermittedCollections(@Context ContainerRequestContext crc, @Context Request req, @PathParam("identifier") String identifier, @PathParam("permission") String permission) {
AuthenticatedUser authenticatedUser = null;
try {
authenticatedUser = getRequestAuthenticatedUserOrDie(crc);
if (!authenticatedUser.isSuperuser()) {
return error(Response.Status.FORBIDDEN, "This API call can be used by superusers only");
}
} catch (WrappedResponse ex) {
return error(Response.Status.UNAUTHORIZED, "Authentication is required.");
}
try {
AuthenticatedUser userToQuery = authSvc.getAuthenticatedUser(identifier);
JsonObjectBuilder jsonObj = execCommand(new GetUserPermittedCollectionsCommand(createDataverseRequest(getRequestUser(crc)), userToQuery, permission));
return ok(jsonObj);
} catch (WrappedResponse ex) {
return ex.getResponse();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package edu.harvard.iq.dataverse.engine.command.impl;

import edu.harvard.iq.dataverse.Dataverse;
import edu.harvard.iq.dataverse.DvObject;
import edu.harvard.iq.dataverse.authorization.Permission;
import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser;
import edu.harvard.iq.dataverse.engine.command.AbstractCommand;
import edu.harvard.iq.dataverse.engine.command.CommandContext;
import edu.harvard.iq.dataverse.engine.command.DataverseRequest;
import edu.harvard.iq.dataverse.engine.command.RequiredPermissions;
import edu.harvard.iq.dataverse.engine.command.exception.CommandException;
import jakarta.json.Json;
import jakarta.json.JsonArrayBuilder;
import jakarta.json.JsonObjectBuilder;

import java.util.List;
import java.util.logging.Logger;

import static edu.harvard.iq.dataverse.util.json.JsonPrinter.json;

@RequiredPermissions({})
public class GetUserPermittedCollectionsCommand extends AbstractCommand<JsonObjectBuilder> {
private static final Logger logger = Logger.getLogger(GetUserPermittedCollectionsCommand.class.getCanonicalName());

private DataverseRequest request;
private AuthenticatedUser user;
private String permission;
public GetUserPermittedCollectionsCommand(DataverseRequest request, AuthenticatedUser user, String permission) {
super(request, (DvObject) null);
this.request = request;
this.user = user;
this.permission = permission;
}

@Override
public JsonObjectBuilder execute(CommandContext ctxt) throws CommandException {
if (user == null) {
throw new CommandException("User not found.", this);
}
int permissionBit;
try {
permissionBit = permission.equalsIgnoreCase("any") ?
Integer.MAX_VALUE : (1 << Permission.valueOf(permission).ordinal());
} catch (IllegalArgumentException e) {
throw new CommandException("Permission not valid.", this);
}
List<Dataverse> collections = ctxt.permissions().findPermittedCollections(user, permissionBit);
if (collections != null) {
JsonObjectBuilder job = Json.createObjectBuilder();
JsonArrayBuilder jab = Json.createArrayBuilder();
for (Dataverse dv : collections) {
jab.add(json(dv));
}
job.add("count", collections.size());
job.add("items", jab);
return job;
}
return null;
}
}
76 changes: 76 additions & 0 deletions src/test/java/edu/harvard/iq/dataverse/api/UsersIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,82 @@ public void testDeleteAuthenticatedUser() {

}

@Test
public void testUserPermittedDataverses() {
Response createSuperuser = UtilIT.createRandomUser();
String superuserUsername = UtilIT.getUsernameFromResponse(createSuperuser);
String superuserApiToken = UtilIT.getApiTokenFromResponse(createSuperuser);
Response toggleSuperuser = UtilIT.makeSuperUser(superuserUsername);
toggleSuperuser.then().assertThat()
.statusCode(OK.getStatusCode());

Response createUser = UtilIT.createRandomUser();
createUser.prettyPrint();
assertEquals(200, createUser.getStatusCode());
String usernameOfUser = UtilIT.getUsernameFromResponse(createUser);
String userApiToken = UtilIT.getApiTokenFromResponse(createUser);

Response createDataverse1 = UtilIT.createRandomDataverse(superuserApiToken);
createDataverse1.prettyPrint();
createDataverse1.then().assertThat()
.statusCode(CREATED.getStatusCode());
String dataverseAlias1 = UtilIT.getAliasFromResponse(createDataverse1);

// create a second Dataverse and add a Group with permissions
Response createDataverse2 = UtilIT.createRandomDataverse(superuserApiToken);
createDataverse2.prettyPrint();
createDataverse2.then().assertThat()
.statusCode(CREATED.getStatusCode());
String dataverseAlias2 = UtilIT.getAliasFromResponse(createDataverse2);
String aliasInOwner = "groupFor" + dataverseAlias2;
String displayName = "Group for " + dataverseAlias2;
Response createGroup = UtilIT.createGroup(dataverseAlias2, aliasInOwner, displayName, superuserApiToken);
String groupIdentifier = JsonPath.from(createGroup.asString()).getString("data.identifier");
Response grantRoleResponse = UtilIT.grantRoleOnDataverse(dataverseAlias2, DataverseRole.EDITOR.toString(), groupIdentifier, superuserApiToken);
grantRoleResponse.prettyPrint();
grantRoleResponse.then().assertThat()
.statusCode(OK.getStatusCode());

Response collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, userApiToken, "ViewUnpublishedDataset");
assertEquals(403, collectionsResp.getStatusCode());
collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, "", "ViewUnpublishedDataset");
assertEquals(401, collectionsResp.getStatusCode());
collectionsResp = UtilIT.getUserPermittedCollections("fakeUser", superuserApiToken, "ViewUnpublishedDataset");
assertEquals(500, collectionsResp.getStatusCode());
collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, superuserApiToken, "bad");
assertEquals(500, collectionsResp.getStatusCode());

// Testing adding an explicit permission/role to one dataverse
collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, superuserApiToken, "DownloadFile");
collectionsResp.prettyPrint();
collectionsResp.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.count", equalTo(0));

Response assignRole = UtilIT.grantRoleOnDataverse(dataverseAlias1, DataverseRole.EDITOR.toString(),
"@" + usernameOfUser, superuserApiToken);
assignRole.prettyPrint();
assertEquals(200, assignRole.getStatusCode());

collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, superuserApiToken, "DownloadFile");
collectionsResp.prettyPrint();
collectionsResp.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.count", equalTo(1));

// Add user to group and test with both explicit and group permissions
Response addToGroup = UtilIT.addToGroup(dataverseAlias2, aliasInOwner, List.of("@" + usernameOfUser), superuserApiToken);
addToGroup.prettyPrint();
addToGroup.then().assertThat()
.statusCode(OK.getStatusCode());

collectionsResp = UtilIT.getUserPermittedCollections(usernameOfUser, superuserApiToken, "DownloadFile");
collectionsResp.prettyPrint();
collectionsResp.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.count", equalTo(2));
}

private Response convertUserFromBcryptToSha1(long idOfBcryptUserToConvert, String password) {
JsonObjectBuilder data = Json.createObjectBuilder();
data.add("builtinUserId", idOfBcryptUserToConvert);
Expand Down
9 changes: 9 additions & 0 deletions src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -1344,6 +1344,15 @@ public static Response getUserTraces(String username, String apiToken) {
return response;
}

public static Response getUserPermittedCollections(String username, String apiToken, String permission) {
RequestSpecification requestSpecification = given();
if (!StringUtil.isEmpty(apiToken)) {
requestSpecification.header(API_TOKEN_HTTP_HEADER, apiToken);
}
Response response = requestSpecification.get("/api/users/" + username + "/allowedcollections/" + permission);
return response;
}

public static Response reingestFile(Long fileId, String apiToken) {
Response response = given()
.header(API_TOKEN_HTTP_HEADER, apiToken)
Expand Down
Loading