Serve error pages (as per ViaHTML) from Checkmate

[jon-betts]

For: #15

This adds a few things:

• A view which will render an error page in HTML given a url and reason code
• A service which signs params and creates URLs to the view, and checks that params are signed correctly
• Updates the main checking code to add the signed URL in the feedback so the client can use it

Review notes:

• The caching here is a fixed 3600 seconds (1h) without any fancy stuff like cache invalidation (i.e. whitenoise)
• This means if we deploy changes, they might take 1h to show up in the wild
• The templates are as they are in ViaHTML at the moment (without the new additions to Via)

Testing notes:

• Run make dev
• Check a URL like: http://localhost:9099/api/check?url=http://bad.web.app
• Get the links.html out of the JSON and look at it to view the error page
• Try changing params (edit, remove, add) - This should result in a 401

[jon-betts]

Ah... so sue me. We need to create a weird situation for testing here where something is "signed" but not correctly. Also... it's convenient

[seanh]

Posting a partial review with some suggestions. I got part way through reviewing this and ran out of time, I'll continue next week

[seanh]

👍 We sometimes use values like "not_a_secret" for these

[seanh]

👍 I like the simple approach to config settings. If the CHECKMATE_SECRET envvar is missing the app will simply crash on startup and it'll be pretty obvious why, and this keeps our code simple. h and lms support reading any setting from either the config file or the environment, but I don't think that's really necessary

[seanh]

What's the reasoning for the cache_max_age=3600?

I think 3600 is the default value.

Both h and lms do cache_max_age=None:

• https://github.com/hypothesis/lms/blob/7c10eb3ddae26434133b59b96d7874b851f61d94/lms/assets.py#L145
• https://github.com/hypothesis/h/blob/f46082b4a3f5fe1f7c70898ed6ba224cca5c35a8/h/assets.py#L135

At first I thought None must disable caching, but actually I think it might allow caching forever, because both h and lms add what look like cache-busting query params onto static asset URLs.

We might want to do that for Checkmate too so that we can let assets be cached forever and (more importantly) so that we don't have trouble with old cached versions of assets taking time to expire. I'm not exactly sure how h and lms do it but Pyramid has a built-in cache-busting thing that changes a cache-busting query param each time the app server restarts: https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/assets.html#cache-busting That's not going to be perfect for us because each app server will cache each asset separately, but it might be good enough? You can also customize the cache buster so we could put in a better mechanism.

[jon-betts]

Until we have some cache busting we can't serve the content with an infinite expiry date. And it's not trivial to add. We have delayed most of the fancy caching for checkmate as not being essential.

If 3600 is the default, it doesn't hurt to make it clear it's happening, and we do want some caching on this content, otherwise it's going to be served live every time.

[seanh]

I suppose, since these are just error pages with a mailto link rather than an interface that you actually use (like an admin page, or fancier error pages), and since the only assets are a CSV file and an SVG, a one-hour delay in someone seeing an updated version of the page or the page loading with different versions of different assets (e.g. new CSV but old SVG) is unlikely to cause many problems.

In more complex cases I think old cached versions can cause breakage. So we'll probably have to deal with cache-busting when we add static assets for the admin pages (unless we use a different static view for those and don't cache them) or if the error pages become much fancier.

[jon-betts]

Yeah, we will want it, I just remember it not being trivial to add

[seanh]

:return: Boolean should be :rtype: bool.

I think verify_*() might be a better name than signed_*(), since verifying the args is what the method does. signed_args() sounds like a method that takes some args and returns a signed copy of them.

I think there's some confusion here because there are two different URLs in play: there's the URL that someone is trying to proxy that has been blocked. And then there's the URL of the block page. But both are called url in this code. Maybe consistently referring to the block page URL as "block page URL" would help (e.g. block_page_url etc). Or alternatively consistently refer to the blocked URL as "blocked URL" (blocked_url etc). Or both, maybe plain url is a confusing name in a class where two different URLs are in play.

I also wonder if, for symmetry, both methods could accept the entire URL rather than just the args? Then it'd be get_url(url, reason) and verify_url(url). (Or alternatively I guess both methods could deal with args only, which would remove the route_url dependency.)

class BlockURLService:
    """Methods for generating and verifying block page URLs."""

    ...

    def __init__(...):
        ...

    def get_block_page_url(self, url, reason):
        """Return the block page URL for the given blocked URL and reason.

        :param url: The URL that has been blocked
        :param reason: The Reason object describing the problem
        :return: The URL of the block page
        """
        ...

    def verify_block_page_url(self, block_page_url):
        """Verify that the given block page URL has been signed correctly.

        :param args: The URL to verify
        :rtype: bool
        """
        ...

[jon-betts]

It's worth pointing out you don't verify the URL, but the arguments which contain a URL. There's some confusion about the names here for sure though. The collision of the block list HTML view, this service and the URL being blocked is a bit unfortunate.

I'm fairly certain you've told me in the past it's verbotten to have get_ on the front of things, but I forget why.

[seanh]

Ah yeah, it's verifying the args not the URL. Maybe it could just work with args then, and no route_url dependency?

class BlockURLArgsService:
    def get_args(self, url, reason):
        ...

    def verify_args(self, args):
        ...

Or maybe give up on the symmetry and have it generate URLs but verify args (same as you have currently but with slightly different names):

class BlockURLService:
    def get_url(self, blocked_url, reason):
        ...

    def verify_args(self, args):
        ...

(Or verify_args() could still take the entire block_page_url as argument but be called verify_args(), then you'd still have the symmetry.)

[seanh]

Regarding get_(): I think avoiding it is a common style in Python because it's considered "just noise" and because you're supposed to use simple attributes or (if necessary) @property's and you wouldn't call an @property something like get_foo it'd just be foo. When something needs arguments it has to be a method but following the same style you'd call it foo(bar) not get_foo(bar). I don't think get_() is forbidden though, it depends on the particular example, for example on whether the method name without get_() is going to be clear enough in a given example, or whether the get_() is just noise.

See for example Do not use get_ here: https://python-consistency.readthedocs.io/en/latest/conventions.html#properties

Or this answer: https://softwareengineering.stackexchange.com/questions/334135/how-should-i-name-functions-that-return-values-in-python

(But again I don't think it's forbidden.)

[seanh]

BlockPageURLService might be a clearer name since it generates URLs for block pages and verifies args from block page URLs. Just BlockURLService sounds like it might have something to do with the actual blocking of URLs (especially in Checkmate's context) but this actually just has to do with the block pages.

[jon-betts]

I've totally reworked this now. I've separated all the knowledge of the block page out of the service and changed it into a SecureLinkService. This has two methods now route_url which works identically to the pyramid route_url but also signs the request (along with the route name now) and is_secure which accepts the request object and checks it.

I think the result is much nicer name-wise. It's also a bit cleaner as the job and the whole job of this class is signing and checking things, not packing params for the block page.

[jon-betts]

Missed a bit of coverage

[seanh]

Much appreciated the testing instructions in the PR description. Tested and works for me.

The commit history looks like it might need a little tidying up before merge.

Lots of small comments and suggestions, but I don't think any of them are required changes so this is approved.

I love the idea of SecureLinkService.route_url() following Request.route_url(). I bet we could go one step further and add it as a request method, Request.secure_route_url() (and maybe also Request.is_secure_link as a property). And I suspect those could still call a SecureLinkService behind the scenes if they had to. And reify=True would handle caching. See: https://docs.pylonsproject.org/projects/pyramid/en/latest/api/config.html#pyramid.config.Configurator.add_request_method. That might be pretty nice but there's no real need: the service approach is just fine.

[seanh]

👍, yeah there seems little point in testing a method like this. My preference would probably be to add a test that simply calls the method but doesn't assert anything, so it at least tests that the method doesn't crash. But I think no cover on something like this is fair enough

[seanh]

Are these actually TOKEN_KEY and VERSION_KEY? They're actually the keys not the values, and "arg" made met think of values rather than keys. They're awkward to name though, *_KEY doesn't seem wonderfully clear either. I think the code might actually be clearer and just as easy to maintain if it just duplicated the strings "sec" and "v" when needed instead of using constants, the indirection and constant names might just add confusion

[seanh]

👍 It's really nice to inject things like secret directly and decouple SecureLinkService from reading a config setting or envvar itself

[seanh]

I love the idea of a generic secure links service and following request.route_url()'s interface

[seanh]

Suggested change

	raise ValueError("You must provide some parameters to sign")
	raise ValueError("You must provide some query parameters to sign")

(Just slightly clearer maybe)

[seanh]

Ok, so it adds &v=1&sec=*** to the URL's query string. Sounds good 👍

We could get defensive and have it raise if either "v" or "sec" are already in args (could just be a couple of assert's). I think as written the method will behave strangely if args already contains "v" and "sec": it'll ovewrite them, but it'll also have signed the version of the args with the original "sec" so it'll return a secure link that actually won't pass the is_secure() test

[seanh]

Ooh, you could totally add a resource and permission for this route that only grants the permission if the is_secure() test passes, then Pyramid would handle the error response, and this view (or any view with permission="secure_link") would only get called if is_secure() had passed.

I think doing it directly in the view like this (bypassing Pyramid's authorization system) is totally acceptable for this though

[seanh]

Given that route_url is a third-party thing, I think it'd be totally fine for these tests to use the real Request.route_url and assert about the URL strings returned. But I'm guessing the mock made the asserts easier to write?

[seanh]

This could be any ValueError, the test doesn't actually test the "you must provide parameters" was the reason the error was raised:

Suggested change

	with pytest.raises(ValueError):
	with pytest.raises(ValueError, match="^You must provide some parameters to sign$")):

[seanh]

👍

[jon-betts]

I've consolidated the commits. This is going to cause some almighty rebasing pain in all the other PRs, but hey ho.

[jon-betts]

Secrets have been added to the live instances so we can deploy this.