Allow the script handler to wait for the script #1307

cornelinux · 2018-11-12T15:50:30Z

Script handles can now be configured to wait for the
script to execute.
The default behaviour is to put the script in the background.

Closes #1222

General Comment
# Codecov Report

Merging #1307 into master will increase coverage by 0.1%.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##           master    #1307     +/-   ##
=========================================
+ Coverage   95.81%   95.91%   +0.1%
=========================================
Files         142      142
Lines       17233    17839    +606
=========================================
+ Hits        16512    17111    +599
- Misses        721      728      +7

Impacted Files	Coverage Δ
privacyidea/lib/config.py	`94.83% <ø> (+0.02%)`	⬆️
privacyidea/lib/eventhandler/scripthandler.py	`81.57% <81.81%> (-5.31%)`	⬇️
privacyidea/lib/log.py	`34.24% <0%> (-9.59%)`	⬇️
privacyidea/lib/tokens/u2f.py	`94.16% <0%> (-1.67%)`	⬇️
privacyidea/lib/policy.py	`99.84% <0%> (+0.43%)`	⬆️
privacyidea/lib/subscriptions.py	`87.7% <0%> (+0.81%)`	⬆️
privacyidea/lib/utils.py	`97.33% <0%> (+0.88%)`	⬆️
privacyidea/api/lib/postpolicy.py	`97.64% <0%> (+0.99%)`	⬆️
privacyidea/lib/error.py	`91.91% <0%> (+1.01%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1d0351...7bd3d27. Read the comment docs.

File: privacyidea/lib/eventhandler/scripthandler.py:L180-206
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?

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.
You're right, we would need to re-raise an error. We could, however, just raise a privacyIDEA ServerError then.
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 :-/
File: privacyidea/lib/eventhandler/scripthandler.py:L180-206
Should we add an else branch like

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

?

No, but I will add an else branch, if raise_error is not set.
Then it would be great to have this merged ;-)
done
File: privacyidea/lib/eventhandler/scripthandler.py:L51-74
I think this should be self.script_directory :-)
True!
👍
done
File: privacyidea/lib/eventhandler/scripthandler.py:L43-74
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?
Done in 5fc5322
File: privacyidea/lib/eventhandler/scripthandler.py:L43-74
Please use except RuntimeError as e: since Python 3 doesn't allow the old syntax.
Done in bea2309
File: privacyidea/lib/eventhandler/scripthandler.py:L89-108
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.
ok
Done in 5fc5322
File: privacyidea/lib/eventhandler/scripthandler.py:L186-209
This is missing a is_true, I think
No. If raise error is set, then we will raise an error.
If it is not set, we will not raise an error - right? Or am I missing something?
If the option is disabled, handler_options.get("raise_error") is actually the string "False", so we need to use is_true. I forgot about that too :-)
Thanks. Done in 83f6eaa
File: privacyidea/lib/eventhandler/scripthandler.py:L186-209
Should we raise an error here if raise_error is True?
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 ;-)
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.
You are adding Feature requests in the reviews! ;-)
Currently "raise_error" can only be set for "background" mode.
Should we change this?
Done in e2b1d60 :-)

Script handles can now be configured to wait for the script to execute. The default behaviour is to put the script in the background. Closes #1222

codecov · 2018-11-12T16:11:40Z

Codecov Report

Merging #1307 into master will increase coverage by 0.1%.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##           master    #1307     +/-   ##
=========================================
+ Coverage   95.81%   95.91%   +0.1%     
=========================================
  Files         142      142             
  Lines       17233    17839    +606     
=========================================
+ Hits        16512    17111    +599     
- Misses        721      728      +7

Impacted Files	Coverage Δ
privacyidea/lib/config.py	`94.83% <ø> (+0.02%)`	⬆️
privacyidea/lib/eventhandler/scripthandler.py	`81.57% <81.81%> (-5.31%)`	⬇️
privacyidea/lib/log.py	`34.24% <0%> (-9.59%)`	⬇️
privacyidea/lib/tokens/u2f.py	`94.16% <0%> (-1.67%)`	⬇️
privacyidea/lib/policy.py	`99.84% <0%> (+0.43%)`	⬆️
privacyidea/lib/subscriptions.py	`87.7% <0%> (+0.81%)`	⬆️
privacyidea/lib/utils.py	`97.33% <0%> (+0.88%)`	⬆️
privacyidea/api/lib/postpolicy.py	`97.64% <0%> (+0.99%)`	⬆️
privacyidea/lib/error.py	`91.91% <0%> (+1.01%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1d0351...7bd3d27. Read the comment docs.

fredreichbier · 2018-11-13T10:28:09Z

privacyidea/lib/eventhandler/scripthandler.py

+                    script_name, e))
+                log.warning(traceback.format_exc())
+                if handler_options.get("raise_error"):
+                    raise e


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?

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.

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

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 :-/

fredreichbier · 2018-11-13T10:29:10Z

privacyidea/lib/eventhandler/scripthandler.py

+                    script_name, e))
+                log.warning(traceback.format_exc())
+                if handler_options.get("raise_error"):
+                    raise e



Should we add an else branch like

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

?

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

Closes #1222

fredreichbier · 2018-11-13T16:40:56Z

privacyidea/lib/eventhandler/scripthandler.py

+                                                        "/etc/privacyidea/scripts")
+            except RuntimeError:
+                # In case of the tests we are outside of the application context
+                script_directory = "tests/testdata/scripts"


I think this should be self.script_directory :-)

cornelinux · 2018-11-15T13:32:54Z

privacyidea/lib/eventhandler/scripthandler.py

-                                           "/etc/privacyidea/scripts")
-    except RuntimeError:
-        # In case of the tests we are outside of the application context
-        script_directory = "tests/testdata/scripts"


plettich · 2018-11-15T13:54:52Z

privacyidea/lib/eventhandler/scripthandler.py

+            try:
+                self.script_directory = get_from_config("PI_SCRIPT_HANDLER_DIRECTORY",
+                                                        "/etc/privacyidea/scripts")
+            except RuntimeError:


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

Done in bea2309

fredreichbier · 2018-11-15T13:53:26Z

privacyidea/lib/eventhandler/scripthandler.py

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


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?

Done in 5fc5322

fredreichbier · 2018-11-15T14:02:12Z

privacyidea/lib/eventhandler/scripthandler.py

+                    "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]


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.

Done in 5fc5322

fredreichbier · 2018-11-15T14:02:24Z

privacyidea/lib/eventhandler/scripthandler.py

+        if rcode:
+            log.warning("Script {script!r} failed to execute with error code {error!r}".format(script=script_name,
+                                                                                               error=rcode))
+            if handler_options.get("raise_error"):


This is missing a is_true, I think

No. If raise error is set, then we will raise an error.
If it is not set, we will not raise an error - right? Or am I missing something?

If the option is disabled, handler_options.get("raise_error") is actually the string "False", so we need to use is_true. I forgot about that too :-)

Thanks. Done in 83f6eaa

fredreichbier · 2018-11-15T14:02:41Z

privacyidea/lib/eventhandler/scripthandler.py

-                     " {process!r}".format(script=script_name, process=p))
+            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())


Should we raise an error here if raise_error is True?

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 ;-)

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.

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

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

Done in e2b1d60 :-)

plettich · 2018-11-15T14:07:26Z

privacyidea/lib/eventhandler/scripthandler.py

+            try:
+                self.script_directory = get_from_config("PI_SCRIPT_HANDLER_DIRECTORY",
+                                                        "/etc/privacyidea/scripts")
+            except RuntimeError:


rename defines for wait and background

fredreichbier · 2018-11-15T15:13:27Z

privacyidea/lib/eventhandler/scripthandler.py

-                                           "/etc/privacyidea/scripts")
-    except RuntimeError:
-        # In case of the tests we are outside of the application context
-        script_directory = "tests/testdata/scripts"


fredreichbier · 2018-11-15T15:13:28Z

privacyidea/lib/eventhandler/scripthandler.py

+                    "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]


fredreichbier · 2018-11-15T15:13:28Z

privacyidea/lib/eventhandler/scripthandler.py

+                    script_name, e))
+                log.warning(traceback.format_exc())
+                if handler_options.get("raise_error"):
+                    raise e


fredreichbier · 2018-11-15T15:13:29Z

privacyidea/lib/eventhandler/scripthandler.py

+                    script_name, e))
+                log.warning(traceback.format_exc())
+                if handler_options.get("raise_error"):
+                    raise e



fredreichbier · 2018-11-15T15:13:30Z

privacyidea/lib/eventhandler/scripthandler.py

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


fredreichbier · 2018-11-15T15:13:31Z

privacyidea/lib/eventhandler/scripthandler.py

+        if rcode:
+            log.warning("Script {script!r} failed to execute with error code {error!r}".format(script=script_name,
+                                                                                               error=rcode))
+            if handler_options.get("raise_error"):


Allow the script handler to wait for the script

13c2c35

Script handles can now be configured to wait for the script to execute. The default behaviour is to put the script in the background. Closes #1222

cornelinux requested a review from a team November 12, 2018 15:50

fredreichbier reviewed Nov 13, 2018

View reviewed changes

Fix script handling

8f306eb

Closes #1222

fredreichbier reviewed Nov 13, 2018

View reviewed changes

Fix script directory

040d0db

cornelinux commented Nov 15, 2018

View reviewed changes

Write to log, if an error occurs

bea2309

plettich reviewed Nov 15, 2018

View reviewed changes

Modify exception to be python3 compatible

5fc5322

fredreichbier suggested changes Nov 15, 2018

View reviewed changes

plettich reviewed Nov 15, 2018

View reviewed changes

cornelinux added 3 commits November 15, 2018 15:25

Fix config for script directory

83f6eaa

rename defines for wait and background

Fix the checking for True in the event options

e2b1d60

Raise an error if in the wait stait script fails to start.

7bd3d27

fredreichbier reviewed Nov 15, 2018

View reviewed changes

fredreichbier merged commit f21955e into master Nov 15, 2018

fredreichbier deleted the 1222/wait-for-script branch November 15, 2018 15:49

fredreichbier mentioned this pull request Nov 19, 2018

Handle non-existent resources more strictly #1310

Merged

10 tasks

Allow the script handler to wait for the script #1307

Allow the script handler to wait for the script #1307

Conversation

cornelinux commented Nov 12, 2018 • edited

codecov bot commented Nov 12, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cornelinux commented Nov 12, 2018 •

edited

codecov bot commented Nov 12, 2018 •

edited