-
Notifications
You must be signed in to change notification settings - Fork 490
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
Sword Auth: use Dataverse 4 permission model #1070 #2495 #784 #3137
Conversation
We are switching away from the old "admin only" permission checks we were forced to used in the DVN 3.x days when the permission system was less flexible. Now the rule is that if the permission system doesn't prevent you from performing an operation (create dataset, upload file) SWORD won't prevent you either. Two SWORD operations had to be rewritten to support Dataverse 4 permissions. First, retrieval of the Service Document with permissionService.getDataversesUserHasPermissionOn has been changed to pay attention to a user's groups. It's very common for the ":authenticated-users" group to be given a role that should allow SWORD operations, for example. Second, the "list datasets in a dataverse" operation (listCollectionContents) was rewritten to allow anyone with `AddDataset` permission to be able to execute the command to they can see if a dataverse has been published or not but the only datasets that are returned are those for which the user can issue `UpdateDatasetCommand`. In the root dataverse, for example, the user should only see their datasets.
I just wanted to mention that @axfelix and I chatted a bit about OJS testing at http://irclog.iq.harvard.edu/dataverse/2016-05-24#i_35578 For OSF, conversation continues at CenterForOpenScience/osf.io#5344 |
Got this in Apache logs from a stock OJS 2.4.8 instance hooked into dev1.dataverse.org when trying to deposit. Will drill down a little later unless you have any immediate ideas:
|
@axfelix uh oh, line 666 even. I'm assuming you got a 500 response? I'll past below the stack trace I'm seeing (line numbers for 94f57b0). The Let me knock on some doors around here and see what may be going on. @axfelix meanwhile if you could repeat your test for https://demo.dataverse.org (which is running v. 4.3.1 build 94-be5b26e) I'd appreciate it. I'm also sort of wondering if it's a transient error since my test suite since just completed without error though it did seem to hang for many seconds when creating datasets. So if you could run your test against https://dev1.dataverse.org a second time I'm appreciate it. If you ping me at http://chat.dataverse.org I can be tailing the logs while you're running your tests.
|
Indeed, I couldn't duplicate! https://dev1.dataverse.org/dataset.xhtml?persistentId=doi:10.5072/FK2/UZAIBX&version=DRAFT Worked fine the second time (with an .md file rather than .py, if it matters, though I'd expect not). |
@axfelix thanks for re-testing. At https://ezidstatus.wordpress.com/feed/ I'm seeing the following...
... so I'm guessing that's why you got that error. That feed and https://ezidstatus.wordpress.com are linked to from http://ezid.cdlib.org/home/documentation (thank you @sekmiller for the pro tip). |
@scolapasta talked about the code as of 96d9ac4 and decided: |
hasAccessToModifyDataverse was hard-coded to true since we have added other checks to be compatible with Dataverse 4 permissions. Removing this method is a no-op but it looks like a massive change, unfortunately, because of the changes in nesting.
@scolapasta I'm passing this back to you because I made the two changes you definitely wanted and played around with the third that we talked about. Changed in this pull request:
Played around with this in a new branch (not in this pull request):
|
@kcondon has been testing this pull request and noticed that groups within groups are not honored by the SWORD Service Document. This is a known issue with MyData as well (#3056) so we agreed to list this as a known issue with SWORD, which I did in af47f90. While I was on this branch I went ahead and merged the latest from develop (bd50ab7) but then the app no longer compiled! I fixed this (got the app compiling again) in aa2d7ce. This was just in a test class (UtilIT.java) so no big deal. |
@scolapasta @pdurbin Ready for merge. |
RFI Checklist
We are switching away from the old "admin only" permission checks we were forced to used in the DVN 3.x days when the permission system was less flexible. Now the rule is that if the permission system doesn't prevent you from performing an operation (create dataset, upload file, etc.) SWORD won't prevent you either.
Two SWORD operations had to be rewritten in this pull request to support Dataverse 4 permissions.
First, retrieval of the Service Document with permissionService.getDataversesUserHasPermissionOn has been changed to pay attention to a user's groups. It's very common for the ":authenticated-users" group to be given a role that should allow SWORD operations, for example. I'm a bit concerned about the performance impact of this change, especially with production data, which I haven't tested with. I'm putting #784 thought QA and I'd like to remind us that we the reason we lost support for groups in our performance-related fix for #2012. I basically iterated on that fix, bringing group support back in what I hope is a performant way.
Second, the "list datasets in a dataverse" operation (listCollectionContents) was rewritten to allow anyone with
AddDataset
permission to be able to execute the command to they can see if a dataverse has been published or not but the only datasets that are returned are those for which the user can issueUpdateDatasetCommand
. In the root dataverse, for example, the user should only see their datasets. This is potentially a n expensive operation in a dataverse with many datasets, such as the root.The other SWORD operations received no change to their business logic but Dataverse 4 style permission checks were added and the old "admin only" check was removed.
I updated the SWORD API Guide to explain the necessary permissions for each operation. You can see the diff at the top of 94f57b0
I also noted in the Shibboleth section of the Installation Guide that Shib groups are not supported via SWORD: 94f57b0.
1. Related Issues
2. Pull Request Checklist
3. Review Checklist
After the pull request has been submitted, fill out this section.
We are switching away from the old "admin only" permission checks we
were forced to used in the DVN 3.x days when the permission system was
less flexible. Now the rule is that if the permission system doesn't
prevent you from performing an operation (create dataset, upload file)
SWORD won't prevent you either.
Two SWORD operations had to be rewritten to support Dataverse 4
permissions.
First, retrieval of the Service Document with
permissionService.getDataversesUserHasPermissionOn has been changed to
pay attention to a user's groups. It's very common for the
":authenticated-users" group to be given a role that should allow SWORD
operations, for example.
Second, the "list datasets in a dataverse" operation
(listCollectionContents) was rewritten to allow anyone with
AddDataset
permission to be able to execute the command to they can see if a
dataverse has been published or not but the only datasets that are
returned are those for which the user can issue
UpdateDatasetCommand
.In the root dataverse, for example, the user should only see their
datasets.