Merge pull request #8 in INT/security-event-cli from juliya.smith/INTEG-698/error-message-for-tcp-port-error to master

* commit '6f8130c85abcc44e3fc353f302648a3bfd37456d':
  Remove none checks because they will never happen
  Seperate state modifications to other function in args
  Set defaults in mian
  Add docstring for get_logger
  Format
  Fix tests
  Code clean up
  better error handlin
  better error handlin
  Use dest args dto
  Better error handling:
  Use OSError instead of socket_error
  Test for socket error
  Remove args arg
  Remove args arg
  Catch all socket errors

Author: Juliya Smith

c42seceventcli/aed/args.py

@@ -27,6 +27,7 @@ def get_args():
     cli_args = vars(parser.parse_args())
     args = _union_cli_args_with_config_file_args(cli_args)
     args.cli_parser = parser
+    args.initialize_args()
     args.verify_authority_arg()
     args.verify_username_arg()
     args.verify_destination_args()
@@ -99,6 +100,14 @@ def __init__(self):
         self.begin_date = AEDArgs._get_default_begin_date()
         self.end_date = AEDArgs._get_default_end_date()
 
+    def initialize_args(self):
+        self.destination_type = self.destination_type.lower()
+        try:
+            self.destination_port = int(self.destination_port)
+        except ValueError:
+            msg = "Destination port '{0}' not a base 10 integer.".format(self.destination_port)
+            self._raise_value_error(msg)
+
     @staticmethod
     def _get_default_begin_date():
         default_begin_date = datetime.now() - timedelta(days=60)
@@ -118,6 +127,10 @@ def verify_username_arg(self):
             self._raise_value_error("Code42 username not provided.")
 
     def verify_destination_args(self):
+        self._verify_stdout_destination()
+        self._verify_server_destination()
+
+    def _verify_stdout_destination(self):
         if self.destination_type == "stdout" and self.destination is not None:
             msg = (
                 "Destination '{0}' not applicable for stdout. "
@@ -126,10 +139,12 @@ def verify_destination_args(self):
             msg = msg.format(self.destination)
             self._raise_value_error(msg)
 
+    def _verify_file_destination(self):
         if self.destination_type == "file" and self.destination is None:
             msg = "Missing file name. Try: '--dest path/to/file'."
             self._raise_value_error(msg)
 
+    def _verify_server_destination(self):
         if self.destination_type == "server" and self.destination is None:
             msg = "Missing server URL. Try: '--dest https://syslog.example.com'."
             self._raise_value_error(msg)

c42seceventcli/aed/main.py

@@ -1,5 +1,5 @@
 import json
-from socket import gaierror
+from socket import gaierror, herror, timeout
 from urllib3 import disable_warnings
 from urllib3.exceptions import InsecureRequestWarning
 from datetime import datetime, timedelta
@@ -58,51 +58,50 @@ def _create_handlers(args):
     handlers.handle_error = error_logger.error
     output_format = args.output_format
     logger_formatter = _get_log_formatter(output_format)
+    destination_args = _create_destination_args(args)
     logger = _get_logger(
-        args,
-        formatter=logger_formatter,
-        service_name=_SERVICE_NAME,
-        destination=args.destination,
-        destination_type=args.destination_type,
-        destination_port=int(args.destination_port),
-        destination_protocol=args.destination_protocol,
+        formatter=logger_formatter, service_name=_SERVICE_NAME, destination_args=destination_args
     )
     handlers.handle_response = _get_response_handler(logger)
     return handlers
 
 
-def _get_logger(
-    args,
-    formatter,
-    service_name,
-    destination,
-    destination_type,
-    destination_port=514,
-    destination_protocol="TCP",
-):
+def _create_destination_args(args):
+    destination_args = common.DestinationArgs()
+    destination_args.destination_type = args.destination_type
+    destination_args.destination = args.destination
+    destination_args.destination_port = args.destination_port
+    destination_args.destination_protocol = args.destination_protocol
+    return destination_args
+
+
+def _get_logger(formatter, service_name, destination_args):
     try:
         return common.get_logger(
-            formatter=formatter,
-            service_name=service_name,
-            destination=destination,
-            destination_type=destination_type,
-            destination_port=destination_port,
-            destination_protocol=destination_protocol,
-        )
-    except gaierror:
-        args.cli_parser.print_usage()
-        print(
-            "Error with provided server destination arguments: hostname={0}, port={1}, protocol={2}.".format(
-                destination, destination_port, destination_protocol
-            )
+            formatter=formatter, service_name=service_name, destination_args=destination_args
         )
+    except (herror, gaierror, timeout) as ex:
+        print(repr(ex))
+        _print_server_args(destination_args)
         exit(1)
-    except IOError:
-        args.cli_parser.print_usage()
-        print("Error with provided file path {0}. Try --dest path/to/file.".format(destination))
+    except IOError as ex:
+        print(repr(ex))
+        if ex.errno == 61:
+            _print_server_args(destination_args)
+            exit(1)
+
+        print("File path: {0}.".format(destination_args.destination))
         exit(1)
 
 
+def _print_server_args(server_args):
+    print(
+        "Hostname={0}, port={1}, protocol={2}.".format(
+            server_args.destination, server_args.destination_port, server_args.destination_protocol
+        )
+    )
+
+
 def _set_up_cursor_store(record_cursor, clear_cursor, handlers):
     if record_cursor or clear_cursor:
         store = AEDCursorStore()

c42seceventcli/common/util.py

@@ -65,39 +65,45 @@ def get_error_logger(service_name):
     return logger
 
 
-def get_logger(
-    formatter,
-    service_name,
-    destination,
-    destination_type,
-    destination_port=514,
-    destination_protocol="TCP",
-):
-    destination_type = destination_type.lower()
+class DestinationArgs(object):
+    destination_type = None
+    destination = None
+    destination_port = None
+    destination_protocol = None
+
+
+def get_logger(formatter, service_name, destination_args):
+    """Args:
+           formatter: The formatter for logger.
+           service_name: The name of the script getting the logger.
+                Necessary for distinguishing multiple loggers.
+           destination_args: DTO holding the destination_type, destination, destination_port, and destination_protocol.
+        Returns:
+            A logger with the correct handler per destination_type.
+            For destination_type == stdout, it uses a StreamHandler.
+            For destination_type == file, it uses a FileHandler.
+            For destination_type == server, it uses a NoPrioritySyslogHandler.
+    """
+
     logger = logging.getLogger("{0}_logger".format(service_name))
-    handler = _get_log_handler(
-        destination=destination,
-        destination_type=destination_type,
-        destination_port=destination_port,
-        destination_protocol=destination_protocol,
-    )
+    handler = _get_log_handler(destination_args)
     handler.setFormatter(formatter)
     logger.addHandler(handler)
     logger.setLevel(logging.INFO)
     return logger
 
 
-def _get_log_handler(
-    destination, destination_type, destination_port=514, destination_protocol="TCP"
-):
-    if destination_type == "stdout":
+def _get_log_handler(destination_args):
+    if destination_args.destination_type == "stdout":
         return logging.StreamHandler(sys.stdout)
-    elif destination_type == "server":
+    elif destination_args.destination_type == "server":
         return NoPrioritySysLogHandler(
-            hostname=destination, port=destination_port, protocol=destination_protocol
+            hostname=destination_args.destination,
+            port=destination_args.destination_port,
+            protocol=destination_args.destination_protocol,
         )
-    elif destination_type == "file":
-        return logging.FileHandler(filename=destination)
+    elif destination_args.destination_type == "file":
+        return logging.FileHandler(filename=destination_args.destination)
 
 
 def get_stored_password(service_name, username):

tests/aed/test_args.py

@@ -84,14 +84,18 @@ def test_get_args_calls_sec_args_try_set_with_expected_args(
     mock_setter.assert_called_once_with(key, expected_cli_val, expected_config_val)
 
 
-def test_get_args_when_destination_is_not_none_and_destination_type_is_stdout_raises_value_error(patches):
+def test_get_args_when_destination_is_not_none_and_destination_type_is_stdout_raises_value_error(
+    patches
+):
     patches.cli_args.destination_type = "stdout"
     patches.cli_args.destination = "Delaware"
     with pytest.raises(ValueError):
         get_args()
 
 
-def test_get_args_when_destination_is_none_and_destination_type_is_server_raises_value_error(patches):
+def test_get_args_when_destination_is_none_and_destination_type_is_server_raises_value_error(
+    patches
+):
     patches.cli_args.destination_type = "server"
     patches.cli_args.destination = None
     with pytest.raises(ValueError):

tests/aed/test_main.py

@@ -1,6 +1,6 @@
 import pytest
 from datetime import datetime, timedelta
-from socket import gaierror
+from socket import herror, gaierror, timeout
 
 from py42 import settings
 import py42.debug_level as debug_level
@@ -222,30 +222,59 @@ def test_main_when_destination_port_is_set_passes_port_to_get_logger(patches):
     expected = 1000
     patches.aed_args.destination_port = expected
     main.main()
-    actual = patches.get_logger.call_args[1]["destination_port"]
+    actual = patches.get_logger.call_args[1]["destination_args"].destination_port
     assert actual == expected
 
 
 def test_main_when_given_destination_protocol_via_cli_passes_port_to_get_logger(patches):
     expected = "SOME PROTOCOL"
     patches.aed_args.destination_protocol = expected
     main.main()
-    actual = patches.get_logger.call_args[1]["destination_protocol"]
+    actual = patches.get_logger.call_args[1]["destination_args"].destination_protocol
     assert actual == expected
 
 
-def test_main_when_get_logger_raises_io_error_causes_exit(patches):
+def test_main_when_get_logger_raises_io_error_without_errno_61_print_error_about_file_path(
+    patches, capsys
+):
     patches.get_logger.side_effect = IOError
     with pytest.raises(SystemExit):
         main.main()
 
+    assert "file path" in capsys.readouterr().out.lower()
+
+
+def test_main_when_get_logger_raises_io_error_with_errno_61_prints_error_about_hostname(
+    patches, capsys
+):
+    err = IOError()
+    err.errno = 61
+    patches.get_logger.side_effect = err
+
+    with pytest.raises(SystemExit):
+        main.main()
+
+    assert "hostname" in capsys.readouterr().out.lower()
+
 
-def test_main_when_get_logger_raises_gaierror_causes_exit(patches):
+def test_main_when_get_logger_raises_h_error_causes_exit(patches):
     patches.get_logger.side_effect = gaierror
     with pytest.raises(SystemExit):
         main.main()
 
 
+def test_main_when_get_logger_raises_gai_error_causes_exit(patches):
+    patches.get_logger.side_effect = herror
+    with pytest.raises(SystemExit):
+        main.main()
+
+
+def test_main_when_get_logger_raises_timeout_causes_exit(patches):
+    patches.get_logger.side_effect = timeout
+    with pytest.raises(SystemExit):
+        main.main()
+
+
 def test_main_when_record_cursor_is_true_overrides_handlers_record_cursor_position(mocker, patches):
     expected = mocker.MagicMock()
     AEDCursorStore.replace_stored_insertion_timestamp = expected

tests/common/test_common.py

@@ -2,7 +2,6 @@
 from os import path
 from datetime import datetime, timedelta
 from logging import StreamHandler, FileHandler
-from logging.handlers import RotatingFileHandler
 
 from c42secevents.logging.handlers import NoPrioritySysLogHandler
 
@@ -15,6 +14,7 @@
     get_stored_password,
     delete_stored_password,
     get_user_project_path,
+    DestinationArgs,
 )
 
 
@@ -223,7 +223,9 @@ def test_get_error_logger_uses_rotating_file_with_expected_args(mocker, mock_get
 
 def test_get_logger_when_destination_type_is_stdout_adds_stream_handler_to_logger(mock_get_logger):
     service = "TEST_SERVICE"
-    logger = get_logger(None, service, "Somewhere", "stdout")
+    args = DestinationArgs()
+    args.destination_type = "stdout"
+    logger = get_logger(None, service, args)
     actual = type(logger.addHandler.call_args[0][0])
     expected = StreamHandler
     assert actual == expected
@@ -233,7 +235,9 @@ def test_get_logger_when_destination_type_is_file_adds_file_handler_to_logger(
     mock_get_logger, mock_file_handler
 ):
     service = "TEST_SERVICE"
-    logger = get_logger(None, service, "Somewhere", "file")
+    args = DestinationArgs()
+    args.destination_type = "file"
+    logger = get_logger(None, service, args)
     actual = type(logger.addHandler.call_args[0][0])
     expected = FileHandler
     assert actual == expected
@@ -243,7 +247,9 @@ def test_get_logger_when_destination_type_is_server_adds_no_priority_syslog_hand
     mock_get_logger, mock_no_priority_syslog_handler
 ):
     service = "TEST_SERVICE"
-    logger = get_logger(None, service, "Somewhere", "server")
+    args = DestinationArgs()
+    args.destination_type = "server"
+    logger = get_logger(None, service, args)
     actual = type(logger.addHandler.call_args[0][0])
     expected = NoPrioritySysLogHandler
     assert actual == expected