Bug 20956 - BorrowersLog is not logging permission changes
Summary: BorrowersLog is not logging permission changes
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P2 normal
Assignee: Hammat wele
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22794
  Show dependency treegraph
 
Reported: 2018-06-15 15:03 UTC by Christopher Brannon
Modified: 2025-07-12 17:15 UTC (History)
19 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 20956: BorrowersLog is not logging permission changes (2.07 KB, patch)
2025-06-04 16:05 UTC, Hammat wele
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2018-06-15 15:03:42 UTC
I made changes to permissions, and I can see that it is not being logged.  This is a security issue and should be fixed immediately.  We cannot have people changing permissions undetected.
Comment 1 Jonathan Druart 2018-06-18 14:50:12 UTC
It sounds more like an enhancement request.
Comment 2 Katrin Fischer 2018-06-18 20:43:07 UTC
I agree with both Christopher and Jonathan. We never logged permissions and this has never come up before, so I would also classify this as an enhancement. But at the same time I think it would be a very good and valuable addition. Currently there is some work done on permissions, we might want to wait for bug 20813 before implementing this.
Comment 3 Christopher Brannon 2018-06-18 22:25:26 UTC
I'm fine with waiting.  Added dependency.
Comment 4 Katrin Fischer 2020-07-06 06:22:25 UTC
It looks like bug 20813 is not happening, we might want to come back to this.
Comment 5 Liz Rea 2021-11-19 20:36:26 UTC
We definitely want to come back to this, there is a whole slew of legislation out right now that is going to require certifications that changes such as this can be audited (at least in the US, and I presume elsewhere).
Comment 6 Liz Rea 2021-11-19 20:46:53 UTC
Also, due to the legalities, this is an actual missing feature that qualifies as a bug.
Comment 7 Tomás Cohen Arazi (tcohen) 2021-11-19 23:58:44 UTC
Should it be in the action_logs? Maybe another table?
Comment 8 Kyle M Hall (khall) 2021-12-02 18:03:09 UTC
It would be good to also log permission changes via Koha::Logger with a namespace so these changes can be logged to external facilities.
Comment 9 Katrin Fischer 2021-12-05 22:20:16 UTC
I think action_logs would be the logical place for it as it also has the other changes to the patron account for the librarians as it can be looked up easily then, but something extermal might be nice additionally.
Comment 10 David Cook 2022-11-02 01:34:26 UTC
I reckon we log it in action_logs, but just make it so that it can't be toggled off.
Comment 11 Katrin Fischer 2022-11-02 23:09:32 UTC
There are scripts that delete older entries from action_logs, something to think about if you don't want these to be deleted by accident.
Comment 12 David Cook 2022-11-02 23:33:13 UTC
(In reply to Katrin Fischer from comment #11)
> There are scripts that delete older entries from action_logs, something to
> think about if you don't want these to be deleted by accident.

I suppose that's a problem we'll face with any kind of logging potentially.

But that does explain Kyle's suggestion of logging it outside of Koha. Log rotation would be up to the sysadmins.
Comment 13 Katrin Fischer 2023-07-11 08:56:19 UTC
We just had a case where this kind of log would have been very useful. 

(In reply to Katrin Fischer from comment #11)
> There are scripts that delete older entries from action_logs, something to
> think about if you don't want these to be deleted by accident.

We can limit deletions now to certain modules and even actions I think. So that should no longer be a problem.

To be on the save side we could create a PermissionLog maybe? Keep it separate from other changes to the patron record and a separate module.
Comment 14 Magnus Enger 2024-01-29 14:13:28 UTC
How verbose should we do the logging? As far as I can tell, the information about permissions is stored in a combination of borrowers.flags and the user_permissions table. 

The information from the user_permissions table could probably be logged as is, and be pretty readable. 

But the flags is just a number, that needs to be interpreted in some way to make sense. Should we log just the number, or the interpretation?
Comment 15 Kyle M Hall (khall) 2024-01-29 14:16:44 UTC
(In reply to Magnus Enger from comment #14)
> How verbose should we do the logging? As far as I can tell, the information
> about permissions is stored in a combination of borrowers.flags and the
> user_permissions table. 
> 
> The information from the user_permissions table could probably be logged as
> is, and be pretty readable. 
> 
> But the flags is just a number, that needs to be interpreted in some way to
> make sense. Should we log just the number, or the interpretation?

My initial concern was performance, but considering user permissions changes are not a frequent activity I think either solution is reasonable. Either decode it and store it as a list of key/value pairs or just log the binary representation with a link to a wiki page where we can explain how to decode it.
Comment 16 Christopher Brannon 2024-01-30 15:53:21 UTC
(In reply to Magnus Enger from comment #14)
> How verbose should we do the logging? As far as I can tell, the information
> about permissions is stored in a combination of borrowers.flags and the
> user_permissions table. 
> 
> The information from the user_permissions table could probably be logged as
> is, and be pretty readable. 
> 
> But the flags is just a number, that needs to be interpreted in some way to
> make sense. Should we log just the number, or the interpretation?

If this is a log that a non-admin should be able to interpret, it should be the interpretation.  You could log both, especially for debugging, but the interpretation should definitely be there.  Why put only the number and make people jump through hoops to interpret if the system can do it without sacrificing performance?
Comment 17 Katrin Fischer 2024-01-30 16:56:18 UTC
I agree that we need a nice display in the log viewer at least, if we store it somewhat machine readable I would not mind that much as long as it display nicely in the GUI. Please also keep translatability in mind (also a pro for a template side solution).
Comment 18 Fiona Borthwick 2024-07-26 08:49:14 UTC
Would like to resurrect this one please. Not storing changes to patron permissions is a security concern. We are also finding this requirement coming up in tenders.
Comment 19 Lisette Scheer 2024-10-02 17:24:47 UTC
Now that we store the diff in the json, maybe we can tie this into Bug 36698 to make a nice diff view of permission changes?
Comment 20 David Cook 2024-10-09 04:53:48 UTC
(In reply to Lisette Scheer from comment #19)
> Now that we store the diff in the json, maybe we can tie this into Bug 36698
> to make a nice diff view of permission changes?

Yeah, that could be nice.

I think in the past I was concerned about using action_logs since they can be truncated periodically, but something would also be better than nothing, and this would be a nice way to do it.
Comment 21 Hammat wele 2025-06-04 16:05:01 UTC
Created attachment 182955 [details] [review]
Bug 20956: BorrowersLog is not logging permission changes

Changes to the flags field in the borrowers table are currently not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This represents a security issue.

Test plan:

    1. Go to a staff interface
    2. Search for a patron and change their permissions (note their borrowernumber)
    3. Go to tools/Log viewer
    4. Filter by Module = Patrons, Object = borrowernumber.
    5. Click on submit
       ==> No permission changes are logged.
    6. Apply the patch
    7. Repeat step 1, 2, 3, 4, 5
       ==> Permission changes are now logged
Comment 22 Blou 2025-06-10 19:09:33 UTC
This doesn't need to depend on Bug 20813 for a quick, simple solution.  I removed the dependency
Comment 23 Lisette Scheer 2025-06-10 19:45:01 UTC
Great start! I'm glad to see this is moving.

A few issues:

The action log should also follow the coding guideline for the diff column:
https://wiki.koha-community.org/wiki/Coding_Guidelines#ACTN1:_New_additions_to_actions_logs_should_use_the_JSON_Diff_format

Per Comment 10 it shouldn't be toggleable, so it should not follow the borrowers log sys pref. 

Additionally only the top level permissions are saved, not if there are changes to subpermissions. You'll get something like this:
{ "flags" : { "after" : 32, "before" : 32 } }

Please see also the discussion in comments 14-17 about how those flags/subpermissions should ideally display as readable versions instead of just numbers that need to be decoded. 

I'm really excited about this getting some movement.