Merge pull request #104 from flimzy/clam-opts

Fix clamdscan configuration handling

Makefile.PL

@@ -17,6 +17,7 @@ WriteMakefile(
         'Net::IP'         => 0,
         'Time::HiRes'     => 0,
         'IO::Socket::SSL' => 0,
+        'ClamAV::Client'  => 0,      # virus/clamdscan
 # Dev/Test modules
         'Test::More'      => 0,
         'Test::Output'    => 0,

plugins/virus/clamdscan

@@ -136,10 +136,14 @@ sub register {
     }
 
     # Set sensible defaults
-    $self->{_args}{deny_viruses} ||= 'yes';
     $self->{_args}{max_size}     ||= 1024;
-    $self->{_args}{scan_all}     ||= 1;
+    if ( ! defined $self->{_args}{deny_viruses} ) {
-    for my $setting ('deny_viruses', 'defer_on_error') {
+        $self->{_args}{deny_viruses} = 'yes';
+    }
+    if ( ! defined $self->{_args}{scan_all} ) {
+        $self->{_args}{scan_all} = 1;
+    }
+    for my $setting (qw( deny_viruses defer_on_error scan_all )) {
         next unless $self->{'_args'}{$setting};
         if (lc $self->{'_args'}{$setting} eq 'no') {
             $self->{'_args'}{$setting} = 0;
@@ -155,10 +159,9 @@ sub data_post_handler {
 
     if ($self->connection->notes('naughty')) {
         $self->log(LOGINFO, "skip, naughty");
-        return (DECLINED);
+        return DECLINED;
     }
-    return (DECLINED) if $self->is_too_big($transaction);
+    return DECLINED if ! $self->should_scan($transaction);
-    return (DECLINED) if $self->is_not_multipart($transaction);
 
     my $clamd = $self->get_clamd()
       or return $self->err_and_return("Cannot instantiate ClamAV::Client");
@@ -193,19 +196,19 @@ sub data_post_handler {
         $self->adjust_karma(-1);
 
         if ($self->{_args}{deny_viruses}) {
-            return (DENY, "Virus found: $found");
+            return DENY, "Virus found: $found";
         }
 
         $transaction->header->add('X-Virus-Found',   'Yes',  0);
         $transaction->header->add('X-Virus-Details', $found, 0);
-        return (DECLINED);
+        return DECLINED;
     }
 
     $self->log(LOGINFO, "pass, clean");
     $transaction->header->add('X-Virus-Found', 'No', 0);
     $transaction->header->add('X-Virus-Checked',
                               "by $version on " . $self->qp->config('me'), 0);
-    return (DECLINED);
+    return DECLINED;
 }
 
 sub assemble_message {
@@ -223,9 +226,9 @@ sub err_and_return {
     if ($message) {
         $self->log(LOGERROR, $message);
     }
-    return (DENYSOFT, "Unable to scan for viruses")
+    return DENYSOFT, "Unable to scan for viruses"
       if $self->{_args}{defer_on_error};
-    return (DECLINED, "skip");
+    return DECLINED, "skip";
 }
 
 sub get_filename {
@@ -317,24 +320,31 @@ sub is_too_big {
     }
 
     $self->log(LOGDEBUG, "data_size, $size");
-    return;
+    return 0;
 }
 
-sub is_not_multipart {
+sub is_multipart {
     my $self = shift;
     my $transaction = shift || $self->qp->transaction;
 
-    return if $self->{'_args'}{'scan_all'};
+    return 0 if !$transaction->header;
-
-    return 1 if !$transaction->header;
 
     # Ignore non-multipart emails
-    my $content_type = $transaction->header->get('Content-Type') or return 1;
+    my $content_type = $transaction->header->get('Content-Type') or return 0;
     $content_type =~ s/\s/ /g;
     if ($content_type !~ m!\bmultipart/.*\bboundary="?([^"]+)!i) {
         $self->log(LOGNOTICE, "skip, not multipart");
+        return 0;
+    }
+
     return 1;
 }
 
-    return;
+sub should_scan {
+    my $self = shift;
+    my $tran = shift;
+    return 0 if $self->is_too_big($tran);
+    return 1 if $self->{_args}{scan_all};
+    return 0 if ! $self->is_multipart($tran);
+    return 1;
 }

t/plugin_tests/virus/clamdscan

@@ -4,28 +4,41 @@ use strict;
 use warnings;
 
 use Qpsmtpd::Constants;
+use Qpsmtpd::Transaction;
+use Mail::Header;
 
 sub register_tests {
     my $self = shift;
 
+    SKIP: {
         eval 'use ClamAV::Client';  ## no critic (Stringy)
-    if ( ! $@ ) {
+        skip "Could not load ClamAV::Client", 4
-        $self->register_test('test_register', 3);
+            if $@;
+        $self->register_test('test_register', 6);
         $self->register_test('test_get_clamd', 1);
-    };
+    }
     $self->register_test('test_err_and_return', 2);
     $self->register_test('test_get_filename', 1);
     $self->register_test('test_set_permission', 1);
     $self->register_test('test_is_too_big', 2);
-    $self->register_test('test_is_not_multipart', 2);
+    $self->register_test('test_is_multipart', 2);
+    $self->register_test('test_should_scan',4);
 }
 
 sub test_register {
     my $self = shift;
     
-    ok( $self->{_args}{deny_viruses} eq 'yes', "deny_viruses");
+    ok( $self->{_args}{deny_viruses},   "deny_viruses 1");
-    ok( $self->{_args}{max_size} == 128, "max_size");
+    is( $self->{_args}{max_size}, 1024, "max_size 1");
-    ok( $self->{_args}{scan_all} == 0, "scan_all");
+    ok( $self->{_args}{scan_all},       "scan_all 1");
+
+    my $qp = $self->qp;
+
+    # Re-initialize the plugin with some different options
+    $self->register($qp,qw( scan_all 0 max_size 200 deny_viruses no));
+    ok( ! $self->{_args}{deny_viruses}, "deny_viruses 2");
+    is( $self->{_args}{max_size}, 200,  "max_size 2");
+    ok( !$self->{_args}{scan_all},      "scan_all 2");
 };
 
 sub test_err_and_return {
@@ -62,28 +75,40 @@ sub test_get_clamd {
 
 sub test_is_too_big {
     my $self = shift;
-    my $tran = shift || $self->qp->transaction();
+    my $tran = Qpsmtpd::Transaction->new();
 
     $self->{_args}{max_size} = 8;
-    $tran->{_body_size} = (7 * 1024 );
+    $tran->{_body_size} = 7 * 1024;
-    ok( ! $self->is_too_big( $tran ), "is_too_big");
+    ok( ! $self->is_too_big( $tran ), "is_too_big 1");
 
-    $tran->{_body_size} = (9 * 1024 );
+    $tran->{_body_size} = 9 * 1024;
-    ok( $self->is_too_big( $tran ), "is_too_big");
+    ok( $self->is_too_big( $tran ), "is_too_big 2");
 }
 
-sub test_is_not_multipart {
+sub test_is_multipart {
     my $self = shift;
-    my $tran = shift || $self->qp->transaction();
+    my $tran = Qpsmtpd::Transaction->new();
 
-    ok( $self->is_not_multipart(), "not_multipart" );
+    ok( ! $self->is_multipart($tran), "is_multipart 1" );
-
+    $tran->header( Mail::Header->new( [
-    if ( $tran->header ) {
+        'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"'
-        $tran->header->add('Content-Type', 'multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"');
+    ] ) );
-        ok( ! $self->is_not_multipart(), "not_multipart" );
+    ok( $self->is_multipart($tran), "is_multipart 2" );
-    }
-    else {
-        ok( 1 );
-    }
 }
 
+sub test_should_scan {
+    my $self = shift;
+    my $trans = Qpsmtpd::Transaction->new();
+    $trans->{_body_size} = 1;
+    $self->{_args}{scan_all} = 1;
+    ok( $self->should_scan($trans), "Should scan small message, scan_all=1");
+    $self->{_args}{scan_all} = 0;
+    ok( ! $self->should_scan($trans), "Should not scan small message, scan_all=0");
+    $trans->{_body_size} = 99999999999;
+    ok( !$self->should_scan($trans), "Should not scan large message" );
+    $trans->{_body_size} = 1;
+    $trans->header( Mail::Header->new( [
+        'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"'
+    ] ) );
+    ok( $self->should_scan($trans), "Should not scan multi-part message" );
+}