2
\$\begingroup\$

I've just solve one problem that notifies users if their password is going to expire in 14 days. The script is working fine. I just want to improve my code and desire some suggestions.

#!/usr/bin/env python
# Author :- Rahul Patil<linuxian.com>
# 

import sys
import os

#-----------------------------------------------------------------
# Provide Ldap DN Details to Perform Ldap Search using anonymouse
#-----------------------------------------------------------------
Domain = 'linuxian.com'
EmailDomain = 'abc.om' # if your mail domain is different this will becom user@abc.com 
ConnectDC = 'ldap://localhost:389'
# if 14 days remains to expire password then it will send email to that user 
# until user update the new password
PwdWarnDays = 14
pwdMaxAge = 45 # default password expire in 45 days as per ldap ppolicy  

Subject = "Ldap Password Expiry Details"

MsgBody = """
Dear %s,
    Your Password Will be Expire in %s, we request you to please
change your password, your last password change date is %s

Best Regards,
Linux Admin
"""


def GetUserDetails():
        """ This Function Will save all details in file
        it will use ldap search query for the same."""
        # Create bind dn eg. dc=example,dc=com
        BindDN = ','.join([ 'dc=' + d for d in Domain.split('.') ])
        #
        import ldap 
        l = ldap.initialize(ConnectDC)
        # Perform Ldap Search
        return  l.search_s(
                BindDN,
                ldap.SCOPE_SUBTREE,
                '(uid=*)',['uid','pwdChangedTime']
            )


def CheckExpiry():
        """ 
        This Function will Check each user ExpiryWarning
        if it more thans WarningDays then it will send Emails
        to that particuler user
        """
         import datetime
         for k,v in Users:
                    uid = ''.join(v['uid'])
                    if 'pwdChangedTime' not in v:
                            pass
                            #print "User " + uid + "  not Updated Password" 
                    try:
                          l = ''.join(v['pwdChangedTime'])
                    except:
                            pass

                    if 'pwdChangedTime' in v:
                        # To extrace year month day
                        d1 = datetime.date.today()
                        d2 = datetime.date(int(l[0:4]),int(l[4:6]),int(l[6:8]))
                        DaysOfPasswordChange = (d1 - d2).days
                        d2 = d2.strftime('%d, %b %Y')

                        ExpireIn = pwdMaxAge - DaysOfPasswordChange

                        # if password not changed before 14 days 
                        if ExpireIn <= PwdWarnDays:
                            SendMail = "echo '" + MsgBody % (uid,ExpireIn,d2) + "' \
                                      mail -s " + '"' + Subject + '"' + ' ' + \
                                      uid + '@' + EmailDomain 
                            #os.system(SendMail)
                            print SendMail



if __name__ == '__main__':
        Users = GetUserDetails()
        CheckExpiry()
\$\endgroup\$
6
\$\begingroup\$

I'm just going to comment on these three lines:

SendMail = "echo '" + MsgBody % (uid,ExpireIn,d2) + "' \
          mail -s " + '"' + Subject + '"' + ' ' + \
          uid + '@' + EmailDomain 
#os.system(SendMail)
print SendMail
  1. This doesn't work! You've commented out the line that sends the e-mail. Now, I understand that you did this so that you can test the script without spamming your users with these messages, and that you intend to uncomment the line when you deploy the script. But this isn't the best approach.

    The first problem is that the need to test the script will come up again after you deploy it — you'll need to revise the text of the messages or change some other aspect the way it works, and then you'll want to test it again. So it would be worth your while to build in a permanent testing mechanism that's better than commenting out a line.

    The second problem is that just printing the mail command doesn't actually constitute a test. You have to read the output in order to check that it's correct, and you could easily miss a problem.

    What I suggest is that you have a setting that can be turned on and off to indicate whether the script is in testing or deployment:

    deployed = False           # Is script deployed?
    test_uid = 'rahul.patil'   # Send e-mail to this user if not deployed.
    

    and that you send the e-mail to the test user when testing:

    if not deployed:
        uid = test_uid
    SendMail = "echo '" + MsgBody % (uid,ExpireIn,d2) + "' \
              mail -s " + '"' + Subject + '"' + ' ' + \
              uid + '@' + EmailDomain 
    os.system(SendMail)
    

    Ideally you would provide a way to set the deployed flag (and the test_uid) through the script's command-line interface — see the argparse module for one approach to doing this.

  2. When you run a command using os.system, the command gets interpreted by the shell. This is risky, because the shell has complicated quoting and evaluation rules that you may not understand. For example, suppose that you edited MsgBody like this:

    MsgBody = """
    Dear %s,
         Your password will expire in %s day(s). We're sorry for the
    inconvenience, but we need you to change your password by %s.
    """
    

    Looks good, right? But if you actually try this, you'll find that the call to os.system fails with an error like this:

    sh: line 3: inconvenience,: command not found
    sh: -c: line 4: unexpected EOF while looking for matching `''
    sh: -c: line 5: syntax error: unexpected end of file
    

    That's because the single quote in We're was interpreted by the shell as terminating the argument to echo.

    In order to avoid problems like this (where data gets wrongly interpreted as code by the shell), you should avoid passing the arguments to the shell. Instead of os.system, you should use the functions in the subprocess module. For example, like this:

    import subprocess
    
    p = subprocess.Popen(['mail', '-s', Subject, uid + '@' + EmailDomain],
                         stdin=subprocess.PIPE)
    p.communicate(MsgBody % (uid, ExpireIn, d2))
    
\$\endgroup\$
  • \$\begingroup\$ thanks for your reply , it is useful, but I want to say that mail command is working without any issue, I have tested.. may be because of my Luck :P \$\endgroup\$ – Rahul Patil Oct 9 '13 at 16:06
  • 1
    \$\begingroup\$ That's fine for now, but what about later, after you or some other developer makes a change? \$\endgroup\$ – Gareth Rees Oct 9 '13 at 16:08
  • 1
    \$\begingroup\$ Better yet, send mail using a library call instead of a shell command. \$\endgroup\$ – 200_success Oct 9 '13 at 16:11
  • 1
    \$\begingroup\$ @200_success: I did consider that, but it's not all that compelling in a simple case like this one. (A couple of lines with subprocess versus eight lines with email and smtplib.) \$\endgroup\$ – Gareth Rees Oct 9 '13 at 16:15
  • \$\begingroup\$ @GarethRees I'm adopting your suggestion into my code, that's working, can you please help me with print that mail command on terminal \$\endgroup\$ – Rahul Patil Oct 9 '13 at 16:33
4
\$\begingroup\$

You should read through the Python Style Guide. You code does not follow the naming conventions most Python code uses.


Single letter variables should be used sparingly because they might not be as intuitive as you think. A more descriptive name is better.


Imports should generally be done at the top of the file.


string.format() would likely me more readable than the string concatenation you are doing at the end of CheckExpiry().


Your docstrings start with "This Function Will". This is unnecessary since documentation will always be describing what the function does.


At the beginning of your script, you define constants that set the date boundaries. This is good. However, you then repeatably use the value in comments. This is bad because if the policy changes to warn at 21 days, the comments need to be updated in addition to the constant's value.


You don't need a comment for every operation. In general, comments are better at saying why you are doing something, not saying what you are doing.

# if password not changed before 14 days 
if ExpireIn <= PwdWarnDays:

You have good variable names, so this line of code explains itself, the comment doesn't add anything. If you have a line of code that is complicated enough that you feel you need to document what it is doing, this is an indication that there might be a better way to do it.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much.., Amazing answer, I will follow that \$\endgroup\$ – Rahul Patil Oct 9 '13 at 14:58
2
\$\begingroup\$

I would add "bind_s" to connect to ldap with a user

user_dn = 'someuser'
user_pwd = 'somepassword'
l = ldap.initialize(ConnectDC)
l.bind_s(user_dn, user_pwd) #LDAP admin would not allow to query pwdChangedTime as anonymous

and change some code to make it more beautiful when sending mail:

return l.search_s(bind_dn, ldap.SCOPE_SUBTREE, '(uid=*)', ['displayName', 'pwdChangedTime', 'uid'])

for k, v in Users:
    uid = ''.join(v['uid'])
    if 'displayName' in v:
        displayname = ''.join(v['displayName'])
    else:
        displayname = uid
...
if 'pwdChangedTime' in v:
...
else:
    d2 = '[You have not changed your password since account created]'
...
print mail_body % (displayname, pwd_expire_in_days, d2)
\$\endgroup\$
1
\$\begingroup\$

I would separate the function definitions from the script functionality with if __name__ == '__main__': as is customary for Python. Also I would have CheckExpiry() take "Users" as an argument and return a list of expired users (or a tuple of name and e-mail address for each). So that would read more like:

#!python
# ....
if __name__ == '__main__':
    userdata = GetUserDetails()
    expired = CheckExpiry(userdata)
# ...

From there I'd create a template (perhaps as simple as one using string.Template from the Python standard libraries, or possibly using something like Jinja2 ... and use it to generate the messages to each user.

If you really want to get fancy you could track the number of warnings you've sent to each user, perhaps in a very simple local flat file database using SQLite3 (also from the standard library).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.