[HOTFIX-#264] Cherry-Pick FIX-#192

scenario_player/exceptions/cli.py

@@ -0,0 +1,8 @@
+from scenario_player.exceptions import ScenarioError
+
+
+class WrongPassword(ScenarioError):
+    """
+    Generic Error that gets raised if eth_keystore raises ValueError("MAC mismatch")
+    Usually that's caused by an invalid password
+    """

scenario_player/main.py

@@ -24,6 +24,7 @@
 from raiden.utils.cli import EnumChoiceType
 from scenario_player import tasks
 from scenario_player.exceptions import ScenarioAssertionError, ScenarioError
+from scenario_player.exceptions.cli import WrongPassword
 from scenario_player.exceptions.services import ServiceProcessException
 from scenario_player.runner import ScenarioRunner
 from scenario_player.services.common.app import ServiceProcess
@@ -35,6 +36,7 @@
     post_task_state_to_rc,
     send_notification_mail,
 )
+from scenario_player.utils.legacy import MutuallyExclusiveOption
 from scenario_player.utils.logs import (
     pack_n_latest_logs_for_scenario_in_dir,
     pack_n_latest_node_logs_in_dir,
@@ -89,6 +91,22 @@ def load_account_obj(keystore_file, password):
         return account
 
 
+def get_password(password, password_file):
+    if password_file:
+        password = open(password_file, "r").read().strip()
+    if password == password_file is None:
+        password = click.prompt(text="Please enter your password: ", hide_input=True)
+    return password
+
+
+def get_account(keystore_file, password):
+    try:
+        account = load_account_obj(keystore_file, password)
+    except ValueError:
+        raise WrongPassword
+    return account
+
+
 @click.group(invoke_without_command=True, context_settings={"max_content_width": 120})
 @click.option(
     "--data-path",
@@ -116,7 +134,20 @@ def main(ctx, chains, data_path):
 @main.command(name="run")
 @click.argument("scenario-file", type=click.File(), required=False)
 @click.option("--keystore-file", required=True, type=click.Path(exists=True, dir_okay=False))
-@click.password_option("--password", envvar="ACCOUNT_PASSWORD", required=True)
+@click.option(
+    "--password-file",
+    type=click.Path(exists=True, dir_okay=False),
+    cls=MutuallyExclusiveOption,
+    mutually_exclusive=["password"],
+    default=None,
+)
+@click.option(
+    "--password",
+    envvar="ACCOUNT_PASSWORD",
+    cls=MutuallyExclusiveOption,
+    mutually_exclusive=["password-file"],
+    default=None,
+)
 @click.option("--auth", default="")
 @click.option("--mailgun-api-key")
 @click.option(
@@ -133,7 +164,15 @@ def main(ctx, chains, data_path):
 )
 @click.pass_context
 def run(
-    ctx, mailgun_api_key, auth, password, keystore_file, scenario_file, notify_tasks, enable_ui
+    ctx,
+    mailgun_api_key,
+    auth,
+    password,
+    keystore_file,
+    scenario_file,
+    notify_tasks,
+    enable_ui,
+    password_file,
 ):
     scenario_file = Path(scenario_file.name).absolute()
     data_path = ctx.obj["data_path"]
@@ -142,7 +181,9 @@ def run(
     log_file_name = construct_log_file_name("run", data_path, scenario_file)
     configure_logging_for_subcommand(log_file_name)
 
-    account = load_account_obj(keystore_file, password)
+    password = get_password(password, password_file)
+
+    account = get_account(keystore_file, password)
 
     notify_tasks_callable = None
     if notify_tasks is TaskNotifyType.ROCKETCHAT:
@@ -239,20 +280,34 @@ def run(
 
 @main.command(name="reclaim-eth")
 @click.option("--keystore-file", required=True, type=click.Path(exists=True, dir_okay=False))
-@click.password_option("--password", envvar="ACCOUNT_PASSWORD", required=True)
+@click.option(
+    "--password-file",
+    type=click.Path(exists=True, dir_okay=False),
+    cls=MutuallyExclusiveOption,
+    mutually_exclusive=["password"],
+    default=None,
+)
+@click.option(
+    "--password",
+    envvar="ACCOUNT_PASSWORD",
+    cls=MutuallyExclusiveOption,
+    mutually_exclusive=["password-file"],
+    default=None,
+)
 @click.option(
     "--min-age",
     default=72,
     show_default=True,
     help="Minimum account non-usage age before reclaiming eth. In hours.",
 )
 @click.pass_context
-def reclaim_eth(ctx, min_age, password, keystore_file):
+def reclaim_eth(ctx, min_age, password, password_file, keystore_file):
     from scenario_player.utils import reclaim_eth
 
     data_path = ctx.obj["data_path"]
     chain_rpc_urls = ctx.obj["chain_rpc_urls"]
-    account = load_account_obj(keystore_file, password)
+    password = get_password(password, password_file)
+    account = get_account(keystore_file, password)
 
     configure_logging_for_subcommand(construct_log_file_name("reclaim-eth", data_path))
 

scenario_player/utils/legacy.py

@@ -93,6 +93,27 @@ def convert(self, value, param, ctx):  # pylint: disable=unused-argument
         return name, rpc_url
 
 
+class MutuallyExclusiveOption(click.Option):
+    def __init__(self, *args, **kwargs):
+        self.mutually_exclusive = set(kwargs.pop("mutually_exclusive", []))
+        help = kwargs.get("help", "")
+        if self.mutually_exclusive:
+            ex_str = ", ".join(self.mutually_exclusive)
+            kwargs["help"] = help + (
+                " NOTE: This argument is mutually exclusive with " " arguments: [" + ex_str + "]."
+            )
+        super(MutuallyExclusiveOption, self).__init__(*args, **kwargs)
+
+    def handle_parse_result(self, ctx, opts, args):
+        if self.mutually_exclusive.intersection(opts) and self.name in opts:
+            raise click.UsageError(
+                f"Illegal usage: {self.name} is mutually exclusive with "
+                f"arguments {', '.join(self.mutually_exclusive)}."
+            )
+
+        return super(MutuallyExclusiveOption, self).handle_parse_result(ctx, opts, args)
+
+
 class HTTPExecutor(mirakuru.HTTPExecutor):
     def start(self, stdout=subprocess.PIPE, stderr=subprocess.PIPE):
         """ Merged copy paste from the inheritance chain with modified stdout/err behaviour """

tests/unittests/cli/keystore/UTC--1

@@ -0,0 +1 @@
+{"id":"e66b03f8-4ce7-8f9b-b5d1-36dc9d5154ba","version":3,"crypto":{"cipher":"aes-128-ctr","cipherparams":{"iv":"49b8891b02604fb05884280509b82fbb"},"ciphertext":"4ce3a7059820b516746d7cc5defca17c2a54ea9dbb7e3a5411a01630b5021244","kdf":"pbkdf2","kdfparams":{"c":10240,"dklen":32,"prf":"hmac-sha256","salt":"9ef5e56c835cf59f4c9dbe6bd6a88b11056d6f01c0590c17d1cbb1653f749e7a"},"mac":"861354d5dd3efb2c6470090fb901957c53035175ababc9dc1863f5c07572d33b"},"address":"ec1fdb2d29c5689416b3f1b55a4d879fddf0e6e3","name":"","meta":"{}"}

tests/unittests/cli/keystore/password

@@ -0,0 +1 @@
+123

tests/unittests/cli/keystore/wrong_password

@@ -0,0 +1 @@
+123456

tests/unittests/cli/scenario/join-network-scenario-J1.yaml

@@ -0,0 +1,48 @@
+version: 2
+
+settings:
+  gas_price: "fast"
+  chain: any
+  services:
+    pfs:
+      url: https://pfs-goerli.services-dev.raiden.network
+    udc:
+      enable: true
+      token:
+        deposit: true
+
+token:
+
+nodes:
+  mode: managed
+  count: 4
+
+  default_options:
+    gas-price: fast
+    environment-type: development
+    routing-mode: pfs
+    pathfinding-max-paths: 5
+    pathfinding-max-fee: 10
+
+scenario:
+  serial:
+    tasks:
+      - parallel:
+          name: "Setting up a network"
+          tasks:
+            - open_channel: {from: 0, to: 1, total_deposit: 10, expected_http_status: 201}
+            - open_channel: {from: 0, to: 2, total_deposit: 10, expected_http_status: 201}
+            - open_channel: {from: 1, to: 2, total_deposit: 10, expected_http_status: 201}
+      - serial:
+          name: "Checking the network"
+          tasks:
+            - assert: {from: 0, to: 1, total_deposit: 10, balance: 10, state: "opened"}
+            - assert: {from: 0, to: 2, total_deposit: 10, balance: 10, state: "opened"}
+            - assert: {from: 1, to: 2, total_deposit: 10, balance: 10, state: "opened"}
+      - serial:
+          name: "Node Nr. 4 joins"
+          tasks:
+            - join_network: {from: 3, funds: 100, initial_channel_target: 3, joinable_funds_target: 0.4, expected_http_status: 204}
+            - assert: {from: 3, to: 0, total_deposit: 20, balance: 20, state: "opened"}
+            - assert: {from: 3, to: 1, total_deposit: 20, balance: 20, state: "opened"}
+            - assert: {from: 3, to: 2, total_deposit: 20, balance: 20, state: "opened"}

tests/unittests/cli/test_cli.py

@@ -0,0 +1,88 @@
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+from click.testing import CliRunner
+
+from scenario_player import main
+from scenario_player.exceptions.cli import WrongPassword
+
+KEYSTORE_PATH = str(Path(__file__).resolve().parents[0].joinpath("keystore"))
+SCENARIO = f"{Path(__file__).parent.joinpath('scenario', 'join-network-scenario-J1.yaml')}"
+CLI_ARGS = f"--chain goerli:http://geth.goerli.ethnodes.brainbot.com:8545 run " \
+               f"--keystore-file " + KEYSTORE_PATH + "/UTC--1 " \
+               f"--no-ui " \
+               f"{{pw_option}} " \
+               f"{SCENARIO}"
+
+
+@pytest.fixture(scope="module")
+def runner():
+    return CliRunner()
+
+
+class Sentinel(Exception):
+    pass
+
+
+class TestPasswordHandling:
+    # use a fixture instead of patch directly,
+    # to avoid having to pass an extra argument to all methods.
+    @pytest.fixture(autouse=True)
+    def patch_collect_tasks_on_setup(self):
+        with patch("scenario_player.main.collect_tasks", side_effect=Sentinel):
+            # Yield instead of return,
+            # as that allows the patching to be undone after the test is complete.
+            yield
+
+    def test_password_file_not_existent(self, runner):
+        """A not existing password file should raise error"""
+        result = runner.invoke(
+            main.main,
+            CLI_ARGS.format(pw_option=f"--password-file /does/not/exist").split(" ")
+        )
+        assert result.exit_code == 2
+        assert '"--password-file": File "/does/not/exist" does not exist.' in result.output
+
+    def test_mutually_exclusive(self, runner):
+        result = runner.invoke(
+            main.main,
+            CLI_ARGS.format(
+                pw_option=
+                f"--password-file {KEYSTORE_PATH}" + "/password " + "--password 123").split(" ")
+        )
+        assert result.exit_code == 2
+        assert 'Error: Illegal usage: password_file is mutually exclusive' in result.output
+
+    @pytest.mark.parametrize(
+        "password_file, expected_exec",
+        argvalues=[("/wrong_password", WrongPassword), ("/password", Sentinel)],
+        ids=["wrong password", "correct password"],
+    )
+    def test_password_file(self, password_file, expected_exec, runner):
+        result = runner.invoke(main.main, CLI_ARGS.format(
+            pw_option=f"--password-file {KEYSTORE_PATH + password_file}"))
+        assert result.exc_info[0] == expected_exec
+        assert result.exit_code == 1
+
+    @pytest.mark.parametrize(
+        "password, expected_exc",
+        argvalues=[("wrong_password", WrongPassword), ("123", Sentinel)],
+        ids=["wrong password", "correct password"],
+    )
+    def test_password(self, password, expected_exc, runner):
+        result = runner.invoke(main.main,
+                               CLI_ARGS.format(pw_option=f"--password {password}").split(" "))
+        assert result.exc_info[0] == expected_exc
+        assert result.exit_code == 1
+
+    @pytest.mark.parametrize(
+        "user_input, expected_exc",
+        argvalues=[("wrongpassword", WrongPassword), ("123", Sentinel)],
+        ids=["wrong password", "correct password"],
+    )
+    def test_manual_password_validation(self, user_input, expected_exc, runner):
+        result = runner.invoke(main.main,
+                               CLI_ARGS.format(pw_option=f"--password {user_input}").split(" "))
+        assert result.exc_info[0] == expected_exc
+        assert result.exit_code == 1