more cleanups

lib/Qpsmtpd/Command.pm

@@ -33,14 +33,17 @@ Inside a plugin
 
  sub hook_unrecognized_command_parse {
     my ($self, $transaction, $cmd) = @_;
-    return OK, \&bdat_parser if $cmd eq 'bdat';
+    if ($cmd eq 'bdat') {
+      return OK, \&bdat_parser;
+    };
  }
 
  sub bdat_parser {
     my ($self,$cmd,$line) = @_;
     # .. do something with $line...
-    return DENY, "Invalid arguments"
+    if ($some_reason_why_there_is_a_syntax_error) {
-      if $some_reason_why_there_is_a_syntax_error;
+      return DENY, "Invalid arguments";
+    };
     return OK, @args;
  }
 
@@ -72,9 +75,7 @@ sub parse {
             return DENY, $line;
         }
         ## my @log = @ret;
-        ## for (@log) {
+        ## for (@log) { $_ ||= ""; }
-        ##     $_ ||= "";
-        ## }
         ## $self->log(LOGDEBUG, "parse($cmd) => [".join("], [", @log)."]");
         return @ret;
     }
@@ -94,14 +95,18 @@ sub parse {
 
 sub parse_rcpt {
     my ($self, $cmd, $line) = @_;
-    return DENY, "Syntax error in command" if $line !~ s/^to:\s*//i;
+    if ($line !~ s/^to:\s*//i) {
-    return &_get_mail_params($cmd, $line);
+        return DENY, "Syntax error in command";
+    };
+    return _get_mail_params($cmd, $line);
 }
 
 sub parse_mail {
     my ($self, $cmd, $line) = @_;
-    return DENY, "Syntax error in command" if $line !~ s/^from:\s*//i;
+    if ($line !~ s/^from:\s*//i) {
-    return &_get_mail_params($cmd, $line);
+        return DENY, "Syntax error in command";
+    };
+    return _get_mail_params($cmd, $line);
 }
 ### RFC 1869:
 ## 6.  MAIL FROM and RCPT TO Parameters
@@ -141,28 +146,28 @@ sub _get_mail_params {
 
         # parameter syntax error, i.e. not all of the arguments were
         # stripped by the while() loop:
-        return DENY, "Syntax error in parameters" if $line =~ /\@.*\s/;
+        if ($line =~ /\@.*\s/) {
+            return DENY, "Syntax error in parameters";
+        };
         return OK, $line, @params;
     }
 
     $line = shift @params;
-    if ($cmd eq "mail") {
+    if ($cmd eq 'mail') {
-        return OK, "<>" if !$line;    # 'MAIL FROM:' --> 'MAIL FROM:<>'
+        return OK, '<>' if !$line;    # 'MAIL FROM:' --> 'MAIL FROM:<>'
-        return DENY, "Syntax error in parameters" if $line =~ /\@.*\s/; # parameter syntax error
+        if ($line =~ /\@.*\s/) {
-    }
+            return DENY, "Syntax error in parameters";
-    else {
-        if ($line =~ /\@/) {
-            return DENY, "Syntax error in parameters" if $line =~ /\@.*\s/;
-        }
-        else {
-            # XXX: what about 'abuse' in Qpsmtpd::Address?
-            return DENY, "Syntax error in parameters" if $line =~ /\s/;
-            return DENY, "Syntax error in address" if $line !~ /^(postmaster|abuse)$/i;
         }
+        return OK, $line, @params;
     }
-    ## XXX:  No: let this do a plugin, so it's not up to us to decide
+
-    ##       if we require <> around an address :-)
+    if ($line =~ /\@/) {
-    ## unless ($line =~ /^<.*>$/) { $line = "<".$line.">"; }
+        return DENY, "Syntax error in parameters" if $line =~ /\@.*\s/;
+        return OK, $line, @params;
+    }
+
+    return DENY, "Syntax error in parameters" if $line =~ /\s/;
+    return DENY, "Syntax error in address" if $line !~ /^(postmaster|abuse)$/i;
     return OK, $line, @params;
 }
 

plugins/parse_addr_withhelo

@@ -38,33 +38,34 @@ sub _parse {
     my ($self, $cmd, $line) = @_;
     $self->log(LOGDEBUG, "_parse() cmd=[$cmd], line=[$line]");
     if ($cmd eq 'mail') {
-        return DENY, "Syntax error in command" if $line !~ s/^from:\s*//i;
+        if ($line !~ s/^from:\s*//i) {
+            return DENY, "Syntax error in command";
+        };
     }
     else {    # cmd eq 'rcpt'
         return DENY, "Syntax error in command" if $line !~ s/^to:\s*//i;
     }
 
     if ($line =~ s/^(<.*>)\s*//) {
-        my $addr = $1;
+        return DENY, "No parameters allowed in " . uc($cmd) if $line =~ /^\S/;
-        return DENY, "No parameters allowed in " . uc($cmd)
+        return OK, $1;  # $1 is captured address
-            if $line =~ /^\S/;
-        return OK, $addr, ();
     }
 
     ## now, no <> are given
     $line =~ s/\s*$//;
     if ($line =~ /\@/) {
-        return DENY, "No parameters allowed in " . uc($cmd)
+        if ($line =~ /\@\S+\s+\S/) {
-          if $line =~ /\@\S+\s+\S/;
+            return DENY, "No parameters allowed in " . uc($cmd);
-        return OK, $line, ();
+        };
+        return OK, $line;
     }
 
-    if ($cmd eq "mail") {
+    if ($cmd eq 'mail') {
-        return OK, "<>" if !$line;    # 'MAIL FROM:' -> 'MAIL FROM:<>'
+        return OK, '<>' if !$line;    # 'MAIL FROM:' -> 'MAIL FROM:<>'
         return DENY, "Could not parse your MAIL FROM command";
     }
-    else {
+
-        return DENY, "Could not parse your RCPT TO command"
+    if ($line !~ /^(postmaster|abuse)$/i) {
-            if $line !~ /^(postmaster|abuse)$/i;
+        return DENY, "Could not parse your RCPT TO command";
-    }
+    };
 }

plugins/queue/qmail-queue

@@ -21,7 +21,7 @@ If set the environment variable QMAILQUEUE overrides this setting.
 =cut
 
 use strict;
-use warnings;
+#use warnings;
 
 use Qpsmtpd::Constants;
 use POSIX ();
@@ -31,8 +31,7 @@ sub register {
 
     if (@args > 0) {
         $self->{_queue_exec} = $args[0];
-        $self->log(LOGWARN, "WARNING: Ignoring additional arguments.")
+        $self->log(LOGWARN, "Ignoring additional arguments.") if @args > 1;
-          if @args > 1;
     }
 
     $self->{_queue_exec} ||= ($ENV{QMAIL} || '/var/qmail') . "/bin/qmail-queue";
@@ -49,8 +48,7 @@ sub hook_queue {
 
     local $SIG{PIPE} = sub { die 'SIGPIPE' };
     my $child = fork();
-
+    die "Could not fork" if !defined $child;
-    !defined $child and die "Could not fork";
 
     if ($child) {
 
@@ -87,44 +85,40 @@ sub hook_queue {
         $msg_id = "<$msg_id>" unless $msg_id =~ /^<.*>$/;    # surround in <>'s
         return OK, "Queued! " . time . " qp $child $msg_id";
     }
-    elsif (defined $child) {
 
-        # Child
+    return if !defined $child;
-        close MESSAGE_WRITER  or exit 1;
-        close ENVELOPE_WRITER or exit 2;
 
-        # Untaint $self->{_queue_exec}
+    # Child
-        my $queue_exec = $self->{_queue_exec};
+    close MESSAGE_WRITER  or exit 1;
-        if ($queue_exec =~ /^(\/[\/\-\_\.a-z0-9A-Z]*)$/) {
+    close ENVELOPE_WRITER or exit 2;
-            $queue_exec = $1;
-        }
-        else {
-            $self->log(LOGERROR,
-"FATAL ERROR: Unexpected characters in qmail-queue plugin argument"
-            );
 
-            # This exit is ok as we're exiting a forked child process.
+    # Untaint $self->{_queue_exec}
-            exit 3;
+    my $queue_exec = $self->{_queue_exec};
-        }
+    if ($queue_exec =~ /^(\/[\/\-\_\.a-z0-9A-Z]*)$/) {
-
+        $queue_exec = $1;
-        # save the original STDIN and STDOUT in case exec() fails below
-        open(SAVE_STDIN,  "<&STDIN");
-        open(SAVE_STDOUT, ">&STDOUT");
-
-        POSIX::dup2(fileno(MESSAGE_READER), 0)
-          or die "Unable to dup MESSAGE_READER: $!";
-        POSIX::dup2(fileno(ENVELOPE_READER), 1)
-          or die "Unable to dup ENVELOPE_READER: $!";
-
-        my $ppid = getppid();
-        $self->log(LOGNOTICE, "(for $ppid) Queuing to $queue_exec");
-
-        my $rc = exec $queue_exec;
-
-        # close the pipe
-        close(MESSAGE_READER);
-        close(MESSAGE_WRITER);
-
-        exit 6;    # we'll only get here if the exec fails
     }
+    else {
+        $self->log(LOGERROR, "FATAL: Unexpected characters in plugin argument");
+        exit 3;  # exiting the forked child process.
+    }
+
+    # save the original STDIN and STDOUT in case exec() fails below
+    open(SAVE_STDIN,  "<&STDIN");
+    open(SAVE_STDOUT, ">&STDOUT");
+
+    POSIX::dup2(fileno(MESSAGE_READER), 0)
+        or die "Unable to dup MESSAGE_READER: $!";
+    POSIX::dup2(fileno(ENVELOPE_READER), 1)
+        or die "Unable to dup ENVELOPE_READER: $!";
+
+    my $ppid = getppid();
+    $self->log(LOGNOTICE, "(for $ppid) Queuing to $queue_exec");
+
+    my $rc = exec $queue_exec;
+
+    # close the pipe
+    close(MESSAGE_READER);
+    close(MESSAGE_WRITER);
+
+    exit 6;    # we'll only get here if the exec fails
 }

plugins/quit_fortune

@@ -1,16 +1,26 @@
 #!perl -w
+use strict;
 
-sub hook_quit {
+use Qpsmtpd::Constants;
-    my $qp = shift->qp;
 
-    # if she talks EHLO she is probably too sophisticated to enjoy the
+sub register {
-    # fun, so skip it.
+    my $self = shift;
-    return DECLINED if ($qp->connection->hello || '') eq "ehlo";
+    $self->{_fortune} = '/usr/games/fortune';
+    return if ! $self->{_fortune};
 
-    my $fortune = '/usr/games/fortune';
+    # if fortune not installed, don't register hook
-    return DECLINED if !-e $fortune;
+    $self->register_hook('quit', 'fortune');
+}
 
-    my @fortune = `$fortune -s`;
+sub fortune {
+    my $self = shift;
+    my $qp = $self->qp;
+
+    # if she talks EHLO she is probably too sophisticated to enjoy the fun
+    return DECLINED if !$qp->connection->hello;
+    return DECLINED if $qp->connection->hello eq 'ehlo';
+
+    my @fortune = `$self->{_fortune} -s`;
     @fortune = map { chop; s/^/  \/ /; $_ } @fortune;
     $qp->respond(221, $qp->config('me') . " closing connection.", @fortune);
     return DONE;

plugins/rcpt_map

@@ -1,4 +1,5 @@
 #!perl -w
+use strict;
 
 =head1 NAME
 
@@ -92,13 +93,12 @@ our %map;
 sub register {
     my ($self, $qp, %args) = @_;
     foreach my $arg (qw(domain file default)) {
-        next unless exists $args{$arg};
+        next if !exists $args{$arg};
-        if ($arg eq "default") {
+        if ($arg eq 'default') {
             my ($code, $msg) = split /=/, $args{$arg};
 
-            $code = Qpsmtpd::Constants::return_code($code);
+            $code = Qpsmtpd::Constants::return_code($code)
-            die "Not a valid constant for 'default' arg"
+                or die "Not a valid constant for 'default' arg";
-              unless defined $code;
 
             $msg or $msg = "No such user.";
             $msg =~ s/_/ /g;
@@ -110,21 +110,15 @@ sub register {
         }
     }
 
-    $self->{_default}
+    $self->{_default} ||= [DENY, 'No such user.'];
-      or $self->{_default} = [DENY, "No such user."];
+    $self->{_file}   or die "No map file given...";
-
+    $self->{_domain} or die "No domain name given...";
-    $self->{_file}
-      or die "No map file given...";
-
-    $self->{_domain}
-      or die "No domain name given...";
     $self->{_domain} = lc $self->{_domain};
 
     $self->log(LOGDEBUG,
              "Using map " . $self->{_file} . " for domain " . $self->{_domain});
     %map = $self->read_map(1);
-    die "Empty map file " . $self->{_file}
+    die "Empty map file " . $self->{_file} unless keys %map;
-      unless keys %map;
 }
 
 sub hook_pre_connection {
@@ -132,8 +126,7 @@ sub hook_pre_connection {
     my ($time) = (stat($self->{_file}))[9] || 0;
     if ($time > $self->{_time}) {
         my %temp = $self->read_map();
-        keys %temp
+        return DECLINED if !keys %temp;
-          or return DECLINED;
         %map = %temp;
     }
     return DECLINED;
@@ -142,13 +135,15 @@ sub hook_pre_connection {
 sub read_map {
     my $self = shift;
     my %hash = ();
-    open F, $self->{_file}
+    open my $F, '<', $self->{_file} or do {
-      or do { $_[0] ? die "ERROR opening: $!" : return (); };
+        die "ERROR opening: $!" if $_[0];
+        return;
+    };
 
-    ($self->{_time}) = (stat(F))[9] || 0;
+    ($self->{_time}) = (stat($F))[9] || 0;
 
     my $line = 0;
-    while (<F>) {
+    while (<$F>) {
         ++$line;
         s/^\s*//;
         next if /^#/;
@@ -156,34 +151,32 @@ sub read_map {
         my ($addr, $code, $msg) = split / /, $_, 3;
         next unless $addr;
 
-        unless ($code) {
+        if (!$code) {
             $self->log(LOGERROR,
                        "No constant in line $line in " . $self->{_file});
             next;
         }
-        $code = Qpsmtpd::Constants::return_code($code);
+
-        unless (defined $code) {
+        $code = Qpsmtpd::Constants::return_code($code) or do {
             $self->log(LOGERROR,
                      "Not a valid constant in line $line in " . $self->{_file});
             next;
-        }
+        };
         $msg or $msg = "No such user.";
         $hash{$addr} = [$code, $msg];
     }
+    close $F;
     return %hash;
 }
 
 sub hook_rcpt {
     my ($self, $transaction, $recipient) = @_;
-    return DECLINED
-      unless $recipient->host && $recipient->user;
 
-    return DECLINED
+    return DECLINED unless $recipient->host && $recipient->user;
-      unless lc($recipient->host) eq $self->{_domain};
+    return DECLINED if lc($recipient->host) ne $self->{_domain};
 
     my $rcpt = lc $recipient->user . '@' . lc $recipient->host;
-    return @{$self->{_default}}
-      unless exists $map{$rcpt};
 
+    return @{$self->{_default}} if ! exists $map{$rcpt};
     return @{$map{$rcpt}};
 }

t/auth.t

@@ -2,18 +2,16 @@
 use strict;
 use warnings;
 
-use lib 't';
-use lib 'lib';
-
 use Data::Dumper;
 use Digest::HMAC_MD5 qw(hmac_md5_hex);
 use English qw/ -no_match_vars /;
 use File::Path;
-
-use Qpsmtpd::Constants;
 use Scalar::Util qw( openhandle );
 use Test::More qw(no_plan);
 
+use lib 't';
+use lib 'lib';
+use Qpsmtpd::Constants;
 use_ok('Test::Qpsmtpd');
 use_ok('Qpsmtpd::Auth');
 
@@ -22,10 +20,6 @@ my ($smtpd, $conn) = Test::Qpsmtpd->new_conn();
 ok($smtpd, "get new connection ($smtpd)");
 isa_ok($conn, 'Qpsmtpd::Connection', "get new connection");
 
-#warn Dumper($smtpd) and exit;
-#my $hooks = $smtpd->hooks;
-#warn Dumper($hooks) and exit;
-
 my $r;
 my $user      = 'good@example.com';
 my $pass      = 'good_pass';