I have this following module using for adding and enabling/disabling Windows Firewall rules using Python.

I currently use subprocess.call to execute the netsh command inside Python. I'm wondering if there is any better method to do this? Executing the cmd command inside Python seems to be impractical to me.

import subprocess, ctypes, os, sys

from subprocess import Popen, DEVNULL



def chkAdmin():

    """ Force to start application with admin rights """

    try:

        isAdmin = ctypes.windll.shell32.IsUserAnAdmin()

    except AttributeError:

        isAdmin = False

    if not isAdmin:

        ctypes.windll.shell32.ShellExecuteW(None, "runas", sys.executable, __file__, None, 1)



def addRule(rule_name, file_path):

    """ Add rule to Windows Firewall """

    subprocess.call("netsh advfirewall firewall add rule name="+ rule_name +" dir=out action=block enable=no program=" + file_path, shell=True, stdout=DEVNULL, stderr=DEVNULL)

    print("Rule", rule_name, "for", file_path, "added")



def modifyRule(rule_name, state):

    """ Enable/Disable specific rule, 0 = Disable / 1 = Enable """

    if state:

        subprocess.call("netsh advfirewall firewall set rule name="+ rule_name +" new enable=yes", shell=True, stdout=DEVNULL, stderr=DEVNULL)

        print("Rule", rule_name, "Enabled")

    else:

        subprocess.call("netsh advfirewall firewall set rule name="+ rule_name +" new enable=no", shell=True, stdout=DEVNULL, stderr=DEVNULL)

        print("Rule", rule_name, "Disabled")



chkAdmin()

addRule("RULE_NAME", "PATH_TO_FILE")

modifyRule("RULE_NAME", 1)

This seems OK

We can add a little flavor to it:

1. Don't use string concatenation, but use f"{strings}" or "{}".format(strings)



2. 
   

   Your modify rule, can be simplified

   
   
   

   The if else don't differ that much, you can use a (Python)ternary to calculate the variables beforehand

   
   



3. Consider to chop up the lines, to make it a little more readable




4. Functions and variables should be snake_case according to PEP8




5. Use a if __name__ == '__main__' guard



6. 
   

   As mentioned, you could use os.system("command") instead of subprocess

   
   
   

   But honestly I would stick with subprocess, since it will give greater control over how commands are executed

   
   

Code

import subprocess, ctypes, os, sys

from subprocess import Popen, DEVNULL



def check_admin():

    """ Force to start application with admin rights """

    try:

        isAdmin = ctypes.windll.shell32.IsUserAnAdmin()

    except AttributeError:

        isAdmin = False

    if not isAdmin:

        ctypes.windll.shell32.ShellExecuteW(None, "runas", sys.executable, __file__, None, 1)



def add_rule(rule_name, file_path):

    """ Add rule to Windows Firewall """

    subprocess.call(

        f"netsh advfirewall firewall add rule name={rule_name} dir=out action=block enable=no program={file_path}", 

        shell=True, 

        stdout=DEVNULL, 

        stderr=DEVNULL

    )

    print(f"Rule {rule_name} for {file_path} added")



def modify_rule(rule_name, state):

    """ Enable/Disable specific rule, 0 = Disable / 1 = Enable """

    state, message = ("yes", "Enabled") if state else ("no", "Disabled")

    subprocess.call(

        f"netsh advfirewall firewall set rule name={rule_name} new enable={state}", 

        shell=True, 

        stdout=DEVNULL, 

        stderr=DEVNULL

    )

    print(f"Rule {rule_name} {message}")



if __name__ == '__main__':

    check_admin()

    add_rule("RULE_NAME", "PATH_TO_FILE")

    modify_rule("RULE_NAME", 1)

I agree with @Ludisposed answer, but you have a few subprocess gotchas:

• You don't need to spawn a shell in order to run the command, simply build your command as a list of arguments and it will be fine. This is especially important if your rules names may contains spaces as the command would be treated entirelly differently in your implementation;


• 
  Replace the old subprocess.call by subprocess.run;


• You may be interested to run subprocess.run by specifying check=True in order to generate an exception and be alerted if something does not go according to plan.

Applying these changes to e.g. modify_rule can lead to:

def modify_rule(rule_name, enabled=True):

    """Enable or Disable a specific rule"""

    subprocess.run(

        [

            'netsh', 'advfirewall', 'firewall',

            'set', 'rule', f'name={rule_name}',

            'new', f'enable={"yes" if enabled else "no"}',

        ],

        check=True,

        stdout=DEVNULL,

        stderr=DEVNULL

    )

Also note that I removed the print call from the function as it impairs reusability. If the caller want this kind of messages, it should be responsible for printing them, not this function.