From 2582a1b1dcd5fe596b74fe7081e3bfa3c88bb4d0 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 14 May 2024 12:30:16 -0400 Subject: [PATCH] Refactor how RBAC enabled/disabled status is determined for Grafana Cloud stacks (#4279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # What this PR does In cloud we are currently (somewhat) improperly determining whether or not a Grafana stack had the `accessControlOnCall` feature flag enabled. At first things worked fine. We would enable this feature toggle via the Grafana Admin UI, and then the OnCall backend would read this value from GCOM's `GET /instance/` endpoint (via `config.feature_toggles`), and everything worked as expected. There was a recent change made in `grafana/deployment_tools` to set this feature flag to True for all stacks. However, for some reason, the GCOM endpoint above doesn't return the `accessControlOnCall` feature toggle value in `config.feature_toggles` if it is set in this manner (it only returns the value if it is set via the Grafana Admin UI). So what we should instead be doing is such instead of asking GCOM for this feature toggle, infer whether RBAC is enabled on the stack by doing a `HEAD /api/access-control/users/permissions/search` (this endpoint _is only_ available on a Grafana stack if `accessControlOnCall` is enabled). **Few caveats to this ☝️** 1. we first have to make sure that the cloud stack is in an `active` state (ie. not paused). This is because, no matter if the `accessControlOnCall` is enabled or not, if the stack is in a `paused` state it will ALWAYS return `HTTP 200` which can be misleading and lead to bugs (this feels like a bug on the Grafana API, will follow up with core grafana team) 2. Once we roll out this change we will effectively **actually** be enabling RBAC for OnCall for all orgs. The Identity Access team would prefer a progressive rollout, which is why I decided to introduce the concept of [`settings.CLOUD_RBAC_ROLLOUT_PERCENTAGE`](https://github.com/grafana/oncall/pull/4279/files#diff-3383aef931e41e44d95829ad971641eeb98fe001be2f5da92217446d300ea1b3R918) (see also [`Organization. should_be_considered_for_rbac_permissioning`](https://github.com/grafana/oncall/pull/4279/files#diff-2ca9917f4f56349be39545ee8abd459be5076295d02ca3a7ec545152fcddccdfR348-R362)) ## Which issue(s) this PR closes Related to https://github.com/grafana/identity-access-team/issues/667 ## 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 | 74 +--- .../tests/test_gcom_api_client.py | 79 ---- .../tests/test_grafana_api_client.py | 96 ++++- .../user_management/models/organization.py | 15 + engine/apps/user_management/sync.py | 24 +- .../tests/test_organization.py | 38 ++ .../apps/user_management/tests/test_sync.py | 343 +++++++----------- engine/common/utils.py | 10 + engine/conftest.py | 6 +- engine/settings/base.py | 4 +- 10 files changed, 318 insertions(+), 371 deletions(-) diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index ea7488b4..b792994e 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -34,7 +34,7 @@ UserPermissionsDict = typing.Dict[str, typing.List[GrafanaAPIPermission]] class GCOMInstanceInfoConfigFeatureToggles(typing.TypedDict): - accessControlOnCall: str + accessControlOnCall: typing.NotRequired[str] class GCOMInstanceInfoConfig(typing.TypedDict): @@ -211,7 +211,7 @@ class GrafanaAPIClient(APIClient): def check_token(self) -> APIClientResponse: return self.api_head("api/org") - def get_users_permissions(self, rbac_is_enabled_for_org: bool) -> UserPermissionsDict: + def get_users_permissions(self) -> typing.Optional[UserPermissionsDict]: """ It is possible that this endpoint may not be available for certain Grafana orgs. Ex: for Grafana Cloud orgs whom have pinned their Grafana version to an earlier version @@ -229,13 +229,9 @@ class GrafanaAPIClient(APIClient): } } """ - if not rbac_is_enabled_for_org: - return {} response, _ = self.api_get(self.USER_PERMISSION_ENDPOINT) - if response is None: - return {} - elif isinstance(response, list): - return {} + if response is None or isinstance(response, list): + return None data: typing.Dict[str, typing.Dict[str, typing.List[str]]] = response @@ -259,7 +255,13 @@ class GrafanaAPIClient(APIClient): users: GrafanaUsersWithPermissions = users_response - user_permissions = self.get_users_permissions(rbac_is_enabled_for_org) + user_permissions = {} + if rbac_is_enabled_for_org: + user_permissions = self.get_users_permissions(rbac_is_enabled_for_org) + if user_permissions is None: + # If we cannot fetch permissions when RBAC is enabled (ex. HTTP 500), we should not return any users + # to avoid potentially wiping-out OnCall's copy of permissions for all users + return [] # merge the users permissions response into the org users response for user in users: @@ -349,51 +351,6 @@ class GcomAPIClient(APIClient): data, _ = self.api_get(url) return data - def _feature_is_enabled_via_enable_key( - self, instance_feature_toggles: GCOMInstanceInfoConfigFeatureToggles, feature_name: str, delimiter: str - ): - return feature_name in instance_feature_toggles.get("enable", "").split(delimiter) - - def _feature_toggle_is_enabled(self, instance_info: GCOMInstanceInfo, feature_name: str) -> bool: - """ - there are two ways that feature toggles can be enabled, this method takes into account both - https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#enable - """ - instance_info_config = instance_info.get("config", {}) - if not instance_info_config: - return False - - instance_feature_toggles = instance_info_config.get("feature_toggles", {}) - - if not instance_feature_toggles: - return False - - # features enabled via enable key can be either space or comma delimited - # https://raintank-corp.slack.com/archives/C036J5B39/p1690183217162019 - - feature_enabled_via_enable_key_space_delimited = self._feature_is_enabled_via_enable_key( - instance_feature_toggles, feature_name, " " - ) - feature_enabled_via_enable_key_comma_delimited = self._feature_is_enabled_via_enable_key( - instance_feature_toggles, feature_name, "," - ) - feature_enabled_via_direct_key = instance_feature_toggles.get(feature_name, "false") == "true" - - return ( - feature_enabled_via_direct_key - or feature_enabled_via_enable_key_space_delimited - or feature_enabled_via_enable_key_comma_delimited - ) - - def is_rbac_enabled_for_stack(self, stack_id: str) -> bool: - """ - NOTE: must use an "Admin" GCOM token when calling this method - """ - instance_info = self.get_instance_info(stack_id, True) - if not instance_info: - return False - return self._feature_toggle_is_enabled(instance_info, "accessControlOnCall") - def get_instances(self, query: str, page_size=None): if not page_size: page, _ = self.api_get(query) @@ -409,11 +366,20 @@ class GcomAPIClient(APIClient): yield page cursor = page["nextCursor"] + def _is_stack_in_certain_state(self, stack_id: str, state: str) -> bool: + instance_info = self.get_instance_info(stack_id) + if not instance_info: + return False + return instance_info.get("status") == state + def is_stack_deleted(self, stack_id: str) -> bool: url = f"instances?includeDeleted=true&id={stack_id}" instance_infos, _ = self.api_get(url) return instance_infos["items"] and instance_infos["items"][0].get("status") == self.STACK_STATUS_DELETED + def is_stack_active(self, stack_id: str) -> bool: + return self._is_stack_in_certain_state(stack_id, self.STACK_STATUS_ACTIVE) + def post_active_users(self, body) -> APIClientResponse: return self.api_post("app-active-users", body) diff --git a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py index 4b93437a..219396a2 100644 --- a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py @@ -8,85 +8,6 @@ from apps.grafana_plugin.helpers.gcom import get_instance_ids from settings.base import CLOUD_LICENSE_NAME -class TestIsRbacEnabledForStack: - TEST_FEATURE_TOGGLE = "helloWorld" - - @pytest.mark.parametrize( - "gcom_api_response,expected", - [ - (None, False), - ({}, False), - ({"config": {}}, False), - ({"config": {"feature_toggles": {}}}, False), - ({"config": {"feature_toggles": {"accessControlOnCall": "false"}}}, False), - ({"config": {"feature_toggles": {"accessControlOnCall": "true"}}}, True), - ], - ) - @patch("apps.grafana_plugin.helpers.client.GcomAPIClient.api_get") - def test_it_returns_based_on_feature_toggle_value( - self, mocked_gcom_api_client_api_get, gcom_api_response, expected - ): - stack_id = 5 - mocked_gcom_api_client_api_get.return_value = (gcom_api_response, {"status_code": 200}) - - api_client = GcomAPIClient("someFakeApiToken") - assert api_client.is_rbac_enabled_for_stack(stack_id) == expected - assert mocked_gcom_api_client_api_get.called_once_with(f"instances/{stack_id}?config=true") - - @pytest.mark.parametrize( - "instance_info_feature_toggles,delimiter,expected", - [ - ({}, " ", False), - ({"enable": "foo,bar,baz"}, " ", False), - ({"enable": "foo,bar,baz"}, ",", False), - ({"enable": f"foo,bar,baz{TEST_FEATURE_TOGGLE}"}, " ", False), - ({"enable": f"foo,bar,baz{TEST_FEATURE_TOGGLE}"}, ",", False), - ({"enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}abc"}, ",", False), - ({"enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}"}, ",", True), - ], - ) - def test_feature_is_enabled_via_enable_key(self, instance_info_feature_toggles, delimiter, expected) -> None: - assert ( - GcomAPIClient("someFakeApiToken")._feature_is_enabled_via_enable_key( - instance_info_feature_toggles, self.TEST_FEATURE_TOGGLE, delimiter - ) - == expected - ) - - @pytest.mark.parametrize( - "instance_info,expected", - [ - ({}, False), - ({"config": {}}, False), - ({"config": {"feature_toggles": {}}}, False), - ({"config": {"feature_toggles": {"enable": "foo,bar,baz"}}}, False), - ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "false"}}}, False), - ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE}baz"}}}, False), - ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE},baz"}}}, True), - ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, True), - ({"config": {"feature_toggles": {"enable": "foo bar baz", TEST_FEATURE_TOGGLE: "true"}}}, True), - ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "true"}}}, True), - # this case will probably never happen, but lets account for it anyways - ( - { - "config": { - "feature_toggles": { - "enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}", - TEST_FEATURE_TOGGLE: "false", - } - } - }, - True, - ), - ], - ) - def test_feature_toggle_is_enabled(self, instance_info, expected) -> None: - assert ( - GcomAPIClient("someFakeApiToken")._feature_toggle_is_enabled(instance_info, self.TEST_FEATURE_TOGGLE) - == expected - ) - - def build_paged_responses(page_size, pages, total_items): response = [] remaining = total_items 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 4dcb70c1..65e83504 100644 --- a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py @@ -9,19 +9,12 @@ API_TOKEN = "dfjkfdjkfd" class TestGetUsersPermissions: - def test_rbac_is_not_enabled_for_org(self): - api_client = GrafanaAPIClient(API_URL, API_TOKEN) - permissions = api_client.get_users_permissions(False) - assert len(permissions.keys()) == 0 - + @pytest.mark.parametrize("api_response_data", [None, []]) @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_get") - def test_api_call_returns_none(self, mocked_grafana_api_client_api_get): - mocked_grafana_api_client_api_get.return_value = (None, "dfkjfdkj") - + def test_api_call_returns_none_or_list(self, mocked_grafana_api_client_api_get, api_response_data): + mocked_grafana_api_client_api_get.return_value = (api_response_data, "dfkjfdkj") api_client = GrafanaAPIClient(API_URL, API_TOKEN) - - permissions = api_client.get_users_permissions(True) - assert len(permissions.keys()) == 0 + assert api_client.get_users_permissions() is None @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_get") def test_it_properly_transforms_the_data(self, mocked_grafana_api_client_api_get): @@ -32,7 +25,7 @@ class TestGetUsersPermissions: api_client = GrafanaAPIClient(API_URL, API_TOKEN) - permissions = api_client.get_users_permissions(True) + permissions = api_client.get_users_permissions() assert permissions == { "1": [ {"action": "grafana-oncall-app.alert-groups:read"}, @@ -41,6 +34,85 @@ class TestGetUsersPermissions: } +class TestGetUsers: + @pytest.mark.parametrize( + "rbac_is_enabled,api_get_return_value,get_users_permissions_return_value,expected", + [ + # RBAC is enabled - permissions are returned + ( + True, + [ + {"userId": 1, "foo": "bar"}, + {"userId": 2, "foo": "baz"}, + ], + { + "1": [ + {"action": "grafana-oncall-app.alert-groups:read"}, + {"action": "grafana-oncall-app.alert-groups:write"}, + ], + }, + [ + { + "userId": 1, + "foo": "bar", + "permissions": [ + {"action": "grafana-oncall-app.alert-groups:read"}, + {"action": "grafana-oncall-app.alert-groups:write"}, + ], + }, + { + "userId": 2, + "foo": "baz", + "permissions": [], + }, + ], + ), + # RBAC is enabled - permissions endpoint returns no permissions (ex. HTTP 500) + ( + True, + [ + {"userId": 1, "foo": "bar"}, + {"userId": 2, "foo": "baz"}, + ], + None, + [], + ), + # RBAC is not enabled - we don't fetch permissions (hence don't care about its response) + ( + False, + [ + {"userId": 1, "foo": "bar"}, + {"userId": 2, "foo": "baz"}, + ], + None, + [ + {"userId": 1, "foo": "bar", "permissions": []}, + { + "userId": 2, + "foo": "baz", + "permissions": [], + }, + ], + ), + ], + ) + @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_get") + @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.get_users_permissions") + def test_it_returns_none_if_permissions_call_returns_none( + self, + mocked_grafana_api_client_get_users_permissions, + mocked_grafana_api_client_api_get, + rbac_is_enabled, + api_get_return_value, + get_users_permissions_return_value, + expected, + ): + mocked_grafana_api_client_api_get.return_value = (api_get_return_value, "dfjkfdjkfd") + mocked_grafana_api_client_get_users_permissions.return_value = get_users_permissions_return_value + api_client = GrafanaAPIClient(API_URL, API_TOKEN) + assert api_client.get_users(rbac_is_enabled) == expected + + class TestIsRbacEnabledForOrganization: @pytest.mark.parametrize( "api_response_connected,expected", diff --git a/engine/apps/user_management/models/organization.py b/engine/apps/user_management/models/organization.py index 8564a195..5db1cd0c 100644 --- a/engine/apps/user_management/models/organization.py +++ b/engine/apps/user_management/models/organization.py @@ -1,4 +1,5 @@ import logging +import math import typing import uuid from urllib.parse import urljoin @@ -344,6 +345,20 @@ class Organization(MaintainableObject): .distinct() ) + def should_be_considered_for_rbac_permissioning(self) -> bool: + """ + this is sort of a hacky workaround to address a cloud issue we introduced with the accessControlOncall + feature flag. The flag is technically enabled for all stacks, but the way in which OnCall used to be + reading it (via GCOM config.feature_flags for the stack) made it such that RBAC wasn't actually being + enabled for most stacks from the oncall backend perspective. Once we change things to start HEADing + the permissions search endpoint, this will effectively turn on RBAC for all orgs.. soo instead lets + slowly turn it on via the logic here + """ + # if rbac permissions are already enabled for the org, they're "grandfathered" in + if self.is_rbac_permissions_enabled: + return True + return self.id <= math.floor(Organization.objects.last().id * settings.CLOUD_RBAC_ROLLOUT_PERCENTAGE) + @property def web_link(self): return urljoin(self.grafana_url, "a/grafana-oncall-app/") diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index a106a193..e6c6008d 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -28,16 +28,28 @@ 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 Cloud we should make a call to the GCOM API, using an admin API token, and get the list of - # feature_toggles enabled for the 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. 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. 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) - rbac_is_enabled = gcom_client.is_rbac_enabled_for_stack(organization.stack_id) + + if not organization.should_be_considered_for_rbac_permissioning(): + rbac_is_enabled = False + elif 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() diff --git a/engine/apps/user_management/tests/test_organization.py b/engine/apps/user_management/tests/test_organization.py index 58636f88..90fb73a6 100644 --- a/engine/apps/user_management/tests/test_organization.py +++ b/engine/apps/user_management/tests/test_organization.py @@ -278,3 +278,41 @@ def test_get_notifiable_direct_paging_integrations( make_channel_filter(arc, is_default=False) notifiable_direct_paging_integrations = _assert(org, arc) assert notifiable_direct_paging_integrations.count() == 1 + + +@pytest.mark.parametrize( + "is_rbac_permissions_enabled_initially,rollout_percentage,expected", + [ + # env var is not set, no orgs have is_rbac_permissions_enabled set + (False, 0.0, False), + # env var is not set but is_rbac_permissions_enabled is already set for all orgs + (True, 0.0, True), + # env var is set, only some orgs should be considered + (False, 0.5, "partial"), + (False, 1.0, True), + ], +) +@pytest.mark.django_db +def test_should_be_considered_for_rbac_permissioning( + make_organization, + settings, + is_rbac_permissions_enabled_initially, + rollout_percentage, + expected, +): + NUM_ORGS = 5 + settings.CLOUD_RBAC_ROLLOUT_PERCENTAGE = rollout_percentage + + orgs = [ + make_organization(is_rbac_permissions_enabled=is_rbac_permissions_enabled_initially) for _ in range(NUM_ORGS) + ] + + assert all(org.is_rbac_permissions_enabled is is_rbac_permissions_enabled_initially for org in orgs) + + if expected == "partial": + assert all( + org.should_be_considered_for_rbac_permissioning() == (org.id <= NUM_ORGS * rollout_percentage) + for org in orgs + ) + else: + assert all(org.should_be_considered_for_rbac_permissioning() is expected for org in orgs) diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index f24edaac..131893ab 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -1,3 +1,4 @@ +from contextlib import contextmanager from dataclasses import dataclass from typing import Optional from unittest.mock import patch @@ -20,6 +21,64 @@ from apps.user_management.sync import ( MOCK_GRAFANA_INCIDENT_BACKEND_URL = "https://grafana-incident.test" +@contextmanager +def patched_grafana_api_client(organization, is_rbac_enabled_for_organization=False): + GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY = "backendUrl" + + with patch("apps.user_management.sync.GrafanaAPIClient") as mock_grafana_api_client: + mock_grafana_api_client.GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY = GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY + + mock_client_instance = mock_grafana_api_client.return_value + + mock_client_instance.get_users.return_value = [ + { + "userId": 1, + "email": "test@test.test", + "name": "Test", + "login": "test", + "role": "admin", + "avatarUrl": "test.test/test", + "permissions": [], + }, + ] + mock_client_instance.get_teams.return_value = ( + { + "totalCount": 1, + "teams": ( + { + "id": 1, + "name": "Test", + "email": "test@test.test", + "avatarUrl": "test.test/test", + }, + ), + }, + None, + ) + mock_client_instance.get_team_members.return_value = ( + [ + { + "orgId": organization.org_id, + "teamId": 1, + "userId": 1, + }, + ], + None, + ) + mock_client_instance.get_grafana_incident_plugin_settings.return_value = ( + {"enabled": True, "jsonData": {GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY: MOCK_GRAFANA_INCIDENT_BACKEND_URL}}, + None, + ) + mock_client_instance.get_grafana_labels_plugin_settings.return_value = ( + {"enabled": True, "jsonData": {}}, + None, + ) + mock_client_instance.check_token.return_value = (None, {"connected": True}) + mock_client_instance.is_rbac_enabled_for_organization.return_value = is_rbac_enabled_for_organization + + yield mock_grafana_api_client + + @pytest.mark.django_db def test_sync_users_for_organization(make_organization, make_user_for_organization): organization = make_organization(grafana_url="https://test.test") @@ -184,79 +243,11 @@ def test_sync_users_for_team(make_organization, make_user_for_organization, make @pytest.mark.django_db -@patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=False) -@patch.object( - GrafanaAPIClient, - "get_users", - return_value=[ - { - "userId": 1, - "email": "test@test.test", - "name": "Test", - "login": "test", - "role": "admin", - "avatarUrl": "test.test/test", - "permissions": [], - } - ], -) -@patch.object( - GrafanaAPIClient, - "get_teams", - return_value=( - { - "totalCount": 1, - "teams": ( - { - "id": 1, - "name": "Test", - "email": "test@test.test", - "avatarUrl": "test.test/test", - }, - ), - }, - None, - ), -) -@patch.object(GrafanaAPIClient, "check_token", return_value=(None, {"connected": True})) -@patch.object(GrafanaAPIClient, "get_grafana_incident_plugin_settings") -@patch.object(GrafanaAPIClient, "get_grafana_labels_plugin_settings") @patch("apps.user_management.sync.org_sync_signal") -def test_sync_organization( - mocked_org_sync_signal, - mock_get_grafana_labels_plugin_settings, - mock_get_grafana_incident_plugin_settings, - _mock_check_token, - _mock_get_teams, - _mock_get_users, - _mock_is_rbac_enabled_for_organization, - make_organization, -): - # Set optimistic responses from grafana api. - # All cases are tested properly in test_sync_grafana_incident_plugin/test_sync_grafana_labels_plugin - mock_get_grafana_incident_plugin_settings.return_value = ( - { - "enabled": True, - "jsonData": {"backendUrl": MOCK_GRAFANA_INCIDENT_BACKEND_URL}, - }, - None, - ) - mock_get_grafana_labels_plugin_settings.return_value = ({"enabled": True, "jsonData": {}}, None) - +def test_sync_organization(mocked_org_sync_signal, make_organization): organization = make_organization() - api_members_response = ( - [ - { - "orgId": organization.org_id, - "teamId": 1, - "userId": 1, - } - ], - None, - ) - - with patch.object(GrafanaAPIClient, "get_team_members", return_value=api_members_response): + with patched_grafana_api_client(organization): sync_organization(organization) # check that users are populated @@ -283,154 +274,69 @@ def test_sync_organization( mocked_org_sync_signal.send.assert_called_once_with(sender=None, organization=organization) -@pytest.mark.parametrize("grafana_api_response", [False, True]) +@pytest.mark.parametrize("is_rbac_enabled_for_organization", [False, True]) @override_settings(LICENSE=settings.OPEN_SOURCE_LICENSE_NAME) @pytest.mark.django_db -def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organization, grafana_api_response): +def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organization, is_rbac_enabled_for_organization): organization = make_organization() - api_users_response = ( - { - "userId": 1, - "email": "test@test.test", - "name": "Test", - "login": "test", - "role": "admin", - "avatarUrl": "test.test/test", - "permissions": [], - }, - ) - - api_teams_response = { - "totalCount": 1, - "teams": ( - { - "id": 1, - "name": "Test", - "email": "test@test.test", - "avatarUrl": "test.test/test", - }, - ), - } - - api_members_response = ( - { - "orgId": organization.org_id, - "teamId": 1, - "userId": 1, - }, - ) - 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): - with patch.object(GrafanaAPIClient, "get_teams", return_value=(api_teams_response, None)): - with patch.object(GrafanaAPIClient, "get_team_members", return_value=(api_members_response, None)): - with patch.object( - GrafanaAPIClient, "check_token", return_value=(None, api_check_token_call_status) - ): - with patch.object( - GrafanaAPIClient, - "get_grafana_incident_plugin_settings", - return_value=( - {"enabled": True, "jsonData": {"backendUrl": MOCK_GRAFANA_INCIDENT_BACKEND_URL}}, - None, - ), - ): - with patch.object( - GrafanaAPIClient, - "get_grafana_labels_plugin_settings", - return_value=( - {"enabled": True, "jsonData": {}}, - None, - ), - ): - sync_organization(organization) + with patched_grafana_api_client(organization, is_rbac_enabled_for_organization): + sync_organization(organization) organization.refresh_from_db() - assert organization.is_rbac_permissions_enabled == grafana_api_response + assert organization.is_rbac_permissions_enabled == is_rbac_enabled_for_organization -@pytest.mark.parametrize("gcom_api_response", [False, True]) +@pytest.mark.parametrize( + "should_be_considered_for_rbac_permissioning,gcom_api_response,grafana_api_response,org_initial_value,org_is_rbac_permissions_enabled_expected_value", + [ + # org shouldn't be considered for RBAC permissioning + (False, True, True, True, False), + # org should be considered for RBAC permissioning + # + # stack is in an inactive state, rely on org's previous state of is_rbac_permissions_enabled + (True, False, False, False, False), + (True, False, False, True, True), + # stack is active, Grafana API tells us RBAC is not enabled + (True, True, False, True, False), + # stack is active, Grafana API tells us RBAC is enabled + (True, True, True, False, True), + ], +) +@patch("apps.user_management.models.Organization.should_be_considered_for_rbac_permissioning") @patch("apps.user_management.sync.GcomAPIClient") -@patch("common.utils.cache") @override_settings(LICENSE=settings.CLOUD_LICENSE_NAME) -@override_settings(GRAFANA_COM_ADMIN_API_TOKEN="mockedToken") @pytest.mark.django_db def test_sync_organization_is_rbac_permissions_enabled_cloud( - mock_cache, mocked_gcom_client, make_organization, gcom_api_response + mock_gcom_client, + mock_should_be_considered_for_rbac_permissioning, + make_organization, + should_be_considered_for_rbac_permissioning, + gcom_api_response, + grafana_api_response, + org_initial_value, + org_is_rbac_permissions_enabled_expected_value, ): + mock_should_be_considered_for_rbac_permissioning.return_value = should_be_considered_for_rbac_permissioning + stack_id = 5 - organization = make_organization(stack_id=stack_id) + organization = make_organization(stack_id=stack_id, is_rbac_permissions_enabled=org_initial_value) + mock_gcom_client.return_value.is_stack_active.return_value = gcom_api_response - api_check_token_call_status = {"connected": True} + assert organization.is_rbac_permissions_enabled == org_initial_value - mocked_gcom_client.return_value.is_rbac_enabled_for_stack.return_value = gcom_api_response - - api_users_response = ( - { - "userId": 1, - "email": "test@test.test", - "name": "Test", - "login": "test", - "role": "admin", - "avatarUrl": "test.test/test", - "permissions": [], - }, - ) - - api_teams_response = { - "totalCount": 1, - "teams": ( - { - "id": 1, - "name": "Test", - "email": "test@test.test", - "avatarUrl": "test.test/test", - }, - ), - } - - api_members_response = ( - { - "orgId": organization.org_id, - "teamId": 1, - "userId": 1, - }, - ) - - random_uuid = "random" - with patch("apps.user_management.sync.uuid.uuid4", return_value=random_uuid): - with patch.object(GrafanaAPIClient, "check_token", return_value=(None, api_check_token_call_status)): - with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response): - with patch.object(GrafanaAPIClient, "get_teams", return_value=(api_teams_response, None)): - with patch.object(GrafanaAPIClient, "get_team_members", return_value=(api_members_response, None)): - with patch.object( - GrafanaAPIClient, - "get_grafana_incident_plugin_settings", - return_value=( - {"enabled": True, "jsonData": {"backendUrl": MOCK_GRAFANA_INCIDENT_BACKEND_URL}}, - None, - ), - ): - with patch.object( - GrafanaAPIClient, - "get_grafana_labels_plugin_settings", - return_value=( - {"enabled": True, "jsonData": {}}, - None, - ), - ): - sync_organization(organization) + with patched_grafana_api_client(organization, grafana_api_response) as mock_grafana_api_client: + sync_organization(organization) organization.refresh_from_db() - # lock is set and released - mock_cache.add.assert_called_once_with(f"sync-organization-lock-{organization.id}", random_uuid, 60 * 10) - mock_cache.delete.assert_called_once_with(f"sync-organization-lock-{organization.id}") - assert mocked_gcom_client.return_value.called_once_with("mockedToken") - assert mocked_gcom_client.return_value.is_rbac_enabled_for_stack.called_once_with(stack_id) - assert organization.is_rbac_permissions_enabled == gcom_api_response + assert organization.is_rbac_permissions_enabled == org_is_rbac_permissions_enabled_expected_value + + if should_be_considered_for_rbac_permissioning: + mock_gcom_client.return_value.is_stack_active.assert_called_once_with(stack_id) + + if gcom_api_response: + mock_grafana_api_client.return_value.is_rbac_enabled_for_organization.assert_called_once_with() @pytest.mark.django_db @@ -468,13 +374,7 @@ def test_duplicate_user_ids(make_organization, make_user_for_organization): @pytest.mark.django_db -@pytest.mark.parametrize( - "is_deleted", - [ - True, - False, - ], -) +@pytest.mark.parametrize("is_deleted", [True, False]) def test_cleanup_organization_deleted(make_organization, is_deleted): organization = make_organization(gcom_token="TEST_GCOM_TOKEN") @@ -501,19 +401,30 @@ def test_organization_not_deleted(make_organization): @pytest.mark.django_db -def test_sync_organization_lock(make_organization): +@pytest.mark.parametrize("task_lock_acquired", [True, False]) +@patch("apps.user_management.sync.task_lock") +@patch("apps.user_management.sync.uuid.uuid4", return_value="random") +def test_sync_organization_lock( + _mock_uuid4, + mock_task_lock, + make_organization, + task_lock_acquired, +): organization = make_organization() - random_uuid = "random" - with patch("apps.user_management.sync.GrafanaAPIClient") as mock_client: - with patch("apps.user_management.sync.uuid.uuid4", return_value=random_uuid): - with patch("apps.user_management.sync.task_lock") as mock_task_lock: - # lock couldn't be acquired - mock_task_lock.return_value.__enter__.return_value = False - sync_organization(organization) + lock_cache_key = f"sync-organization-lock-{organization.id}" + mock_task_lock.return_value.__enter__.return_value = task_lock_acquired - mock_task_lock.assert_called_once_with(f"sync-organization-lock-{organization.id}", random_uuid) - assert not mock_client.called + with patched_grafana_api_client(organization) as mock_grafana_api_client: + sync_organization(organization) + + mock_task_lock.assert_called_once_with(lock_cache_key, "random") + + if task_lock_acquired: + mock_grafana_api_client.assert_called_once() + else: + # task lock could not be acquired + mock_grafana_api_client.assert_not_called() @dataclass diff --git a/engine/common/utils.py b/engine/common/utils.py index 05141120..b902fe21 100644 --- a/engine/common/utils.py +++ b/engine/common/utils.py @@ -139,6 +139,16 @@ def getenv_integer(variable_name: str, default: int) -> int: return default +def getenv_float(variable_name: str, default: float) -> float: + value = os.environ.get(variable_name) + if value is None: + return default + try: + return float(value) + except ValueError: + return default + + def getenv_list(variable_name: str, default: list) -> list: value = os.environ.get(variable_name) if value is None: diff --git a/engine/conftest.py b/engine/conftest.py index 9bd2e4eb..899679fd 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -230,9 +230,9 @@ def mock_is_labels_feature_enabled_for_org(settings): @pytest.fixture def make_organization(): def _make_organization(**kwargs): - return OrganizationFactory( - **kwargs, is_rbac_permissions_enabled=IS_RBAC_ENABLED, is_grafana_labels_enabled=True - ) + if "is_rbac_permissions_enabled" not in kwargs: + kwargs["is_rbac_permissions_enabled"] = IS_RBAC_ENABLED + return OrganizationFactory(**kwargs, is_grafana_labels_enabled=True) return _make_organization diff --git a/engine/settings/base.py b/engine/settings/base.py index ecdac1a1..48c9e16b 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -7,7 +7,7 @@ from random import randrange from celery.schedules import crontab from firebase_admin import credentials, initialize_app -from common.utils import getenv_boolean, getenv_integer, getenv_list +from common.utils import getenv_boolean, getenv_float, getenv_integer, getenv_list VERSION = "dev-oss" SEND_ANONYMOUS_USAGE_STATS = getenv_boolean("SEND_ANONYMOUS_USAGE_STATS", default=True) @@ -914,3 +914,5 @@ ZVONOK_VERIFICATION_TEMPLATE = os.getenv("ZVONOK_VERIFICATION_TEMPLATE", None) DETACHED_INTEGRATIONS_SERVER = getenv_boolean("DETACHED_INTEGRATIONS_SERVER", default=False) ACKNOWLEDGE_REMINDER_TASK_EXPIRY_DAYS = os.environ.get("ACKNOWLEDGE_REMINDER_TASK_EXPIRY_DAYS", default=14) + +CLOUD_RBAC_ROLLOUT_PERCENTAGE = getenv_float("CLOUD_RBAC_ROLLOUT_PERCENTAGE", default=0.0)