Don't update RBAC status on Grafana server error (#4753)

# What this PR does

Fixes a bug when RBAC permissions are getting erased when Grafana's API
returns a 5xx server error on organization sync.

## Which issue(s) this PR closes

Closes https://github.com/grafana/oncall-private/issues/2834

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] Added the relevant release notes label (see labels prefixed w/
`release:`). These labels dictate how your PR will
    show up in the autogenerated release notes.

engine/apps/grafana_plugin/helpers/client.py

@@ -241,9 +241,9 @@ class GrafanaAPIClient(APIClient):
 
         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)

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 @@ class TestGetUsers:
 
 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

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")

engine/apps/grafana_plugin/views/self_hosted_install.py

@@ -43,7 +43,7 @@ class SelfHostedInstallView(GrafanaHeadersMixin, APIView):
             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()

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
+    gcom_client = GcomAPIClient(settings.GRAFANA_COM_ADMIN_API_TOKEN)
 
-    # 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
+    # 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)

engine/apps/user_management/tests/test_sync.py

@@ -22,7 +22,7 @@ MOCK_GRAFANA_INCIDENT_BACKEND_URL = "https://grafana-incident.test"
 
 
 @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")