Review Submissions panel shows more works than it should

[julesies]

Descriptive summary
reported by @cpd3149 - moved from samvera-labs/nurax-pre2023#99

As a non-administrator user with manager participation for an admin set(s), I should be able to review submissions from that(those) admin set(s). Instead, I see way more works from other admin sets in the Review Submissions panel.

Evidence

These are the admin sets I manage, containing 13 works.

https://user-images.githubusercontent.com/24395592/31195535-44988ba4-a910-11e7-8ce4-afe0a3b35db6.png

These are the works I can review:

https://user-images.githubusercontent.com/24395592/31195551-52043f40-a910-11e7-8363-6e3181a70334.png

I can see even more that have been published:

https://user-images.githubusercontent.com/24395592/31195570-62c6c78a-a910-11e7-9c78-7254f45b01d1.png

Expected behavior

Managers should only be able to review works in admin sets they manage.

Actual behavior

Managers see more works than they have access to.

Steps to reproduce the behavior

Become a manager of an admin set (with 1-step workflow)
Go to Dashboard -> Review Submissions
Are those works in admin sets you manage, or are there more there?

[julesies]

this happens with managers assigned to admin sets with 1-step workflow, probably need to test for other admin sets.

[mjgiarlo]

@julesies @vantuyls We need to assign a milestone and labels to this. (Blocker-worthy for 2.0.0?)

[chrisdaaz]

this could be confusing for non-administrator managers of admin sets (i.e. a department assistant reviewed masters theses). when they click on "review submissions" from their dashboard, they see a bunch of works and have no way of knowing which ones they can act on.

[vantuyls]

i'm concerned that this issue is related to a more general lack of understanding that we have about how roles and permissions work. Is this expected behavior? Was this designed to function this way, or is this a bug?

[julesies]

I really don't remember it behaving this way when we tested before...if it's supposed to work this way that's a real concern IMO. This exposes content to people who shouldn't see it - so they can edit it.

I just edited this as a manager with no permissions to the admin set: https://nurax.curationexperts.com/concern/generic_works/tt44pm87g?locale=en

We didn't have this step explicit in the testing before though...

[mjgiarlo]

2.0.0 blocker or no?

[vantuyls]

i'd like us to dig a little on this with a bit more testing. can @julesies and @cpd3149 and I talk about this tomorrow some time on slack?

[mjgiarlo]

@vantuyls 👍

[jonathandixon]

The query for the "Review Submissions" page selects any work that allows action by any workflow role the current user has. There are a couple of issues with this approach:

1. Any works added to the admin set before the user was granted manage permissions will show up in the query result. There is no check that the user has the required permissions on the individual works. Changes to admin set permissions are not applied to existing works.

2. If the same workflow is used on multiple admin sets and a user is given manage permissions on one of those admin sets, then the review submissions page will show works in any of the admin sets.

[julesies]

@jonathandixon thanks for this explanation. I think this helps explain some other bugs we were seeing...as we were using the same workflow on multiple admin sets. I'm guessing reusing workflows is not something we want to recommend to people who need to implemented review with multiple departments. @vantuyls @mjgiarlo So the recommendation to repo-managers would be at this point, to create a workflow for each admin set - it is actually the same workflow steps, but named to match the admin set (Right?). This is the way around giving managers with a workflow role too much power and to prevent them from seeing more than they should. @cpd3149 and I just re-tested a lot of these others so we can close some.

[mjgiarlo]

I believe the current setup is that workflows are not shared between AdminSets -- this was a big gnarly problem with the initial workflow buildout. If AS1 uses mediated deposit workflow and AS2 does, those are created as separate entities in the database and should be treated separately.

[julesies]

So is this a legit bug and not working as expected? @mjgiarlo?

[chrisdaaz]

can the Review Submissions query be changed to select and display only works that reside in the admin sets the user manages or owns (rather than selecting by workflow type)?

say i have two departments using the same workflow to review theses. i can train them to ignore the "review submissions" page because it won't be helpful to them and instruct them to use the admin set view to approve or request changes on work submissions. if this isn't fixed for 2.0, i'll probably just hide the "review submissions" link from the dashboard.

[vantuyls]

the current setup is that you can do whatever you want, i think there is a misalignment in the functionality of workflows and permissions that we've never tested (well enough) or specced out properly (i'll take 60% of the blame on that).

I think, for this reason, this is working as designed but not as expected. Maybe @samvera/hyrax-repo-managers can advise as to whether this is a manageable issue (a la what @cpd3149 suggested) until a more thorough solution can be determined?

[mjgiarlo]

@julesies

If the same workflow is used on multiple admin sets and a user is given manage permissions on one of those admin sets, then the review submissions page will show works in any of the admin sets. -- @jonathandixon

☝️ sounds like a bug to me, yes. As to whether it's a blocker... I dig @vantuyls's idea of getting a sense from @samvera/hyrax-repo-managers as to what they would find acceptable in 2.0.0, with the understanding that we could prioritize a fix for this in 2.0.1, which could drop a few weeks after 2.0.0. I'd prefer that to adding more 2.0.0 blockers, just so that we can try to get in the rhythm of smaller releases cut more frequently (this is/has been my preference), but if folks feel strongly that this should block 2.0.0, so be it!

[jonathandixon]

@mjgiarlo That's right the workflows are not technically shared, as there are unique entries in the database for each admin set and workflow combination. But the name of the workflow remains the same for each entry as do the workflow role names.

The problem lies with how the actionable_workflow_roles are stored on the work and the query the StatusListService uses. This is done by concatenating the workflow name with the workflow role name (see https://github.com/samvera/hyrax/blob/master/app/services/hyrax/workflow/status_list_service.rb#L53). In the case where multiple admin sets use the same workflow definition, this string is the same for all.

[mjgiarlo]

@jonathandixon Ah, good eye! What if we replaced that line with something like "#{wf.permission_template.admin_set_id}-#{wf.name}-#{wf_role.role.name}" to disambiguate the different workflows?

[jonathandixon]

@mjgiarlo You could update that line, but you would also have to update Hyrax::IndexesWorkflow to do the same. And this change would require reindexing.

[mjgiarlo]

@jonathandixon Yep, working on that PR right now. Will add the reindexing to the 2.0.0 release notes if merged.

[mjgiarlo]

@jonathandixon #1779

[jonathandixon]

@mjgiarlo It's worth mentioning that the PR doesn't fully address the issue. If a user is added as a manager after works have already been added to the workflow, then they will see those works on the Review Submissions page, but they will not be able to edit or approve them (maybe that's a separate issue).

[mjgiarlo]

@jonathandixon 👍 to separate issue for that, which perhaps is less of a 2.0.0 blocker (though needs writing up and schedling, perhaps for 2.x series). 👈 @vantuyls @julesies ?

[julesies]

one thing i'm still unclear on is this with this fix...is this how you expect it to work now? Managers of work-flowed admin sets will see the works added to the admin set before the manager became a manager, but only in Review Submissions. They should still not have view or edit access to those added before they were granted manager status. (It's really just showing them works that they can't take action on?).

[mjgiarlo]

Yes, I think that is correct, @julesies. If we want to ticket exploring this further in 2.x so we don't mislead users, I think that is also totally reasonable!