diff --git a/api/environments/permissions/views.py b/api/environments/permissions/views.py index 0dbff15e7c4b..07d32d7e48b0 100644 --- a/api/environments/permissions/views.py +++ b/api/environments/permissions/views.py @@ -52,7 +52,7 @@ def perform_update(self, serializer): # type: ignore[no-untyped-def] class UserPermissionGroupEnvironmentPermissionsViewSet(viewsets.ModelViewSet): # type: ignore[type-arg] pagination_class = None - permission_classes = [IsAuthenticated] + permission_classes = [IsAuthenticated, NestedEnvironmentPermissions] def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): diff --git a/api/features/feature_external_resources/views.py b/api/features/feature_external_resources/views.py index 253c69cb1b64..6bd959ca84cc 100644 --- a/api/features/feature_external_resources/views.py +++ b/api/features/feature_external_resources/views.py @@ -1,5 +1,6 @@ import re +from common.projects.permissions import VIEW_PROJECT from django.shortcuts import get_object_or_404 from django.utils.decorators import method_decorator from drf_spectacular.utils import extend_schema @@ -41,9 +42,14 @@ def get_queryset(self): # type: ignore[no-untyped-def] if "pk" in self.kwargs: return FeatureExternalResource.objects.filter(id=self.kwargs["pk"]) - else: - features_pk = self.kwargs["feature_pk"] - return FeatureExternalResource.objects.filter(feature=features_pk) + + feature = get_object_or_404( + Feature.objects.filter( + project__in=self.request.user.get_permitted_projects(VIEW_PROJECT) # type: ignore[union-attr] + ), + pk=self.kwargs["feature_pk"], + ) + return feature.external_resources.all() def list(self, request, *args, **kwargs) -> Response: # type: ignore[no-untyped-def] queryset = self.get_queryset() # type: ignore[no-untyped-call] diff --git a/api/features/multivariate/views.py b/api/features/multivariate/views.py index 8045887eb59f..aecb0dee4a6d 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -76,7 +76,10 @@ def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): return MultivariateFeatureOption.objects.none() - feature = get_object_or_404(Feature, pk=self.kwargs["feature_pk"]) + feature = get_object_or_404( + Feature.objects.filter(project__id=self.kwargs["project_pk"]), + pk=self.kwargs["feature_pk"], + ) return feature.multivariate_options.all() diff --git a/api/integrations/github/permissions.py b/api/integrations/github/permissions.py index ec14952ed275..dfd21201b04e 100644 --- a/api/integrations/github/permissions.py +++ b/api/integrations/github/permissions.py @@ -1,5 +1,7 @@ from rest_framework.permissions import BasePermission +from integrations.github.models import GithubConfiguration + class HasPermissionToGithubConfiguration(BasePermission): """ @@ -10,4 +12,17 @@ class HasPermissionToGithubConfiguration(BasePermission): def has_permission(self, request, view): # type: ignore[no-untyped-def] organisation_id = view.kwargs.get("organisation_pk") - return request.user.belongs_to(organisation_id=int(organisation_id)) + if not request.user.belongs_to(organisation_id=int(organisation_id)): + return False + + # For nested routes, ensure the GitHub configuration in the URL belongs + # to the organisation the caller is authorised for + if (github_pk := view.kwargs.get("github_pk")) is not None and str( + github_pk + ).isdigit(): + return GithubConfiguration.objects.filter( + id=github_pk, + organisation_id=organisation_id, + ).exists() + + return True diff --git a/api/tests/unit/environments/permissions/test_unit_environments_views.py b/api/tests/unit/environments/permissions/test_unit_environments_views.py index b5597750423a..9d25b988b062 100644 --- a/api/tests/unit/environments/permissions/test_unit_environments_views.py +++ b/api/tests/unit/environments/permissions/test_unit_environments_views.py @@ -14,7 +14,7 @@ UserEnvironmentPermission, UserPermissionGroupEnvironmentPermission, ) -from organisations.models import Organisation +from organisations.models import Organisation, OrganisationRole from tests.types import WithEnvironmentPermissionsCallable from users.models import FFAdminUser, UserPermissionGroup @@ -273,3 +273,73 @@ def test_delete_user_group_permission__existing_permission__removes_permission( assert not UserPermissionGroupEnvironmentPermission.objects.filter( id=user_group_environment_permission.id ).exists() + + +def test_list_user_group_permission__different_organisation__returns_403( + environment: Environment, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + other_org_user = FFAdminUser.objects.create(email="other-org-user@example.com") + other_org_user.add_organisation(organisation_two, role=OrganisationRole.ADMIN) + other_org_client = APIClient() + other_org_client.force_authenticate(other_org_user) + + user_permission_group = UserPermissionGroup.objects.create( + name="Test group", + organisation=organisation, + ) + user_group_environment_permission = ( + UserPermissionGroupEnvironmentPermission.objects.create( + group=user_permission_group, + environment=environment, + ) + ) + read_permission = EnvironmentPermissionModel.objects.get(key=VIEW_ENVIRONMENT) + user_group_environment_permission.permissions.set([read_permission]) + + url = f"/api/v1/environments/{environment.api_key}/user-group-permissions/" + + # When + response = other_org_client.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_delete_user_group_permission__different_organisation__returns_403( + environment: Environment, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + other_org_user = FFAdminUser.objects.create(email="other-org-user@example.com") + other_org_user.add_organisation(organisation_two, role=OrganisationRole.ADMIN) + other_org_client = APIClient() + other_org_client.force_authenticate(other_org_user) + + user_permission_group = UserPermissionGroup.objects.create( + name="Test group", + organisation=organisation, + ) + user_group_environment_permission = ( + UserPermissionGroupEnvironmentPermission.objects.create( + group=user_permission_group, + environment=environment, + ) + ) + + url = ( + f"/api/v1/environments/{environment.api_key}" + f"/user-group-permissions/{user_group_environment_permission.id}/" + ) + + # When + response = other_org_client.delete(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + assert UserPermissionGroupEnvironmentPermission.objects.filter( + id=user_group_environment_permission.id + ).exists() diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py index b98f1323155e..3fb923d257b3 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -8,8 +8,12 @@ from django.urls import reverse from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from rest_framework import status +from rest_framework.test import APIClient +from features.models import Feature +from features.multivariate.models import MultivariateFeatureOption from features.multivariate.views import MultivariateFeatureOptionViewSet +from projects.models import Project from projects.permissions import NestedProjectPermissions @@ -72,3 +76,32 @@ def test_get_mv_feature_option_by_uuid__nonexistent_uuid__returns_404( # type: # Then assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_list_mv_options__feature_in_other_project__returns_404( + admin_client: APIClient, + project: Project, + organisation_two_project_one: Project, +) -> None: + # Given + feature = Feature.objects.create( + name="feature", + project=organisation_two_project_one, + type="MULTIVARIATE", + initial_value="control", + ) + for percentage_allocation in (30, 30, 40): + MultivariateFeatureOption.objects.create( + feature=feature, + default_percentage_allocation=percentage_allocation, + type="unicode", + string_value=f"multivariate option for {percentage_allocation}%", + ) + + url = f"/api/v1/projects/{project.id}/features/{feature.id}/mv-options/" + + # When + response = admin_client.get(url) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/api/tests/unit/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index af38dfac4d2c..c00d128e8ba4 100644 --- a/api/tests/unit/features/test_unit_feature_external_resources_views.py +++ b/api/tests/unit/features/test_unit_feature_external_resources_views.py @@ -934,3 +934,31 @@ def test_create_feature_external_resource__duplicate_feature_and_url__returns_40 response.json()["non_field_errors"][0] == "The fields feature, url must make a unique set." ) + + +def test_list_feature_external_resources__feature_in_other_project__returns_404( + admin_client: APIClient, + project: Project, + organisation_two_project_one: Project, +) -> None: + # Given + feature = Feature.objects.create( + name="feature", project=organisation_two_project_one + ) + FeatureExternalResource.objects.create( + url="https://gitlab.com/owner/repo/-/issues/1", + type="GITLAB_ISSUE", + feature=feature, + metadata='{"status": "open"}', + ) + + url = ( + f"/api/v1/projects/{project.id}" + f"/features/{feature.id}/feature-external-resources/" + ) + + # When + response = admin_client.get(url) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/api/tests/unit/integrations/github/test_unit_github_views.py b/api/tests/unit/integrations/github/test_unit_github_views.py index 6f6cf5ead487..80cf3707ab12 100644 --- a/api/tests/unit/integrations/github/test_unit_github_views.py +++ b/api/tests/unit/integrations/github/test_unit_github_views.py @@ -261,6 +261,29 @@ def test_get_github_repository__github_pk_not_a_number__returns_400( # type: ig assert response.json() == {"github_pk": ["Must be an integer"]} +def test_get_github_repository__configuration_in_other_organisation__returns_403( + admin_client_new: APIClient, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + configuration = GithubConfiguration.objects.create( + organisation=organisation_two, + installation_id=7654321, + ) + + url = ( + f"/api/v1/organisations/{organisation.id}" + f"/integrations/github/{configuration.id}/repositories/" + ) + + # When + response = admin_client_new.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + @responses.activate def test_create_github_repository__valid_data__returns_201( admin_client_new: APIClient,