fix(featurestate-permissions): Add misc extra checks (#2712)

Co-authored-by: Matthew Elwell <[email protected]>

api/environments/tests/test_views.py

@@ -83,32 +83,6 @@ def test_should_return_identities_for_an_environment(self):
         assert response.data["results"][0]["identifier"] == identifier_one
         assert response.data["results"][1]["identifier"] == identifier_two
 
-    def test_should_update_value_of_feature_state(self):
-        # Given
-        feature = Feature.objects.create(name="feature", project=self.project)
-        environment = Environment.objects.create(name="test env", project=self.project)
-        feature_state = FeatureState.objects.get(
-            feature=feature, environment=environment
-        )
-        url = reverse(
-            "api-v1:environments:environment-featurestates-detail",
-            args=[environment.api_key, feature_state.id],
-        )
-
-        # When
-        response = self.client.put(
-            url,
-            data=self.fs_put_template % (feature_state.id, True, "This is a value"),
-            content_type="application/json",
-        )
-
-        # Then
-        feature_state.refresh_from_db()
-
-        assert response.status_code == status.HTTP_200_OK
-        assert feature_state.get_feature_state_value() == "This is a value"
-        assert feature_state.enabled
-
     def test_audit_log_entry_created_when_new_environment_created(self):
         # Given
         url = reverse("api-v1:environments:environment-list")

api/features/serializers.py

@@ -308,11 +308,30 @@ def save(self, **kwargs):
         except django.core.exceptions.ValidationError as e:
             raise serializers.ValidationError(e.message)
 
+    def validate_feature(self, feature):
+        if self.instance and self.instance.feature_id != feature.id:
+            raise serializers.ValidationError(
+                "Cannot change the feature of a feature state"
+            )
+        return feature
+
+    def validate_environment(self, environment):
+        if self.instance and self.instance.environment_id != environment.id:
+            raise serializers.ValidationError(
+                "Cannot change the environment of a feature state"
+            )
+        return environment
+
     def validate(self, attrs):
-        environment = attrs.get("environment")
+        environment = attrs.get("environment") or self.context["environment"]
         identity = attrs.get("identity")
         feature_segment = attrs.get("feature_segment")
         identifier = attrs.pop("identifier", None)
+        feature = attrs.get("feature")
+        if feature and feature.project_id != environment.project_id:
+            error = {"feature": "Feature does not exist in project"}
+            raise serializers.ValidationError(error)
+
         if identifier:
             try:
                 identity = Identity.objects.get(

api/features/views.py

@@ -299,6 +299,12 @@ def get_serializer_class(self):
         else:
             return FeatureStateSerializerCreate
 
+    def get_serializer_context(self):
+        context = super().get_serializer_context()
+        if self.action in ["update", "create"]:
+            context["environment"] = self.get_environment_from_request()
+        return context
+
     def get_queryset(self):
         """
         Override queryset to filter based on provided URL parameters.
@@ -341,46 +347,26 @@ def get_environment_from_request(self):
         )
         return environment
 
-    def get_identity_from_request(self, environment):
-        """
-        Get identity object from URL parameters in request.
-        """
-        identity = Identity.objects.get(pk=self.kwargs["identity_pk"])
-        return identity
-
     def create(self, request, *args, **kwargs):
         """
         DEPRECATED: please use `/features/featurestates/` instead.
         Override create method to add environment and identity (if present) from URL parameters.
         """
         data = request.data
-        environment = self.get_environment_from_request()
-        if (
-            environment.project.organisation
-            not in self.request.user.organisations.all()
-        ):
-            return Response(status.HTTP_403_FORBIDDEN)
-
-        data["environment"] = environment.id
-
         if "feature" not in data:
             error = {"detail": "Feature not provided"}
             return Response(error, status=status.HTTP_400_BAD_REQUEST)
 
-        feature_id = int(data["feature"])
-
-        if feature_id not in [
-            feature.id for feature in environment.project.features.all()
-        ]:
-            error = {"detail": "Feature does not exist in project"}
-            return Response(error, status=status.HTTP_400_BAD_REQUEST)
+        environment = self.get_environment_from_request()
+        data["environment"] = environment.id
 
         identity_pk = self.kwargs.get("identity_pk")
         if identity_pk:
             data["identity"] = identity_pk
 
         serializer = self.get_serializer(data=data)
-        if serializer.is_valid():
+
+        if serializer.is_valid(raise_exception=True):
             feature_state = serializer.save()
             headers = self.get_success_headers(serializer.data)
 
@@ -394,10 +380,6 @@ def create(self, request, *args, **kwargs):
                 status=status.HTTP_201_CREATED,
                 headers=headers,
             )
-        else:
-            logger.error(serializer.errors)
-            error = {"detail": "Couldn't create feature state."}
-            return Response(error, status=status.HTTP_400_BAD_REQUEST)
 
     def update(self, request, *args, **kwargs):
         """

api/tests/unit/conftest.py

@@ -182,3 +182,10 @@ def project_two_environment(project_two: Project) -> Environment:
     return Environment.objects.create(
         name="Test Project two Environment", project=project_two
     )
+
+
+@pytest.fixture
+def project_two_feature(project_two: Project) -> Feature:
+    return Feature.objects.create(
+        name="project_two_feature", project=project_two, initial_value="initial_value"
+    )

api/tests/unit/features/test_unit_features_views.py

@@ -864,3 +864,158 @@ def test_create_feature_reaching_max_limit(client, project, settings):
         response.json()["project"]
         == "The Project has reached the maximum allowed features limit."
     )
+
+
+@pytest.mark.parametrize(
+    "client",
+    [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))],
+)
+def test_create_segment_override_using_environment_viewset(
+    client, environment, feature, feature_segment
+):
+    # Given
+    url = reverse(
+        "api-v1:environments:environment-featurestates-list",
+        args=[environment.api_key],
+    )
+    new_value = "new-value"
+    data = {
+        "feature_state_value": new_value,
+        "enabled": False,
+        "feature": feature.id,
+        "environment": environment.id,
+        "identity": None,
+        "feature_segment": feature_segment.id,
+    }
+
+    # When
+    response = client.post(url, data=json.dumps(data), content_type="application/json")
+
+    # Then
+    assert response.status_code == status.HTTP_201_CREATED
+    response.json()["feature_state_value"] == new_value
+
+
+@pytest.mark.parametrize(
+    "client",
+    [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))],
+)
+def test_cannot_create_feature_state_for_feature_from_different_project(
+    client, environment, project_two_feature, feature_segment, project_two
+):
+    # Given
+    url = reverse(
+        "api-v1:environments:environment-featurestates-list",
+        args=[environment.api_key],
+    )
+    new_value = "new-value"
+    data = {
+        "feature_state_value": new_value,
+        "enabled": False,
+        "feature": project_two_feature.id,
+        "environment": environment.id,
+        "identity": None,
+        "feature_segment": feature_segment.id,
+    }
+
+    # When
+    response = client.post(url, data=json.dumps(data), content_type="application/json")
+
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.json()["feature"][0] == "Feature does not exist in project"
+
+
+@pytest.mark.parametrize(
+    "client",
+    [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))],
+)
+def test_create_feature_state_environment_is_read_only(
+    client, environment, feature, feature_segment, environment_two
+):
+    # Given
+    url = reverse(
+        "api-v1:environments:environment-featurestates-list",
+        args=[environment.api_key],
+    )
+    new_value = "new-value"
+    data = {
+        "feature_state_value": new_value,
+        "enabled": False,
+        "feature": feature.id,
+        "environment": environment_two.id,
+        "feature_segment": feature_segment.id,
+    }
+
+    # When
+    response = client.post(url, data=json.dumps(data), content_type="application/json")
+
+    # Then
+    assert response.status_code == status.HTTP_201_CREATED
+    assert response.json()["environment"] == environment.id
+
+
+@pytest.mark.parametrize(
+    "client",
+    [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))],
+)
+def test_cannot_update_environment_of_a_feature_state(
+    client, environment, feature, feature_state, environment_two
+):
+    # Given
+    url = reverse(
+        "api-v1:environments:environment-featurestates-detail",
+        args=[environment.api_key, feature_state.id],
+    )
+    new_value = "new-value"
+    data = {
+        "id": feature_state.id,
+        "feature_state_value": new_value,
+        "enabled": False,
+        "feature": feature.id,
+        "environment": environment_two.id,
+        "identity": None,
+        "feature_segment": None,
+    }
+
+    # When
+    response = client.put(url, data=json.dumps(data), content_type="application/json")
+
+    # Then - it did not change the environment field on the feature state
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert (
+        response.json()["environment"][0]
+        == "Cannot change the environment of a feature state"
+    )
+
+
+@pytest.mark.parametrize(
+    "client",
+    [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))],
+)
+def test_cannot_update_feature_of_a_feature_state(
+    client, environment, feature_state, feature, identity, project
+):
+    # Given
+    another_feature = Feature.objects.create(
+        name="another_feature", project=project, initial_value="initial_value"
+    )
+    url = reverse("api-v1:features:featurestates-detail", args=[feature_state.id])
+
+    feature_state_value = "New value"
+    data = {
+        "enabled": True,
+        "feature_state_value": {"type": "unicode", "string_value": feature_state_value},
+        "environment": environment.id,
+        "feature": another_feature.id,
+    }
+
+    # When
+    response = client.put(url, data=json.dumps(data), content_type="application/json")
+
+    # Then
+    assert another_feature.feature_states.count() == 1
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert (
+        response.json()["feature"][0] == "Cannot change the feature of a feature state"
+    )