Attachment 124247 Details for Bug 28772 – Bug 28772: Make Koha::ApiKey->store encrypt the secret

[patch] Bug 28772: Make Koha::ApiKey->store encrypt the secret

Bug-28772-Make-KohaApiKey-store-encrypt-the-secret.patch (text/plain), 6.68 KB, created by Tomás Cohen Arazi (tcohen) on 2021-08-30 15:18:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2021-08-30 15:18:00 UTC

Size: 6.68 KB

patch

obsolete

>From b38a8efdcb09bd7f05fa1669605354a8a96e8a9a Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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<store> 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<secret> 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<undef> 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

Actions: View | Diff | Splinter Review

Attachments on bug 28772: 124195 | 124247 | 124248 | 124249 | 124250 | 124251 | 124487 | 124488 | 124489 | 124490 | 124491 | 124691 | 124692 | 124693 | 124694 | 124695 | 124696 | 124697 | 124698 | 124699 | 124700 | 124701 | 124702 | 124703 | 124704 | 124705 | 124727 | 125088 | 125112 | 125116 | 125118 | 125128