cc: simplify constructor/factory interface for EnvironmentConfig

The `get_from_json()` and `get_from_dict()` static methods were really
just used for testing. The `EnvironmentConfig` class needs to store its
file path so it can wite to the file if needed. In practical usage,
`EnvironmentConfig` objects are initialized from files, so a simpler
interface is for its constructor to take a file path.

monkey/monkey_island/cc/environment/environment_config.py

@@ -12,52 +12,42 @@
 from monkey_island.cc.resources.auth.user_store import UserStore
 
 
-
 class EnvironmentConfig:
-    def __init__(self,
-                 server_config: str,
-                 deployment: str,
-                 user_creds: UserCreds,
-                 aws=None):
-        self.server_config_path = None
-        self.server_config = server_config
-        self.deployment = deployment
-        self.user_creds = user_creds
-        self.aws = aws
-
-    @staticmethod
-    def get_from_json(config_json: str) -> EnvironmentConfig:
-        data = json.loads(config_json)
-        return EnvironmentConfig.get_from_dict(data)
+    def __init__(self, file_path):
+        self._server_config_path = os.path.expanduser(file_path)
+        self.server_config = None
+        self.deployment = None
+        self.user_creds = None
+        self.aws = None
 
-    @staticmethod
-    def get_from_dict(dict_data: Dict) -> EnvironmentConfig:
-        user_creds = UserCreds.get_from_dict(dict_data)
-        aws = dict_data['aws'] if 'aws' in dict_data else None
-        return EnvironmentConfig(server_config=dict_data['server_config'],
-                                 deployment=dict_data['deployment'],
-                                 user_creds=user_creds,
-                                 aws=aws)
-
-    def save_to_file(self):
-        with open(self.server_config_path, 'w') as f:
-            f.write(json.dumps(self.to_dict(), indent=2))
+        self._load_from_file(self._server_config_path)
 
-    @staticmethod
-    def get_from_file(file_path=DEFAULT_SERVER_CONFIG_PATH) -> EnvironmentConfig:
+    def _load_from_file(self, file_path=DEFAULT_SERVER_CONFIG_PATH):
         file_path = os.path.expanduser(file_path)
 
         if not Path(file_path).is_file():
             server_config_generator.create_default_config_file(file_path)
         with open(file_path, 'r') as f:
             config_content = f.read()
 
-        environment_config = EnvironmentConfig.get_from_json(config_content)
-        # TODO: Populating this property is not ideal. Revisit this when you
-        #       make the logger config file configurable at runtime.
-        environment_config.server_config_path = file_path
+        self._load_from_json(config_content)
 
-        return environment_config
+    def _load_from_json(self, config_json: str) -> EnvironmentConfig:
+        data = json.loads(config_json)
+        self._load_from_dict(data)
+
+    def _load_from_dict(self, dict_data: Dict):
+        user_creds = UserCreds.get_from_dict(dict_data)
+        aws = dict_data['aws'] if 'aws' in dict_data else None
+
+        self.server_config = dict_data['server_config']
+        self.deployment = dict_data['deployment']
+        self.user_creds = user_creds
+        self.aws = aws
+
+    def save_to_file(self):
+        with open(self._server_config_path, 'w') as f:
+            f.write(json.dumps(self.to_dict(), indent=2))
 
     def to_dict(self) -> Dict:
         config_dict = {'server_config': self.server_config,

monkey/monkey_island/cc/environment/environment_singleton.py

@@ -41,7 +41,7 @@ def set_to_standard():
 
 def initialize_from_file(file_path):
     try:
-        config = EnvironmentConfig.get_from_file(file_path)
+        config = EnvironmentConfig(file_path)
 
         __env_type = config.server_config
         set_env(__env_type, config)

monkey/monkey_island/cc/environment/test__init__.py

@@ -1,9 +1,11 @@
 import json
 import os
+import tempfile
 from typing import Dict
 from unittest import TestCase
 from unittest.mock import MagicMock, patch
 
+from monkey_island.cc.consts import MONKEY_ISLAND_ABS_PATH
 import monkey_island.cc.testing.environment.server_config_mocks as config_mocks
 from common.utils.exceptions import (AlreadyRegisteredError,
                                      CredentialsNotRequiredError,
@@ -12,6 +14,30 @@
 from monkey_island.cc.environment import (Environment, EnvironmentConfig,
                                           UserCreds)
 
+TEST_RESOURCES_DIR = os.path.join(MONKEY_ISLAND_ABS_PATH, "cc", "testing", "environment")
+
+WITH_CREDENTIALS = os.path.join(TEST_RESOURCES_DIR, "server_config_with_credentials.json")
+NO_CREDENTIALS = os.path.join(TEST_RESOURCES_DIR, "server_config_no_credentials.json")
+PARTIAL_CREDENTIALS = os.path.join(TEST_RESOURCES_DIR, "server_config_partial_credentials.json")
+STANDARD_WITH_CREDENTIALS = os.path.join(TEST_RESOURCES_DIR,
+                                         "server_config_standard_with_credentials.json")
+STANDARD_ENV = os.path.join(TEST_RESOURCES_DIR,
+                                   "server_config_standard_env.json")
+
+def get_tmp_file():
+    with tempfile.NamedTemporaryFile(delete=False) as f:
+        return f.name
+
+class StubEnvironmentConfig(EnvironmentConfig):
+    def __init__(self, server_config, deployment, user_creds):
+        self.server_config = server_config
+        self.deployment = deployment
+        self.user_creds = user_creds
+        self.server_config_path = get_tmp_file()
+
+    def __del__(self):
+        os.remove(self.server_config_path)
+
 
 def get_server_config_file_path_test_version():
     return os.path.join(os.getcwd(), 'test_config.json')
@@ -21,7 +47,7 @@ class TestEnvironment(TestCase):
 
     class EnvironmentCredentialsNotRequired(Environment):
         def __init__(self):
-            config = EnvironmentConfig('test', 'test', UserCreds())
+            config = StubEnvironmentConfig('test', 'test', UserCreds())
             super().__init__(config)
 
         _credentials_required = False
@@ -31,7 +57,7 @@ def get_auth_users(self):
 
     class EnvironmentCredentialsRequired(Environment):
         def __init__(self):
-            config = EnvironmentConfig('test', 'test', UserCreds())
+            config = StubEnvironmentConfig('test', 'test', UserCreds())
             super().__init__(config)
 
         _credentials_required = True
@@ -41,7 +67,7 @@ def get_auth_users(self):
 
     class EnvironmentAlreadyRegistered(Environment):
         def __init__(self):
-            config = EnvironmentConfig('test', 'test', UserCreds('test_user', 'test_secret'))
+            config = StubEnvironmentConfig('test', 'test', UserCreds('test_user', 'test_secret'))
             super().__init__(config)
 
         _credentials_required = True
@@ -77,36 +103,37 @@ def test_try_needs_registration(self):
         self.assertTrue(env._try_needs_registration())
 
     def test_needs_registration(self):
+
         env = TestEnvironment.EnvironmentCredentialsRequired()
-        self._test_bool_env_method("needs_registration", env, config_mocks.CONFIG_WITH_CREDENTIALS, False)
-        self._test_bool_env_method("needs_registration", env, config_mocks.CONFIG_NO_CREDENTIALS, True)
-        self._test_bool_env_method("needs_registration", env, config_mocks.CONFIG_PARTIAL_CREDENTIALS, True)
+        self._test_bool_env_method("needs_registration", env, WITH_CREDENTIALS, False)
+        self._test_bool_env_method("needs_registration", env, NO_CREDENTIALS, True)
+        self._test_bool_env_method("needs_registration", env, PARTIAL_CREDENTIALS, True)
 
         env = TestEnvironment.EnvironmentCredentialsNotRequired()
-        self._test_bool_env_method("needs_registration", env, config_mocks.CONFIG_STANDARD_ENV, False)
-        self._test_bool_env_method("needs_registration", env, config_mocks.CONFIG_STANDARD_WITH_CREDENTIALS, False)
+        self._test_bool_env_method("needs_registration", env, STANDARD_ENV, False)
+        self._test_bool_env_method("needs_registration", env, STANDARD_WITH_CREDENTIALS, False)
 
     def test_is_registered(self):
         env = TestEnvironment.EnvironmentCredentialsRequired()
-        self._test_bool_env_method("_is_registered", env, config_mocks.CONFIG_WITH_CREDENTIALS, True)
-        self._test_bool_env_method("_is_registered", env, config_mocks.CONFIG_NO_CREDENTIALS, False)
-        self._test_bool_env_method("_is_registered", env, config_mocks.CONFIG_PARTIAL_CREDENTIALS, False)
+        self._test_bool_env_method("_is_registered", env, WITH_CREDENTIALS, True)
+        self._test_bool_env_method("_is_registered", env, NO_CREDENTIALS, False)
+        self._test_bool_env_method("_is_registered", env, PARTIAL_CREDENTIALS, False)
 
         env = TestEnvironment.EnvironmentCredentialsNotRequired()
-        self._test_bool_env_method("_is_registered", env, config_mocks.CONFIG_STANDARD_ENV, False)
-        self._test_bool_env_method("_is_registered", env, config_mocks.CONFIG_STANDARD_WITH_CREDENTIALS, False)
+        self._test_bool_env_method("_is_registered", env, STANDARD_ENV, False)
+        self._test_bool_env_method("_is_registered", env, STANDARD_WITH_CREDENTIALS, False)
 
     def test_is_credentials_set_up(self):
         env = TestEnvironment.EnvironmentCredentialsRequired()
-        self._test_bool_env_method("_is_credentials_set_up", env, config_mocks.CONFIG_NO_CREDENTIALS, False)
-        self._test_bool_env_method("_is_credentials_set_up", env, config_mocks.CONFIG_WITH_CREDENTIALS, True)
-        self._test_bool_env_method("_is_credentials_set_up", env, config_mocks.CONFIG_PARTIAL_CREDENTIALS, False)
+        self._test_bool_env_method("_is_credentials_set_up", env, NO_CREDENTIALS, False)
+        self._test_bool_env_method("_is_credentials_set_up", env, WITH_CREDENTIALS, True)
+        self._test_bool_env_method("_is_credentials_set_up", env, PARTIAL_CREDENTIALS, False)
 
         env = TestEnvironment.EnvironmentCredentialsNotRequired()
-        self._test_bool_env_method("_is_credentials_set_up", env, config_mocks.CONFIG_STANDARD_ENV, False)
+        self._test_bool_env_method("_is_credentials_set_up", env, STANDARD_ENV, False)
 
     def _test_bool_env_method(self, method_name: str, env: Environment, config: Dict, expected_result: bool):
-        env._config = EnvironmentConfig.get_from_json(json.dumps(config))
+        env._config = EnvironmentConfig(config)
         method = getattr(env, method_name)
         if expected_result:
             self.assertTrue(method())

monkey/monkey_island/cc/environment/test_environment_config.py

@@ -1,99 +1,90 @@
 import json
 import os
+import pathlib
+import shutil
 from typing import Dict
 
 import pytest
 
+from monkey_island.cc.consts import MONKEY_ISLAND_ABS_PATH
 import monkey_island.cc.testing.environment.server_config_mocks as config_mocks
 from monkey_island.cc.environment.environment_config import EnvironmentConfig
 from monkey_island.cc.environment.user_creds import UserCreds
 
+TEST_RESOURCES_DIR = os.path.join(
+    MONKEY_ISLAND_ABS_PATH, "cc", "testing", "environment"
+)
+
+WITH_CREDENTIALS = os.path.join(
+    TEST_RESOURCES_DIR, "server_config_with_credentials.json"
+)
+NO_CREDENTIALS = os.path.join(TEST_RESOURCES_DIR, "server_config_no_credentials.json")
+PARTIAL_CREDENTIALS = os.path.join(
+    TEST_RESOURCES_DIR, "server_config_partial_credentials.json"
+)
+STANDARD_WITH_CREDENTIALS = os.path.join(
+    TEST_RESOURCES_DIR, "server_config_standard_with_credentials.json"
+)
+
 
 @pytest.fixture
 def config_file(tmpdir):
     return os.path.join(tmpdir, "test_config.json")
 
 
-def test_get_with_credentials(config_file):
-    test_conf = config_mocks.CONFIG_WITH_CREDENTIALS
-
-    _write_test_config_to_tmp(config_file, test_conf)
-    config_dict = EnvironmentConfig.get_from_file(config_file).to_dict()
+def test_get_with_credentials():
+    config_dict = EnvironmentConfig(WITH_CREDENTIALS).to_dict()
 
     assert len(config_dict.keys()) == 4
-    assert config_dict["server_config"] == test_conf["server_config"]
-    assert config_dict["deployment"] == test_conf["deployment"]
-    assert config_dict["user"] == test_conf["user"]
-    assert config_dict["password_hash"] == test_conf["password_hash"]
-
+    assert config_dict["server_config"] == "password"
+    assert config_dict["deployment"] == "develop"
+    assert config_dict["user"] == "test"
+    assert config_dict["password_hash"] == "abcdef"
 
-def test_get_with_no_credentials(config_file):
-    test_conf = config_mocks.CONFIG_NO_CREDENTIALS
 
-    _write_test_config_to_tmp(config_file, test_conf)
-    config_dict = EnvironmentConfig.get_from_file(config_file).to_dict()
+def test_get_with_no_credentials():
+    config_dict = EnvironmentConfig(NO_CREDENTIALS).to_dict()
 
     assert len(config_dict.keys()) == 2
-    assert config_dict["server_config"] == test_conf["server_config"]
-    assert config_dict["deployment"] == test_conf["deployment"]
+    assert config_dict["server_config"] == "password"
+    assert config_dict["deployment"] == "develop"
 
 
-def test_get_with_partial_credentials(config_file):
-    test_conf = config_mocks.CONFIG_PARTIAL_CREDENTIALS
-
-    _write_test_config_to_tmp(config_file, test_conf)
-    config_dict = EnvironmentConfig.get_from_file(config_file).to_dict()
+def test_get_with_partial_credentials():
+    config_dict = EnvironmentConfig(PARTIAL_CREDENTIALS).to_dict()
 
     assert len(config_dict.keys()) == 3
-    assert config_dict["server_config"] == test_conf["server_config"]
-    assert config_dict["deployment"] == test_conf["deployment"]
-    assert config_dict["user"] == test_conf["user"]
-
-
-def _write_test_config_to_tmp(config_file, config: Dict):
-    with open(config_file, "wt") as f:
-        json.dump(config, f)
+    assert config_dict["server_config"] == "password"
+    assert config_dict["deployment"] == "develop"
+    assert config_dict["user"] == "test"
 
 
 def test_save_to_file(config_file):
-    server_config = "standard"
-    deployment = "develop"
-    user = "test_user"
-    password_hash = "abcdef"
-    aws = "test"
-
-    environment_config = EnvironmentConfig(
-        server_config, deployment, UserCreds(user, password_hash), aws
-    )
-    environment_config.server_config_path = config_file
+    shutil.copyfile(STANDARD_WITH_CREDENTIALS, config_file)
 
+    environment_config = EnvironmentConfig(config_file)
+    environment_config.aws = "test_aws"
     environment_config.save_to_file()
+
     with open(config_file, "r") as f:
         from_file = json.load(f)
 
     assert len(from_file.keys()) == 5
-    assert from_file["server_config"] == server_config
-    assert from_file["deployment"] == deployment
-    assert from_file["user"] == user
-    assert from_file["password_hash"] == password_hash
-    assert from_file["aws"] == aws
+    assert from_file["server_config"] == "standard"
+    assert from_file["deployment"] == "develop"
+    assert from_file["user"] == "test"
+    assert from_file["password_hash"] == "abcdef"
+    assert from_file["aws"] == "test_aws"
 
 
 def test_add_user(config_file):
-    server_config = "standard"
-    deployment = "develop"
-    user = "test_user"
-    password_hash = "abcdef"
-
     new_user = "new_user"
     new_password_hash = "fedcba"
     new_user_creds = UserCreds(new_user, new_password_hash)
 
-    environment_config = EnvironmentConfig(
-        server_config, deployment, UserCreds(user, password_hash)
-    )
-    environment_config.server_config_path = config_file
+    shutil.copyfile(STANDARD_WITH_CREDENTIALS, config_file)
 
+    environment_config = EnvironmentConfig(config_file)
     environment_config.add_user(new_user_creds)
 
     with open(config_file, "r") as f:
@@ -105,23 +96,17 @@ def test_add_user(config_file):
 
 
 def test_get_users():
-    server_config = "standard"
-    deployment = "develop"
-    user = "test_user"
-    password_hash = "abcdef"
-
-    environment_config = EnvironmentConfig(
-        server_config, deployment, UserCreds(user, password_hash)
-    )
-
+    environment_config = EnvironmentConfig(STANDARD_WITH_CREDENTIALS)
     users = environment_config.get_users()
+
     assert len(users) == 1
     assert users[0].id == 1
-    assert users[0].username == user
-    assert users[0].secret == password_hash
+    assert users[0].username == "test"
+    assert users[0].secret == "abcdef"
+
 
 def test_generate_default_file(config_file):
-    environment_config = EnvironmentConfig.get_from_file(config_file)
+    environment_config = EnvironmentConfig(config_file)
 
     assert os.path.isfile(config_file)
 
@@ -130,4 +115,3 @@ def test_generate_default_file(config_file):
     assert environment_config.user_creds.username == ""
     assert environment_config.user_creds.password_hash == ""
     assert environment_config.aws is None
-    environment_config.server_config_path == config_file