diff --git a/tabcmd/commands/auth/session.py b/tabcmd/commands/auth/session.py index eb8a0448..4f01e2cf 100644 --- a/tabcmd/commands/auth/session.py +++ b/tabcmd/commands/auth/session.py @@ -81,6 +81,8 @@ def _update_session_data(self, args): self.no_prompt = args.no_prompt # have to set this on every call? self.certificate = args.certificate or self.certificate self.no_certcheck = args.no_certcheck # have to set this on every call? + if args.no_proxy and args.proxy: + Errors.exit_with_error(self.logger, _("session.errors.proxy_conflict")) self.no_proxy = args.no_proxy # have to set this on every call? self.proxy = args.proxy or self.proxy self.timeout = self.timeout_as_integer(self.logger, args.timeout, self.timeout) diff --git a/tabcmd/execution/parent_parser.py b/tabcmd/execution/parent_parser.py index ad3173f5..0da7ec3d 100644 --- a/tabcmd/execution/parent_parser.py +++ b/tabcmd/execution/parent_parser.py @@ -21,7 +21,10 @@ """ -def parent_parser_with_global_options(): +def parent_parser_with_global_options(suppress_defaults: bool = False): + """Global options are accepted both before and after the subcommand. + The copy used by the subcommand parsers must not set defaults: argparse would otherwise + overwrite a value given before the subcommand with the default.""" parser = argparse.ArgumentParser(usage=argparse.SUPPRESS, add_help=False) parser._optionals.title = _("tabcmdparser.global.behaviors") @@ -139,6 +142,9 @@ def parent_parser_with_global_options(): metavar="", help=_("tabcmd.global.help.page_size"), ) + if suppress_defaults: + for action in parser._actions: + action.default = argparse.SUPPRESS return parser @@ -148,6 +154,7 @@ class ParentParser: def __init__(self): self.global_options = parent_parser_with_global_options() + self.subcommand_global_options = parent_parser_with_global_options(suppress_defaults=True) self.root = argparse.ArgumentParser( prog="tabcmd", description="tabcmd -- " + _("tabcmd.howto"), @@ -173,7 +180,7 @@ def connect_commands(self): def include(self, command): additional_parser = self.subparsers.add_parser( - command.name, help=command.description, parents=[self.global_options] + command.name, help=command.description, parents=[self.subcommand_global_options] ) additional_parser._optionals.title = _("tabcmdparser.global.connections") # This line is where we actually set each parser to call the correct command @@ -184,7 +191,7 @@ def include(self, command): # Help isn't added like the others because it has to have access to the rest, to get their args def include_help(self): additional_parser = self.subparsers.add_parser( - "help", help=_("tabcmdparser.help.description"), parents=[self.global_options] + "help", help=_("tabcmdparser.help.description"), parents=[self.subcommand_global_options] ) additional_parser._optionals.title = _("tabcmdparser.global.connections") additional_parser.set_defaults(func=Help(self)) diff --git a/tabcmd/locales/en/tabcmd_messages_en.properties b/tabcmd/locales/en/tabcmd_messages_en.properties index d9b5f57f..cad45868 100644 --- a/tabcmd/locales/en/tabcmd_messages_en.properties +++ b/tabcmd/locales/en/tabcmd_messages_en.properties @@ -122,6 +122,7 @@ session.auto_site_login=Auto-sign in to site: {0} session.connecting=Connecting to the server... session.continuing_session=Continuing previous session session.errors.missing_arguments=Cannot sign in because of missing arguments: {0} +session.errors.proxy_conflict=Cannot use --proxy together with --no-proxy. Pass only one of them. session.errors.password_file_not_found=Password file not found: '{0}'. Check the path passed to --password-file. session.errors.token_file_not_found=Token file not found: '{0}'. Check the path passed to --token-file. session.errors.script_no_password=Tabcmd was run from a script or IDE but no password was provided. A password must be provided as an argument to tabcmd in order to log in. diff --git a/tests/commands/test_session.py b/tests/commands/test_session.py index bd14bf31..661aec23 100644 --- a/tests/commands/test_session.py +++ b/tests/commands/test_session.py @@ -199,6 +199,31 @@ def test__create_new_username_credential_succeeds_from_args(self, mock_pass): auth = active_session._create_new_credential(test_args.password, Session.PASSWORD_CRED_TYPE) +class ProxyOptionTests(unittest.TestCase): + def test_proxy_with_no_proxy_is_rejected(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = "host:1" + test_args.no_proxy = True + with self.assertRaises(SystemExit): + Session()._update_session_data(test_args) + + def test_proxy_alone_is_accepted(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = "host:1" + test_args.no_proxy = False + mock_session = Session() + mock_session._update_session_data(test_args) + assert mock_session.proxy == "host:1" + + def test_no_proxy_alone_is_accepted(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = None + test_args.no_proxy = True + mock_session = Session() + mock_session._update_session_data(test_args) + assert mock_session.no_proxy is True + + class PromptingTests(unittest.TestCase): def test_show_prompt_if_user_didnt_say(self): test_args = Namespace(**vars(args_to_mock)) diff --git a/tests/parsers/test_global_options_position.py b/tests/parsers/test_global_options_position.py new file mode 100644 index 00000000..ef08fdd5 --- /dev/null +++ b/tests/parsers/test_global_options_position.py @@ -0,0 +1,150 @@ +import argparse +import unittest +from unittest import mock + +from tabcmd.commands.auth.login_command import LoginCommand +from tabcmd.execution.parent_parser import ParentParser, parent_parser_with_global_options +from .common_setup import * + +commandname: str = "login" + + +@mock.patch("sys.argv", None) +class GlobalOptionsPositionTest(ParserTest): + """Global options must mean the same thing before and after the subcommand.""" + + @classmethod + def setUpClass(cls): + cls.parser_under_test = initialize_test_pieces(commandname, LoginCommand) + + def test_global_options_before_command_are_kept(self): + args = self.parser_under_test.parse_args( + ["--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de", commandname] + ) + assert args.server == "the-server", args + assert args.site_name == "the-site", args + assert args.token_name == "n", args + assert args.language == "de", args + + def test_global_options_after_command_are_kept(self): + args = self.parser_under_test.parse_args( + [commandname, "--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de"] + ) + assert args.server == "the-server", args + assert args.site_name == "the-site", args + assert args.token_name == "n", args + assert args.language == "de", args + + def test_option_after_command_wins_over_option_before(self): + args = self.parser_under_test.parse_args(["--server", "first", commandname, "--server", "second"]) + assert args.server == "second", args + + def test_defaults_apply_when_option_not_given(self): + args = self.parser_under_test.parse_args([commandname]) + assert args.server is None, args + assert args.language is None, args + assert args.site_name == "", args + assert args.logging_level == "INFO", args + + +# every global option, with a value where it takes one; each must mean the same thing in either position +GLOBAL_OPTIONS = [ + ["-s", "the-server"], + ["-t", "the-site"], + ["-u", "the-user"], + ["--token-name", "the-token-name"], + ["-p", "the-password"], + ["--token-value", "the-token-value"], + ["--password-file", "the-file"], + ["--token-file", "the-file"], + ["--no-prompt"], + ["-c", "the-cert"], + ["--no-certcheck"], + ["--no-cookie"], + ["-x", "host:1"], + ["--no-proxy"], + ["--timeout", "30"], + ["--continue-if-exists"], + ["--country", "DE"], + ["--language", "DE"], + ["-l", "debug"], + ["--query-page-size", "5"], +] + +# commands that parse with the given arguments and nothing else +COMMANDS = [["login"], ["logout"], ["listsites"], ["get", "/views/a/b"], ["creategroup", "the-group"]] + + +def real_parser() -> argparse.ArgumentParser: + manager = ParentParser() + manager.connect_commands() + manager.include_help() + return manager.get_root_parser() + + +def parse(parser: argparse.ArgumentParser, args: list) -> dict: + values = vars(parser.parse_args(args)) + values.pop("func") + return values + + +class GlobalOptionsRealCommandsTest(unittest.TestCase): + """The same checks against the parser the CLI builds, not a single mocked command.""" + + parser: argparse.ArgumentParser + + @classmethod + def setUpClass(cls): + cls.parser = real_parser() + + def test_every_global_option_means_the_same_before_and_after_the_command(self): + for command in COMMANDS: + defaults = parse(self.parser, command) + for option in GLOBAL_OPTIONS: + with self.subTest(command=command[0], option=option): + before = parse(self.parser, option + command) + after = parse(self.parser, command + option) + assert before == after, (before, after) + assert before != defaults, "{} had no effect".format(option) + + def test_option_values_before_the_command_are_converted(self): + values = parse( + self.parser, ["-l", "debug", "--language", "DE", "--country", "FR", "--query-page-size", "5", "login"] + ) + assert values["logging_level"] == "DEBUG", values + assert values["language"] == "de", values + assert values["country"] == "fr", values + assert values["query_page_size"] == 5, values + + def test_boolean_flags_default_to_false_and_are_kept_before_the_command(self): + flags = { + "--no-prompt": "no_prompt", + "--no-certcheck": "no_certcheck", + "--no-cookie": "no_cookie", + "--no-proxy": "no_proxy", + "--continue-if-exists": "continue_if_exists", + } + defaults = parse(self.parser, ["login"]) + for flag, name in flags.items(): + with self.subTest(flag=flag): + assert defaults[name] is False, defaults + assert parse(self.parser, [flag, "login"])[name] is True + + def test_every_global_default_is_present_for_every_command(self): + expected = {a.dest for a in parent_parser_with_global_options()._actions if a.default is not argparse.SUPPRESS} + assert len(expected) >= 19, expected + for command in COMMANDS: + with self.subTest(command=command[0]): + missing = expected - set(parse(self.parser, command)) + assert not missing, missing + + def test_help_command_accepts_global_options_in_either_position(self): + defaults = parse(self.parser, ["help"]) + assert defaults["server"] is None, defaults + assert defaults["logging_level"] == "INFO", defaults + assert parse(self.parser, ["-s", "the-server", "help"])["server"] == "the-server" + assert parse(self.parser, ["help", "-s", "the-server"])["server"] == "the-server" + + +if __name__ == "__main__": + unittest.main()