Merge pull request #1957 from guardicore/1941-api-spec

API review

monkey/monkey_island/cc/app.py

@@ -173,6 +173,7 @@ def init_api_resources(api: FlaskDIWrapper):
     api.add_resource(Log)
     api.add_resource(IslandLog)
 
+    # API Spec: These two should be the same resource, GET for download and POST for upload
     api.add_resource(PBAFileDownload)
     api.add_resource(FileUpload)
 
@@ -183,6 +184,9 @@ 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
+    # Note: Preferably, the API will provide a rich feature set and allow access to all of the
+    #       necessary data. This would make these endpoints obsolete.
     api.add_resource(MonkeyBlackboxEndpoint)
     api.add_resource(ClearCaches)
     api.add_resource(LogBlackboxEndpoint)

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

@@ -3,6 +3,8 @@ from monkey_island.cc.services.infection_lifecycle import should_agent_die
 
 
 class StopAgentCheck(AbstractResource):
+    # API Spec: Rename to AgentStopStatus or something, endpoint for this could be
+    # "/api/agents/<GUID>/stop-status"
     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/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

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_import.py

@@ -39,6 +39,7 @@ class ResponseContents:
 
 
 class ConfigurationImport(AbstractResource):
+    # API Spec: Should probably be merged with IslandConfiguration
     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/island_configuration.py

@@ -21,9 +21,16 @@ class IslandConfiguration(AbstractResource):
     @jwt_required
     def post(self):
         config_json = json.loads(request.data)
+        # API Spec: Makes more sense to have a PATCH request for this since the resource,
+        # i.e. the configuration, is being updated.
         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)?
+        # Simplest way it to just send a GET request after this to get the updated config.
         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,7 @@ logger = logging.getLogger(__name__)
 
 
 class IslandMode(AbstractResource):
+    # API Spec: Instead of POST, this could just be PATCH
     urls = ["/api/island-mode"]
 
     @jwt_required
@@ -33,6 +34,7 @@ class IslandMode(AbstractResource):
             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,9 +10,11 @@ from monkey_island.cc.services.run_local_monkey import LocalMonkeyRunService
 
 
 class LocalRun(AbstractResource):
-
     urls = ["/api/local-monkey"]
 
+    # API Spec: Both of these methods should be separated to their own resources
+
+    # API Spec: This should be a REST endpoint, /api/monkeys or something
     @jwt_required
     def get(self):
         island_monkey = NodeService.get_monkey_island_monkey()
@@ -23,12 +25,16 @@ class LocalRun(AbstractResource):
 
         return jsonify(is_running=is_monkey_running)
 
+    # API Spec: This should be an RPC-style endpoint
     @jwt_required
     def post(self):
         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,8 @@ class UnsupportedOSError(Exception):
 
 
 class MonkeyDownload(AbstractResource):
+    # API Spec: Rename to /api/agent-binaries, because information about agent runs
+    # and binary files are different resources
     urls = ["/api/agent/download/<string:host_os>"]
 
     # Used by monkey. can't secure.

monkey/monkey_island/cc/resources/pba_file_upload.py

@@ -18,6 +18,7 @@ WINDOWS_PBA_TYPE = "PBAwindows"
 
 
 class FileUpload(AbstractResource):
+    # API Spec: FileUpload -> PBAFileUpload. Change endpoint accordingly.
     """
     File upload endpoint used to send/receive Custom PBA files
     """
@@ -80,6 +81,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 +103,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/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 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/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/version_update.py

@@ -8,6 +8,7 @@ logger = logging.getLogger(__name__)
 
 
 class VersionUpdate(AbstractResource):
+    # API Spec: Rename to /version-info
     urls = ["/api/version-update"]
 
     def __init__(self):

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