diff --git a/.github/workflows/api-deploy-production-ecs.yml b/.github/workflows/api-deploy-production-ecs.yml index 823b271c9ae5..e033d36069ac 100644 --- a/.github/workflows/api-deploy-production-ecs.yml +++ b/.github/workflows/api-deploy-production-ecs.yml @@ -39,7 +39,7 @@ jobs: flagsmith_saml_revision: v1.1.0 flagsmith_workflows_revision: v1.2.5 flagsmith_auth_controller_revision: v0.0.1 - flagsmith_rbac_revision: v0.3.1 + flagsmith_rbac_revision: v0.4.1 - name: Deploy task processor to Production uses: ./.github/actions/task-processor-deploy-ecs diff --git a/.github/workflows/api-deploy-staging-ecs.yml b/.github/workflows/api-deploy-staging-ecs.yml index 1c77b5d7393d..99dc6b08c65e 100644 --- a/.github/workflows/api-deploy-staging-ecs.yml +++ b/.github/workflows/api-deploy-staging-ecs.yml @@ -39,7 +39,7 @@ jobs: flagsmith_saml_revision: v1.1.0 flagsmith_workflows_revision: v1.2.5 flagsmith_auth_controller_revision: v0.0.1 - flagsmith_rbac_revision: v0.3.1 + flagsmith_rbac_revision: v0.4.1 - name: Deploy task processor to Staging uses: ./.github/actions/task-processor-deploy-ecs diff --git a/.github/workflows/platform-docker-publish-all-features-image.yml b/.github/workflows/platform-docker-publish-all-features-image.yml index 95e1d7da9529..0441968fa225 100644 --- a/.github/workflows/platform-docker-publish-all-features-image.yml +++ b/.github/workflows/platform-docker-publish-all-features-image.yml @@ -7,7 +7,7 @@ on: env: FLAGSMITH_SAML_REVISION: v1.1.0 - FLAGSMITH_RBAC_REVISION: v0.3.1 + FLAGSMITH_RBAC_REVISION: v0.4.1 FLAGSMITH_WORKFLOWS_REVISION: v1.2.5 FLAGSMITH_AUTH_CONTROLLER_REVISION: v0.0.1 diff --git a/api/api_keys/user.py b/api/api_keys/user.py index 33d370ffcb52..5ce5125acb99 100644 --- a/api/api_keys/user.py +++ b/api/api_keys/user.py @@ -44,14 +44,19 @@ def is_project_admin(self, project: "Project") -> bool: def is_environment_admin(self, environment: "Environment") -> bool: return is_master_api_key_environment_admin(self.key, environment) - def has_project_permission(self, permission: str, project: "Project") -> bool: - return project in self.get_permitted_projects(permission) + def has_project_permission( + self, permission: str, project: "Project", tag_ids: typing.List[int] = None + ) -> bool: + return project in self.get_permitted_projects(permission, tag_ids) def has_environment_permission( - self, permission: str, environment: "Environment" + self, + permission: str, + environment: "Environment", + tag_ids: typing.List[int] = None, ) -> bool: return environment in self.get_permitted_environments( - permission, environment.project + permission, environment.project, tag_ids ) def has_organisation_permission( @@ -61,12 +66,16 @@ def has_organisation_permission( self.key, organisation, permission_key ) - def get_permitted_projects(self, permission_key: str) -> QuerySet["Project"]: - return get_permitted_projects_for_master_api_key(self.key, permission_key) + def get_permitted_projects( + self, permission_key: str, tag_ids: typing.List[int] = None + ) -> QuerySet["Project"]: + return get_permitted_projects_for_master_api_key( + self.key, permission_key, tag_ids + ) def get_permitted_environments( - self, permission_key: str, project: "Project" + self, permission_key: str, project: "Project", tag_ids: typing.List[int] = None ) -> QuerySet["Environment"]: return get_permitted_environments_for_master_api_key( - self.key, project, permission_key + self.key, project, permission_key, tag_ids ) diff --git a/api/environments/permissions/constants.py b/api/environments/permissions/constants.py index 1710c8c7a5fe..adfc9aba81df 100644 --- a/api/environments/permissions/constants.py +++ b/api/environments/permissions/constants.py @@ -19,3 +19,5 @@ "Permission to approve change requests in the given environment.", ), ] + +TAG_SUPPORTED_PERMISSIONS = [UPDATE_FEATURE_STATE] diff --git a/api/features/models.py b/api/features/models.py index 3b8c0a350592..6091eded2f86 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -23,6 +23,7 @@ from django_lifecycle import ( AFTER_CREATE, BEFORE_CREATE, + BEFORE_SAVE, LifecycleModelMixin, hook, ) @@ -673,18 +674,21 @@ def get_multivariate_feature_state_value( return getattr(self, "feature_state_value", None) @hook(BEFORE_CREATE) - def check_for_existing_feature_state(self): - # prevent duplicate feature states being created for an environment + @hook(BEFORE_SAVE, when="deleted", is_not=True) + def check_for_duplicate_feature_state(self): if self.version is None: return - - if FeatureState.objects.filter( + filter_ = Q( environment=self.environment, feature=self.feature, version=self.version, feature_segment=self.feature_segment, identity=self.identity, - ).exists(): + ) + if self.id: + filter_ &= ~Q(id=self.id) + + if FeatureState.objects.filter(filter_).exists(): raise ValidationError( "Feature state already exists for this environment, feature, " "version, segment & identity combination" diff --git a/api/features/permissions.py b/api/features/permissions.py index 2d08db72165d..a0a49dbf0667 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -4,12 +4,20 @@ from rest_framework.permissions import IsAuthenticated 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 Feature from projects.models import Project -from projects.permissions import CREATE_FEATURE, DELETE_FEATURE, VIEW_PROJECT +from projects.permissions import CREATE_FEATURE, DELETE_FEATURE +from projects.permissions import ( + TAG_SUPPORTED_PERMISSIONS as TAG_SUPPORTED_PROJECT_PERMISSIONS, +) +from projects.permissions import VIEW_PROJECT ACTION_PERMISSIONS_MAP = { "retrieve": VIEW_PROJECT, @@ -28,6 +36,10 @@ def has_permission(self, request, view): if not super().has_permission(request, view): return False + if view.detail: + # handled by has_object_permission + return True + try: project_id = view.kwargs.get("project_pk") or request.data.get("project") project = Project.objects.get(id=project_id) @@ -46,8 +58,13 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): # map of actions and their required permission if view.action in ACTION_PERMISSIONS_MAP: + tag_ids = [] + required_permission = ACTION_PERMISSIONS_MAP.get(view.action) + if required_permission in TAG_SUPPORTED_PROJECT_PERMISSIONS: + tag_ids = list(obj.tags.values_list("id", flat=True)) + return request.user.has_project_permission( - ACTION_PERMISSIONS_MAP[view.action], obj.project + ACTION_PERMISSIONS_MAP[view.action], obj.project, tag_ids=tag_ids ) if view.action == "segments": @@ -76,17 +93,30 @@ def has_permission(self, request, view): if environment and (isinstance(environment, int) or environment.isdigit()): environment = Environment.objects.get(id=int(environment)) + + 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) + + 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 - except Environment.DoesNotExist: + except (Environment.DoesNotExist, Feature.DoesNotExist): return False def has_object_permission(self, request, view, obj): + tag_ids = None + if UPDATE_FEATURE_STATE in TAG_SUPPORTED_ENVIRONMENT_PERMISSIONS: + tag_ids = list(obj.feature.tags.values_list("id", flat=True)) + return request.user.has_environment_permission( - UPDATE_FEATURE_STATE, environment=obj.environment + UPDATE_FEATURE_STATE, environment=obj.environment, tag_ids=tag_ids ) diff --git a/api/permissions/permission_service.py b/api/permissions/permission_service.py index d925869f2ff4..accfe66739b1 100644 --- a/api/permissions/permission_service.py +++ b/api/permissions/permission_service.py @@ -1,4 +1,4 @@ -from typing import TYPE_CHECKING, Union +from typing import TYPE_CHECKING, List, Union from django.db.models import Q, QuerySet @@ -59,7 +59,7 @@ def is_master_api_key_environment_admin( def get_permitted_projects_for_user( - user: "FFAdminUser", permission_key: str + user: "FFAdminUser", permission_key: str, tag_ids: List[int] = None ) -> QuerySet[Project]: """ Get all projects that the user has the given permissions for. @@ -70,8 +70,15 @@ def get_permitted_projects_for_user( - User is an admin for the organisation the project belongs to - User has a role attached with the required permissions(if rbac is enabled) - User is in a UserPermissionGroup that has a role attached with the required permissions + NOTE: + - If `tag_ids` is None, tags filter will not be applied + - If `tag_ids` is an empty list, only project with no tags will be returned + - If `tag_ids` is a list of tag IDs, only project with one of those tags will + be returned """ - base_filter = get_base_permission_filter(user, Project, permission_key) + base_filter = get_base_permission_filter( + user, Project, permission_key, tag_ids=tag_ids + ) organisation_filter = Q( organisation__userorganisation__user=user, @@ -82,13 +89,13 @@ def get_permitted_projects_for_user( def get_permitted_projects_for_master_api_key( - master_api_key: "MasterAPIKey", permission_key: str + master_api_key: "MasterAPIKey", permission_key: str, tag_ids: List[int] = None ) -> QuerySet[Project]: if master_api_key.is_admin: return Project.objects.filter(organisation_id=master_api_key.organisation_id) return get_permitted_projects_for_master_api_key_using_roles( - master_api_key, permission_key + master_api_key, permission_key, tag_ids ) @@ -96,6 +103,7 @@ def get_permitted_environments_for_user( user: "FFAdminUser", project: Project, permission_key: str, + tag_ids: List[int] = None, ) -> QuerySet[Environment]: """ Get all environments that the user has the given permissions for. @@ -107,12 +115,19 @@ def get_permitted_environments_for_user( - User is an admin for the organisation the environment belongs to - User has a role attached with the required permissions(if rbac is enabled) - User is in a UserPermissionGroup that has a role attached with the required permissions(if rbac is enabled) + NOTE: + - If `tag_ids` is None, tags filter will not be applied + - If `tag_ids` is an empty list, only environments with no tags will be returned + - If `tag_ids` is a list of tag IDs, only environments with one of those tags will + be returned """ if is_user_project_admin(user, project): return project.environments.all() - base_filter = get_base_permission_filter(user, Environment, permission_key) + base_filter = get_base_permission_filter( + user, Environment, permission_key, tag_ids=tag_ids + ) filter_ = base_filter & Q(project=project) return Environment.objects.filter(filter_).distinct().defer("description") @@ -122,12 +137,13 @@ def get_permitted_environments_for_master_api_key( master_api_key: "MasterAPIKey", project: Project, permission_key: str, + tag_ids: List[int] = None, ) -> QuerySet[Environment]: if is_master_api_key_project_admin(master_api_key, project): return project.environments.all() return get_permitted_environments_for_master_api_key_using_roles( - master_api_key, project, permission_key + master_api_key, project, permission_key, tag_ids ) @@ -173,12 +189,13 @@ def get_base_permission_filter( for_model: Union[Organisation, Project, Environment] = None, permission_key: str = None, allow_admin: bool = True, + tag_ids=None, ) -> Q: user_filter = get_user_permission_filter(user, permission_key, allow_admin) group_filter = get_group_permission_filter(user, permission_key, allow_admin) role_filter = get_role_permission_filter( - user, for_model, permission_key, allow_admin + user, for_model, permission_key, allow_admin, tag_ids ) return user_filter | group_filter | role_filter diff --git a/api/permissions/rbac_wrapper.py b/api/permissions/rbac_wrapper.py index c2077c6be032..13faebd842c0 100644 --- a/api/permissions/rbac_wrapper.py +++ b/api/permissions/rbac_wrapper.py @@ -1,4 +1,4 @@ -from typing import Union +from typing import List, Union from django.conf import settings from django.db.models import Q, QuerySet @@ -46,12 +46,14 @@ def is_master_api_key_object_admin( def get_permitted_projects_for_master_api_key_using_roles( - master_api_key: "MasterAPIKey", permission_key: str + master_api_key: "MasterAPIKey", permission_key: str, tag_ids=None ) -> QuerySet[Project]: if not settings.IS_RBAC_INSTALLED: return Project.objects.none() - filter_ = get_role_permission_filter(master_api_key, Project, permission_key) + filter_ = get_role_permission_filter( + master_api_key, Project, permission_key, tag_ids=tag_ids + ) return Project.objects.filter(filter_).distinct() @@ -59,12 +61,13 @@ def get_permitted_environments_for_master_api_key_using_roles( master_api_key: "MasterAPIKey", project: Project, permission_key: str, + tag_ids: List[int] = None, ) -> QuerySet[Environment]: if not settings.IS_RBAC_INSTALLED: return Environment.objects.none() base_filter = get_role_permission_filter( - master_api_key, Environment, permission_key + master_api_key, Environment, permission_key, tag_ids=tag_ids ) filter_ = base_filter & Q(project=project) diff --git a/api/projects/permissions.py b/api/projects/permissions.py index 467e2142dd4e..c0f170f1c139 100644 --- a/api/projects/permissions.py +++ b/api/projects/permissions.py @@ -18,6 +18,8 @@ EDIT_FEATURE = "EDIT_FEATURE" MANAGE_SEGMENTS = "MANAGE_SEGMENTS" +TAG_SUPPORTED_PERMISSIONS = [DELETE_FEATURE] + PROJECT_PERMISSIONS = [ (VIEW_PROJECT, "View permission for the given project."), (CREATE_ENVIRONMENT, "Ability to create an environment in the given project."), diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index a0b34bf256f2..4214dc3434b1 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -955,6 +955,65 @@ def test_create_feature_state_environment_is_read_only( assert response.json()["environment"] == environment.id +@pytest.mark.parametrize( + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], +) +def test_cannot_create_feature_state_of_feature_from_different_project( + client, environment, project_two_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": 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_field_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"))], diff --git a/api/users/models.py b/api/users/models.py index 6b5ab7063df9..54cb8b81f973 100644 --- a/api/users/models.py +++ b/api/users/models.py @@ -246,28 +246,37 @@ def get_user_organisation( % (self.id, getattr(organisation, "id", organisation)) ) - def get_permitted_projects(self, permission_key: str) -> QuerySet[Project]: - return get_permitted_projects_for_user(self, permission_key) + def get_permitted_projects( + self, permission_key: str, tag_ids: typing.List[int] = None + ) -> QuerySet[Project]: + return get_permitted_projects_for_user(self, permission_key, tag_ids) - def has_project_permission(self, permission: str, project: Project) -> bool: + def has_project_permission( + self, permission: str, project: Project, tag_ids: typing.List[int] = None + ) -> bool: if self.is_project_admin(project): return True - return project in self.get_permitted_projects(permission) + return project in self.get_permitted_projects(permission, tag_ids=tag_ids) def has_environment_permission( - self, permission: str, environment: Environment + self, + permission: str, + environment: Environment, + tag_ids: typing.List[int] = None, ) -> bool: return environment in self.get_permitted_environments( - permission, environment.project + permission, environment.project, tag_ids=tag_ids ) def is_project_admin(self, project: Project) -> bool: return is_user_project_admin(self, project) def get_permitted_environments( - self, permission_key: str, project: Project + self, permission_key: str, project: Project, tag_ids: typing.List[int] = None ) -> QuerySet[Environment]: - return get_permitted_environments_for_user(self, project, permission_key) + return get_permitted_environments_for_user( + self, project, permission_key, tag_ids + ) @staticmethod def send_alert_to_admin_users(subject, message):