Merge pull request #651 from cisagov/nmb/544-authz

Add abstract views that enforce permissions

src/registrar/config/urls.py

@@ -59,12 +59,12 @@ urlpatterns = [
     ),
     path(
         "application/<int:pk>/withdraw",
-        views.ApplicationWithdraw.as_view(),
+        views.ApplicationWithdrawConfirmation.as_view(),
         name="application-withdraw-confirmation",
     ),
     path(
         "application/<int:pk>/withdrawconfirmed",
-        views.ApplicationWithdraw.updatestatus,
+        views.ApplicationWithdrawn.as_view(),
         name="application-withdrawn",
     ),
     path("health/", views.health),

src/registrar/templates/application_status.html

@@ -31,7 +31,7 @@
     <p> <b class="review__step__name">Last updated:</b> {{domainapplication.updated_at|date:"F j, Y"}}<br>
     <b class="review__step__name">Request #:</b> {{domainapplication.id}}</p>
     <p>{% include "includes/domain_application.html" %}</p>
-    <p><a href="{% url 'application-withdraw-confirmation' domainapplication.id %}" class="usa-button usa-button--outline withdraw_outline">
+    <p><a href="{% url 'application-withdraw-confirmation' pk=domainapplication.id %}" class="usa-button usa-button--outline withdraw_outline">
       Withdraw request</a>
     </p>
   </div>

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)

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
@@ -14,7 +13,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 +477,31 @@ class Finished(ApplicationWizard):
         return render(self.request, self.template_name, context)
 
 
-class ApplicationStatus(generic.DetailView):
+class ApplicationStatus(DomainApplicationPermissionView):
-    model = DomainApplication
     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):
+    The DomainApplicationPermissionView restricts access so that only the
-    model = DomainApplication
+    `creator` of the application may withdraw it.
-    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
     """
 
-    def updatestatus(request, pk):
+    template_name = "application_withdraw_confirmation.html"
-        """If user click on withdraw confirm button, it will be updated to withdraw
+
-        and send back to homepage"""
+
-        application = DomainApplication.objects.get(id=pk)
+class ApplicationWithdrawn(DomainApplicationPermissionView):
+    # this view renders no template
+    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.
+        """
+        application = DomainApplication.objects.get(id=self.kwargs["pk"])
         application.status = "withdrawn"
         application.save()
         return HttpResponseRedirect(reverse("home"))

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
 
@@ -7,35 +12,30 @@ 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 FormMixin
-from django.views.generic.edit import DeleteView, 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
-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 +96,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 +139,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 +155,6 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView):
     """
 
     template_name = "domain_add_user.html"
-    model = Domain
     form_class = DomainAddUserForm
 
     def get_success_url(self):
@@ -239,8 +234,9 @@ class DomainAddUserView(DomainPermission, FormMixin, DetailView):
         return redirect(self.get_success_url())
 
 
-class DomainInvitationDeleteView(SuccessMessageMixin, DeleteView):
+class DomainInvitationDeleteView(
-    model = DomainInvitation
+    DomainInvitationPermissionDeleteView, SuccessMessageMixin
+):
     object: DomainInvitation  # workaround for type mismatch in DeleteView
 
     def get_success_url(self):

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,
+)

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

src/registrar/views/utility/permission_views.py

@@ -0,0 +1,69 @@
+"""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