Island: Make AuthenticationService explicitly stateful

monkey/monkey_island/cc/resources/auth/auth.py

@@ -29,6 +29,9 @@ class Authenticate(AbstractResource):
 
     urls = ["/api/auth"]
 
+    def __init__(self, authentication_service: AuthenticationService):
+        self._authentication_service = authentication_service
+
     def post(self):
         """
         Example request: \
@@ -41,7 +44,7 @@ class Authenticate(AbstractResource):
         username, password = get_username_password_from_request(request)
 
         try:
-            AuthenticationService.authenticate(username, password)
+            self._authentication_service.authenticate(username, password)
             access_token = create_access_token(username)
         except IncorrectCredentialsError:
             return make_response({"error": "Invalid credentials"}, 401)

monkey/monkey_island/cc/resources/auth/registration.py

@@ -14,14 +14,17 @@ class Registration(AbstractResource):
 
     urls = ["/api/registration"]
 
+    def __init__(self, authentication_service: AuthenticationService):
+        self._authentication_service = authentication_service
+
     def get(self):
-        return {"needs_registration": AuthenticationService.needs_registration()}
+        return {"needs_registration": self._authentication_service.needs_registration()}
 
     def post(self):
         username, password = get_username_password_from_request(request)
 
         try:
-            AuthenticationService.register_new_user(username, password)
+            self._authentication_service.register_new_user(username, password)
             return make_response({"error": ""}, 200)
         # API Spec: HTTP status code for AlreadyRegisteredError should be 409 (CONFLICT)
         except (InvalidRegistrationCredentialsError, AlreadyRegisteredError) as e:

monkey/monkey_island/cc/services/authentication/authentication_service.py

@@ -18,52 +18,40 @@ from .user_creds import UserCreds
 
 
 class AuthenticationService:
-    DATA_DIR = None
-    user_datastore = None
+    def __init__(self, data_dir: Path, user_datastore: IUserDatastore):
+        self._data_dir = data_dir
+        self._user_datastore = user_datastore
 
-    # TODO: A number of these services should be instance objects instead of
-    # static/singleton hybrids. At the moment, this requires invasive refactoring that's
-    # not a priority.
-    @classmethod
-    def initialize(cls, data_dir: Path, user_datastore: IUserDatastore):
-        cls.DATA_DIR = data_dir
-        cls.user_datastore = user_datastore
+    def needs_registration(self) -> bool:
+        return not self._user_datastore.has_registered_users()
 
-    @classmethod
-    def needs_registration(cls) -> bool:
-        return not cls.user_datastore.has_registered_users()
-
-    @classmethod
-    def register_new_user(cls, username: str, password: str):
+    def register_new_user(self, username: str, password: str):
         if not username or not password:
             raise InvalidRegistrationCredentialsError("Username or password can not be empty.")
 
         credentials = UserCreds(username, _hash_password(password))
-        cls.user_datastore.add_user(credentials)
-        cls._reset_datastore_encryptor(username, password)
+        self._user_datastore.add_user(credentials)
+        self._reset_datastore_encryptor(username, password)
         reset_database()
 
-    @classmethod
-    def authenticate(cls, username: str, password: str):
+    def authenticate(self, username: str, password: str):
         try:
-            registered_user = cls.user_datastore.get_user_credentials(username)
+            registered_user = self._user_datastore.get_user_credentials(username)
         except UnknownUserError:
             raise IncorrectCredentialsError()
 
         if not _credentials_match_registered_user(username, password, registered_user):
             raise IncorrectCredentialsError()
 
-        cls._unlock_datastore_encryptor(username, password)
+        self._unlock_datastore_encryptor(username, password)
 
-    @classmethod
-    def _unlock_datastore_encryptor(cls, username: str, password: str):
+    def _unlock_datastore_encryptor(self, username: str, password: str):
         secret = _get_secret_from_credentials(username, password)
-        unlock_datastore_encryptor(cls.DATA_DIR, secret)
+        unlock_datastore_encryptor(self._data_dir, secret)
 
-    @classmethod
-    def _reset_datastore_encryptor(cls, username: str, password: str):
+    def _reset_datastore_encryptor(self, username: str, password: str):
         secret = _get_secret_from_credentials(username, password)
-        reset_datastore_encryptor(cls.DATA_DIR, secret)
+        reset_datastore_encryptor(self._data_dir, secret)
 
 
 def _hash_password(plaintext_password: str) -> str:

monkey/monkey_island/cc/services/initialize.py

@@ -41,6 +41,7 @@ from monkey_island.cc.services.telemetry.processing.processing import (
 from monkey_island.cc.setup.mongo.mongo_setup import MONGO_URL
 
 from . import AuthenticationService, JsonFileUserDatastore
+from .authentication.i_user_datastore import IUserDatastore
 from .reporting.report import ReportService
 
 logger = logging.getLogger(__name__)
@@ -64,7 +65,6 @@ def initialize_services(data_dir: Path) -> DIContainer:
 
     # This is temporary until we get DI all worked out.
     PostBreachFilesService.initialize(container.resolve(IFileRepository))
-    AuthenticationService.initialize(data_dir, JsonFileUserDatastore(data_dir))
     ReportService.initialize(container.resolve(AWSService))
 
     return container
@@ -95,6 +95,7 @@ def _register_repositories(container: DIContainer, data_dir: Path):
     container.register_instance(
         ICredentialsRepository, container.resolve(MongoCredentialsRepository)
     )
+    container.register_instance(IUserDatastore, container.resolve(JsonFileUserDatastore))
 
 
 def _decorate_file_repository(file_repository: IFileRepository) -> IFileRepository:
@@ -140,6 +141,7 @@ def _register_services(container: DIContainer):
     container.register_instance(AWSService, container.resolve(AWSService))
     container.register_instance(LocalMonkeyRunService, container.resolve(LocalMonkeyRunService))
     container.register_instance(IslandModeService, container.resolve(IslandModeService))
+    container.register_instance(AuthenticationService, container.resolve(AuthenticationService))
 
 
 def _patch_credentials_parser(container: DIContainer):

monkey/tests/unit_tests/monkey_island/cc/resources/auth/conftest.py

@@ -0,0 +1,24 @@
+from unittest.mock import MagicMock
+
+import pytest
+from tests.common import StubDIContainer
+
+from monkey_island.cc.services import AuthenticationService
+
+
+@pytest.fixture
+def mock_authentication_service():
+    mock_service = MagicMock(spec=AuthenticationService)
+    mock_service.authenticate = MagicMock()
+
+    return mock_service
+
+
+@pytest.fixture
+def flask_client(build_flask_client, mock_authentication_service):
+    container = StubDIContainer()
+
+    container.register_instance(AuthenticationService, mock_authentication_service)
+
+    with build_flask_client(container) as flask_client:
+        yield flask_client

monkey/tests/unit_tests/monkey_island/cc/resources/auth/test_auth.py

@@ -11,16 +11,6 @@ PASSWORD = "test_password"
 TEST_REQUEST = f'{{"username": "{USERNAME}", "password": "{PASSWORD}"}}'
 
 
-@pytest.fixture
-def mock_authentication_service(monkeypatch):
-    mock_service = MagicMock()
-    mock_service.authenticate = MagicMock()
-
-    monkeypatch.setattr("monkey_island.cc.resources.auth.auth.AuthenticationService", mock_service)
-
-    return mock_service
-
-
 @pytest.fixture
 def make_auth_request(flask_client):
     url = Authenticate.urls[0]

monkey/tests/unit_tests/monkey_island/cc/resources/auth/test_registration.py

@@ -12,19 +12,6 @@ USERNAME = "test_user"
 PASSWORD = "test_password"
 
 
-@pytest.fixture
-def mock_authentication_service(monkeypatch):
-    mock_service = MagicMock()
-    mock_service.register_new_user = MagicMock()
-    mock_service.needs_registration = MagicMock()
-
-    monkeypatch.setattr(
-        "monkey_island.cc.resources.auth.registration.AuthenticationService", mock_service
-    )
-
-    return mock_service
-
-
 @pytest.fixture
 def make_registration_request(flask_client):
     def inner(request_body):

monkey/tests/unit_tests/monkey_island/cc/services/test_authentication_service.py

@@ -69,8 +69,7 @@ def test_needs_registration__true(tmp_path):
     has_registered_users = False
     mock_user_datastore = MockUserDatastore(lambda: has_registered_users, None, None)
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     assert a_s.needs_registration()
 
@@ -79,8 +78,7 @@ def test_needs_registration__false(tmp_path):
     has_registered_users = True
     mock_user_datastore = MockUserDatastore(lambda: has_registered_users, None, None)
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     assert not a_s.needs_registration()
 
@@ -92,8 +90,7 @@ def test_register_new_user__fails(
 ):
     mock_user_datastore = MockUserDatastore(lambda: True, MagicMock(side_effect=error), None)
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     with pytest.raises(error):
         a_s.register_new_user(USERNAME, PASSWORD)
@@ -107,8 +104,7 @@ def test_register_new_user__empty_password_fails(
 ):
     mock_user_datastore = MockUserDatastore(lambda: False, None, None)
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     with pytest.raises(InvalidRegistrationCredentialsError):
         a_s.register_new_user(USERNAME, "")
@@ -122,8 +118,7 @@ def test_register_new_user(tmp_path, mock_reset_datastore_encryptor, mock_reset_
     mock_add_user = MagicMock()
     mock_user_datastore = MockUserDatastore(lambda: False, mock_add_user, None)
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     a_s.register_new_user(USERNAME, PASSWORD)
 
@@ -144,8 +139,7 @@ def test_authenticate__success(tmp_path, mock_unlock_datastore_encryptor):
         lambda _: UserCreds(USERNAME, PASSWORD_HASH),
     )
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     # If authentication fails, this function will raise an exception and the test will fail.
     a_s.authenticate(USERNAME, PASSWORD)
@@ -165,8 +159,7 @@ def test_authenticate__failed_wrong_credentials(
         lambda _: UserCreds(USERNAME, PASSWORD_HASH),
     )
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     with pytest.raises(IncorrectCredentialsError):
         a_s.authenticate(username, password)
@@ -179,8 +172,7 @@ def test_authenticate__failed_no_registered_user(tmp_path, mock_unlock_datastore
         lambda: True, None, MagicMock(side_effect=UnknownUserError)
     )
 
-    a_s = AuthenticationService()
-    a_s.initialize(tmp_path, mock_user_datastore)
+    a_s = AuthenticationService(tmp_path, mock_user_datastore)
 
     with pytest.raises(IncorrectCredentialsError):
         a_s.authenticate(USERNAME, PASSWORD)