From 3b873b2f24dd9522053efce0c11e76db35294b89 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 09:48:47 +0530 Subject: [PATCH 1/9] feat(feature/models/get_environment_flags_qs): add feature name arg --- api/features/models.py | 8 ++++++-- .../unit/features/test_unit_features_models.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/api/features/models.py b/api/features/models.py index 4580c378c430..8aa132e90c52 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -562,12 +562,16 @@ def get_environment_flags_list( return list(feature_states_dict.values()) @classmethod - def get_environment_flags_queryset(cls, environment_id: int) -> QuerySet: + def get_environment_flags_queryset( + cls, environment_id: int, feature_name: str = None + ) -> QuerySet: """ Get a queryset of the latest live versions of an environments' feature states """ - feature_states_list = cls.get_environment_flags_list(environment_id) + feature_states_list = cls.get_environment_flags_list( + environment_id, feature_name + ) return FeatureState.objects.filter(id__in=[fs.id for fs in feature_states_list]) @classmethod diff --git a/api/tests/unit/features/test_unit_features_models.py b/api/tests/unit/features/test_unit_features_models.py index 20db6514320e..1fce9bbee982 100644 --- a/api/tests/unit/features/test_unit_features_models.py +++ b/api/tests/unit/features/test_unit_features_models.py @@ -51,6 +51,23 @@ def test_project_hide_disabled_flags_have_no_effect_on_feature_state_get_environ assert feature_states.count() == 2 +def test_feature_states_get_environment_flags_queryset_filter_using_feature_name( + environment, project +): + # Given + Feature.objects.create(default_enabled=False, name="disable_flag", project=project) + Feature.objects.create(default_enabled=True, name="enabled_flag", project=project) + + # When + feature_states = FeatureState.get_environment_flags_queryset( + environment_id=environment.id, feature_name="disabled_flag" + ) + + # Then + assert feature_states.count() == 1 + assert feature_states.first().feature.name == "enabled_flag" + + @pytest.mark.parametrize( "feature_state_version_generator", ( From 6d9dba563d0dc900fda82e508ff2a08d4b444726 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:25:30 +0530 Subject: [PATCH 2/9] Add MasterKey support to environment feature states --- api/features/views.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/features/views.py b/api/features/views.py index dfb0fcabb662..e7761c02308d 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -49,6 +49,7 @@ FeaturePermissions, FeatureStatePermissions, IdentityFeatureStatePermissions, + MasterAPIKeyEnvironmentFeatureStatePermissions, MasterAPIKeyFeatureStatePermissions, ) from .serializers import ( @@ -484,7 +485,10 @@ def update_feature_state_value(self, value, feature_state): class EnvironmentFeatureStateViewSet(BaseFeatureStateViewSet): - permission_classes = [IsAuthenticated, EnvironmentFeatureStatePermissions] + permission_classes = [ + EnvironmentFeatureStatePermissions + | MasterAPIKeyEnvironmentFeatureStatePermissions + ] def get_queryset(self): queryset = super().get_queryset().filter(feature_segment=None) From c822d6c29caa8a0e08ff0139d722d67c351560ba Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:27:08 +0530 Subject: [PATCH 3/9] features state viewset add feature name filter --- api/features/views.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/api/features/views.py b/api/features/views.py index e7761c02308d..9929165eb7c7 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -12,11 +12,7 @@ from drf_yasg2.utils import swagger_auto_schema from rest_framework import mixins, status, viewsets from rest_framework.decorators import action -from rest_framework.exceptions import ( - NotFound, - PermissionDenied, - ValidationError, -) +from rest_framework.exceptions import NotFound, ValidationError from rest_framework.generics import GenericAPIView, get_object_or_404 from rest_framework.permissions import IsAuthenticated from rest_framework.renderers import JSONRenderer @@ -35,7 +31,6 @@ from environments.authentication import EnvironmentKeyAuthentication from environments.identities.models import Identity from environments.models import Environment -from environments.permissions.constants import VIEW_ENVIRONMENT from environments.permissions.permissions import ( EnvironmentKeyPermissions, NestedEnvironmentPermissions, @@ -268,6 +263,13 @@ def _filter_queryset(self, queryset: QuerySet) -> QuerySet: required=False, type=openapi.TYPE_INTEGER, ), + openapi.Parameter( + "feature_name", + openapi.IN_QUERY, + "Name of the feature to filter by.", + required=False, + type=openapi.TYPE_STRING, + ), openapi.Parameter( "anyIdentity", openapi.IN_QUERY, @@ -304,13 +306,9 @@ def get_queryset(self): try: environment = Environment.objects.get(api_key=environment_api_key) - if not self.request.user.has_environment_permission( - VIEW_ENVIRONMENT, environment - ): - raise PermissionDenied() - queryset = FeatureState.get_environment_flags_queryset( - environment_id=environment.id + environment_id=environment.id, + feature_name=self.request.query_params.get("feature_name"), ) queryset = self._apply_query_param_filters(queryset) From ba98941502d67bde6a9b3908c1e96c60aa0ac4f5 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:27:53 +0530 Subject: [PATCH 4/9] move fs list action permission check to permission class --- api/environments/permissions/permissions.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/environments/permissions/permissions.py b/api/environments/permissions/permissions.py index c38dfde1baed..1327881b39ff 100644 --- a/api/environments/permissions/permissions.py +++ b/api/environments/permissions/permissions.py @@ -5,6 +5,7 @@ from rest_framework.permissions import BasePermission from environments.models import Environment +from environments.permissions.constants import VIEW_ENVIRONMENT from projects.models import Project @@ -101,7 +102,12 @@ def has_permission(self, request, view): elif view.action == "create": return request.user.is_environment_admin(environment) - return view.action == "list" or view.detail + elif view.action == "list": + return request.user.has_environment_permission( + VIEW_ENVIRONMENT, environment + ) + + return view.detail def has_object_permission(self, request, view, obj): if view.action in self.action_permission_map: From 30709eae76dff2bab42013a98cd0452180336d5e Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:28:42 +0530 Subject: [PATCH 5/9] Add master api key permission for feature states --- api/features/permissions.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/api/features/permissions.py b/api/features/permissions.py index 9a7f3776044d..4676cb04345a 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -113,14 +113,40 @@ def has_object_permission( return False -class EnvironmentFeatureStatePermissions(BasePermission): +class MasterAPIKeyEnvironmentFeatureStatePermissions(BasePermission): + def has_permission(self, request: HttpRequest, view: str) -> bool: + master_api_key = getattr(request, "master_api_key", None) + if not master_api_key: + return False + environment_api_key = view.kwargs.get("environment_api_key") + if not environment_api_key: + return False + + with suppress(Environment.DoesNotExist): + environment = Environment.objects.get(api_key=environment_api_key) + return environment.project.organisation == master_api_key.organisation + return False + + def has_object_permission( + self, request: HttpRequest, view: str, obj: FeatureState + ) -> bool: + master_api_key = getattr(request, "master_api_key", None) + if master_api_key: + return obj.environment.project.organisation == master_api_key.organisation + return False + + +class EnvironmentFeatureStatePermissions(IsAuthenticated): def has_permission(self, request, view): + if not super().has_permission(request, view): + return False + if view.action == "create": environment_api_key = view.kwargs.get("environment_api_key") if not environment_api_key: return False - environment = Environment.objects.get(api_key=environment_api_key) + return request.user.has_environment_permission( permission=UPDATE_FEATURE_STATE, environment=environment ) @@ -132,6 +158,8 @@ def has_permission(self, request, view): return view.detail def has_object_permission(self, request, view, obj): + if request.user.is_anonymous: + return False return request.user.has_environment_permission( permission=UPDATE_FEATURE_STATE, environment=obj.environment ) From a69baf02b584dc6946f7af616d42e45c80445c69 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:30:42 +0530 Subject: [PATCH 6/9] refac/Add tests for master api key --- api/features/tests/test_views.py | 126 ------------------ .../test_environment_featurestate_viewset.py | 35 +++++ api/tests/unit/conftest.py | 10 ++ .../unit/features/test_unit_features_views.py | 120 ++++++++++++++++- 4 files changed, 163 insertions(+), 128 deletions(-) create mode 100644 api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py diff --git a/api/features/tests/test_views.py b/api/features/tests/test_views.py index 498dc1cb813c..fea57f309f71 100644 --- a/api/features/tests/test_views.py +++ b/api/features/tests/test_views.py @@ -6,7 +6,6 @@ import pytz from django.forms import model_to_dict from django.urls import reverse -from django.utils import timezone from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -870,131 +869,6 @@ def test_create_feature_only_triggers_write_to_dynamodb_once_per_environment( mock_dynamo_environment_wrapper.write_environments.assert_called_once() -@pytest.mark.django_db() -class FeatureStateViewSetTestCase(TestCase): - def setUp(self) -> None: - self.organisation = Organisation.objects.create(name="Test org") - self.project = Project.objects.create( - name="Test project", organisation=self.organisation - ) - self.environment = Environment.objects.create( - project=self.project, name="Test environment" - ) - self.feature = Feature.objects.create( - name="test-feature", project=self.project, type="CONFIG", initial_value=12 - ) - self.user = FFAdminUser.objects.create(email="test@example.com") - self.user.add_organisation(self.organisation, OrganisationRole.ADMIN) - self.client = APIClient() - self.client.force_authenticate(self.user) - - def test_update_feature_state_value_updates_feature_state_value(self): - # Given - feature_state = FeatureState.objects.get( - environment=self.environment, feature=self.feature - ) - url = reverse( - "api-v1:environments:environment-featurestates-detail", - args=[self.environment.api_key, feature_state.id], - ) - new_value = "new-value" - data = { - "id": feature_state.id, - "feature_state_value": new_value, - "enabled": False, - "feature": self.feature.id, - "environment": self.environment.id, - "identity": None, - "feature_segment": None, - } - - # When - self.client.put(url, data=json.dumps(data), content_type="application/json") - - # Then - feature_state.refresh_from_db() - assert feature_state.get_feature_state_value() == new_value - - def test_can_filter_feature_states_to_show_identity_overrides_only(self): - # Given - FeatureState.objects.get(environment=self.environment, feature=self.feature) - - identifier = "test-identity" - identity = Identity.objects.create( - identifier=identifier, environment=self.environment - ) - FeatureState.objects.create( - environment=self.environment, feature=self.feature, identity=identity - ) - - base_url = reverse( - "api-v1:environments:environment-featurestates-list", - args=[self.environment.api_key], - ) - url = base_url + "?anyIdentity&feature=" + str(self.feature.id) - - # When - res = self.client.get(url) - - # Then - assert res.status_code == status.HTTP_200_OK - - # and - assert len(res.json().get("results")) == 1 - - # and - assert res.json()["results"][0]["identity"]["identifier"] == identifier - - def test_get_feature_states_only_returns_latest_versions(self): - # Given - feature_state = FeatureState.objects.get( - environment=self.environment, feature=self.feature - ) - feature_state_v2 = feature_state.clone( - env=self.environment, live_from=timezone.now(), version=2 - ) - - url = reverse( - "api-v1:environments:environment-featurestates-list", - args=[self.environment.api_key], - ) - - # When - response = self.client.get(url) - - # Then - assert response.status_code == status.HTTP_200_OK - - response_json = response.json() - assert len(response_json["results"]) == 1 - assert response_json["results"][0]["id"] == feature_state_v2.id - - def test_get_feature_states_does_not_return_null_versions(self): - # Given - feature_state = FeatureState.objects.get( - environment=self.environment, feature=self.feature - ) - - FeatureState.objects.create( - environment=self.environment, feature=self.feature, version=None - ) - - url = reverse( - "api-v1:environments:environment-featurestates-list", - args=[self.environment.api_key], - ) - - # When - response = self.client.get(url) - - # Then - assert response.status_code == status.HTTP_200_OK - - response_json = response.json() - assert len(response_json["results"]) == 1 - assert response_json["results"][0]["id"] == feature_state.id - - @pytest.mark.django_db class SDKFeatureStatesTestCase(APITestCase): def setUp(self) -> None: diff --git a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py new file mode 100644 index 000000000000..dabfc7b2d1cc --- /dev/null +++ b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py @@ -0,0 +1,35 @@ +import json + +import pytest +from django.urls import reverse +from pytest_lazyfixture import lazy_fixture +from rest_framework import status + + +@pytest.mark.parametrize( + "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] +) +def test_update_feature_state_value_updates_feature_state_value( + client, environment, environment_api_key, feature, feature_state +): + url = reverse( + "api-v1:environments:environment-featurestates-detail", + args=[environment_api_key, feature_state], + ) + new_value = "new-value" + data = { + "id": feature_state, + "feature_state_value": new_value, + "enabled": False, + "feature": feature, + "environment": environment, + "identity": None, + "feature_segment": None, + } + + # When + response = client.put(url, data=json.dumps(data), content_type="application/json") + + # Then + assert response.status_code == status.HTTP_200_OK + response.json()["feature_state_value"] == new_value diff --git a/api/tests/unit/conftest.py b/api/tests/unit/conftest.py index 787db993bfb9..e2f4f463bb9f 100644 --- a/api/tests/unit/conftest.py +++ b/api/tests/unit/conftest.py @@ -1,4 +1,5 @@ import pytest +from rest_framework.test import APIClient from api_keys.models import MasterAPIKey from environments.models import Environment @@ -101,3 +102,12 @@ def organisation_one_project_one_feature_one(organisation_one_project_one): def master_api_key(organisation): _, key = MasterAPIKey.objects.create_key(name="test_key", organisation=organisation) return key + + +@pytest.fixture() +def master_api_key_client(master_api_key): + # Can not use `api_client` fixture here because: + # https://docs.pytest.org/en/6.2.x/fixture.html#fixtures-can-be-requested-more-than-once-per-test-return-values-are-cached + api_client = APIClient() + api_client.credentials(HTTP_AUTHORIZATION="Api-Key " + master_api_key) + return api_client diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index cc82d38cd601..97c691cc154f 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -1,9 +1,13 @@ +import pytest from django.urls import reverse +from django.utils import timezone +from pytest_lazyfixture import lazy_fixture from rest_framework import status +from environments.identities.models import Identity from environments.models import Environment from features.feature_types import MULTIVARIATE -from features.models import Feature +from features.models import Feature, FeatureState from features.multivariate.models import MultivariateFeatureOption from organisations.models import Organisation from projects.models import Project, UserProjectPermission @@ -78,7 +82,7 @@ def test_list_feature_states_nested_environment_view_set( Feature.objects.create(name="another_feature", project=project) # When - with django_assert_num_queries(6): + with django_assert_num_queries(5): response = admin_client.get(base_url) # Then @@ -86,3 +90,115 @@ def test_list_feature_states_nested_environment_view_set( response_json = response.json() assert response_json["count"] == 3 + + +@pytest.mark.parametrize( + "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] +) +def test_environment_feature_states_filter_using_feataure_name( + environment, project, feature, client +): + # Given + Feature.objects.create(name="another_feature", project=project) + base_url = reverse( + "api-v1:environments:environment-featurestates-list", + args=[environment.api_key], + ) + url = f"{base_url}?feature_name={feature.name}" + + # When + response = client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["count"] == 1 + assert response.json()["results"][0]["feature"] == feature.id + + +@pytest.mark.parametrize( + "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] +) +def test_environment_feature_states_filter_to_show_identity_override_only( + environment, feature, client +): + # Given + FeatureState.objects.get(environment=environment, feature=feature) + + identifier = "test-identity" + identity = Identity.objects.create(identifier=identifier, environment=environment) + FeatureState.objects.create( + environment=environment, feature=feature, identity=identity + ) + + base_url = reverse( + "api-v1:environments:environment-featurestates-list", + args=[environment.api_key], + ) + url = base_url + "?anyIdentity&feature=" + str(feature.id) + + # When + res = client.get(url) + + # Then + assert res.status_code == status.HTTP_200_OK + + # and + assert len(res.json().get("results")) == 1 + + # and + assert res.json()["results"][0]["identity"]["identifier"] == identifier + + +@pytest.mark.parametrize( + "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] +) +def test_environment_feature_states_only_returns_latest_versions( + environment, feature, client +): + # Given + feature_state = FeatureState.objects.get(environment=environment, feature=feature) + feature_state_v2 = feature_state.clone( + env=environment, live_from=timezone.now(), version=2 + ) + + url = reverse( + "api-v1:environments:environment-featurestates-list", + args=[environment.api_key], + ) + + # When + response = client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert len(response_json["results"]) == 1 + assert response_json["results"][0]["id"] == feature_state_v2.id + + +@pytest.mark.parametrize( + "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] +) +def test_environment_feature_states_does_not_return_null_versions( + environment, feature, client +): + # Given + feature_state = FeatureState.objects.get(environment=environment, feature=feature) + + FeatureState.objects.create(environment=environment, feature=feature, version=None) + + url = reverse( + "api-v1:environments:environment-featurestates-list", + args=[environment.api_key], + ) + + # When + response = client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert len(response_json["results"]) == 1 + assert response_json["results"][0]["id"] == feature_state.id From 5678b64d23ad163c521dd5fcccafc2ea092caae2 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 11:53:00 +0530 Subject: [PATCH 7/9] fix test cases --- api/tests/unit/features/test_unit_features_models.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/api/tests/unit/features/test_unit_features_models.py b/api/tests/unit/features/test_unit_features_models.py index 1fce9bbee982..9ed73c5ae840 100644 --- a/api/tests/unit/features/test_unit_features_models.py +++ b/api/tests/unit/features/test_unit_features_models.py @@ -55,17 +55,18 @@ def test_feature_states_get_environment_flags_queryset_filter_using_feature_name environment, project ): # Given - Feature.objects.create(default_enabled=False, name="disable_flag", project=project) - Feature.objects.create(default_enabled=True, name="enabled_flag", project=project) + flag_1_name = "flag_1" + Feature.objects.create(default_enabled=True, name=flag_1_name, project=project) + Feature.objects.create(default_enabled=True, name="flag_2", project=project) # When feature_states = FeatureState.get_environment_flags_queryset( - environment_id=environment.id, feature_name="disabled_flag" + environment_id=environment.id, feature_name=flag_1_name ) # Then assert feature_states.count() == 1 - assert feature_states.first().feature.name == "enabled_flag" + assert feature_states.first().feature.name == "flag_1" @pytest.mark.parametrize( From 9eb5eda086e9adbffed72d227d0c91168a3be07a Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 23 Jun 2022 12:56:00 +0530 Subject: [PATCH 8/9] minor --- .../featurestate/test_environment_featurestate_viewset.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py index dabfc7b2d1cc..8f0c9a74cd31 100644 --- a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py +++ b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py @@ -12,6 +12,7 @@ def test_update_feature_state_value_updates_feature_state_value( client, environment, environment_api_key, feature, feature_state ): + # Given url = reverse( "api-v1:environments:environment-featurestates-detail", args=[environment_api_key, feature_state], From cd9ee4f7d00d3b1bba3ffcfab80853982946085c Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 29 Jun 2022 10:03:20 +0530 Subject: [PATCH 9/9] fix/permission add handle list --- api/features/permissions.py | 24 +++++++++---------- .../unit/features/test_unit_features_views.py | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/api/features/permissions.py b/api/features/permissions.py index 4676cb04345a..4c8279d584f1 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -138,24 +138,24 @@ def has_object_permission( class EnvironmentFeatureStatePermissions(IsAuthenticated): def has_permission(self, request, view): + action_permission_map = { + "list": VIEW_ENVIRONMENT, + "create": UPDATE_FEATURE_STATE, + } if not super().has_permission(request, view): return False - if view.action == "create": - environment_api_key = view.kwargs.get("environment_api_key") - if not environment_api_key: - return False - environment = Environment.objects.get(api_key=environment_api_key) + # detail view means we can just defer to object permissions + if view.detail: + return True + environment_api_key = view.kwargs.get("environment_api_key") + with suppress(Environment.DoesNotExist): + environment = Environment.objects.get(api_key=environment_api_key) return request.user.has_environment_permission( - permission=UPDATE_FEATURE_STATE, environment=environment + action_permission_map.get(view.action), environment ) - - if view.action == "list": - return True - - # move on to object specific permissions - return view.detail + return False def has_object_permission(self, request, view, obj): if request.user.is_anonymous: diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 97c691cc154f..395cfd628448 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -82,7 +82,7 @@ def test_list_feature_states_nested_environment_view_set( Feature.objects.create(name="another_feature", project=project) # When - with django_assert_num_queries(5): + with django_assert_num_queries(7): response = admin_client.get(base_url) # Then