From 3bd61181f33156a4cb9cfa03fa3afb8426d3e3de 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 Signed-off-by: Lisette Scheer --- Koha/ApiKey.pm | 67 +++++++++++++++++------------------- t/db_dependent/Koha/ApiKey.t | 13 ++++--- 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/Koha/ApiKey.pm b/Koha/ApiKey.pm index a89b20f76cc..d648f5b708f 100644 --- a/Koha/ApiKey.pm +++ b/Koha/ApiKey.pm @@ -19,6 +19,7 @@ package Koha::ApiKey; use Modern::Perl; +use Clone qw( clone ); use C4::Log qw( logaction ); use Koha::AuthUtils qw(hash_password); use Koha::Exceptions::ApiKey; @@ -51,7 +52,7 @@ sub store { my $is_new = !$self->in_storage; - if ( $self->in_storage ) { + if ( !$is_new ) { my %dirty_columns = $self->_result->get_dirty_columns; # only allow 'description' and 'active' to be updated @@ -70,19 +71,22 @@ sub store { ); } - my $result = $self->SUPER::store(); + my $log = !$params->{skip_log} && C4::Context->preference('ApiKeyLog'); + my $original; # 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 ) - ); - } + $original = $is_new ? undef : $self->get_from_storage() + if $log; + + $self->SUPER::store(); + + if ( $log && $is_new ) { + logaction( 'APIKEYS', 'CREATE', $self->patron_id, $self, undef ); + } elsif ($log) { + logaction( 'APIKEYS', 'MODIFY', $self->patron_id, $self, undef, $original ); } - return $result; + return $self; } =head3 validate_secret @@ -132,18 +136,11 @@ Overloaded delete method to add logging. sub delete { my ($self) = @_; - my $client_id = $self->client_id; - my $description = $self->description; - my $patron_id = $self->patron_id; - - my $result = $self->SUPER::delete(); + my $patron_id = $self->patron_id; + 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 ) + if C4::Context->preference('ApiKeyLog'); return $result; } @@ -163,14 +160,14 @@ sub revoke { Koha::Exceptions::ApiKey::AlreadyRevoked->throw if !$self->active; - $self->active(0)->store( { skip_log => 1 } ); + my $log = C4::Context->preference('ApiKeyLog'); + my $original = $log ? clone( $self->unblessed ) : undef; - if ( C4::Context->preference('ApiKeyLog') ) { - logaction( - 'APIKEYS', 'REVOKE', $self->patron_id, - sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) - ); - } + $self->active(0); + $self->store( { skip_log => 1 } ); + + logaction( 'APIKEYS', 'REVOKE', $self->patron_id, $self, undef, $original ) + if $log; return $self; } @@ -190,14 +187,14 @@ sub activate { Koha::Exceptions::ApiKey::AlreadyActive->throw if $self->active; - $self->active(1)->store( { skip_log => 1 } ); + my $log = C4::Context->preference('ApiKeyLog'); + my $original = $log ? clone( $self->unblessed ) : undef; - if ( C4::Context->preference('ApiKeyLog') ) { - logaction( - 'APIKEYS', 'ACTIVATE', $self->patron_id, - sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) - ); - } + $self->active(1); + $self->store( { skip_log => 1 } ); + + logaction( 'APIKEYS', 'ACTIVATE', $self->patron_id, $self, undef, $original ) + if $log; return $self; } diff --git a/t/db_dependent/Koha/ApiKey.t b/t/db_dependent/Koha/ApiKey.t index 0af9e38fc1e..f953d7a49fb 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.39.5