Island: Add comments about the current API design

monkey/monkey_island/cc/app.py

@@ -183,6 +183,7 @@ def init_api_resources(api: FlaskDIWrapper):
     api.add_resource(StopAllAgents)
 
     # Resources used by black box tests
+    # API Spec: Fix all the following endpoints, see comments in the resource classes
     api.add_resource(MonkeyBlackboxEndpoint)
     api.add_resource(ClearCaches)
     api.add_resource(LogBlackboxEndpoint)

monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py

@@ -1,8 +1,9 @@
 from monkey_island.cc.resources.AbstractResource import AbstractResource
 from monkey_island.cc.services.infection_lifecycle import should_agent_die
 
-
 class StopAgentCheck(AbstractResource):
+    # API Spec: Not a resource, checking is an action; RPC-style endpoint?
+    # REST feels okay too but probably rename it to AgentStopStatus or something.
     urls = ["/api/monkey-control/needs-to-stop/<int:monkey_guid>"]
 
     def get(self, monkey_guid: int):

monkey/monkey_island/cc/resources/agent_controls/stop_all_agents.py

@@ -9,6 +9,7 @@ from monkey_island.cc.services.infection_lifecycle import set_stop_all, should_a
 
 
 class StopAllAgents(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/monkey-control/stop-all-agents"]
 
     @jwt_required
@@ -21,5 +22,6 @@ class StopAllAgents(AbstractResource):
             else:
                 return make_response({}, 400)
 
+    # API Spec: This is the exact same thing as what's in StopAgentCheck
     def get(self, monkey_guid):
         return {"stop_agent": should_agent_die(monkey_guid)}

monkey/monkey_island/cc/resources/attack/attack_report.py

@@ -7,6 +7,7 @@ from monkey_island.cc.services.attack.attack_schema import SCHEMA
 
 
 class AttackReport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/report/attack"]
 
     @jwt_required

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

@@ -46,4 +46,5 @@ class Authenticate(AbstractResource):
         except IncorrectCredentialsError:
             return make_response({"error": "Invalid credentials"}, 401)
 
+        # API Spec: Why are we sending "error" here?
         return make_response({"access_token": access_token, "error": ""}, 200)

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

@@ -23,5 +23,6 @@ class Registration(AbstractResource):
         try:
             AuthenticationService.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:
             return make_response({"error": str(e)}, 400)

monkey/monkey_island/cc/resources/blackbox/clear_caches.py

@@ -13,6 +13,7 @@ logger = logging.getLogger(__name__)
 
 
 class ClearCaches(AbstractResource):
+    # API Spec: Action, not a resource; RPC-style endpoint?
     urls = ["/api/test/clear-caches"]
     """
     Used for timing tests - we want to get actual execution time of functions in BlackBox without
@@ -34,6 +35,8 @@ class ClearCaches(AbstractResource):
 
         if ReportService.is_report_generated() or AttackReportService.is_report_generated():
             logger.error(NOT_ALL_REPORTS_DELETED)
+            # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the
+            # server. 40x makes more sense.
             flask_restful.abort(500, error_info=NOT_ALL_REPORTS_DELETED)
 
         return {"success": "true"}

monkey/monkey_island/cc/resources/blackbox/log_blackbox_endpoint.py

@@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required
 
 
 class LogBlackboxEndpoint(AbstractResource):
+    # API Spec: Rename to noun, BlackboxTestsLogs or something
     urls = ["/api/test/log"]
 
     @jwt_required

monkey/monkey_island/cc/resources/blackbox/monkey_blackbox_endpoint.py

@@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required
 
 
 class MonkeyBlackboxEndpoint(AbstractResource):
+    # API Spec: Rename to noun, BlackboxTestsMonkeys or something
     urls = ["/api/test/monkey"]
 
     @jwt_required

monkey/monkey_island/cc/resources/blackbox/telemetry_blackbox_endpoint.py

@@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required
 
 
 class TelemetryBlackboxEndpoint(AbstractResource):
+    # API Spec: Rename to noun, BlackboxTestsTelemetries or something
     urls = ["/api/test/telemetry"]
 
     @jwt_required

monkey/monkey_island/cc/resources/configuration_export.py

@@ -9,6 +9,7 @@ from monkey_island.cc.services.config import ConfigService
 
 
 class ConfigurationExport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/configuration/export"]
 
     @jwt_required

monkey/monkey_island/cc/resources/configuration_import.py

@@ -39,6 +39,7 @@ class ResponseContents:
 
 
 class ConfigurationImport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/configuration/import"]
     SUCCESS = False
 
@@ -56,11 +57,13 @@ class ConfigurationImport(AbstractResource):
                     config_schema=ConfigService.get_config_schema(),
                     import_status=ImportStatuses.UNSAFE_OPTION_VERIFICATION_REQUIRED,
                 ).form_response()
+        # API Spec: HTTP status code should be 401 here
         except InvalidCredentialsError:
             return ResponseContents(
                 import_status=ImportStatuses.INVALID_CREDENTIALS,
                 message="Invalid credentials provided",
             ).form_response()
+        # API Spec: HTTP status code should be 400 (or something else) here
         except InvalidConfigurationError:
             return ResponseContents(
                 import_status=ImportStatuses.INVALID_CONFIGURATION,

monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py

@@ -6,6 +6,11 @@ from monkey_island.cc.services.reporting.exploitations.manual_exploitation impor
 
 
 class ManualExploitation(AbstractResource):
+    # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no
+    # "resource", however one could argue that at some point in time, we may want to access
+    # specific manual exploitation data based on, say, the monkey guid, at a later stage. Leave
+    # as is in that case, and consider modifying the logic to make this behave more like a
+    # "resource" and change the name.
     urls = ["/api/exploitations/manual"]
 
     @jwt_required

monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py

@@ -6,6 +6,11 @@ from monkey_island.cc.services.reporting.exploitations.monkey_exploitation impor
 
 
 class MonkeyExploitation(AbstractResource):
+    # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no
+    # "resource", however one could argue that at some point in time, we may want to access
+    # specific monkey exploitation data based on, say, the monkey guid, at a later stage. Leave
+    # as is in that case, and consider modifying the logic to make this behave more like a
+    # "resource" and change the name.
     urls = ["/api/exploitations/monkey"]
 
     @jwt_required

monkey/monkey_island/cc/resources/island_configuration.py

@@ -21,9 +21,14 @@ class IslandConfiguration(AbstractResource):
     @jwt_required
     def post(self):
         config_json = json.loads(request.data)
+        # API Spec: Resetting config is an action; separate RPC-style endpoint for this?
         if "reset" in config_json:
             ConfigService.reset_config()
         else:
             if not ConfigService.update_config(config_json, should_encrypt=True):
                 abort(400)
+        # API Spec: We're updating the config and then returning the config back?
+        # RESTfulness of a POST request is to return an identifier of the updated/newly created
+        # resource. Since there's only one thing we're updating (and not multiple "resources"),
+        # should this also be an RPC-style endpoint (/api/resetConfig and /api/updateConfig)?
         return self.get()

monkey/monkey_island/cc/resources/island_logs.py

@@ -8,6 +8,7 @@ logger = logging.getLogger(__name__)
 
 
 class IslandLog(AbstractResource):
+    # API Spec: Why the inconsistency in endpoints of IslandLog and Log?
     urls = ["/api/log/island/download"]
 
     @jwt_required

monkey/monkey_island/cc/resources/island_mode.py

@@ -13,6 +13,9 @@ logger = logging.getLogger(__name__)
 
 
 class IslandMode(AbstractResource):
+    # API Spec: Should these be RPC-style endpoints?
+    # POST is not updating/creating a "resource" but setting a property of the Island.
+    # Perhaps /api/setMode and /api/getMode would be more appropriate.
     urls = ["/api/island-mode"]
 
     @jwt_required
@@ -30,9 +33,13 @@ class IslandMode(AbstractResource):
                     "Using default advanced configuration."
                 )
 
+            # API Spec: RESTful way is to return an identifier of the updated/newly created
+            # resource but it doesn't make sense here which brings me back to the comment on
+            # lines 16-18.
             return make_response({}, 200)
         except (AttributeError, json.decoder.JSONDecodeError):
             return make_response({}, 400)
+        # API Spec: Check if HTTP codes make sense
         except ValueError:
             return make_response({}, 422)
 

monkey/monkey_island/cc/resources/local_run.py

@@ -10,7 +10,8 @@ from monkey_island.cc.services.run_local_monkey import LocalMonkeyRunService
 
 
 class LocalRun(AbstractResource):
-
+    # API Spec: This should be an RPC-style API i.e. two endpoints -
+    # "/api/getLocalMonkeyRunningStatus" and "/api/startLocalMonkey"
     urls = ["/api/local-monkey"]
 
     @jwt_required
@@ -28,7 +29,10 @@ class LocalRun(AbstractResource):
         body = json.loads(request.data)
         if body.get("action") == "run":
             local_run = LocalMonkeyRunService.run_local_monkey()
+            # API Spec: Feels weird to return "error_text" even when "is_running" is True
             return jsonify(is_running=local_run[0], error_text=local_run[1])
 
         # default action
+        # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the
+        # server. 40x makes more sense.
         return make_response({"error": "Invalid action"}, 500)

monkey/monkey_island/cc/resources/log.py

@@ -12,6 +12,7 @@ from monkey_island.cc.services.node import NodeService
 
 
 class Log(AbstractResource):
+    # API Spec: What log? Agent log?
     urls = ["/api/log"]
 
     @jwt_required

monkey/monkey_island/cc/resources/monkey.py

@@ -17,9 +17,11 @@ from monkey_island.cc.services.node import NodeService
 
 
 class Monkey(AbstractResource):
+    # API Spec: Resource name should be plural
     urls = [
         "/api/agent",
         "/api/agent/<string:guid>",
+        # API Spec: Resource names should alternate with IDs (/api/agents/123/config-format/xyz)
         "/api/agent/<string:guid>/<string:config_format>",
     ]
 
@@ -64,6 +66,7 @@ class Monkey(AbstractResource):
         ttl = create_monkey_ttl_document(DEFAULT_MONKEY_TTL_EXPIRY_DURATION_IN_SECONDS)
         update["$set"]["ttl_ref"] = ttl.id
 
+        # API Spec: What is this returning? Check that it follows rules.
         return mongo.db.monkey.update({"_id": monkey["_id"]}, update, upsert=False)
 
     # Used by monkey. can't secure.

monkey/monkey_island/cc/resources/monkey_download.py

@@ -20,6 +20,7 @@ class UnsupportedOSError(Exception):
 
 
 class MonkeyDownload(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/agent/download/<string:host_os>"]
 
     # Used by monkey. can't secure.

monkey/monkey_island/cc/resources/pba_file_download.py

@@ -9,6 +9,7 @@ logger = logging.getLogger(__file__)
 
 
 class PBAFileDownload(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/pba/download/<string:filename>"]
     """
     File download endpoint used by monkey to download user's PBA file

monkey/monkey_island/cc/resources/pba_file_upload.py

@@ -18,6 +18,8 @@ WINDOWS_PBA_TYPE = "PBAwindows"
 
 
 class FileUpload(AbstractResource):
+    # API Spec: FileUpload -> CustomPBAFile or something to make it more RESTful. Change endpoint
+    # accordingly.
     """
     File upload endpoint used to send/receive Custom PBA files
     """
@@ -80,6 +82,7 @@ class FileUpload(AbstractResource):
             safe_filename,
         )
 
+        # API Spec: HTTP status code should be 201
         response = Response(response=safe_filename, status=200, mimetype="text/plain")
         return response
 
@@ -101,6 +104,7 @@ class FileUpload(AbstractResource):
             self._file_storage_service.delete_file(filename)
             ConfigService.set_config_value(filename_path, "")
 
+        # API Spec: HTTP status code should be 204
         return make_response({}, 200)
 
     @staticmethod

monkey/monkey_island/cc/resources/ransomware_report.py

@@ -6,6 +6,7 @@ from monkey_island.cc.services.ransomware import ransomware_report
 
 
 class RansomwareReport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/report/ransomware"]
 
     @jwt_required

monkey/monkey_island/cc/resources/remote_run.py

@@ -20,6 +20,9 @@ NO_CREDS_ERROR_FORMAT = (
 
 
 class RemoteRun(AbstractResource):
+    # API Spec: POST request is an action, it's not updating/creating any resource.
+    # GET makes sense. The resource should be split up since these two use cases don't
+    # really go together.
     urls = ["/api/remote-monkey"]
 
     def __init__(self, aws_service: AWSService):
@@ -35,9 +38,11 @@ class RemoteRun(AbstractResource):
                 try:
                     resp["instances"] = self._aws_service.get_managed_instances()
                 except NoCredentialsError as e:
+                    # API Spec: HTTP status code should be 401
                     resp["error"] = NO_CREDS_ERROR_FORMAT.format(e)
                     return jsonify(resp)
                 except ClientError as e:
+                    # API Spec: HTTP status code should not be 200
                     resp["error"] = CLIENT_ERROR_FORMAT.format(e)
                     return jsonify(resp)
             return jsonify(resp)
@@ -49,9 +54,13 @@ class RemoteRun(AbstractResource):
         body = json.loads(request.data)
         if body.get("type") == "aws":
             results = self.run_aws_monkeys(body)
+            # API Spec: POST should return identifier or updated/newly created resource, not some
+            # kind of data. That's more of a GET thing.
             return RemoteRun._encode_results(results)
 
         # default action
+        # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the
+        # server. 40x makes more sense.
         return make_response({"error": "Invalid action"}, 500)
 
     def run_aws_monkeys(self, request_body) -> Sequence[AWSCommandResults]:

monkey/monkey_island/cc/resources/security_report.py

@@ -4,6 +4,7 @@ from monkey_island.cc.services.reporting.report import ReportService
 
 
 class SecurityReport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/report/security"]
 
     @jwt_required

monkey/monkey_island/cc/resources/telemetry.py

@@ -18,6 +18,7 @@ logger = logging.getLogger(__name__)
 
 
 class Telemetry(AbstractResource):
+    # API Spec: Resource name should be plural
     urls = ["/api/telemetry", "/api/telemetry/<string:monkey_guid>"]
 
     @jwt_required
@@ -62,6 +63,7 @@ class Telemetry(AbstractResource):
 
         process_telemetry(telemetry_json)
 
+        # API Spec: RESTful way is to return an identifier of the updated/newly created resource
         return {}, 201
 
     @staticmethod

monkey/monkey_island/cc/resources/telemetry_feed.py

@@ -39,6 +39,7 @@ class TelemetryFeed(AbstractResource):
             }
         except KeyError as err:
             logger.error("Failed parsing telemetries. Error: {0}.".format(err))
+            # API Spec: Should return HTTP status code 404 (?)
             return {"telemetries": [], "timestamp": datetime.now().isoformat()}
 
     @staticmethod

monkey/monkey_island/cc/resources/zero_trust/finding_event.py

@@ -8,6 +8,8 @@ from monkey_island.cc.services.zero_trust.monkey_findings.monkey_zt_finding_serv
 
 
 class ZeroTrustFindingEvent(AbstractResource):
+    # API Spec: Why is the endpoint separated? Why not just
+    # "/api/zero-trust-finding-event/<string:finding_id>"?
     urls = ["/api/zero-trust/finding-event/<string:finding_id>"]
 
     @jwt_required

monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py

@@ -17,6 +17,7 @@ REPORT_DATA_PRINCIPLES_STATUS = "principles"
 
 
 class ZeroTrustReport(AbstractResource):
+    # API Spec: This is an action and there's no "resource"; RPC-style endpoint?
     urls = ["/api/report/zero-trust/<string:report_data>"]
 
     @jwt_required