#10403 if a dataset is not published only users with ViewUnpublishedD…

[pkiraly]

…ataset AND DownloadFile rights can download

What this PR does / why we need it:
There are two permissions which have undocumented hierarchical relationship: ViewUnpublishedDataset and DownloadFile. The user who has no DownloadFile, but has ViewUnpublishedDataset permission can download files, which is - according to our users and I agree with them - counter intuitive. I expect that the person who does not have right to download files should not be able to download files. So this PR adds a second condition: only users can download filed from unpublished datasets who have both ViewUnpublishedDataset and DownloadFile rights.

Which issue(s) this PR closes:

Closes #10403

Special notes for your reviewer:

Suggestions on how to test this:

1. Create a user with a role covering ViewUnpublishedDataset (user1), and another one who have role covering ViewUnpublishedDataset and DownloadFile (user2)
2. create a dataset (but not publish it) with a single file
3. be sure that user1 can not download the file, but user2 can

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No

Is there a release notes update needed for this change?:
No

Additional documentation:
No

[qqmyers]

@pkiraly - I haven't tested this, but I assume a change like this would have impacts outside just this one permission check, e.g. file download permissions would need to be added when creating a dataset, and, in existing systems, permissions would have to be added so access to current datasets won't change just by installing the new version, etc. Are you looking into those?

Given that, another question - would it acceptable/easier just to change the label(s) of the permission(s), e.g. ViewAndDownloadFromUnpublishedDataset and DownloadReleasedFile or similar? If there are really use cases where someone needs access to view the dataset but not be able to access the files, then perhaps the permission definitions need to change as you indicate, but if it is just a matter of clarifying what is allowed, perhaps label changes are enough?

[pkiraly]

@qqmyers This feature s really requested by our users. I am testing other use cases if it cases any problems elsewhere.

[pdurbin]

@pkiraly any news on your further testing? I agree with @qqmyers that the current behavior is so baked into the system that there are probably other places in the code that would need to be updated for consistency.

[stevenwinship]

For consistency, consider adding the Permission.DownloadFile permission check to line 682

if (draft != null && permissionService.requestOn(req, retrieved).has(Permission.ViewUnpublishedDataset)) {                    
                    String fileIds = getFileIdsAsCommaSeparated(draft.getFileMetadatas());
                    // We don't want downloads from Draft versions to be counted, 
                    // so we are setting the gbrecs (aka "do not write guestbook response") 
                    // variable accordingly:
                    return downloadDatafiles(getRequestUser(crc), fileIds, true, uriInfo, headers, response);
                }

[pdurbin]

@pkiraly hi! I'm just checking in. Are you still working on this?

[pdurbin]

@pkiraly we haven't heard from you so I'm going to close this. Please consider it a soft close. 😄 We can reopen it, if you like. Please see the feedback above, especially the suspicion that there are probably additional places in the code that probably need to be changed.