Bugzilla – Attachment 186541 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: Add revoke() and activate() methods to Koha::ApiKey
Bug-20638-Add-revoke-and-activate-methods-to-KohaA.patch (text/plain), 14.25 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-09-17 17:49:40 UTC
(
hide
)
Description:
Bug 20638: Add revoke() and activate() methods to Koha::ApiKey
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-09-17 17:49:40 UTC
Size:
14.25 KB
patch
obsolete
>From 708965a305ba9fdea170fd51cba31439ac8bd3e9 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= <tomascohen@theke.io> >Date: Wed, 17 Sep 2025 13:06:13 -0300 >Subject: [PATCH] Bug 20638: Add revoke() and activate() methods to > Koha::ApiKey > >This patch adds two new methods to Koha::ApiKey: >- revoke(): Sets active to 0, throws exception if already revoked >- activate(): Sets active to 1, throws exception if already active > >Both methods include proper exception handling and comprehensive tests. > >Test plan: >1. prove t/db_dependent/Koha/ApiKey.t >2. Verify new subtests pass for revoke() and activate() methods >3. Confirm exceptions are thrown for invalid state transitions >--- > Koha/ApiKey.pm | 103 +++++++++++++++++++++- > Koha/Exceptions/ApiKey.pm | 41 +++++++++ > t/db_dependent/Koha/ApiKey.t | 163 ++++++++++++++++++++++++++++++++++- > 3 files changed, 303 insertions(+), 4 deletions(-) > create mode 100644 Koha/Exceptions/ApiKey.pm > >diff --git a/Koha/ApiKey.pm b/Koha/ApiKey.pm >index 3e79c8f479..a6c613021a 100644 >--- a/Koha/ApiKey.pm >+++ b/Koha/ApiKey.pm >@@ -19,7 +19,9 @@ package Koha::ApiKey; > > use Modern::Perl; > >+use C4::Log qw( logaction ); > use Koha::AuthUtils qw(hash_password); >+use Koha::Exceptions::ApiKey; > use Koha::Exceptions::Object; > > use List::MoreUtils qw(any); >@@ -44,7 +46,10 @@ Overloaded I<store> method. > =cut > > sub store { >- my ($self) = @_; >+ my ( $self, $params ) = @_; >+ $params //= {}; >+ >+ my $is_new = !$self->in_storage; > > if ( $self->in_storage ) { > my %dirty_columns = $self->_result->get_dirty_columns; >@@ -60,11 +65,24 @@ sub store { > { > secret => Koha::AuthUtils::hash_password( $self->{_plain_text_secret} ), > client_id => $self->_generate_unused_uuid('client_id'), >+ active => 1, > } > ); > } > >- return $self->SUPER::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 ) >+ ); >+ } >+ } >+ >+ return $result; > } > > =head3 validate_secret >@@ -103,6 +121,87 @@ sub plain_text_secret { > return; > } > >+=head3 delete >+ >+ $api_key->delete; >+ >+Overloaded delete method to add logging. >+ >+=cut >+ >+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 ) >+ ); >+ } >+ >+ return $result; >+} >+ >+=head3 revoke >+ >+ $api_key->revoke; >+ >+Revokes the API key by setting active to 0. >+Throws an exception if the key is already revoked. >+ >+=cut >+ >+sub revoke { >+ my ($self) = @_; >+ >+ Koha::Exceptions::ApiKey::AlreadyRevoked->throw >+ if !$self->active; >+ >+ $self->active(0)->store( { skip_log => 1 } ); >+ >+ if ( C4::Context->preference('ApiKeyLog') ) { >+ logaction( >+ 'APIKEYS', 'REVOKE', $self->patron_id, >+ sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) >+ ); >+ } >+ >+ return $self; >+} >+ >+=head3 activate >+ >+ $api_key->activate; >+ >+Activates the API key by setting active to 1. >+Throws an exception if the key is already active. >+ >+=cut >+ >+sub activate { >+ my ($self) = @_; >+ >+ Koha::Exceptions::ApiKey::AlreadyActive->throw >+ if $self->active; >+ >+ $self->active(1)->store( { skip_log => 1 } ); >+ >+ if ( C4::Context->preference('ApiKeyLog') ) { >+ logaction( >+ 'APIKEYS', 'ACTIVATE', $self->patron_id, >+ sprintf( "Client ID: %s, Description: %s", $self->client_id, $self->description ) >+ ); >+ } >+ >+ return $self; >+} >+ > =head2 Internal methods > > =cut >diff --git a/Koha/Exceptions/ApiKey.pm b/Koha/Exceptions/ApiKey.pm >new file mode 100644 >index 0000000000..8ab98df0c0 >--- /dev/null >+++ b/Koha/Exceptions/ApiKey.pm >@@ -0,0 +1,41 @@ >+package Koha::Exceptions::ApiKey; >+ >+use Modern::Perl; >+ >+use Koha::Exception; >+ >+use Exception::Class ( >+ 'Koha::Exceptions::ApiKey' => { >+ isa => 'Koha::Exception', >+ }, >+ 'Koha::Exceptions::ApiKey::AlreadyRevoked' => { >+ isa => 'Koha::Exceptions::ApiKey', >+ description => 'API key is already revoked' >+ }, >+ 'Koha::Exceptions::ApiKey::AlreadyActive' => { >+ isa => 'Koha::Exceptions::ApiKey', >+ description => 'API key is already active' >+ }, >+); >+ >+=head1 NAME >+ >+Koha::Exceptions::ApiKey - Base class for API key exceptions >+ >+=head1 Exceptions >+ >+=head2 Koha::Exceptions::ApiKey >+ >+Generic API key exception. >+ >+=head2 Koha::Exceptions::ApiKey::AlreadyRevoked >+ >+Exception thrown when trying to revoke an already revoked API key. >+ >+=head2 Koha::Exceptions::ApiKey::AlreadyActive >+ >+Exception thrown when trying to activate an already active API key. >+ >+=cut >+ >+1; >diff --git a/t/db_dependent/Koha/ApiKey.t b/t/db_dependent/Koha/ApiKey.t >index 945cbbbe2f..205e41d525 100755 >--- a/t/db_dependent/Koha/ApiKey.t >+++ b/t/db_dependent/Koha/ApiKey.t >@@ -18,12 +18,13 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 5; >+use Test::More tests => 7; > use Test::MockModule; > use Test::Exception; > use Test::Warn; > > use t::lib::TestBuilder; >+use t::lib::Mocks; > > BEGIN { > use_ok('Koha::ApiKeys'); >@@ -121,7 +122,7 @@ subtest 'store() tests' => sub { > > subtest 'validate_secret() tests' => sub { > >- plan tests => 2; >+ plan tests => 12; > > $schema->storage->txn_begin; > >@@ -138,6 +139,57 @@ subtest 'validate_secret() tests' => sub { > is( $api_key->validate_secret($secret), 1, 'Valid secret returns true' ); > is( $api_key->validate_secret('Wrong secret'), 0, 'Invalid secret returns false' ); > >+ # Test CREATE logging when ApiKeyLog is enabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 1 ); >+ my $test_patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'CREATE' } )->count; >+ my $logged_key = Koha::ApiKey->new( { description => 'logged', patron_id => $test_patron->id } )->store; >+ my $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'CREATE' } )->count; >+ is( $logs_after, $logs_before + 1, 'CREATE action logged when ApiKeyLog enabled' ); >+ >+ # Verify log entry parameters >+ my $log_entry = $schema->resultset('ActionLog')->search( >+ { module => 'APIKEYS', action => 'CREATE', object => $test_patron->id }, >+ { order_by => { -desc => 'action_id' } } >+ )->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' ); >+ >+ # Test no CREATE logging when ApiKeyLog is disabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); >+ $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'CREATE' } )->count; >+ my $unlogged_key = Koha::ApiKey->new( { description => 'unlogged', patron_id => $test_patron->id } )->store; >+ $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'CREATE' } )->count; >+ is( $logs_after, $logs_before, 'CREATE action not logged when ApiKeyLog disabled' ); >+ >+ # Test DELETE logging when ApiKeyLog is enabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 1 ); >+ $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'DELETE' } )->count; >+ my $client_id_to_delete = $logged_key->client_id; >+ $logged_key->delete; >+ $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'DELETE' } )->count; >+ is( $logs_after, $logs_before + 1, 'DELETE action logged when ApiKeyLog enabled' ); >+ >+ # Verify DELETE log entry parameters >+ my $delete_log = $schema->resultset('ActionLog')->search( >+ { module => 'APIKEYS', action => 'DELETE', object => $test_patron->id }, >+ { order_by => { -desc => 'action_id' } } >+ )->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' >+ ); >+ >+ # Test no DELETE logging when ApiKeyLog is disabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); >+ $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'DELETE' } )->count; >+ $unlogged_key->delete; >+ $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'DELETE' } )->count; >+ is( $logs_after, $logs_before, 'DELETE action not logged when ApiKeyLog disabled' ); >+ > $schema->storage->txn_rollback; > }; > >@@ -158,3 +210,110 @@ subtest 'plain_text_secret() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'revoke() tests' => sub { >+ >+ plan tests => 8; >+ >+ $schema->storage->txn_begin; >+ >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $api_key = Koha::ApiKey->new( { description => 'test', patron_id => $patron->id } )->store; >+ >+ # Test successful revoke >+ is( $api_key->active, 1, 'API key starts as active' ); >+ $api_key->revoke; >+ is( $api_key->active, 0, 'API key is revoked' ); >+ >+ # Test exception when already revoked >+ throws_ok { $api_key->revoke } 'Koha::Exceptions::ApiKey::AlreadyRevoked', >+ 'Exception thrown when trying to revoke already revoked key'; >+ >+ # Test logging when ApiKeyLog is enabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 1 ); >+ my $api_key2 = Koha::ApiKey->new( { description => 'test2', patron_id => $patron->id } )->store; >+ >+ my $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'REVOKE' } )->count; >+ my $client_id_to_revoke = $api_key2->client_id; >+ $api_key2->revoke; >+ my $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'REVOKE' } )->count; >+ is( $logs_after, $logs_before + 1, 'REVOKE action logged when ApiKeyLog enabled' ); >+ >+ # Verify REVOKE log entry parameters >+ my $revoke_log = $schema->resultset('ActionLog')->search( >+ { module => 'APIKEYS', action => 'REVOKE', object => $patron->id }, >+ { order_by => { -desc => 'action_id' } } >+ )->first; >+ 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 contains client_id and description' >+ ); >+ >+ # Test no logging when ApiKeyLog is disabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); >+ my $api_key3 = Koha::ApiKey->new( { description => 'test3', patron_id => $patron->id } )->store; >+ >+ $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'REVOKE' } )->count; >+ $api_key3->revoke; >+ $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'REVOKE' } )->count; >+ is( $logs_after, $logs_before, 'REVOKE action not logged when ApiKeyLog disabled' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'activate() tests' => sub { >+ >+ plan tests => 8; >+ >+ $schema->storage->txn_begin; >+ >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $api_key = Koha::ApiKey->new( { description => 'test', patron_id => $patron->id } )->store; >+ $api_key->active(0)->store; # Start as revoked >+ >+ # Test successful activate >+ is( $api_key->active, 0, 'API key starts as revoked' ); >+ $api_key->activate; >+ is( $api_key->active, 1, 'API key is activated' ); >+ >+ # Test exception when already active >+ throws_ok { $api_key->activate } 'Koha::Exceptions::ApiKey::AlreadyActive', >+ 'Exception thrown when trying to activate already active key'; >+ >+ # Test logging when ApiKeyLog is enabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 1 ); >+ my $api_key2 = Koha::ApiKey->new( { description => 'test2', patron_id => $patron->id } )->store; >+ $api_key2->active(0)->store( { skip_log => 1 } ); # Start as revoked >+ >+ my $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'ACTIVATE' } )->count; >+ my $client_id_to_activate = $api_key2->client_id; >+ $api_key2->activate; >+ my $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'ACTIVATE' } )->count; >+ is( $logs_after, $logs_before + 1, 'ACTIVATE action logged when ApiKeyLog enabled' ); >+ >+ # Verify ACTIVATE log entry parameters >+ my $activate_log = $schema->resultset('ActionLog')->search( >+ { module => 'APIKEYS', action => 'ACTIVATE', object => $patron->id }, >+ { order_by => { -desc => 'action_id' } } >+ )->first; >+ 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 contains client_id and description' >+ ); >+ >+ # Test no logging when ApiKeyLog is disabled >+ t::lib::Mocks::mock_preference( 'ApiKeyLog', 0 ); >+ my $api_key3 = Koha::ApiKey->new( { description => 'test3', patron_id => $patron->id } )->store; >+ $api_key3->active(0)->store( { skip_log => 1 } ); # Start as revoked >+ >+ $logs_before = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'ACTIVATE' } )->count; >+ $api_key3->activate; >+ $logs_after = $schema->resultset('ActionLog')->search( { module => 'APIKEYS', action => 'ACTIVATE' } )->count; >+ is( $logs_after, $logs_before, 'ACTIVATE action not logged when ApiKeyLog disabled' ); >+ >+ $schema->storage->txn_rollback; >+}; >-- >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