Topdesk Assetmanagement API

[WildDogOne]

Status

• [ ] In Progress
• [x] Ready
• [ ] In Hold - (Reason for hold)

Description

This addition to the package will be used to add the TOPdesk assetmanagement API to the existing TOPdesk integration.

Must have

• [ ] Tests
• [ ] Documentation

Discussion Points

The TOPdesk Asset management API returns fields sometimes with an @ prepended. For the user this might be annoying, so I cut them off. However, this cutting off could confuse people since the asset list function can be customised to return specific fields, and of course when a user configures that feature, they have to use the actual TOPdesk fields with @, not the human readable fields I push to context

[content-bot]

Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @mmhw will know the proposed changes are ready to be reviewed. For your convenience, here is a link to the contributions SLAs document.

[content-bot]

Hi @WildDogOne, thanks for contributing to the XSOAR marketplace. To receive credit for your generous contribution please follow this link.

[itssapir]

Hi @WildDogOne, thank you for your contribution!

The PR is marked as a draft, is it still in progress? When ready please ensure you mark it as ready for review, Thanks!

[WildDogOne]

@itssapir I think I am now ready for review. There are some issues however:

1. The TOPdesk Asset management API returns fields sometimes with an @ prepended. For the user this might be annoying, so I cut them off. It also seems that there is a function "capitalize_for_outputs" which is used to capitalize the keys for the output context. However, this cutting off amd capitalizing could confuse people since the asset list function can be customised to return specific fields, and of course when a user configures that feature, they have to use the actual TOPdesk fields with @ and without caps, not the human readable fields I push to context. Please advise

2. The Unittest is a mystery to me. I have added a test "test_assets_list", and it works, but I don't know why. Also I am not sure how to mock a test for pushing data, so for now there is no test on that.

[itssapir]

@itssapir I think I am now ready for review. There are some issues however:

1. The TOPdesk Asset management API returns fields sometimes with an @ prepended. For the user this might be annoying, so I cut them off. It also seems that there is a function "capitalize_for_outputs" which is used to capitalize the keys for the output context. However, this cutting off amd capitalizing could confuse people since the asset list function can be customised to return specific fields, and of course when a user configures that feature, they have to use the actual TOPdesk fields with @ and without caps, not the human readable fields I push to context. Please advise
2. The Unittest is a mystery to me. I have added a test "test_assets_list", and it works, but I don't know why. Also I am not sure how to mock a test for pushing data, so for now there is no test on that.

Hi @WildDogOne, Firstly, thank you for your contribution :) Regarding point 1, If the asset names are expected to be used for other inputs/commands, we want them to be returned in the usable format. When would the user expect the "@" sign to not be there? from your description it sounds to me that we should not be removing it, some elaboration could help here.

Regarding 2, I can give you some pointers over slack, feel free to reach out and we can set up some time to talk.

[WildDogOne]

@itssapir The problem is that is is an existing integration. And I noticed that the other commands make the output more human readable. I personally prefer to adjust my code to be as similar to the existing as possible. So I built the output in the same style. But it could cause confusion since if people look at the API documentation, they would assume the fields to come in a different format.

[itssapir]

@WildDogOne I'm not sure I completely understand, lets set up a call to discuss this, can you reach out to me in the DFIR community?

[WildDogOne]

Hi @itssapir I just tried to join the slack, but that did not work

so basically I am stuck :)

[itssapir]

@WildDogOne can you please give me a full name and email address, I'll see to it that you are added.

[itssapir]

@WildDogOne, Please see the remaining comments and look at the pre-commit errors that need to be fixed.

[richardbluestone]

Doc review done

[CLAassistant]

All committers have signed the CLA.

[content-bot]

For the Reviewer: Trigger build request has been accepted for this contribution PR.

[content-bot]

For the Reviewer: Successfully created a pipeline in GitLab with url: https://gitlab.xdr.pan.local/xdr/cortex-content/content/-/pipelines/3965871

[content-bot]

Validate summary The following errors were thrown as a part of this pr: RN106, RM110. The following errors can be ignored: RM110. The following errors cannot be ignored: RN106. The following errors don't run as part of the nightly flow and therefore can be force merged: RN106.

Verdict: PR can be force merged from validate perspective? ❌

[github-actions[bot]]

Thank you for your contribution. Your external PR has been merged and the changes are now included in an internal PR for further review. The internal PR will be merged to the master branch within 3 business days.