From 0ef6bb7edaf7a34cf8425add266ac9102eb55e1e Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Sep 2024 16:51:37 -0400 Subject: [PATCH 1/2] main -> dev (#5084) Co-authored-by: GitHub Actions Co-authored-by: Matias Bordese Co-authored-by: Vadim Stepanov Co-authored-by: Dominik Broj Co-authored-by: Michael Derynck Co-authored-by: Yulya Artyukhina Co-authored-by: Innokentii Konstantinov Co-authored-by: Ildar Iskhakov Co-authored-by: grafana-irm-app[bot] <165293418+grafana-irm-app[bot]@users.noreply.github.com> --- .../workflows/on-helm-release-pr-merged.yml | 35 ++++++------------- helm/oncall/Chart.yaml | 4 +-- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/.github/workflows/on-helm-release-pr-merged.yml b/.github/workflows/on-helm-release-pr-merged.yml index 9f107bfefd..4d1dde5939 100644 --- a/.github/workflows/on-helm-release-pr-merged.yml +++ b/.github/workflows/on-helm-release-pr-merged.yml @@ -8,35 +8,20 @@ on: - helm/oncall/Chart.yaml jobs: - get-irm-app-token: - runs-on: ubuntu-latest - outputs: - token: ${{ steps.generate-token.outputs.token }} - steps: - - name: Get Vault secrets - id: get-secrets - uses: grafana/shared-workflows/actions/get-vault-secrets@main - with: - repo_secrets: | - GH_APP_ID=github-app:app-id - GH_APP_PRIVATE_KEY=github-app:private-key - - - name: Generate Github App token - id: generate-token - uses: actions/create-github-app-token@v1 - with: - app-id: ${{ env.GH_APP_ID }} - private-key: ${{ env.GH_APP_PRIVATE_KEY }} - owner: grafana - repositories: "helm-charts" - + # NOTE: unfortunately we need to store GH_APP_ID and GH_APP_PRIVATE_KEY as repository secrets + # (even though we already store them in Vault), because GitHub does not allow passing the `token` output + # of the `actions/create-github-app-token` action ACROSS jobs. + # + # Because grafana/helm-charts/.github/workflows/update-helm-repo.yaml is a reusable workflow, and not a composite + # action, there is no way to run job steps before the reusable workflow to do so within the same job. + # + # see https://github.com/actions/create-github-app-token/issues/66 for more details call-update-helm-repo: uses: grafana/helm-charts/.github/workflows/update-helm-repo.yaml@main - needs: - - get-irm-app-token with: charts_dir: helm cr_configfile: helm/cr.yaml ct_configfile: helm/ct.yaml secrets: - helm_repo_token: ${{ needs.get-irm-app-token.outputs.token }} + github_app_id: ${{ secrets.GH_APP_ID }} + github_app_pem: ${{ secrets.GH_APP_PRIVATE_KEY }} diff --git a/helm/oncall/Chart.yaml b/helm/oncall/Chart.yaml index a39062a80c..1362b78265 100644 --- a/helm/oncall/Chart.yaml +++ b/helm/oncall/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: oncall description: Developer-friendly incident response with brilliant Slack integration type: application -version: 1.9.27 -appVersion: v1.9.27 +version: 1.9.29 +appVersion: v1.9.29 dependencies: - name: cert-manager version: v1.8.0 From b260a8e82b7c98bfe2cd969355c78875555159fc Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 26 Sep 2024 12:40:07 -0400 Subject: [PATCH 2/2] fix: address RBAC Admin issue (#5087) # What this PR does **NOTE**: should be merged/released after https://github.com/grafana/irm/pull/183 has been rolled out to most stacks (as that frontend update is what will grant that new RBAC "action" to users whom already have the "OnCall Admin" RBAC role assigned) tldr; from the comment in the `RBACPermission.Permission.ADMIN` comment in `engine/apps/api/permissions.py`: > NOTE: this is a bit of a hack for now. See https://github.com/grafana/support-escalations/issues/12625 > Basically when it comes to filtering teams that are configured to share their resources with > "Team members and admins", we have no way of knowing, when a user is ACTUALLY an Admin when RBAC is involed. > > Example: Take a user with the basic role of None/Editor/Viewer but with the "OnCall Admin" role assigned. > Without this RBAC permission, we have no way of knowing that the user is ACTUALLY an "Admin". ## Which issue(s) this PR closes Closes https://github.com/grafana/support-escalations/issues/12625 ## 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/api/permissions.py | 10 +++ .../serializers/user_notification_policy.py | 15 ++-- engine/apps/user_management/models/user.py | 24 ++--- .../apps/user_management/tests/test_user.py | 87 +++++++++++++++++-- engine/common/api_helpers/mixins.py | 3 +- grafana-plugin/src/plugin.json | 7 +- 6 files changed, 113 insertions(+), 33 deletions(-) diff --git a/engine/apps/api/permissions.py b/engine/apps/api/permissions.py index 57b92a202c..771139935c 100644 --- a/engine/apps/api/permissions.py +++ b/engine/apps/api/permissions.py @@ -66,6 +66,8 @@ class Resources(enum.Enum): USER_SETTINGS = "user-settings" OTHER_SETTINGS = "other-settings" + ADMIN = "admin" + class Actions(enum.Enum): READ = "read" @@ -144,6 +146,14 @@ def user_is_authorized( class RBACPermission(permissions.BasePermission): class Permissions: + # NOTE: this is a bit of a hack for now. See https://github.com/grafana/support-escalations/issues/12625 + # Basically when it comes to filtering teams that are configured to share their resources with + # "Team members and admins", we have no way of knowing, when a user is ACTUALLY an Admin when RBAC is involed. + # + # Example: Take a user with the basic role of None/Editor/Viewer but with the "OnCall Admin" role assigned. + # Without this RBAC permission, we have no way of knowing that the user is ACTUALLY an "Admin". + ADMIN = LegacyAccessControlCompatiblePermission(Resources.ADMIN, Actions.ADMIN, LegacyAccessControlRole.ADMIN) + ALERT_GROUPS_READ = LegacyAccessControlCompatiblePermission( Resources.ALERT_GROUPS, Actions.READ, LegacyAccessControlRole.VIEWER ) diff --git a/engine/apps/api/serializers/user_notification_policy.py b/engine/apps/api/serializers/user_notification_policy.py index 67694042b3..e2f6222abb 100644 --- a/engine/apps/api/serializers/user_notification_policy.py +++ b/engine/apps/api/serializers/user_notification_policy.py @@ -83,14 +83,12 @@ class UserNotificationPolicySerializer(UserNotificationPolicyBaseSerializer): def create(self, validated_data): user = validated_data.get("user") or self.context["request"].user - organization = self.context["request"].auth.organization - self_or_admin = user.self_or_admin(user_to_check=self.context["request"].user, organization=organization) - if not self_or_admin: + if not user.self_or_has_user_settings_admin_permission( + user_to_check=self.context["request"].user, organization=self.context["request"].auth.organization + ): raise Forbidden() - - instance = UserNotificationPolicy.objects.create(**validated_data) - return instance + return UserNotificationPolicy.objects.create(**validated_data) class UserNotificationPolicyUpdateSerializer(UserNotificationPolicyBaseSerializer): @@ -104,10 +102,9 @@ class Meta(UserNotificationPolicyBaseSerializer.Meta): read_only_fields = UserNotificationPolicyBaseSerializer.Meta.read_only_fields + ["user", "important"] def update(self, instance, validated_data): - self_or_admin = instance.user.self_or_admin( + if not instance.user.self_or_has_user_settings_admin_permission( user_to_check=self.context["request"].user, organization=self.context["request"].user.organization - ) - if not self_or_admin: + ): raise Forbidden() if validated_data.get("step") == UserNotificationPolicy.Step.WAIT and not validated_data.get("wait_delay"): validated_data["wait_delay"] = UserNotificationPolicy.FIVE_MINUTES diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index a5bff7aa07..278800ec2d 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -172,11 +172,19 @@ def __str__(self): return f"{self.pk}: {self.username}" @property - def available_teams(self): - if self.role == LegacyAccessControlRole.ADMIN: + def is_admin(self) -> bool: + return user_is_authorized(self, [RBACPermission.Permissions.ADMIN]) + + @property + def available_teams(self) -> "RelatedManager['Team']": + if self.is_admin: return self.organization.teams.all() return self.organization.teams.filter(Q(is_sharing_resources_to_all=True) | Q(users=self)).distinct() + @property + def is_notification_allowed(self) -> bool: + return user_is_authorized(self, [RBACPermission.Permissions.NOTIFICATIONS_READ]) + @property def is_authenticated(self): return True @@ -223,15 +231,9 @@ def clear_phone_numbers(self) -> None: def is_telegram_connected(self): return hasattr(self, "telegram_connection") - def self_or_admin(self, user_to_check, organization) -> bool: - has_admin_permission = user_is_authorized(user_to_check, [RBACPermission.Permissions.USER_SETTINGS_ADMIN]) - return user_to_check.pk == self.pk or ( - has_admin_permission and organization.pk == user_to_check.organization_id - ) - - @property - def is_notification_allowed(self): - return user_is_authorized(self, [RBACPermission.Permissions.NOTIFICATIONS_READ]) + def self_or_has_user_settings_admin_permission(self, user_to_check: "User", organization: "Organization") -> bool: + has_permission = user_is_authorized(user_to_check, [RBACPermission.Permissions.USER_SETTINGS_ADMIN]) + return user_to_check.pk == self.pk or (has_permission and organization.pk == user_to_check.organization_id) def get_username_with_slack_verbal(self, mention=False) -> str: slack_verbal = None diff --git a/engine/apps/user_management/tests/test_user.py b/engine/apps/user_management/tests/test_user.py index 7d29770b1f..314f7f7f99 100644 --- a/engine/apps/user_management/tests/test_user.py +++ b/engine/apps/user_management/tests/test_user.py @@ -3,26 +3,97 @@ import pytest from django.utils import timezone -from apps.api.permissions import LegacyAccessControlRole +from apps.api.permissions import GrafanaAPIPermission, LegacyAccessControlRole, RBACPermission from apps.google import constants as google_constants from apps.google.models import GoogleOAuth2User from apps.user_management.models import User @pytest.mark.django_db -def test_self_or_admin(make_organization, make_user_for_organization): - organization = make_organization() +def test_self_or_has_user_settings_admin_permission(make_organization, make_user_for_organization): + # RBAC not enabled for org + organization = make_organization(is_rbac_permissions_enabled=False) admin = make_user_for_organization(organization) second_admin = make_user_for_organization(organization) editor = make_user_for_organization(organization, role=LegacyAccessControlRole.EDITOR) - another_organization = make_organization() + another_organization = make_organization(is_rbac_permissions_enabled=False) admin_from_another_organization = make_user_for_organization(another_organization) - assert admin.self_or_admin(admin, organization) is True - assert admin.self_or_admin(editor, organization) is False - assert admin.self_or_admin(second_admin, organization) is True - assert admin.self_or_admin(admin_from_another_organization, organization) is False + assert organization.is_rbac_permissions_enabled is False + assert another_organization.is_rbac_permissions_enabled is False + + assert admin.self_or_has_user_settings_admin_permission(admin, organization) is True + assert admin.self_or_has_user_settings_admin_permission(editor, organization) is False + assert admin.self_or_has_user_settings_admin_permission(second_admin, organization) is True + + assert admin.self_or_has_user_settings_admin_permission(admin_from_another_organization, organization) is False + + assert editor.self_or_has_user_settings_admin_permission(editor, organization) is True + assert editor.self_or_has_user_settings_admin_permission(admin, organization) is True + + # RBAC enabled org + organization_with_rbac = make_organization(is_rbac_permissions_enabled=True) + user_with_perms = make_user_for_organization( + organization_with_rbac, + role=LegacyAccessControlRole.NONE, + permissions=[GrafanaAPIPermission(action=RBACPermission.Permissions.USER_SETTINGS_ADMIN.value)], + ) + user_without_perms = make_user_for_organization( + organization_with_rbac, + role=LegacyAccessControlRole.NONE, + permissions=[], + ) + + assert organization_with_rbac.is_rbac_permissions_enabled is True + + # true because self + assert user_with_perms.self_or_has_user_settings_admin_permission(user_with_perms, organization_with_rbac) is True + assert ( + user_without_perms.self_or_has_user_settings_admin_permission(user_without_perms, organization_with_rbac) + is True + ) + + # true because user_with_perms has proper admin RBAC permission + assert ( + user_without_perms.self_or_has_user_settings_admin_permission(user_with_perms, organization_with_rbac) is True + ) + + # false because user_without_perms does not have proper admin RBAC permission + assert ( + user_with_perms.self_or_has_user_settings_admin_permission(user_without_perms, organization_with_rbac) is False + ) + + +@pytest.mark.django_db +def test_is_admin(make_organization, make_user_for_organization): + # RBAC not enabled for org + organization = make_organization(is_rbac_permissions_enabled=False) + admin = make_user_for_organization(organization, role=LegacyAccessControlRole.ADMIN) + editor = make_user_for_organization(organization, role=LegacyAccessControlRole.EDITOR) + + assert organization.is_rbac_permissions_enabled is False + + assert admin.is_admin is True + assert editor.is_admin is False + + # RBAC enabled org + organization_with_rbac = make_organization(is_rbac_permissions_enabled=True) + user_with_perms = make_user_for_organization( + organization_with_rbac, + role=LegacyAccessControlRole.NONE, + permissions=[GrafanaAPIPermission(action=RBACPermission.Permissions.ADMIN.value)], + ) + user_without_perms = make_user_for_organization( + organization_with_rbac, + role=LegacyAccessControlRole.NONE, + permissions=[], + ) + + assert organization_with_rbac.is_rbac_permissions_enabled is True + + assert user_with_perms.is_admin is True + assert user_without_perms.is_admin is False @pytest.mark.django_db diff --git a/engine/common/api_helpers/mixins.py b/engine/common/api_helpers/mixins.py index 4aacd63b33..d6e31c4261 100644 --- a/engine/common/api_helpers/mixins.py +++ b/engine/common/api_helpers/mixins.py @@ -21,7 +21,6 @@ AlertWebTemplater, TemplateLoader, ) -from apps.api.permissions import LegacyAccessControlRole from apps.base.messaging import get_messaging_backends from common.api_helpers.exceptions import BadRequest from common.jinja_templater import apply_jinja_template @@ -183,7 +182,7 @@ def available_teams_lookup_args(self): NOTE: use .distinct() after filtering by available teams as it may return duplicate instances. """ available_teams_lookup_args = [] - if not self.request.user.role == LegacyAccessControlRole.ADMIN: + if not self.request.user.is_admin: available_teams_lookup_args = [ Q(**{f"{self.TEAM_LOOKUP}__users": self.request.user}) | Q(**{f"{self.TEAM_LOOKUP}__is_sharing_resources_to_all": True}) diff --git a/grafana-plugin/src/plugin.json b/grafana-plugin/src/plugin.json index b87a72ac7e..5108a0613b 100644 --- a/grafana-plugin/src/plugin.json +++ b/grafana-plugin/src/plugin.json @@ -174,9 +174,10 @@ { "action": "grafana-oncall-app.user-settings:admin" }, { "action": "grafana-oncall-app.other-settings:read" }, - { "action": "grafana-oncall-app.other-settings:write" } - ], - "hidden": true + { "action": "grafana-oncall-app.other-settings:write" }, + + { "action": "grafana-oncall-app.admin:admin" } + ] }, "grants": ["Grafana Admin", "Admin"] },