happy-river
commented
3 years ago This would be better as three pull requests (CSRF tokens, the non-null constraint migration, and tests) than as one, since all these commits will be squashed together when merged. Our future selves will thank us for giving them smaller, single-topic lists of revisions to sort through, if they have to come back to look at one of the changes in this PR.
Making the buttons on the admin page display below their labels as before can probably be solved by wrapping the labels in a <div>.
Flask has a function called flash which can be used to display a message after a redirect, which you could use on the delete announcement route.
The migration to add the non-null constraints looks good. I checked our production database and there are no nulls in those fields.
robjwells
This pull request converts a few outstanding routes that perform destructive actions to use POST requests only, requiring a valid CSRF token in the posted data. This closes a few unlikely and low-severity CSRF vulnerabilities.
CSRF vulnerabilities
The routes in question are:
/do/admin/enable_posting/do/admin/enable_registration/do/admin/require_captchas/do/admin/deleteannouncement/do/create_inviteCurrently, should a malicious party convince an authenticated user to click a link to any of these, the corresponding action will be performed. For the first four routes, this user has to be an administrator, so this would have to be highly targeted.
While these are destructive in that they cause changes to the state of the application, for the first three routes which act as toggles, the user is redirected to the admin index page and a message is displayed describing the action taken, allowing the user to quickly reverse the action.
The announcement-deleting route also redirects the user to the admin index page, though it may not be so obvious that there is now no announcement set.
And while the final route will create a new invite code from the user’s pool, this is really just an annoyance.
Contents of the pull request
This pull request looks quite large but the vast majority of the additions are in new tests for the routes and some additional helper functions.
Please note that this PR also includes changes to some Sub and SubPost model constraints to disallow null in some fields. This is only the case where such a null field would cause an exception in the application (or, for SubPost.title, cause its own
__repr__to fail). (This came about while writing test helpers, which is why this change is present in this PR.)I have written a migration to add the not-null constraints in the database, but this is a schema-only migration. I think it’s probably unlikely that nulls exist in these columns in the real world, given the effect on the application, but I would very much appreciate if this could be reviewed carefully. (It, er, also wasn’t clear to me how to perform an appropriate data migration for null fields in all cases.)
Template changes
Most of the template changes are straightforwardly swapping links for forms. On the admin page the buttons display slightly differently than before; I’m not sure what is preferable and I’m not really familiar with the CSS so I’d appreciate some guidance here if it’s not quite right.
Additional dependencies
I’ve added factory-boy and Faker as dependencies, but these are just test dependencies to make creating model instances easier and faster. I think there’s scope to extend the use of these factories, as well as some other helpers that I’ve written for this PR, to other tests. But I haven’t done that here, not least because there will need to be some consideration where the logic of the existing helpers (by, eg, registering a user by making requests to the application) is actually an important part of the behaviour tested and not just incidental complexity.