Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hello Wise People,

I written the peice of code below but i am not sure if it is very good coding (as its my first go at doing perl). Its all in a bit of a jumble and i don't know if there are better ways of doing the tings i am trying to do. Help would be appreciated very much (because as you can see, i can't even lay it out properly).


#!/usr/bin/perl

#Grab Input From Command Line
use strict;
use Getopt::Long;
my ($username, $domain, $password, $email, $ip, $service);
my $result = GetOptions("username=s" => \$username,
                        "domain=s" => \$domain,
                        "email=s" => \$email,
                        "ip=s" => \$ip,
                        "service=s" => \$service);

#Generate Random Password And Encrypt It
my @chars = ('a'..'z', 'A'..'Z', 0..9);
my $plaintext_pass = do {{
  local $_ = join "" => map {$chars [rand @chars]} 1..8;
  redo unless /[a-z]/ && /[A-Z]/ && /\d/;
  $_;
}};
my $salt = join '', 
   ('.', '/', 0..9, 
   'A'..'Z', 'a'..'z')[rand 64, rand 64];
my $encrypted_pass = crypt($plaintext_pass, $salt);

#Take Current System Date And Add Two Weeks On For Trial
use constant WEEK => 60 * 60 * 24 * 7 ;
my @later = localtime( time + ( WEEK * 2 ) ) ;
my $expiry_date = sprintf "%04d-%02d-%02d",
                 $later[5] + 1900,
                 $later[4] + 1,
                 $later[3] ;

#Add User
if (!system("adduser $username -g 100 -s /bin/false -d /home/$username
+ -p $encrypted_pass -e $expiry_date")) {

  #Edit Postfix Virtual File
  my $datafile = '/etc/postfix/virtual' ;
  my $req_addr = "$username\@$domain" ;
  my ( $name, $domain ) = $req_addr =~ /^([\w.-]+)(@[\w.-]+)$/ ;
  open FH, $datafile or die "Couldn't read $datafile: $!" ;
  my @virtual = <FH> ;
  close FH ;
  open FH, ">$datafile" or die "Couldn't write $datafile: $!" ;
  my $wrote_it = 0 ;
  for ( @virtual )
  {
    print FH $_ ;
    if ( $_ =~ /$domain/ && !$wrote_it )
    {
            print FH "$req_addr\t$username\n" ;
      $wrote_it++ ;
    }
  }
  close FH ;
  system ("postmap /etc/postfix/virtual");
  #Send Emails
  open (MAIL, "|/usr/sbin/sendmail -t");
  print MAIL "To: bar\@foo.com\n";
  print MAIL "Reply-to: $email\n";
  print MAIL "From: $email\n";
  print MAIL "Subject: AP3k New User Signup\n";
  print MAIL "\n\n";
  print MAIL "Test Mail1\n";
  print MAIL "\n\n";
  close (MAIL);

  open (MAIL, "|/usr/sbin/sendmail -t");
  print MAIL "To: $email\n";
  print MAIL "Reply-to: bar\@foo.com\n";
  print MAIL "From: foo\@bar.com\n";
  print MAIL "Subject: Test\n";
  print MAIL "\n\n";
  print MAIL "Test Mail2\n";
  close (MAIL);
  print "Account Created Successfully\n";
  exit 0;
} else {
  print "Account Error\n";
  exit 122;
}

[download]


All Of The Variables Have Been Sanitised By Another Perl Script. Thanks

Replies are listed 'Best First'.
Re: Desperate Coding Tidy-up
by grinder (Bishop) on Aug 29, 2002 at 08:16 UTC

    For generating passwords, you might want to take a look at spew - print out random characters, which will generate the desired number of random characters from the set of characters you choose.

    Your "two weeks in the future" code could be more tightly written as:

    use constant WEEK => 60 * 60 * 24 * 7;
my $expiry_date = sprintf '%04d-%02d-%02d',
  sub { $_[5]+1900, $_[4]+1, $_[3] }->(localtime(time + 2 * WEEK));

    [download]

    The code to update the postfix virtual table looks possibly more complicated than it should be, but I'm not clear on your intent. What are you trying to do?

    You should use Mail::Sendmail (which, despite its name, does not rely on sendmail(1)), or another mail sending module, rather than attacking the sendmail program directly. Instead of a row of print statements, you should use a heredoc:

    print MAIL <<"END_OF_MAIL";
To: bar\@foo.com
Reply-to: $email
From: $email
Subject: AP3k New User Signup

Test Mail1

END_OF_MAIL

    [download]
    All Of The Variables Have Been Sanitised By Another Perl Script

    That means nothing. You should re-sanitise them again in this script.


    print@_{sort keys %_},$/if%_=split//,'= & *a?b:e\f/h^h!j+n,o@o;r$s-t%t#u'
[reply]
[d/l]
[select]

      I have to admit that I really like that anonymous sub idiom you use. I saw it in your post the other day and upvoted the node just because of that.

      I also have to admit that, seeing it here next to the more obvious approach, it loses a bit of its appeal to me. It really doesn't buy you much and it's a little hard on the eyes.

      Don't get me wrong. I still like it. I'm just going to have to search for other places to use it. :-)

      -sauoq
"My two cents aren't worth a dime.";
[reply]
      Hi There,

      Thanks for the pointers and snippets, they are much appreciated.

      The postfix file looks like this:
      
foo.com        VIRTUALDOMAIN
@foo.com       bob
bob@foo.com       bob
chris@foo.com       chris

bar.com        VIRTUALDOMAIN
@bar.com       bob
bob@bar.com       bob
chris@bar.com       chris

      [download]

      I need to: use the variable $domain which will contain e.g "foo.com", seek to the line that starts with foo.com, move down two lines, do a return, and then print $username\@$ddomain\t$username. So if the username is don and the domain is bar.com, i need the code to locate the line starting with bar.com, drop down to lines, do a return and then print so the postfix file will then look like: 
      foo.com        VIRTUALDOMAIN
@foo.com       bob
bob@foo.com       bob
chris@foo.com       chris

bar.com        VIRTUALDOMAIN
@bar.com       bob
don@bar.com    don
bob@bar.com       bob
chris@bar.com       chris

      [download]

      Cheers
[reply]
[d/l]
[select]
Re: Desperate Coding Tidy-up
by sauoq (Abbot) on Aug 29, 2002 at 09:03 UTC

    Overall, it's pretty good. Good enough that I have a hard time believing your claim that this is your "first go at doing perl." :-)

    If you don't want to use grinder's spew script (say you don't have /dev/urandom for instance) you might consider making this: 

    my @chars = ('a'..'z', 'A'..'Z', 0..9);
my $plaintext_pass = do {{
  local $_ = join "" => map {$chars [rand @chars]} 1..8;
  redo unless /[a-z]/ && /[A-Z]/ && /\d/;
  $_;
}};

    [download]
    a little more readable like this: 
    my @chars = ('a'..'z', 'A'..'Z', 0 .. 9);
my $plaintext_pass;
PICK: {
    $plaintext_pass = '';
    $plaintext_pass .= $chars[rand @chars] for 1..8;
    /[a-z]/ && /[A-Z]/ && /\d/ or redo PICK for $plaintext_pass;
}

    [download]
    shrug

    You might also want to reuse @chars in the list you use when picking the salt.

    I agree with grinder's suggestion that using a module to send the mail is cleaner. I also agree with his suggestion that you re-scrub your variables.

    Edit: Password picking code fixed to squash the bug that Arien pointed out in his reply. Arien++

    -sauoq
"My two cents aren't worth a dime.";
[reply]
[d/l]
[select]
      my @chars = ('a'..'z', 'A'..'Z', 0 .. 9);
my $plaintext_pass;
PICK: {
    $plaintext_pass .= $chars[rand @chars] for 1..8;
    /[a-z]/ && /[A-Z]/ && /\d/ or redo PICK for $plaintext_pass;
}

      [download]

      Your password will be multiples of eight in length. (You append 8 chars every time you redo PICK.)

      — Arien

[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom