Fix allow-list handling with subresources

Problem
-------

When Via HTML is using Checkmate's allow-list (`CHECKMATE_ALLOW_ALL` is
disabled) website subresources (CSS, images, etc) are getting blocked
because they're not on the allow-list.

`AuthenticationView` allows the subresource requests because they have
either a `Sec-Fetch-Site: same-origin` header or a `Referer` header that
refers to Via itself:

https://github.com/hypothesis/viahtml/blob/24a8d24c2188b4b30909d4df55efaea41740f8ef/viahtml/views/authentication.py#L70-L90

But `BlocklistView` then passes `allow_all=False` to Checkmate (because
the `CHECKMATE_ALLOW_ALL` setting is disabled) and Checkmate blocks the
URLs because they're not on the allow-list:

https://github.com/hypothesis/viahtml/blob/24a8d24c2188b4b30909d4df55efaea41740f8ef/viahtml/views/blocklist.py#L40-L47

Solution
--------

The `allow_all` parameter to Checkmate needs to vary depending on the
request's origin:

1. If the request is a top-level document request that's being allowed
   because it has a `Referer` that's on the `VIA_ALLOWED_REFERRERS` list
   then `allow_all` should be either `True` or `False` depending on the
   `CHECKMATE_ALLOW_ALL` setting.

   This happens in the LMS context when the LMS app redirects the
   browser to Via HTML. In this context `CHECKMATE_ALLOW_ALL` is `True` -
   we don't use the allow-list in the LMS context.

   This also happens in the public context when Via 3 redirects the
   browser to Via HTML. In this context `CHECKMATE_ALLOW_ALL` is
   `False` - we _do_ use the allow-list in the public context.

   **Further work:** when the referrer is Via 3 it's technically
   unnecessary for Via HTML to call Checkmate at all since Via 3 will
   already have done so. But Via HTML doesn't currently have any way to
   distinguish Via 3 from other allowed referrers that won't have called
   Checkmate themselves (like LMS). Also if we're going to skip the
   blocklist we might want to verify that the referrer really was Via 3
   in some more secure way than just looking at the `Referer` header. So
   there's no way to implement this optimization currently.

2. If the request is being allowed because Via HTML's `Referer` /
   `Sec-Fetch-Site`-based authentication is disabled (the
   `VIA_DISABLE_AUTHENTICATION` setting is on) then `allow_all` should
   come from the `CHECKMATE_ALLOW_ALL` setting. This can be used in dev
   (though it's not by default).

3. If the request is a subresource request that's being allowed because
   it has a `Sec-Fetch-Site: same-origin` header or because it has a
   `Referer` header that refers to Via itself then `allow_all` should
   always be `True` regardless of the `CHECKMATE_ALLOW_ALL` setting -
   we don't apply the allow-list to subresource requests.

   This third case is the change that actually fixes the bug.

   This needs to happen whether the `VIA_DISABLE_AUTHENTICATION` setting
   is on or off: when `VIA_DISABLE_AUTHENTICATION` is on we still need
   to look for same-origin `Sec-Fetch-Site` and `Referer` headers so we
   can identify subresource requests and set `allow_all` to `True`.

Refactoring
-----------

This means that the logic of `AuthenticationView` and `BlocklistView` is
coupled: `AuthenticationView` decides whether a request is a subresource
request or not, and based on this decision `BlocklistView` needs to
alter the `allow_all` param to Checkmate.

For this reason I've merged `AuthenticationView` and `BlocklistView`
into a new `SecurityView` that contains everything that
`AuthenticationView` and `BlocklistView` used to do before, plus the
above change.

This results in a `SecurityView` class that triggers PyLint's
`too-many-arguments` warning, but I think the class is actually quite
simple. It's less than 150 lines and it's good to have all the security
code and tests together in one place.

This class is much more thoroughly unit-tested than the previous two
classes were.

Further work
------------

I think the `VIA_DISABLE_AUTHENTICATION` setting can be deleted as it's
not used by default (even in dev) and not needed. This'll simplify
things. I'll send a follow-up PR to do that.

.pydocstyle

@@ -1,8 +1,6 @@
 [pydocstyle]
-ignore = D104,  # Missing docstring in public package
-                # Prevents us from having useless comments in __init__.py
-         D105,  # Missing docstring in magic method
-         D107,  # Missing docstring in __init__
+ignore = # Missing docstrings
+         D100,D101,D102,D103,D104,D105,D106,D107,
          D202,  # "No blank lines allowed after function docstring" conflicts with
                 # the Black code formatter, which insists on inserting blank lines
                 # after function docstrings.

.pylintrc

@@ -15,12 +15,14 @@ enable=
     bad-inline-option,
     deprecated-pragma,
     useless-suppression,
-    use-symbolic-message-instead
+    use-symbolic-message-instead,
 
 disable=bad-continuation,
         missing-type-doc,
+        missing-return-doc,
         missing-return-type-doc,
-        too-few-public-methods
+        too-few-public-methods,
+        missing-docstring,
 
 [REPORTS]
 output-format=colorized

tests/unit/conftest.py

@@ -38,5 +38,5 @@ def start_response():
 @pytest.fixture
 def context(start_response):
     return create_autospec(
-        Context, instance=True, spec_set=True, start_response=start_response
+        Context, instance=True, spec_set=True, debug=True, start_response=start_response
     )

tests/unit/viahtml/app_test.py

@@ -27,7 +27,7 @@ def test_it_applies_post_app_hooks(self, apply_post_app_hooks, Hooks):
     @pytest.mark.parametrize(
         "variable,value,expected",
         (
-            ("VIA_DEBUG", "value", {"debug": "value"}),
+            ("VIA_DEBUG", True, {"debug": True}),
             (
                 "VIA_IGNORE_PREFIXES",
                 "  value_1, value_2 ",
@@ -133,7 +133,7 @@ def test_it_applies_views(
 
         result = app(environ, start_response)
 
-        Context.assert_called_once_with(environ, start_response)
+        Context.assert_called_once_with(True, environ, start_response)
         view.assert_called_once_with(Context.return_value)
         assert result == view.return_value
 
@@ -248,8 +248,7 @@ def Hooks():
 def with_patched_views(patch):
     for view_class in (
         "StatusView",
-        "AuthenticationView",
-        "BlocklistView",
+        "SecurityView",
         "RoutingView",
     ):
         view = patch(f"viahtml.app.{view_class}")

tests/unit/viahtml/context_test.py

@@ -186,7 +186,7 @@ def environ(self):
 
     @pytest.fixture
     def context(self, environ, start_response):
-        return Context(environ, start_response)
+        return Context(debug=True, http_environ=environ, start_response=start_response)
 
     @pytest.fixture
     def wsgi(self, patch):