From a46295b885ecaf2a40a2f626a46c3a46a323f833 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 25 Aug 2023 07:59:39 +0530 Subject: [PATCH] feat(master-api-key/roles): Add roles to master api key (#2436) --- api/api_keys/authentication.py | 23 ++ api/api_keys/middleware.py | 23 -- .../migrations/0003_masterapikey_is_admin.py | 18 ++ api/api_keys/models.py | 1 + api/api_keys/serializers.py | 10 + api/api_keys/user.py | 68 +++++ api/app/settings/common.py | 3 +- api/app/settings/test.py | 21 +- api/conftest.py | 35 ++- api/core/models.py | 9 + api/core/permissions.py | 8 - api/core/signals.py | 5 +- api/environments/permissions/permissions.py | 34 --- api/environments/tests/test_views.py | 36 ++- api/environments/views.py | 14 +- api/features/feature_segments/permissions.py | 36 +++ api/features/feature_segments/serializers.py | 9 + .../feature_segments/tests/test_views.py | 94 +++++- api/features/feature_segments/views.py | 18 +- api/features/multivariate/views.py | 39 +-- api/features/permissions.py | 77 +---- api/features/serializers.py | 2 +- api/features/tests/test_views.py | 9 +- api/features/views.py | 32 +- api/organisations/urls.py | 5 +- api/permissions/permission_service.py | 62 +++- api/permissions/rbac_wrapper.py | 63 +++- api/projects/permissions.py | 59 ---- api/projects/tests/test_views.py | 25 +- api/projects/views.py | 23 +- api/segments/permissions.py | 33 +- api/segments/views.py | 23 +- .../integration/api_keys/test_viewset.py | 95 ++++-- api/tests/integration/conftest.py | 10 +- .../environments/test_clone_environment.py | 3 +- .../test_environment_featurestate_viewset.py | 3 +- .../test_simple_featurestate_viewset.py | 15 +- .../test_integration_multivariate.py | 24 +- .../unit/api_keys/test_authentication.py | 66 ++++ api/tests/unit/api_keys/test_middleware.py | 18 -- api/tests/unit/api_keys/test_user.py | 288 ++++++++++++++++++ api/tests/unit/conftest.py | 36 ++- api/tests/unit/core/test_permissions.py | 20 -- .../test_unit_multivariate_views.py | 13 +- .../unit/features/test_unit_features_views.py | 75 +++-- .../permission_service/conftest.py | 13 - .../test_master_api_key_permission_service.py | 192 ++++++++++++ .../unit/projects/test_unit_projects_views.py | 6 +- .../unit/segments/test_unit_segments_views.py | 53 ++-- api/users/abc.py | 56 ++++ api/users/models.py | 7 + 51 files changed, 1344 insertions(+), 566 deletions(-) create mode 100644 api/api_keys/authentication.py delete mode 100644 api/api_keys/middleware.py create mode 100644 api/api_keys/migrations/0003_masterapikey_is_admin.py create mode 100644 api/api_keys/user.py delete mode 100644 api/core/permissions.py create mode 100644 api/features/feature_segments/permissions.py create mode 100644 api/tests/unit/api_keys/test_authentication.py delete mode 100644 api/tests/unit/api_keys/test_middleware.py create mode 100644 api/tests/unit/api_keys/test_user.py delete mode 100644 api/tests/unit/core/test_permissions.py create mode 100644 api/tests/unit/permissions/permission_service/test_master_api_key_permission_service.py create mode 100644 api/users/abc.py diff --git a/api/api_keys/authentication.py b/api/api_keys/authentication.py new file mode 100644 index 000000000000..1982b7500a29 --- /dev/null +++ b/api/api_keys/authentication.py @@ -0,0 +1,23 @@ +from contextlib import suppress + +from rest_framework import authentication, exceptions +from rest_framework_api_key.permissions import KeyParser + +from api_keys.models import MasterAPIKey +from api_keys.user import APIKeyUser + +key_parser = KeyParser() + + +class MasterAPIKeyAuthentication(authentication.BaseAuthentication): + def authenticate(self, request): + key = key_parser.get(request) + if not key: + return None + + with suppress(MasterAPIKey.DoesNotExist): + key = MasterAPIKey.objects.get_from_key(key) + if not key.has_expired: + return APIKeyUser(key), None + + raise exceptions.AuthenticationFailed("Valid Master API Key not found.") diff --git a/api/api_keys/middleware.py b/api/api_keys/middleware.py deleted file mode 100644 index 83c7712c760c..000000000000 --- a/api/api_keys/middleware.py +++ /dev/null @@ -1,23 +0,0 @@ -from contextlib import suppress - -from django.core.exceptions import ObjectDoesNotExist -from rest_framework_api_key.permissions import KeyParser - -from api_keys.models import MasterAPIKey - -key_parser = KeyParser() - - -class MasterAPIKeyMiddleware: - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - key = key_parser.get(request) - with suppress(ObjectDoesNotExist): - if key: - key = MasterAPIKey.objects.get_from_key(key) - request.master_api_key = key - - response = self.get_response(request) - return response diff --git a/api/api_keys/migrations/0003_masterapikey_is_admin.py b/api/api_keys/migrations/0003_masterapikey_is_admin.py new file mode 100644 index 000000000000..8bd43d00363b --- /dev/null +++ b/api/api_keys/migrations/0003_masterapikey_is_admin.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.20 on 2023-07-14 03:38 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api_keys', '0002_soft_delete_api_keys'), + ] + + operations = [ + migrations.AddField( + model_name='masterapikey', + name='is_admin', + field=models.BooleanField(default=True), + ), + ] diff --git a/api/api_keys/models.py b/api/api_keys/models.py index d0a01a1ffa5f..ec6311ce83f4 100644 --- a/api/api_keys/models.py +++ b/api/api_keys/models.py @@ -17,3 +17,4 @@ class MasterAPIKey(AbstractAPIKey, SoftDeleteObject): ) objects = MasterAPIKeyManager() + is_admin = models.BooleanField(default=True) diff --git a/api/api_keys/serializers.py b/api/api_keys/serializers.py index 692d8f6f0437..e3e14b4ea3d6 100644 --- a/api/api_keys/serializers.py +++ b/api/api_keys/serializers.py @@ -1,3 +1,4 @@ +from django.conf import settings from rest_framework import serializers from .models import MasterAPIKey @@ -9,6 +10,7 @@ class MasterAPIKeySerializer(serializers.ModelSerializer): help_text="Since we don't store the api key itself(i.e: we only store the hash) this key will be none " "for every endpoint apart from create", ) + is_admin = serializers.BooleanField(default=True) class Meta: model = MasterAPIKey @@ -19,6 +21,7 @@ class Meta: "revoked", "expiry_date", "key", + "is_admin", ) read_only_fields = ("prefix", "created", "key") @@ -26,3 +29,10 @@ def create(self, validated_data): obj, key = MasterAPIKey.objects.create_key(**validated_data) obj.key = key return obj + + def validate_is_admin(self, is_admin: bool): + if is_admin is False and not settings.IS_RBAC_INSTALLED: + raise serializers.ValidationError( + "RBAC is not installed, cannot create non-admin key" + ) + return is_admin diff --git a/api/api_keys/user.py b/api/api_keys/user.py new file mode 100644 index 000000000000..2cd32f85c3b9 --- /dev/null +++ b/api/api_keys/user.py @@ -0,0 +1,68 @@ +import typing + +from django.db.models import QuerySet + +from organisations.models import Organisation +from permissions.permission_service import ( + get_permitted_environments_for_master_api_key, + get_permitted_projects_for_master_api_key, + is_master_api_key_environment_admin, + is_master_api_key_project_admin, + master_api_key_has_organisation_permission, +) +from users.abc import UserABC + +from .models import MasterAPIKey + +if typing.TYPE_CHECKING: + from environments.models import Environment + from projects.models import Project + + +class APIKeyUser(UserABC): + def __init__(self, key: MasterAPIKey): + self.key = key + + @property + def is_authenticated(self) -> bool: + return True + + @property + def is_master_api_key_user(self) -> bool: + return True + + def belongs_to(self, organisation_id: int) -> bool: + return self.key.organisation_id == organisation_id + + def is_project_admin(self, project: "Project") -> bool: + return is_master_api_key_project_admin(self.key, project) + + 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_environment_permission( + self, permission: str, environment: "Environment" + ) -> bool: + return environment in self.get_permitted_environments( + permission, environment.project + ) + + def has_organisation_permission( + self, organisation: Organisation, permission_key: str + ) -> bool: + return master_api_key_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_environments( + self, permission_key: str, project: "Project" + ) -> QuerySet["Environment"]: + return get_permitted_environments_for_master_api_key( + self.key, project, permission_key + ) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index 20982d8a604c..3a09a440beba 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -221,6 +221,7 @@ "DEFAULT_PERMISSION_CLASSES": ["rest_framework.permissions.IsAuthenticated"], "DEFAULT_AUTHENTICATION_CLASSES": ( "rest_framework.authentication.TokenAuthentication", + "api_keys.authentication.MasterAPIKeyAuthentication", ), "PAGE_SIZE": 10, "UNICODE_JSON": False, @@ -248,8 +249,6 @@ "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", "simple_history.middleware.HistoryRequestMiddleware", - # Add master api key object to request - "api_keys.middleware.MasterAPIKeyMiddleware", ] ADD_NEVER_CACHE_HEADERS = env.bool("ADD_NEVER_CACHE_HEADERS", True) diff --git a/api/app/settings/test.py b/api/app/settings/test.py index 94d3b3b28ec4..9823dde8aeea 100644 --- a/api/app/settings/test.py +++ b/api/app/settings/test.py @@ -1,21 +1,12 @@ from app.settings.common import * # noqa +from app.settings.common import REST_FRAMEWORK # We dont want to track tests ENABLE_TELEMETRY = False -REST_FRAMEWORK = { - "DEFAULT_PERMISSION_CLASSES": ["rest_framework.permissions.IsAuthenticated"], - "DEFAULT_AUTHENTICATION_CLASSES": ( - "rest_framework.authentication.TokenAuthentication", - ), - "PAGE_SIZE": 10, - "UNICODE_JSON": False, - "DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.PageNumberPagination", - "DEFAULT_THROTTLE_RATES": { - "login": "100/min", - "mfa_code": "5/min", - "invite": "10/min", - "signup": "100/min", - }, - "DEFAULT_FILTER_BACKENDS": ["django_filters.rest_framework.DjangoFilterBackend"], +REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"] = { + "login": "100/min", + "mfa_code": "5/min", + "invite": "10/min", + "signup": "100/min", } diff --git a/api/conftest.py b/api/conftest.py index eb1dff40d5a2..b6605d6e4d63 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -1,4 +1,4 @@ -from typing import Tuple +import typing import pytest from django.contrib.contenttypes.models import ContentType @@ -292,19 +292,44 @@ def environment_api_key(environment): @pytest.fixture() -def master_api_key(organisation) -> Tuple[MasterAPIKey, str]: +def admin_master_api_key(organisation: Organisation) -> typing.Tuple[MasterAPIKey, str]: master_api_key, key = MasterAPIKey.objects.create_key( - name="test_key", organisation=organisation + name="test_key", organisation=organisation, is_admin=True ) return master_api_key, key @pytest.fixture() -def master_api_key_client(master_api_key): +def master_api_key(organisation: Organisation) -> typing.Tuple[MasterAPIKey, str]: + master_api_key, key = MasterAPIKey.objects.create_key( + name="test_key", organisation=organisation, is_admin=False + ) + return master_api_key, key + + +@pytest.fixture +def master_api_key_object( + master_api_key: typing.Tuple[MasterAPIKey, str] +) -> MasterAPIKey: + return master_api_key[0] + + +@pytest.fixture +def admin_master_api_key_object( + admin_master_api_key: typing.Tuple[MasterAPIKey, str] +) -> MasterAPIKey: + return admin_master_api_key[0] + + +@pytest.fixture() +def admin_master_api_key_client( + admin_master_api_key: typing.Tuple[MasterAPIKey, str] +) -> APIClient: + key = admin_master_api_key[1] # 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[1]) + api_client.credentials(HTTP_AUTHORIZATION="Api-Key " + key) return api_client diff --git a/api/core/models.py b/api/core/models.py index b51cb9ad9eba..cb795b2bb4e0 100644 --- a/api/core/models.py +++ b/api/core/models.py @@ -4,6 +4,7 @@ from django.db import models from django.db.models import Manager +from django.http import HttpRequest from simple_history.models import HistoricalRecords from softdelete.models import SoftDeleteManager, SoftDeleteObject @@ -132,6 +133,13 @@ def _get_project(self) -> typing.Optional["Project"]: return None +def get_history_user( + instance: typing.Any, request: HttpRequest +) -> typing.Optional["FFAdminUser"]: + user = getattr(request, "user", None) + return None if getattr(user, "is_master_api_key_user", False) else user + + def abstract_base_auditable_model_factory( historical_records_excluded_fields: typing.List[str] = None, ) -> typing.Type[_AbstractBaseAuditableModel]: @@ -139,6 +147,7 @@ class Base(_AbstractBaseAuditableModel): history = HistoricalRecords( bases=[BaseHistoricalModel], excluded_fields=historical_records_excluded_fields or [], + get_user=get_history_user, inherit=True, ) diff --git a/api/core/permissions.py b/api/core/permissions.py deleted file mode 100644 index f65c45add8e6..000000000000 --- a/api/core/permissions.py +++ /dev/null @@ -1,8 +0,0 @@ -from django.http import HttpRequest -from rest_framework.permissions import BasePermission - - -class HasMasterAPIKey(BasePermission): - def has_permission(self, request: HttpRequest, view: str) -> bool: - master_api_key = getattr(request, "master_api_key", None) - return bool(master_api_key) diff --git a/api/core/signals.py b/api/core/signals.py index d354281889d4..b32d2719c58d 100644 --- a/api/core/signals.py +++ b/api/core/signals.py @@ -43,8 +43,7 @@ def create_audit_log_from_historical_record( def add_master_api_key(sender, **kwargs): try: history_instance = kwargs["history_instance"] - history_instance.master_api_key = ( - HistoricalRecords.thread.request.master_api_key - ) + master_api_key = HistoricalRecords.thread.request.user.key + history_instance.master_api_key = master_api_key except (KeyError, AttributeError): pass diff --git a/api/environments/permissions/permissions.py b/api/environments/permissions/permissions.py index a42c57d8e007..dc033d7249f1 100644 --- a/api/environments/permissions/permissions.py +++ b/api/environments/permissions/permissions.py @@ -1,7 +1,6 @@ import typing from django.db.models import Model, Q -from django.http import HttpRequest from rest_framework import exceptions from rest_framework.permissions import BasePermission, IsAuthenticated @@ -44,9 +43,6 @@ def has_permission(self, request, view): return True def has_object_permission(self, request, view, obj): - if request.user.is_anonymous: - return False - if view.action == "clone": return request.user.has_project_permission(CREATE_ENVIRONMENT, obj.project) @@ -55,36 +51,6 @@ def has_object_permission(self, request, view, obj): ] -class MasterAPIKeyEnvironmentPermissions(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 - - if view.action == "create": - try: - project_id = request.data.get("project") - project = Project.objects.get(id=project_id) - return master_api_key.organisation_id == project.organisation.id - - except Project.DoesNotExist: - return False - - # return true as list will be handled by view and obj permissions will be handled later - return True - - def has_object_permission( - self, request: HttpRequest, view: str, obj: Model - ) -> bool: - master_api_key = getattr(request, "master_api_key", None) - - if not master_api_key: - return False - - return master_api_key.organisation_id == obj.project.organisation_id - - class IdentityPermissions(BasePermission): def has_permission(self, request, view): try: diff --git a/api/environments/tests/test_views.py b/api/environments/tests/test_views.py index 237ed607368a..238f74d84259 100644 --- a/api/environments/tests/test_views.py +++ b/api/environments/tests/test_views.py @@ -501,14 +501,14 @@ def test_delete_api_key(self): @pytest.mark.parametrize( - "client, is_master_api_key_client", + "client, is_admin_master_api_key_client", [ - (lazy_fixture("master_api_key_client"), True), + (lazy_fixture("admin_master_api_key_client"), True), (lazy_fixture("admin_client"), False), ], ) def test_should_create_environments( - project, client, admin_user, is_master_api_key_client + project, client, admin_user, is_admin_master_api_key_client ): # Given url = reverse("api-v1:environments:environment-list") @@ -529,14 +529,15 @@ def test_should_create_environments( assert response.json()["use_identity_composite_key_for_hashing"] is True # and user is admin - if not is_master_api_key_client: + if not is_admin_master_api_key_client: assert UserEnvironmentPermission.objects.filter( user=admin_user, admin=True, environment__id=response.json()["id"] ).exists() @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_environment_without_required_metadata_returns_400( project, @@ -562,7 +563,8 @@ def test_create_environment_without_required_metadata_returns_400( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_environment_with_required_metadata_returns_201( project, @@ -599,7 +601,8 @@ def test_create_environment_with_required_metadata_returns_201( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_environment_metadata( project, @@ -643,7 +646,8 @@ def test_update_environment_metadata( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_log_entry_created_when_environment_updated(environment, project, client): # Given @@ -687,7 +691,8 @@ def test_audit_log_entry_created_when_environment_updated(environment, project, @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_document(environment, project, client, feature, segment): # Given @@ -714,7 +719,8 @@ def test_get_document(environment, project, client, feature, segment): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_all_trait_keys_for_environment_only_returns_distinct_keys( identity, client, trait, environment @@ -755,7 +761,8 @@ def test_get_all_trait_keys_for_environment_only_returns_distinct_keys( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_delete_trait_keys_deletes_traits_matching_provided_key_only( environment, client, identity, trait @@ -787,7 +794,8 @@ def test_delete_trait_keys_deletes_traits_matching_provided_key_only( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_user_can_list_environment_permission(client, environment): # Given @@ -804,7 +812,7 @@ def test_user_can_list_environment_permission(client, environment): def test_environment_my_permissions_reruns_400_for_master_api_key( - master_api_key_client, environment + admin_master_api_key_client, environment ): # Given url = reverse( @@ -812,7 +820,7 @@ def test_environment_my_permissions_reruns_400_for_master_api_key( ) # When - response = master_api_key_client.get(url) + response = admin_master_api_key_client.get(url) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/api/environments/views.py b/api/environments/views.py index e23209558273..c4fec4cd322f 100644 --- a/api/environments/views.py +++ b/api/environments/views.py @@ -16,7 +16,6 @@ from environments.permissions.permissions import ( EnvironmentAdminPermission, EnvironmentPermissions, - MasterAPIKeyEnvironmentPermissions, NestedEnvironmentPermissions, ) from permissions.permissions_calculator import get_environment_permission_data @@ -66,7 +65,7 @@ ) class EnvironmentViewSet(viewsets.ModelViewSet): lookup_field = "api_key" - permission_classes = [EnvironmentPermissions | MasterAPIKeyEnvironmentPermissions] + permission_classes = [EnvironmentPermissions] def get_serializer_class(self): if self.action == "trait_keys": @@ -98,11 +97,6 @@ def get_queryset(self): except Project.DoesNotExist: raise ValidationError("Invalid or missing value for project parameter.") - if self.request.user.is_anonymous: - return ( - self.request.master_api_key.organisation.projects.environments.all() - ) - return self.request.user.get_permitted_environments( "VIEW_ENVIRONMENT", project=project ) @@ -119,7 +113,7 @@ def get_queryset(self): def perform_create(self, serializer): environment = serializer.save() - if not self.request.user.is_anonymous: + if getattr(self.request.user, "is_master_api_key_user", False) is False: UserEnvironmentPermission.objects.create( user=self.request.user, environment=environment, admin=True ) @@ -153,7 +147,7 @@ def clone(self, request, *args, **kwargs): serializer.is_valid(raise_exception=True) clone = serializer.save(source_env=self.get_object()) - if not self.request.user.is_anonymous: + if getattr(request.user, "is_master_api_key_user", False) is False: UserEnvironmentPermission.objects.create( user=self.request.user, environment=clone, admin=True ) @@ -189,7 +183,7 @@ def permissions(self, *args, **kwargs): url_name="my-permissions", ) def user_permissions(self, request, *args, **kwargs): - if request.user.is_anonymous: + if getattr(request.user, "is_master_api_key_user", False) is True: return Response( status=status.HTTP_400_BAD_REQUEST, data={ diff --git a/api/features/feature_segments/permissions.py b/api/features/feature_segments/permissions.py new file mode 100644 index 000000000000..6a07f66eb407 --- /dev/null +++ b/api/features/feature_segments/permissions.py @@ -0,0 +1,36 @@ +from contextlib import suppress + +from rest_framework.permissions import IsAuthenticated + +from environments.models import Environment +from environments.permissions.constants import UPDATE_FEATURE_STATE + + +class FeatureSegmentPermissions(IsAuthenticated): + def has_permission(self, request, view): + if not super().has_permission(request, view): + return False + + # detail view means we can just defer to object permissions + if view.detail: + return True + + # handle by the view + if view.action in ["list", "get_by_uuid", "update_priorities"]: + return True + + if view.action == "create": + with suppress(Environment.DoesNotExist, ValueError): + environment = request.data.get("environment") + environment = Environment.objects.get(id=int(environment)) + + return request.user.has_environment_permission( + UPDATE_FEATURE_STATE, environment + ) + + return False + + def has_object_permission(self, request, view, obj): + return request.user.has_environment_permission( + UPDATE_FEATURE_STATE, environment=obj.environment + ) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 64189e05696c..7a2e89d823bd 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -1,5 +1,7 @@ from rest_framework import serializers +from rest_framework.exceptions import PermissionDenied +from environments.permissions.constants import UPDATE_FEATURE_STATE from features.models import FeatureSegment @@ -110,6 +112,13 @@ def validate(self, attrs): "All feature segments must belong to the same feature & environment." ) + environment = environments.pop() + + if not self.context["request"].user.has_environment_permission( + UPDATE_FEATURE_STATE, environment + ): + raise PermissionDenied("You do not have permission to perform this action.") + return validated_attrs def create(self, validated_data): diff --git a/api/features/feature_segments/tests/test_views.py b/api/features/feature_segments/tests/test_views.py index 314371815740..65f5b51407ef 100644 --- a/api/features/feature_segments/tests/test_views.py +++ b/api/features/feature_segments/tests/test_views.py @@ -14,7 +14,10 @@ "client, num_queries", [ (lazy_fixture("admin_client"), 2), # 1 for paging, 1 for result - (lazy_fixture("master_api_key_client"), 3), # an extra one for master_api_key + ( + lazy_fixture("admin_master_api_key_client"), + 3, + ), # an extra one for master_api_key ], ) def test_list_feature_segments( @@ -63,7 +66,7 @@ def test_list_feature_segments( @pytest.mark.parametrize( "client", - [lazy_fixture("admin_client"), lazy_fixture("master_api_key_client")], + [lazy_fixture("admin_client"), lazy_fixture("admin_master_api_key_client")], ) def test_list_feature_segments_is_feature_specific( segment, @@ -94,7 +97,8 @@ def test_list_feature_segments_is_feature_specific( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_feature_segment(segment, feature, environment, client): # Given @@ -114,8 +118,29 @@ def test_create_feature_segment(segment, feature, environment, client): assert response_json["id"] +def test_create_feature_segment_without_permission_returns_403( + segment, feature, environment, test_user_client +): + # Given + data = { + "feature": feature.id, + "segment": segment.id, + "environment": environment.id, + } + url = reverse("api-v1:features:feature-segment-list") + + # When + response = test_user_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("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_delete_feature_segment(segment, feature, environment, client): # Given @@ -133,9 +158,10 @@ def test_delete_feature_segment(segment, feature, environment, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) -def test_priority_of_multiple_feature_segments( +def test_update_priority_of_multiple_feature_segments( feature_segment, project, client, @@ -171,8 +197,32 @@ def test_priority_of_multiple_feature_segments( assert json_response[1]["id"] == another_feature_segment.id +def test_update_priority_returns_403_if_user_does_not_have_permission( + feature_segment, + project, + environment, + feature, + test_user_client, +): + # Given + url = reverse("api-v1:features:feature-segment-update-priorities") + + data = [ + {"id": feature_segment.id, "priority": 1}, + ] + + # When + response = test_user_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("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_priorities_empty_list(client): # Given @@ -187,7 +237,8 @@ def test_update_priorities_empty_list(client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_feature_segment_by_uuid( feature_segment, project, client, environment, feature @@ -207,8 +258,24 @@ def test_get_feature_segment_by_uuid( 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 +): + # Given + url = reverse( + "api-v1:features:feature-segment-get-by-uuid", args=[feature_segment.uuid] + ) + + # When + response = test_user_client.get(url) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND + + @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_feature_segment_by_id( feature_segment, project, client, environment, feature @@ -227,7 +294,8 @@ def test_get_feature_segment_by_id( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_creating_segment_override_for_feature_based_segment_returns_400_for_wrong_feature( client, feature_based_segment, project, environment @@ -254,7 +322,8 @@ def test_creating_segment_override_for_feature_based_segment_returns_400_for_wro @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_creating_segment_override_for_feature_based_segment_returns_201_for_correct_feature( client, feature_based_segment, project, environment, feature @@ -274,7 +343,8 @@ def test_creating_segment_override_for_feature_based_segment_returns_201_for_cor @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_creating_segment_override_reaching_max_limit( client, segment, environment, project, feature, feature_based_segment diff --git a/api/features/feature_segments/views.py b/api/features/feature_segments/views.py index f7f672c248f4..8f2e8f09d991 100644 --- a/api/features/feature_segments/views.py +++ b/api/features/feature_segments/views.py @@ -1,12 +1,10 @@ import logging -from core.permissions import HasMasterAPIKey from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema from rest_framework import viewsets from rest_framework.decorators import action from rest_framework.generics import get_object_or_404 -from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from features.feature_segments.serializers import ( @@ -16,9 +14,10 @@ FeatureSegmentQuerySerializer, ) from features.models import FeatureSegment -from projects.models import Project from projects.permissions import VIEW_PROJECT +from .permissions import FeatureSegmentPermissions + logger = logging.getLogger(__name__) @@ -35,20 +34,15 @@ class FeatureSegmentViewSet( viewsets.ModelViewSet, ): - permission_classes = [IsAuthenticated | HasMasterAPIKey] + permission_classes = [FeatureSegmentPermissions] def get_queryset(self): if getattr(self, "swagger_fake_view", False): return FeatureSegment.objects.none() - if hasattr(self.request, "master_api_key"): - permitted_projects = Project.objects.filter( - organisation_id=self.request.master_api_key.organisation_id - ) - else: - permitted_projects = self.request.user.get_permitted_projects( - permission_key=VIEW_PROJECT - ) + 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/multivariate/views.py b/api/features/multivariate/views.py index 0bb3aaba1dde..80a2d8c06037 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -1,16 +1,13 @@ -from core.permissions import HasMasterAPIKey from drf_yasg.utils import swagger_auto_schema from rest_framework import viewsets -from rest_framework.decorators import api_view, permission_classes +from rest_framework.decorators import api_view from rest_framework.generics import get_object_or_404 -from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from features.models import Feature from projects.permissions import ( CREATE_FEATURE, VIEW_PROJECT, - NestedProjectMasterAPIKeyPermissions, NestedProjectPermissions, ) @@ -19,27 +16,21 @@ class MultivariateFeatureOptionViewSet(viewsets.ModelViewSet): - permission_classes = [ - NestedProjectPermissions | NestedProjectMasterAPIKeyPermissions - ] serializer_class = MultivariateFeatureOptionSerializer def get_permissions(self): - action_permission_map = { - "list": VIEW_PROJECT, - "detail": VIEW_PROJECT, - "create": CREATE_FEATURE, - "update": CREATE_FEATURE, - "partial_update": CREATE_FEATURE, - "destroy": CREATE_FEATURE, - } - - permission_kwargs = { - "action_permission_map": action_permission_map, - "get_project_from_object_callable": lambda o: o.feature.project, - } return [ - permission(**permission_kwargs) for permission in self.permission_classes + NestedProjectPermissions( + action_permission_map={ + "list": VIEW_PROJECT, + "detail": VIEW_PROJECT, + "create": CREATE_FEATURE, + "update": CREATE_FEATURE, + "partial_update": CREATE_FEATURE, + "destroy": CREATE_FEATURE, + }, + get_project_from_object_callable=lambda o: o.feature.project, + ) ] def get_queryset(self): @@ -54,12 +45,8 @@ def get_queryset(self): responses={200: MultivariateFeatureOptionSerializer()}, method="get" ) @api_view(["GET"]) -@permission_classes([IsAuthenticated | HasMasterAPIKey]) def get_mv_feature_option_by_uuid(request, uuid): - if getattr(request, "master_api_key", None): - accessible_projects = request.master_api_key.organisation.projects.all() - else: - accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) + accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) qs = MultivariateFeatureOption.objects.filter( feature__project__in=accessible_projects ) diff --git a/api/features/permissions.py b/api/features/permissions.py index f4acfe9acee7..2d08db72165d 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -1,15 +1,13 @@ from contextlib import suppress -from django.http import HttpRequest from django.shortcuts import get_object_or_404 -from rest_framework.permissions import BasePermission, IsAuthenticated +from rest_framework.permissions import IsAuthenticated from environments.models import Environment from environments.permissions.constants import ( UPDATE_FEATURE_STATE, VIEW_ENVIRONMENT, ) -from features.models import Feature, FeatureState from projects.models import Project from projects.permissions import CREATE_FEATURE, DELETE_FEATURE, VIEW_PROJECT @@ -46,9 +44,6 @@ def has_permission(self, request, view): return False def has_object_permission(self, request, view, obj): - if request.user.is_anonymous: - return False - # map of actions and their required permission if view.action in ACTION_PERMISSIONS_MAP: return request.user.has_project_permission( @@ -61,24 +56,6 @@ def has_object_permission(self, request, view, obj): return False -class MasterAPIKeyFeaturePermissions(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 - with suppress(Project.DoesNotExist): - project_id = view.kwargs.get("project_pk") or request.data.get("project") - project = Project.objects.get(id=project_id) - - return project.organisation_id == master_api_key.organisation_id - return False - - def has_object_permission( - self, request: HttpRequest, view: str, obj: Feature - ) -> bool: - return self.has_permission(request, view) - - class FeatureStatePermissions(IsAuthenticated): def has_permission(self, request, view): action_permission_map = { @@ -108,60 +85,11 @@ def has_permission(self, request, view): return False def has_object_permission(self, request, view, obj): - if request.user.is_anonymous: - return False - return request.user.has_environment_permission( UPDATE_FEATURE_STATE, environment=obj.environment ) -class MasterAPIKeyFeatureStatePermissions(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 = request.data.get("environment") or request.query_params.get( - "environment" - ) - if environment and (isinstance(environment, int) or environment.isdigit()): - with suppress(Environment.DoesNotExist): - environment = Environment.objects.get(id=int(environment)) - 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 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): action_permission_map = { @@ -185,9 +113,6 @@ def has_permission(self, request, view): return False def has_object_permission(self, request, view, obj): - if request.user.is_anonymous: - return False - action_permission_map = {"retrieve": VIEW_ENVIRONMENT} return request.user.has_environment_permission( diff --git a/api/features/serializers.py b/api/features/serializers.py index 191c4981fa47..4d105f2c6429 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -129,7 +129,7 @@ def create(self, validated_data: dict) -> Feature: # NOTE: pop the user before passing the data to create user = validated_data.pop("user", None) instance = super(ListCreateFeatureSerializer, self).create(validated_data) - if user and not user.is_anonymous: + if user and getattr(user, "is_master_api_key_user", False) is False: instance.owners.add(user) return instance diff --git a/api/features/tests/test_views.py b/api/features/tests/test_views.py index c09dd80999a3..062104ea32e2 100644 --- a/api/features/tests/test_views.py +++ b/api/features/tests/test_views.py @@ -717,7 +717,8 @@ def test_get_flags__server_key_only_feature__server_key_auth__return_expected( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_get_feature_states_by_uuid(client, environment, feature, feature_state): # Given @@ -736,7 +737,8 @@ def test_get_feature_states_by_uuid(client, environment, feature, feature_state) @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_deleted_features_are_not_listed(client, project, environment, feature): # Given @@ -752,7 +754,8 @@ def test_deleted_features_are_not_listed(client, project, environment, feature): @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_get_feature_evaluation_data(project, feature, environment, mocker, client): # Given diff --git a/api/features/views.py b/api/features/views.py index 2c52caaebb27..94e0be84a8d0 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -5,7 +5,6 @@ from app_analytics.analytics_db_service import get_feature_evaluation_data from app_analytics.influxdb_wrapper import get_multiple_event_list_for_feature from core.constants import FLAGSMITH_UPDATED_AT_HEADER -from core.permissions import HasMasterAPIKey from core.request_origin import RequestOrigin from django.conf import settings from django.core.cache import caches @@ -45,9 +44,6 @@ FeaturePermissions, FeatureStatePermissions, IdentityFeatureStatePermissions, - MasterAPIKeyEnvironmentFeatureStatePermissions, - MasterAPIKeyFeaturePermissions, - MasterAPIKeyFeatureStatePermissions, ) from .serializers import ( CreateSegmentOverrideFeatureStateSerializer, @@ -79,12 +75,8 @@ @swagger_auto_schema(responses={200: ListCreateFeatureSerializer()}, method="get") @api_view(["GET"]) -@permission_classes([IsAuthenticated | HasMasterAPIKey]) def get_feature_by_uuid(request, uuid): - if getattr(request, "master_api_key", None): - accessible_projects = request.master_api_key.organisation.projects.all() - else: - accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) + accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) qs = Feature.objects.filter(project__in=accessible_projects).prefetch_related( "multivariate_options", "owners", "tags" ) @@ -98,7 +90,7 @@ def get_feature_by_uuid(request, uuid): decorator=swagger_auto_schema(query_serializer=FeatureQuerySerializer()), ) class FeatureViewSet(viewsets.ModelViewSet): - permission_classes = [FeaturePermissions | MasterAPIKeyFeaturePermissions] + permission_classes = [FeaturePermissions] pagination_class = CustomPagination def get_serializer_class(self): @@ -114,12 +106,7 @@ def get_queryset(self): if getattr(self, "swagger_fake_view", False): return Feature.objects.none() - if self.request.user.is_anonymous: - accessible_projects = ( - self.request.master_api_key.organisation.projects.all() - ) - else: - accessible_projects = self.request.user.get_permitted_projects(VIEW_PROJECT) + accessible_projects = self.request.user.get_permitted_projects(VIEW_PROJECT) project = get_object_or_404(accessible_projects, pk=self.kwargs["project_pk"]) queryset = project.features.all().prefetch_related( @@ -478,10 +465,7 @@ def update_feature_state_value(self, value, feature_state): class EnvironmentFeatureStateViewSet(BaseFeatureStateViewSet): - permission_classes = [ - EnvironmentFeatureStatePermissions - | MasterAPIKeyEnvironmentFeatureStatePermissions - ] + permission_classes = [EnvironmentFeatureStatePermissions] def get_queryset(self): queryset = super().get_queryset().filter(feature_segment=None) @@ -530,7 +514,7 @@ class SimpleFeatureStateViewSet( viewsets.GenericViewSet, ): serializer_class = WritableNestedFeatureStateSerializer - permission_classes = [FeatureStatePermissions | MasterAPIKeyFeatureStatePermissions] + permission_classes = [FeatureStatePermissions] filterset_fields = ["environment", "feature", "feature_segment"] def get_queryset(self): @@ -556,12 +540,8 @@ def get_queryset(self): responses={200: WritableNestedFeatureStateSerializer()}, method="get" ) @api_view(["GET"]) -@permission_classes([IsAuthenticated | HasMasterAPIKey]) def get_feature_state_by_uuid(request, uuid): - if getattr(request, "master_api_key", None): - accessible_projects = request.master_api_key.organisation.projects.all() - else: - accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) + accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) qs = FeatureState.objects.filter( feature__project__in=accessible_projects ).select_related("feature_state_value") diff --git a/api/organisations/urls.py b/api/organisations/urls.py index e1a7098a0e90..c32d9d2ad304 100644 --- a/api/organisations/urls.py +++ b/api/organisations/urls.py @@ -98,6 +98,7 @@ if settings.IS_RBAC_INSTALLED: from rbac.views import ( GroupRoleViewSet, + MasterAPIKeyRoleViewSet, RoleEnvironmentPermissionsViewSet, RoleOrganisationPermissionViewSet, RoleProjectPermissionsViewSet, @@ -126,7 +127,9 @@ ) nested_roles_router.register("groups", GroupRoleViewSet, basename="group-roles") nested_roles_router.register("users", UserRoleViewSet, basename="user-roles") - + nested_roles_router.register( + "master-api-keys", MasterAPIKeyRoleViewSet, basename="master-api-key-roles" + ) urlpatterns.extend( [ url(r"^", include(organisations_router.urls)), diff --git a/api/permissions/permission_service.py b/api/permissions/permission_service.py index 79a923b7c466..d925869f2ff4 100644 --- a/api/permissions/permission_service.py +++ b/api/permissions/permission_service.py @@ -6,9 +6,16 @@ from organisations.models import Organisation, OrganisationRole from projects.models import Project -from .rbac_wrapper import get_role_permission_filter +from .rbac_wrapper import ( + get_permitted_environments_for_master_api_key_using_roles, + get_permitted_projects_for_master_api_key_using_roles, + get_role_permission_filter, + is_master_api_key_object_admin, + master_api_key_has_organisation_permission_using_roles, +) if TYPE_CHECKING: + from api_keys.models import MasterAPIKey from users.models import FFAdminUser @@ -33,6 +40,24 @@ def is_user_environment_admin(user: "FFAdminUser", environment: Environment) -> ) +def is_master_api_key_project_admin( + master_api_key: "MasterAPIKey", project: Project +) -> bool: + if master_api_key.is_admin: + return master_api_key.organisation_id == project.organisation_id + return is_master_api_key_object_admin(master_api_key, project) + + +def is_master_api_key_environment_admin( + master_api_key: "MasterAPIKey", environment: Environment +) -> bool: + if master_api_key.is_admin: + return master_api_key.organisation_id == environment.project.organisation_id + return is_master_api_key_project_admin( + master_api_key, environment.project + ) or is_master_api_key_object_admin(master_api_key, environment) + + def get_permitted_projects_for_user( user: "FFAdminUser", permission_key: str ) -> QuerySet[Project]: @@ -56,6 +81,17 @@ def get_permitted_projects_for_user( return Project.objects.filter(filter_).distinct() +def get_permitted_projects_for_master_api_key( + master_api_key: "MasterAPIKey", permission_key: str +) -> 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 + ) + + def get_permitted_environments_for_user( user: "FFAdminUser", project: Project, @@ -82,6 +118,19 @@ def get_permitted_environments_for_user( return Environment.objects.filter(filter_).distinct().defer("description") +def get_permitted_environments_for_master_api_key( + master_api_key: "MasterAPIKey", + project: Project, + permission_key: str, +) -> 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 + ) + + def user_has_organisation_permission( user: "FFAdminUser", organisation: Organisation, permission_key: str ) -> bool: @@ -99,6 +148,17 @@ def user_has_organisation_permission( return Organisation.objects.filter(filter_).exists() +def master_api_key_has_organisation_permission( + master_api_key: "MasterAPIKey", organisation: Organisation, permission_key: str +) -> bool: + if master_api_key.is_admin: + return master_api_key.organisation == organisation + + return master_api_key_has_organisation_permission_using_roles( + master_api_key, organisation, permission_key + ) + + def _is_user_object_admin( user: "FFAdminUser", object_: Union[Project, Environment] ) -> bool: diff --git a/api/permissions/rbac_wrapper.py b/api/permissions/rbac_wrapper.py index b24627043ef4..c2077c6be032 100644 --- a/api/permissions/rbac_wrapper.py +++ b/api/permissions/rbac_wrapper.py @@ -1,5 +1,12 @@ +from typing import Union + from django.conf import settings -from django.db.models import Q +from django.db.models import Q, QuerySet + +from api_keys.models import MasterAPIKey +from environments.models import Environment +from organisations.models import Organisation +from projects.models import Project if settings.IS_RBAC_INSTALLED: from rbac.permission_service import get_role_permission_filter @@ -23,3 +30,57 @@ def get_roles_permission_data_for_environment(*args, **kwargs): def get_role_permission_filter(*args, **kwargs) -> Q: return Q() + + +def is_master_api_key_object_admin( + master_api_key: "MasterAPIKey", object_: Union[Project, Environment] +) -> bool: + if not settings.IS_RBAC_INSTALLED: + return False + + ModelClass = type(object_) + + base_filter = get_role_permission_filter(master_api_key, ModelClass) + filter_ = base_filter & Q(id=object_.id) + return ModelClass.objects.filter(filter_).exists() + + +def get_permitted_projects_for_master_api_key_using_roles( + master_api_key: "MasterAPIKey", permission_key: str +) -> QuerySet[Project]: + if not settings.IS_RBAC_INSTALLED: + return Project.objects.none() + + filter_ = get_role_permission_filter(master_api_key, Project, permission_key) + return Project.objects.filter(filter_).distinct() + + +def get_permitted_environments_for_master_api_key_using_roles( + master_api_key: "MasterAPIKey", + project: Project, + permission_key: str, +) -> QuerySet[Environment]: + if not settings.IS_RBAC_INSTALLED: + return Environment.objects.none() + + base_filter = get_role_permission_filter( + master_api_key, Environment, permission_key + ) + + filter_ = base_filter & Q(project=project) + + return Environment.objects.filter(filter_).distinct().defer("description") + + +def master_api_key_has_organisation_permission_using_roles( + master_api_key: "MasterAPIKey", organisation: Organisation, permission_key: str +) -> bool: + if not settings.IS_RBAC_INSTALLED: + return False + + base_filter = get_role_permission_filter( + master_api_key, Organisation, permission_key + ) + filter_ = base_filter & Q(id=organisation.id) + + return Organisation.objects.filter(filter_).exists() diff --git a/api/projects/permissions.py b/api/projects/permissions.py index 5e3fc6452cad..467e2142dd4e 100644 --- a/api/projects/permissions.py +++ b/api/projects/permissions.py @@ -1,8 +1,6 @@ import typing -from contextlib import suppress from django.db.models import Model -from django.http import HttpRequest from rest_framework.exceptions import APIException, PermissionDenied from rest_framework.permissions import BasePermission, IsAuthenticated @@ -56,9 +54,6 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): """Check if user has permission to view / edit / delete project""" - if request.user.is_anonymous: - return False - if request.user.is_project_admin(obj): return True @@ -76,30 +71,6 @@ def has_object_permission(self, request, view, obj): return False -class MasterAPIKeyProjectPermissions(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 - - if view.action == "create": - organisation_id = int(request.data.get("organisation")) - return organisation_id == master_api_key.organisation_id - - if view.action in ("list", "permissions", "get_by_uuid"): - return True - - # move on to object specific permissions - return view.detail - - def has_object_permission( - self, request: HttpRequest, view: str, obj: Project - ) -> bool: - master_api_key = request.master_api_key - return master_api_key.organisation_id == obj.organisation_id - - class IsProjectAdmin(BasePermission): def __init__( self, @@ -169,9 +140,6 @@ def has_permission(self, request, view): return view.detail def has_object_permission(self, request, view, obj): - if request.user.is_anonymous: - return False - if view.action in self.action_permission_map: return request.user.has_project_permission( self.action_permission_map[view.action], @@ -179,30 +147,3 @@ def has_object_permission(self, request, view, obj): ) return request.user.is_project_admin(self.get_project_from_object_callable(obj)) - - -class NestedProjectMasterAPIKeyPermissions(BasePermission): - # NOTE: In order to compose different permissions using Python bitwise operators - # all the permissions must have same __init__ signature. The __init__ method defined below - # provides the same signature as the one defined in the NestedProjectPermissions class - # to support composition using bitwise operators. - def __init__(self, *args, **kwargs): - super().__init__() - - 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 - - with suppress(Project.DoesNotExist): - project_id = view.kwargs.get("project_pk") - project = Project.objects.get(id=project_id) - - return project.organisation_id == master_api_key.organisation_id - return False - - def has_object_permission( - self, request: HttpRequest, view: str, obj: Model - ) -> bool: - return self.has_permission(request, view) diff --git a/api/projects/tests/test_views.py b/api/projects/tests/test_views.py index 09db0ffe6bb2..34dd87868ab9 100644 --- a/api/projects/tests/test_views.py +++ b/api/projects/tests/test_views.py @@ -65,8 +65,8 @@ def test_should_create_a_project(settings, admin_user, admin_client, organisatio assert get_project_response.status_code == status.HTTP_200_OK -def test_should_create_a_project_with_master_api_key_client( - settings, organisation, master_api_key_client +def test_should_create_a_project_with_admin_master_api_key_client( + settings, organisation, admin_master_api_key_client ): # Given project_name = "project1" @@ -75,7 +75,7 @@ def test_should_create_a_project_with_master_api_key_client( url = reverse("api-v1:projects:project-list") # When - response = master_api_key_client.post(url, data=data) + response = admin_master_api_key_client.post(url, data=data) # Then assert response.status_code == status.HTTP_201_CREATED @@ -88,7 +88,8 @@ def test_should_create_a_project_with_master_api_key_client( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_can_update_project(client, project, organisation): # Given @@ -105,7 +106,8 @@ def test_can_update_project(client, project, organisation): @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_can_list_project_permission(client, project): # Given @@ -122,13 +124,13 @@ def test_can_list_project_permission(client, project): def test_my_permissions_for_a_project_return_400_with_master_api_key( - master_api_key_client, project, organisation + admin_master_api_key_client, project, organisation ): # Given url = reverse("api-v1:projects:project-my-permissions", args=[project.id]) # When - response = master_api_key_client.get(url) + response = admin_master_api_key_client.get(url) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -637,7 +639,8 @@ def test_list_project_with_uuid_filter_returns_correct_project( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_get_project_by_uuid(client, project, mocker, settings, organisation): # Given @@ -652,7 +655,8 @@ def test_get_project_by_uuid(client, project, mocker, settings, organisation): @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_can_enable_realtime_updates_for_project( client, project, mocker, settings, organisation @@ -676,7 +680,8 @@ def test_can_enable_realtime_updates_for_project( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_update_project(client, project, mocker, settings, organisation): # Given diff --git a/api/projects/views.py b/api/projects/views.py index 0de363563c67..a7d8d7ae2790 100644 --- a/api/projects/views.py +++ b/api/projects/views.py @@ -36,7 +36,6 @@ from projects.permissions import ( VIEW_PROJECT, IsProjectAdmin, - MasterAPIKeyProjectPermissions, ProjectPermissions, ) from projects.serializers import ( @@ -71,24 +70,20 @@ ), ) class ProjectViewSet(viewsets.ModelViewSet): + permission_classes = [ProjectPermissions] + def get_serializer_class(self): if self.action == "retrieve": return ProjectRetrieveSerializer return ProjectListSerializer - permission_classes = [ProjectPermissions | MasterAPIKeyProjectPermissions] pagination_class = None def get_queryset(self): if getattr(self, "swagger_fake_view", False): return Project.objects.none() - if hasattr(self.request, "master_api_key"): - queryset = self.request.master_api_key.organisation.projects.all() - else: - queryset = self.request.user.get_permitted_projects( - permission_key=VIEW_PROJECT - ) + queryset = self.request.user.get_permitted_projects(permission_key=VIEW_PROJECT) organisation_id = self.request.query_params.get("organisation") if organisation_id: @@ -102,12 +97,10 @@ def get_queryset(self): def perform_create(self, serializer): project = serializer.save() - if self.request.user.is_anonymous: - return - - UserProjectPermission.objects.create( - user=self.request.user, project=project, admin=True - ) + if getattr(self.request.user, "is_master_api_key_user", False) is False: + UserProjectPermission.objects.create( + user=self.request.user, project=project, admin=True + ) @action( detail=False, @@ -145,7 +138,7 @@ def permissions(self, *args, **kwargs): url_name="my-permissions", ) def user_permissions(self, request: Request, pk: int = None): - if request.user.is_anonymous: + if getattr(request.user, "is_master_api_key_user", False) is True: return Response( status=status.HTTP_400_BAD_REQUEST, data={ diff --git a/api/segments/permissions.py b/api/segments/permissions.py index 366c4cbd7183..efb901bc0177 100644 --- a/api/segments/permissions.py +++ b/api/segments/permissions.py @@ -1,11 +1,8 @@ -from django.http import HttpRequest -from rest_framework.permissions import BasePermission, IsAuthenticated +from rest_framework.permissions import IsAuthenticated from projects.models import Project from projects.permissions import MANAGE_SEGMENTS, VIEW_PROJECT -from .models import Segment - class SegmentPermissions(IsAuthenticated): def has_permission(self, request, view): @@ -31,35 +28,7 @@ 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_project_permission(MANAGE_SEGMENTS, obj.project) or ( view.action == "retrieve" and request.user.has_project_permission(VIEW_PROJECT, obj.project) ) - - -class MasterAPIKeySegmentPermissions(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 - - project_pk = view.kwargs.get("project_pk") - if not project_pk: - return False - - project = Project.objects.get(pk=project_pk) - - return project.organisation_id == master_api_key.organisation_id - - def has_object_permission( - self, request: HttpRequest, view: str, obj: Segment - ) -> bool: - master_api_key = getattr(request, "master_api_key", None) - return ( - master_api_key - and master_api_key.organisation_id == obj.project.organisation_id - ) diff --git a/api/segments/views.py b/api/segments/views.py index af346974c18b..ec864edf20f6 100644 --- a/api/segments/views.py +++ b/api/segments/views.py @@ -1,13 +1,11 @@ import logging -from core.permissions import HasMasterAPIKey from django.utils.decorators import method_decorator from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema from rest_framework import viewsets -from rest_framework.decorators import action, api_view, permission_classes +from rest_framework.decorators import action, api_view from rest_framework.generics import get_object_or_404 -from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from app.pagination import CustomPagination @@ -18,7 +16,7 @@ from projects.permissions import VIEW_PROJECT from .models import Segment -from .permissions import MasterAPIKeySegmentPermissions, SegmentPermissions +from .permissions import SegmentPermissions from .serializers import SegmentSerializer logger = logging.getLogger() @@ -47,19 +45,16 @@ ) class SegmentViewSet(viewsets.ModelViewSet): serializer_class = SegmentSerializer - permission_classes = [SegmentPermissions | MasterAPIKeySegmentPermissions] + permission_classes = [SegmentPermissions] pagination_class = CustomPagination def get_queryset(self): if getattr(self, "swagger_fake_view", False): return Segment.objects.none() - if hasattr(self.request, "master_api_key"): - permitted_projects = self.request.master_api_key.organisation.projects.all() - else: - permitted_projects = self.request.user.get_permitted_projects( - permission_key=VIEW_PROJECT - ) + permitted_projects = self.request.user.get_permitted_projects( + permission_key=VIEW_PROJECT + ) project = get_object_or_404(permitted_projects, pk=self.kwargs["project_pk"]) queryset = project.segments.all() @@ -111,12 +106,8 @@ def associated_features(self, request, *args, **kwargs): @swagger_auto_schema(responses={200: SegmentSerializer()}, method="get") @api_view(["GET"]) -@permission_classes([IsAuthenticated | HasMasterAPIKey]) def get_segment_by_uuid(request, uuid): - if getattr(request, "master_api_key", None): - accessible_projects = request.master_api_key.organisation.projects.all() - else: - accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) + accessible_projects = request.user.get_permitted_projects(VIEW_PROJECT) qs = Segment.objects.filter(project__in=accessible_projects) segment = get_object_or_404(qs, uuid=uuid) serializer = SegmentSerializer(instance=segment) diff --git a/api/tests/integration/api_keys/test_viewset.py b/api/tests/integration/api_keys/test_viewset.py index 8d6830cefc73..9d7e44691545 100644 --- a/api/tests/integration/api_keys/test_viewset.py +++ b/api/tests/integration/api_keys/test_viewset.py @@ -1,11 +1,7 @@ -import typing - from django.urls import reverse from rest_framework import status from rest_framework.test import APIClient -from api_keys.models import MasterAPIKey - def test_create_master_api_key_returns_key_in_response(admin_client, organisation): # Given @@ -21,13 +17,36 @@ def test_create_master_api_key_returns_key_in_response(admin_client, organisatio # Then assert response.status_code == status.HTTP_201_CREATED assert response.json()["key"] is not None + assert response.json()["is_admin"] is True -def test_delete_master_api_key(admin_client, organisation, master_api_key_prefix): +def test_creating_non_admin_master_api_key_without_rbac_returns_400( + admin_client, organisation, settings +): + # Given + settings.IS_RBAC_INSTALLED = False + + url = reverse( + "api-v1:organisations:organisation-master-api-keys-list", + args=[organisation], + ) + data = {"name": "test_key", "organisation": organisation, "is_admin": False} + + # When + response = admin_client.post(url, data=data) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["is_admin"] == [ + "RBAC is not installed, cannot create non-admin key" + ] + + +def test_delete_master_api_key(admin_client, organisation, admin_master_api_key_prefix): # Given url = reverse( "api-v1:organisations:organisation-master-api-keys-detail", - args=[organisation, master_api_key_prefix], + args=[organisation, admin_master_api_key_prefix], ) # When @@ -37,7 +56,7 @@ def test_delete_master_api_key(admin_client, organisation, master_api_key_prefix assert response.status_code == status.HTTP_204_NO_CONTENT -def test_list_master_api_keys(admin_client, organisation, master_api_key_prefix): +def test_list_master_api_keys(admin_client, organisation, admin_master_api_key_prefix): # Given url = reverse( "api-v1:organisations:organisation-master-api-keys-list", @@ -49,14 +68,16 @@ def test_list_master_api_keys(admin_client, organisation, master_api_key_prefix) # Then assert response.status_code == status.HTTP_200_OK assert response.json()["count"] == 1 - assert response.json()["results"][0]["prefix"] == master_api_key_prefix + assert response.json()["results"][0]["prefix"] == admin_master_api_key_prefix -def test_retrieve_master_api_key(admin_client, organisation, master_api_key_prefix): +def test_retrieve_master_api_key( + admin_client, organisation, admin_master_api_key_prefix +): # Given url = reverse( "api-v1:organisations:organisation-master-api-keys-detail", - args=[organisation, master_api_key_prefix], + args=[organisation, admin_master_api_key_prefix], ) # When @@ -64,21 +85,26 @@ def test_retrieve_master_api_key(admin_client, organisation, master_api_key_pref # Then assert response.status_code == status.HTTP_200_OK - assert response.json()["prefix"] == master_api_key_prefix + assert response.json()["prefix"] == admin_master_api_key_prefix -def test_update_master_api_key(admin_client, organisation, master_api_key_prefix): +def test_update_master_api_key( + admin_client, organisation, admin_master_api_key_prefix, settings +): # Given + settings.IS_RBAC_INSTALLED = True + url = reverse( "api-v1:organisations:organisation-master-api-keys-detail", - args=[organisation, master_api_key_prefix], + args=[organisation, admin_master_api_key_prefix], ) new_name = "updated_test_key" data = { - "prefix": master_api_key_prefix, + "prefix": admin_master_api_key_prefix, "revoked": True, "organisation": organisation, "name": new_name, + "is_admin": False, } # When @@ -86,9 +112,38 @@ def test_update_master_api_key(admin_client, organisation, master_api_key_prefix # Then assert response.status_code == status.HTTP_200_OK - assert response.json()["prefix"] == master_api_key_prefix + assert response.json()["prefix"] == admin_master_api_key_prefix assert response.json()["revoked"] is True assert response.json()["name"] == new_name + assert response.json()["is_admin"] is False + + +def test_update_master_api_key_is_admin_returns_400_if_rbac_is_not_installed( + admin_client, organisation, admin_master_api_key_prefix, settings +): + # Given + settings.IS_RBAC_INSTALLED = False + + url = reverse( + "api-v1:organisations:organisation-master-api-keys-detail", + args=[organisation, admin_master_api_key_prefix], + ) + new_name = "updated_test_key" + data = { + "prefix": admin_master_api_key_prefix, + "organisation": organisation, + "name": new_name, + "is_admin": False, + } + + # When + response = admin_client.put(url, data=data) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["is_admin"] == [ + "RBAC is not installed, cannot create non-admin key" + ] def test_api_returns_403_if_user_is_not_the_org_admin(non_admin_client, organisation): @@ -135,8 +190,8 @@ def test_create_master_api_key_ignores_organisation_in_body(admin_client, organi def test_deleted_api_key_is_not_returned_in_list_and_cannot_be_used( admin_client: APIClient, organisation: int, - master_api_key: typing.Tuple[MasterAPIKey, str], - master_api_key_client: APIClient, + admin_master_api_key_client: APIClient, + admin_master_api_key_prefix: str, ) -> None: # Given # the relevant URLs @@ -146,7 +201,7 @@ def test_deleted_api_key_is_not_returned_in_list_and_cannot_be_used( ) detail_url = reverse( "api-v1:organisations:organisation-master-api-keys-detail", - args=[organisation, master_api_key["prefix"]], + args=[organisation, admin_master_api_key_prefix], ) list_projects_url = "%s?organisation=%s" % ( reverse("api-v1:projects:project-list"), @@ -155,7 +210,7 @@ def test_deleted_api_key_is_not_returned_in_list_and_cannot_be_used( # and we verify that before deletion, the master api key authenticated client # can retrieve the projects for the organisation - valid_response = master_api_key_client.get(list_projects_url) + valid_response = admin_master_api_key_client.get(list_projects_url) assert valid_response.status_code == 200 # When @@ -170,5 +225,5 @@ def test_deleted_api_key_is_not_returned_in_list_and_cannot_be_used( # And # it cannot be used to authenticate with the API anymore - invalid_response = master_api_key_client.get(list_projects_url) + invalid_response = admin_master_api_key_client.get(list_projects_url) assert invalid_response.status_code == 401 diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index 7913ba38eafa..0e291e97ab7a 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -329,7 +329,7 @@ def identity_document_without_fs(environment_api_key, identity_traits): @pytest.fixture() -def master_api_key(organisation, admin_client): +def admin_master_api_key(organisation: int, admin_client: APIClient) -> dict: url = reverse( "api-v1:organisations:organisation-master-api-keys-list", args=[organisation], @@ -341,16 +341,16 @@ def master_api_key(organisation, admin_client): @pytest.fixture() -def master_api_key_prefix(master_api_key): - return master_api_key["prefix"] +def admin_master_api_key_prefix(admin_master_api_key: dict) -> str: + return admin_master_api_key["prefix"] @pytest.fixture() -def master_api_key_client(master_api_key): +def admin_master_api_key_client(admin_master_api_key: dict) -> APIClient: # 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["key"]) + api_client.credentials(HTTP_AUTHORIZATION="Api-Key " + admin_master_api_key["key"]) return api_client diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py index 2b5b3e127e92..22ac7c720d90 100644 --- a/api/tests/integration/environments/test_clone_environment.py +++ b/api/tests/integration/environments/test_clone_environment.py @@ -11,7 +11,8 @@ @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_clone_environment_clones_feature_states_with_value( client, project, environment, environment_api_key, feature 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 8f0c9a74cd31..138049033eaa 100644 --- a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py +++ b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py @@ -7,7 +7,8 @@ @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_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 diff --git a/api/tests/integration/features/featurestate/test_simple_featurestate_viewset.py b/api/tests/integration/features/featurestate/test_simple_featurestate_viewset.py index 9127837d092f..72e911071503 100644 --- a/api/tests/integration/features/featurestate/test_simple_featurestate_viewset.py +++ b/api/tests/integration/features/featurestate/test_simple_featurestate_viewset.py @@ -7,7 +7,8 @@ @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_create_feature_state_for_identity_override( client, environment, identity, feature @@ -32,7 +33,8 @@ def test_create_feature_state_for_identity_override( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_create_feature_state_for_identity_with_identifier( client, environment, identity, feature, identity_identifier @@ -57,7 +59,8 @@ def test_create_feature_state_for_identity_with_identifier( @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_list_feature_states_for_environment(client, environment, feature): # Given @@ -76,7 +79,8 @@ def test_list_feature_states_for_environment(client, environment, feature): @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_update_feature_state(client, environment, feature_state, feature, identity): # Given @@ -100,7 +104,8 @@ def test_update_feature_state(client, environment, feature_state, feature, ident @pytest.mark.parametrize( - "client", [(lazy_fixture("master_api_key_client")), (lazy_fixture("admin_client"))] + "client", + [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], ) def test_update_feature_state_for_identity_with_identifier( client, environment, identity_featurestate, feature, identity, identity_identifier diff --git a/api/tests/integration/features/multivariate/test_integration_multivariate.py b/api/tests/integration/features/multivariate/test_integration_multivariate.py index 4fe8d0005b45..f423d29cf000 100644 --- a/api/tests/integration/features/multivariate/test_integration_multivariate.py +++ b/api/tests/integration/features/multivariate/test_integration_multivariate.py @@ -7,7 +7,8 @@ @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_create_mv_option(client, project, mv_option_50_percent, feature): # Given @@ -34,7 +35,8 @@ def test_can_create_mv_option(client, project, mv_option_50_percent, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_list_mv_option(project, mv_option_50_percent, client, feature): # Given @@ -54,7 +56,8 @@ def test_can_list_mv_option(project, mv_option_50_percent, client, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_creating_mv_options_with_accumulated_total_gt_100_returns_400( project, mv_option_50_percent, client, feature @@ -83,7 +86,8 @@ def test_creating_mv_options_with_accumulated_total_gt_100_returns_400( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_update_default_percentage_allocation( project, mv_option_50_percent, client, feature @@ -112,7 +116,8 @@ def test_can_update_default_percentage_allocation( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_updating_default_percentage_allocation_that_pushes_the_total_percentage_allocation_over_100_returns_400( project, mv_option_50_percent, client, feature @@ -161,7 +166,8 @@ def test_updating_default_percentage_allocation_that_pushes_the_total_percentage @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_remove_mv_option(project, mv_option_50_percent, client, feature): # Given @@ -192,7 +198,8 @@ def test_can_remove_mv_option(project, mv_option_50_percent, client, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_and_update_multivariate_feature_with_2_variations_50_percent( project, environment, environment_api_key, client, feature @@ -286,7 +293,8 @@ def test_create_and_update_multivariate_feature_with_2_variations_50_percent( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_modify_weight_of_2_variations_in_single_request( project, environment, environment_api_key, client, feature diff --git a/api/tests/unit/api_keys/test_authentication.py b/api/tests/unit/api_keys/test_authentication.py new file mode 100644 index 000000000000..b0f2b2181366 --- /dev/null +++ b/api/tests/unit/api_keys/test_authentication.py @@ -0,0 +1,66 @@ +import pytest +from django.utils import timezone +from rest_framework.exceptions import AuthenticationFailed + +from api_keys.authentication import MasterAPIKeyAuthentication + + +def test_authenticate_returns_api_key_user_for_valid_key(master_api_key, rf): + # Given + master_api_key, key = master_api_key + request = rf.get("/some-endpoint", HTTP_AUTHORIZATION="Api-Key " + key) + + # When + user, _ = MasterAPIKeyAuthentication().authenticate(request) + + # Then + assert user.key == master_api_key + + +def test_authenticate_returns_none_if_no_key_provider(rf): + # Given + request = rf.get("/some-endpoint") + + # When + assert MasterAPIKeyAuthentication().authenticate(request) is None + + +def test_authenticate_raises_error_for_expired_key(rf, master_api_key): + # Given + master_api_key, key = master_api_key + + request = rf.get("/some-endpoint", HTTP_AUTHORIZATION="Api-Key " + key) + master_api_key.expiry_date = timezone.now() + master_api_key.save() + + # When + with pytest.raises(AuthenticationFailed): + MasterAPIKeyAuthentication().authenticate(request) + + # Then - exception was raised + + +def test_authenticate_raises_error_for_invalid_key(rf, db): + # Given + request = rf.get("/some-endpoint", HTTP_AUTHORIZATION="Api-Key something_random") + + # When + with pytest.raises(AuthenticationFailed): + MasterAPIKeyAuthentication().authenticate(request) + + # Then - exception was raised + + +def test_authenticate_raises_error_for_revoked_key(rf, master_api_key): + # Given + master_api_key, key = master_api_key + + request = rf.get("/some-endpoint", HTTP_AUTHORIZATION="Api-Key " + key) + master_api_key.revoked = True + master_api_key.save() + + # When + with pytest.raises(AuthenticationFailed): + MasterAPIKeyAuthentication().authenticate(request) + + # Then - exception was raised diff --git a/api/tests/unit/api_keys/test_middleware.py b/api/tests/unit/api_keys/test_middleware.py deleted file mode 100644 index 55974a420325..000000000000 --- a/api/tests/unit/api_keys/test_middleware.py +++ /dev/null @@ -1,18 +0,0 @@ -from api_keys.middleware import MasterAPIKeyMiddleware - - -def test_MasterAPIKeyMiddleware_adds_master_api_key_object_to_request( - master_api_key, rf, mocker -): - # Given - request = rf.get( - "/some-endpoint", HTTP_AUTHORIZATION="Api-Key " + master_api_key[1] - ) - mocked_get_response = mocker.MagicMock() - middleware = MasterAPIKeyMiddleware(mocked_get_response) - - # When - _ = middleware(request) - - # Then - assert request.master_api_key.id is not None diff --git a/api/tests/unit/api_keys/test_user.py b/api/tests/unit/api_keys/test_user.py new file mode 100644 index 000000000000..9e5626e2bacc --- /dev/null +++ b/api/tests/unit/api_keys/test_user.py @@ -0,0 +1,288 @@ +import pytest +from pytest_lazyfixture import lazy_fixture + +from api_keys.user import APIKeyUser +from environments.permissions.models import EnvironmentPermissionModel +from organisations.permissions.models import OrganisationPermissionModel +from projects.models import ProjectPermissionModel + + +def test_is_authenticated(master_api_key_object): + # Given + user = APIKeyUser(master_api_key_object) + + # Then + assert user.is_authenticated is True + + +@pytest.mark.parametrize( + "for_organisation, expected_result", + [ + (lazy_fixture("organisation"), True), + (lazy_fixture("organisation_two"), False), + ], +) +def test_belongs_to(for_organisation, expected_result, master_api_key_object): + # Given + user = APIKeyUser(master_api_key_object) + + # Then + assert user.belongs_to(for_organisation.id) == expected_result + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_is_admin", + [ + (lazy_fixture("project"), lazy_fixture("admin_master_api_key_object"), True), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ], +) +def test_is_project_admin( + for_project, + for_master_api_key, + expected_is_admin, + admin_master_api_key, + master_api_key, +): + # Given + user = APIKeyUser(for_master_api_key) + + # Then + assert user.is_project_admin(for_project) is expected_is_admin + + +@pytest.mark.parametrize( + "for_environment, for_master_api_key, expected_is_admin", + [ + ( + lazy_fixture("environment"), + lazy_fixture("admin_master_api_key_object"), + True, + ), + (lazy_fixture("environment"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ], +) +def test_is_environment_admin( + for_environment, + for_master_api_key, + expected_is_admin, + organisation_two_project_one_environment_one, +): + # Given + user = APIKeyUser(for_master_api_key) + + # Then + assert user.is_environment_admin(for_environment) is expected_is_admin + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_has_permission", + [ + (lazy_fixture("project"), lazy_fixture("admin_master_api_key_object"), True), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ], +) +def test_has_project_permission( + for_project, for_master_api_key, expected_has_permission +): + # Given + user = APIKeyUser(for_master_api_key) + + # When + for permission in ProjectPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + assert ( + user.has_project_permission(permission, for_project) + is expected_has_permission + ) + + +@pytest.mark.parametrize( + "for_environment, for_master_api_key, expected_has_permission", + [ + ( + lazy_fixture("environment"), + lazy_fixture("admin_master_api_key_object"), + True, + ), + (lazy_fixture("environment"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ], +) +def test_has_environment_permission( + for_environment, for_master_api_key, expected_has_permission +): + # Given + user = APIKeyUser(for_master_api_key) + + # When + for permission in EnvironmentPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + assert ( + user.has_environment_permission(permission, for_environment) + is expected_has_permission + ) + + +@pytest.mark.parametrize( + "for_organisation, for_master_api_key, expected_has_permission", + [ + ( + lazy_fixture("organisation"), + lazy_fixture("admin_master_api_key_object"), + True, + ), + (lazy_fixture("organisation"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two"), + lazy_fixture("master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ], +) +def test_has_organisation_permission( + for_organisation, for_master_api_key, expected_has_permission +): + # Given + user = APIKeyUser(for_master_api_key) + + # When + for permission in OrganisationPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + user.has_organisation_permission( + for_organisation, permission + ) is expected_has_permission + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_project", + [ + ( + lazy_fixture("project"), + lazy_fixture("admin_master_api_key_object"), + lazy_fixture("project"), + ), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), None), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + None, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + None, + ), + ], +) +def test_get_permitted_projects(for_project, for_master_api_key, expected_project): + # Given + user = APIKeyUser(for_master_api_key) + + # When + for permission in ProjectPermissionModel.objects.all().values_list( + "key", flat=True + ): + projects = user.get_permitted_projects(permission) + + # Then + if expected_project is None: + assert projects.count() == 0 + else: + assert projects.count() == 1 + assert projects.first() == expected_project + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_environment", + [ + ( + lazy_fixture("project"), + lazy_fixture("admin_master_api_key_object"), + lazy_fixture("environment"), + ), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), None), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + None, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + None, + ), + ], +) +def test_get_permitted_environments( + for_project, + for_master_api_key, + expected_environment, + environment, + organisation_two_project_one_environment_one, +): + # Given + user = APIKeyUser(for_master_api_key) + + # When + for permission in EnvironmentPermissionModel.objects.all().values_list( + "key", flat=True + ): + environments = user.get_permitted_environments(permission, for_project) + + # Then + if expected_environment is None: + assert environments.count() == 0 + else: + assert environments.count() == 1 + assert environments.first() == expected_environment diff --git a/api/tests/unit/conftest.py b/api/tests/unit/conftest.py index fc99606aef79..1a5cba27ff71 100644 --- a/api/tests/unit/conftest.py +++ b/api/tests/unit/conftest.py @@ -1,7 +1,5 @@ import pytest -from rest_framework.test import APIClient -from api_keys.models import MasterAPIKey from environments.models import Environment from features.models import Feature from organisations.models import Organisation, OrganisationRole @@ -99,23 +97,6 @@ def organisation_one_project_one_feature_one(organisation_one_project_one): ) -@pytest.fixture() -def master_api_key(organisation): - master_api_key, key = MasterAPIKey.objects.create_key( - name="test_key", organisation=organisation - ) - return master_api_key, 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[1]) - return api_client - - @pytest.fixture() def dynamo_enabled_project(organisation): return Project.objects.create( @@ -184,3 +165,20 @@ def tag_two(project): description="Test Tag2 description", project=project, ) + + +@pytest.fixture() +def project_two(organisation: Organisation) -> Project: + return Project.objects.create(name="Test Project Two", organisation=organisation) + + +@pytest.fixture() +def environment_two(project: Project) -> Environment: + return Environment.objects.create(name="Test Environment two", project=project) + + +@pytest.fixture +def project_two_environment(project_two: Project) -> Environment: + return Environment.objects.create( + name="Test Project two Environment", project=project_two + ) diff --git a/api/tests/unit/core/test_permissions.py b/api/tests/unit/core/test_permissions.py deleted file mode 100644 index a0644a125cc7..000000000000 --- a/api/tests/unit/core/test_permissions.py +++ /dev/null @@ -1,20 +0,0 @@ -from core.permissions import HasMasterAPIKey - - -def test_has_master_api_key_return_true_if_master_api_key_is_part_of_request(rf): - # Given - request = rf.get("/some-endpoint") - request.master_api_key = "some_key" - permission = HasMasterAPIKey() - - # Then - assert permission.has_permission(request, "") is True - - -def test_has_master_api_key_return_false_if_master_api_key_is_not_part_of_request(rf): - # Given - request = rf.get("/some-endpoint") - permission = HasMasterAPIKey() - - # Then - assert permission.has_permission(request, "") is False 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 2a1de80eee11..89f2b355b6c8 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -21,12 +21,9 @@ def test_multivariate_feature_options_view_set_get_permissions(): permissions = view_set.get_permissions() # Then - # NOTE: since we are using or(|) operator in the viewset `get_permission` returns - # an instance of `OR` object which store the permissions in attributes named `op1` and `op2` - # ref: https://github.com/encode/django-rest-framework/blob/3.9.x/rest_framework/permissions.py#L71 assert len(permissions) == 1 - assert isinstance(permissions[0].op1, NestedProjectPermissions) - assert permissions[0].op1.action_permission_map == { + assert isinstance(permissions[0], NestedProjectPermissions) + assert permissions[0].action_permission_map == { "list": VIEW_PROJECT, "detail": VIEW_PROJECT, "create": CREATE_FEATURE, @@ -37,7 +34,8 @@ def test_multivariate_feature_options_view_set_get_permissions(): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_mv_feature_option_by_uuid(client, project, multivariate_feature): # Given @@ -56,7 +54,8 @@ def test_get_mv_feature_option_by_uuid(client, project, multivariate_feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_mv_feature_option_by_uuid_returns_404_if_mv_option_does_not_exists( client, project diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index c20922efd9c1..dc8620a8390d 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -100,7 +100,8 @@ def test_list_feature_states_nested_environment_view_set( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_environment_feature_states_filter_using_feataure_name( environment, project, feature, client @@ -123,7 +124,8 @@ def test_environment_feature_states_filter_using_feataure_name( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_environment_feature_states_filter_to_show_identity_override_only( environment, feature, client @@ -157,7 +159,8 @@ def test_environment_feature_states_filter_to_show_identity_override_only( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_environment_feature_states_only_returns_latest_versions( environment, feature, client @@ -185,7 +188,8 @@ def test_environment_feature_states_only_returns_latest_versions( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_environment_feature_states_does_not_return_null_versions( environment, feature, client @@ -214,7 +218,8 @@ def test_environment_feature_states_does_not_return_null_versions( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_feature_default_is_archived_is_false(client, project): # Given - set up data @@ -233,7 +238,8 @@ def test_create_feature_default_is_archived_is_false(client, project): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_feature_is_archived(client, project, feature): # Given @@ -252,7 +258,8 @@ def test_update_feature_is_archived(client, project, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_should_create_feature_states_when_feature_created( client, project, environment @@ -288,7 +295,8 @@ def test_should_create_feature_states_when_feature_created( @pytest.mark.parametrize("default_value", [(12), (True), ("test")]) @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_should_create_feature_states_with_value_when_feature_created( client, project, environment, default_value @@ -318,7 +326,8 @@ def test_should_create_feature_states_with_value_when_feature_created( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_should_delete_feature_states_when_feature_deleted( client, project, feature, environment @@ -349,7 +358,8 @@ def test_should_delete_feature_states_when_feature_deleted( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_feature_returns_201_if_name_matches_regex(client, project): # Given @@ -368,7 +378,8 @@ def test_create_feature_returns_201_if_name_matches_regex(client, project): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_feature_returns_400_if_name_does_not_matches_regex(client, project): # Given @@ -391,7 +402,8 @@ def test_create_feature_returns_400_if_name_does_not_matches_regex(client, proje @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_log_created_when_feature_created(client, project, environment): # Given @@ -421,7 +433,8 @@ def test_audit_log_created_when_feature_created(client, project, environment): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_log_created_when_feature_updated(client, project, feature): # Given @@ -448,7 +461,8 @@ def test_audit_log_created_when_feature_updated(client, project, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_logs_created_when_feature_deleted(client, project, feature): # Given @@ -477,7 +491,8 @@ def test_audit_logs_created_when_feature_deleted(client, project, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_should_create_tags_when_feature_created(client, project, tag_one, tag_two): # Given - set up data @@ -511,7 +526,8 @@ def test_should_create_tags_when_feature_created(client, project, tag_one, tag_t @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_add_owners_adds_owner(client, project): # Given @@ -548,7 +564,8 @@ def test_add_owners_adds_owner(client, project): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_list_features_return_tags(client, project, feature): # Given @@ -568,7 +585,8 @@ def test_list_features_return_tags(client, project, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_project_admin_can_create_mv_options_when_creating_feature(client, project): # Given @@ -590,7 +608,8 @@ def test_project_admin_can_create_mv_options_when_creating_feature(client, proje @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_feature_by_uuid(client, project, feature): # Given @@ -607,7 +626,8 @@ def test_get_feature_by_uuid(client, project, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_feature_by_uuid_returns_404_if_feature_does_not_exists(client, project): # Given @@ -621,7 +641,8 @@ def test_get_feature_by_uuid_returns_404_if_feature_does_not_exists(client, proj @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_feature_state_value_triggers_dynamo_rebuild( client, project, environment, feature, feature_state, settings, mocker @@ -651,7 +672,8 @@ def test_update_feature_state_value_triggers_dynamo_rebuild( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_segment_overrides_creates_correct_audit_log_messages( client, feature, segment, environment @@ -707,7 +729,8 @@ def test_create_segment_overrides_creates_correct_audit_log_messages( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_list_features_provides_information_on_number_of_overrides( feature, @@ -738,7 +761,8 @@ def test_list_features_provides_information_on_number_of_overrides( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_list_features_provides_segment_overrides_for_dynamo_enabled_project( dynamo_enabled_project, dynamo_enabled_project_environment_one, client @@ -818,7 +842,8 @@ def test_create_segment_override_reaching_max_limit( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_feature_reaching_max_limit(client, project, settings): # Given diff --git a/api/tests/unit/permissions/permission_service/conftest.py b/api/tests/unit/permissions/permission_service/conftest.py index da3b9b7e9cd7..625f12e35406 100644 --- a/api/tests/unit/permissions/permission_service/conftest.py +++ b/api/tests/unit/permissions/permission_service/conftest.py @@ -1,18 +1,5 @@ import pytest -from environments.models import Environment -from projects.models import Project - - -@pytest.fixture() -def project_two(organisation): - return Project.objects.create(name="Test Project Two", organisation=organisation) - - -@pytest.fixture -def project_two_environment(project_two): - return Environment.objects.create(name="Test Environment Two", project=project_two) - @pytest.fixture def project_permission_using_user_permission(user_project_permission): diff --git a/api/tests/unit/permissions/permission_service/test_master_api_key_permission_service.py b/api/tests/unit/permissions/permission_service/test_master_api_key_permission_service.py new file mode 100644 index 000000000000..d0da1f6ec51d --- /dev/null +++ b/api/tests/unit/permissions/permission_service/test_master_api_key_permission_service.py @@ -0,0 +1,192 @@ +import pytest +from pytest_lazyfixture import lazy_fixture + +from environments.permissions.models import EnvironmentPermissionModel +from organisations.permissions.models import OrganisationPermissionModel +from permissions.permission_service import ( + get_permitted_environments_for_master_api_key, + get_permitted_projects_for_master_api_key, + is_master_api_key_environment_admin, + is_master_api_key_project_admin, + master_api_key_has_organisation_permission, +) +from projects.models import ProjectPermissionModel + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_is_admin", + [ + (lazy_fixture("project"), lazy_fixture("admin_master_api_key_object"), True), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ], +) +def test_is_master_api_key_project_admin( + for_project, + for_master_api_key, + expected_is_admin, + admin_master_api_key, + master_api_key, +): + assert ( + is_master_api_key_project_admin(for_master_api_key, for_project) + is expected_is_admin + ) + + +@pytest.mark.parametrize( + "for_environment, for_master_api_key, expected_is_admin", + [ + ( + lazy_fixture("environment"), + lazy_fixture("admin_master_api_key_object"), + True, + ), + (lazy_fixture("environment"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two_project_one_environment_one"), + lazy_fixture("master_api_key_object"), + False, + ), + ], +) +def test_is_master_api_key_environment_admin( + for_environment, + for_master_api_key, + expected_is_admin, + organisation_two_project_one_environment_one, +): + assert ( + is_master_api_key_environment_admin(for_master_api_key, for_environment) + is expected_is_admin + ) + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_count", + [ + (lazy_fixture("project"), lazy_fixture("admin_master_api_key_object"), 1), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), 0), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + 0, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + 0, + ), + ], +) +def test_get_permitted_projects_for_master_api_key( + for_project, + for_master_api_key, + expected_count, +): + # When + for permission in ProjectPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + assert ( + get_permitted_projects_for_master_api_key( + for_master_api_key, permission + ).count() + == expected_count + ) + + +@pytest.mark.parametrize( + "for_project, for_master_api_key, expected_count", + [ + (lazy_fixture("project"), lazy_fixture("admin_master_api_key_object"), 2), + (lazy_fixture("project"), lazy_fixture("master_api_key_object"), 0), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("master_api_key_object"), + 0, + ), + ( + lazy_fixture("organisation_two_project_one"), + lazy_fixture("admin_master_api_key_object"), + 0, + ), + ], +) +def test_get_permitted_environments_for_master_api_key( + for_project, + for_master_api_key, + expected_count, + environment, + environment_two, + organisation_two_project_one_environment_one, + admin_master_api_key, + master_api_key, +): + # When + for permission in EnvironmentPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + assert ( + get_permitted_environments_for_master_api_key( + for_master_api_key, for_project, permission + ).count() + == expected_count + ) + + +@pytest.mark.parametrize( + "for_organisation, for_master_api_key, expected_has_permission", + [ + ( + lazy_fixture("organisation"), + lazy_fixture("admin_master_api_key_object"), + True, + ), + (lazy_fixture("organisation"), lazy_fixture("master_api_key_object"), False), + ( + lazy_fixture("organisation_two"), + lazy_fixture("master_api_key_object"), + False, + ), + ( + lazy_fixture("organisation_two"), + lazy_fixture("admin_master_api_key_object"), + False, + ), + ], +) +def test_master_api_key_has_organisation_permission( + for_organisation, + for_master_api_key, + expected_has_permission, + admin_master_api_key, + master_api_key, +): + # When + for permission in OrganisationPermissionModel.objects.all().values_list( + "key", flat=True + ): + # Then + assert ( + master_api_key_has_organisation_permission( + for_master_api_key, for_organisation, permission + ) + is expected_has_permission + ) diff --git a/api/tests/unit/projects/test_unit_projects_views.py b/api/tests/unit/projects/test_unit_projects_views.py index f2b7100e39bb..3f032f6f27de 100644 --- a/api/tests/unit/projects/test_unit_projects_views.py +++ b/api/tests/unit/projects/test_unit_projects_views.py @@ -9,7 +9,8 @@ @pytest.mark.parametrize( - "client", (lazy_fixture("admin_client"), lazy_fixture("master_api_key_client")) + "client", + (lazy_fixture("admin_client"), lazy_fixture("admin_master_api_key_client")), ) def test_get_project_list_data(client, organisation): # Given @@ -54,7 +55,8 @@ def test_get_project_list_data(client, organisation): @pytest.mark.parametrize( - "client", (lazy_fixture("admin_client"), lazy_fixture("master_api_key_client")) + "client", + (lazy_fixture("admin_client"), lazy_fixture("admin_master_api_key_client")), ) def test_get_project_data_by_id(client, organisation, project): # Given diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index 55736880a542..ebdbe7c545c3 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -18,7 +18,8 @@ @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_filter_by_identity_to_get_only_matching_segments( project, client, environment, identity, trait, identity_matching_segment, segment @@ -35,7 +36,8 @@ def test_can_filter_by_identity_to_get_only_matching_segments( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_cannot_create_segments_without_rules(project, client): # Given @@ -50,7 +52,8 @@ def test_cannot_create_segments_without_rules(project, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_create_segments_with_boolean_condition(project, client): # Given @@ -77,7 +80,8 @@ def test_can_create_segments_with_boolean_condition(project, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_create_segments_with_condition_that_has_null_value(project, client): # Given @@ -102,7 +106,8 @@ def test_can_create_segments_with_condition_that_has_null_value(project, client) @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_segments_reaching_max_limit(project, client, settings): # Given @@ -139,7 +144,8 @@ def test_create_segments_reaching_max_limit(project, client, settings): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_log_created_when_segment_updated(project, segment, client): # Given @@ -168,7 +174,8 @@ def test_audit_log_created_when_segment_updated(project, segment, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_audit_log_created_when_segment_created(project, client): # Given @@ -193,7 +200,8 @@ def test_audit_log_created_when_segment_created(project, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_filter_by_edge_identity_to_get_only_matching_segments( project, @@ -226,7 +234,8 @@ def test_can_filter_by_edge_identity_to_get_only_matching_segments( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_associated_features_returns_all_the_associated_features( project, environment, feature, segment, segment_featurestate, client @@ -252,7 +261,8 @@ def test_associated_features_returns_all_the_associated_features( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_can_create_feature_based_segment(project, client, feature): # Given @@ -273,7 +283,8 @@ def test_can_create_feature_based_segment(project, client, feature): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_get_segment_by_uuid(client, project, segment): # Given @@ -291,7 +302,10 @@ def test_get_segment_by_uuid(client, project, segment): @pytest.mark.parametrize( "client, num_queries", - [(lazy_fixture("master_api_key_client"), 11), (lazy_fixture("admin_client"), 10)], + [ + (lazy_fixture("admin_master_api_key_client"), 11), + (lazy_fixture("admin_client"), 10), + ], ) def test_list_segments(django_assert_num_queries, project, client, num_queries): # Given @@ -326,7 +340,8 @@ def test_list_segments(django_assert_num_queries, project, client, num_queries): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_search_segments(django_assert_num_queries, project, client): # Given @@ -361,7 +376,8 @@ def test_search_segments(django_assert_num_queries, project, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_create_segments_with_description_condition(project, client): # Given @@ -396,7 +412,8 @@ def test_create_segments_with_description_condition(project, client): @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_segment_add_new_condition(project, client, segment, segment_rule): # Given @@ -461,7 +478,8 @@ def test_update_segment_add_new_condition(project, client, segment, segment_rule @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_segment_delete_existing_condition( project, client, segment, segment_rule @@ -515,7 +533,8 @@ def test_update_segment_delete_existing_condition( @pytest.mark.parametrize( - "client", [lazy_fixture("master_api_key_client"), lazy_fixture("admin_client")] + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) def test_update_segment_delete_existing_rule(project, client, segment, segment_rule): # Given diff --git a/api/users/abc.py b/api/users/abc.py new file mode 100644 index 000000000000..103e9aca0d08 --- /dev/null +++ b/api/users/abc.py @@ -0,0 +1,56 @@ +from abc import ABC, abstractmethod + +from django.db.models import QuerySet + +from environments.models import Environment +from organisations.models import Organisation +from projects.models import Project + + +class UserABC(ABC): + @property + @abstractmethod + def is_authenticated(self) -> bool: + raise NotImplementedError() + + @abstractmethod + def belongs_to(self, organisation_id: int) -> bool: + raise NotImplementedError() + + @abstractmethod + def is_project_admin(self, project: "Project") -> bool: + raise NotImplementedError() + + @abstractmethod + def is_environment_admin(self, environment: "Environment") -> bool: + raise NotImplementedError() + + @abstractmethod + def has_project_permission(self, permission: str, project: "Project") -> bool: + raise NotImplementedError() + + @abstractmethod + def has_environment_permission( + self, permission: str, environment: "Environment" + ) -> bool: + raise NotImplementedError() + + @abstractmethod + def has_organisation_permission( + self, organisation: Organisation, permission_key: str + ) -> bool: + raise NotImplementedError() + + @abstractmethod + def get_permitted_projects(self, permission_key: str) -> QuerySet["Project"]: + raise NotImplementedError() + + @abstractmethod + def get_permitted_environments( + self, permission_key: str, project: "Project" + ) -> QuerySet["Environment"]: + raise NotImplementedError() + + @classmethod + def __subclasshook__(cls, subclass): + return all([hasattr(subclass, attr) for attr in cls.__abstractmethods__]) diff --git a/api/users/models.py b/api/users/models.py index 3d8f5e2e0544..6b5ab7063df9 100644 --- a/api/users/models.py +++ b/api/users/models.py @@ -30,6 +30,7 @@ user_has_organisation_permission, ) from projects.models import Project, UserProjectPermission +from users.abc import UserABC from users.auth_type import AuthType from users.constants import DEFAULT_DELETE_ORPHAN_ORGANISATIONS_VALUE from users.exceptions import InvalidInviteError @@ -324,6 +325,12 @@ def remove_as_group_admin(self, group_id: int): ).update(group_admin=False) +# Since we can't enforce FFAdminUser to implement the UserABC interface using inheritance +# we use __subclasshook__[1] method on UserABC to check if FFAdminUser implements the required interface +# [1]https://docs.python.org/3/library/abc.html#abc.ABCMeta.__subclasshook__ +assert issubclass(FFAdminUser, UserABC) + + class UserPermissionGroupMembership(models.Model): userpermissiongroup = models.ForeignKey( "users.UserPermissionGroup",