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>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

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();
 
-    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;
 }

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'
     );