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.

Sign up for GitHub

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

Jump to bottom

Allow the script handler to wait for the script #1307

Merged
merged 8 commits into from Nov 15, 2018
Merged

Allow the script handler to wait for the script #1307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
13c2c35
Allow the script handler to wait for the script
cornelinux Nov 12, 2018
8f306eb
Fix script handling
cornelinux Nov 13, 2018
040d0db
Fix script directory
cornelinux Nov 14, 2018
bea2309
Write to log, if an error occurs
cornelinux Nov 15, 2018
5fc5322
Modify exception to be python3 compatible
cornelinux Nov 15, 2018
83f6eaa
Fix config for script directory
cornelinux Nov 15, 2018
e2b1d60
Fix the checking for True in the event options
cornelinux Nov 15, 2018
7bd3d27
Raise an error if in the wait stait script fails to start.
cornelinux Nov 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 36 additions & 8 deletions privacyidea/lib/eventhandler/scripthandler.py
View file
Open in desktop
Expand Up @@ -42,6 +42,9 @@

log = logging.getLogger(__name__)

SCRIPT_BACKGROUND = "Run in background"
SCRIPT_WAIT = "Wait for script to finish"


class ScriptEventHandler(BaseEventHandler):
"""
Expand Down Expand Up @@ -80,6 +83,19 @@ def actions(cls):
actions = {}
for script in scripts:
actions[script] = {
"background": {
"type": "str",
"required": True,
"description": _("Wait for script to complete or run script in background. This will "
"either return the HTTP request early or could also block the request."),
"value": [SCRIPT_BACKGROUND, SCRIPT_WAIT]
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about changing the values of SCRIPT_BACKGROUND and SCRIPT_WAIT to "background" and "wait", respectively? We cannot localize the phrases "Run in background" and "Wait for script to finish" anyway, which would look weird in German.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

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.

👍

},
"raise_error": {
"type": "bool",
"visibleIf": "background",
"visibleValue": SCRIPT_WAIT,
"description": _("On script error raise exception in HTTP request.")
},
"serial": {
"type": "bool",
"description": _("Add '--serial <serial number>' as script "
Expand Down Expand Up @@ -164,14 +180,26 @@ def do(self, action, options=None):
proc_args.append("--logged_in_role")
proc_args.append(logged_in_user.get("role", "none"))

try:
p = subprocess.Popen(proc_args, cwd=self.script_directory)
log.info("Started script {script!r}:"
" {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())
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
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 is mergable, but just wanted to point out potential for code deduplication here: We could get rid of the outer if-branch and instead use

            try:
                p = subprocess.Popen(proc_args, cwd=self.script_directory)
                log.info("Started script {script!r}:"
                         " {process!r}".format(script=script_name, process=p))
                if handler_options.get("background") == SCRIPT_WAIT:
                    log.info("waiting for script to finish ...")
                    ret = p.wait()
                    # handle error case if ret != 0
                    ...
            except Exception as e:
                log.warning("Failed to execute script {0!r}: {1!r}".format(
                    script_name, e))
                log.warning(traceback.format_exc())

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not aware of p.wait(). The question is, if it also raises an error accordingly?
Then we would - if raise_error was configured - have to reraise the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, we would need to re-raise an error. We could, however, just raise a privacyIDEA ServerError then.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, that different situations would be handled differently. p.wait() does not raise an exception in case of an exit code <> 0.

The next problem is, that for some reason the tests do not work anymore - since the fail.sh script somehow does not work :-/

Copy link
Contributor

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.

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

49 changes: 46 additions & 3 deletions tests/test_lib_events.py
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@
UserNotificationEventHandler, NOTIFY_TYPE)
from privacyidea.lib.eventhandler.tokenhandler import (TokenEventHandler,
ACTION_TYPE, VALIDITY)
from privacyidea.lib.eventhandler.scripthandler import ScriptEventHandler
from privacyidea.lib.eventhandler.scripthandler import ScriptEventHandler, SCRIPT_WAIT, SCRIPT_BACKGROUND
from privacyidea.lib.eventhandler.counterhandler import CounterEventHandler
from privacyidea.models import EventCounter
from privacyidea.lib.eventhandler.federationhandler import FederationEventHandler
Expand Down Expand Up @@ -511,12 +511,12 @@ def test_01_runscript(self):
"request": req,
"response": resp,
"handler_def": {
"options":{
"options": {
"user": "1",
"realm": "1",
"serial": "1",
"logged_in_user": "1",
"logged_in_role": "1",}
"logged_in_role": "1"}
}
}

Expand All @@ -526,6 +526,49 @@ def test_01_runscript(self):
self.assertTrue(res)
remove_token("SPASS01")

def test_02_failscript(self):
g = FakeFlaskG()
audit_object = FakeAudit()
audit_object.audit_data["serial"] = "SPASS01"

g.logged_in_user = {"username": "admin",
"role": "admin",
"realm": ""}
g.audit_object = audit_object

builder = EnvironBuilder(method='POST',
data={'serial': "SPASS01"},
headers={})

env = builder.get_environ()
# Set the remote address so that we can filter for it
env["REMOTE_ADDR"] = "10.0.0.1"
g.client_ip = env["REMOTE_ADDR"]
req = Request(env)
req.all_data = {"serial": "SPASS01", "type": "spass"}
req.User = User()
resp = Response()
resp.data = """{"result": {"value": true}}"""

options = {"g": g,
"request": req,
"response": resp,
"handler_def": {
"options": {
"background": SCRIPT_WAIT,
"raise_error": True,
"realm": "1",
"serial": "1",
"logged_in_user": "1",
"logged_in_role": "1"}
}
}

script_name = "fail.sh"
t_handler = ScriptEventHandler()
self.assertRaises(Exception, t_handler.do, script_name, options=options)
remove_token("SPASS01")


class FederationEventTestCase(MyTestCase):

Expand Down
2 changes: 2 additions & 0 deletions tests/testdata/scripts/fail.sh
View file
Open in desktop
@@ -0,0 +1,2 @@
#!/bin/bash
exit 1