Bug 7996 - Patron modification log requires parameters permission
Summary: Patron modification log requires parameters permission
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 12:41 UTC by Nicole C. Engard
Modified: 2019-10-14 19:58 UTC (History)
10 users (show)

See Also:
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 (131.46 KB, image/jpeg)
2012-04-23 12:41 UTC, Nicole C. Engard
Details
with parameters permission (128.13 KB, image/jpeg)
2012-04-23 12:41 UTC, Nicole C. Engard
Details
Bug 7996: Patron Modification Log Requires Parameters Permission (4.21 KB, patch)
2015-01-14 22:06 UTC, Rochelle Healy
Details | Diff | Splinter Review
Bug 7996: Correct wrong permissions for modification log (3.47 KB, patch)
2018-08-17 11:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7996: Correct wrong permissions for modification log (3.52 KB, patch)
2018-08-17 17:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7996: Correct wrong permissions for modification log (3.57 KB, patch)
2018-08-17 17:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7996: Correct wrong permissions for modification log (3.61 KB, patch)
2018-08-21 20:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7996: (QA follow-up) Remove unnecessary permission check view logs template (1.24 KB, patch)
2018-08-21 20:25 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 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