diff --git a/api/app/settings/local.py b/api/app/settings/local.py index 4ff77165c5b5..becc3a0cea18 100644 --- a/api/app/settings/local.py +++ b/api/app/settings/local.py @@ -5,7 +5,7 @@ ALLOWED_HOSTS.extend([".ngrok.io", "127.0.0.1", "localhost"]) -INSTALLED_APPS.extend(["debug_toolbar"]) +INSTALLED_APPS.extend(["debug_toolbar", "django_extensions"]) MIDDLEWARE.extend(["debug_toolbar.middleware.DebugToolbarMiddleware"]) diff --git a/api/conftest.py b/api/conftest.py index fd2ae4b61d36..b1c381d4191e 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -80,8 +80,10 @@ def test_user_client(api_client, test_user): def staff_user(django_user_model): """ A non-admin user fixture. + To add to an environment with permissions use the fixture - with_environment_permissions. + with_environment_permissions, or similar with the fixture + This fixture is attached to the organisation fixture. """ @@ -199,6 +201,29 @@ def _with_environment_permissions( return _with_environment_permissions +@pytest.fixture() +def with_project_permissions( + project: Project, staff_user: FFAdminUser +) -> typing.Callable: + """ + Add project permissions to the staff_user fixture. + Defaults to associating to the project fixture. + """ + + def _with_project_permissions( + permission_keys: list[str], project_id: typing.Optional[int] = None + ) -> UserProjectPermission: + project_id = project_id or project.id + upp, __ = UserProjectPermission.objects.get_or_create( + project_id=project_id, user=staff_user + ) + upp.permissions.add(*permission_keys) + + return upp + + return _with_project_permissions + + @pytest.fixture() def identity(environment): return Identity.objects.create(identifier="test_identity", environment=environment) diff --git a/api/environments/migrations/0010_auto_20200219_2343.py b/api/environments/migrations/0010_auto_20200219_2343.py index b7c102929f30..119513c81c84 100644 --- a/api/environments/migrations/0010_auto_20200219_2343.py +++ b/api/environments/migrations/0010_auto_20200219_2343.py @@ -1,9 +1,20 @@ # Generated by Django 2.2.10 on 2020-02-19 23:43 from django.db import migrations - -from environments.permissions.constants import ENVIRONMENT_PERMISSIONS - +from environments.permissions.constants import VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE, MANAGE_IDENTITIES, CREATE_CHANGE_REQUEST, APPROVE_CHANGE_REQUEST +ENVIRONMENT_PERMISSIONS = [ + (VIEW_ENVIRONMENT, "View permission for the given environment."), + (UPDATE_FEATURE_STATE, "Update the state or value for a given feature state."), + (MANAGE_IDENTITIES, "Manage identities in the given environment."), + ( + CREATE_CHANGE_REQUEST, + "Permission to create change requests in the given environment.", + ), + ( + APPROVE_CHANGE_REQUEST, + "Permission to approve change requests in the given environment.", + ), +] def create_default_permissions(apps, schema_editor): EnvironmentPermission = apps.get_model('environments', 'EnvironmentPermission') diff --git a/api/environments/permissions/constants.py b/api/environments/permissions/constants.py index adfc9aba81df..90b4f6a192ea 100644 --- a/api/environments/permissions/constants.py +++ b/api/environments/permissions/constants.py @@ -5,19 +5,6 @@ VIEW_IDENTITIES = "VIEW_IDENTITIES" CREATE_CHANGE_REQUEST = "CREATE_CHANGE_REQUEST" APPROVE_CHANGE_REQUEST = "APPROVE_CHANGE_REQUEST" - -ENVIRONMENT_PERMISSIONS = [ - (VIEW_ENVIRONMENT, "View permission for the given environment."), - (UPDATE_FEATURE_STATE, "Update the state or value for a given feature state."), - (MANAGE_IDENTITIES, "Manage identities in the given environment."), - ( - CREATE_CHANGE_REQUEST, - "Permission to create change requests in the given environment.", - ), - ( - APPROVE_CHANGE_REQUEST, - "Permission to approve change requests in the given environment.", - ), -] +MANAGE_SEGMENT_OVERRIDES = "MANAGE_SEGMENT_OVERRIDES" TAG_SUPPORTED_PERMISSIONS = [UPDATE_FEATURE_STATE] diff --git a/api/environments/permissions/migrations/0008_add_manage_segment_overrides_permission.py b/api/environments/permissions/migrations/0008_add_manage_segment_overrides_permission.py new file mode 100644 index 000000000000..4ac562b6ceeb --- /dev/null +++ b/api/environments/permissions/migrations/0008_add_manage_segment_overrides_permission.py @@ -0,0 +1,55 @@ +# Generated by Django 3.2.20 on 2023-11-01 19:54 + +from django.db import migrations +from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES, UPDATE_FEATURE_STATE +from core.migration_helpers import create_new_environment_permissions + +from permissions.models import ENVIRONMENT_PERMISSION_TYPE + +def add_manage_segment_overrides_permission(apps, schema_editor): + PermissionModel = apps.get_model("permissions", "PermissionModel") + UserEnvironmentPermission = apps.get_model( + "environment_permissions", + "UserEnvironmentPermission", + ) + UserPermissionGroupEnvironmentPermission = apps.get_model( + "environment_permissions", + "UserPermissionGroupEnvironmentPermission", + ) + manage_segment_overrides_permission, _ = PermissionModel.objects.get_or_create( + key=MANAGE_SEGMENT_OVERRIDES, + description="Permission to manage segment overrides in the given environment", + type=ENVIRONMENT_PERMISSION_TYPE, + ) + + # Add MANAGE_SEGMENT_OVERRIDES to existing UPDATE_FEATURE_STATE holders + create_new_environment_permissions( + UPDATE_FEATURE_STATE, + UserEnvironmentPermission, + "userenvironmentpermission", + [manage_segment_overrides_permission], + ) + create_new_environment_permissions( + UPDATE_FEATURE_STATE, + UserPermissionGroupEnvironmentPermission, + "userpermissiongroupenvironmentpermission", + [manage_segment_overrides_permission], + ) + + +def remove_manage_segment_overrides_permission(apps, schema_editor): + PermissionModel = apps.get_model("permissions", "PermissionModel") + PermissionModel.objects.filter(key=MANAGE_SEGMENT_OVERRIDES).delete() + +class Migration(migrations.Migration): + + dependencies = [ + ('environment_permissions', '0007_add_unique_permission_constraint'), + ] + + operations = [ + migrations.RunPython( + add_manage_segment_overrides_permission, + reverse_code=remove_manage_segment_overrides_permission, + ) + ] diff --git a/api/features/feature_segments/permissions.py b/api/features/feature_segments/permissions.py index 6a07f66eb407..134e0320e89f 100644 --- a/api/features/feature_segments/permissions.py +++ b/api/features/feature_segments/permissions.py @@ -3,7 +3,7 @@ from rest_framework.permissions import IsAuthenticated from environments.models import Environment -from environments.permissions.constants import UPDATE_FEATURE_STATE +from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES class FeatureSegmentPermissions(IsAuthenticated): @@ -25,12 +25,12 @@ def has_permission(self, request, view): environment = Environment.objects.get(id=int(environment)) return request.user.has_environment_permission( - UPDATE_FEATURE_STATE, environment + MANAGE_SEGMENT_OVERRIDES, environment ) return False def has_object_permission(self, request, view, obj): return request.user.has_environment_permission( - UPDATE_FEATURE_STATE, environment=obj.environment + MANAGE_SEGMENT_OVERRIDES, environment=obj.environment ) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 7a2e89d823bd..f5f35d986a19 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -1,7 +1,7 @@ from rest_framework import serializers from rest_framework.exceptions import PermissionDenied -from environments.permissions.constants import UPDATE_FEATURE_STATE +from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES from features.models import FeatureSegment @@ -115,7 +115,7 @@ def validate(self, attrs): environment = environments.pop() if not self.context["request"].user.has_environment_permission( - UPDATE_FEATURE_STATE, environment + MANAGE_SEGMENT_OVERRIDES, environment ): raise PermissionDenied("You do not have permission to perform this action.") diff --git a/api/features/feature_segments/tests/test_views.py b/api/features/feature_segments/tests/test_views.py index 65f5b51407ef..1a510442582d 100644 --- a/api/features/feature_segments/tests/test_views.py +++ b/api/features/feature_segments/tests/test_views.py @@ -1,4 +1,5 @@ import json +from typing import Callable import pytest from django.urls import reverse @@ -6,8 +7,15 @@ from rest_framework import status from environments.models import Environment +from environments.permissions.constants import ( + MANAGE_SEGMENT_OVERRIDES, + UPDATE_FEATURE_STATE, +) from features.models import Feature, FeatureSegment +from projects.models import Project, UserProjectPermission +from projects.permissions import VIEW_PROJECT from segments.models import Segment +from users.models import FFAdminUser @pytest.mark.parametrize( @@ -138,6 +146,59 @@ def test_create_feature_segment_without_permission_returns_403( assert response.status_code == status.HTTP_403_FORBIDDEN +def test_create_feature_segment_staff_with_permission( + segment: Segment, + feature: Feature, + environment: Environment, + staff_client: FFAdminUser, + staff_user: FFAdminUser, + with_environment_permissions: Callable, +) -> None: + # Given + data = { + "feature": feature.id, + "segment": segment.id, + "environment": environment.id, + } + url = reverse("api-v1:features:feature-segment-list") + with_environment_permissions([MANAGE_SEGMENT_OVERRIDES]) + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_create_feature_segment_staff_wrong_permission( + segment: Segment, + feature: Feature, + environment: Environment, + staff_client: FFAdminUser, + staff_user: FFAdminUser, + with_environment_permissions: Callable, +): + # Given + data = { + "feature": feature.id, + "segment": segment.id, + "environment": environment.id, + } + url = reverse("api-v1:features:feature-segment-list") + # Former permission; no longer authorizes. + with_environment_permissions([UPDATE_FEATURE_STATE]) + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], @@ -197,6 +258,33 @@ def test_update_priority_of_multiple_feature_segments( assert json_response[1]["id"] == another_feature_segment.id +def test_update_priority_for_staff( + feature_segment: FeatureSegment, + project: Project, + environment: Environment, + feature: Feature, + staff_client: FFAdminUser, + staff_user: FFAdminUser, + with_environment_permissions: Callable, +) -> None: + # Given + url = reverse("api-v1:features:feature-segment-update-priorities") + + data = [ + {"id": feature_segment.id, "priority": 1}, + ] + + with_environment_permissions([MANAGE_SEGMENT_OVERRIDES]) + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + def test_update_priority_returns_403_if_user_does_not_have_permission( feature_segment, project, @@ -258,6 +346,33 @@ def test_get_feature_segment_by_uuid( assert json_response["uuid"] == str(feature_segment.uuid) +def test_get_feature_segment_by_uuid_for_staff( + feature_segment: FeatureSegment, + project: Project, + staff_client: FFAdminUser, + staff_user: FFAdminUser, + environment: Environment, + feature: Feature, + with_environment_permissions: Callable, + with_project_permissions: Callable, +) -> None: + # Given + url = reverse( + "api-v1:features:feature-segment-get-by-uuid", args=[feature_segment.uuid] + ) + with_environment_permissions([UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + json_response = response.json() + assert json_response["id"] == feature_segment.id + assert json_response["uuid"] == str(feature_segment.uuid) + + def test_get_feature_segment_by_uuid_returns_404_if_user_does_not_have_access( feature_segment, project, test_user_client, environment, feature ): @@ -293,6 +408,34 @@ def test_get_feature_segment_by_id( assert json_response["uuid"] == str(feature_segment.uuid) +def test_get_feature_segment_by_id_for_staff( + feature_segment: FeatureSegment, + project: Project, + staff_client: FFAdminUser, + staff_user: FFAdminUser, + environment: Environment, + feature: Feature, + with_environment_permissions: Callable, +): + # Given + url = reverse("api-v1:features:feature-segment-detail", args=[feature_segment.id]) + + with_environment_permissions([MANAGE_SEGMENT_OVERRIDES]) + user_project_permission = UserProjectPermission.objects.create( + user=staff_user, project=project + ) + user_project_permission.add_permission(VIEW_PROJECT) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + json_response = response.json() + assert json_response["id"] == feature_segment.id + assert json_response["uuid"] == str(feature_segment.uuid) + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], diff --git a/api/features/feature_segments/views.py b/api/features/feature_segments/views.py index 8f2e8f09d991..e47213577401 100644 --- a/api/features/feature_segments/views.py +++ b/api/features/feature_segments/views.py @@ -43,6 +43,7 @@ def get_queryset(self): permitted_projects = self.request.user.get_permitted_projects( permission_key=VIEW_PROJECT ) + queryset = FeatureSegment.objects.filter( feature__project__in=permitted_projects ) diff --git a/api/features/permissions.py b/api/features/permissions.py index 8d84b426829d..23c6061e8429 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -4,6 +4,7 @@ from rest_framework.permissions import IsAuthenticated from environments.models import Environment +from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES from environments.permissions.constants import ( TAG_SUPPORTED_PERMISSIONS as TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS, ) @@ -166,8 +167,7 @@ def has_permission(self, request, view): Environment, api_key=view.kwargs["environment_api_key"] ) - # TODO: create dedicated permission for creating segment overrides return request.user.has_environment_permission( - permission=UPDATE_FEATURE_STATE, + permission=MANAGE_SEGMENT_OVERRIDES, environment=environment, ) diff --git a/api/features/tests/test_views.py b/api/features/tests/test_views.py index 826d07ad3314..5b66a2ff93eb 100644 --- a/api/features/tests/test_views.py +++ b/api/features/tests/test_views.py @@ -19,6 +19,8 @@ from audit.models import AuditLog, RelatedObjectType from environments.identities.models import Identity from environments.models import Environment, EnvironmentAPIKey +from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES +from environments.permissions.models import UserEnvironmentPermission from features.models import ( Feature, FeatureSegment, @@ -787,6 +789,74 @@ def test_get_feature_evaluation_data(project, feature, environment, mocker, clie ) +def test_create_segment_override_forbidden( + feature: Feature, + segment: Segment, + environment: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, +) -> None: + # Given + url = reverse( + "api-v1:environments:create-segment-override", + args=[environment.api_key, feature.id], + ) + + # When + enabled = True + string_value = "foo" + data = { + "feature_state_value": {"string_value": string_value}, + "enabled": enabled, + "feature_segment": {"segment": segment.id}, + } + + # Staff client lacks permission to create segment. + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == 403 + assert response.data == { + "detail": "You do not have permission to perform this action." + } + + +def test_create_segment_override_staff( + feature: Feature, + segment: Segment, + environment: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, +) -> None: + # Given + url = reverse( + "api-v1:environments:create-segment-override", + args=[environment.api_key, feature.id], + ) + + # When + enabled = True + string_value = "foo" + data = { + "feature_state_value": {"string_value": string_value}, + "enabled": enabled, + "feature_segment": {"segment": segment.id}, + } + user_environment_permission = UserEnvironmentPermission.objects.create( + user=staff_user, admin=False, environment=environment + ) + user_environment_permission.permissions.add(MANAGE_SEGMENT_OVERRIDES) + + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + # Then + assert response.status_code == 201 + assert response.data["feature_segment"]["segment"] == segment.id + + def test_create_segment_override(admin_client, feature, segment, environment): # Given url = reverse( diff --git a/api/features/views.py b/api/features/views.py index f53beca91f9b..9ff5a2edaeb7 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -733,8 +733,8 @@ def organisation_has_got_feature(request, organisation): request_body=CreateSegmentOverrideFeatureStateSerializer(), responses={200: CreateSegmentOverrideFeatureStateSerializer()}, ) -@permission_classes([CreateSegmentOverridePermissions()]) @api_view(["POST"]) +@permission_classes([CreateSegmentOverridePermissions]) def create_segment_override( request: Request, environment_api_key: str, feature_pk: int ): diff --git a/api/permissions/migrations/0001_initial.py b/api/permissions/migrations/0001_initial.py index 4fe8eaf2b9bb..33a0f25ce2d8 100644 --- a/api/permissions/migrations/0001_initial.py +++ b/api/permissions/migrations/0001_initial.py @@ -2,9 +2,22 @@ from django.db import migrations, models -from environments.permissions.constants import ENVIRONMENT_PERMISSIONS from permissions.models import PROJECT_PERMISSION_TYPE, ENVIRONMENT_PERMISSION_TYPE from projects.permissions import PROJECT_PERMISSIONS +from environments.permissions.constants import VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE, MANAGE_IDENTITIES, CREATE_CHANGE_REQUEST, APPROVE_CHANGE_REQUEST +ENVIRONMENT_PERMISSIONS = [ + (VIEW_ENVIRONMENT, "View permission for the given environment."), + (UPDATE_FEATURE_STATE, "Update the state or value for a given feature state."), + (MANAGE_IDENTITIES, "Manage identities in the given environment."), + ( + CREATE_CHANGE_REQUEST, + "Permission to create change requests in the given environment.", + ), + ( + APPROVE_CHANGE_REQUEST, + "Permission to approve change requests in the given environment.", + ), +] def insert_default_project_permissions(apps, schema_model): diff --git a/api/poetry.lock b/api/poetry.lock index b8f500fe0f24..c4c397512176 100644 --- a/api/poetry.lock +++ b/api/poetry.lock @@ -986,6 +986,20 @@ files = [ {file = "django_environ-0.4.5-py2.py3-none-any.whl", hash = "sha256:c57b3c11ec1f319d9474e3e5a79134f40174b17c7cc024bbb2fad84646b120c4"}, ] +[[package]] +name = "django-extensions" +version = "3.2.3" +description = "Extensions for Django" +optional = false +python-versions = ">=3.6" +files = [ + {file = "django-extensions-3.2.3.tar.gz", hash = "sha256:44d27919d04e23b3f40231c4ab7af4e61ce832ef46d610cc650d53e68328410a"}, + {file = "django_extensions-3.2.3-py3-none-any.whl", hash = "sha256:9600b7562f79a92cbf1fde6403c04fee314608fefbb595502e34383ae8203401"}, +] + +[package.dependencies] +Django = ">=3.2" + [[package]] name = "django-filter" version = "2.4.0" @@ -4341,4 +4355,4 @@ requests = ">=2.7,<3.0" [metadata] lock-version = "2.0" python-versions = "^3.10" -content-hash = "34d4e3a4f1c1ccd342d97262a2b6f0bc1c293fad2541ab7faffb5594d25ad16f" +content-hash = "6fcfd26eb38ab86df49a98ddb6da7afbd484f50fca8798612eac0f658afd8b6a" diff --git a/api/pyproject.toml b/api/pyproject.toml index 2cb6b6e538da..9f19ee523390 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -129,6 +129,7 @@ pip-tools = "~6.13.0" pytest-cov = "~4.1.0" datamodel-code-generator = "~0.22" requests-mock = "^1.11.0" +django-extensions = "^3.2.3" pdbpp = "^0.10.3" django-capture-on-commit-callbacks = "^1.11.0" diff --git a/api/tests/unit/environments/test_unit_environments_views.py b/api/tests/unit/environments/test_unit_environments_views.py index 7e34e6cad930..0cbba069fbe3 100644 --- a/api/tests/unit/environments/test_unit_environments_views.py +++ b/api/tests/unit/environments/test_unit_environments_views.py @@ -891,9 +891,7 @@ def test_user_can_list_environment_permission(client, environment): # Then assert response.status_code == status.HTTP_200_OK - assert ( - len(response.json()) == 6 - ) # hard code how many permissions we expect there to be + assert len(response.json()) == 7 def test_environment_my_permissions_reruns_400_for_master_api_key(