From 10185553de21bfc882f9a439df9817e25805408b Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Wed, 24 May 2023 15:28:52 -0500 Subject: [PATCH 1/2] Add abstract views that enforce permissions. --- src/registrar/config/urls.py | 4 +- .../templates/application_status.html | 2 +- src/registrar/tests/test_views.py | 40 ++++++++++- src/registrar/views/application.py | 38 ++++++----- src/registrar/views/domain.py | 33 ++++----- src/registrar/views/utility/__init__.py | 7 +- src/registrar/views/utility/mixins.py | 47 ++++++++++++- .../views/utility/permission_views.py | 67 +++++++++++++++++++ 8 files changed, 194 insertions(+), 44 deletions(-) create mode 100644 src/registrar/views/utility/permission_views.py diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index e903b4e8f..126be0293 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -59,12 +59,12 @@ urlpatterns = [ ), path( "application//withdraw", - views.ApplicationWithdraw.as_view(), + views.ApplicationWithdrawConfirmation.as_view(), name="application-withdraw-confirmation", ), path( "application//withdrawconfirmed", - views.ApplicationWithdraw.updatestatus, + views.ApplicationWithdrawn.as_view(), name="application-withdrawn", ), path("health/", views.health), diff --git a/src/registrar/templates/application_status.html b/src/registrar/templates/application_status.html index fadd8ec36..054e04460 100644 --- a/src/registrar/templates/application_status.html +++ b/src/registrar/templates/application_status.html @@ -31,7 +31,7 @@

Last updated: {{domainapplication.updated_at|date:"F j, Y"}}
Request #: {{domainapplication.id}}

{% include "includes/domain_application.html" %}

-

+

Withdraw request

diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 959183f34..928ba2b6f 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -1136,7 +1136,7 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.assertContains(response, "Add another user") def test_domain_user_add_form(self): - """Adding a user works.""" + """Adding an existing user works.""" other_user, _ = get_user_model().objects.get_or_create( email="mayor@igorville.gov" ) @@ -1219,6 +1219,22 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): with self.assertRaises(DomainInvitation.DoesNotExist): DomainInvitation.objects.get(id=invitation.id) + def test_domain_invitation_cancel_no_permissions(self): + """Posting to the delete view as a different user should fail.""" + EMAIL = "mayor@igorville.gov" + invitation, _ = DomainInvitation.objects.get_or_create( + domain=self.domain, email=EMAIL + ) + + other_user = User() + other_user.save() + self.client.force_login(other_user) + with less_console_noise(): # permission denied makes console errors + result = self.client.post( + reverse("invitation-delete", kwargs={"pk": invitation.id}) + ) + self.assertEqual(result.status_code, 403) + @boto3_mocking.patching def test_domain_invitation_flow(self): """Send an invitation to a new user, log in and load the dashboard.""" @@ -1330,6 +1346,7 @@ class TestApplicationStatus(TestWithUser, WebTest): def setUp(self): super().setUp() self.app.set_user(self.user.username) + self.client.force_login(self.user) def _completed_application( self, @@ -1443,3 +1460,24 @@ class TestApplicationStatus(TestWithUser, WebTest): ) home_page = self.app.get("/") self.assertContains(home_page, "Withdrawn") + + def test_application_status_no_permissions(self): + """Can't access applications without being the creator.""" + application = self._completed_application() + other_user = User() + other_user.save() + application.creator = other_user + application.save() + + # PermissionDeniedErrors make lots of noise in test output + with less_console_noise(): + for url_name in [ + "application-status", + "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) diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index f08c80a24..e0c9773f6 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -14,7 +14,7 @@ from registrar.models import DomainApplication from registrar.utility import StrEnum from registrar.views.utility import StepsHelper -from .utility import DomainPermission +from .utility import DomainApplicationPermissionView logger = logging.getLogger(__name__) @@ -478,29 +478,31 @@ class Finished(ApplicationWizard): return render(self.request, self.template_name, context) -class ApplicationStatus(generic.DetailView): - model = DomainApplication +class ApplicationStatus(DomainApplicationPermissionView): template_name = "application_status.html" - def get_context_data(self, **kwargs): - """Get context details to process information from application""" - context = super(ApplicationStatus, self).get_context_data(**kwargs) - return context +class ApplicationWithdrawConfirmation(DomainApplicationPermissionView): + """This page will ask user to confirm if they want to withdraw -class ApplicationWithdraw(LoginRequiredMixin, generic.DetailView, DomainPermission): - model = DomainApplication - template_name = "application_withdraw_confirmation.html" - """ The page above will display asking user to confirm if they want to withdraw; - - Note it uses "DomainPermission" from Domain to ensure that the person who - applied only have access to withdraw the request + The DomainApplicationPermissionView restricts access so that only the + `creator` of the application may withdraw it. """ - def updatestatus(request, pk): - """If user click on withdraw confirm button, it will be updated to withdraw - and send back to homepage""" - application = DomainApplication.objects.get(id=pk) + template_name = "application_withdraw_confirmation.html" + + +class ApplicationWithdrawn(DomainApplicationPermissionView): + # this view renders no template + template_name = None + + def get(self, *args, **kwargs): + """View class that does the actual withdrawing. + + If user click on withdraw confirm button, this view updates the status to withdraw + and send back to homepage. + """ + application = DomainApplication.objects.get(id=self.kwargs["pk"]) application.status = "withdrawn" application.save() return HttpResponseRedirect(reverse("home")) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index b5962c398..e954ffefc 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1,4 +1,9 @@ -"""View for a single Domain.""" +"""Views for a single Domain. + +Authorization is handled by the `DomainPermissionView`. To ensure that only +authorized users can see information on a domain, every view here should +inherit from `DomainPermissionView` (or DomainInvitationPermissionDeleteView). +""" import logging @@ -14,28 +19,24 @@ from registrar.models import Domain, DomainInvitation, User, UserDomainRole from ..forms import DomainAddUserForm, NameserverFormset, DomainSecurityEmailForm from ..utility.email import send_templated_email, EmailSendingError -from .utility import DomainPermission +from .utility import DomainPermissionView, DomainInvitationPermissionDeleteView logger = logging.getLogger(__name__) -class DomainView(DomainPermission, DetailView): +class DomainView(DomainPermissionView): """Domain detail overview page.""" - model = Domain template_name = "domain_detail.html" - context_object_name = "domain" -class DomainNameserversView(DomainPermission, FormMixin, DetailView): +class DomainNameserversView(DomainPermissionView, FormMixin): """Domain nameserver editing view.""" - model = Domain template_name = "domain_nameservers.html" - context_object_name = "domain" form_class = NameserverFormset def get_initial(self): @@ -96,13 +97,11 @@ class DomainNameserversView(DomainPermission, FormMixin, DetailView): return super().form_valid(formset) -class DomainSecurityEmailView(DomainPermission, FormMixin, DetailView): +class DomainSecurityEmailView(DomainPermissionView, FormMixin): """Domain security email editing view.""" - model = Domain template_name = "domain_security_email.html" - context_object_name = "domain" form_class = DomainSecurityEmailForm def get_initial(self): @@ -141,16 +140,14 @@ class DomainSecurityEmailView(DomainPermission, FormMixin, DetailView): return redirect(self.get_success_url()) -class DomainUsersView(DomainPermission, DetailView): +class DomainUsersView(DomainPermissionView): """User management page in the domain details.""" - model = Domain template_name = "domain_users.html" - context_object_name = "domain" -class DomainAddUserView(DomainPermission, FormMixin, DetailView): +class DomainAddUserView(DomainPermissionView, FormMixin): """Inside of a domain's user management, a form for adding users. @@ -159,7 +156,6 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): """ template_name = "domain_add_user.html" - model = Domain form_class = DomainAddUserForm def get_success_url(self): @@ -239,10 +235,7 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView): return redirect(self.get_success_url()) -class DomainInvitationDeleteView(SuccessMessageMixin, DeleteView): - model = DomainInvitation - object: DomainInvitation # workaround for type mismatch in DeleteView - +class DomainInvitationDeleteView(DomainInvitationPermissionDeleteView): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.domain.id}) diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index a492b5cb3..6c656c614 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -1,3 +1,8 @@ from .steps_helper import StepsHelper from .always_404 import always_404 -from .mixins import DomainPermission + +from .permission_views import ( + DomainPermissionView, + DomainApplicationPermissionView, + DomainInvitationPermissionDeleteView, +) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 71129a1f6..890b10a02 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -2,7 +2,7 @@ from django.contrib.auth.mixins import PermissionRequiredMixin -from registrar.models import UserDomainRole +from registrar.models import UserDomainRole, DomainApplication, DomainInvitation class PermissionsLoginMixin(PermissionRequiredMixin): @@ -35,3 +35,48 @@ class DomainPermission(PermissionsLoginMixin): # if we need to check more about the nature of role, do it here. return True + + +class DomainApplicationPermission(PermissionsLoginMixin): + + """Does the logged-in user have access to this domain application?""" + + def has_permission(self): + """Check if this user has access to this domain application. + + The user is in self.request.user and the domain needs to be looked + up from the domain's primary key in self.kwargs["pk"] + """ + 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 + + return True + + +class DomainInvitationPermission(PermissionsLoginMixin): + + """Does the logged-in user have access to this domain invitation? + + A user has access to a domain invitation if they have a role on the + associated domain. + """ + + def has_permission(self): + """Check if this user has a role on the domain of this invitation.""" + if not self.request.user.is_authenticated: + return False + + if not DomainInvitation.objects.filter( + id=self.kwargs["pk"], domain__permissions__user=self.request.user + ).exists(): + return False + + return True diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py new file mode 100644 index 000000000..d5f4525d8 --- /dev/null +++ b/src/registrar/views/utility/permission_views.py @@ -0,0 +1,67 @@ +"""View classes that enforce authorization.""" + +import abc # abstract base class + +from django.views.generic import DetailView, DeleteView + +from registrar.models import Domain, DomainApplication, DomainInvitation + +from .mixins import ( + DomainPermission, + DomainApplicationPermission, + DomainInvitationPermission, +) + + +class DomainPermissionView(DomainPermission, DetailView, abc.ABC): + + """Abstract base view for domains that enforces permissions. + + This abstract view cannot be instantiated. Actual views must specify `template_name`. + """ + + # DetailView property for what model this is viewing + model = Domain + # variable name in template context for the model object + context_object_name = "domain" + + # Abstract property enforces NotImplementedError on an attribute. + @property + @abc.abstractmethod + def template_name(self): + raise NotImplementedError + + +class DomainApplicationPermissionView(DomainApplicationPermission, DetailView, abc.ABC): + + """Abstract base view for domain applications that enforces permissions + + 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 = "domainapplication" + + # Abstract property enforces NotImplementedError on an attribute. + @property + @abc.abstractmethod + def template_name(self): + raise NotImplementedError + + +class DomainInvitationPermissionDeleteView( + DomainInvitationPermission, DeleteView, abc.ABC +): + + """Abstract view for deleting a domain invitation. + + This one is fairly specialized, but this is the only thing that we do + right now with domain invitations. We still have the full + `DomainInvitationPermission` class, but here we just pair it with a + DeleteView. + """ + + model = DomainInvitation + object: DomainInvitation # workaround for type mismatch in DeleteView From a277336a067aad3be0cd6d0fcdd6d2aa8e789d3b Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Wed, 24 May 2023 15:38:26 -0500 Subject: [PATCH 2/2] fix linting errors --- src/registrar/views/application.py | 7 +++---- src/registrar/views/domain.py | 11 +++++++---- src/registrar/views/utility/permission_views.py | 6 ++++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index e0c9773f6..b8454bf78 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -6,7 +6,6 @@ from django.shortcuts import redirect, render from django.urls import resolve, reverse from django.utils.translation import gettext_lazy as _ from django.views.generic import TemplateView -from django.views import generic from django.contrib import messages from registrar.forms import application_wizard as forms @@ -494,13 +493,13 @@ class ApplicationWithdrawConfirmation(DomainApplicationPermissionView): class ApplicationWithdrawn(DomainApplicationPermissionView): # this view renders no template - template_name = None + template_name = "" def get(self, *args, **kwargs): """View class that does the actual withdrawing. - If user click on withdraw confirm button, this view updates the status to withdraw - and send back to homepage. + If user click on withdraw confirm button, this view updates the status + to withdraw and send back to homepage. """ application = DomainApplication.objects.get(id=self.kwargs["pk"]) application.status = "withdrawn" diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index e954ffefc..361175295 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -12,10 +12,9 @@ from django.contrib.messages.views import SuccessMessageMixin from django.db import IntegrityError from django.shortcuts import redirect from django.urls import reverse -from django.views.generic import DetailView -from django.views.generic.edit import DeleteView, FormMixin +from django.views.generic.edit import FormMixin -from registrar.models import Domain, DomainInvitation, User, UserDomainRole +from registrar.models import DomainInvitation, User, UserDomainRole from ..forms import DomainAddUserForm, NameserverFormset, DomainSecurityEmailForm from ..utility.email import send_templated_email, EmailSendingError @@ -235,7 +234,11 @@ class DomainAddUserView(DomainPermissionView, FormMixin): return redirect(self.get_success_url()) -class DomainInvitationDeleteView(DomainInvitationPermissionDeleteView): +class DomainInvitationDeleteView( + DomainInvitationPermissionDeleteView, SuccessMessageMixin +): + object: DomainInvitation # workaround for type mismatch in DeleteView + def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.domain.id}) diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index d5f4525d8..e52ed102c 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -17,7 +17,8 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): """Abstract base view for domains that enforces permissions. - This abstract view cannot be instantiated. Actual views must specify `template_name`. + This abstract view cannot be instantiated. Actual views must specify + `template_name`. """ # DetailView property for what model this is viewing @@ -36,7 +37,8 @@ class DomainApplicationPermissionView(DomainApplicationPermission, DetailView, a """Abstract base view for domain applications that enforces permissions - This abstract view cannot be instantiated. Actual views must specify `template_name`. + This abstract view cannot be instantiated. Actual views must specify + `template_name`. """ # DetailView property for what model this is viewing