Iqss/8109 separate managefilepermissions #8174

qqmyers · 2021-10-21T19:40:40Z

What this PR does / why we need it:

Adds a new ManageFilePermissions permission defined on datasets that is required to add/revoke FileDownload permissions and view file download requests, etc.

This then allows defining a role that would allow a user to respond to file download requests without also giving them the power to manage the dataset (which allows you to then increase your own permissions on that dataset.)

Any existing roles with the ManageDatasetPermissions permission are updated to also have this new permission (the existing admin and curator roles are the only two standard roles that have the ManageDatasetPermissions permission) via a flyway script and the role definitions for admin and curator ( json files in the scripts/api/data folder) are also updated.

Which issue(s) this PR closes:

Closes #8109

Special notes for your reviewer: Should this PR define a new role that can manage files and not the dataset permissions (i.e. the use case driving the PR) versus having sites create their own custom roles? Any name suggestions?

Suggestions on how to test this:
1)Verify that admins and curators can still add/remove file download permissions (and can do all of the other dataset-level permission assignments as well ).
2) Create a role having the new ManageFilePermissions permission and not the ManageDatasetPermissions, assign a user to that role for a given dataset, and verify that a) that user can see the Permissions/File menu item and not the Permissions/Dataset option, and b) that they can then use the file permissions page to add/revoke file download permissions. Also verify that the user can run the /api/access/datafile/{id}/listRequests and /api/access//datafile/{id}/rejectAccess/{identifier} api calls on that dataset/its files.
3) FWIW: In the db, doing a select id, perimissionbits::bit(14) from dataverserole; is a useful way to see the bitmap of permissions assigned to a role. What should be happening in the update it that any role with bit 9 should have a new bit 10 with the same value and the existing bit 10-13 values shifted into bits 11-14. Using letters for bit positions (a=1 through m=13):
mlkjihgfedcba --> mlkjiihgfedcba

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Changes the Dataset/Edit Dataset/Permissions menu to only have one option - Dataset or File - if you only have the original ManageDatasetPermissions or the new ManageFilePermissions permission, respectively. (Existing roles which have both permissions after the PR still see a menu with the original two options.).
The new ManageFilePermissions permission shows up in the list for the admin and curator roles where that is shown (e.g. on the permission management dialogs).

Is there a release notes update needed for this change?: probably worth mentioning - will wait to draft until we decide whether there should be a new standard role having this permission or not.

Additional documentation: tbd - same issue as release notes - will wait for decision re: whether there's a new standard role involved.

…anagefilepermissions

coveralls · 2021-10-21T19:46:21Z

Coverage decreased (-0.001%) to 18.923% when pulling dc0b83e on GlobalDataverseCommunityConsortium:IQSS/8109-separate-managefilepermissions into 3a65b68 on IQSS:develop.

djbrooke · 2021-10-21T21:05:28Z

@scolapasta I know it's still a draft PR but I'll pre-assign you as I know you are the permissions guru.

qqmyers · 2021-10-26T14:04:27Z

DANS looked this over and tested a bit, so it should be good to go.

scolapasta · 2021-10-27T19:14:36Z

src/main/java/propertyFiles/Bundle.properties

 permission.viewUnpublishedDataset=View an unpublished dataset and its files
 permission.viewUnpublishedDataverse=View an unpublished dataverse
 permission.addDatasetDataverse=Add a dataset to a dataverse
+permission.managePermissionsDatasetFiles=Manage permissions for a dataset's file


should this be files?

scolapasta · 2021-10-27T19:17:13Z

src/main/java/propertyFiles/Bundle.properties

 permission.ViewUnpublishedDataset.desc=View an unpublished dataset and its files
 permission.ViewUnpublishedDataverse.desc=View an unpublished dataverse
 permission.AddDataset.desc=Add a dataset to a dataverse
+permission.ManageFilePermissions.desc=Manage permissions for a file


not exactly sure when this is used vs the one without "desc" buty should it match - the others seem to

scolapasta · 2021-10-27T19:50:13Z

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RevokeRoleCommand.java


 	public RevokeRoleCommand(RoleAssignment toBeRevoked, DataverseRequest aRequest) {
-        // for data file check permission on owning dataset
+        // for data file, report affect object as owning dataset


See my comment above on permissions, but if we say it applies to files, should the affected object just be the file now?

scolapasta · 2021-10-27T19:51:17Z

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AssignRoleCommand.java

        return Collections.singletonMap("",
                defPoint instanceof Dataverse ? Collections.singleton(Permission.ManageDataversePermissions)
-                : Collections.singleton(Permission.ManageDatasetPermissions));
+                : defPoint instanceof Dataset ? Collections.singleton(Permission.ManageDatasetPermissions) : Collections.singleton(Permission.ManageFilePermissions));


See my comment above on permissions, but if we say it applies to files, should the affected object just be the file now? (added here as github didn't seem to let me add this comment to line 48)

I chose this because I think that makes it appear as one of the dataset-level permissions you can assign on a dataset. The language is confusing but I think this currently is a dataset-level permission allowing you to grant the individual file-level download permissions.

I think it could be redesigned as a file-level permission to that let's you assign download permissions to that file, but I'm not sure that's needed and it seems like a lot of extra assignment points.

I may be missing something in all of this though.

So, to be clear - roles are what appear to assign. But yes roles will only show up if they have permissions for your type of object, or lower. So a role with only dataverse level permissions will not show up when you assign a role at the dataset level. BUT do note the "or lower" part. Since roles can cascade down, a role with only file level permissions will still show up when you assign a role at the dataset level. For example, file downloader.

Changing it now to file level won't change anything about the way you have it working, users will still assign these roles at the dataset and it will apply to granting permission for ALL files in that dataset. But it would allow us to some day add a workflow to have someone only grant access to some files.

It seems to me a small enough change - change the level on the permission and the affected object on those two commands, and you can see it will still work as designed for assigning at the dataset.

My vote would be to change this now, BUT if we decide not to change it now; let's add a comment on the permission so we know what we need to to make this change in the future sometime.

OK but I'm still confused some about the specific change: I can change the object type to DataFile in the Permissions class. Is there anything else?

The Assign/Revoke role commands are reporting the owning Dataset as the object if the role is on a file already - for the DownloadPermission. Changing the ManageFilePermissions to use DataFile, would without further changes, still report it as affecting the owning Dataset. Whereas if I change the commands to report DataFile as the affected object, it would affect DownloadPermission as well. So - any change in the Commands now?

On the UI side, the menu appears/disappears if you have ManageFilePermissions on the Dataset and the canManageFilesOnDataset(Dataset) method checks that Dataset. To really use this on DataFiles, we'd eventually need a canManageFile(DataFile) method as well? (instead?) Is there any change in these areas at this point?

@qqmyers let's discuss briefly today and we can decide on the change or not and move along.

scolapasta · 2021-10-27T19:55:28Z

src/main/java/edu/harvard/iq/dataverse/authorization/Permission.java

-    DeleteDatasetDraft(BundleUtil.getStringFromBundle("permission.deleteDataset"), true, Dataset.class);
+    DeleteDatasetDraft(BundleUtil.getStringFromBundle("permission.deleteDataset"), true, Dataset.class),
+    //Update again
+    ManageFilePermissions(BundleUtil.getStringFromBundle("permission.managePermissionsDatasetFiles"), true, Dataset.class);


I know this was added at the end because easier to change permission bits in db. BUT this does show up in this order in the UI, so I suggest we move it up next to the other Manage perms and redo the SQL script to handle.

Also, should the applies to be datafile (I know for know we will only apply to Dataset and it will cascade down, since otherwise would require some new UI changes), but to future proof in case some day we want to have a role that can only manage permissions on a subset of dataset files?

This makes sense. I don't have time right now to do this but will try to get to it. Shouldn't be that hard.

Updated to move the Permission in the enum. Added lots of description in the flyway script to help in reviewing the logic there.

…anagefilepermissions

scolapasta · 2021-11-04T19:28:03Z

src/main/java/edu/harvard/iq/dataverse/authorization/Permission.java

    ManageDataversePermissions(BundleUtil.getStringFromBundle("permission.managePermissionsDataverse"), true, Dataverse.class),
    ManageDatasetPermissions(BundleUtil.getStringFromBundle("permission.managePermissionsDataset"), true, Dataset.class),
-    ManageFilePermissions(BundleUtil.getStringFromBundle("permission.managePermissionsDatasetFiles"), true, Dataset.class),
+    ManageFilePermissions(BundleUtil.getStringFromBundle("permission.managePermissionsDataFiles"), true, DataFile.class),


Very Minor change, but let's make this singular to be in parallel with the others.

scolapasta · 2021-11-04T19:29:40Z

src/main/java/propertyFiles/Bundle.properties

 permission.viewUnpublishedDataverse=View an unpublished dataverse
 permission.addDatasetDataverse=Add a dataset to a dataverse
-permission.managePermissionsDatasetFiles=Manage permissions for a dataset's files
+permission.managePermissionsDataFiles=Manage permissions for a dataFile(s)


remove the s and there's an extraneous "a"

scolapasta · 2021-11-04T19:30:03Z

src/main/java/propertyFiles/Bundle.properties

 permission.ViewUnpublishedDataverse.desc=View an unpublished dataverse
 permission.AddDataset.desc=Add a dataset to a dataverse
-permission.ManageFilePermissions.desc=Manage permissions for a dataset's files
+permission.ManageFilePermissions.desc=Manage permissions for a file(s)


remove the s and there's an extraneous "a" (and one above says dataFiles and this says files - let's keep the same still

Changed the text and moved these next to the dataset/dataverse ones. Note that this line doesn't have an extraneous s (if I understand) - the names of the permissions are plural, but the labels defined for them are a different form and reference a singular object.

scolapasta

Changes look good. Thanks.

pdurbin · 2021-11-04T19:53:42Z

Is there a release notes update needed for this change?: probably worth mentioning - will wait to draft until we decide whether there should be a new standard role having this permission or not.

Additional documentation: tbd - same issue as release notes - will wait for decision re: whether there's a new standard role involved.

I saw this went into QA which is great but there are no docs and no release note. From standup yesterday it sounds like we decided that we're not adding a new role in this pull request, which sounds fine. That probably means we don't need any docs. Does it also mean we don't need a release note? Perhaps a release note could at least mention the new ManageFilePermissions permission and indicate UI changes (from above):

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Changes the Dataset/Edit Dataset/Permissions menu to only have one option - Dataset or File - if you only have the original ManageDatasetPermissions or the new ManageFilePermissions permission, respectively. (Existing roles which have both permissions after the PR still see a menu with the original two options.).
The new ManageFilePermissions permission shows up in the list for the admin and curator roles where that is shown (e.g. on the permission management dialogs).

scolapasta · 2021-11-04T19:59:17Z

Well, with the core code there would not actually be a UI change - only with a new role that only had one or the other permission.

I agree that I don't think we actually need docs (unless we do document already the permissions / roles that exist with that level of detail.

As far as release notes, do we normally create a note as part of the PR for a new feature like this if it doesn't actually require anything of the admin? Or does @djbrooke just add some detail for the new "feature" when he creates the notes?

djbrooke · 2021-11-04T20:05:02Z

@pdurbin @scolapasta I can add something to this branch, probably at the use case level. Not sure it raises to the level of a big chunk. :)

qqmyers · 2021-11-04T20:36:45Z

FWIW: I added a short release note - feel free to use/ignore the text there.

IQSS/8109-separate-managefilepermissions Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RevokeRoleCommand.java

kcondon · 2021-11-16T20:54:44Z

Preliminary testing:
Basic smoketest works, admin and curator roles still can assign/revoke perms, ManageFilePermissions user can access ListRequests endpoint, non permitted user cannot.
Issues/questions:

Roles with only ManageFilePermissions or ManageDatasetPermissions cannot see edit->permissions in UI for dataset.
ListRequests endpoint throws log exception if don't provide api token, likely preexisting issue.
Was not able to get grantAccess or rejectAccess endpoints to work, likely syntax issue due to returning 404.

qqmyers added 9 commits October 19, 2021 17:02

add permission definition

5da5652

update to use ManageFilePermission

af2291a

update curator role to add managefilepermission

93a7b52

flyway script to add managefileperm to roles that have managedatasetperm

7541c04

add labels and flyway script name change

e7a5801

require manage file perm on file target

4111ee3

adjust set permissions menu

6736d37

check new manage file perm when revoking file download perm

0e15991

Merge remote-tracking branch 'IQSS/develop' into IQSS/8109-separate-m…

0d3183c

…anagefilepermissions

djbrooke assigned scolapasta Oct 21, 2021

qqmyers marked this pull request as ready for review October 26, 2021 14:03

qqmyers added the GDCC: DANS related to GDCC work for DANS label Oct 26, 2021

This was referenced Oct 26, 2021

As an admin, I'd like to be able to entirely delegate requests for access to depositors/owners #7252

Closed

dataset.xhtml - Edit button not rendered if user has only ManageDatasetPermissions #3981

Closed

scolapasta requested changes Oct 27, 2021

View reviewed changes

text changes per review comments

f9385ea

scolapasta assigned qqmyers Oct 28, 2021

qqmyers added 4 commits October 29, 2021 12:44

Move the new permission in the enum and rework flyway update script

42c32af

Merge remote-tracking branch 'IQSS/develop' into IQSS/8109-separate-m…

8646346

…anagefilepermissions

Change permission object to DataFile per review request.

a73fd49

typo

05f9055

scolapasta reviewed Nov 4, 2021

View reviewed changes

qqmyers added 2 commits November 4, 2021 15:31

singularize string name

d97f36a

more conformity

706fc17

scolapasta approved these changes Nov 4, 2021

View reviewed changes

djbrooke unassigned scolapasta and qqmyers Nov 4, 2021

djbrooke self-assigned this Nov 4, 2021

a possible release note addition

dc0b83e

djbrooke removed their assignment Nov 4, 2021

kcondon self-assigned this Nov 15, 2021

Merge remote-tracking branch 'IQSS/develop' into

97e0cc2

IQSS/8109-separate-managefilepermissions Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RevokeRoleCommand.java

kcondon assigned qqmyers and unassigned kcondon Nov 16, 2021

qqmyers added 2 commits November 18, 2021 15:42

fix npe, update flyway numbering, tweak error status/text

3054eac

moving up in the merge queue! - rename flyway script

6c609c4

kcondon merged commit 42d668a into IQSS:develop Nov 19, 2021

kcondon assigned kcondon and unassigned qqmyers Nov 19, 2021

djbrooke added this to the 5.9 milestone Nov 22, 2021

qqmyers mentioned this pull request Nov 22, 2021

IQSS/8252: Correct error response to 403/forbidden in test #8253

Merged

pdurbin mentioned this pull request Sep 30, 2022

Should we have a ManageTemplates permission? #1603

Closed

pdurbin mentioned this pull request May 21, 2024

Add a ManagePrivateURL permission #10574

Open

PaulBoon mentioned this pull request Jun 26, 2024

User responsible for handling access requests (with ManageFilePermissions) does not get notifications for this #10656

Closed

This was referenced Jul 23, 2025

Revert "feat: "Link Dataset/Dataverse" permission" #11682

Merged

feat: "Link Dataset/Dataverse" permission #11534

Merged

Iqss/8109 separate managefilepermissions #8174

Iqss/8109 separate managefilepermissions #8174

Uh oh!

Conversation

qqmyers commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djbrooke commented Oct 21, 2021

Uh oh!

qqmyers commented Oct 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scolapasta left a comment

Choose a reason for hiding this comment

Uh oh!

pdurbin commented Nov 4, 2021

Uh oh!

scolapasta commented Nov 4, 2021

Uh oh!

djbrooke commented Nov 4, 2021

Uh oh!

qqmyers commented Nov 4, 2021

Uh oh!

kcondon commented Nov 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

qqmyers commented Oct 21, 2021 •

edited

Loading

coveralls commented Oct 21, 2021 •

edited

Loading