From da6d57960e66b68a8e8e2aa518cd3c20e531d6da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Thu, 18 Sep 2025 16:05:29 -0300 Subject: [PATCH] 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 --- Koha/ApiKey.pm | 32 +++++++++----------------------- t/db_dependent/Koha/ApiKey.t | 13 ++++++------- 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/Koha/ApiKey.pm b/Koha/ApiKey.pm index a6c613021a..1d6cf2faa1 100644 --- a/Koha/ApiKey.pm +++ b/Koha/ApiKey.pm @@ -70,19 +70,14 @@ sub store { ); } - my $result = $self->SUPER::store(); - # Log the action unless explicitly skipped if ( !$params->{skip_log} && C4::Context->preference('ApiKeyLog') ) { if ($is_new) { - logaction( - 'APIKEYS', 'CREATE', $self->patron_id, - sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) - ); + logaction( 'APIKEYS', 'CREATE', $self->patron_id, $self, undef, $self->get_from_storage() ); } } - return $result; + return $self->SUPER::store(); } =head3 validate_secret @@ -139,10 +134,7 @@ sub delete { my $result = $self->SUPER::delete(); if ( C4::Context->preference('ApiKeyLog') ) { - logaction( - 'APIKEYS', 'DELETE', $patron_id, - sprintf( "Client ID: %s, Description: %s", $client_id, $description ) - ); + logaction( 'APIKEYS', 'DELETE', $patron_id, undef, undef, $self ); } return $result; @@ -163,16 +155,13 @@ sub revoke { Koha::Exceptions::ApiKey::AlreadyRevoked->throw if !$self->active; - $self->active(0)->store( { skip_log => 1 } ); + $self->active(0); if ( C4::Context->preference('ApiKeyLog') ) { - logaction( - 'APIKEYS', 'REVOKE', $self->patron_id, - sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) - ); + logaction( 'APIKEYS', 'REVOKE', $self->patron_id, $self, undef, $self->get_from_storage() ); } - return $self; + return $self->store( { skip_log => 1 } ); } =head3 activate @@ -190,16 +179,13 @@ sub activate { Koha::Exceptions::ApiKey::AlreadyActive->throw if $self->active; - $self->active(1)->store( { skip_log => 1 } ); + $self->active(1); if ( C4::Context->preference('ApiKeyLog') ) { - logaction( - 'APIKEYS', 'ACTIVATE', $self->patron_id, - sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) - ); + logaction( 'APIKEYS', 'ACTIVATE', $self->patron_id, $self, undef, $self->get_from_storage() ); } - return $self; + return $self->store( { skip_log => 1 } ); } =head2 Internal methods diff --git a/t/db_dependent/Koha/ApiKey.t b/t/db_dependent/Koha/ApiKey.t index 205e41d525..a520f85eaa 100755 --- a/t/db_dependent/Koha/ApiKey.t +++ b/t/db_dependent/Koha/ApiKey.t @@ -154,7 +154,7 @@ subtest 'validate_secret() tests' => sub { )->first; ok( $log_entry, 'CREATE log entry found' ); is( $log_entry->object, $test_patron->id, 'Log object is patron_id' ); - like( $log_entry->info, qr/Client ID: .+, Description: logged/, 'Log info contains client_id and description' ); + like( $log_entry->info, qr/logged/, 'Log info contains description' ); # Test no CREATE logging when ApiKeyLog is disabled t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); @@ -178,10 +178,9 @@ subtest 'validate_secret() tests' => sub { )->first; ok( $delete_log, 'DELETE log entry found' ); is( $delete_log->object, $test_patron->id, 'DELETE log object is patron_id' ); - like( - $delete_log->info, qr/Client ID: $client_id_to_delete, Description: logged/, - 'DELETE log info contains client_id and description' - ); + + # DELETE logs pass undef for info field, object is in diff field + is( $delete_log->info, undef, 'DELETE log info is undef as expected' ); # Test no DELETE logging when ApiKeyLog is disabled t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); @@ -247,7 +246,7 @@ subtest 'revoke() tests' => sub { ok( $revoke_log, 'REVOKE log entry found' ); is( $revoke_log->object, $patron->id, 'REVOKE log object is patron_id' ); like( - $revoke_log->info, qr/Client ID: $client_id_to_revoke, Description: test2/, + $revoke_log->info, qr/$client_id_to_revoke.*test2/s, 'REVOKE log info contains client_id and description' ); @@ -301,7 +300,7 @@ subtest 'activate() tests' => sub { ok( $activate_log, 'ACTIVATE log entry found' ); is( $activate_log->object, $patron->id, 'ACTIVATE log object is patron_id' ); like( - $activate_log->info, qr/Client ID: $client_id_to_activate, Description: test2/, + $activate_log->info, qr/$client_id_to_activate.*test2/s, 'ACTIVATE log info contains client_id and description' ); -- 2.51.0