From 84db27c847e1249ad57bb92893dee61f8f3fdc37 Mon Sep 17 00:00:00 2001 From: PagesCoffy Date: Wed, 27 Nov 2024 09:05:07 +0000 Subject: [PATCH] [Integration][Snyk] - Validation to Prevent Fetching Non Existent Users (#1098) # Description What - Customers had the impression that the integration was bringing data from other organizations that were not specified during the installation process. The particular cause of concern was the Snyk API that enriches the project data with the importer and owner details. It is possible that an importer or the owner of the Snyk project have left the organization. In this instance, when we query the API, we will get 404, which is expected. But the fact that the customer sees an API call to the owners new organization makes it appear that the integration is pulling data from other org. Why - How - Added validation to prevent the integration from making attempts to fetch users from other organisation instead of the ones provided to the integration ## Type of change Please leave one option from the following and delete the rest: - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] New Integration (non-breaking change which adds a new integration) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Non-breaking change (fix of existing functionality that will not change current behavior) - [ ] Documentation (added/updated documentation)

All tests should be run against the port production environment(using a testing org).

### Core testing checklist - [ ] Integration able to create all default resources from scratch - [ ] Resync finishes successfully - [ ] Resync able to create entities - [ ] Resync able to update entities - [ ] Resync able to detect and delete entities - [ ] Scheduled resync able to abort existing resync and start a new one - [ ] Tested with at least 2 integrations from scratch - [ ] Tested with Kafka and Polling event listeners - [ ] Tested deletion of entities that don't pass the selector ### Integration testing checklist - [ ] Integration able to create all default resources from scratch - [ ] Resync able to create entities - [ ] Resync able to update entities - [ ] Resync able to detect and delete entities - [ ] Resync finishes successfully - [ ] If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the `examples` folder in the integration directory. - [ ] If resource kind is updated, run the integration with the example data and check if the expected result is achieved - [ ] If new resource kind is added or updated, validate that live-events for that resource are working as expected - [ ] Docs PR link [here](#) ### Preflight checklist - [ ] Handled rate limiting - [ ] Handled pagination - [ ] Implemented the code in async - [ ] Support Multi account ## Screenshots Include screenshots from your environment showing how the resources of the integration will look. ## API Documentation Provide links to the API documentation used for this integration. --------- Co-authored-by: shariff-6 Co-authored-by: Tom Tankilevitch <59158507+Tankilevitch@users.noreply.github.com> --- integrations/snyk/CHANGELOG.md | 8 + integrations/snyk/pyproject.toml | 2 +- integrations/snyk/snyk/client.py | 37 +--- .../snyk/test_snyk_client_user_details.py | 191 ++++++++++++++++++ 4 files changed, 211 insertions(+), 27 deletions(-) create mode 100644 integrations/snyk/tests/snyk/test_snyk_client_user_details.py diff --git a/integrations/snyk/CHANGELOG.md b/integrations/snyk/CHANGELOG.md index 1c739b3e6f..1e65b9dfba 100644 --- a/integrations/snyk/CHANGELOG.md +++ b/integrations/snyk/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 +## 0.1.106 (2024-11-26) + + +### Improvements + +- Added validation to skip fetching user details if the user_reference does not belong to an organization associated with the integration, preventing unnecessary API calls for non-existent users + + ## 0.1.105 (2024-11-25) diff --git a/integrations/snyk/pyproject.toml b/integrations/snyk/pyproject.toml index 719b1cac9e..c5c14f9cea 100644 --- a/integrations/snyk/pyproject.toml +++ b/integrations/snyk/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "snyk" -version = "0.1.105" +version = "0.1.106" description = "Snyk integration powered by Ocean" authors = ["Isaac Coffie "] diff --git a/integrations/snyk/snyk/client.py b/integrations/snyk/snyk/client.py index e86af6681f..930302176b 100644 --- a/integrations/snyk/snyk/client.py +++ b/integrations/snyk/snyk/client.py @@ -262,38 +262,23 @@ async def get_single_project(self, org_id: str, project_id: str) -> dict[str, An return project - async def _get_user_details_old(self, user_id: str | None) -> dict[str, Any]: + async def _get_user_details(self, user_reference: str | None) -> dict[str, Any]: if ( - not user_id + not user_reference ): ## Some projects may not have been assigned to any owner yet. In this instance, we can return an empty dict return {} - cached_details = event.attributes.get(f"{CacheKeys.USER}-{user_id}") - if cached_details: - return cached_details - - try: - user_details = await self._send_api_request( - url=f"{self.api_url}/user/{user_id}" - ) - if not user_details: - return {} + # The user_reference is in the format of /rest/orgs/{org_id}/users/{user_id}. Some users may not be associated with the organization that the integration is configured with. In this instance, we can return an empty dict + reference_parts = user_reference.split("/") + org_id = reference_parts[3] + user_id = reference_parts[-1] - event.attributes[f"{CacheKeys.USER}-{user_id}"] = user_details - return user_details - except httpx.HTTPStatusError as e: - if e.response.status_code == 404: - logger.debug(f"user {user_id} not was not found, skipping...") - return {} - else: - raise - - async def _get_user_details(self, user_reference: str | None) -> dict[str, Any]: - if ( - not user_reference - ): ## Some projects may not have been assigned to any owner yet. In this instance, we can return an empty dict + if self.organization_ids and org_id not in self.organization_ids: + logger.debug( + f"User {user_id} in organization {org_id} is not associated with any of the organizations provided to the integration org_id. Skipping..." + ) return {} - user_id = user_reference.split("/")[-1] + user_cache_key = f"{CacheKeys.USER}-{user_id}" user_reference = user_reference.replace("/rest", "") cached_details = event.attributes.get(user_cache_key) diff --git a/integrations/snyk/tests/snyk/test_snyk_client_user_details.py b/integrations/snyk/tests/snyk/test_snyk_client_user_details.py new file mode 100644 index 0000000000..8db3f95dc0 --- /dev/null +++ b/integrations/snyk/tests/snyk/test_snyk_client_user_details.py @@ -0,0 +1,191 @@ +import pytest +from unittest.mock import patch, AsyncMock, MagicMock +import httpx +from typing import Generator +from port_ocean.context.event import event +from port_ocean.context.event import event_context +from port_ocean.exceptions.context import PortOceanContextAlreadyInitializedError +from port_ocean.context.ocean import initialize_port_ocean_context +from snyk.client import SnykClient +from aiolimiter import AsyncLimiter + +MOCK_API_URL = "https://api.test.com" +MOCK_TOKEN = "test-token" +MOCK_ORG_URL = "https://test.com" +MOCK_PERSONAL_ACCESS_TOKEN = "test-personal-token" + + +# Port Ocean Mocks +@pytest.fixture(autouse=True) +def mock_ocean_context() -> None: + """Fixture to mock the Ocean context initialization.""" + try: + mock_ocean_app = MagicMock() + mock_ocean_app.config.integration.config = { + "organization_url": MOCK_ORG_URL, + "personal_access_token": MOCK_PERSONAL_ACCESS_TOKEN, + } + mock_ocean_app.integration_router = MagicMock() + mock_ocean_app.port_client = MagicMock() + initialize_port_ocean_context(mock_ocean_app) + except PortOceanContextAlreadyInitializedError: + pass + + +class TestSnykClientUserDetails: + @pytest.fixture + def snyk_client(self) -> SnykClient: + """Fixture to create a SnykClient instance for testing.""" + return SnykClient( + token=MOCK_TOKEN, + api_url=MOCK_API_URL, + app_host=None, + organization_ids=["org123"], + group_ids=None, + webhook_secret=None, + rate_limiter=AsyncLimiter(5, 1), + ) + + @pytest.fixture + def mock_event_context(self) -> Generator[MagicMock, None, None]: + """Create a mock event context for tests""" + mock_event = MagicMock() + mock_event.attributes = {} + + with patch("port_ocean.context.event.event", mock_event): + yield mock_event + + @pytest.mark.asyncio + async def test_none_user_reference(self, snyk_client: SnykClient) -> None: + """Test handling of None user reference""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + result = await snyk_client._get_user_details(None) + assert result == {} + mock_send_api_request.assert_not_called() + + @pytest.mark.asyncio + async def test_user_from_different_org(self, snyk_client: SnykClient) -> None: + """Test handling of user from non-configured organization""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_reference = "/rest/orgs/different_org/users/user123" + + # Act + result = await snyk_client._get_user_details(user_reference) + + # Assert + assert result == {} + mock_send_api_request.assert_not_called() + + @pytest.mark.asyncio + async def test_cached_user_details(self, snyk_client: SnykClient) -> None: + """Test retrieval of cached user details""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_id = "user123" + user_reference = f"/rest/orgs/org123/users/{user_id}" + cached_data = {"data": {"id": user_id, "name": "Test User"}} + event.attributes[f"user-{user_id}"] = cached_data + + # Act + result = await snyk_client._get_user_details(user_reference) + + # Assert + assert result == cached_data + mock_send_api_request.assert_not_called() + + @pytest.mark.asyncio + async def test_successful_user_details_fetch(self, snyk_client: SnykClient) -> None: + """Test successful user details fetch""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_id = "user123" + user_reference = f"/rest/orgs/org123/users/{user_id}" + api_response = {"data": {"id": user_id, "name": "Test User"}} + mock_send_api_request.return_value = api_response + + # Act + result = await snyk_client._get_user_details(user_reference) + + # Assert + assert result == api_response["data"] + mock_send_api_request.assert_called_once_with( + url=f"{MOCK_API_URL}/rest/orgs/org123/users/{user_id}", + query_params={"version": "2024-06-21~beta"}, + ) + assert event.attributes[f"user-{user_id}"] == api_response + + @pytest.mark.asyncio + async def test_404_error_handling(self, snyk_client: SnykClient) -> None: + """Test 404 error handling""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_reference = "/rest/orgs/org123/users/user123" + mock_response = MagicMock(spec=httpx.Response) + mock_response.status_code = 404 + error = httpx.HTTPStatusError( + "Not Found", + request=MagicMock(spec=httpx.Request), + response=mock_response, + ) + mock_send_api_request.side_effect = error + + # Act + result = await snyk_client._get_user_details(user_reference) + + # Assert + assert result == {} + + @pytest.mark.asyncio + async def test_non_404_error_handling(self, snyk_client: SnykClient) -> None: + """Test non-404 error handling""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_reference = "/rest/orgs/org123/users/user123" + mock_response = MagicMock(spec=httpx.Response) + mock_response.status_code = 500 + error = httpx.HTTPStatusError( + "Server Error", + request=MagicMock(spec=httpx.Request), + response=mock_response, + ) + mock_send_api_request.side_effect = error + + # Act/Assert + with pytest.raises(httpx.HTTPStatusError): + await snyk_client._get_user_details(user_reference) + + @pytest.mark.asyncio + async def test_empty_api_response(self, snyk_client: SnykClient) -> None: + """Test empty API response""" + with patch.object( + snyk_client, "_send_api_request", new_callable=AsyncMock + ) as mock_send_api_request: + async with event_context("test_event"): + # Arrange + user_reference = "/rest/orgs/org123/users/user123" + mock_send_api_request.return_value = None + + # Act + result = await snyk_client._get_user_details(user_reference) + + # Assert + assert result == {}