Actions on API keys shoudl be logged and auditable (i.e. who creates the API keys, who revokes, etc).
Created attachment 186540 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0)
Created attachment 186541 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions
Created attachment 186542 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility
Created attachment 186543 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators.
Created attachment 186544 [details] [review] Bug 20638: Include APIKEYS in 'Modification log'
Created attachment 186558 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186559 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186560 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186561 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186562 [details] [review] Bug 20638: Include APIKEYS in 'Modification log' Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Hi Tomás, this is awesome! qa script complains, though: qa Tag v25.05.00-1 found in /kohadevbox/koha — checking out branch main in /kohadevbox/qa-test-tools Already on 'main' Your branch is up to date with 'origin/main'. testing 5 commit(s) (applied to 1516f22 'f9c Bug 40665: Fix Cypress tests') Processing files [RUNNING] Koha/ApiKey.pm [RUNNING] Koha/Exceptions/ApiKey.pm [RUNNING] installer/data/mysql/atomicupdate/bug_20638.pl [RUNNING] installer/data/mysql/mandatory/sysprefs.sql [FAIL] installer/data/mysql/mandatory/sysprefs.sql [RUNNING] koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc [SKIP] Koha/Exceptions/ApiKey.pm [FAIL] installer/data/mysql/atomicupdate/bug_20638.pl [PASS] Koha/ApiKey.pm [RUNNING] koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc [RUNNING] koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref [RUNNING] koha-tmpl/intranet-tmpl/prog/en/modules/members/apikeys.tt [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref [PASS] koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc [RUNNING] koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt [RUNNING] members/apikeys.pl [PASS] koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc [RUNNING] t/db_dependent/Koha/ApiKey.t [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt [PASS] members/apikeys.pl [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/members/apikeys.tt [RUNNING] tools/viewlog.pl [PASS] t/db_dependent/Koha/ApiKey.t [PASS] tools/viewlog.pl Result: [PASS] Koha/ApiKey.pm [SKIP] Koha/Exceptions/ApiKey.pm SKIP pod_coverage No POD exists [FAIL] installer/data/mysql/atomicupdate/bug_20638.pl FAIL forbidden_patterns forbidden pattern: Have you considered sharing the new syspref to HEA? (see bug 23849 and C4/UsageStats.pm) (line 13) [FAIL] installer/data/mysql/mandatory/sysprefs.sql FAIL semicolon semicolon found instead of comma at line 896 [PASS] koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc [PASS] koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/members/apikeys.tt [PASS] koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt [PASS] members/apikeys.pl [PASS] t/db_dependent/Koha/ApiKey.t [PASS] tools/viewlog.pl Processing additional checks PASS! * Was this bug sponsored by anyone? Running tests (1) * Proving /kohadevbox/koha/t/db_dependent/Koha/ApiKey.t PASS
Additionally new logs need to use the diff https://wiki.koha-community.org/wiki/Coding_Guidelines#ACTN1:_New_additions_to_actions_logs_should_use_the_JSON_Diff_format
(In reply to Paul Derscheid from comment #11) > Hi Tomás, this is awesome! > > qa script complains, though They all seem wrong checks. * Exceptions file contains POD, this check always fails. I think it is because our POD describes the various exceptions, which are defined inside the `use` statement. And there are no methods defined so... we might need to fix the QA tools or just not care about this particular error. * The semi-colon one I don't understand it. It is probably another QA script bug. * The HEA one I don't really understand it. I mentioned on the bug that it needs to be revisited. Probably as a warning and with different wording. (In reply to Lisette Scheer from comment #12) > Additionally new logs need to use the diff > https://wiki.koha-community.org/wiki/Coding_Guidelines#ACTN1: > _New_additions_to_actions_logs_should_use_the_JSON_Diff_format Thanks for this one. I really didn't know about it.
Created attachment 186577 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0)
Created attachment 186578 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions
Created attachment 186579 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility
Created attachment 186580 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators.
Created attachment 186581 [details] [review] Bug 20638: Include APIKEYS in 'Modification log'
Created attachment 186582 [details] [review] Bug 20638: Improve ApiKey logging to use structured data This patch enhances the ApiKey logging mechanism to pass structured object data instead of formatted strings to logaction(), following the ACTN1 coding guideline for action logs. Changes: - Pass the ApiKey object directly to logaction() for CREATE/REVOKE/ACTIVATE - Pass undef for info and the object in diff field for DELETE operations - Refactor store/revoke/activate methods to improve logging consistency - Update tests to handle the new structured logging format The new approach provides richer, more structured log data that can be better utilized by log analysis tools and maintains consistency with other Koha object logging patterns. Test plan: 1. Enable ApiKeyLog system preference 2. Create, revoke, activate, and delete API keys 3. Verify action_logs table contains structured object data 4. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 186583 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186584 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186585 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186586 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186587 [details] [review] Bug 20638: Include APIKEYS in 'Modification log' Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 186588 [details] [review] Bug 20638: Improve ApiKey logging to use structured data This patch enhances the ApiKey logging mechanism to pass structured object data instead of formatted strings to logaction(), following the ACTN1 coding guideline for action logs. Changes: - Pass the ApiKey object directly to logaction() for CREATE/REVOKE/ACTIVATE - Pass undef for info and the object in diff field for DELETE operations - Refactor store/revoke/activate methods to improve logging consistency - Update tests to handle the new structured logging format The new approach provides richer, more structured log data that can be better utilized by log analysis tools and maintains consistency with other Koha object logging patterns. Test plan: 1. Enable ApiKeyLog system preference 2. Create, revoke, activate, and delete API keys 3. Verify action_logs table contains structured object data 4. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
The info doesn't seem to be accurate, it's showing what it was before instead of what it is now (not sure if that's expected) and the just shows {} Create "$VAR1 = { 'active' => 1," Revoke "$VAR1 = { 'active' => 1," Activate" $VAR1 = { 'active' => 0," Cataloging log shoes before, but it shares that it's before: biblio BEFORE=> Patron log also shows before in the info { "middle_name" : { "after" : "middle name", "before" : null } } There's also some discussion on Bug 20956 about if the example in the ACTN1 is the way we should be doing this.
Created attachment 187004 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187005 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187006 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187007 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187008 [details] [review] Bug 20638: Include APIKEYS in 'Modification log' Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187009 [details] [review] Bug 20638: Improve ApiKey logging to use structured data This patch enhances the ApiKey logging mechanism to pass structured object data instead of formatted strings to logaction(), following the ACTN1 coding guideline for action logs. Changes: - Pass the ApiKey object directly to logaction() for CREATE/REVOKE/ACTIVATE - Pass undef for info and the object in diff field for DELETE operations - Refactor store/revoke/activate methods to improve logging consistency - Update tests to handle the new structured logging format The new approach provides richer, more structured log data that can be better utilized by log analysis tools and maintains consistency with other Koha object logging patterns. Test plan: 1. Enable ApiKeyLog system preference 2. Create, revoke, activate, and delete API keys 3. Verify action_logs table contains structured object data 4. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 187011 [details] [review] Bug 20638: (follow-up) Silence viewlog.pl warning This patch fixes useless warnings being printed by `viewlog.pl` I noticed while testing this new feature. To test: 1. Test the feature introduced by this patchset 2. Make sure you are running: $ ktd --shell k$ tail -f /var/log/koha/kohadev/*.log => FAIL: You see things like: ``` [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:43:44] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:43:44] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. ``` 3. Refresh the viewlog page => FAIL: They are consistent 4. Apply this patch 5. Repeat 3 => SUCCESS: No more warnings! 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Thanks Lisette for testing! I found out the `C4::Log::actionlog` method is actually performing an implicit `$infos->get_from_storage()` which breaks the logic I implemented, as my original implementation was: 1. modify the object ($self) 2. create an unmodified copy of the object ($self->get_from_storage) [1] 3. pass both to `logaction()` 4. call `store()` So it was always comparing the untouched object with itself, even though I was passing a modified one! I changed it to match what `actionlog` is designed to, even though I dislike this a lot :-D. About [1] I opted to use `clone` and save a DB query.
Created attachment 187020 [details] [review] Bug 20638: Add ApiKeyLog system preference This patch adds the ApiKeyLog system preference to control logging of API key actions (creation, deletion, revocation, activation). The preference follows the same pattern as BorrowersLog and other logging preferences. Test plan: 1. Apply patch and run database update 2. Go to Administration > System preferences > Logs 3. Verify ApiKeyLog preference appears with Yes/No options 4. Verify default value is 'Don't log' (0) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187021 [details] [review] Bug 20638: Add revoke() and activate() methods to Koha::ApiKey This patch adds two new methods to Koha::ApiKey: - revoke(): Sets active to 0, throws exception if already revoked - activate(): Sets active to 1, throws exception if already active Both methods include proper exception handling and comprehensive tests. Test plan: 1. prove t/db_dependent/Koha/ApiKey.t 2. Verify new subtests pass for revoke() and activate() methods 3. Confirm exceptions are thrown for invalid state transitions Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187022 [details] [review] Bug 20638: Update apikeys.pl and template with proper exception handling This patch comprehensively updates the API keys management interface to use the new revoke() and activate() methods with proper exception handling. Controller changes (members/apikeys.pl): - Add Try::Tiny and Scalar::Util imports for exception handling - Replace direct field manipulation (active(0)/active(1)) with proper method calls (revoke()/activate()) to ensure logging - Wrap method calls in try/catch blocks to handle specific exceptions: * Koha::Exceptions::ApiKey::AlreadyRevoked * Koha::Exceptions::ApiKey::AlreadyActive - Implement standard Koha messages pattern with @messages array - Improve code structure with elsif chain instead of multiple if statements - Add proper error handling for key_not_found cases - Include error_string for unhandled exceptions to aid debugging Template changes (koha-tmpl/.../apikeys.tt): - Add comprehensive error message handling for all exception types: * already_revoked: Clear message for revoked keys * already_active: Clear message for active keys * key_not_found: Message for missing keys * unhandled_exception: Generic message with detailed error string Key benefits: - All API key state changes are now properly logged when ApiKeyLog is enabled - Better user experience with specific, actionable error messages - Robust exception handling prevents crashes and provides debugging info - Improved code maintainability and extensibility Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187023 [details] [review] Bug 20638: Add APIKEYS support to log viewer Add comprehensive support for viewing API key logs in the log viewer: tools/viewlog.pl: - Add APIKEYS to patron lookup logic alongside MEMBERS, CIRCULATION, FINES - Enable patron name display for API key log entries koha-tmpl/.../viewlog.tt: - Add APIKEYS to the modules list for filtering - Add ACTIVATE and REVOKE to the actions list for filtering - Include APIKEYS in patron display logic for object column koha-tmpl/.../action-logs.inc: - Add APIKEYS module translation: 'API keys' - Include ApiKeyLog system preference check with warning icon - Add ACTIVATE and REVOKE action translations - Maintain alphabetical order in translation blocks This enables librarians to view and filter API key creation, activation, revocation, and deletion activities in the standard log viewer interface, with proper patron name resolution and preference-based logging status indicators. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187024 [details] [review] Bug 20638: Include APIKEYS in 'Modification log' Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187025 [details] [review] Bug 20638: Improve ApiKey logging to use structured data This patch enhances the ApiKey logging mechanism to pass structured object data instead of formatted strings to logaction(), following the ACTN1 coding guideline for action logs. Changes: - Pass the ApiKey object directly to logaction() for CREATE/REVOKE/ACTIVATE - Pass undef for info and the object in diff field for DELETE operations - Refactor store/revoke/activate methods to improve logging consistency - Update tests to handle the new structured logging format The new approach provides richer, more structured log data that can be better utilized by log analysis tools and maintains consistency with other Koha object logging patterns. Test plan: 1. Enable ApiKeyLog system preference 2. Create, revoke, activate, and delete API keys 3. Verify action_logs table contains structured object data 4. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 187026 [details] [review] Bug 20638: (follow-up) Silence viewlog.pl warning This patch fixes useless warnings being printed by `viewlog.pl` I noticed while testing this new feature. To test: 1. Test the feature introduced by this patchset 2. Make sure you are running: $ ktd --shell k$ tail -f /var/log/koha/kohadev/*.log => FAIL: You see things like: ``` [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:37:39] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:43:44] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. [2025/09/26 18:43:44] [WARN] Use of uninitialized value in substr at /kohadevbox/koha/tools/viewlog.pl line 156. ``` 3. Refresh the viewlog page => FAIL: They are consistent 4. Apply this patch 5. Repeat 3 => SUCCESS: No more warnings! 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Sorry, I think I have to ask more an additional external QA stamp here. Or if Paul is willing to trade SO/QA with Lisette that would work.
(In reply to Lucas Gass (lukeg) from comment #42) > Sorry, I think I have to ask more an additional external QA stamp here. Or > if Paul is willing to trade SO/QA with Lisette that would work. When I say external, I mean I would like a non-ByWater QA here.