Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: OpenCTI Improvements #37270

Open
wants to merge 39 commits into
base: contrib/jesusgpo_feature/opencti-rename-indicator-commands-to-observable-1
Choose a base branch
from

Conversation

jesusgpo
Copy link
Contributor

@jesusgpo jesusgpo commented Nov 18, 2024

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

In progess doing test, waiting for paloalto review

Description

  • Rename indicators commands to observable commands according to OpenCTI naming the deprecation
    • opencti-get-indicators -> opencti-get-observables
    • opencti-indicator-delete -> opencti-observable-delete
    • opencti-indicator-field-update -> opencti-observable-field-update
    • opencti-indicator-create -> opencti-observable-create
    • opencti-indicator-field-add -> opencti-observable-field-add
    • opencti-indicator-field-remove -> opencti-observable-field-remove
  • Develop Incident commands
    • opencti-incident-create
    • opencti-incident-delete
    • opencti-incident-types-list
  • Develop indicators commands
    • opencti-indicator-create
    • opencti-indicator-update
    • opencti-indicator-field-add
    • opencti-indicator-field-remove
    • opencti-get-indicators
    • opencti-indicator-types-list
    • opencti-relationship-create: Required to create entities relations

Must have

  • Tests
  • Documentation

@samuelFain samuelFain self-assigned this Nov 18, 2024
@samuelFain samuelFain requested review from samuelFain and removed request for MLainer1 November 18, 2024 14:49
@samuelFain
Copy link
Contributor

@jesusgpo Following our sync on this contribution:

  1. Please make sure you sign the Contribution Registration form so I can start my review on this PR.
  2. We are currently reviewing the original commands depreciation issue and whether we should keep or completely remove them, I will update you as soon as I have more information.

Thanks!

@jesusgpo
Copy link
Contributor Author

jesusgpo commented Nov 20, 2024

@jesusgpo Following our sync on this contribution:

  1. Please make sure you sign the Contribution Registration form so I can start my review on this PR.
  2. We are currently reviewing the original commands depreciation issue and whether we should keep or completely remove them, I will update you as soon as I have more information.

Thanks!

Hi Samuel, I have just completed the form and I filled the description with the commands developed and updated, my next step is develop the tests for the commands

@samuelFain
Copy link
Contributor

@jesusgpo Following our sync on this contribution:

Hi Samuel, I have just completed the form and I filled the description with the commands developed and updated, my next step is develop the tests for the commands

Thanks for the update! I will start reviewing the PR.

Copy link
Contributor

@samuelFain samuelFain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesusgpo You have done amazing work so far!
I reviewed most of the command changes so I'm posting my initial review for them.
I will be reviewing the incident commands (3 commands), test file and other file changes in this PR shortly.
In the meanwhile, please see my comments, some of them might have already been addressed by your latests commits.
Also, it seems like you have a conflict in README.md.

Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.py Show resolved Hide resolved
Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.py Show resolved Hide resolved
Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.py Show resolved Hide resolved
Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.yml Outdated Show resolved Hide resolved
Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.yml Outdated Show resolved Hide resolved
@samuelFain
Copy link
Contributor

samuelFain commented Nov 27, 2024

@jesusgpo A few more issues that came up when running demisto-sdk pre-commit command on your PR (I strongly suggest you use it as well, more about this command here):

  • ruff error code E731:

    Do not assign a lambda expression, use a def.

    For example in: query = lambda *args, **kwargs: None.

    Full error:

    Packs/OpenCTI/Integrations/OpenCTI/OpenCTI_test.py:11:5: E731 Do not assign a lambda expression, use a def
    Found 1 error.

  • Getting a mypy linting error where the indicator and main_observable_type arguments have a type of Optional[str] but expects str for this line:
    pattern = build_stix_pattern(indicator, main_observable_type)

    Full error:

    Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.py:935: error: Argument 1 to
    "build_stix_pattern" has incompatible type "Optional[str]"; expected "str"
    [arg-type]
    pattern = build_stix_pattern(indicator, main_observable_type)
    ^~~~~~~~~
    Packs/OpenCTI/Integrations/OpenCTI/OpenCTI.py:935: error: Argument 2 to
    "build_stix_pattern" has incompatible type "Optional[str]"; expected "str"
    [arg-type]
    pattern = build_stix_pattern(indicator, main_observable_type)
    ^~~~~~~~~~~~~~~~~~~~
    Found 2 errors in 1 file (checked 1 source file)

  • I also have 3 UTs failing when trying to run them via pytest on the docker container, not sure if there is real issue with them or not, we can wait until the conflict in Packs/OpenCTI/Integrations/OpenCTI/README.md is resolved and then pre-commit will run automatically inside one of this PRs Github actions (also runs the UTs), giving us something to compare with:

    • test_incident_create_command_exception
    • test_incident_types_list_command_exception
    • OpenCTI_test.py::test_incident_create_command_exception_id

@jesusgpo jesusgpo force-pushed the feature/opencti-rename-indicator-commands-to-observable branch from 88217b3 to 49147b1 Compare November 28, 2024 09:36
@jesusgpo jesusgpo force-pushed the feature/opencti-rename-indicator-commands-to-observable branch from 49147b1 to ac16fee Compare November 28, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Form Filled Whether contribution form filled or not. Contribution Thank you! Contributions are always welcome! External PR Partner Xsoar Support Level Indicates that the contribution is for XSOAR supported pack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants