From 6d2ab58988bf36bf78668f6b51b91340abc9eab1 Mon Sep 17 00:00:00 2001 From: Novak Zaballa <41410593+novakzaballa@users.noreply.github.com> Date: Mon, 21 Oct 2024 04:58:19 -0400 Subject: [PATCH] fix: Tag Based permissions only validate some views (#4523) Co-authored-by: Gagan Trivedi --- api/Makefile | 2 +- api/features/permissions.py | 24 +++++++++++++++---- api/features/versioning/permissions.py | 24 ++++++++++++++++--- .../unit/features/test_unit_features_views.py | 6 ++--- api/users/abc.py | 6 +++-- 5 files changed, 48 insertions(+), 14 deletions(-) diff --git a/api/Makefile b/api/Makefile index 44863d3cf51b..b62f0cc2ab65 100644 --- a/api/Makefile +++ b/api/Makefile @@ -12,7 +12,7 @@ POETRY_VERSION ?= 1.8.3 GUNICORN_LOGGER_CLASS ?= util.logging.GunicornJsonCapableLogger SAML_REVISION ?= v1.6.4 -RBAC_REVISION ?= v0.8.0 +RBAC_REVISION ?= v0.9.0 -include .env-local -include $(DOTENV_OVERRIDE_FILE) diff --git a/api/features/permissions.py b/api/features/permissions.py index f725ab6ae24b..57cd146fe153 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -156,19 +156,35 @@ def has_permission(self, request, view): return True environment_api_key = view.kwargs.get("environment_api_key") - with suppress(Environment.DoesNotExist): + with suppress(Environment.DoesNotExist, Feature.DoesNotExist): environment = Environment.objects.get(api_key=environment_api_key) + + tag_ids = None + required_permission = action_permission_map.get(view.action) + + if required_permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS: + feature_id = request.data.get("feature") + feature = Feature.objects.get( + id=feature_id, project=environment.project + ) + tag_ids = list(feature.tags.values_list("id", flat=True)) + return request.user.has_environment_permission( - action_permission_map.get(view.action), environment + required_permission, environment, tag_ids=tag_ids ) return False def has_object_permission(self, request, view, obj): action_permission_map = {"retrieve": VIEW_ENVIRONMENT} + permission = action_permission_map.get(view.action, UPDATE_FEATURE_STATE) + + tag_ids = None + if permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS: + tag_ids = list(obj.feature.tags.values_list("id", flat=True)) + return request.user.has_environment_permission( - permission=action_permission_map.get(view.action, UPDATE_FEATURE_STATE), - environment=obj.environment, + permission=permission, environment=obj.environment, tag_ids=tag_ids ) diff --git a/api/features/versioning/permissions.py b/api/features/versioning/permissions.py index 538029704259..0023ff33b6f6 100644 --- a/api/features/versioning/permissions.py +++ b/api/features/versioning/permissions.py @@ -3,11 +3,14 @@ from rest_framework.viewsets import GenericViewSet from environments.models import Environment +from environments.permissions.constants import ( + TAG_SUPPORTED_PERMISSIONS as TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS, +) from environments.permissions.constants import ( UPDATE_FEATURE_STATE, VIEW_ENVIRONMENT, ) -from features.models import FeatureState +from features.models import Feature, FeatureState from features.versioning.models import EnvironmentFeatureVersion @@ -19,8 +22,17 @@ def has_permission(self, request: Request, view: GenericViewSet) -> bool: environment_pk = view.kwargs["environment_pk"] environment = Environment.objects.get(id=environment_pk) + + tag_ids = None + required_permission = UPDATE_FEATURE_STATE + + if required_permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS: + feature_id = view.kwargs["feature_pk"] + feature = Feature.objects.get(id=feature_id, project=environment.project) + tag_ids = list(feature.tags.values_list("id", flat=True)) + return request.user.has_environment_permission( - permission=UPDATE_FEATURE_STATE, environment=environment + permission=required_permission, environment=environment, tag_ids=tag_ids ) def has_object_permission( @@ -30,8 +42,14 @@ def has_object_permission( # permissions for retrieving handled in view.get_queryset return True + tag_ids = None + required_permission = UPDATE_FEATURE_STATE + + if required_permission in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS: + tag_ids = list(obj.feature.tags.values_list("id", flat=True)) + return request.user.has_environment_permission( - permission=UPDATE_FEATURE_STATE, environment=obj.environment + permission=required_permission, environment=obj.environment, tag_ids=tag_ids ) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 97c5d701426f..02705c03c8ef 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -2193,8 +2193,7 @@ def test_cannot_create_feature_state_for_feature_from_different_project( ) # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json()["feature"][0] == "Feature does not exist in project" + assert response.status_code == status.HTTP_403_FORBIDDEN def test_create_feature_state_environment_is_read_only( @@ -2255,8 +2254,7 @@ def test_cannot_create_feature_state_of_feature_from_different_project( ) # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json()["feature"][0] == "Feature does not exist in project" + assert response.status_code == status.HTTP_403_FORBIDDEN def test_create_feature_state_environment_field_is_read_only( diff --git a/api/users/abc.py b/api/users/abc.py index 69696f69ac05..cc768b9fd6d1 100644 --- a/api/users/abc.py +++ b/api/users/abc.py @@ -30,12 +30,14 @@ def is_group_admin(self, group_id: int) -> bool: raise NotImplementedError() @abstractmethod - def has_project_permission(self, permission: str, project: "Project") -> bool: + def has_project_permission( + self, permission: str, project: "Project", tag_ids: list[int] = None + ) -> bool: raise NotImplementedError() @abstractmethod def has_environment_permission( - self, permission: str, environment: "Environment" + self, permission: str, environment: "Environment", tag_ids: list[int] = None ) -> bool: raise NotImplementedError()