From 1f81443ea33376f3a92577800b3210b8273cac29 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Mon, 18 Dec 2023 21:54:43 -0800 Subject: [PATCH 1/9] Add restriction to withdraw domain application --- src/registrar/views/application.py | 6 +++--- src/registrar/views/utility/__init__.py | 1 + src/registrar/views/utility/mixins.py | 15 +++++++++++++ .../views/utility/permission_views.py | 21 +++++++++++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index 0a6eb5b7b..f18ba1310 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -13,7 +13,7 @@ from registrar.models import DomainApplication from registrar.utility import StrEnum from registrar.views.utility import StepsHelper -from .utility import DomainApplicationPermissionView, ApplicationWizardPermissionView +from .utility import DomainApplicationPermissionView, DomainApplicationPermissionWithdrawView, ApplicationWizardPermissionView logger = logging.getLogger(__name__) @@ -544,7 +544,7 @@ class ApplicationStatus(DomainApplicationPermissionView): template_name = "application_status.html" -class ApplicationWithdrawConfirmation(DomainApplicationPermissionView): +class ApplicationWithdrawConfirmation(DomainApplicationPermissionWithdrawView): """This page will ask user to confirm if they want to withdraw The DomainApplicationPermissionView restricts access so that only the @@ -554,7 +554,7 @@ class ApplicationWithdrawConfirmation(DomainApplicationPermissionView): template_name = "application_withdraw_confirmation.html" -class ApplicationWithdrawn(DomainApplicationPermissionView): +class ApplicationWithdrawn(DomainApplicationPermissionWithdrawView): # this view renders no template template_name = "" diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 71d3edb91..3d1a64628 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -4,6 +4,7 @@ from .always_404 import always_404 from .permission_views import ( DomainPermissionView, DomainApplicationPermissionView, + DomainApplicationPermissionWithdrawView, DomainInvitationPermissionDeleteView, ApplicationWizardPermissionView, ) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 37c0f6e98..cb6d89f7d 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -154,6 +154,21 @@ class DomainApplicationPermission(PermissionsLoginMixin): return True +class DomainApplicationPermissionWithdraw(DomainApplicationPermission): + + """Does the logged-in user have access to withdraw this domain application?""" + + def has_permission(self): + """Check if this user has access to withdraw this domain application. + """ + if not self.request.user.is_authenticated: + return False + + # only users with admin role can withdraw a domain request + if self.request.user.is_restricted(): + return False + + class ApplicationWizardPermission(PermissionsLoginMixin): """Does the logged-in user have permission to start or edit an application?""" diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 0e5d89ee2..e0fbc6cf6 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -8,6 +8,7 @@ from registrar.models import Domain, DomainApplication, DomainInvitation from .mixins import ( DomainPermission, DomainApplicationPermission, + DomainApplicationPermissionWithdraw, DomainInvitationPermission, ApplicationWizardPermission, ) @@ -74,6 +75,26 @@ class DomainApplicationPermissionView(DomainApplicationPermission, DetailView, a raise NotImplementedError +class DomainApplicationPermissionWithdrawView(DomainApplicationPermissionWithdraw, DetailView, abc.ABC): + + """Abstract base view for domain application withdraw function + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ + + # DetailView property for what model this is viewing + model = DomainApplication + # variable name in template context for the model object + context_object_name = "domainapplicationwithdraw" + + # Abstract property enforces NotImplementedError on an attribute. + @property + @abc.abstractmethod + def template_name(self): + raise NotImplementedError + + class ApplicationWizardPermissionView(ApplicationWizardPermission, TemplateView, abc.ABC): """Abstract base view for the application form that enforces permissions From 1ec27580a0e68f1529431f0636cfeff653cbe50a Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:00:06 -0800 Subject: [PATCH 2/9] Add True value to withdraw permission check --- src/registrar/views/utility/mixins.py | 2 ++ src/registrar/views/utility/permission_views.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index cb6d89f7d..ffc498958 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -168,6 +168,8 @@ class DomainApplicationPermissionWithdraw(DomainApplicationPermission): if self.request.user.is_restricted(): return False + return True + class ApplicationWizardPermission(PermissionsLoginMixin): diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index e0fbc6cf6..1798ec79d 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -86,7 +86,7 @@ class DomainApplicationPermissionWithdrawView(DomainApplicationPermissionWithdra # DetailView property for what model this is viewing model = DomainApplication # variable name in template context for the model object - context_object_name = "domainapplicationwithdraw" + context_object_name = "domainapplication" # Abstract property enforces NotImplementedError on an attribute. @property From 88dc5cb436dbc8cee9ff444fd6a0418d741f9cdf Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:00:57 -0800 Subject: [PATCH 3/9] Correct information in comments --- src/registrar/views/utility/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index ffc498958..26a02ce47 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -164,7 +164,7 @@ class DomainApplicationPermissionWithdraw(DomainApplicationPermission): if not self.request.user.is_authenticated: return False - # only users with admin role can withdraw a domain request + # Restricted users should not be able to withdraw domain requests if self.request.user.is_restricted(): return False From bc9bc704654e0f1fe0435423d1457fa1dad06b95 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Tue, 19 Dec 2023 16:48:53 -0800 Subject: [PATCH 4/9] Fix bug in withdraw permission --- src/registrar/views/utility/mixins.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 26a02ce47..853824d5a 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -154,7 +154,7 @@ class DomainApplicationPermission(PermissionsLoginMixin): return True -class DomainApplicationPermissionWithdraw(DomainApplicationPermission): +class DomainApplicationPermissionWithdraw(PermissionsLoginMixin): """Does the logged-in user have access to withdraw this domain application?""" @@ -164,6 +164,12 @@ class DomainApplicationPermissionWithdraw(DomainApplicationPermission): if not self.request.user.is_authenticated: return False + # user needs to be the creator of the application + # this query is empty if there isn't a domain application with this + # id and this user as creator + if not DomainApplication.objects.filter(creator=self.request.user, id=self.kwargs["pk"]).exists(): + return False + # Restricted users should not be able to withdraw domain requests if self.request.user.is_restricted(): return False From a7b460f8c4702f7569055bbd138f125eb2c121ed Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 20 Dec 2023 09:36:59 -0800 Subject: [PATCH 5/9] Fix lint errors --- src/registrar/views/application.py | 6 +++++- src/registrar/views/utility/mixins.py | 3 +-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index f18ba1310..41052e164 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -13,7 +13,11 @@ from registrar.models import DomainApplication from registrar.utility import StrEnum from registrar.views.utility import StepsHelper -from .utility import DomainApplicationPermissionView, DomainApplicationPermissionWithdrawView, ApplicationWizardPermissionView +from .utility import ( + DomainApplicationPermissionView, + DomainApplicationPermissionWithdrawView, + ApplicationWizardPermissionView, +) logger = logging.getLogger(__name__) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 853824d5a..03c11bc5f 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -159,8 +159,7 @@ class DomainApplicationPermissionWithdraw(PermissionsLoginMixin): """Does the logged-in user have access to withdraw this domain application?""" def has_permission(self): - """Check if this user has access to withdraw this domain application. - """ + """Check if this user has access to withdraw this domain application.""" if not self.request.user.is_authenticated: return False From a205b9e25eac29859d81455ac2d541df6976f1ce Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 20 Dec 2023 11:09:02 -0800 Subject: [PATCH 6/9] Add test for withdrawing as restricted user --- src/registrar/tests/test_views.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 57fa03f52..e417eb697 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -2229,6 +2229,37 @@ class TestApplicationStatus(TestWithUser, WebTest): home_page = self.app.get("/") self.assertContains(home_page, "Withdrawn") + def test_application_withdraw_no_permissions(self): + """Can't withdraw applications as a restricted user.""" + self.user.status = User.RESTRICTED + self.user.save() + application = completed_application(status=DomainApplication.ApplicationStatus.SUBMITTED, user=self.user) + application.save() + + home_page = self.app.get("/") + self.assertContains(home_page, "city.gov") + # click the "Manage" link + detail_page = home_page.click("Manage", index=0) + self.assertContains(detail_page, "city.gov") + self.assertContains(detail_page, "city1.gov") + self.assertContains(detail_page, "Chief Tester") + self.assertContains(detail_page, "testy@town.com") + self.assertContains(detail_page, "Admin Tester") + self.assertContains(detail_page, "Status:") + # Restricted user trying to withdraw results in 403 error + with less_console_noise(): + for url_name in [ + "application-withdraw-confirmation", + "application-withdrawn", + ]: + with self.subTest(url_name=url_name): + page = self.client.get(reverse(url_name, kwargs={"pk": application.pk})) + self.assertEqual(page.status_code, 403) + + # reset user status from restricted to unrestricted + self.user.status = None + self.user.save() + def test_application_status_no_permissions(self): """Can't access applications without being the creator.""" application = completed_application(status=DomainApplication.ApplicationStatus.SUBMITTED, user=self.user) From c47c366e562033646dfe424cc1744114c38be8e3 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 20 Dec 2023 11:09:25 -0800 Subject: [PATCH 7/9] Fix typo in user_group logger --- src/registrar/models/user_group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 0f12a2e84..a733239c7 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -114,7 +114,7 @@ class UserGroup(Group): ) cisa_analysts_group.save() - logger.debug("CISA Analyt permissions added to group " + cisa_analysts_group.name) + logger.debug("CISA Analyst permissions added to group " + cisa_analysts_group.name) except Exception as e: logger.error(f"Error creating analyst permissions group: {e}") From 9a9b29e271f7cd9ab4ae56155ce4ababe4cfed55 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 20 Dec 2023 14:32:59 -0800 Subject: [PATCH 8/9] Remove unneeded user reset in views test --- src/registrar/tests/test_views.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index e417eb697..b7ae8c613 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -2256,10 +2256,6 @@ class TestApplicationStatus(TestWithUser, WebTest): page = self.client.get(reverse(url_name, kwargs={"pk": application.pk})) self.assertEqual(page.status_code, 403) - # reset user status from restricted to unrestricted - self.user.status = None - self.user.save() - def test_application_status_no_permissions(self): """Can't access applications without being the creator.""" application = completed_application(status=DomainApplication.ApplicationStatus.SUBMITTED, user=self.user) From 0ab4e67ec753a51ca5246d97b0bb2f65eaab10c5 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 20 Dec 2023 14:40:24 -0800 Subject: [PATCH 9/9] Refactor class descriptions on mixins --- src/registrar/views/utility/mixins.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 03c11bc5f..5ff291d69 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -26,7 +26,8 @@ class PermissionsLoginMixin(PermissionRequiredMixin): class DomainPermission(PermissionsLoginMixin): - """Does the logged-in user have access to this domain?""" + """Permission mixin that redirects to domain if user has access, + otherwise 403""" def has_permission(self): """Check if this user has access to this domain. @@ -134,7 +135,8 @@ class DomainPermission(PermissionsLoginMixin): class DomainApplicationPermission(PermissionsLoginMixin): - """Does the logged-in user have access to this domain application?""" + """Permission mixin that redirects to domain application if user + has access, otherwise 403""" def has_permission(self): """Check if this user has access to this domain application. @@ -156,7 +158,8 @@ class DomainApplicationPermission(PermissionsLoginMixin): class DomainApplicationPermissionWithdraw(PermissionsLoginMixin): - """Does the logged-in user have access to withdraw this domain application?""" + """Permission mixin that redirects to withdraw action on domain application + if user has access, otherwise 403""" def has_permission(self): """Check if this user has access to withdraw this domain application.""" @@ -178,7 +181,8 @@ class DomainApplicationPermissionWithdraw(PermissionsLoginMixin): class ApplicationWizardPermission(PermissionsLoginMixin): - """Does the logged-in user have permission to start or edit an application?""" + """Permission mixin that redirects to start or edit domain application if + user has access, otherwise 403""" def has_permission(self): """Check if this user has permission to start or edit an application. @@ -195,7 +199,8 @@ class ApplicationWizardPermission(PermissionsLoginMixin): class DomainInvitationPermission(PermissionsLoginMixin): - """Does the logged-in user have access to this domain invitation? + """Permission mixin that redirects to domain invitation if user has + access, otherwise 403" A user has access to a domain invitation if they have a role on the associated domain.