From dc37d5d45f783824151053d2bbaf5e8ff8664452 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= 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 Signed-off-by: Paul Derscheid --- 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 3e79c8f4797..a6c613021a7 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 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 00000000000..8ab98df0c0b --- /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 945cbbbe2f2..205e41d525e 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.39.5