-
Notifications
You must be signed in to change notification settings - Fork 535
Iqss/8109 separate managefilepermissions #8174
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
Merged
kcondon
merged 20 commits into
IQSS:develop
from
GlobalDataverseCommunityConsortium:IQSS/8109-separate-managefilepermissions
Nov 19, 2021
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
5da5652
add permission definition
qqmyers af2291a
update to use ManageFilePermission
qqmyers 93a7b52
update curator role to add managefilepermission
qqmyers 7541c04
flyway script to add managefileperm to roles that have managedatasetperm
qqmyers e7a5801
add labels and flyway script name change
qqmyers 4111ee3
require manage file perm on file target
qqmyers 6736d37
adjust set permissions menu
qqmyers 0e15991
check new manage file perm when revoking file download perm
qqmyers 0d3183c
Merge remote-tracking branch 'IQSS/develop' into IQSS/8109-separate-m…
qqmyers f9385ea
text changes per review comments
qqmyers 42c32af
Move the new permission in the enum and rework flyway update script
qqmyers 8646346
Merge remote-tracking branch 'IQSS/develop' into IQSS/8109-separate-m…
qqmyers a73fd49
Change permission object to DataFile per review request.
qqmyers 05f9055
typo
qqmyers d97f36a
singularize string name
qqmyers 706fc17
more conformity
qqmyers dc0b83e
a possible release note addition
qqmyers 97e0cc2
Merge remote-tracking branch 'IQSS/develop' into
qqmyers 3054eac
fix npe, update flyway numbering, tweak error status/text
qqmyers 6c609c4
moving up in the merge queue! - rename flyway script
qqmyers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## New ManageFilePermissions Permission | ||
|
|
||
| Dataverse can now support a use case in which a Admin or Curator would like to delegate the ability to grant access to restricted files to other users. This can be implemented by creating a custom role (e.g. DownloadApprover) that has the new ManageFilePermissions permission. This release introduces the new permission ( and adjusts the existing standard Admin and Curator roles so they continue to have the ability to grant file download requrests). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 changes: 25 additions & 0 deletions
25
src/main/resources/db/migration/V5.8.0.1__8109-add-manage-files-permission.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /* We're adding a new permission at bit 10 (512 ManageFilePermissions) that should be set for any role that has the permission | ||
| in bit 9 (256 ManageDatasetPermissions). | ||
| That means that any roles with current bits 10-13 (512+1024+2048+4096 = 7680) needs to have the corresponding bits 11-14 set | ||
| after the update, which can be accomplished by adding permissionbits & 7680 to the original value. (So a role with just bit 10 | ||
| set (512) would have 512&7680 (=512) added to it and become bit 11 (1024). The same logic also shifts any values in bits 11-13 | ||
| into bits 12-14. | ||
|
|
||
| In addition, any role wite permission for bit 9 set now should also have bit 10 set after the update | ||
| (any current role with ManageDatasetPermissions gets ManageDatasetPermissions and ManageFilePermissions after the update). | ||
| To do this we just add an addition bit 10 (512) if permissionbits&256!=0 | ||
|
|
||
| Finally, to make this idempotent, at least under the assumption that the standard admin role with all permissions | ||
| (or some role with current permission bit 13 set), we check to make sure no role has bit 14+ set - max(permissionsbits) <8192. | ||
| With the IF statement, running this script more than once will only cause an update on the first pass. | ||
|
|
||
| */ | ||
| DO | ||
| $do$ | ||
| BEGIN | ||
| IF (SELECT MAX(permissionbits) FROM dataverserole) < 8192 THEN | ||
| UPDATE dataverserole SET permissionbits=permissionbits + (permissionbits & 7680) WHERE (permissionbits & 256 =0); | ||
| UPDATE dataverserole SET permissionbits=permissionbits + (permissionbits & 7680) +512 WHERE (permissionbits & 256 !=0); | ||
| END IF; | ||
| END | ||
| $do$; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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)
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.
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.
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.
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.
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.
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?
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.
@qqmyers let's discuss briefly today and we can decide on the change or not and move along.