From 830cf667e7d900fc145716c1569610c838ec4dec Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 14 Mar 2024 11:36:29 -0600 Subject: [PATCH 1/6] Fix bug --- src/registrar/models/domain.py | 1 + src/registrar/views/utility/permission_views.py | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 079fce3bc..fa499893c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -325,6 +325,7 @@ class Domain(TimeStampedModel, DomainHelper): Subordinate hosts (something.your-domain.gov) MUST have IP addresses, while non-subordinate hosts MUST NOT. """ + raise ValueError("test") try: # attempt to retrieve hosts from registry and store in cache and db hosts = self._get_property("hosts") diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index f2752c3b5..f086747f8 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,6 +2,7 @@ import abc # abstract base class +from django.conf import settings from django.views.generic import DetailView, DeleteView, TemplateView from registrar.models import Domain, DomainRequest, DomainInvitation from registrar.models.user_domain_role import UserDomainRole @@ -31,6 +32,16 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): # variable name in template context for the model object context_object_name = "domain" + def dispatch(self, request, *args, **kwargs): + """ + Custom implementation of dispatch to ensure that 500 error pages (and others) + have access to the IS_PRODUCTION flag + """ + if "IS_PRODUCTION" not in request.session: + # Pass the production flag to the context + request.session["IS_PRODUCTION"] = settings.IS_PRODUCTION + return super().dispatch(request, *args, **kwargs) + # Adds context information for user permissions def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) From 1aefca21f00d3d9fbee3119ae009da4d629126ff Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 14 Mar 2024 12:37:02 -0600 Subject: [PATCH 2/6] Use "is_production" instead --- src/registrar/views/utility/permission_views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index f086747f8..fde01253e 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,8 +2,8 @@ import abc # abstract base class -from django.conf import settings from django.views.generic import DetailView, DeleteView, TemplateView +from registrar.context_processors import is_production from registrar.models import Domain, DomainRequest, DomainInvitation from registrar.models.user_domain_role import UserDomainRole @@ -39,7 +39,8 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): """ if "IS_PRODUCTION" not in request.session: # Pass the production flag to the context - request.session["IS_PRODUCTION"] = settings.IS_PRODUCTION + production_status = is_production(request) + request.session.update(production_status) return super().dispatch(request, *args, **kwargs) # Adds context information for user permissions From 089763c57874253a9d3132336f060e4c943c054c Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Mar 2024 08:07:04 -0600 Subject: [PATCH 3/6] Update permission_views.py --- src/registrar/views/utility/permission_views.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index fde01253e..f086747f8 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,8 +2,8 @@ import abc # abstract base class +from django.conf import settings from django.views.generic import DetailView, DeleteView, TemplateView -from registrar.context_processors import is_production from registrar.models import Domain, DomainRequest, DomainInvitation from registrar.models.user_domain_role import UserDomainRole @@ -39,8 +39,7 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): """ if "IS_PRODUCTION" not in request.session: # Pass the production flag to the context - production_status = is_production(request) - request.session.update(production_status) + request.session["IS_PRODUCTION"] = settings.IS_PRODUCTION return super().dispatch(request, *args, **kwargs) # Adds context information for user permissions From d14dbd082d0d021300f7c4916b661030011c65ef Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Mar 2024 09:43:43 -0600 Subject: [PATCH 4/6] Add unit tests, fix bug --- src/api/tests/common.py | 14 +++++ src/registrar/models/domain.py | 1 - src/registrar/tests/test_views.py | 92 ++++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 2 deletions(-) diff --git a/src/api/tests/common.py b/src/api/tests/common.py index 122965ae8..1a8c32526 100644 --- a/src/api/tests/common.py +++ b/src/api/tests/common.py @@ -49,3 +49,17 @@ def less_console_noise(): handler.setStream(restore[handler.name]) # close the file we opened devnull.close() + + +def less_console_noise_decorator(func): + """ + Decorator to silence console logging using the less_console_noise() function. + """ + + # "Wrap" the original function in the less_console_noise with clause, + # then just return this wrapper. + def wrapper(*args, **kwargs): + with less_console_noise(): + return func(*args, **kwargs) + + return wrapper diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index fa499893c..079fce3bc 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -325,7 +325,6 @@ class Domain(TimeStampedModel, DomainHelper): Subordinate hosts (something.your-domain.gov) MUST have IP addresses, while non-subordinate hosts MUST NOT. """ - raise ValueError("test") try: # attempt to retrieve hosts from registry and store in cache and db hosts = self._get_property("hosts") diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index eec12e463..94dfc842e 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -1,8 +1,14 @@ from django.test import Client, TestCase, override_settings from django.contrib.auth import get_user_model -from .common import MockEppLib # type: ignore +from api.tests.common import less_console_noise_decorator +from registrar.models.domain import Domain +from registrar.models.user_domain_role import UserDomainRole +from registrar.views.domain import DomainNameserversView +from .common import MockEppLib, less_console_noise # type: ignore +from unittest.mock import patch +from django.urls import reverse from registrar.models import ( DomainRequest, @@ -66,6 +72,7 @@ class TestEnvironmentVariablesEffects(TestCase): def tearDown(self): super().tearDown() + Domain.objects.all().delete() self.user.delete() @override_settings(IS_PRODUCTION=True) @@ -79,3 +86,86 @@ class TestEnvironmentVariablesEffects(TestCase): """Banner on non-prod.""" home_page = self.client.get("/") self.assertContains(home_page, "You are on a test site.") + + def side_effect_raise_value_error(self): + """Side effect that raises a 500 error""" + raise ValueError("Some error") + + @less_console_noise_decorator + @override_settings(IS_PRODUCTION=False) + def test_non_production_environment_raises_500_and_shows_banner(self): + """Tests if the non-prod banner is still shown on a 500""" + fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") + + # Add a role + fake_role, _ = UserDomainRole.objects.get_or_create( + user=self.user, domain=fake_domain, role=UserDomainRole.Roles.MANAGER + ) + + with patch.object(DomainNameserversView, "get_initial", side_effect=self.side_effect_raise_value_error): + with self.assertRaises(ValueError): + contact_page_500 = self.client.get( + reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + ) + + # Check that a 500 response is returned + self.assertEqual(contact_page_500.status_code, 500) + + self.assertContains(contact_page_500, "You are on a test site.") + + @less_console_noise_decorator + @override_settings(IS_PRODUCTION=True) + def test_production_environment_raises_500_and_doesnt_show_banner(self): + """Test if the non-prod banner is not shown on production when a 500 is raised""" + + fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") + + # Add a role + fake_role, _ = UserDomainRole.objects.get_or_create( + user=self.user, domain=fake_domain, role=UserDomainRole.Roles.MANAGER + ) + + with patch.object(DomainNameserversView, "get_initial", side_effect=self.side_effect_raise_value_error): + with self.assertRaises(ValueError): + contact_page_500 = self.client.get( + reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + ) + + # Check that a 500 response is returned + self.assertEqual(contact_page_500.status_code, 500) + + self.assertNotContains(contact_page_500, "You are on a test site.") + + @less_console_noise_decorator + @override_settings(IS_PRODUCTION=False) + def test_non_production_environment_raises_403_and_shows_banner(self): + """Test if the non-prod banner is shown when a 403 is raised""" + + fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") + + # Test navigating to the contact page. Should return a 403, + # but the banner should still appear. + contact_page_403 = self.client.get( + reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + ) + + self.assertEqual(contact_page_403.status_code, 403) + + self.assertContains(contact_page_403, "You are on a test site.", status_code=403) + + @less_console_noise_decorator + @override_settings(IS_PRODUCTION=True) + def test_production_environment_raises_403_and_doesnt_show_banner(self): + """Test if the non-prod banner is not shown on production when a 403 is raised""" + + fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") + + # Test navigating to the contact page. Should return a 403, + # but the banner should still appear. + contact_page_403 = self.client.get( + reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), + ) + + self.assertEqual(contact_page_403.status_code, 403) + + self.assertNotContains(contact_page_403, "You are on a test site.", status_code=403) From b4829d650adb9ad8ef7b5caa7da8afbc65923bfa Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Mar 2024 12:28:21 -0600 Subject: [PATCH 5/6] Refactor --- src/djangooidc/views.py | 33 ++++++++----------- src/registrar/config/urls.py | 6 +++- src/registrar/models/domain.py | 1 + src/registrar/tests/test_views.py | 33 ------------------- src/registrar/views/utility/error_views.py | 16 +++++++++ .../views/utility/permission_views.py | 10 ------ 6 files changed, 35 insertions(+), 64 deletions(-) create mode 100644 src/registrar/views/utility/error_views.py diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 2d3c842d2..2d7131d50 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -6,12 +6,13 @@ from django.conf import settings from django.contrib.auth import logout as auth_logout from django.contrib.auth import authenticate, login from django.http import HttpResponseRedirect -from django.shortcuts import redirect, render +from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode from djangooidc.oidc import Client from djangooidc import exceptions as o_e from registrar.models import User +from registrar.views.utility.error_views import custom_500_error_view, custom_401_error_view logger = logging.getLogger(__name__) @@ -49,27 +50,19 @@ def error_page(request, error): """Display a sensible message and log the error.""" logger.error(error) if isinstance(error, o_e.AuthenticationFailed): - return render( - request, - "401.html", - context={ - "friendly_message": error.friendly_message, - "log_identifier": error.locator, - }, - status=401, - ) + context={ + "friendly_message": error.friendly_message, + "log_identifier": error.locator, + } + return custom_401_error_view(request, context) if isinstance(error, o_e.InternalError): - return render( - request, - "500.html", - context={ - "friendly_message": error.friendly_message, - "log_identifier": error.locator, - }, - status=500, - ) + context={ + "friendly_message": error.friendly_message, + "log_identifier": error.locator, + } + return custom_500_error_view(request, context) if isinstance(error, Exception): - return render(request, "500.html", status=500) + return custom_500_error_view(request) def openid(request): diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 9049d718c..058fc720c 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -3,7 +3,7 @@ For more information see: https://docs.djangoproject.com/en/4.0/topics/http/urls/ """ - +from django.conf.urls import handler500 from django.contrib import admin from django.urls import include, path from django.views.generic import RedirectView @@ -149,6 +149,10 @@ urlpatterns = [ ), ] +# Djangooidc strips out context data from that context, so we define a custom error +# view through this method. +handler500 = "registrar.views.utility.error_views.custom_500_error_view" + # we normally would guard these with `if settings.DEBUG` but tests run with # DEBUG = False even when these apps have been loaded because settings.DEBUG # was actually True. Instead, let's add these URLs any time we are able to diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 079fce3bc..fa499893c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -325,6 +325,7 @@ class Domain(TimeStampedModel, DomainHelper): Subordinate hosts (something.your-domain.gov) MUST have IP addresses, while non-subordinate hosts MUST NOT. """ + raise ValueError("test") try: # attempt to retrieve hosts from registry and store in cache and db hosts = self._get_property("hosts") diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 94dfc842e..053281e99 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -136,36 +136,3 @@ class TestEnvironmentVariablesEffects(TestCase): self.assertNotContains(contact_page_500, "You are on a test site.") - @less_console_noise_decorator - @override_settings(IS_PRODUCTION=False) - def test_non_production_environment_raises_403_and_shows_banner(self): - """Test if the non-prod banner is shown when a 403 is raised""" - - fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") - - # Test navigating to the contact page. Should return a 403, - # but the banner should still appear. - contact_page_403 = self.client.get( - reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), - ) - - self.assertEqual(contact_page_403.status_code, 403) - - self.assertContains(contact_page_403, "You are on a test site.", status_code=403) - - @less_console_noise_decorator - @override_settings(IS_PRODUCTION=True) - def test_production_environment_raises_403_and_doesnt_show_banner(self): - """Test if the non-prod banner is not shown on production when a 403 is raised""" - - fake_domain, _ = Domain.objects.get_or_create(name="igorville.gov") - - # Test navigating to the contact page. Should return a 403, - # but the banner should still appear. - contact_page_403 = self.client.get( - reverse("domain-dns-nameservers", kwargs={"pk": fake_domain.id}), - ) - - self.assertEqual(contact_page_403.status_code, 403) - - self.assertNotContains(contact_page_403, "You are on a test site.", status_code=403) diff --git a/src/registrar/views/utility/error_views.py b/src/registrar/views/utility/error_views.py new file mode 100644 index 000000000..c7841911c --- /dev/null +++ b/src/registrar/views/utility/error_views.py @@ -0,0 +1,16 @@ +"""Custom views that allow for error view customization""" +from django.shortcuts import render + +def custom_500_error_view(request, context=None): + """Used to redirect 500 errors to a custom view""" + if context is None: + return render(request, "500.html", status=500) + else: + return render(request, "500.html", context=context, status=500) + +def custom_401_error_view(request, context=None): + """Used to redirect 401 errors to a custom view""" + if context is None: + return render(request, "401.html", status=401) + else: + return render(request, "401.html", context=context, status=401) diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index f086747f8..8f0b8e0d1 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -32,16 +32,6 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): # variable name in template context for the model object context_object_name = "domain" - def dispatch(self, request, *args, **kwargs): - """ - Custom implementation of dispatch to ensure that 500 error pages (and others) - have access to the IS_PRODUCTION flag - """ - if "IS_PRODUCTION" not in request.session: - # Pass the production flag to the context - request.session["IS_PRODUCTION"] = settings.IS_PRODUCTION - return super().dispatch(request, *args, **kwargs) - # Adds context information for user permissions def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) From 361392ba71c12e2b152c07d7d99b9dc107e638cf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Mar 2024 14:44:27 -0600 Subject: [PATCH 6/6] Linting activities --- src/djangooidc/views.py | 4 ++-- src/registrar/config/urls.py | 10 +++++++++- src/registrar/models/domain.py | 1 - src/registrar/tests/test_views.py | 3 +-- src/registrar/views/utility/error_views.py | 18 +++++++++++++++++- .../views/utility/permission_views.py | 1 - 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 2d7131d50..8e112769b 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -50,13 +50,13 @@ def error_page(request, error): """Display a sensible message and log the error.""" logger.error(error) if isinstance(error, o_e.AuthenticationFailed): - context={ + context = { "friendly_message": error.friendly_message, "log_identifier": error.locator, } return custom_401_error_view(request, context) if isinstance(error, o_e.InternalError): - context={ + context = { "friendly_message": error.friendly_message, "log_identifier": error.locator, } diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 058fc720c..c743aed0c 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -3,7 +3,7 @@ For more information see: https://docs.djangoproject.com/en/4.0/topics/http/urls/ """ -from django.conf.urls import handler500 + from django.contrib import admin from django.urls import include, path from django.views.generic import RedirectView @@ -151,6 +151,14 @@ urlpatterns = [ # Djangooidc strips out context data from that context, so we define a custom error # view through this method. +# If Djangooidc is left to its own devices and uses reverse directly, +# then both context and session information will be obliterated due to: + +# a) Djangooidc being out of scope for context_processors +# b) Potential cyclical import errors restricting what kind of data is passable. + +# Rather than dealing with that, we keep everything centralized in one location. +# This way, we can share a view for djangooidc, and other pages as we see fit. handler500 = "registrar.views.utility.error_views.custom_500_error_view" # we normally would guard these with `if settings.DEBUG` but tests run with diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index fa499893c..079fce3bc 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -325,7 +325,6 @@ class Domain(TimeStampedModel, DomainHelper): Subordinate hosts (something.your-domain.gov) MUST have IP addresses, while non-subordinate hosts MUST NOT. """ - raise ValueError("test") try: # attempt to retrieve hosts from registry and store in cache and db hosts = self._get_property("hosts") diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 053281e99..b8055f288 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -6,7 +6,7 @@ from registrar.models.domain import Domain from registrar.models.user_domain_role import UserDomainRole from registrar.views.domain import DomainNameserversView -from .common import MockEppLib, less_console_noise # type: ignore +from .common import MockEppLib # type: ignore from unittest.mock import patch from django.urls import reverse @@ -135,4 +135,3 @@ class TestEnvironmentVariablesEffects(TestCase): self.assertEqual(contact_page_500.status_code, 500) self.assertNotContains(contact_page_500, "You are on a test site.") - diff --git a/src/registrar/views/utility/error_views.py b/src/registrar/views/utility/error_views.py index c7841911c..48ae628a4 100644 --- a/src/registrar/views/utility/error_views.py +++ b/src/registrar/views/utility/error_views.py @@ -1,6 +1,21 @@ -"""Custom views that allow for error view customization""" +""" +Custom views that allow for error view customization. + +Used as a general handler for 500 errors both coming from the registrar app, but +also the djangooidc app. + +If Djangooidc is left to its own devices and uses reverse directly, +then both context and session information will be obliterated due to: + +a) Djangooidc being out of scope for context_processors +b) Potential cyclical import errors restricting what kind of data is passable. + +Rather than dealing with that, we keep everything centralized in one location. +""" + from django.shortcuts import render + def custom_500_error_view(request, context=None): """Used to redirect 500 errors to a custom view""" if context is None: @@ -8,6 +23,7 @@ def custom_500_error_view(request, context=None): else: return render(request, "500.html", context=context, status=500) + def custom_401_error_view(request, context=None): """Used to redirect 401 errors to a custom view""" if context is None: diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 8f0b8e0d1..f2752c3b5 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,7 +2,6 @@ import abc # abstract base class -from django.conf import settings from django.views.generic import DetailView, DeleteView, TemplateView from registrar.models import Domain, DomainRequest, DomainInvitation from registrar.models.user_domain_role import UserDomainRole