From ae3ba84f1adf25d712ac84bfda55eb208c871038 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Thu, 1 Jun 2023 16:03:31 -0500 Subject: [PATCH 1/8] Add LoginRequiredMiddleware to make (almost) every URL login required by default --- src/Pipfile | 1 + src/Pipfile.lock | 45 +++++++++++++++++------------- src/api/tests/test_available.py | 3 ++ src/registrar/config/settings.py | 8 ++++++ src/registrar/tests/test_views.py | 5 ++-- src/registrar/views/application.py | 3 +- src/registrar/views/health.py | 4 +++ src/requirements.txt | 5 ++-- 8 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/Pipfile b/src/Pipfile index 433f41632..3d92c36ab 100644 --- a/src/Pipfile +++ b/src/Pipfile @@ -24,6 +24,7 @@ django-fsm = "*" django-phonenumber-field = {extras = ["phonenumberslite"], version = "*"} boto3 = "*" typing-extensions ='*' +django-login-required-middleware = "*" [dev-packages] django-debug-toolbar = "*" diff --git a/src/Pipfile.lock b/src/Pipfile.lock index e485a9b1a..cba61498c 100644 --- a/src/Pipfile.lock +++ b/src/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "6a6f3722d88e1f0059e38f23c104708d7922d41b42be1f17287cd4d84dcf9b05" + "sha256": "187278933085d8c5448015e2d28c23934c2098f4aadd82157379174629d8cf6b" }, "pipfile-spec": 6, "requires": {}, @@ -24,19 +24,19 @@ }, "boto3": { "hashes": [ - "sha256:2eb9e688aa86bf1fadcec0b6995b42ec9788e7cd5f1a9c8ac1b66a2506aa209f", - "sha256:5b7e9f2674fe8aa99e2d168744023a3f66da12d9c51e0624489dd0db7aafe30d" + "sha256:30f8ab1cf89d5864a80ba2d5eb5316dbd2a63c9469877e0cffb522630438aa85", + "sha256:77e8fa7c257f9ed8bfe0c3ffc2ccc47b1cfa27058f99415b6003699d1202e0c0" ], "index": "pypi", - "version": "==1.26.144" + "version": "==1.26.145" }, "botocore": { "hashes": [ - "sha256:c60b9158cbc7447411abdec77b87a71d86d9404064702e92d317dca6a1ec9a5b", - "sha256:e2b970e68643cf4752cad4e45ba3319fc35707f1bff7f150f7ffcac1b1427b47" + "sha256:264a3f19ed280d80711b7e278be09acff7ed379a96432fdf179b4e6e3a687e6a", + "sha256:65e2a2b1cc70583225f87d6d63736215f93c6234721967bdab872270ba7a1f45" ], "markers": "python_version >= '3.7'", - "version": "==1.29.144" + "version": "==1.29.145" }, "cachetools": { "hashes": [ @@ -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" @@ -793,11 +800,11 @@ }, "boto3": { "hashes": [ - "sha256:2eb9e688aa86bf1fadcec0b6995b42ec9788e7cd5f1a9c8ac1b66a2506aa209f", - "sha256:5b7e9f2674fe8aa99e2d168744023a3f66da12d9c51e0624489dd0db7aafe30d" + "sha256:30f8ab1cf89d5864a80ba2d5eb5316dbd2a63c9469877e0cffb522630438aa85", + "sha256:77e8fa7c257f9ed8bfe0c3ffc2ccc47b1cfa27058f99415b6003699d1202e0c0" ], "index": "pypi", - "version": "==1.26.144" + "version": "==1.26.145" }, "boto3-mocking": { "hashes": [ @@ -809,27 +816,27 @@ }, "boto3-stubs": { "hashes": [ - "sha256:1062612f63f154f47a4f5b7b40c2cb15debe5f44774587110da76fd292f528f0", - "sha256:bc0cc5067f55b2da628db8a73119ecccd74b27cf424af83e56526cd90beaf9f8" + "sha256:9413cb395c803d5b85e9ec7b16fba855a613ecd78b2e0011e2f6b62cf0b4fc1e", + "sha256:be2007f92138781288c7a22eba30b7d60742466fc28edd04637b31fabee854a5" ], "index": "pypi", - "version": "==1.26.144" + "version": "==1.26.145" }, "botocore": { "hashes": [ - "sha256:c60b9158cbc7447411abdec77b87a71d86d9404064702e92d317dca6a1ec9a5b", - "sha256:e2b970e68643cf4752cad4e45ba3319fc35707f1bff7f150f7ffcac1b1427b47" + "sha256:264a3f19ed280d80711b7e278be09acff7ed379a96432fdf179b4e6e3a687e6a", + "sha256:65e2a2b1cc70583225f87d6d63736215f93c6234721967bdab872270ba7a1f45" ], "markers": "python_version >= '3.7'", - "version": "==1.29.144" + "version": "==1.29.145" }, "botocore-stubs": { "hashes": [ - "sha256:b9db32981b4deefb01784d9b196afeaca7df6f6f185d8ba7f96c02b1c3bc0d90", - "sha256:d456543af79fbdd23df76a2d7a7525cd672b4bb5b057d7e060bc117d9af71694" + "sha256:80ffab72ad428d20cb1cf538ee55fcd94f7d81315b77d84fec99e218c3974e8b", + "sha256:928c58a434dd83bef956e3b5bb1e96278fff5eee9f8b8ab08d916cef1e9a2014" ], "markers": "python_version >= '3.7' and python_version < '4.0'", - "version": "==1.29.144" + "version": "==1.29.145" }, "click": { "hashes": [ diff --git a/src/api/tests/test_available.py b/src/api/tests/test_available.py index 061b6274c..39ddba071 100644 --- a/src/api/tests/test_available.py +++ b/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) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 54eb35d9f..5020fe382 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -134,6 +134,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 @@ -461,6 +463,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" diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index bf1cef2f2..500912952 100644 --- a/src/registrar/tests/test_views.py +++ b/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.assertContains(response, "Sign in") + self.assertEqual(response.status_code, 302) def test_whoami_page_no_user(self): """Whoami page not accessible without a logged-in user.""" diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index b8454bf78..d4368e868 100644 --- a/src/registrar/views/application.py +++ b/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. diff --git a/src/registrar/views/health.py b/src/registrar/views/health.py index 53179382a..40f7330a0 100644 --- a/src/registrar/views/health.py +++ b/src/registrar/views/health.py @@ -1,6 +1,10 @@ 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( 'OK - Get.govOK' diff --git a/src/requirements.txt b/src/requirements.txt index 9d17f87f7..077cfed2a 100644 --- a/src/requirements.txt +++ b/src/requirements.txt @@ -1,7 +1,7 @@ -i https://pypi.python.org/simple asgiref==3.7.2 ; python_version >= '3.7' -boto3==1.26.144 -botocore==1.29.144 ; python_version >= '3.7' +boto3==1.26.145 +botocore==1.29.145 ; python_version >= '3.7' cachetools==5.3.1 certifi==2023.5.7 ; python_version >= '3.6' cfenv==0.5.3 @@ -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 From 2e7644bdc3fc8b477c54e64a2832f30079cb319d Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Thu, 1 Jun 2023 16:05:15 -0500 Subject: [PATCH 2/8] fix linting errors --- src/registrar/views/health.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/views/health.py b/src/registrar/views/health.py index 40f7330a0..276dc4b09 100644 --- a/src/registrar/views/health.py +++ b/src/registrar/views/health.py @@ -2,6 +2,7 @@ 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 From 85a8b108ded61637db64948715b71d3ae72d345c Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Thu, 1 Jun 2023 16:35:40 -0500 Subject: [PATCH 3/8] Add test that login is required for almost every URL --- src/registrar/tests/test_url_auth.py | 158 +++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 src/registrar/tests/test_url_auth.py diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py new file mode 100644 index 000000000..8547e0bba --- /dev/null +++ b/src/registrar/tests/test_url_auth.py @@ -0,0 +1,158 @@ +"""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/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) From 84b13bd178ccb95c8bd59e4ecb1ff61bf95b72b5 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Thu, 1 Jun 2023 18:23:06 -0500 Subject: [PATCH 4/8] Fix test failure from CI --- src/registrar/tests/test_url_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 8547e0bba..17ad2c329 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -114,6 +114,7 @@ class TestURLAuth(TestCase): "/openid/login/", "/openid/logout/", "/openid/callback", + "/openid/callback/login/", "/openid/callback/logout/", ] From 0254d879e45ed13932ba72aa35427b4aa64fcb30 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 5 Jun 2023 14:42:11 -0500 Subject: [PATCH 5/8] Tweak auto-login configuration --- .github/workflows/security-check.yaml | 2 +- .github/workflows/test.yaml | 2 +- src/registrar/tests/test_url_auth.py | 1 - src/zap.conf | 1 + 4 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/security-check.yaml b/.github/workflows/security-check.yaml index 39f8a80a3..b2d840c84 100644 --- a/.github/workflows/security-check.yaml +++ b/.github/workflows/security-check.yaml @@ -72,7 +72,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",/' \ src/registrar/config/settings.py working-directory: ./src diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index a45b9fa72..6d34f80b5 100644 --- a/.github/workflows/test.yaml +++ b/.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 diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 17ad2c329..a81e0a4c3 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -112,7 +112,6 @@ class TestURLAuth(TestCase): # These are the OIDC auth endpoints that always need # to be public. "/openid/login/", - "/openid/logout/", "/openid/callback", "/openid/callback/login/", "/openid/callback/logout/", diff --git a/src/zap.conf b/src/zap.conf index 6a5e9bf77..1aa905458 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -53,6 +53,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 From 816c6c8c90b48e339541598c211f4cd376ade07e Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Mon, 5 Jun 2023 14:51:21 -0500 Subject: [PATCH 6/8] try to make python tests pass --- src/registrar/tests/test_url_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index a81e0a4c3..17ad2c329 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -112,6 +112,7 @@ class TestURLAuth(TestCase): # These are the OIDC auth endpoints that always need # to be public. "/openid/login/", + "/openid/logout/", "/openid/callback", "/openid/callback/login/", "/openid/callback/logout/", From 0ca6889ee3bdeba07d3c3a9ecbc12cec84b8002a Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Fri, 9 Jun 2023 13:16:24 -0500 Subject: [PATCH 7/8] New OWASP false positives with MockLogin --- src/zap.conf | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/zap.conf b/src/zap.conf index 0a219ae06..b2234250e 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -64,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) @@ -150,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) From 45f317d11334ac4aafda527a4fce3dc07fbf1e58 Mon Sep 17 00:00:00 2001 From: Neil Martinsen-Burrell Date: Fri, 9 Jun 2023 13:51:26 -0500 Subject: [PATCH 8/8] Fix Disable Login step --- .github/workflows/security-check.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/security-check.yaml b/.github/workflows/security-check.yaml index b2d840c84..dd75d5c98 100644 --- a/.github/workflows/security-check.yaml +++ b/.github/workflows/security-check.yaml @@ -73,7 +73,7 @@ jobs: run: | perl -pi \ -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