Bugzilla – Attachment 190461 Details for
Bug 40811
Enhance Koha::File::Transport API
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40811: (follow-up) Fix list context bug and add comprehensive tests
6fe6177.patch (text/plain), 11.38 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-12-12 13:01:46 UTC
(
hide
)
Description:
Bug 40811: (follow-up) Fix list context bug and add comprehensive tests
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-12-12 13:01:46 UTC
Size:
11.38 KB
patch
obsolete
>From 6fe61773dc3bbbe9570922fd72cf8d615180946b Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Thu, 23 Oct 2025 09:47:45 +0100 >Subject: [PATCH] Bug 40811: (follow-up) Fix list context bug and add > comprehensive tests >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >This patch fixes a critical bug where plain_text_password() was called >in list context during hash construction. When a transport has no >password, the method's bare 'return' statement returns an empty list () >in list context rather than undef, causing all subsequent key-value >pairs to shift positions. This resulted in malformed connection >parameters being passed to Net::SFTP::Foreign and Net::FTP. > >Example of the bug: > my %hash = ( > user => 'john', > password => $self->plain_text_password, # Returns () when no password > timeout => 10 > ); > # Results in: (user => 'john', timeout => 10) instead of > # (user => 'john', password => undef, timeout => 10) > >Changes to code: >- Wrap plain_text_password calls with scalar() to force scalar context > in both FTP.pm and SFTP.pm >- Make SFTP key and password authentication mutually exclusive using > ternary operator >- Prefer key-based auth over password when both are available (SFTP) > >Test plan: >1. Run t/db_dependent/Koha/File/Transport/FTP.t - should pass >2. Run t/db_dependent/Koha/File/Transport/SFTP.t - should pass >3. Verify tests cover: > - Scalar context behavior with no password > - FTP authentication with proper parameter passing > - SFTP password-only authentication > - SFTP key-only authentication > - SFTP key preference when both key and password exist > >Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> >--- > Koha/File/Transport/FTP.pm | 2 +- > Koha/File/Transport/SFTP.pm | 13 ++- > t/db_dependent/Koha/File/Transport/FTP.t | 58 +++++++++++- > t/db_dependent/Koha/File/Transport/SFTP.t | 109 +++++++++++++++++++++- > 4 files changed, 174 insertions(+), 8 deletions(-) > >diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm >index 47bc264054..d2ad1dafeb 100644 >--- a/Koha/File/Transport/FTP.pm >+++ b/Koha/File/Transport/FTP.pm >@@ -46,7 +46,7 @@ sub _connect { > Passive => $self->passive ? 1 : 0, > ) or return $self->_abort_operation($operation); > >- $self->{connection}->login( $self->user_name, $self->plain_text_password ) >+ $self->{connection}->login( $self->user_name, scalar $self->plain_text_password ) > or return $self->_abort_operation($operation); > > $self->add_message( >diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm >index 0df9c30b75..51f609a7b8 100644 >--- a/Koha/File/Transport/SFTP.pm >+++ b/Koha/File/Transport/SFTP.pm >@@ -23,6 +23,7 @@ use File::Spec; > use IO::File; > use Net::SFTP::Foreign; > use Try::Tiny; >+use JSON qw( decode_json encode_json ); > > use base qw(Koha::File::Transport); > >@@ -47,12 +48,14 @@ sub _connect { > # String to capture STDERR output > $self->{stderr_capture} = ''; > open my $stderr_fh, '>', \$self->{stderr_capture} or die "Can't open scalar as filehandle: $!"; >+ >+ # Use key-based auth if available, otherwise password auth >+ my $key_file = $self->_locate_key_file; > $self->{connection} = Net::SFTP::Foreign->new( >- host => $self->host, >- port => $self->port, >- user => $self->user_name, >- password => $self->plain_text_password, >- $self->_locate_key_file ? ( key_path => $self->_locate_key_file ) : (), >+ host => $self->host, >+ port => $self->port, >+ user => $self->user_name, >+ $key_file ? ( key_path => $key_file ) : ( password => scalar $self->plain_text_password ), > timeout => $self->DEFAULT_TIMEOUT, > stderr_fh => $stderr_fh, > more => [qw( -v -o StrictHostKeyChecking=no)], >diff --git a/t/db_dependent/Koha/File/Transport/FTP.t b/t/db_dependent/Koha/File/Transport/FTP.t >index 86aa400e45..214314c7f4 100755 >--- a/t/db_dependent/Koha/File/Transport/FTP.t >+++ b/t/db_dependent/Koha/File/Transport/FTP.t >@@ -17,10 +17,11 @@ > > use Modern::Perl; > >-use Test::More tests => 6; >+use Test::More tests => 7; > use Test::Exception; > use Test::NoWarnings; > use Test::Warn; >+use Test::MockModule; > > use Koha::File::Transports; > >@@ -30,6 +31,61 @@ use t::lib::Mocks; > my $schema = Koha::Database->new->schema; > my $builder = t::lib::TestBuilder->new; > >+subtest 'scalar context in authentication tests' => sub { >+ plan tests => 4; >+ >+ $schema->storage->txn_begin; >+ >+ # Test 1: Transport with no password - plain_text_password should return undef in scalar context >+ my $transport_no_pass = $builder->build_object( >+ { >+ class => 'Koha::File::Transports', >+ value => { transport => 'ftp', password => undef } >+ } >+ ); >+ >+ # Mock Net::FTP to verify parameters passed to login() >+ my $login_called = 0; >+ my @login_params; >+ my $mock_ftp = Test::MockModule->new('Net::FTP'); >+ $mock_ftp->mock( >+ 'new', >+ sub { >+ my $class = shift; >+ return bless {}, $class; >+ } >+ ); >+ $mock_ftp->mock( >+ 'login', >+ sub { >+ $login_called = 1; >+ @login_params = @_; >+ return 1; >+ } >+ ); >+ $mock_ftp->mock( 'quit', sub { return 1; } ); >+ $mock_ftp->mock( 'abort', sub { return 1; } ); >+ $mock_ftp->mock( 'status', sub { return 0; } ); >+ $mock_ftp->mock( 'message', sub { return ''; } ); >+ >+ # Test that plain_text_password returns undef when no password set >+ is( >+ scalar $transport_no_pass->plain_text_password, undef, >+ 'plain_text_password returns undef in scalar context when no password' >+ ); >+ >+ # Test 2: Attempt connection with no password - should pass undef not empty list >+ $transport_no_pass->connect; >+ is( $login_called, 1, 'login was called' ); >+ is( scalar @login_params, 3, 'login called with correct number of parameters (self, user, password)' ); >+ is( $login_params[2], undef, 'password parameter is undef, not empty list' ); >+ >+ # Clean up connection to avoid warnings in DESTROY >+ $transport_no_pass->{connection} = undef; >+ >+ $schema->storage->txn_rollback; >+}; >+ > subtest 'connect() tests' => sub { > plan tests => 1; > >diff --git a/t/db_dependent/Koha/File/Transport/SFTP.t b/t/db_dependent/Koha/File/Transport/SFTP.t >index 8e5febfcfa..33ec965ba9 100755 >--- a/t/db_dependent/Koha/File/Transport/SFTP.t >+++ b/t/db_dependent/Koha/File/Transport/SFTP.t >@@ -17,11 +17,13 @@ > > use Modern::Perl; > >-use Test::More tests => 8; >+use Test::More tests => 9; > use Test::Exception; > use Test::NoWarnings; > use Test::Warn; >+use Test::MockModule; > >+use Koha::Encryption; > use Koha::File::Transports; > > use t::lib::TestBuilder; >@@ -30,6 +32,111 @@ use t::lib::Mocks; > my $schema = Koha::Database->new->schema; > my $builder = t::lib::TestBuilder->new; > >+subtest 'scalar context and mutually exclusive authentication tests' => sub { >+ plan tests => 10; >+ >+ $schema->storage->txn_begin; >+ >+ # Setup mock upload path for key file operations >+ my $path = '/tmp/kohadev_test'; >+ t::lib::Mocks::mock_config( 'upload_path', $path ); >+ mkdir $path if !-d $path; >+ >+ # Mock Net::SFTP::Foreign to capture constructor parameters >+ my %sftp_new_params; >+ my $mock_sftp = Test::MockModule->new('Net::SFTP::Foreign'); >+ $mock_sftp->mock( >+ 'new', >+ sub { >+ my $class = shift; >+ %sftp_new_params = @_; >+ my $obj = bless { status => 0, error => undef }, $class; >+ return $obj; >+ } >+ ); >+ my $undef; >+ $mock_sftp->mock( 'error', sub { return $undef; } ); >+ $mock_sftp->mock( 'status', sub { return 0; } ); >+ $mock_sftp->mock( 'cwd', sub { return '/'; } ); >+ >+ # Test 1: Transport with no password and no key - plain_text_password returns undef >+ my $transport_no_auth = $builder->build_object( >+ { >+ class => 'Koha::File::Transports', >+ value => { transport => 'sftp', password => undef, key_file => undef } >+ } >+ ); >+ >+ is( >+ scalar $transport_no_auth->plain_text_password, undef, >+ 'plain_text_password returns undef in scalar context when no password' >+ ); >+ >+ # Test 2: Connection with no auth should pass password => undef (not empty list) >+ $transport_no_auth->connect; >+ ok( exists $sftp_new_params{password}, 'password key exists in connection params' ); >+ is( $sftp_new_params{password}, undef, 'password parameter is undef when no password set' ); >+ ok( !exists $sftp_new_params{key_path}, 'key_path is not set when no key file exists' ); >+ >+ # Test 3: Transport with key only - should use key_path, not password >+ # Must encrypt key_file since plain_text_key() will decrypt it >+ my $encrypted_key = Koha::Encryption->new->encrypt_hex('test_key_content'); >+ my $transport_key_only = $builder->build_object( >+ { >+ class => 'Koha::File::Transports', >+ value => { transport => 'sftp', password => undef, key_file => $encrypted_key } >+ } >+ ); >+ $transport_key_only->store; # Trigger key file write >+ $transport_key_only->discard_changes; >+ >+ %sftp_new_params = (); # Clear params >+ $transport_key_only->connect; >+ ok( exists $sftp_new_params{key_path}, 'key_path exists in connection params when key is set' ); >+ ok( !exists $sftp_new_params{password}, 'password is not set when using key-based auth' ); >+ >+ # Test 4: Transport with password only - should use password, not key_path >+ # Must encrypt password since plain_text_password() will decrypt it >+ my $encrypted_pass = Koha::Encryption->new->encrypt_hex('testpass'); >+ my $transport_pass_only = $builder->build_object( >+ { >+ class => 'Koha::File::Transports', >+ value => { transport => 'sftp', password => $encrypted_pass, key_file => undef } >+ } >+ ); >+ >+ %sftp_new_params = (); # Clear params >+ $transport_pass_only->connect; >+ ok( exists $sftp_new_params{password}, 'password exists in connection params when password is set' ); >+ is( $sftp_new_params{password}, 'testpass', 'password is correctly decrypted' ); >+ ok( !exists $sftp_new_params{key_path}, 'key_path is not set when using password auth' ); >+ >+ # Test 5: Transport with both key and password - should prefer key >+ my $encrypted_both_pass = Koha::Encryption->new->encrypt_hex('bothpass'); >+ my $encrypted_both_key = Koha::Encryption->new->encrypt_hex('both_test_key'); >+ my $transport_both = $builder->build_object( >+ { >+ class => 'Koha::File::Transports', >+ value => { transport => 'sftp', password => $encrypted_both_pass, key_file => $encrypted_both_key } >+ } >+ ); >+ $transport_both->store; # Trigger key file write >+ $transport_both->discard_changes; >+ >+ %sftp_new_params = (); # Clear params >+ $transport_both->connect; >+ ok( >+ exists $sftp_new_params{key_path} && !exists $sftp_new_params{password}, >+ 'When both key and password exist, key_path is used and password is excluded' >+ ); >+ >+ # Cleanup >+ unlink $transport_key_only->_locate_key_file if $transport_key_only->_locate_key_file; >+ unlink $transport_both->_locate_key_file if $transport_both->_locate_key_file; >+ >+ $schema->storage->txn_rollback; >+}; >+ > subtest 'store() and _post_store_trigger() tests' => sub { > plan tests => 2; > >-- >2.50.1 (Apple Git-155) >
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 40811
:
186858
|
188035
|
188152
|
188313
|
190460
| 190461