CIAC-16805: Add Token URL parameter for OAuth 2.0 endpoint override in FireEye ETP#45037
CIAC-16805: Add Token URL parameter for OAuth 2.0 endpoint override in FireEye ETP#45037lironcohen272 wants to merge 1 commit into
Conversation
🤖 AI-Powered Code Review AvailableYou can leverage AI-powered code review to assist with this PR! Available Commands:
|
|
|
|
🤖 Analysis started. Please wait for results... |
🤖 AI Review DisclaimerThis review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause. |
marketplace-ai-reviewer
left a comment
There was a problem hiding this comment.
Thanks for your contribution to the FireEye ETP pack! I've reviewed your changes and left a few notes to help align the integration with our standards.
Most importantly, since a new parameter was added, this requires a minor version bump (2.2.0) rather than a patch. You'll also need to adjust the YAML files so the root keys start with name or display, add the required isFetchEvents parameter to the event collector, and clean up some of the unit tests by avoiding __init__ mocks.
Thanks again, and let me know if you have any questions!
Additionally, please address the following file-level notes:
Packs/FireEyeETP/Integrations/FireEyeETP/FireEyeETP.yml: CRITICAL: The first key defined at the root level of the.ymlfile must always benameordisplay. Currently, it starts withcommonfields. Please movenameordisplayto the top of the file.Packs/FireEyeETP/Integrations/FireEyeETPEventCollector/FireEyeETPEventCollector.yml: - CRITICAL: The first key defined at the root level of the.ymlfile must always benameordisplay. Currently, it starts withcategory. Please movenameordisplayto the top of the file.- As per Event Collector guidelines, all event collectors must configure the
isFetchEventsparameter in theconfigurationsection. Please add it:
- display: Fetch Events
name: isFetchEvents
type: 8
required: false- The
fireeye-etp-get-eventscommand description must end with the exact sentence: "Use this command for development and debugging only, as it may produce duplicate events, exceed API rate limits, or disrupt the fetch mechanism." Please update the description to comply with the Event Collector guidelines. Packs/FireEyeETP/ReleaseNotes/2_1_6.md: The release notes describe adding a new parameter (Added the *Token URL* parameter), which is a feature addition. According to the versioning guidelines, adding new features requires a minor version bump, but the current version is2.1.6(a patch bump). Please bump the version to2.2.0and rename this file to2_2_0.md. You can do this by runningdemisto-sdk update-release-notes -i Packs/FireEyeETP -u minor.
For more information, see the Release Notes Documentation.
@kamalq97, @lironcohen272 please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.
| Then: | ||
| - Ensure the OAuth token request is sent to the expected resolved URL. | ||
| """ | ||
| mocker.patch.object( |
There was a problem hiding this comment.
Avoid mocking __init__ and manually replicating its logic. You can instantiate the Client normally by setting up your mocks beforehand.
| assert actual_url == expected_url | ||
|
|
||
|
|
||
| def test_token_url_resolution_with_override(): |
There was a problem hiding this comment.
Remove tests that only verify standard library functions or constant values.
| name: token_url | ||
| type: 0 | ||
| required: false | ||
| additionalinfo: "Override the OAuth 2.0 token endpoint base URL. Leave empty to use the default Trellix IAM endpoint. For Trellix GovCloud tenants, set to https://iam.us.trellix-gov.com." |
There was a problem hiding this comment.
The additionalinfo description should start with "The " and end with a period, and use a descriptive form rather than an active form ("Override...").
| name: token_url | ||
| type: 0 | ||
| required: false | ||
| additionalinfo: "Override the OAuth 2.0 token endpoint base URL. Leave empty to use the default Trellix IAM endpoint. For Trellix GovCloud tenants, set to https://iam.us.trellix-gov.com." |
There was a problem hiding this comment.
The additionalinfo description should start with "The " and end with a period, and use a descriptive form rather than an active form ("Override...").
|
|
||
| ##### Trellix Email Security - Cloud | ||
|
|
||
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use `https://iam.us.trellix-gov.com` for Trellix GovCloud tenants. |
There was a problem hiding this comment.
Literal values surfaced to the user, such as configuration values or URLs, should be styled in italics rather than backticks. See Entity Styling for more details.
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use `https://iam.us.trellix-gov.com` for Trellix GovCloud tenants. | |
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use *https://iam.us.trellix-gov.com* for Trellix GovCloud tenants. |
|
|
||
| ##### Trellix Email Security - Cloud Event Collector | ||
|
|
||
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use `https://iam.us.trellix-gov.com` for Trellix GovCloud tenants. |
There was a problem hiding this comment.
Literal values surfaced to the user, such as configuration values or URLs, should be styled in italics rather than backticks. See Entity Styling for more details.
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use `https://iam.us.trellix-gov.com` for Trellix GovCloud tenants. | |
| - Added the *Token URL* parameter to allow overriding the OAuth 2.0 token endpoint per-instance. Use *https://iam.us.trellix-gov.com* for Trellix GovCloud tenants. |
kamalq97
left a comment
There was a problem hiding this comment.
Approved from my end (a couple of minor things to validate below)
@DanielTal87 Please review the event collector changes and let us know if you have any feedback
|
Validate summary Verdict: PR can be force merged from validate perspective? ✅ |
🔍 AI Triage Report AvailableAn automated triage report has been generated for this pipeline. Status: 📋 Triage Report
|
Related Issues
related: CIAC-16806
Description
Add Token URL parameter for OAuth 2.0 endpoint override in FireEye ETP