Bug 7996

Summary: Patron modification log requires parameters permission
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: PatronsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: chris, fiona.borthwick, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, lisettepalouse+koha, martin.renvoize, rochellehealy.student, severine.queune
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: without parameters permission
with parameters permission
Bug 7996: Patron Modification Log Requires Parameters Permission
Bug 7996: Correct wrong permissions for modification log
Bug 7996: Correct wrong permissions for modification log
Bug 7996: Correct wrong permissions for modification log
Bug 7996: Correct wrong permissions for modification log
Bug 7996: (QA follow-up) Remove unnecessary permission check view logs template

Description Nicole C. Engard 2012-04-23 12:41:18 UTC
Created attachment 9274 [details]
without parameters permission

See attached.

The "Modification Log" tab does not appear while viewing a patron's record
(Detail, Check Out) unless the staff member has the "Set Koha system
parameters" priviledge enabled.
Comment 1 Nicole C. Engard 2012-04-23 12:41:53 UTC
Created attachment 9275 [details]
with parameters permission
Comment 2 Katrin Fischer 2015-01-06 14:24:35 UTC
Maybe this would be a good one to clean up for the academy too. As far I can see there are 4 files linking to the modification logs:

koha-tmpl/intranet-tmpl/prog/en/includes/biblio-view-menu.inc	
33 [% IF ( CAN_user_tools_view_system_logs ) %][% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&amp;modules=CATALOGUING&amp;action=MODIFY&amp;object=[% biblionumber %]">Modification log</a> </li>[% END %]

koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc	
77         [% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&amp;modules=MEMBERS&amp;modules=circulation&amp;object=[% borrowernumber %]&amp;src=circ">Modification log</a></li>

koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.tt	
77     [% IF ( CAN_user_parameters ) %][% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&amp;modules=MEMBERS&amp;modules=circulation&amp;object=[% borrower.borrowernumber %]&amp;src=circ">Modification log</a></li>[% END %]

koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc	
13         [% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&amp;modules=MEMBERS&amp;action=MODIFY&amp;object=[% borrowernumber %]">Modification log</a></li>

Only the first seems to be using the correct permission check.
Comment 3 Rochelle Healy 2015-01-14 22:06:20 UTC
Created attachment 35262 [details] [review]
Bug 7996: Patron Modification Log Requires Parameters Permission

To test
1- Open 'set permissions' on patron page and uncheck '(parameters) Set koha system parameters'
2- Open patron page and notice that there is no 'modification log' tab
3- Apply patch
4- Refresh page and notice 'modification log' tab is there
Comment 4 Katrin Fischer 2015-01-19 06:56:31 UTC
Hi Rochelle,

the way it works after your patch, it will need both the permission 'parameters' and 'view_system_logs'. I think what we need to do here is take a look at all the conditions [IF ...] that apply to show the link and check which are really needed / make sense. 

My feeling is that it would make sense to use 'view_system_logs' everywhere, as this is the more specific permission. What do others think?
Comment 5 Nick Clemens 2018-08-17 11:11:54 UTC
Created attachment 77974 [details] [review]
Bug 7996: Correct wrong permissions for modification log

To test:
 1 - Create a staff patron with catalogue,borrowers,and view_system_logs
permissions
 2 - Log in using that patron
 3 - Go to tools
 4 - Click 'Log viewer'
 5 - You get a blank page
 6 - View borrower circ screen, note you do not see 'Modification log'
 7 - Edit a borrower, note you do not see 'Modification log'
 8 - Apply patch
 9 - 'Log Viewer' should now load correctly
10 - You should see 'Modification log' tab during circ
11 - You should see 'Modification log' tab when editing patron
Comment 6 Owen Leonard 2018-08-17 17:00:07 UTC
Created attachment 77991 [details] [review]
Bug 7996: Correct wrong permissions for modification log

To test:
 1 - Create a staff patron with catalogue,borrowers,and view_system_logs
     permissions
 2 - Log in using that patron
 3 - Go to tools
 4 - Click 'Log viewer'
 5 - You get a blank page
 6 - View borrower circ screen, note you do not see 'Modification log'
 7 - Edit a borrower, note you do not see 'Modification log'
 8 - Apply patch
 9 - 'Log Viewer' should now load correctly
10 - You should see 'Modification log' tab during circ
11 - You should see 'Modification log' tab when editing patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Jonathan Druart 2018-08-17 17:32:08 UTC
It seems that we could remove the condition in viewlog.tt, do you confirm?
Comment 8 Jonathan Druart 2018-08-17 17:32:52 UTC
Created attachment 77993 [details] [review]
Bug 7996: Correct wrong permissions for modification log

To test:
 1 - Create a staff patron with catalogue,borrowers,and view_system_logs
     permissions
 2 - Log in using that patron
 3 - Go to tools
 4 - Click 'Log viewer'
 5 - You get a blank page
 6 - View borrower circ screen, note you do not see 'Modification log'
 7 - Edit a borrower, note you do not see 'Modification log'
 8 - Apply patch
 9 - 'Log Viewer' should now load correctly
10 - You should see 'Modification log' tab during circ
11 - You should see 'Modification log' tab when editing patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Jonathan Druart 2018-08-17 17:33:41 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 77993 [details] [review] [review]

Rebased on top of bug 13618.
Comment 10 Nick Clemens 2018-08-20 11:52:47 UTC
(In reply to Jonathan Druart from comment #7)
> It seems that we could remove the condition in viewlog.tt, do you confirm?

Yes, the permission check is done in the script, we shouldn't need a second
Comment 11 Katrin Fischer 2018-08-21 20:18:07 UTC
Created attachment 78052 [details] [review]
Bug 7996: Correct wrong permissions for modification log

To test:
 1 - Create a staff patron with catalogue,borrowers,and view_system_logs
     permissions
 2 - Log in using that patron
 3 - Go to tools
 4 - Click 'Log viewer'
 5 - You get a blank page
 6 - View borrower circ screen, note you do not see 'Modification log'
 7 - Edit a borrower, note you do not see 'Modification log'
 8 - Apply patch
 9 - 'Log Viewer' should now load correctly
10 - You should see 'Modification log' tab during circ
11 - You should see 'Modification log' tab when editing patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2018-08-21 20:25:20 UTC
Created attachment 78053 [details] [review]
Bug 7996: (QA follow-up)  Remove unnecessary permission check view logs template

Permission is checked in the perl file, no need to check again
in the template.
Comment 13 Nick Clemens 2018-08-22 15:02:49 UTC
Awesome work all!

Pushed to master for 18.11
Comment 14 Martin Renvoize 2018-08-24 10:30:47 UTC
Pushed to 18.05.x for 18.05.03
Comment 15 Fridolin Somers 2018-09-21 13:35:30 UTC
Pushed to 17.11.x for 17.11.10