From d460dc86e35555bb98e48b5e0fe4e90aa8741a5e Mon Sep 17 00:00:00 2001
From: Matt Simerson <matt@tnpi.net>
Date: Fri, 22 Jun 2012 18:29:28 -0400
Subject: [PATCH] spamassassin: add explicit default reject_type

consolidate the two data_post methods into one (more linear, simpler)
more informative log message
add new headers to top of headers (not bottom (consistent MTA behavior))
---
 plugins/spamassassin        | 54 +++++++++++++++++-----------
 t/plugin_tests/spamassassin | 70 +++++++++++++++++++------------------
 2 files changed, 70 insertions(+), 54 deletions(-)

diff --git a/plugins/spamassassin b/plugins/spamassassin
index 1101f8e..2d7d2e5 100644
--- a/plugins/spamassassin
+++ b/plugins/spamassassin
@@ -141,6 +141,7 @@ use warnings;
 
 use Qpsmtpd::Constants;
 use Qpsmtpd::DSN;
+
 use Socket qw(:DEFAULT :crlf);
 use IO::Handle;
 
@@ -155,12 +156,14 @@ sub register {
     if ( ! defined $self->{_args}{reject} && defined $self->{_args}{reject_threshold} ) {
         $self->{_args}{reject} = $self->{_args}{reject_threshold};
     };
+    if ( ! defined $self->{_args}{reject_type} ) {
+        $self->{_args}{reject_type} = 'perm';
+    };
 
-    $self->register_hook('data_post', 'check_spam_reject');
-    $self->register_hook('data_post', 'check_spam_munge_subject');
+    $self->register_hook('data_post', 'data_post_handler');
 }
 
-sub hook_data_post {
+sub data_post_handler {
     my ($self, $transaction) = @_;
 
     return (DECLINED) if $self->is_immune();
@@ -180,7 +183,8 @@ sub hook_data_post {
     my $headers = $self->parse_spamd_response( $SPAMD ) or return (DECLINED);
 
     $self->insert_spam_headers( $transaction, $headers, $username );
-    return (DECLINED);
+    $self->munge_subject( $transaction );
+    return $self->reject( $transaction );
 };
 
 sub select_spamd_username {
@@ -361,52 +365,62 @@ sub print_to_spamd {
     $self->log(LOGDEBUG, "check_spam: finished sending to spamd");
 };
 
-sub check_spam_reject {
+sub reject {
     my ($self, $transaction) = @_;
 
     my $sa_results = $self->get_spam_results($transaction) or do {
-        $self->log(LOGNOTICE, "skip: no spamassassin results");
+        $self->log(LOGNOTICE, "skip, no results");
         return DECLINED;
     };
     my $score  = $sa_results->{score} or do {
-        $self->log(LOGERROR, "skip: error getting spamassassin score");
+        $self->log(LOGERROR, "skip, error getting score");
         return DECLINED;
     };
 
     my $ham_or_spam = $sa_results->{is_spam} eq 'Yes' ? 'Spam' : 'Ham';
+    my $status = "$ham_or_spam, $score";
+    my $learn;
+    if ( $sa_results->{autolearn} ) {
+        $learn = "learn=". $sa_results->{autolearn};
+    };
 
     my $reject = $self->{_args}{reject} or do {
-        $self->log(LOGERROR, "skip: reject not set ($ham_or_spam, $score)");
+        $self->log(LOGERROR, "skip, reject disabled ($status, $learn)");
         return DECLINED;
     };
 
     if ( $score < $reject ) {
-        $self->log(LOGINFO, "pass, $ham_or_spam, $score < $reject");
-        return DECLINED;
-    };
+        if ( $ham_or_spam eq 'Spam' ) {
+            $self->log(LOGINFO, "fail, $status < $reject, $learn");
+            return DECLINED;
+        }
+        else {
+            $self->log(LOGINFO, "pass, $status < $reject, $learn");
+            return DECLINED;
+        }
+    }
 
+    $self->connection->notes('karma', $self->connection->notes('karma') - 1);
 # default of media_unsupported is DENY, so just change the message
-    $self->log(LOGINFO, "deny, $ham_or_spam, $score > $reject");
-    return Qpsmtpd::DSN->media_unsupported("spam score exceeded threshold");
+    $self->log(LOGINFO, "deny, $status, > $reject, $learn");
+    return ($self->get_reject_type(), "spam score exceeded threshold");
 }
 
-sub check_spam_munge_subject {
+sub munge_subject {
     my ($self, $transaction) = @_;
 
+    my $sa = $self->get_spam_results($transaction) or return;
     my $qp_num = $self->{_args}{munge_subject_threshold};
-    my $sa = $self->get_spam_results($transaction) or return DECLINED;
 
     my $required = $sa->{required} || $qp_num or do {
         $self->log(LOGDEBUG, "skipping munge, no user or qpsmtpd pref set");
-        return DECLINED;
+        return;
     };
-    return DECLINED unless $sa->{score} > $required;
+    return unless $sa->{score} > $required;
 
     my $subject_prefix = $self->qp->config('subject_prefix') || '*** SPAM ***';
     my $subject = $transaction->header->get('Subject') || '';
     $transaction->header->replace('Subject', "$subject_prefix $subject");
-
-    return DECLINED;
 }
 
 sub get_spam_results {
@@ -465,7 +479,7 @@ sub _cleanup_spam_header {
     $old_header_name = ($old_header_name =~ s/^X-//) ? "X-Old-$old_header_name" : "Old-$old_header_name";
 
     for my $header ( $transaction->header->get($header_name) ) {
-        $transaction->header->add($old_header_name, $header) if $action eq 'rename';
+        $transaction->header->add($old_header_name, $header, 0) if $action eq 'rename';
         $transaction->header->delete($header_name);
     }
 }
diff --git a/t/plugin_tests/spamassassin b/t/plugin_tests/spamassassin
index 67018b4..dfe6409 100644
--- a/t/plugin_tests/spamassassin
+++ b/t/plugin_tests/spamassassin
@@ -25,9 +25,9 @@ sub register_tests {
 
     $self->register_test('test_connect_to_spamd', 4);
     $self->register_test('test_parse_spam_header', 10);
-    $self->register_test('test_get_spam_results', 19);
-    $self->register_test('test_check_spam_munge_subject', 4);
-    $self->register_test('test_check_spam_reject', 2);
+    $self->register_test('test_get_spam_results', 20);
+    $self->register_test('test_munge_subject', 4);
+    $self->register_test('test_reject', 2);
 }
 
 sub test_connect_to_spamd {
@@ -43,38 +43,38 @@ sub test_connect_to_spamd {
     $self->{_args}{spamd_socket} = '/var/run/spamd/spamd.socket';
     my $SPAMD = $self->connect_to_spamd();
     if ( $SPAMD ) {
-        ok( $SPAMD, "connect_to_spamd, socket");
-        
+        ok( $SPAMD, "socket");
+
         $self->print_to_spamd( $SPAMD, $message, $length, $username );
         shutdown($SPAMD, 1); # close our side of the socket (tell spamd we're done)
         my $headers = $self->parse_spamd_response( $SPAMD );
         #warn Data::Dumper::Dumper($headers);
-        ok( $headers, "connect_to_spamd, socket response\n");
+        ok( $headers, "socket response\n");
     }
     else {
-        ok( 1 == 1, "connect_to_spamd, socket connect FAILED");
-        ok( 1 == 1, "connect_to_spamd, socket response FAILED");
+        ok( 1 == 1, "socket connect FAILED");
+        ok( 1 == 1, "socket response FAILED");
     };
 
     # Try a TCP/IP connection
     $self->{_args}{spamd_socket} = '127.0.0.1:783';
     $SPAMD = $self->connect_to_spamd();
     if ( $SPAMD ) {
-        ok( $SPAMD, "connect_to_spamd, tcp/ip");
+        ok( $SPAMD, "tcp/ip");
         #warn Data::Dumper::Dumper($SPAMD);
         $self->print_to_spamd( $SPAMD, $message, $length, $username );
         shutdown($SPAMD, 1); # close our side of the socket (tell spamd we're done)
         my $headers = $self->parse_spamd_response( $SPAMD );
         #warn Data::Dumper::Dumper($headers);
-        ok( $headers, "connect_to_spamd, tcp/ip response\n");
+        ok( $headers, "tcp/ip response\n");
     }
     else {
-        ok( 1 == 1, "connect_to_spamd, tcp/ip connect FAILED");
-        ok( 1 == 1, "connect_to_spamd, tcp/ip response FAILED");
+        ok( 1 == 1, "tcp/ip connect FAILED");
+        ok( 1 == 1, "tcp/ip response FAILED");
     };
 };
 
-sub test_check_spam_reject {
+sub test_reject {
     my $self = shift;
 
     my $transaction = $self->qp->transaction;
@@ -83,17 +83,17 @@ sub test_check_spam_reject {
     # message scored a 10, should pass
     $self->{_args}{reject} = 12;
     $transaction->notes('spamassassin', { is_spam => 'Yes', score => 10 } );
-    my $r = $self->check_spam_reject($transaction);
-    cmp_ok( DECLINED, '==', $r, "check_spam_reject, $r");
-    
+    my $r = $self->reject($transaction);
+    cmp_ok( DECLINED, '==', $r, "r: $r");
+
     # message scored a 15, should fail
     $self->{_args}{reject} = 12;
     $transaction->notes('spamassassin', { is_spam => 'Yes', score => 15 } );
-    ($r) = $self->check_spam_reject($transaction);
-    cmp_ok( DENY, '==', $r, "check_spam_reject, $r");
+    ($r) = $self->reject($transaction);
+    cmp_ok( DENY, '==', $r, "r: $r");
 };
 
-sub test_check_spam_munge_subject {
+sub test_munge_subject {
     my $self = shift;
 
     my $transaction = $self->qp->transaction;
@@ -103,31 +103,31 @@ sub test_check_spam_munge_subject {
     $self->{_args}{munge_subject_threshold} = 5;
     $transaction->notes('spamassassin', { score => 6 } );
     $transaction->header->add('Subject', $subject);
-    $self->check_spam_munge_subject($transaction);
+    $self->munge_subject($transaction);
     my $r = $transaction->header->get('Subject'); chomp $r;
-    cmp_ok($r, 'eq', "*** SPAM *** $subject", "check_spam_munge_subject +");
+    cmp_ok($r, 'eq', "*** SPAM *** $subject", "+");
 
     $transaction->header->delete('Subject');  # cleanup
     $self->{_args}{munge_subject_threshold} = 5;
     $transaction->notes('spamassassin', { score => 3 } );
     $transaction->header->add('Subject', $subject);
-    $self->check_spam_munge_subject($transaction);
+    $self->munge_subject($transaction);
     $r = $transaction->header->get('Subject'); chomp $r;
-    cmp_ok($r, 'eq', $subject, "check_spam_munge_subject -");
+    cmp_ok($r, 'eq', $subject, "-");
 
     $transaction->header->delete('Subject');  # cleanup
     $transaction->notes('spamassassin', { score => 3, required => 4 } );
     $transaction->header->add('Subject', $subject);
-    $self->check_spam_munge_subject($transaction);
+    $self->munge_subject($transaction);
     $r = $transaction->header->get('Subject'); chomp $r;
-    cmp_ok($r, 'eq', $subject, "check_spam_munge_subject -");
+    cmp_ok($r, 'eq', $subject, "-");
 
     $transaction->header->delete('Subject');  # cleanup
     $transaction->notes('spamassassin', { score => 5, required => 4 } );
     $transaction->header->add('Subject', $subject);
-    $self->check_spam_munge_subject($transaction);
+    $self->munge_subject($transaction);
     $r = $transaction->header->get('Subject'); chomp $r;
-    cmp_ok($r, 'eq', "*** SPAM *** $subject", "check_spam_munge_subject +");
+    cmp_ok($r, 'eq', "*** SPAM *** $subject", "+");
 };
 
 sub test_get_spam_results {
@@ -145,15 +145,17 @@ sub test_get_spam_results {
             $r_ref->{hits} = delete $r_ref->{score}; # SA v2 compat
         };
         my $r2 = _reassemble_header($r_ref);
-        cmp_ok( $h, 'eq', $r2, "get_spam_results ($h)" );
+        cmp_ok( $h, 'eq', $r2, $h );
 
         # this time it should be cached
         $r_ref = $self->get_spam_results($transaction);
-        next if $h =~ /hits=/;   # caching is broken for SA v2 headers 
+        if ( $h =~ /hits=/ ) {
+            ok( 1 );
+            next;
+        };   # caching is broken for SA v2 headers
         $r2 = _reassemble_header($r_ref);
-        cmp_ok( $h, 'eq', $r2, "get_spam_results ($h)" );
+        cmp_ok( $h, 'eq', $r2, $h );
     };
-
 };
 
 sub test_parse_spam_header {
@@ -161,11 +163,11 @@ sub test_parse_spam_header {
 
     foreach my $h ( @sample_headers ) {
         my $r_ref = $self->parse_spam_header($h);
-        if ( $h =~ /hits=/ ) { 
+        if ( $h =~ /hits=/ ) {
             $r_ref->{hits} = delete $r_ref->{score}; # SA v2 compat
         };
         my $r2 = _reassemble_header($r_ref);
-        cmp_ok( $h, 'eq', $r2, "parse_spam_header ($h)" );
+        cmp_ok( $h, 'eq', $r2, $h );
     };
 };
 
@@ -181,7 +183,7 @@ sub test_message {
     return <<'EO_MESSAGE'
 To: Fictitious User <fict@example.com>
 From: No Such <such@example.com>
-Subject: jose can you see, by the dawns early light?  
+Subject: jose can you see, by the dawns early light?
 
 What so proudly we.
 EO_MESSAGE