From d33cc1c8ccf51d8b7d0c7365ba3c990463ec2a87 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 12:17:59 -0600 Subject: [PATCH 1/7] Remove perms --- .../migrations/0081_create_groups_v10.py | 37 +++++++++++++++++++ src/registrar/models/user_group.py | 5 --- 2 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 src/registrar/migrations/0081_create_groups_v10.py diff --git a/src/registrar/migrations/0081_create_groups_v10.py b/src/registrar/migrations/0081_create_groups_v10.py new file mode 100644 index 000000000..d65b6dbd2 --- /dev/null +++ b/src/registrar/migrations/0081_create_groups_v10.py @@ -0,0 +1,37 @@ +# This migration creates the create_full_access_group and create_cisa_analyst_group groups +# It is dependent on 0079 (which populates federal agencies) +# If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS +# in the user_group model then: +# [NOT RECOMMENDED] +# 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 +# [RECOMMENDED] +# Alternatively: +# step 1: duplicate the migration that loads data +# step 2: docker-compose exec app ./manage.py migrate + +from django.db import migrations +from registrar.models import UserGroup +from typing import Any + + +# For linting: RunPython expects a function reference, +# so let's give it one +def create_groups(apps, schema_editor) -> Any: + UserGroup.create_cisa_analyst_group(apps, schema_editor) + UserGroup.create_full_access_group(apps, schema_editor) + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0080_create_groups_v09"), + ] + + operations = [ + migrations.RunPython( + create_groups, + reverse_code=migrations.RunPython.noop, + atomic=True, + ), + ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 2aa2f642e..e8636a462 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -26,11 +26,6 @@ class UserGroup(Group): "model": "contact", "permissions": ["change_contact"], }, - { - "app_label": "registrar", - "model": "domaininformation", - "permissions": ["change_domaininformation"], - }, { "app_label": "registrar", "model": "domainrequest", From b942d348928cde4feb7c796f3a386cac5a31561f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 12:58:17 -0600 Subject: [PATCH 2/7] Add has change perm --- src/registrar/admin.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5137300c7..e04509776 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1408,6 +1408,11 @@ class DomainInformationInline(admin.StackedInline): "submitter", ] + def has_change_permission(self, request, obj=None): + if request.user.has_perm("registrar.analyst_access_permission"): + return True + return super().has_change_permission(request, obj) + def formfield_for_manytomany(self, db_field, request, **kwargs): """customize the behavior of formfields with manytomany relationships. the customized behavior includes sorting of objects in lists as well as customizing helper text""" From 2150873ec1de8677dc0a8122d907945ead0eade7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 13:00:46 -0600 Subject: [PATCH 3/7] Update admin.py --- src/registrar/admin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e04509776..d16f5e315 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1409,6 +1409,8 @@ class DomainInformationInline(admin.StackedInline): ] def has_change_permission(self, request, obj=None): + """Custom has_change_permission override so that we can specify that + analysts can edit this through this inline, but not through the model normally""" if request.user.has_perm("registrar.analyst_access_permission"): return True return super().has_change_permission(request, obj) From b12669d7a5b52e61fac6feb9ea8a77a12da24049 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 14:27:58 -0600 Subject: [PATCH 4/7] Fix unit tests --- src/registrar/tests/test_admin.py | 8 ++++---- src/registrar/tests/test_migrations.py | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 7c0c81db4..dcffce6f8 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1960,8 +1960,8 @@ class TestDomainInformationAdmin(TestCase): # Get the other contact other_contact = domain_info.other_contacts.all().first() - p = "userpass" - self.client.login(username="staffuser", password=p) + p = "adminpass" + self.client.login(username="superuser", password=p) response = self.client.get( "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), @@ -2005,8 +2005,8 @@ class TestDomainInformationAdmin(TestCase): domain_request.approve() domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get() - p = "userpass" - self.client.login(username="staffuser", password=p) + p = "adminpass" + self.client.login(username="superuser", password=p) response = self.client.get( "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), follow=True, diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index bf3b09d0d..add65105a 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -34,7 +34,6 @@ class TestGroups(TestCase): "view_logentry", "change_contact", "view_domain", - "change_domaininformation", "add_domaininvitation", "view_domaininvitation", "change_domainrequest", From dd7b0fc0bc81bbe04fc9979ca9671e535da98b81 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:49:54 -0600 Subject: [PATCH 5/7] Unit tests --- src/registrar/tests/test_admin.py | 81 +++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index dcffce6f8..8e83dedde 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -129,6 +129,49 @@ class TestDomainAdmin(MockEppLib, WebTest): ) mock_add_message.assert_has_calls([expected_call], 1) + @less_console_noise_decorator + def test_analyst_can_see_inline_domain_information_in_domain_change_form(self): + """Tests if the analyst can still see the inline domain information form""" + + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + _domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + + # Creates a Domain and DomainInformation object + _domain_request.approve() + + domain_information = DomainInformation.objects.filter(domain_request=_domain_request).get() + domain_information.organization_name = "MonkeySeeMonkeyDo" + domain_information.save() + + # We use filter here rather than just domain_information.domain just to get the latest data. + domain = Domain.objects.filter(domain=domain_information.domain) + + p = "userpass" + self.client.login(username="staffuser", password=p) + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + + # Check for one of the inline headers + expected_header = ".gov domain" + self.assertContains(response, expected_header) + + # Test for data. We only need to test one since its all interconnected. + expected_organization_name = "MonkeySeeMonkeyDo" + self.assertContains(response, expected_organization_name) + @patch("registrar.admin.DomainAdmin._get_current_date", return_value=date(2024, 1, 1)) def test_extend_expiration_date_button_epp(self, mock_date_today): """ @@ -1982,6 +2025,44 @@ class TestDomainInformationAdmin(TestCase): expected_url = "Testy Tester" self.assertContains(response, expected_url) + @less_console_noise_decorator + def test_analyst_cant_access_domain_information(self): + """Ensures that analysts can't directly access the DomainInformation page through /admin""" + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + domain_request.approve() + domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get() + + p = "userpass" + self.client.login(username="staffuser", password=p) + response = self.client.get( + "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), + follow=True, + ) + + # Make sure that we're denied access + self.assertEqual(response.status_code, 403) + + # To make sure that its not a fluke, swap to an admin user + # and try to access the same page. This should succeed. + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_info.domain.name) + @less_console_noise_decorator def test_contact_fields_have_detail_table(self): """Tests if the contact fields have the detail table which displays title, email, and phone""" From 7ac203913e7126f152dba6621bb4f974629296a5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:53:58 -0600 Subject: [PATCH 6/7] Update test_admin.py --- src/registrar/tests/test_admin.py | 46 +++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 8e83dedde..49bd1f299 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -131,7 +131,7 @@ class TestDomainAdmin(MockEppLib, WebTest): @less_console_noise_decorator def test_analyst_can_see_inline_domain_information_in_domain_change_form(self): - """Tests if the analyst can still see the inline domain information form""" + """Tests if an analyst can still see the inline domain information form""" # Create fake creator _creator = User.objects.create( @@ -151,7 +151,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain=domain_information.domain) + domain = Domain.objects.filter(domain_info=domain_information) p = "userpass" self.client.login(username="staffuser", password=p) @@ -172,6 +172,48 @@ class TestDomainAdmin(MockEppLib, WebTest): expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name) + @less_console_noise_decorator + def test_admin_can_see_inline_domain_information_in_domain_change_form(self): + """Tests if an admin can still see the inline domain information form""" + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + _domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + + # Creates a Domain and DomainInformation object + _domain_request.approve() + + domain_information = DomainInformation.objects.filter(domain_request=_domain_request).get() + domain_information.organization_name = "MonkeySeeMonkeyDo" + domain_information.save() + + # We use filter here rather than just domain_information.domain just to get the latest data. + domain = Domain.objects.filter(domain_info=domain_information) + + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + + # Check for one of the inline headers + expected_header = ".gov domain" + self.assertContains(response, expected_header) + + # Test for data. We only need to test one since its all interconnected. + expected_organization_name = "MonkeySeeMonkeyDo" + self.assertContains(response, expected_organization_name) + @patch("registrar.admin.DomainAdmin._get_current_date", return_value=date(2024, 1, 1)) def test_extend_expiration_date_button_epp(self, mock_date_today): """ From c595405709bee9d556aa595bdcd6281bcbea4200 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:57:51 -0600 Subject: [PATCH 7/7] Fix unit tests --- src/registrar/tests/test_admin.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49bd1f299..f3c854aa0 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -151,7 +151,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain_info=domain_information) + domain = Domain.objects.filter(domain_info=domain_information).get() p = "userpass" self.client.login(username="staffuser", password=p) @@ -164,10 +164,6 @@ class TestDomainAdmin(MockEppLib, WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - # Check for one of the inline headers - expected_header = ".gov domain" - self.assertContains(response, expected_header) - # Test for data. We only need to test one since its all interconnected. expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name) @@ -193,7 +189,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain_info=domain_information) + domain = Domain.objects.filter(domain_info=domain_information).get() p = "adminpass" self.client.login(username="superuser", password=p) @@ -206,10 +202,6 @@ class TestDomainAdmin(MockEppLib, WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - # Check for one of the inline headers - expected_header = ".gov domain" - self.assertContains(response, expected_header) - # Test for data. We only need to test one since its all interconnected. expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name)