diff --git a/.github/workflows/security-check.yaml b/.github/workflows/security-check.yaml index 39f8a80a3..dd75d5c98 100644 --- a/.github/workflows/security-check.yaml +++ b/.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",/' \ - src/registrar/config/settings.py + -e 's/"django.contrib.auth.middleware.AuthenticationMiddleware",/$&"registrar.tests.common.MockUserLogin",/' \ + registrar/config/settings.py working-directory: ./src - name: OWASP scan 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/Pipfile b/src/Pipfile index 857505daf..6900b0bcf 100644 --- a/src/Pipfile +++ b/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 = "*" diff --git a/src/Pipfile.lock b/src/Pipfile.lock index 5b518dcf1..d13ed6382 100644 --- a/src/Pipfile.lock +++ b/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:3a8b36f13dd5fdc5d1b16fe317f5668545de77fa0b8e02006381fd49d731ab98" + "sha256:88a4153d8505aabbb4e13aacb7c486c2b4a33ca3b3f807914a9b4c844c471c26", + "sha256:d91d5919357fe7f681a9f2b5b4cb2a5f1ef0a1e9f59c4d8ff0d3491e05c0ffd5" ], "index": "pypi", - "version": "==4.6.2" + "version": "==4.6.3" }, "urllib3": { "hashes": [ @@ -951,19 +958,19 @@ }, "django-stubs": { "hashes": [ - "sha256:93baff824f0a056e71036b423b942a74f07b909e45e3fa38185b910f597c5c08", - "sha256:d2c671989efb3f7b0fa91e461909ad5a5a52155fe7fe6d1f2058cb88e3afb123" + "sha256:66477bdba25407623f4079205e58f3c7265a4f0d8f7c9f540a6edc16f8883a5b", + "sha256:8c15d5f7b05926805cfb25f2bfbf3509c37792fbd8aec5aedea358b85d8bccd5" ], "index": "pypi", - "version": "==4.2.0" + "version": "==4.2.1" }, "django-stubs-ext": { "hashes": [ - "sha256:55b2e3077f883e0131a7596f8ff8b19f8fc3ca325a3318ccacf5331acb2601e4", - "sha256:7789f0caeca7152fef07ad6b94dec7310a05d0b8dab77f7979e19db0037b5127" + "sha256:2696d6f7d8538341b060cffa9565c72ea797e866687e040b86d29cad8799e5fe", + "sha256:4b6b63e49f4ba30d93ec46f87507648c99c9de6911e651ad69db7084fd5b2f4e" ], - "markers": "python_version >= '3.7'", - "version": "==4.2.0" + "markers": "python_version >= '3.8'", + "version": "==4.2.1" }, "django-webtest": { "hashes": [ @@ -1306,11 +1313,11 @@ }, "typing-extensions": { "hashes": [ - "sha256:06006244c70ac8ee83fa8282cb188f697b8db25bc8b4df07be1873c43897060c", - "sha256:3a8b36f13dd5fdc5d1b16fe317f5668545de77fa0b8e02006381fd49d731ab98" + "sha256:88a4153d8505aabbb4e13aacb7c486c2b4a33ca3b3f807914a9b4c844c471c26", + "sha256:d91d5919357fe7f681a9f2b5b4cb2a5f1ef0a1e9f59c4d8ff0d3491e05c0ffd5" ], "index": "pypi", - "version": "==4.6.2" + "version": "==4.6.3" }, "urllib3": { "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 8f3307d80..b8b32df41 100644 --- a/src/registrar/config/settings.py +++ b/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" diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py new file mode 100644 index 000000000..17ad2c329 --- /dev/null +++ b/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) 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..276dc4b09 100644 --- a/src/registrar/views/health.py +++ b/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( 'OK - Get.govOK' diff --git a/src/requirements.txt b/src/requirements.txt index f0ac3966c..a7a1cdddc 100644 --- a/src/requirements.txt +++ b/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 diff --git a/src/zap.conf b/src/zap.conf index b79fac71e..b2234250e 100644 --- a/src/zap.conf +++ b/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)