Tests for newly CSRF-protected routes

[robjwells]

This commit squashes together a lot of work on tests for several destructive routes that were previously unprotected against CSRF attacks (plus the 'make announcement' route, which was fine, in order to better understand the 'delete announcement' route). This PR depends on #369, which was also spun out of the original #366 after feedback, for the tests to actually pass and so it’s marked as a draft PR for the moment.

Included are new tests for:

• Making and deleting announcements,
• Toggling registration/posting/captchas,
• User creation of invite codes.

In addition, there are new test helpers for creating models, logging-in users, retrieving CSRF tokens (without making a request) etc.

This is all squashed together as I couldn't arrive at a clean history separate from the rest of the work where the tests would still pass at every revision, so I thought it better to commit the final result. Please be assured these tests were arrived at over the course of understanding, characterising and refactoring the routes, and not just conjured up after the fact.

The original history of the work, in case it’s needed for diagnosing problems, is at robjwells/close-csrf-hole-for-do-actions.

[happy-river]

The tests are a worthwhile addition to the project, but I'm not so sure about the test helpers.

Adding a convenience function for the tests to get a CSRF token without making a request means the tests will fail to test that the CSRF token is actually present on the pages being tested.

Constructing users for the tests by making requests is slower than the factory approach, no doubt, but it also leaves app/views/auth.py's register as the sole authority of what a correctly created user is and makes sure the tests work with correctly created users. Having a User factory that is only used in the tests and doesn't fully create a user and will most likely not be maintained to the same level as register, will lead to tests that aren't testing what we think they are.

It is reasonable to have the tests query the database for sanity checks, such as checking the number of invite codes.

Using the -k flag to pytest, or giving it a module name, are good ways to keep slow tests from slowing down your workflow.

[robjwells]

Thank you very much for your feedback, I really appreciate it.

Adding a convenience function for the tests to get a CSRF token without making a request means the tests will fail to test that the CSRF token is actually present on the pages being tested.

Yeah, this is true. But for me I feel that this is an indication that a separate and specific test is needed for that aspect, which could also do additional checks on the end-result presented to users (such as making sure form actions are right) — something that I'd be very keen to work on.

Stepping back from the specifics of the CSRF token, I think my concern really is that the current set of helpers (test.utilities.csrf_token, test.utilities.register_user and friends) effectively conduct an implicit test in themselves whenever they’re called. I feel that lifting that behaviour into separate, explicit tests would be a benefit in itself, while tests that are not directly concerned with that behaviour don’t have to pay the cost of the implicit test.

Constructing users for the tests by making requests is slower than the factory approach, no doubt, but it also leaves app/views/auth.py's register as the sole authority of what a correctly created user is and makes sure the tests work with correctly created users. Having a User factory that is only used in the tests and doesn't fully create a user and will most likely not be maintained to the same level as register, will lead to tests that aren't testing what we think they are.

I very much agree with you. Specifically for the UserFactory, I’m not sure why I didn’t use app.auth.create_user, which works perfectly fine. In other cases (such as for SubPosts), though, there aren’t equivalents of create_user as the single place of creation logic separate from the user-facing view function.

It is reasonable to have the tests query the database for sanity checks, such as checking the number of invite codes.

Actually the invite-code helpers are the ones I'm pretty desperate to get rid of, and fold them into the application code itself. (Fixing #303 has been my goal this entire time, and in trying to understand how invite codes work, I feel there are opportunities to wrap some repeated logic in nicer internal interfaces.)

Thanks again for taking the time to consider this PR and give your feedback.

[robjwells]

Hey, sorry for the long silence on the PR. It’s going to be a couple of months until I can contribute again, so I’m going to close this draft for now.

[Polsaker]

No worries! Thanks for all the help :)