From b61f2ce41fd5f6ddb8fa771af3b6520b81d37ca1 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 23 Feb 2023 14:23:57 +0100 Subject: [PATCH] patch minor sync issue when HTTP 302 is received from Grafana API instance (#1393) # What this PR does this PR refactors the `sync_organization` and `GrafanaAPIClient.is_rbac_enabled_for_organization` methods to check the connected response bool rather than explicit check on HTTP 200. This handles the legitimate case where the Grafana instance may return an HTTP 302 (redirect) rather than an HTTP 200. ## Which issue(s) this PR fixes See [this](https://grafana.slack.com/archives/C02LSUUSE2G/p1677136582890269) Slack thread in the community channel for more context ## Checklist - [x] Tests updated - [ ] Documentation added (N/A) - [x] `CHANGELOG.md` updated --- CHANGELOG.md | 5 +++++ engine/apps/grafana_plugin/helpers/client.py | 2 +- .../grafana_plugin/tests/test_grafana_api_client.py | 11 +++++------ engine/apps/user_management/sync.py | 2 +- engine/apps/user_management/tests/test_sync.py | 6 +++--- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3e91695..4a303404 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Allow creating schedules with type "web" using public API +### Fixed + +- Fixed minor issue during the sync process where an HTTP 302 (redirect) status code from the Grafana + instance would cause the sync to not properly finish + ## v1.1.28 (2023-02-23) ### Fixed diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index edad4812..41e0e29a 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -152,7 +152,7 @@ class GrafanaAPIClient(APIClient): def is_rbac_enabled_for_organization(self) -> bool: _, resp_status = self.api_head(self.USER_PERMISSION_ENDPOINT) - return resp_status["status_code"] == status.HTTP_200_OK + return resp_status["connected"] def get_users(self, rbac_is_enabled_for_org: bool, **kwargs) -> List[GrafanaUserWithPermissions]: users, _ = self.api_get("api/org/users", **kwargs) diff --git a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py index 67e9e575..4dcb70c1 100644 --- a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py @@ -1,7 +1,6 @@ from unittest.mock import patch import pytest -from rest_framework import status from apps.grafana_plugin.helpers.client import GrafanaAPIClient @@ -44,17 +43,17 @@ class TestGetUsersPermissions: class TestIsRbacEnabledForOrganization: @pytest.mark.parametrize( - "grafana_api_status_code,expected", + "api_response_connected,expected", [ - (status.HTTP_200_OK, True), - (status.HTTP_404_NOT_FOUND, False), + (True, True), + (False, False), ], ) @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_head") def test_it_returns_based_on_status_code_of_head_call( - self, mocked_grafana_api_client_api_head, grafana_api_status_code, expected + self, mocked_grafana_api_client_api_head, api_response_connected, expected ): - mocked_grafana_api_client_api_head.return_value = (None, {"status_code": grafana_api_status_code}) + mocked_grafana_api_client_api_head.return_value = (None, {"connected": api_response_connected}) api_client = GrafanaAPIClient(API_URL, API_TOKEN) assert api_client.is_rbac_enabled_for_organization() == expected diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index 01dfabf8..93c4fb5f 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -31,7 +31,7 @@ def sync_organization(organization): _sync_instance_info(organization) _, check_token_call_status = grafana_api_client.check_token() - if check_token_call_status["status_code"] == 200: + if check_token_call_status["connected"]: organization.api_token_status = Organization.API_TOKEN_STATUS_OK sync_users_and_teams(grafana_api_client, organization) organization.last_time_synced = timezone.now() diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index 2b9dddd5..4b6cfc78 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -134,7 +134,7 @@ def test_sync_organization(make_organization, make_team, make_user_for_organizat }, ) - api_check_token_call_status = {"status_code": 200} + api_check_token_call_status = {"connected": True} with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=False): with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response): @@ -203,7 +203,7 @@ def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organiza "userId": 1, }, ) - api_check_token_call_status = {"status_code": 200} + api_check_token_call_status = {"connected": True} with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=grafana_api_response): with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response): @@ -230,7 +230,7 @@ def test_sync_organization_is_rbac_permissions_enabled_cloud(mocked_gcom_client, stack_id = 5 organization = make_organization(stack_id=stack_id) - api_check_token_call_status = {"status_code": 200} + api_check_token_call_status = {"connected": True} mocked_gcom_client.return_value.is_rbac_enabled_for_stack.return_value = gcom_api_response