-
Notifications
You must be signed in to change notification settings - Fork 496
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
base: develop
Are you sure you want to change the base?
Changes from 15 commits
cfd9da8
7f8ed30
66c112d
e51cf6c
3b860e5
83c01d1
728835e
7257e7d
a194138
423077d
ef85364
529db68
5a1f151
a10ba68
6090fb5
be2c143
6560326
5d0d0ec
65e87ed
3606328
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
The following API have been added: | ||
|
||
/api/users/{identifier}/allowedcollections/{permission} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 How would we extend this API to datasets or files, if we needed to? Like below? /api/users/{identifier}/allowedDatasets/{permission} Just playing around below, maybe instead we could have... /api/users/{identifier}/permissions/collections/{permission} ? You might want to get some other opinions. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
On a related note, that comment is above the call to this method on ServiceDocumentManagerImpl
Should this method be replaced by the new one in this PR:
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll swap it out and test it There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 the User requesting their own list of accessible collections or by an Administrator. | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
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.groups.impl.ipaddress.ip.IpAddress; | ||
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(request, 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; | ||
} | ||
} |
There was a problem hiding this comment.
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.