diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index 6a0cdc895e..8eec042e89 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -241,9 +241,9 @@ def get_users_permissions(self) -> typing.Optional[UserPermissionsDict]: return all_users_permissions - def is_rbac_enabled_for_organization(self) -> bool: + def is_rbac_enabled_for_organization(self) -> tuple[bool, bool]: _, resp_status = self.api_head(self.USER_PERMISSION_ENDPOINT) - return resp_status["connected"] + return resp_status["connected"], resp_status["status_code"] >= status.HTTP_500_INTERNAL_SERVER_ERROR def get_users(self, rbac_is_enabled_for_org: bool, **kwargs) -> GrafanaUsersWithPermissions: users_response, _ = 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 65e83504c4..4e72df100e 100644 --- a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py @@ -1,6 +1,7 @@ from unittest.mock import patch import pytest +from rest_framework import status from apps.grafana_plugin.helpers.client import GrafanaAPIClient @@ -115,17 +116,21 @@ def test_it_returns_none_if_permissions_call_returns_none( class TestIsRbacEnabledForOrganization: @pytest.mark.parametrize( - "api_response_connected,expected", + "api_response_connected,api_status_code,expected", [ - (True, True), - (False, False), + (True, status.HTTP_200_OK, (True, False)), + (False, status.HTTP_404_NOT_FOUND, (False, False)), + (False, status.HTTP_503_SERVICE_UNAVAILABLE, (False, True)), ], ) @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, api_response_connected, expected + self, mocked_grafana_api_client_api_head, api_response_connected, api_status_code, expected ): - mocked_grafana_api_client_api_head.return_value = (None, {"connected": api_response_connected}) + mocked_grafana_api_client_api_head.return_value = ( + None, + {"connected": api_response_connected, "status_code": api_status_code}, + ) api_client = GrafanaAPIClient(API_URL, API_TOKEN) assert api_client.is_rbac_enabled_for_organization() == expected diff --git a/engine/apps/grafana_plugin/tests/test_self_hosted_install.py b/engine/apps/grafana_plugin/tests/test_self_hosted_install.py index b91d117413..81144866ed 100644 --- a/engine/apps/grafana_plugin/tests/test_self_hosted_install.py +++ b/engine/apps/grafana_plugin/tests/test_self_hosted_install.py @@ -100,7 +100,7 @@ def test_if_organization_exists_it_is_updated( mocked_provision_plugin.return_value = provision_plugin_response mocked_grafana_api_client.return_value.check_token.return_value = (None, {"status_code": status.HTTP_200_OK}) - mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True + mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True, False client = APIClient() url = reverse("grafana-plugin:self-hosted-install") @@ -141,7 +141,7 @@ def test_if_organization_does_not_exist_it_is_created( mocked_provision_plugin.return_value = provision_plugin_response mocked_grafana_api_client.return_value.check_token.return_value = (None, {"status_code": status.HTTP_200_OK}) - mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True + mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True, False client = APIClient() url = reverse("grafana-plugin:self-hosted-install") diff --git a/engine/apps/grafana_plugin/views/self_hosted_install.py b/engine/apps/grafana_plugin/views/self_hosted_install.py index 3148648775..88dd80235d 100644 --- a/engine/apps/grafana_plugin/views/self_hosted_install.py +++ b/engine/apps/grafana_plugin/views/self_hosted_install.py @@ -43,7 +43,7 @@ def post(self, _request: Request) -> Response: return Response(data=provisioning_info, status=status.HTTP_400_BAD_REQUEST) organization = Organization.objects.filter(stack_id=stack_id, org_id=org_id).first() - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() + rbac_is_enabled, _ = grafana_api_client.is_rbac_enabled_for_organization() if organization: organization.revoke_plugin() diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index 6000a9d1a7..42b83835c2 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -28,30 +28,14 @@ def sync_organization(organization: Organization) -> None: def _sync_organization(organization: Organization) -> None: grafana_api_client = GrafanaAPIClient(api_url=organization.grafana_url, api_token=organization.api_token) - rbac_is_enabled = organization.is_rbac_permissions_enabled - - # NOTE: checking whether or not RBAC is enabled depends on whether we are dealing with an open-source or cloud - # stack. For open-source, simply make a HEAD request to the grafana instance's API and consider RBAC enabled if - # the list RBAC permissions endpoint returns 200. - # - # For cloud, we need to check the stack's status first. If the stack is active, we can make the same HEAD request - # to the grafana instance's API. If the stack is not active, we will simply rely on the org's previous state of - # org.is_rbac_permissions_enabled - if settings.LICENSE == settings.CLOUD_LICENSE_NAME: - # We cannot simply rely on the HEAD call in cloud because if an instance is not active - # the grafana gateway will still return 200 for the HEAD request. - stack_id = organization.stack_id - gcom_client = GcomAPIClient(settings.GRAFANA_COM_ADMIN_API_TOKEN) - - if gcom_client.is_stack_active(stack_id): - # the stack MUST be active for this check.. if it is in any other state - # the Grafana API risks returning an HTTP 200 but the actual permissions data that is - # synced later on will be empty (and we'd erase all RBAC permissions stored in OnCall) - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() - else: - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() - - organization.is_rbac_permissions_enabled = rbac_is_enabled + gcom_client = GcomAPIClient(settings.GRAFANA_COM_ADMIN_API_TOKEN) + + # Update organization's RBAC status if it's an open-source instance, or it's an active cloud instance. + # Don't update non-active cloud instances (e.g. paused) as they can return 200 OK but not have RBAC enabled. + if settings.LICENSE == settings.OPEN_SOURCE_LICENSE_NAME or gcom_client.is_stack_active(organization.stack_id): + rbac_enabled, server_error = grafana_api_client.is_rbac_enabled_for_organization() + if not server_error: # Only update RBAC status if Grafana didn't return a server error + organization.is_rbac_permissions_enabled = rbac_enabled logger.info(f"RBAC status org={organization.pk} rbac_enabled={organization.is_rbac_permissions_enabled}") _sync_instance_info(organization) diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index 3cfd875a74..afa889e766 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -22,7 +22,7 @@ @contextmanager -def patched_grafana_api_client(organization, is_rbac_enabled_for_organization=False): +def patched_grafana_api_client(organization, is_rbac_enabled_for_organization=(False, False)): GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY = "backendUrl" with patch("apps.user_management.sync.GrafanaAPIClient") as mock_grafana_api_client: @@ -262,29 +262,40 @@ def test_sync_organization(mocked_org_sync_signal, make_organization): mocked_org_sync_signal.send.assert_called_once_with(sender=None, organization=organization) -@pytest.mark.parametrize("is_rbac_enabled_for_organization", [False, True]) +@pytest.mark.parametrize( + "is_rbac_enabled_for_organization,expected", + [ + ((False, False), False), + ((True, False), True), + ((True, True), False), + ], +) @override_settings(LICENSE=settings.OPEN_SOURCE_LICENSE_NAME) @pytest.mark.django_db -def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organization, is_rbac_enabled_for_organization): - organization = make_organization() +def test_sync_organization_is_rbac_permissions_enabled_open_source( + make_organization, is_rbac_enabled_for_organization, expected +): + organization = make_organization(is_rbac_permissions_enabled=False) with patched_grafana_api_client(organization, is_rbac_enabled_for_organization): sync_organization(organization) organization.refresh_from_db() - assert organization.is_rbac_permissions_enabled == is_rbac_enabled_for_organization + assert organization.is_rbac_permissions_enabled == expected @pytest.mark.parametrize( "gcom_api_response,grafana_api_response,org_initial_value,org_is_rbac_permissions_enabled_expected_value", [ # stack is in an inactive state, rely on org's previous state of is_rbac_permissions_enabled - (False, False, False, False), - (False, False, True, True), + (False, (False, False), False, False), + (False, (False, False), True, True), # stack is active, Grafana API tells us RBAC is not enabled - (True, False, True, False), + (True, (False, False), True, False), # stack is active, Grafana API tells us RBAC is enabled - (True, True, False, True), + (True, (True, False), False, True), + # stack is active, Grafana API returns error + (True, (False, True), True, True), ], ) @patch("apps.user_management.sync.GcomAPIClient")