Skip to content
Closed
Changes from all 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
6 changes: 6 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/api/Access.java
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,12 @@ public BundleDownloadInstance datafileBundleWithGuestbookResponse(@Context Conta
private DataFile findDataFileUserCanSeeOrDieWrapper(String fileId, DataverseRequest req){

DataFile df = null;

if (req.getUser() instanceof GuestUser) {
// For JSF/UI requests, the ContainerRequestContext user can be GuestUser even when the session is authenticated.
// Locally FAIR visibility checks rely on the DataverseRequest user, so pull the session-backed request here.
req = dvRequestService.getDataverseRequest();
}
Comment on lines +283 to +287

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pending IQSS team review and suggestions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix takes the right approach, but I think this is too broad, as it affects all APIs and is essentially the same as https://guides.dataverse.org/en/latest/installation/config.html#dataverse-feature-api-session-auth, with the same risk noted in the Guides. (Nominally a work-around for testing LFAIR though). The access apis are special in that they are the only ones called from the JSF UI directly, so we need to allow session auth just for them even when the flag is off. This is currently being handled in a few related Access methods - see

// checkAuthorization is a convenience method; it calls the boolean method
// isAccessAuthorized(), the actual workhorse, and throws a 403 exception if not.
private void checkAuthorization(User initialUser, DataFile df) throws WebApplicationException {
User user = getRequestor(initialUser);
if (!isAccessAuthorized(user, df)) {
throw new ForbiddenException();
}
}
private User getRequestor(User user) {
// CompoundAuthMechanism should find the user by API Key/Token, Workflow, etc. And for SPA the Bearer Token
// For JSF check if CompoundAuthMechanism couldn't find the user then try to get it from the session
if (session!=null && user instanceof GuestUser) {
user = session.getUser();
}
return user;
}
and
private boolean checkGuestbookRequiredResponse(User user, UriInfo uriInfo, DataFile df, String gbrids) throws WebApplicationException {
// Check if guestbook response is required
Dataset d = df.getOwner();
boolean required = df.getOwner().hasEnabledGuestbook() && !d.getEffectiveGuestbookEntryAtRequest();
boolean wasWrittenInPost = false;
if (required) {
User requestor = getRequestor(user);
if (requestor instanceof AuthenticatedUser && permissionService.userOn(requestor, df.getOwner()).has(Permission.EditDataset)) {
required = false;
}
// Check if we are downloading a thumbnail image which doesn't require a guestbook response
boolean imageThumb = uriInfo.getQueryParameters().containsKey("imageThumb");
if (imageThumb) {
required = false;
}
if (required && gbrids != null && !gbrids.isEmpty()) {
try {
// verify that this id is good
GuestbookResponse gbr = guestbookResponseService.findById(Long.valueOf(gbrids));
if (gbr == null) {
throw new NotFoundException("GuestbookResponse Not Found for id:" + gbrids);
}
Long delta = Instant.now().toEpochMilli() - gbr.getResponseTime().getTime();
wasWrittenInPost = gbr.getDataset().getId().equals(df.getOwner().getId()) && delta <= (GUESTBOOK_RESPONSE_SIGNEDURL_TIMEOUT_MINUTES * 60000L);
} catch (NumberFormatException | DateTimeParseException ex) {
throw new BadRequestException(ex.getMessage());
}
}
}
return required && !wasWrittenInPost;
}
, which are known to have bugs related to privateUrl users - see #12569, #12548, etc. There is also a PR coming that shuffles a lot of this code #12479.

We probably need to discuss this more, but I might suggest we simplify by just exempting the access apis in https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/api/auth/SessionCookieAuthMechanism.java which should resolve this issue, and the private URL one and clean up the code. I'll put this in triage so this gets on the radar Monday.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the detailed review and context!

My original fix was definitely intended as a minimal proof-of-concept to get the root cause and a workaround on the team's radar.

Your suggestion to handle this at the SessionCookieAuthMechanism level makes a lot of sense. I am happy to pause or close this PR and let the team discuss the best path forward.


try {
df = findDataFileUserCanSeeOrDie(fileId, req);
Expand Down
Loading