From 155baa02005733bb23a8ec66dbad147ea5d9d9f9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Sep 2023 13:40:06 -0400 Subject: [PATCH] unit tests, add cisa_analyst permission in the cisa_analysts_group for better grannular hasPerm testing in admin.py --- docs/django-admin/roles.md | 13 ++--- src/registrar/admin.py | 42 ++++++++------ src/registrar/fixtures_applications.py | 3 - src/registrar/fixtures_users.py | 3 +- .../migrations/0034_alter_user_options.py | 1 + .../0035_contenttypes_permissions.py | 15 +++-- .../migrations/0036_create_groups.py | 16 ++++-- src/registrar/models/domain.py | 2 +- src/registrar/models/user.py | 3 +- src/registrar/models/user_group.py | 57 ++++++++++++------- src/registrar/tests/test_migrations.py | 51 +++++++++++++++++ 11 files changed, 142 insertions(+), 64 deletions(-) create mode 100644 src/registrar/tests/test_migrations.py diff --git a/docs/django-admin/roles.md b/docs/django-admin/roles.md index da91f41e0..6a9f0ca75 100644 --- a/docs/django-admin/roles.md +++ b/docs/django-admin/roles.md @@ -9,13 +9,8 @@ our `user_group` model and run in a migration. ## Editing group permissions through code -We can edit and deploy new group permissions by -editing `user_group` then: +We can edit and deploy new group permissions by: -- Duplicating migration `0036_create_groups` -and running migrations (RECOMMENDED METHOD), or - -- Fake the previous migration to run an existing create groups migration: - - step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions - - step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups - - step 3: fake run the latest migration in the migrations list \ No newline at end of file +1. editing `user_group` then: +2. Duplicating migration `0036_create_groups` +and running migrations \ No newline at end of file diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 13659281d..7ef6286fb 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -205,10 +205,10 @@ class MyUserAdmin(BaseUserAdmin): # which is equivalent to superuser. The other group we use to manage # perms is cisa_analysts_group. cisa_analysts_group will never contain # full_access_permission - if request.user.has_perm('registrar.full_access_permission'): + if request.user.has_perm("registrar.full_access_permission"): # Use the default list display for all access users return super().get_list_display(request) - + # Customize the list display for analysts return ( "email", @@ -220,17 +220,23 @@ class MyUserAdmin(BaseUserAdmin): ) def get_fieldsets(self, request, obj=None): - if request.user.has_perm('registrar.full_access_permission'): + if request.user.has_perm("registrar.full_access_permission"): # Show all fields for all access users return super().get_fieldsets(request, obj) - - # show analyst_fieldsets for analysts - return self.analyst_fieldsets + elif request.user.has_perm("registrar.analyst_access_permission"): + # show analyst_fieldsets for analysts + return self.analyst_fieldsets + else: + # any admin user should belong to either full_access_group + # or cisa_analyst_group + return [] def get_readonly_fields(self, request, obj=None): - if request.user.has_perm('registrar.full_access_permission'): + if request.user.has_perm("registrar.full_access_permission"): return () # No read-only fields for all access users - return self.analyst_readonly_fields # Read-only fields for analysts + # Return restrictive Read-only fields for analysts and + # users who might not belong to groups + return self.analyst_readonly_fields class HostIPInline(admin.StackedInline): @@ -409,11 +415,12 @@ class DomainInformationAdmin(ListHeaderAdmin): readonly_fields = list(self.readonly_fields) - if request.user.has_perm('registrar.full_access_permission'): - return readonly_fields - else: - readonly_fields.extend([field for field in self.analyst_readonly_fields]) + if request.user.has_perm("registrar.full_access_permission"): return readonly_fields + # Return restrictive Read-only fields for analysts and + # users who might not belong to groups + readonly_fields.extend([field for field in self.analyst_readonly_fields]) + return readonly_fields # Read-only fields for analysts class DomainApplicationAdminForm(forms.ModelForm): @@ -627,11 +634,12 @@ class DomainApplicationAdmin(ListHeaderAdmin): ["current_websites", "other_contacts", "alternative_domains"] ) - if request.user.has_perm('registrar.full_access_permission'): - return readonly_fields - else: - readonly_fields.extend([field for field in self.analyst_readonly_fields]) + if request.user.has_perm("registrar.full_access_permission"): return readonly_fields + # Return restrictive Read-only fields for analysts and + # users who might not belong to groups + readonly_fields.extend([field for field in self.analyst_readonly_fields]) + return readonly_fields def display_restricted_warning(self, request, obj): if obj and obj.creator.status == models.User.RESTRICTED: @@ -702,7 +710,7 @@ class DomainAdmin(ListHeaderAdmin): search_fields = ["name"] search_help_text = "Search by domain name." change_form_template = "django/admin/domain_change_form.html" - readonly_fields = ["state"] + # readonly_fields = ["state"] def response_change(self, request, obj): # Create dictionary of action functions diff --git a/src/registrar/fixtures_applications.py b/src/registrar/fixtures_applications.py index 2f5965147..18be79814 100644 --- a/src/registrar/fixtures_applications.py +++ b/src/registrar/fixtures_applications.py @@ -10,9 +10,6 @@ from registrar.models import ( Website, ) -from django.contrib.auth.models import Permission -from django.contrib.contenttypes.models import ContentType - fake = Faker() logger = logging.getLogger(__name__) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 5919ef70d..c9d62bd54 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -128,7 +128,7 @@ class UserFixture: "email": "nicolle.leclair@ecstech.com", }, ] - + def load_users(cls, users, group_name): logger.info(f"Going to load {len(users)} users in group {group_name}") for user_data in users: @@ -153,4 +153,3 @@ class UserFixture: def load(cls): cls.load_users(cls, cls.ADMINS, "full_access_group") cls.load_users(cls, cls.STAFF, "cisa_analysts_group") - diff --git a/src/registrar/migrations/0034_alter_user_options.py b/src/registrar/migrations/0034_alter_user_options.py index 633bdd912..06bcaa91e 100644 --- a/src/registrar/migrations/0034_alter_user_options.py +++ b/src/registrar/migrations/0034_alter_user_options.py @@ -13,6 +13,7 @@ class Migration(migrations.Migration): name="user", options={ "permissions": [ + ("analyst_access_permission", "Analyst Access Permission"), ("full_access_permission", "Full Access Permission"), ] }, diff --git a/src/registrar/migrations/0035_contenttypes_permissions.py b/src/registrar/migrations/0035_contenttypes_permissions.py index 18e0348c0..67c792fa3 100644 --- a/src/registrar/migrations/0035_contenttypes_permissions.py +++ b/src/registrar/migrations/0035_contenttypes_permissions.py @@ -1,12 +1,13 @@ # From mithuntnt's answer on: # https://stackoverflow.com/questions/26464838/getting-model-contenttype-in-migration-django-1-7 -# The problem is that ContentType and Permission objects are not already created -# while we're still running migrations, so we'll go ahead and speen up that process +# The problem is that ContentType and Permission objects are not already created +# while we're still running migrations, so we'll go ahead and speed up that process # a bit before we attempt to create groups which require Permissions and ContentType. from django.conf import settings from django.db import migrations + def create_all_contenttypes(**kwargs): from django.apps import apps from django.contrib.contenttypes.management import create_contenttypes @@ -14,6 +15,7 @@ def create_all_contenttypes(**kwargs): for app_config in apps.get_app_configs(): create_contenttypes(app_config, **kwargs) + def create_all_permissions(**kwargs): from django.contrib.auth.management import create_permissions from django.apps import apps @@ -21,20 +23,21 @@ def create_all_permissions(**kwargs): for app_config in apps.get_app_configs(): create_permissions(app_config, **kwargs) + def forward(apps, schema_editor): create_all_contenttypes() create_all_permissions() + def backward(apps, schema_editor): pass + class Migration(migrations.Migration): dependencies = [ migrations.swappable_dependency(settings.AUTH_USER_MODEL), - ('contenttypes', '0002_remove_content_type_name'), + ("contenttypes", "0002_remove_content_type_name"), ("registrar", "0034_alter_user_options"), ] - operations = [ - migrations.RunPython(forward, backward) - ] \ No newline at end of file + operations = [migrations.RunPython(forward, backward)] diff --git a/src/registrar/migrations/0036_create_groups.py b/src/registrar/migrations/0036_create_groups.py index 7d8f5ceb5..ef1034746 100644 --- a/src/registrar/migrations/0036_create_groups.py +++ b/src/registrar/migrations/0036_create_groups.py @@ -1,22 +1,30 @@ # This migration creates the create_full_access_group and create_cisa_analyst_group groups +# It is dependent on 0035 (which populates ContentType and Permissions) # If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS # in the user_group model then: # step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions # step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups # step 3: fake run the latest migration in the migrations list -# Alternatively: +# Alternatively: # Only step: duplicate the migtation that loads data and run: docker-compose exec app ./manage.py migrate from django.db import migrations from registrar.models import UserGroup +def create_groups(): + UserGroup.create_cisa_analyst_group() + UserGroup.create_full_access_group() + + class Migration(migrations.Migration): dependencies = [ ("registrar", "0035_contenttypes_permissions"), ] operations = [ - migrations.RunPython(UserGroup.create_cisa_analyst_group, reverse_code=migrations.RunPython.noop, atomic=True), - migrations.RunPython(UserGroup.create_full_access_group, reverse_code=migrations.RunPython.noop, atomic=True), + migrations.RunPython( + create_groups, # noqa + reverse_code=migrations.RunPython.noop, + atomic=True, + ), ] - diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2c7f8703c..fe978b4b6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -675,7 +675,7 @@ class Domain(TimeStampedModel, DomainHelper): max_length=21, choices=State.choices, default=State.UNKNOWN, - protected=True, # cannot change state directly, particularly in Django admin + protected=False, # cannot change state directly, particularly in Django admin help_text="Very basic info about the lifecycle of this domain object", ) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a21897085..acf59cb68 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -81,8 +81,9 @@ class User(AbstractUser): logger.warn( "Failed to retrieve invitation %s", invitation, exc_info=True ) - + class Meta: permissions = [ + ("analyst_access_permission", "Analyst Access Permission"), ("full_access_permission", "Full Access Permission"), ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 0aabeec82..b6f5b41b2 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -3,14 +3,15 @@ import logging logger = logging.getLogger(__name__) -class UserGroup(Group): +class UserGroup(Group): class Meta: verbose_name = "User group" verbose_name_plural = "User groups" - + def create_cisa_analyst_group(apps, schema_editor): - + """This method gets run from a data migration.""" + # Hard to pass self to these methods as the calls from migrations # are only expecting apps and schema_editor, so we'll just define # apps, schema_editor in the local scope instead @@ -20,7 +21,11 @@ class UserGroup(Group): "model": "logentry", "permissions": ["view_logentry"], }, - {"app_label": "registrar", "model": "contact", "permissions": ["view_contact"]}, + { + "app_label": "registrar", + "model": "contact", + "permissions": ["view_contact"], + }, { "app_label": "registrar", "model": "domaininformation", @@ -31,16 +36,24 @@ class UserGroup(Group): "model": "domainapplication", "permissions": ["change_domainapplication"], }, - {"app_label": "registrar", "model": "domain", "permissions": ["view_domain"]}, + { + "app_label": "registrar", + "model": "domain", + "permissions": ["view_domain"], + }, { "app_label": "registrar", "model": "draftdomain", "permissions": ["change_draftdomain"], }, - {"app_label": "registrar", "model": "user", "permissions": ["change_user"]}, + { + "app_label": "registrar", + "model": "user", + "permissions": ["analyst_access_permission", "change_user"], + }, ] - - # Avoid error: You can't execute queries until the end + + # Avoid error: You can't execute queries until the end # of the 'atomic' block. # From django docs: # https://docs.djangoproject.com/en/4.2/topics/migrations/#data-migrations @@ -49,15 +62,15 @@ class UserGroup(Group): ContentType = apps.get_model("contenttypes", "ContentType") Permission = apps.get_model("auth", "Permission") UserGroup = apps.get_model("registrar", "UserGroup") - + logger.info("Going to create the Analyst Group") try: cisa_analysts_group, _ = UserGroup.objects.get_or_create( name="cisa_analysts_group", ) - + cisa_analysts_group.permissions.clear() - + for permission in CISA_ANALYST_GROUP_PERMISSIONS: app_label = permission["app_label"] model_name = permission["model"] @@ -67,19 +80,17 @@ class UserGroup(Group): content_type = ContentType.objects.get( app_label=app_label, model=model_name ) - + # Retrieve the permissions based on their codenames permissions = Permission.objects.filter( content_type=content_type, codename__in=permissions ) - + # Assign the permissions to the group cisa_analysts_group.permissions.add(*permissions) # Convert the permissions QuerySet to a list of codenames - permission_list = list( - permissions.values_list("codename", flat=True) - ) + permission_list = list(permissions.values_list("codename", flat=True)) logger.debug( app_label @@ -92,14 +103,18 @@ class UserGroup(Group): ) cisa_analysts_group.save() - logger.debug("CISA Analyt permissions added to group " + cisa_analysts_group.name) + logger.debug( + "CISA Analyt permissions added to group " + cisa_analysts_group.name + ) except Exception as e: logger.error(f"Error creating analyst permissions group: {e}") - + def create_full_access_group(apps, schema_editor): + """This method gets run from a data migration.""" + Permission = apps.get_model("auth", "Permission") UserGroup = apps.get_model("registrar", "UserGroup") - + logger.info("Going to create the Full Access Group") try: full_access_group, _ = UserGroup.objects.get_or_create( @@ -107,10 +122,10 @@ class UserGroup(Group): ) # Get all available permissions all_permissions = Permission.objects.all() - + # Assign all permissions to the group full_access_group.permissions.add(*all_permissions) - + full_access_group.save() logger.debug("All permissions added to group " + full_access_group.name) except Exception as e: diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py new file mode 100644 index 000000000..14228a491 --- /dev/null +++ b/src/registrar/tests/test_migrations.py @@ -0,0 +1,51 @@ +from django.test import TestCase + +from registrar.models import ( + UserGroup, +) +import logging + +logger = logging.getLogger(__name__) + + +class TestGroups(TestCase): + def test_groups_created(self): + """The test enviroment contains data that was created in migration, + so we are able to test groups and permissions. + + - Test cisa_analysts_group and full_access_group created + - Test permissions on full_access_group + """ + + # Get the UserGroup objects + cisa_analysts_group = UserGroup.objects.get(name="cisa_analysts_group") + full_access_group = UserGroup.objects.get(name="full_access_group") + + # Assert that the cisa_analysts_group exists in the database + self.assertQuerysetEqual( + UserGroup.objects.filter(name="cisa_analysts_group"), [cisa_analysts_group] + ) + + # Assert that the full_access_group exists in the database + self.assertQuerysetEqual( + UserGroup.objects.filter(name="full_access_group"), [full_access_group] + ) + + # Test permissions for cisa_analysts)group + # Define the expected permission codenames + expected_permissions = [ + "view_logentry", + "view_contact", + "view_domain", + "change_domainapplication", + "change_domaininformation", + "change_draftdomain", + "analyst_access_permission", + "change_user", + ] + + # Get the codenames of actual permissions associated with the group + actual_permissions = [p.codename for p in cisa_analysts_group.permissions.all()] + + # Assert that the actual permissions match the expected permissions + self.assertListEqual(actual_permissions, expected_permissions)