Repository navigation
Migrate CrowdStrikeFalcon integration and fix On-Demand Scan empty JSON result - #1420
ranjanprabhat-cyber wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates the CrowdStrikeFalcon integration to a new directory structure and addresses a bug in the On-Demand Scan functionality. The fix ensures that the scan correctly handles transitional statuses and empty resource lists, preventing false success reports and improving reliability when no valid entities are in scope. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the CrowdStrike Falcon integration, adding various actions, connectors, and core utilities. Key feedback includes addressing a stale timestamp issue in the OnDemandScan action by dynamically calculating since_time, correcting the return state of _finalize_action_on_failure to handle partial successes properly, and fixing a return type mismatch in _perform_action. Additionally, several style guide violations regarding missing type annotations and absolute imports in OnDemandScan.py must be resolved, and a duplicate requests dependency in pyproject.toml should be removed.
| HostIDsTuple = collections.namedtuple("HostIDsTuple", ["valid_hosts_map", "invalid_hosts"]) | ||
| ScanMappingsTuple = collections.namedtuple("ScanMappingsTuple", ["host_group_scan_mapping", "host_scan_mapping"]) | ||
| ScanDetailsTuple = collections.namedtuple("ScanDetailsTuple", ["host_group_scan_details", "host_scan_details"]) | ||
| since_time: str = (datetime.now(timezone.utc) - timedelta(hours=1)).strftime("%Y-%m-%dT%H:%M:%SZ") |
There was a problem hiding this comment.
The since_time variable is defined as a module-level global and evaluated at import time. In long-running environments, this timestamp will become stale and won't reflect the actual execution time of the action. It should be calculated dynamically inside the method where it is used.
| def _finalize_action_on_failure(self) -> ActionResult: | ||
| if self.results.processed_host_scans or self.results.processed_host_group_scans: | ||
| self._finalize_action_on_success() | ||
| # case: some success and some invalid + failed hosts | ||
| if self.results.invalid_hosts or self.results.failed_host_scans: | ||
| self.output_messages.append(self._some_invalid_hosts_err_msg) | ||
|
|
||
| # case: some success and some invalid + failed host groups | ||
| if self.results.invalid_host_groups or self.results.failed_host_group_scans: | ||
| self.output_messages.append(self._some_invalid_host_groups_err_msg) | ||
|
|
||
| else: | ||
| # case: no success and all invalid + failed hosts | ||
| if self.results.invalid_hosts or self.results.failed_host_scans: | ||
| self.output_messages.append(self._all_invalid_hosts_err_msg) | ||
|
|
||
| # case: no success and all invalid + failed host groups | ||
| if self.results.invalid_host_groups or self.results.failed_host_group_scans: | ||
| self.output_messages.append(self._all_invalid_host_groups_err_msg) | ||
|
|
||
| if not ( | ||
| self.results.invalid_hosts | ||
| or self.results.failed_host_scans | ||
| or self.results.invalid_host_groups | ||
| or self.results.failed_host_group_scans | ||
| ): | ||
| self.output_messages.append(self._all_invalid_hosts_err_msg) | ||
|
|
||
| return ActionResult(ExecutionState.COMPLETED, False) |
There was a problem hiding this comment.
In _finalize_action_on_failure, if there are processed scans (i.e., partial success), self._finalize_action_on_success() is called, but its return value is ignored, and the method ultimately returns ActionResult(ExecutionState.COMPLETED, False). This causes the entire action to be marked as failed even if some hosts were successfully scanned. If there is partial success, the method should return ActionResult(ExecutionState.COMPLETED, True) with the appropriate warning messages appended to self.output_messages.
def _finalize_action_on_failure(self) -> ActionResult:\n if self.results.processed_host_scans or self.results.processed_host_group_scans:\n self._finalize_action_on_success()\n # case: some success and some invalid + failed hosts\n if self.results.invalid_hosts or self.results.failed_host_scans:\n self.output_messages.append(self._some_invalid_hosts_err_msg)\n\n # case: some success and some invalid + failed host groups\n if self.results.invalid_host_groups or self.results.failed_host_group_scans:\n self.output_messages.append(self._some_invalid_host_groups_err_msg)\n\n return ActionResult(ExecutionState.COMPLETED, True)\n\n else:\n # case: no success and all invalid + failed hosts\n if self.results.invalid_hosts or self.results.failed_host_scans:\n self.output_messages.append(self._all_invalid_hosts_err_msg)\n\n # case: no success and all invalid + failed host groups\n if self.results.invalid_host_groups or self.results.failed_host_group_scans:\n self.output_messages.append(self._all_invalid_host_groups_err_msg)\n\n if not (\n self.results.invalid_hosts\n or self.results.failed_host_scans\n or self.results.invalid_host_groups\n or self.results.failed_host_group_scans\n ):\n self.output_messages.append(self._all_invalid_hosts_err_msg)\n\n return ActionResult(ExecutionState.COMPLETED, False)| if self.params.max_scan_duration and self.params.max_scan_duration < constants.DEFAULT_MAX_SCAN_DURATION: | ||
| raise ValueError('Please provide positive number in parameter "Max Scan Duration".') | ||
|
|
||
| def _perform_action(self, manager: CrowdStrikeManager) -> tuple[int, bool]: |
There was a problem hiding this comment.
The return type annotation of _perform_action is specified as tuple[int, bool], but the method actually returns ActionResult (via self._finalize_action()). This mismatch violates static type checking and should be updated to ActionResult.
| def _perform_action(self, manager: CrowdStrikeManager) -> tuple[int, bool]: | |
| def _perform_action(self, manager: CrowdStrikeManager) -> ActionResult: |
References
- Static Type Checking: All function parameters and return types must be annotated. (link)
| version = "84.0" | ||
| description = "CrowdStrike Falcon is the leader in next-generation endpoint protection, threat intelligence and incident response through cloud-based endpoint protection." | ||
| requires-python = ">=3.11,<3.12" | ||
| dependencies = [ "environmentcommon", "requests==2.32.5", "requests==2.32.5", "tipcommon", "tldextract==5.1.2",] |
There was a problem hiding this comment.
The dependency requests==2.32.5 is duplicated in the dependencies list. Please remove the duplicate entry.
| dependencies = [ "environmentcommon", "requests==2.32.5", "requests==2.32.5", "tipcommon", "tldextract==5.1.2",] | |
| dependencies = [ "environmentcommon", "requests==2.32.5", "tipcommon", "tldextract==5.1.2",] |
| from crowd_strike_falcon.core.AsyncActionBaseClass import ActionResult, AsyncActionBaseClass | ||
| from crowd_strike_falcon.core import constants | ||
| from crowd_strike_falcon.core.CrowdStrikeManager import CrowdStrikeManager | ||
| from crowd_strike_falcon.core.CrowdStrikeParser import CrowdStrikeParser | ||
| from crowd_strike_falcon.core.datamodels import OnDemandScanData | ||
| from crowd_strike_falcon.core.exceptions import ( | ||
| ODSDetectionLevelError, | ||
| ODSInvalidFilePathError, | ||
| CrowdStrikeBadRequestError, | ||
| InvalidCidError, | ||
| ) | ||
| from crowd_strike_falcon.core.utils import get_entity_original_identifier, validate_device_id |
There was a problem hiding this comment.
Internal imports must be relative per the repository style guide (e.g., from ..core.AsyncActionBaseClass import ... instead of from crowd_strike_falcon.core.AsyncActionBaseClass import ...). Please update the absolute imports on lines 33-44 to be relative.
from ..core.AsyncActionBaseClass import ActionResult, AsyncActionBaseClass\nfrom ..core import constants\nfrom ..core.CrowdStrikeManager import CrowdStrikeManager\nfrom ..core.CrowdStrikeParser import CrowdStrikeParser\nfrom ..core.datamodels import OnDemandScanData\nfrom ..core.exceptions import (\n ODSDetectionLevelError,\n ODSInvalidFilePathError,\n CrowdStrikeBadRequestError,\n InvalidCidError,\n)\nfrom ..core.utils import get_entity_original_identifier, validate_device_idReferences
- Internal imports must be relative: from ..core.Manager import ... not from Manager import ... (link)
|
|
||
|
|
||
| class OnDemandScanAction(AsyncActionBaseClass): | ||
| def __init__(self, siemplify): |
There was a problem hiding this comment.
The __init__ method is missing type annotations for its parameter siemplify and its return type. According to the repository style guide, all function parameters and return types must be annotated. Please update it to def __init__(self, siemplify: SiemplifyAction) -> None:.
| def __init__(self, siemplify): | |
| def __init__(self, siemplify: SiemplifyAction) -> None: |
References
- Static Type Checking: All function parameters and return types must be annotated. (link)
| invalid_hosts=[], | ||
| ) | ||
|
|
||
| def _extract_action_configuration(self): |
There was a problem hiding this comment.
The _extract_action_configuration method is missing a return type annotation. Please update it to def _extract_action_configuration(self) -> None:.
| def _extract_action_configuration(self): | |
| def _extract_action_configuration(self) -> None: |
References
- Static Type Checking: All function parameters and return types must be annotated. (link)
Summary
CrowdStrikeFalconintegration (v84.0) tocontent/response_integrations/google/crowd_strike_falcon.OnDemandScanreturningis_success=Truewith an empty JSON result ([]) when:GET /ods/entities/scans/v1returns unmapped transitional scan statuses (queued,initiated,started) or an emptyresourceslist right after scan creation.result_value=False).test_on_demand_scan.pycovering empty entity scope and unmapped transitional scan statuses.