Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the script handler to wait for the script #1307

Merged
merged 8 commits into from Nov 15, 2018
56 changes: 29 additions & 27 deletions privacyidea/lib/eventhandler/scripthandler.py
Expand Up @@ -33,6 +33,7 @@
"""
from privacyidea.lib.eventhandler.base import BaseEventHandler
from privacyidea.lib.config import get_from_config
from privacyidea.lib.error import ServerError
from privacyidea.lib import _
import json
import logging
Expand All @@ -50,18 +51,23 @@ class ScriptEventHandler(BaseEventHandler):
"""
An Eventhandler needs to return a list of actions, which it can handle.
It also returns a list of allowed action and conditions
It returns an identifier, which can be used in the eventhandlig definitions
It returns an identifier, which can be used in the eventhandling definitions
"""

identifier = "Script"
description = "This event handler can trigger external scripts."

try:
script_directory = get_from_config("PI_SCRIPT_HANDLER_DIRECTORY",
"/etc/privacyidea/scripts")
except RuntimeError:
# In case of the tests we are outside of the application context
script_directory = "tests/testdata/scripts"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

def __init__(self, script_directory=None):
if not script_directory:
try:
self.script_directory = get_from_config("PI_SCRIPT_HANDLER_DIRECTORY",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be get_from_config, i.e. to read from the config database table? From the look of the option, I would think it should rather be set in pi.cfg?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5fc5322

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"/etc/privacyidea/scripts")
except RuntimeError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use except RuntimeError as e: since Python 3 doesn't allow the old syntax.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in bea2309

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# In case of the tests we are outside of the application context
script_directory = "tests/testdata/scripts"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be self.script_directory :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


else:
self.script_directory = script_directory

@property
def allowed_positions(cls):
Expand Down Expand Up @@ -180,26 +186,22 @@ def do(self, action, options=None):
proc_args.append("--logged_in_role")
proc_args.append(logged_in_user.get("role", "none"))

if handler_options.get("background", SCRIPT_BACKGROUND) == SCRIPT_BACKGROUND:
try:
p = subprocess.Popen(proc_args, cwd=self.script_directory)
log.info("Started script {script!r} in background:"
" {process!r}".format(script=script_name, process=p))
except Exception as e:
log.warning("Failed to execute script {0!r}: {1!r}".format(
script_name, e))
log.warning(traceback.format_exc())

elif handler_options.get("background") == SCRIPT_WAIT:
try:
log.info("Starting script {script!r} in foreground.".format(script=script_name))
r = subprocess.check_call(proc_args, cwd=self.script_directory)
except Exception as e:
log.warning("Failed to execute script {0!r}: {1!r}".format(
script_name, e))
log.warning(traceback.format_exc())
if handler_options.get("raise_error"):
raise e
rcode = 0
try:
log.info("Starting script {script!r}.".format(script=script_name))
p = subprocess.Popen(proc_args, cwd=self.script_directory)
if handler_options.get("background") == SCRIPT_WAIT:
rcode = p.wait()

except Exception as e:
log.warning("Failed to execute script {0!r}: {1!r}".format(
script_name, e))
log.warning(traceback.format_exc())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we raise an error here if raise_error is True?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not raise an error before. I would like to keep it that way.
Raising an error could result in the request to fail. But imho the script is often just an addon, which should not interfer with the functinoality of e.g. the authentication.

Imagine an event handler, that is supposed to write a backup to a SMB mount on every 100th auth request. Now the SMB is down or someone change a password or deleted the script.
Full denial of service of your authentication ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I want to avoid that situation, I would probably set raise_error to False :-) If the script is important enough for me that I set raise_error=True, I would probably also want the request to fail if the script is deleted and cannot be executed because of that.

Anyway, I'm also OK with keeping this as it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are adding Feature requests in the reviews! ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently "raise_error" can only be set for "background" mode.
Should we change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e2b1d60 :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


if rcode and handler_options.get("raise_error"):
log.warning("Script {script!r} failed to execute with error code {error!r}".format(script=script_name,
error=rcode))
raise ServerError("Error during execution of the script.")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an else branch like

else:
    log.warning("Invalid value: {!r}".format(handler_options.get("background")))

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I will add an else branch, if raise_error is not set.
Then it would be great to have this merged ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return ret

9 changes: 7 additions & 2 deletions tests/test_lib_events.py
Expand Up @@ -7,6 +7,7 @@

import smtpmock
import responses
import os
from .base import MyTestCase, FakeFlaskG, FakeAudit
from privacyidea.lib.eventhandler.usernotification import (
UserNotificationEventHandler, NOTIFY_TYPE)
Expand Down Expand Up @@ -521,7 +522,9 @@ def test_01_runscript(self):
}

script_name = "ls.sh"
t_handler = ScriptEventHandler()
d = os.getcwd()
d = "{0!s}/tests/testdata/scripts/".format(d)
t_handler = ScriptEventHandler(script_directory=d)
res = t_handler.do(script_name, options=options)
self.assertTrue(res)
remove_token("SPASS01")
Expand Down Expand Up @@ -565,7 +568,9 @@ def test_02_failscript(self):
}

script_name = "fail.sh"
t_handler = ScriptEventHandler()
d = os.getcwd()
d = "{0!s}/tests/testdata/scripts/".format(d)
t_handler = ScriptEventHandler(script_directory=d)
self.assertRaises(Exception, t_handler.do, script_name, options=options)
remove_token("SPASS01")

Expand Down