Bug 22868 - Circulation staff with suggestions_manage can have access to acquisition data
Summary: Circulation staff with suggestions_manage can have access to acquisition data
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical with 10 votes (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 22402 (view as bug list)
Depends on: 11911
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-08 15:26 UTC by Lisette Scheer
Modified: 2020-04-01 20:40 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03, 19.05.08, 18.11.14


Attachments
Bug 22868: Move suggestions_manage subperm out of acquisition perm (9.48 KB, patch)
2020-01-14 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22868: Move suggestions_manage subperm out of acquisition perm (10.43 KB, patch)
2020-01-21 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22868: Move suggestions_manage subperm out of acquisition perm (10.51 KB, patch)
2020-01-26 22:43 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22868: Move suggestions_manage subperm out of acquisition perm (10.54 KB, patch)
2020-01-29 22:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lisette Scheer 2019-05-08 15:26:53 UTC
It would be handy if we could have separate permissions for "Manage Purchase Suggestions" and "Enter Purchase Suggestions"

After bug 11911 was pushed, it is now obvious to people who have the "Manage Purchase Suggestions" permission that they can go in and edit any. 

It would be great if we could separate it out and let staff only add purchase suggestions. 

Many of our branches have staff enter in any purchase suggestions on the staff side using the "Purchase Suggestions" tab when checking out a patron, but don't want them to have the ability to go manage other ones.  12219 Can help some with that by limiting to branch, but if we could also have the ability to only add and not manage, that would be helpful here. 
Lisette
Comment 1 karen.fellows 2019-06-25 16:58:26 UTC
After our upgrade to 18.11 we noticed that staff were able to edit/delete purchase suggestions from the home page link.  In the past, staff were allowed staff to create purchase suggestions for patrons, but they could not edit/delete them.  I agree with Lisette, that it would be great to have a separate permission like "enter purchase sugggestions" for staff.
Comment 2 Christopher Brannon 2019-06-27 20:13:02 UTC
I would also like to say we definitely need to keep these separate.  This has been a little headache.
Comment 3 Christopher Brannon 2019-06-27 20:14:29 UTC
I want to change this to a bug, as I don't know that it was intended for this to happen.
Comment 4 Marie-Luce Laflamme 2020-01-10 20:05:23 UTC
Circulation staff do create purchase suggestions on patron's behalf regularly, and they do need the "managing purchase suggestions" permission to create these suggestions.

After upgrading to 19.05, we have noticed that if you allow the "managing purchase suggestions" permission (without other acquisition permissions), you can access the budget and see what was ordered. So, if you are a circulation staff, you can access the budget. This is not an ideal situation.

I do agree with the others, it would be great to have a separate permission called "enter purchase suggestions" for staff.
Comment 5 Christopher Brannon 2020-01-10 21:19:52 UTC
Oh my gosh!  This is not good at all!  We need this fixed ASAP.  I can only do so much to work around this without crippling our circulation staff.  Someone please fix.
Comment 6 Marie-Luce Laflamme 2020-01-10 22:35:38 UTC
Staff account with limited access issues.

For example: circulation staff will require the "managing purchase suggestion" permission to allow them to enter a purchase suggestion on a patron's behalf (on the staff interface). Unfortunately, this permission will also unlock the access to the budget.


Test plan

1) Create a staff account with the following permissions: "Checkout/checkin", "Add, modify and view patron info", "Staff access, allow viewing of catalogue..." and "Manage purchase suggestions" (without other acquisition permissions)

2) Logout and Login to the staff interface with this new account.
3) Access the Acquisition module and notice the budget
Comment 7 Jonathan Druart 2020-01-13 15:32:34 UTC
Can you confirm that the problematic information is on the Acquisition home page (acqui/acqui-home.pl) and is the fund list?
Comment 8 Jonathan Druart 2020-01-13 15:35:52 UTC
acqui/acqui-home.pl:        flagsrequired   => { acquisition => '*' },
acqui/booksellers.pl:        flagsrequired   => { acquisition => '*' },
acqui/histsearch.pl:        flagsrequired   => { acquisition => '*' },
acqui/invoice-files.pl:        flagsrequired   => { 'acquisition' => '*' },
acqui/invoice.pl:        flagsrequired   => { 'acquisition' => '*' },
acqui/invoices.pl:        flagsrequired   => { 'acquisition' => '*' },
acqui/modordernotes.pl:    flagsrequired   => { 'acquisition' => '*' },
acqui/ordered.pl:        flagsrequired   => { acquisition => '*' },
acqui/showorder.pl:    flagsrequired   => { acquisition => '*' },
acqui/spent.pl:        flagsrequired   => { acquisition => '*' },
acqui/supplier.pl:        flagsrequired   => { acquisition => '*' },
services/itemrecorddisplay.pl:    flagsrequired   => {acquisition => '*'},


I would suggest to move acquisition.suggestions_manage to a new permission "namespace", suggestions.suggestions_manage.
Would it work?
Comment 9 Marie-Luce Laflamme 2020-01-13 15:49:17 UTC
It would be a good thing to separate permissions:
1 - "Enter purchase suggestions"
2 - "Manage purchase suggestions"
Comment 10 Marie-Luce Laflamme 2020-01-13 15:50:21 UTC
Yes it's on (acqui/acqui-home.pl) page.

It would be a good thing to separate permissions:
1 - "Enter purchase suggestions"
2 - "Manage purchase suggestions"
Comment 11 Jonathan Druart 2020-01-13 16:05:45 UTC
(In reply to Marie-Luce Laflamme from comment #10)
> Yes it's on (acqui/acqui-home.pl) page.
> 
> It would be a good thing to separate permissions:
> 1 - "Enter purchase suggestions"
> 2 - "Manage purchase suggestions"

It will not fix the problem.
The issue here is that script are checking for acquisition => '*' permission, which means "whichever permission is ok for access".
Splitting the permission will not block the access to these pages.

That's why I suggestion to move the manage_suggestions out of the "acquisition". We could split the permission later if still needed.
Comment 12 Marie-Luce Laflamme 2020-01-13 16:35:01 UTC
It may not fix the problem, but we should definitely find a better way to handle purchase suggestions.

As mentioned earlier, circulation staff required the "managing purchase suggestions" permission to create purchase suggestions on a patron's behalf. 

However, you do not want circulation staff to decide if a purchase suggestion should be ordered nor access to the budget. Only librarians should manage suggestions.

There are 2 problems:

- access to the budget without permission
- the need of 2 separate permissions (" enter" vs "manage" purchase suggestions)
Comment 13 Jonathan Druart 2020-01-13 16:54:34 UTC
(In reply to Marie-Luce Laflamme from comment #12)
> It may not fix the problem, but we should definitely find a better way to
> handle purchase suggestions.
> 
> As mentioned earlier, circulation staff required the "managing purchase
> suggestions" permission to create purchase suggestions on a patron's behalf. 
> 
> However, you do not want circulation staff to decide if a purchase
> suggestion should be ordered nor access to the budget. Only librarians
> should manage suggestions.
> 
> There are 2 problems:
> 
> - access to the budget without permission
> - the need of 2 separate permissions (" enter" vs "manage" purchase
> suggestions)

Yes, I agree with the 2 permissions but it's an enhancement request, not a "critical" bug (also not a regression).
Comment 14 Christopher Brannon 2020-01-13 16:58:26 UTC
(In reply to Jonathan Druart from comment #13)

> 
> Yes, I agree with the 2 permissions but it's an enhancement request, not a
> "critical" bug (also not a regression).

Not sure why you would call this an enhancement.  This is a bug.  And I don't recall this behavior on past versions.  At some point this changed, and is not a desired effect.
Comment 15 Jonathan Druart 2020-01-13 17:05:00 UTC
(In reply to Christopher Brannon from comment #14)
> (In reply to Jonathan Druart from comment #13)
> 
> > 
> > Yes, I agree with the 2 permissions but it's an enhancement request, not a
> > "critical" bug (also not a regression).
> 
> Not sure why you would call this an enhancement.  This is a bug.  And I
> don't recall this behavior on past versions.  At some point this changed,
> and is not a desired effect.

I did not try but I think that prior to bug 11911, when you were able to create a new suggestion, you also were able to edit it. That is why we need 2 permissions. That must be fixed on a separated bug report, it's an enhancement request.

The "critical" bit here is that you can now access acquisition screens if you have the acquisition.manage_suggestions permission. That must be fixed here, it's a regression.
Comment 16 Katrin Fischer 2020-01-14 07:37:19 UTC
As I am the one to blame here I can say something about previous behaviour.
Before the new permission was added, there was 'no permission' needed for making suggestions and managing them in Koha. If you had the link to the suggestions page, you could do it. Koha was just hiding the link to the acquisition module from display.

So I introduced a new permission to remedy that... but that caused the problem we see now: we have other pages that require very lax permissions (any acq permission).

I think what Jonathan suggests, moving this out of acq will be a nice fix for the bug. It will go back to where we were before, but we will have a permission to turn it on and off.

Then the next step we should discuss how to split it up for editing etc.

Note: I am not a fan of splitting CRUD wise, because this means that you can enter a suggestion with a silly typo (add), but you won't be able to fix it (update) or delete it, if the patron no longer wants it.

I think it's better to think about use cases here. Like: allow to change status, allow to enter acq information or similar.
Comment 17 Katrin Fischer 2020-01-14 07:39:54 UTC
Note: moving it out of acq will also make it way easier to add those sub-permissions!
Comment 18 Jonathan Druart 2020-01-14 09:12:42 UTC
Created attachment 97334 [details] [review]
Bug 22868: Move suggestions_manage subperm out of acquisition perm

Bug 11911 replaced the permission of suggestions.pl (create a purchase
suggestion) from catalogue => 1 to acquisition => 'suggestions_manage'.
However we have a lot of acquisition scripts that have lax permissions
(acquisition => '*' which means any sub permissions of acquisition is
enough).

That causes problem when a circulation staff can create purchase
suggestions but not access acquisition information.

One solution is to move the suggestions_manage subpermission out of the
acquisition permission and create a new suggestion permission.

Test plan:
0. Setup
* Create a patron with several permission (and full acquisition
permission)
* Create another patron with several permission, and suggestions_manage
permission
* Create another patron without the suggestions_manage permission
1. Apply the patch and execute the update database entry
2. Note that the third patron you create still does not have
suggestions_manage
3. Confirm that you can create a purchase suggestion if you have
suggestions_manage, but cannot access acquisition pages if you do not
have any subpermissions of the acquisition permission
Comment 19 Katrin Fischer 2020-01-19 22:03:23 UTC
Marie-Luce or Christopher, could one of you sign-off on this? It should work on on the sandboxes.
Comment 20 Jonathan Druart 2020-01-21 13:43:55 UTC
Created attachment 97666 [details] [review]
Bug 22868: Move suggestions_manage subperm out of acquisition perm

Bug 11911 replaced the permission of suggestions.pl (create a purchase
suggestion) from catalogue => 1 to acquisition => 'suggestions_manage'.
However we have a lot of acquisition scripts that have lax permissions
(acquisition => '*' which means any sub permissions of acquisition is
enough).

That causes problem when a circulation staff can create purchase
suggestions but not access acquisition information.

One solution is to move the suggestions_manage subpermission out of the
acquisition permission and create a new suggestion permission.

Test plan:
0. Setup
* Create a patron with several permission (and full acquisition
permission)
* Create another patron with several permission, and suggestions_manage
permission
* Create another patron without the suggestions_manage permission
1. Apply the patch and execute the update database entry
2. Note that the third patron you create still does not have
suggestions_manage
3. Confirm that you can create a purchase suggestion if you have
suggestions_manage, but cannot access acquisition pages if you do not
have any subpermissions of the acquisition permission
Comment 21 Marie-Luce Laflamme 2020-01-21 18:18:58 UTC
I've tested the patch and here are my results...

- a patron with a super librarian permission can manage and add suggestion
- a patron without "suggestion management" permission cannot add suggestion



There is a tiny issue with the 3rd type of patron account...

- patron with several permissions (including "suggestion management") can add suggestion, but it also has the ability to "manage suggestions" (accept or reject suggestions) when you click on "Back to suggestion."


To reproduce:
Create a patron with "suggestion management" permission (see previous comment for details)
Apply the patch
Login with a patron who has the "suggestion management" permission
Find a patron account.
Click on "Purchase suggestion" and Add a suggestion.
After submitting the suggestion, click on the title of the suggestion to assess the request.
At the bottom of the suggestion form, click on "Back to suggestion"

After clicking on "Back to suggestion", you are sent to the "Suggestion Management" page (Acquisition > Suggestion), where you can accept or reject suggestions.
Comment 22 Jonathan Druart 2020-01-22 08:55:37 UTC
(In reply to Marie-Luce Laflamme from comment #21)
> There is a tiny issue with the 3rd type of patron account...
> 
> - patron with several permissions (including "suggestion management") can
> add suggestion, but it also has the ability to "manage suggestions" (accept
> or reject suggestions) when you click on "Back to suggestion."

Yes, it is the expected behavior.
With suggestions_manage you can manage suggestions :) Which means you can add/edit/delete purchase suggestions.

On this bug report we are dealing with acquisition access, a librarian with suggestions_manage but no acquisition permission cannot access budgets/funds information.
Comment 23 Christopher Brannon 2020-01-22 15:30:24 UTC
Personally, I think if a staff member can add a suggestion, they need to be able to edit or remove as well, but from the patron interface, not from the global table.  Less likely to affect the wrong patron.  That seems the most logical.  The global table needs its own permission.
Comment 24 karen.fellows 2020-01-22 15:41:10 UTC
I agree with Christopher and going off of my comment from above-- "In the past, staff were allowed staff to create purchase suggestions for patrons, but they could not edit/delete them."--We want this feature back.

Circ staff need to be able to enter the suggestions but not manage them--we have technical services staff that complete that task, not circulation staff.  The idea is to make it easier for the circ staff to help the patrons put in suggestions since most of the patrons don't log in to their accounts to do it themselves.
Comment 25 Katrin Fischer 2020-01-22 16:46:50 UTC
(In reply to karen.fellows from comment #24)
> I agree with Christopher and going off of my comment from above-- "In the
> past, staff were allowed staff to create purchase suggestions for patrons,
> but they could not edit/delete them."--We want this feature back.

I believe this is not true - the page was still accessible and staff could have added/edited anytime, just the acq module link would not show on the start page. We have used this 'loophole' for ages by linking to the page directly for libraries not using the acq module otherwise. There was NO permission check on the suggestions module page (apart from "catalogue").

I just tested this in a 17.11.:
- Create a user without any acq access
- Go to any patron account
- Create a new purchase suggestion
- Click on the "purchase suggestions" part of the breadcrumbs
- You can managed (edit/add) your heart out.

The regression is, that you can see other acq details, but not in the suggestions handling. 

> Circ staff need to be able to enter the suggestions but not manage them--we
> have technical services staff that complete that task, not circulation
> staff.  The idea is to make it easier for the circ staff to help the patrons
> put in suggestions since most of the patrons don't log in to their accounts
> to do it themselves.

I am not saying we could not make this more granular. The change here will even make that possible. By moving the suggestion permission to be a top-level permission, we will be able to have sub permissions as a next step. I am just saying, let's fix it to where we were before - then move forward from there.
Comment 26 Hayley Mapley 2020-01-26 22:43:54 UTC
Created attachment 97951 [details] [review]
Bug 22868: Move suggestions_manage subperm out of acquisition perm

Bug 11911 replaced the permission of suggestions.pl (create a purchase
suggestion) from catalogue => 1 to acquisition => 'suggestions_manage'.
However we have a lot of acquisition scripts that have lax permissions
(acquisition => '*' which means any sub permissions of acquisition is
enough).

That causes problem when a circulation staff can create purchase
suggestions but not access acquisition information.

One solution is to move the suggestions_manage subpermission out of the
acquisition permission and create a new suggestion permission.

Test plan:
0. Setup
* Create a patron with several permission (and full acquisition
permission)
* Create another patron with several permission, and suggestions_manage
permission
* Create another patron without the suggestions_manage permission
1. Apply the patch and execute the update database entry
2. Note that the third patron you create still does not have
suggestions_manage
3. Confirm that you can create a purchase suggestion if you have
suggestions_manage, but cannot access acquisition pages if you do not
have any subpermissions of the acquisition permission

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 27 Katrin Fischer 2020-01-29 22:36:48 UTC
Created attachment 98100 [details] [review]
Bug 22868: Move suggestions_manage subperm out of acquisition perm

Bug 11911 replaced the permission of suggestions.pl (create a purchase
suggestion) from catalogue => 1 to acquisition => 'suggestions_manage'.
However we have a lot of acquisition scripts that have lax permissions
(acquisition => '*' which means any sub permissions of acquisition is
enough).

That causes problem when a circulation staff can create purchase
suggestions but not access acquisition information.

One solution is to move the suggestions_manage subpermission out of the
acquisition permission and create a new suggestion permission.

Test plan:
0. Setup
* Create a patron with several permission (and full acquisition
permission)
* Create another patron with several permission, and suggestions_manage
permission
* Create another patron without the suggestions_manage permission
1. Apply the patch and execute the update database entry
2. Note that the third patron you create still does not have
suggestions_manage
3. Confirm that you can create a purchase suggestion if you have
suggestions_manage, but cannot access acquisition pages if you do not
have any subpermissions of the acquisition permission

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2020-01-29 22:40:36 UTC
This patch will fix the regression found (see comments before), for further improving/splitting permissions, it would be great if separate bug reports could be reported.

There are small glitches:
- The top permission has one sub permission now. It might have been slightly more consistent to have it only as a top level module permission, but seeing requests here for more granular permissions, this might see more work later.
- The breadcrumbs show Acquisitions, but when you click on it, you got no permission. I am not seeing this as a blocker, as this is what we had before the regression was introduced. So I think we can think about this separately.
Comment 29 Martin Renvoize 2020-01-30 09:40:59 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 30 Joy Nelson 2020-02-10 23:15:50 UTC
Pushed to 19.11.x branch for 19.11.03
Comment 31 Lucas Gass 2020-02-14 20:28:58 UTC
backported to 19.05.x for 19.05.08
Comment 32 Hayley Mapley 2020-02-17 22:29:36 UTC
Backported to 18.11.x for 18.11.14.
Comment 33 Katrin Fischer 2020-04-01 20:40:04 UTC
*** Bug 22402 has been marked as a duplicate of this bug. ***