-
Notifications
You must be signed in to change notification settings - Fork 39
Tenable: Change the logic of assets #2071
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -67,6 +67,10 @@ def __init__(self, *args, **kwargs): | |||||||||||||||||||||||||||||||||||||||||||||
| start_at=timedelta(days=120), | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
| self._latest_time = self.cursor.offset | ||||||||||||||||||||||||||||||||||||||||||||||
| self._asset_map: dict[str, dict] | None = {} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
| self._asset_map: dict[str, dict] | None = {} | |
| self._asset_map: dict[str, dict] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Clarify _asset_map optionality and initialization pattern.
The type and usage of _asset_map are inconsistent: it’s annotated as dict[str, dict] | None but initialized as {}, and _get_asset_info checks if not self._asset_map or self._should_refresh_asset_cache():. Either:
- Use
Noneto mean “not yet built”: initialize_asset_maptoNone, checkif self._asset_map is None or self._should_refresh_asset_cache():, and have_build_asset_mapalways assign a dict; or - Drop the
Nonefrom the type and rely solely on the refresh logic with a non-optionaldict[str, dict].
That keeps the type hints and control flow aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Return type annotation for _build_asset_map does not match actual behavior.
The method is annotated as returning dict[str, dict] but only mutates self._asset_map and returns nothing, which will cause type-checker errors and mislead callers. Please either return self._asset_map or change the return type to None to match the current behavior.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_build_asset_map() clears self._asset_map before the export loop. If the export call fails part-way (exception), the connector will be left with an empty cache, increasing downstream API calls; consider populating a temporary dict and only replacing the cache on success.
| self._asset_map.clear() | |
| for asset in self.client.exports.assets_v2(since=self._latest_time, chunk_size=4000): | |
| asset_uuid = asset.get("id") | |
| if asset_uuid: | |
| self._asset_map[asset_uuid] = asset | |
| asset_count += 1 | |
| if asset_count >= self._max_cache_size: | |
| self.log(f"Stop uploading, limit exceeded ({self._max_cache_size})", level="warning") | |
| break | |
| temp_asset_map: dict[str, dict] = {} | |
| for asset in self.client.exports.assets_v2(since=self._latest_time, chunk_size=4000): | |
| asset_uuid = asset.get("id") | |
| if asset_uuid: | |
| temp_asset_map[asset_uuid] = asset | |
| asset_count += 1 | |
| if asset_count >= self._max_cache_size: | |
| self.log(f"Stop uploading, limit exceeded ({self._max_cache_size})", level="warning") | |
| break | |
| self._asset_map = temp_asset_map |
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_build_asset_map is annotated/documented as returning dict[str, dict], but it never returns anything (implicitly returns None). Either return the built map explicitly or change the return annotation/docstring to None to avoid misleading callers and type-checking issues.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not self._asset_map or self._should_refresh_asset_cache() treats an empty cache as 'not built'. If the export legitimately returns 0 assets, _get_asset_info will rebuild the cache on every call (potentially an expensive export loop). Prefer checking an explicit sentinel (e.g., self._asset_map is None) for the 'never built' case.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||
| from datetime import datetime, timedelta | ||||||||
| from unittest.mock import patch, Mock, MagicMock | ||||||||
| import pytest | ||||||||
| from unittest.mock import patch, Mock | ||||||||
|
|
||||||||
| from sekoia_automation.asset_connector.models.ocsf.vulnerability import VulnerabilityOCSFModel | ||||||||
| from sekoia_automation.asset_connector.models.ocsf.device import ( | ||||||||
|
|
@@ -271,6 +272,8 @@ def tenable_asset_connector(symphony_storage): | |||||||
| yield test_tenable_asset_connector | ||||||||
|
|
||||||||
|
|
||||||||
| # ==================== Tests existants ==================== | ||||||||
|
|
||||||||
| def test_states_property(tenable_asset_connector): | ||||||||
| expected = [state.value for state in VulnerabilityState] | ||||||||
| assert tenable_asset_connector.states == expected | ||||||||
|
|
@@ -603,8 +606,7 @@ def test_get_asset_info_exception(tenable_asset_connector): | |||||||
| with patch.object(tenable_asset_connector.client.assets, "details", side_effect=Exception("API Error")): | ||||||||
| result = tenable_asset_connector._get_asset_info("test-uuid") | ||||||||
|
|
||||||||
| assert result is None | ||||||||
| tenable_asset_connector.log.assert_called() | ||||||||
| assert result is Nonetenable_asset_connector.log.assert_called() | ||||||||
|
||||||||
| assert result is Nonetenable_asset_connector.log.assert_called() | |
| assert result is None | |
| tenable_asset_connector.log.assert_called() |
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_get_asset_info_exception is defined twice in this file (once around lines 605-610 and again around 686-694). Pytest will keep only the last definition, which can hide failures and makes the test suite confusing; rename one of them or remove the duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add tests for _build_asset_map error handling and cache-size limit behavior
Current tests cover the happy path of _build_asset_map and cache usage in _get_asset_info, but not the error/limit branches. Please also add tests that:
- Simulate
client.exports.assets_v2raising to assertlog_exceptionis called and that_build_asset_mapexits cleanly. - Yield more than
_max_cache_sizeassets to assert the map is capped at_max_cache_sizeand that the warning"Stop uploading, limit exceeded (...)"is logged.
That will exercise the failure and limit behavior of the new export/caching logic under load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (typo): Consider rephrasing this line for grammatical correctness and clarity.
The phrase is a bit awkward and also uses “assets details” instead of the more natural “asset details.” Consider rewording to something like: