From 7a032699c8e7eac128569d21ab480ac654c018e9 Mon Sep 17 00:00:00 2001 From: Martin Braun Date: Fri, 9 Jun 2023 21:52:21 +0200 Subject: [PATCH 1/2] fix behaviour of user permissions and user endpoints --- .../core/components/user/serializers.py | 29 +++++-------------- .../core/components/user/user_permissions.py | 21 ++++++++++---- skipper/skipper/core/constants.py | 2 +- ..._coreuserpermissionspermissions_options.py | 17 +++++++++++ skipper/skipper/core/models/permissions.py | 8 ++--- .../user/test_user_permissions_detail.py | 4 +-- .../user/test_user_permissions_list.py | 4 +-- .../user/test_userpermissions_permissions.py | 18 ++++++------ skipper/skipper/flow/models/permissions.py | 2 +- 9 files changed, 59 insertions(+), 46 deletions(-) create mode 100644 skipper/skipper/core/migrations/0035_alter_coreuserpermissionspermissions_options.py diff --git a/skipper/skipper/core/components/user/serializers.py b/skipper/skipper/core/components/user/serializers.py index b4c73a2..97bd0ae 100644 --- a/skipper/skipper/core/components/user/serializers.py +++ b/skipper/skipper/core/components/user/serializers.py @@ -20,43 +20,30 @@ class UserSerializer(serializers.HyperlinkedModelSerializer): + """ + Tenant unspecific User serializer + """ id = serializers.IntegerField(read_only=True) url = serializers.HyperlinkedIdentityField(view_name=constants.core_user_view_set_name + '-detail') password = serializers.CharField(write_only=True, max_length=128) - username = serializers.CharField(max_length=40) + # we would have 150 - 32 = 118, but leave a bit extra room + username = serializers.CharField(max_length=150) permissions = serializers.HyperlinkedIdentityField(view_name=constants.core_user_permission_view_set_name) - def to_representation(self, instance: Any) -> Any: - representation = super().to_representation(instance) - split = representation['username'].split('@@') - _name: str - if len(split) == 1: - _name = split[0] - else: - _name = representation['username'][len(split[0]) + len('@@'):] - representation['username'] = _name - return representation - def validate_username(self, username: str) -> str: - if '@@' in username: - raise ValidationError("may not contain '@@'") - - tenant = get_current_tenant() - name_with_tenant = f'{tenant.name}@@{username}' - kwargs = self.context.get('view').kwargs # type: ignore if 'pk' not in kwargs: - if User.objects.all().filter(username=name_with_tenant).exists(): + if User.objects.all().filter(username=username).exists(): raise ValidationError( f'username \'{username}\' is already in use by another User') else: _id = kwargs['pk'] _user: User = get_object_or_404( User.objects.filter(id=_id)) - if name_with_tenant != _user.username: + if username != _user.username: raise ValidationError('changing of usernames is not supported!') - return name_with_tenant + return username def validate_password(self, password: str) -> str: password_validation.validate_password(password) diff --git a/skipper/skipper/core/components/user/user_permissions.py b/skipper/skipper/core/components/user/user_permissions.py index 753078e..80c991c 100644 --- a/skipper/skipper/core/components/user/user_permissions.py +++ b/skipper/skipper/core/components/user/user_permissions.py @@ -28,12 +28,21 @@ get_directly_assigned_user_permissions ) from skipper.core.renderers import CustomizableBrowsableAPIRendererObjectMixin - -from skipper.core.models.permissions import get_permissions_class +from skipper.core.models.permissions import get_permissions_class, get_permission_string_for_action_and_http_verb class CoreUserPermissionsViewMixin: - permission_classes: Any = (get_permissions_class('user', 'user-permission'),) + permission_classes: Any = (get_permissions_class('user', 'user_permission'),) + + +""" +includes also all permissions that should not be settable on tenant users +""" +all_generally_assignable_permissions = set([ + get_permission_string_for_action_and_http_verb("user", action, method) + for method in ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] + for action in ["user", "user_permission"] +]).union(generally_assignable_permissions) class UserPermissionsView( @@ -59,7 +68,7 @@ def put(self, request: Request, *args: Any, **kwargs: Any) -> Response: def get_serializer_class(self) -> Any: - _assignable_permissions = get_assignable_permissions(self.request.user, generally_assignable_permissions) + _assignable_permissions = get_assignable_permissions(self.request.user, all_generally_assignable_permissions) class GenericUserPermissionSerializer(BaseSerializer): user_permissions = MultipleChoiceField( @@ -71,7 +80,7 @@ def to_representation(self, obj: User) -> Any: # only the directly assigned ones representation = { 'user_permissions': sorted( - list(get_directly_assigned_user_permissions(obj, generally_assignable_permissions)) + list(get_directly_assigned_user_permissions(obj, all_generally_assignable_permissions)) ) } return representation @@ -84,7 +93,7 @@ def update(self, instance: Model, validated_data: Dict[str, Any]) -> Model: new_permissions = set(validated_data['user_permissions']) _directly_assigned = get_directly_assigned_user_permissions( - cast(User, instance), generally_assignable_permissions + cast(User, instance), all_generally_assignable_permissions ) to_remove = (_directly_assigned - new_permissions) diff --git a/skipper/skipper/core/constants.py b/skipper/skipper/core/constants.py index f85b259..fe0146e 100644 --- a/skipper/skipper/core/constants.py +++ b/skipper/skipper/core/constants.py @@ -22,4 +22,4 @@ def component_root(component: str) -> str: core_tenant_view_set_name = skipper_base_name('tenant') core_tenant_user_view_set_name = skipper_base_name('tenant_user') core_user_view_set_name = skipper_base_name('user') -core_user_permission_view_set_name = skipper_base_name('user-permission') +core_user_permission_view_set_name = skipper_base_name('user_permission') diff --git a/skipper/skipper/core/migrations/0035_alter_coreuserpermissionspermissions_options.py b/skipper/skipper/core/migrations/0035_alter_coreuserpermissionspermissions_options.py new file mode 100644 index 0000000..ef32135 --- /dev/null +++ b/skipper/skipper/core/migrations/0035_alter_coreuserpermissionspermissions_options.py @@ -0,0 +1,17 @@ +# Generated by Django 4.1.9 on 2023-06-09 19:34 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0034_alter_tenant_user_options'), + ] + + operations = [ + migrations.AlterModelOptions( + name='coreuserpermissionspermissions', + options={'default_permissions': [], 'managed': False, 'permissions': [('user_get_user_permission', 'Can run GET on entity user and action user_permission'), ('user_options_user_permission', 'Can run OPTIONS on entity user on action user_permission'), ('user_head_user_permission', 'Can run HEAD on entity user on action user_permission'), ('user_post_user_permission', 'Can run POST on entity user on action user_permission'), ('user_put_user_permission', 'Can run PUT on entity user on action user_permission'), ('user_patch_user_permission', 'Can run PATCH on entity user on action user_permission'), ('user_delete_user_permission', 'Can run DELETE on entity user on action user_permission')]}, + ), + ] diff --git a/skipper/skipper/core/models/permissions.py b/skipper/skipper/core/models/permissions.py index 4e01a17..e982721 100644 --- a/skipper/skipper/core/models/permissions.py +++ b/skipper/skipper/core/models/permissions.py @@ -64,7 +64,7 @@ def get_permissions_class(entity: str, action: str) -> Type[permissions.DjangoMo Example: * changing core/user: entity=user, action=user - * changing core/user's permission field: entity=user, action=user-permission + * changing core/user's permission field: entity=user, action=user_permission """ class Permission(permissions.DjangoModelPermissions): @@ -84,7 +84,7 @@ class Permission(permissions.DjangoModelPermissions): # TODO This is only a hack-solution until we have a custom user model class CoreUserPermissions(Model): """ - global flow permissions, not really a model + global core permissions, not really a model that stores any real data """ class Meta: @@ -99,7 +99,7 @@ class Meta: # TODO This is only a hack-solution until we have a custom user model class CoreUserPermissionsPermissions(Model): """ - global flow permissions, not really a model + global core permissions, not really a model that stores any real data """ class Meta: @@ -107,5 +107,5 @@ class Meta: # operations will be performed for this model default_permissions: List[str] = [] permissions = ( - gen_permissions(entity='user', action='user-permission') + gen_permissions(entity='user', action='user_permission') ) \ No newline at end of file diff --git a/skipper/skipper/core/tests/components/user/test_user_permissions_detail.py b/skipper/skipper/core/tests/components/user/test_user_permissions_detail.py index 5e176b0..20d6cd3 100644 --- a/skipper/skipper/core/tests/components/user/test_user_permissions_detail.py +++ b/skipper/skipper/core/tests/components/user/test_user_permissions_detail.py @@ -39,9 +39,9 @@ def base_add_bare_user(self) -> None: self.user_dict = self.create_payload( url=USER_LIST_URL, payload={ - 'username': 'test_user', + 'username': 'some_test_user', 'password': 'lkshdfjkghjashgoi4ewrpbn', - 'email': 'test_user@neurofoege.de', + 'email': 'some_test_user@neurofoege.de', 'groups': [], 'is_active': True }, diff --git a/skipper/skipper/core/tests/components/user/test_user_permissions_list.py b/skipper/skipper/core/tests/components/user/test_user_permissions_list.py index 786fcb7..59915bf 100644 --- a/skipper/skipper/core/tests/components/user/test_user_permissions_list.py +++ b/skipper/skipper/core/tests/components/user/test_user_permissions_list.py @@ -124,9 +124,9 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: return self.user_client.post( path=self.url_under_test, data={ - 'username': 'test_user', + 'username': 'some_test_user', 'password': 'lkshdfjkghjashgoi4ewrpbn', - 'email': 'test_user@neurofoege.de', + 'email': 'some_test_user@neurofoege.de', 'groups': [], 'is_active': True }, diff --git a/skipper/skipper/core/tests/components/user/test_userpermissions_permissions.py b/skipper/skipper/core/tests/components/user/test_userpermissions_permissions.py index fba5a35..8e2da0b 100644 --- a/skipper/skipper/core/tests/components/user/test_userpermissions_permissions.py +++ b/skipper/skipper/core/tests/components/user/test_userpermissions_permissions.py @@ -39,9 +39,9 @@ def base_add_bare_user(self) -> None: self.user_dict = self.create_payload( url=USER_LIST_URL, payload={ - 'username': 'test_user', + 'username': 'some_test_user', 'password': 'lkshdfjkghjashgoi4ewrpbn', - 'email': 'test_user@neurofoege.de', + 'email': 'some_test_user@neurofoege.de', 'is_active': True }, simulate_tenant=False @@ -51,7 +51,7 @@ def base_add_bare_user(self) -> None: class CoreUserPermissionPermissionGETTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'GET', 'user-permission') + return core_permission_for_rest_method('user', 'GET', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None @@ -71,7 +71,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionHEADTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'HEAD', 'user-permission') + return core_permission_for_rest_method('user', 'HEAD', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None @@ -92,7 +92,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionPOSTTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'POST', 'user-permission') + return core_permission_for_rest_method('user', 'POST', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None @@ -116,7 +116,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionOPTIONSTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'OPTIONS', 'user-permission') + return core_permission_for_rest_method('user', 'OPTIONS', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None @@ -136,7 +136,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionPUTTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'PUT', 'user-permission') + return core_permission_for_rest_method('user', 'PUT', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return self.user_client.put( @@ -166,7 +166,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionPATCHTest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'PATCH', 'user-permission') + return core_permission_for_rest_method('user', 'PATCH', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None @@ -190,7 +190,7 @@ def method_under_test_proper(self) -> Union[HttpResponse, TestHttpResponse]: class CoreUserPermissionPermissionDELETETest(CoreUserPermissionPermissionBaseTest): def permission_code_name(self) -> str: - return core_permission_for_rest_method('user', 'DELETE', 'user-permission') + return core_permission_for_rest_method('user', 'DELETE', 'user_permission') def method_under_test_malformed(self) -> Optional[Union[HttpResponse, TestHttpResponse]]: return None diff --git a/skipper/skipper/flow/models/permissions.py b/skipper/skipper/flow/models/permissions.py index 0809fe5..8197416 100644 --- a/skipper/skipper/flow/models/permissions.py +++ b/skipper/skipper/flow/models/permissions.py @@ -81,7 +81,7 @@ def get_permissions_class(entity: str, action: str) -> Type[permissions.DjangoMo Example: * changing user: entity=user, action=user - * changing user's permission field: entity=user, action=user-permission + * changing user's permission field: entity=user, action=some-action """ class Permission(permissions.DjangoModelPermissions): From b3072419e23e9c2ef620a12c6834f04d4102b72f Mon Sep 17 00:00:00 2001 From: Martin Braun Date: Fri, 9 Jun 2023 22:10:17 +0200 Subject: [PATCH 2/2] minor --- skipper/skipper/core/components/user/serializers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/skipper/skipper/core/components/user/serializers.py b/skipper/skipper/core/components/user/serializers.py index 97bd0ae..4a81d77 100644 --- a/skipper/skipper/core/components/user/serializers.py +++ b/skipper/skipper/core/components/user/serializers.py @@ -26,7 +26,6 @@ class UserSerializer(serializers.HyperlinkedModelSerializer): id = serializers.IntegerField(read_only=True) url = serializers.HyperlinkedIdentityField(view_name=constants.core_user_view_set_name + '-detail') password = serializers.CharField(write_only=True, max_length=128) - # we would have 150 - 32 = 118, but leave a bit extra room username = serializers.CharField(max_length=150) permissions = serializers.HyperlinkedIdentityField(view_name=constants.core_user_permission_view_set_name)