| Summary: | Missing permission check for "record batch delete" from reports | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
| Component: | Reports | Assignee: | Brendan Lawlor <blawlor> |
| Status: | Failed QA --- | QA Contact: | Marcel de Rooy <m.de.rooy> |
| Severity: | minor | ||
| Priority: | P5 - low | CC: | blawlor, m.de.rooy, ovezina |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Small patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 35905: Add permission checks to report results
Bug 35905: Add permission checks to report results Bug 35905: Add permission checks to report results Bug 35905: Add permission checks to report results Bug 35905: (follow-up) Add permission checks to report results Bug 35905: (follow-up) Add permission checks to report reuslts Bug 35905: Add permission checks to report results Bug 35905: Add permission checks to report results |
||
|
Description
Katrin Fischer
2024-01-25 10:25:08 UTC
Created attachment 169260 [details] [review] Bug 35905: Add permission checks to report results Test plan: 1. Apply patch 2. Create a report like: select biblionumber from biblio limit 10 2. Run the report 3. Verify batch operations dropdown contains batch modification and deletion 4. Run the report again as a user that has only catalogue and reports permissions 5. Confirm the batch operations drop down only includes Add to list 6. Confirm the records drop down only includes View record Created attachment 169263 [details] [review] Bug 35905: Add permission checks to report results Test plan: 1. Apply patch 2. Create a report like: select biblionumber from biblio limit 10 2. Run the report 3. Verify batch operations dropdown contains batch modification and deletion 4. Run the report again as a user that has only catalogue and reports permissions 5. Confirm the batch operations drop down only includes Add to list 6. Confirm the records drop down only includes View record Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 170440 [details] [review] Bug 35905: Add permission checks to report results Test plan: 1. Apply patch 2. Create a report like: select biblionumber from biblio limit 10 2. Run the report 3. Verify batch operations dropdown contains batch modification and deletion 4. Run the report again as a user that has only catalogue and reports permissions 5. Confirm the batch operations drop down only includes Add to list 6. Confirm the records drop down only includes View record Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> If I read this correctly, this seems not quite right:
[% IF ( CAN_user_editcatalogue_edit_catalogue || CAN_user_editcatalogue_edit_items ) %]
For "Edit record".
I believe the edit_items is not right here. The page seems only be accessible with either fast_cataloging or edit_catalogue. I think edit_catalogue is the one here.
Can you please revise?
(In reply to Katrin Fischer from comment #4) > If I read this correctly, this seems not quite right: > > [% IF ( CAN_user_editcatalogue_edit_catalogue || > CAN_user_editcatalogue_edit_items ) %] > > For "Edit record". > > I believe the edit_items is not right here. The page seems only be > accessible with either fast_cataloging or edit_catalogue. I think > edit_catalogue is the one here. > > Can you please revise? Thanks Katrin! You are right there is something off here. I think it may need some more logic to handle all the different links that are automatically rendered in report results. I added edit_items here for reports that have the itemnumber in them. Item numbers link to /cataloguing/additem.pl?op=edititem&itemnumber=1 where i think edit_items is maybe the least permission needed to access that page, but I'm not sure if that's exactly correct either. I just realized that the same issue exists for reports with brrowernumber or cardnumber in them too. It's been a while since I looked at this code in detail, but I will give it another shot. What we could also consider: Fixing the batch link here and moving the other permission checks to another report if that makes things easier. Created attachment 170634 [details] [review] Bug 35905: Add permission checks to report results Test plan: 1. Apply patch 2. Create a report like: select biblionumber from biblio limit 10 2. Run the report 3. Verify batch operations dropdown contains batch modification and deletion 4. Run the report again as a user that has only catalogue and reports permissions 5. Confirm the batch operations drop down only includes Add to list 6. Confirm the records drop down only includes View record Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170635 [details] [review] Bug 35905: (follow-up) Add permission checks to report results Created attachment 170636 [details] [review] Bug 35905: (follow-up) Add permission checks to report reuslts To test: 1. Create the following two reports a) select borrowernumber, cardnumber from borrowers limit 100 b) select itemnumber, biblionumber from items limit 100 2. Create a user with only catalogue and reports permissions 3. Run each report as the user with minimal permissions 3. Verify the batch operations button is disabled 4. Verify there is no link to edit patron in borrowernumber columns 5. Verify there is no link to edit record in itemnumber and biblionumber columns 6. Add the following permissions one at a time and re run the reports tools_items_batchmod tools_items_batchdel editcatalogue_edit_catalogue editcatalogue_edit_items tools_edit_patrons borrowers_edit_borrowers 7. Verify the batch operations button is enabled and displays the correct links 8. Verify the links to edit patron and edit record are displayed correctly This update fixes the permission checks for all batch operations and edit links. I think technically a user could still hit a unauthorized page if they don't have permissions to view patron records or to circulate, but that's probably less of an issue. Created attachment 171105 [details] [review] Bug 35905: Add permission checks to report results There are some links on the report results page that the user may not have access to This patch adds permission checks to the report results page to: 1. Disable the batch operations button or remove its dropdown items 2. Remove automatic links from the itemnumber, biblionumber, borrowernumber, and cardnumber columns 3. Remove the 'Hide/Show data menus' button When the user doen't have permission to those actions To test: 1. Create the following two reports a) select borrowernumber, cardnumber from borrowers limit 100 b) select itemnumber, biblionumber from items limit 100 2. Create a user with only catalogue and reports permissions 3. Run each report as the user with minimal permissions 3. Verify the batch operations button is disabled 4. Verify there is no link to edit, view or checkout patron in borrowernumber columns 5. Verify there is no link to edit record in itemnumber and biblionumber columns 6. Add the following permissions one at a time and re run the reports tools_items_batchmod tools_items_batchdel tools_records_batchmod tools_records_batchdel tools_edit_patrons editcatalogue_edit_catalogue editcatalogue_edit_items borrowers_edit_borrowers 7. Verify the batch operations button is enabled and displays the correct links 8. Verify the links in borrowernumber and cardnumber columns to view, edit and checkout patron are displayed correctly 9. Verify the links in itemnumber and biblionumber columns to edit record are displayed correctly I ended up adding a checks circulate_remaining_permissions so I believe this now covers all links on the reports results page that a user might not have permission to access. But in the test plan I forgot to mention testing circulate_remaining_permissions for the checkout links In the batch operations button, i am still getting permission issues with the Patron card creation. If i don't give the label creation permission and click on the button, i am redirected to a login page saying i don't have the permission to access the page. (In reply to Olivier V from comment #13) > In the batch operations button, i am still getting permission issues with > the Patron card creation. If i don't give the label creation permission and > click on the button, i am redirected to a login page saying i don't have the > permission to access the page. On first glance that seems the correct bahvior. Patron cards use the label creator permission. (In reply to Katrin Fischer from comment #14) > (In reply to Olivier V from comment #13) > > In the batch operations button, i am still getting permission issues with > > the Patron card creation. If i don't give the label creation permission and > > click on the button, i am redirected to a login page saying i don't have the > > permission to access the page. > > On first glance that seems the correct bahvior. Patron cards use the label > creator permission. OH! I see what you mean, but I think that's outside of scope here and should go on a separate bug :) For the patron card creation, I wasn't sure what the correct permission was. When I was testing I think it worked with the permission tools_edit_patrons. If the least permission for that is actually tools_label_creator I can update the patch for that. Created attachment 171492 [details] [review] Bug 35905: Add permission checks to report results There are some links on the report results page that the user may not have access to This patch adds permission checks to the report results page to: 1. Disable the batch operations button or remove its dropdown items 2. Remove automatic links from the itemnumber, biblionumber, borrowernumber, and cardnumber columns 3. Remove the 'Hide/Show data menus' button When the user doen't have permission to those actions To test: 1. Create the following two reports a) select borrowernumber, cardnumber from borrowers limit 100 b) select itemnumber, biblionumber from items limit 100 2. Create a user with only catalogue and reports permissions 3. Run each report as the user with minimal permissions 3. Verify the batch operations button is disabled 4. Verify there is no link to edit, view or checkout patron in borrowernumber columns 5. Verify there is no link to edit record in itemnumber and biblionumber columns 6. Add the following permissions one at a time and re run the reports tools_items_batchmod tools_items_batchdel tools_records_batchmod tools_records_batchdel tools_edit_patrons editcatalogue_edit_catalogue editcatalogue_edit_items borrowers_edit_borrowers 7. Verify the batch operations button is enabled and displays the correct links 8. Verify the links in borrowernumber and cardnumber columns to view, edit and checkout patron are displayed correctly 9. Verify the links in itemnumber and biblionumber columns to edit record are displayed correctly Signed-off-by: Olivier V <olivier.vezina@inLibro.com> + [% IF ( batch_biblionumbers || batch_itemnumbers ) || ( batch_cardnumbers || batch_borrowernumbers && (CAN_user_borrowers_list_borrowers || CAN_user_borrowers_edit_borrowers || CAN_user_circulate_circulate_remaining_permissions ) ) %] Please explain this change. Why didnt we have batch_borrowernumbers before? Are you missing parentheses around batch_card OR batch_borrower ? - [% ELSIF header_row.$place.cell == 'cardnumber' || header_types.item(header_row.$place.cell) == 'cardnumber' %] + [% ELSIF CAN_user_circulate_circulate_remaining_permissions && header_row.$place.cell == 'cardnumber' || header_types.item(header_row.$place.cell) == 'cardnumber' %] Are we missing parentheses here ? |