From b38a8efdcb09bd7f05fa1669605354a8a96e8a9a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Thu, 26 Aug 2021 20:24:43 -0300 Subject: [PATCH] Bug 28772: Make Koha::ApiKey->store encrypt the secret This patch refactors the Koha::ApiKey class so: - It encrypts the generated secret - Allows accessing the plain text secret only immediately after the key creation (this implies that it won't be accessible if the key is fetched from the DB). - It implements an allow list for attributes, that are not read only. Changing any other of them will make ->store throw an exception. - A method for validating plain text secrets against the encrypted one is added. - A method for accessing the plain text secret is added. Returns undef if the object is not 'fresh'. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! Expected behavior is confirmed 3. Sign off :-D --- Koha/ApiKey.pm | 61 ++++++++++++++++-- t/db_dependent/Koha/{ApiKeys.t => ApiKey.t} | 68 +++++++++++++++++++-- 2 files changed, 117 insertions(+), 12 deletions(-) rename t/db_dependent/Koha/{ApiKeys.t => ApiKey.t} (63%) diff --git a/Koha/ApiKey.pm b/Koha/ApiKey.pm index d966bc5849..4b93813609 100644 --- a/Koha/ApiKey.pm +++ b/Koha/ApiKey.pm @@ -20,9 +20,10 @@ package Koha::ApiKey; use Modern::Perl; -use Koha::Database; -use Koha::Exceptions; +use Koha::AuthUtils qw(hash_password); +use Koha::Exceptions::Object; +use List::MoreUtils qw(any); use UUID; use base qw(Koha::Object); @@ -46,14 +47,62 @@ Overloaded I method. sub store { my ($self) = @_; - $self->client_id($self->_generate_unused_uuid('client_id')) - unless $self->client_id; - $self->secret($self->_generate_unused_uuid('secret')) - unless $self->secret; + if ( $self->in_storage ) { + my %dirty_columns = $self->_result->get_dirty_columns; + + # only allow 'description' and 'active' to be updated + for my $property ( keys %dirty_columns ) { + Koha::Exceptions::Object::ReadOnlyProperty->throw( property => $property ) + if $property ne 'description' and $property ne 'active'; + } + } else { + $self->{_plain_text_secret} = $self->_generate_unused_uuid('secret'); + $self->set( + { secret => Koha::AuthUtils::hash_password( $self->{_plain_text_secret} ), + client_id => $self->_generate_unused_uuid('client_id'), + } + ); + } return $self->SUPER::store(); } +=head3 validate_secret + + if ( $api_key->validate_secret( $secret ) ) { ... } + +Returns a boolean that tells if the passed secret matches the one on the DB. + +=cut + +sub validate_secret { + my ( $self, $secret ) = @_; + + my $digest = Koha::AuthUtils::hash_password( $secret, $self->secret ); + + return $self->secret eq $digest; +} + +=head3 plain_text_secret + + my $generated_secret = $api_key->store->plain_text_secret; + +Returns the generated I so it can be displayed to the end user. +This is only accessible when the object is new and has just been stored. + +Returns I if the object was retrieved from the database. + +=cut + +sub plain_text_secret { + my ($self) = @_; + + return $self->{_plain_text_secret} + if $self->{_plain_text_secret}; + + return; +} + =head2 Internal methods =cut diff --git a/t/db_dependent/Koha/ApiKeys.t b/t/db_dependent/Koha/ApiKey.t similarity index 63% rename from t/db_dependent/Koha/ApiKeys.t rename to t/db_dependent/Koha/ApiKey.t index 94ce452c8b..4ba0861f0e 100755 --- a/t/db_dependent/Koha/ApiKeys.t +++ b/t/db_dependent/Koha/ApiKey.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 4; use Test::MockModule; use Test::Exception; @@ -30,12 +30,9 @@ BEGIN { my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; -my $uuid_gen_client_id_counter = 0; -my $uuid_gen_secret_counter = 0; - subtest 'store() tests' => sub { - plan tests => 13; + plan tests => 16; $schema->storage->txn_begin; @@ -46,7 +43,13 @@ subtest 'store() tests' => sub { my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); my $description = 'Coral API key'; - my $api_key = Koha::ApiKey->new({ patron_id => $patron_1->id, description => $description })->store; + + my $api_key = Koha::ApiKey->new( + { + patron_id => $patron_1->id, + description => $description + } + )->store; # re-read from DB $api_key->discard_changes; @@ -75,6 +78,21 @@ subtest 'store() tests' => sub { is( $api_key->patron_id, $original_api_key->{patron_id}, '->store() preserves the patron_id' ); is( $api_key->active, 0, '->store() preserves the active value' ); + $api_key->set({ client_id => 'NewID!' }); + + throws_ok { + $api_key->store + } + 'Koha::Exceptions::Object::ReadOnlyProperty', + 'Read-only attribute overwrite attempt raises exception'; + + is( $@->property, 'client_id', 'Correct attribute reported back' ); + + $api_key->discard_changes; + # set a writeable attribute + $api_key->set({ description => 'Hey' }); + lives_ok { $api_key->store } 'Updating a writeable attribute works'; + my $patron_to_delete = $builder->build_object( { class => 'Koha::Patrons' } ); my $deleted_id = $patron_to_delete->id; $patron_to_delete->delete; @@ -96,3 +114,41 @@ subtest 'store() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'validate_secret() tests' => sub { + + plan tests => 2; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $api_key = Koha::ApiKey->new( + { patron_id => $patron->id, + description => 'The description' + } + )->store; + + my $secret = $api_key->plain_text_secret; + + ok( $api_key->validate_secret( $secret ), 'Valid secret returns true' ); + ok( !$api_key->validate_secret( 'Wrong secret' ), 'Invalid secret returns true' ); + + $schema->storage->txn_rollback; +}; + +subtest 'plain_text_secret() tests' => sub { + + plan tests => 2; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + # generate a fresh API key + my $api_key = Koha::ApiKey->new({ description => 'blah', patron_id => $patron->id })->store; + my $plain_text_secret = $api_key->plain_text_secret; + + ok( defined $plain_text_secret, 'A fresh API key carries its plain text secret' ); + ok( $plain_text_secret ne q{}, 'Plain text secret is not an empty string' ); + + $schema->storage->txn_rollback; +}; -- 2.30.2