Bugzilla – Attachment 187025 Details for
Bug 20638
Add audit logging for API key actions
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20638: Improve ApiKey logging to use structured data
Bug-20638-Improve-ApiKey-logging-to-use-structured.patch (text/plain), 7.14 KB, created by
Lisette Scheer
on 2025-09-26 20:59:39 UTC
(
hide
)
Description:
Bug 20638: Improve ApiKey logging to use structured data
Filename:
MIME Type:
Creator:
Lisette Scheer
Created:
2025-09-26 20:59:39 UTC
Size:
7.14 KB
patch
obsolete
>From 3bd61181f33156a4cb9cfa03fa3afb8426d3e3de Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= <tomascohen@theke.io> >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 <paul.derscheid@lmscloud.de> >Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20638
:
186540
|
186541
|
186542
|
186543
|
186544
|
186558
|
186559
|
186560
|
186561
|
186562
|
186577
|
186578
|
186579
|
186580
|
186581
|
186582
|
186583
|
186584
|
186585
|
186586
|
186587
|
186588
|
187004
|
187005
|
187006
|
187007
|
187008
|
187009
|
187011
|
187020
|
187021
|
187022
|
187023
|
187024
| 187025 |
187026