Bugzilla – Attachment 186588 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), 5.87 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-09-18 19:28:01 UTC
(
hide
)
Description:
Bug 20638: Improve ApiKey logging to use structured data
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-09-18 19:28:01 UTC
Size:
5.87 KB
patch
obsolete
>From da6d57960e66b68a8e8e2aa518cd3c20e531d6da 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> >--- > 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
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