Fix user authorization issues with admin sidebar #8607
Conversation
… specific collection
Analogous to SolrServiceIndexCollectionSubmittersPlugin, but for Items instead of Collections, and for WRITE rights instead of ADD rights. Also refactored SolrServiceIndexCollectionSubmittersPlugin to share code with the new plugin.
…ich the current user has editing rights
ybnd
added
bug
high priority
authorization
KoenP
changed the title
There was a problem hiding this comment.
Thanks @KoenP ! This looks great overall. Code looks good and tests look good too. I also manually tested this, and it's working well. It fixes the main bug related to the new/edit menus. I tested these menus as a full Admin, Community Admin, Collection Admin, Submitter and a user with no rights. The menu options look correct now.
(As a sidenote, I've realized there are still other authorization issues outstanding, but those should be left for later PRs... e.g. Community Admins can attempt to create a new Community anywhere, and Collection Admins can attempt to edit any Collection.)
I have a few minor suggestions inline below...mostly cleanup of comments. But, I'm basically +1 this work, as it seems to fix the reported bug.
NOTE to other reviewers: You need to do a full reindex (./dspace index-discover -b) for these new auth checks to work properly!
dspace-api/src/main/java/org/dspace/discovery/IndexingUtils.java
Outdated
Show resolved
Hide resolved
| // Index groups with ADMIN rights on the Collection, on | ||
| // Communities containing those Collections, and recursively on any Community containing such a | ||
| // Community. | ||
| // TODO: Strictly speaking we should also check for epersons who received admin rights directly, |
There was a problem hiding this comment.
Is this TODO still needed? It looks like we are already checking for EPersons who have received admin rights directly in the below code where we call findDirectlyAuthorizedGroupAndEPersonPrefixedIds().
There was a problem hiding this comment.
This TODO is still relevant and I don't think we should remove it. It is related to the "Known bug" section of the PR.
What the method does now is:
- Collect admin groups for the collection and for communities (transitively) containing the currently considered collection.
- Collect epersons and groups with admin or edit rights by resource policy on the collection.
However, what we currently don't do is collecting epersons or groups with admin rights by resource policy on communities (transitively) containing the collection.
There was a problem hiding this comment.
is there a reason why this gap is not fixed right now? I mean the solution for the collection can easily be replicated for the community, right?
dspace-api/src/main/java/org/dspace/discovery/SolrServiceIndexItemEditorsPlugin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@KoenP : Thanks for the updates! The updates & your answers (to my other questions) all make sense. I gave this another look today, and realized that I missed a small detail in my earlier review. It looks like we're accidentally missing tests for the new ItemService methods. If those could be added, I'd appreciate it.
Again, this doesn't change the functionality. This feature works, so I'm basically a +1. But, tests are required for new methods (unless there's a reason they are overly complex to create). Thanks!
There was a problem hiding this comment.
👍 Thanks @KoenP for the additional tests! This now looks good to me & works as described.
@atarix83 : If you can find a chance to test this alongside DSpace/dspace-angular#1999, I'd appreciate it. I think these two PRs are ready to merge, and they are required for additional permission fixes to the sidebar menu.
|
@atarix83 or @LucaGiamminonni : Are you still planning to give a quick review of this (and the corresponding DSpace/dspace-angular#1999) ? These PRs are required for others to also be merged (namely #8616 and DSpace/dspace-angular#2011)... and we are starting to build a backlog of PRs that depend on these. If you could get to these reviews soon (in the next few days), I'd appreciate it. |
abollini
requested review from
abollini
and removed request for
LucaGiamminonni
| // Index groups with ADMIN rights on the Collection, on | ||
| // Communities containing those Collections, and recursively on any Community containing such a | ||
| // Community. | ||
| // TODO: Strictly speaking we should also check for epersons who received admin rights directly, |
There was a problem hiding this comment.
is there a reason why this gap is not fixed right now? I mean the solution for the collection can easily be replicated for the community, right?
| @@ -264,6 +264,9 @@ | |||
| <!-- used to track which group(s) have submit permissions --> | |||
| <field name="submit" type="string" indexed="true" stored="true" omitNorms="true" multiValued="true" docValues="true" /> | |||
|
|
|||
| <!-- used to track which eperson(s) have edit permissions on items --> | |||
There was a problem hiding this comment.
it should be eperson(s) and/or group(s)
|
Merging as this PR & the corresponding frontend are both now at +2. Thanks again @KoenP ! |
References
Description
Instructions for Reviewers
In order to decide whether or not to display the "New->Item" and "Edit->Item" options in the admin sidebar, the frontend needs to know (respectively):
On the backend, this patch adds two new authorization features: "canSubmit" and "canEditItem". On the frontend, the menu resolver now requests these permissions to determine whether to add the corresponding sections to the menu.
Backend overview
Checking directly whether a user has, for instance, edit rights on at least one item is a fairly expensive operation to perform directly. The user can obtain these rights in any one of the following ways:
We don't want to hammer the DB with running the full exhaustive check every time the frontend needs this information (which is every time a user logs in), so we add this information to the index instead.
The patch introduces a new indexer plugin "SolrServiceIndexItemEditorsPlugin", and it also refactors the existing indexer plugin "SolrServiceIndexCollectionSubmittersPlugin". This means the check described above (mostly -- see Known Issues) is run on every index update instead of every time a user logs in.
Known Issues
Integration test
The patch fails
CreateMissingIdentifiersIT.testPerformin CI but not locally. However this is known to be a flaky test and seems to have been fixed here.Performance
The current implementation is not as fast as it could be. For one, the two plugins are run for every item (for the canEditItem plugin) and every collection (for the canSubmit plugin), even though in both cases only a single item/collection needs to be found.
In particular, this makes recursive discovery of an ancestor community/collection for which the user has ADMIN rights less efficient. One can picture this process as a tree where the ADMIN rights "trickle down" to all descendants. The current implementation, however, computes bottom-up, starting from every collection (for the canSubmit plugin) or every item (for the canEditItem plugin), thereby potentially repeatedly exploring the same paths over and over. What's more is that, not only do we run a separate SQL query for every such item/collection involved, but every step of the recursive upward search process requires a separate DB query.
Known bug
The new item editing plugin was modeled on the existing collection submitters plugin, and it replicates a bug in that plugin: when recursively traversing ancestor collections/communities, it only collects the ADMIN groups that are directly registered as "the admin group" of the collections/communities, ignoring resource policies (either for groups or directly for epersons). This behavior was retained out of performance concerns with searching for research policies for every ancestor in the tree (often multiple times, as explained in the Performance subsection).
Proposed solutions
The known bug is easy to fix if the performance of the plugins is improved.
At the beginning of an index update, it should be possible to perform a single, relatively cheap SQL query to recursively trace out admin permissions (that is, yield a table
(group_or_eperson_id, dso_id)and to put the result in an in-memory hash table which lives for the duration of the index update). This avoids duplicate work (because it is performed once for the entire index update, rather than for each updated object in the index) and should be relatively cheap (because in real workloads, we’d expect admin permissions to be rare compared to the number of items/collections).Implementing these changes would require some rearchitecting; we recommend accepting this patch in the mean time.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.