Merge pull request #673 from cisagov/nmb/543-login-required-middleware

Use login required middleware

.github/workflows/security-check.yaml

@@ -72,8 +72,8 @@ jobs:
         # by adding MockUserLogin to settings.MIDDLEWARE
         run: |
           perl -pi \
-          -e 's/"csp.middleware.CSPMiddleware",/$&"registrar.tests.common.MockUserLogin",/' \
+          -e 's/"django.contrib.auth.middleware.AuthenticationMiddleware",/$&"registrar.tests.common.MockUserLogin",/' \
-          src/registrar/config/settings.py
+          registrar/config/settings.py
         working-directory: ./src
 
       - name: OWASP scan

.github/workflows/test.yaml

@@ -59,7 +59,7 @@ jobs:
         # by adding MockUserLogin to settings.MIDDLEWARE
         run: |
           perl -pi \
-          -e 's/"csp.middleware.CSPMiddleware",/$&"registrar.tests.common.MockUserLogin",/' \
+          -e 's/"django.contrib.auth.middleware.AuthenticationMiddleware",/$&"registrar.tests.common.MockUserLogin",/' \
           registrar/config/settings.py
 
       - name: Start container

src/Pipfile

@@ -24,9 +24,9 @@ django-fsm = "*"
 django-phonenumber-field = {extras = ["phonenumberslite"], version = "*"}
 boto3 = "*"
 typing-extensions ='*'
+django-login-required-middleware = "*"
 fred-epplib = {git = "https://github.com/cisagov/epplib.git", ref = "master"}
 
-
 [dev-packages]
 django-debug-toolbar = "*"
 nplusone = "*"

src/Pipfile.lock

@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "fd7d0efa9a87dfe4b2bb228ee0e7978fba16c7cfdd3c443870900cfe899e2cfd"
+            "sha256": "1242c67b31261243a35128410d4a928fca3729ddac13b8c8e25adf31445c6328"
         },
         "pipfile-spec": 6,
         "requires": {},
@@ -306,6 +306,13 @@
             "index": "pypi",
             "version": "==2.8.1"
         },
+        "django-login-required-middleware": {
+            "hashes": [
+                "sha256:847ae9a69fd7a07618ed53192b3c06946af70a0caf6d0f4eb40a8f37593cd970"
+            ],
+            "index": "pypi",
+            "version": "==0.9.0"
+        },
         "django-phonenumber-field": {
             "extras": [
                 "phonenumberslite"
@@ -791,11 +798,11 @@
         },
         "typing-extensions": {
             "hashes": [
-                "sha256:06006244c70ac8ee83fa8282cb188f697b8db25bc8b4df07be1873c43897060c",
+                "sha256:88a4153d8505aabbb4e13aacb7c486c2b4a33ca3b3f807914a9b4c844c471c26",
-                "sha256:3a8b36f13dd5fdc5d1b16fe317f5668545de77fa0b8e02006381fd49d731ab98"
+                "sha256:d91d5919357fe7f681a9f2b5b4cb2a5f1ef0a1e9f59c4d8ff0d3491e05c0ffd5"
             ],
             "index": "pypi",
-            "version": "==4.6.2"
+            "version": "==4.6.3"
         },
         "urllib3": {
             "hashes": [
@@ -951,19 +958,19 @@
         },
         "django-stubs": {
             "hashes": [
-                "sha256:93baff824f0a056e71036b423b942a74f07b909e45e3fa38185b910f597c5c08",
+                "sha256:66477bdba25407623f4079205e58f3c7265a4f0d8f7c9f540a6edc16f8883a5b",
-                "sha256:d2c671989efb3f7b0fa91e461909ad5a5a52155fe7fe6d1f2058cb88e3afb123"
+                "sha256:8c15d5f7b05926805cfb25f2bfbf3509c37792fbd8aec5aedea358b85d8bccd5"
             ],
             "index": "pypi",
-            "version": "==4.2.0"
+            "version": "==4.2.1"
         },
         "django-stubs-ext": {
             "hashes": [
-                "sha256:55b2e3077f883e0131a7596f8ff8b19f8fc3ca325a3318ccacf5331acb2601e4",
+                "sha256:2696d6f7d8538341b060cffa9565c72ea797e866687e040b86d29cad8799e5fe",
-                "sha256:7789f0caeca7152fef07ad6b94dec7310a05d0b8dab77f7979e19db0037b5127"
+                "sha256:4b6b63e49f4ba30d93ec46f87507648c99c9de6911e651ad69db7084fd5b2f4e"
             ],
-            "markers": "python_version >= '3.7'",
+            "markers": "python_version >= '3.8'",
-            "version": "==4.2.0"
+            "version": "==4.2.1"
         },
         "django-webtest": {
             "hashes": [
@@ -1306,11 +1313,11 @@
         },
         "typing-extensions": {
             "hashes": [
-                "sha256:06006244c70ac8ee83fa8282cb188f697b8db25bc8b4df07be1873c43897060c",
+                "sha256:88a4153d8505aabbb4e13aacb7c486c2b4a33ca3b3f807914a9b4c844c471c26",
-                "sha256:3a8b36f13dd5fdc5d1b16fe317f5668545de77fa0b8e02006381fd49d731ab98"
+                "sha256:d91d5919357fe7f681a9f2b5b4cb2a5f1ef0a1e9f59c4d8ff0d3491e05c0ffd5"
             ],
             "index": "pypi",
-            "version": "==4.6.2"
+            "version": "==4.6.3"
         },
         "urllib3": {
             "hashes": [

src/api/tests/test_available.py

@@ -104,6 +104,9 @@ class AvailableAPITest(TestCase):
 
     def test_available_post(self):
         """Cannot post to the /available/ API endpoint."""
+        # have to log in to test the correct thing now that we require login
+        # for all URLs by default
+        self.client.force_login(self.user)
         with less_console_noise():
             response = self.client.post(API_BASE_PATH + "nonsense")
         self.assertEqual(response.status_code, 405)

src/registrar/config/settings.py

@@ -133,6 +133,8 @@ MIDDLEWARE = [
     "django.middleware.csrf.CsrfViewMiddleware",
     # add `user` (the currently-logged-in user) to incoming HttpRequest objects
     "django.contrib.auth.middleware.AuthenticationMiddleware",
+    # Require login for every single request by default
+    "login_required.middleware.LoginRequiredMiddleware",
     # provide framework for displaying messages to the user, see documentation
     "django.contrib.messages.middleware.MessageMiddleware",
     # provide clickjacking protection via the X-Frame-Options header
@@ -460,6 +462,12 @@ AUTHENTICATION_BACKENDS = [
 # the login_required() decorator, LoginRequiredMixin, or AccessMixin
 LOGIN_URL = "/openid/login"
 
+# We don't want the OIDC app to be login-required because then it can't handle
+# the initial login requests without erroring.
+LOGIN_REQUIRED_IGNORE_PATHS = [
+    r"/openid/(.+)$",
+]
+
 # where to go after logging out
 LOGOUT_REDIRECT_URL = "home"
 

src/registrar/tests/test_url_auth.py

@@ -0,0 +1,159 @@
+"""Test that almost all URLs require authentication.
+
+This uses deep Django URLConf pattern magic and was shamelessly lifted from
+https://github.com/18F/tock/blob/main/tock/tock/tests/test_url_auth.py
+"""
+
+from django.test import TestCase
+from django.urls import reverse, URLPattern
+from django.urls.resolvers import URLResolver
+
+import registrar.config.urls
+
+from .common import less_console_noise
+
+# When a URLconf pattern contains named capture groups, we'll use this
+# dictionary to retrieve a sample value for it, which will be included
+# in the sample URLs we generate, when attempting to perform a GET
+# request on the view.
+SAMPLE_KWARGS = {
+    "app_label": "registrar",
+    "pk": "1",
+    "id": "1",
+    "content_type_id": "2",
+    "object_id": "3",
+    "domain": "whitehouse.gov",
+}
+
+# Our test suite will ignore some namespaces.
+IGNORE_NAMESPACES = [
+    # The Django Debug Toolbar (DJDT) ends up in the URL config but it's always
+    # disabled in production, so don't worry about it.
+    "djdt"
+]
+
+# In general, we don't want to have any unnamed views, because that makes it
+# impossible to generate sample URLs that point at them. We'll make exceptions
+# for some namespaces that we don't have control over, though.
+NAMESPACES_WITH_UNNAMED_VIEWS = ["admin", None]
+
+
+def iter_patterns(urlconf, patterns=None, namespace=None):
+    """
+    Iterate through all patterns in the given Django URLconf.  Yields
+    `(viewname, route)` tuples, where `viewname` is the fully-qualified view name
+    (including its namespace, if any), and `route` is a regular expression that
+    corresponds to the part of the pattern that contains any capturing groups.
+    """
+    if patterns is None:
+        patterns = urlconf.urlpatterns
+    for pattern in patterns:
+        # Resolve if it's a route or an include
+        if isinstance(pattern, URLPattern):
+            viewname = pattern.name
+            if viewname is None and namespace not in NAMESPACES_WITH_UNNAMED_VIEWS:
+                raise AssertionError(
+                    f"namespace {namespace} cannot contain unnamed views"
+                )
+            if namespace and viewname is not None:
+                viewname = f"{namespace}:{viewname}"
+            yield (viewname, pattern.pattern)
+        elif isinstance(pattern, URLResolver):
+            if len(pattern.default_kwargs.keys()) > 0:
+                raise AssertionError("resolvers are not expected to have kwargs")
+            if pattern.namespace and namespace is not None:
+                raise AssertionError("nested namespaces are not currently supported")
+            if pattern.namespace in IGNORE_NAMESPACES:
+                continue
+            yield from iter_patterns(
+                urlconf, pattern.url_patterns, namespace or pattern.namespace
+            )
+        else:
+            raise AssertionError("unknown pattern class")
+
+
+def iter_sample_urls(urlconf):
+    """
+    Yields sample URLs for all entries in the given Django URLconf.
+    This gets pretty deep into the muck of RoutePattern
+    https://docs.djangoproject.com/en/2.1/_modules/django/urls/resolvers/
+    """
+
+    for viewname, route in iter_patterns(urlconf):
+        if not viewname:
+            continue
+        if viewname == "auth_user_password_change":
+            print(route)
+            break
+        named_groups = route.regex.groupindex.keys()
+        kwargs = {}
+        args = ()
+
+        for kwarg in named_groups:
+            if kwarg not in SAMPLE_KWARGS:
+                raise AssertionError(
+                    f'Sample value for {kwarg} in pattern "{route}" not found'
+                )
+            kwargs[kwarg] = SAMPLE_KWARGS[kwarg]
+
+        url = reverse(viewname, args=args, kwargs=kwargs)
+        yield (viewname, url)
+
+
+class TestURLAuth(TestCase):
+    """
+    Tests to ensure that most URLs in a Django URLconf are protected by
+    authentication.
+    """
+
+    # We won't test that the following URLs are protected by auth.
+    # Note that the trailing slash is wobbly depending on how the URL was defined.
+    IGNORE_URLS = [
+        # These are the OIDC auth endpoints that always need
+        # to be public.
+        "/openid/login/",
+        "/openid/logout/",
+        "/openid/callback",
+        "/openid/callback/login/",
+        "/openid/callback/logout/",
+    ]
+
+    def assertURLIsProtectedByAuth(self, url):
+        """
+        Make a GET request to the given URL, and ensure that it either redirects
+        to login or denies access outright.
+        """
+
+        try:
+            with less_console_noise():
+                response = self.client.get(url)
+        except Exception as e:
+            # It'll be helpful to provide information on what URL was being
+            # accessed at the time the exception occurred.  Python 3 will
+            # also include a full traceback of the original exception, so
+            # we don't need to worry about hiding the original cause.
+            raise AssertionError(f'Accessing {url} raised "{e}"', e)
+
+        code = response.status_code
+        if code == 302:
+            redirect = response["location"]
+            self.assertRegex(
+                redirect,
+                r"^\/openid\/login",
+                f"GET {url} should redirect to login or deny access, but instead "
+                f"it redirects to {redirect}",
+            )
+        elif code == 401 or code == 403:
+            pass
+        else:
+            raise AssertionError(
+                f"GET {url} returned HTTP {code}, but should redirect to login or "
+                "deny access",
+            )
+
+    def test_login_required_all_urls(self):
+        """All URLs redirect to the login view."""
+        for viewname, url in iter_sample_urls(registrar.config.urls):
+            if url not in self.IGNORE_URLS:
+                with self.subTest(viewname=viewname):
+                    self.assertURLIsProtectedByAuth(url)

src/registrar/tests/test_views.py

@@ -35,10 +35,9 @@ class TestViews(TestCase):
         self.assertContains(response, "OK", status_code=200)
 
     def test_home_page(self):
-        """Home page should be available without a login."""
+        """Home page should NOT be available without a login."""
         response = self.client.get("/")
-        self.assertContains(response, "registrar", status_code=200)
+        self.assertEqual(response.status_code, 302)
-        self.assertContains(response, "Sign in")
 
     def test_whoami_page_no_user(self):
         """Whoami page not accessible without a logged-in user."""

src/registrar/views/application.py

@@ -1,6 +1,5 @@
 import logging
 
-from django.contrib.auth.mixins import LoginRequiredMixin
 from django.http import Http404, HttpResponse, HttpResponseRedirect
 from django.shortcuts import redirect, render
 from django.urls import resolve, reverse
@@ -44,7 +43,7 @@ class Step(StrEnum):
     REVIEW = "review"
 
 
-class ApplicationWizard(LoginRequiredMixin, TemplateView):
+class ApplicationWizard(TemplateView):
     """
     A common set of methods and configuration.
 

src/registrar/views/health.py

@@ -1,6 +1,11 @@
 from django.http import HttpResponse
 
+from login_required import login_not_required
 
+
+# the health check endpoint needs to be globally available so that the
+# PaaS orchestrator can make sure the app has come up properly
+@login_not_required
 def health(request):
     return HttpResponse(
         '<html lang="en"><head><title>OK - Get.gov</title></head><body>OK</body>'

src/requirements.txt

@@ -17,6 +17,7 @@ django-auditlog==2.3.0
 django-cache-url==3.4.4
 django-csp==3.7
 django-fsm==2.8.1
+django-login-required-middleware==0.9.0
 django-phonenumber-field[phonenumberslite]==7.1.0
 django-widget-tweaks==1.4.12
 environs[django]==9.5.0
@@ -47,6 +48,6 @@ s3transfer==0.6.1 ; python_version >= '3.7'
 setuptools==67.8.0 ; python_version >= '3.7'
 six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'
 sqlparse==0.4.4 ; python_version >= '3.5'
-typing-extensions==4.6.2
+typing-extensions==4.6.3
 urllib3==1.26.16 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'
 whitenoise==6.4.0

src/zap.conf

@@ -55,6 +55,7 @@
 10038	OUTOFSCOPE	http://app:8080/users/add
 10038	OUTOFSCOPE	http://app:8080/nameservers
 10038	OUTOFSCOPE	http://app:8080/your-contact-information
+10038	OUTOFSCOPE	http://app:8080/authorizing-official
 10038	OUTOFSCOPE	http://app:8080/security-email
 10038	OUTOFSCOPE	http://app:8080/delete
 10038	OUTOFSCOPE	http://app:8080/withdraw 
@@ -63,6 +64,7 @@
 10038	OUTOFSCOPE	http://app:8080/todo
 # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers
 10038	OUTOFSCOPE	http://app:8080/openid/login/
+10038	OUTOFSCOPE	http://app:8080/openid/logout/
 10039	FAIL	(X-Backend-Server Header Information Leak - Passive/beta)
 10040	FAIL	(Secure Pages Include Mixed Content - Passive/release)
 10041	FAIL	(HTTP to HTTPS Insecure Transition in Form Post - Passive/beta)
@@ -149,6 +151,7 @@
 # OIDC isn't configured in the test environment and DEBUG=True so these error pages
 # trigger this rule in a way that they won't in production
 90022	OUTOFSCOPE	http://app:8080/openid/login/
+90022	OUTOFSCOPE	http://app:8080/openid/logout/
 90023	FAIL	(XML External Entity Attack - Active/beta)
 90024	FAIL	(Generic Padding Oracle - Active/beta)
 90025	FAIL	(Expression Language Injection - Active/beta)