diff --git a/Koha/ApiKey.pm b/Koha/ApiKey.pm index a89b20f76c..d648f5b708 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 $patron_id = $self->patron_id; + my $result = $self->SUPER::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 ) + 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 0af9e38fc1..f953d7a49f 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' );