fix: Tag Based permissions only validate some views (#4523)

Co-authored-by: Gagan Trivedi <[email protected]>
Flagsmith · Oct 21, 2024 · 6d2ab58 · 6d2ab58
1 parent 99876ca
commit 6d2ab58
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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()