Add step to show version data and allow for confirmation prior to to reversion

[tysonclugg]

When reversion has been configured for the admin, the history view of an object shows the following:

The issue is that clicking on any of the linked versions immediately results in an attempt at reverting to that version. Regardless of any wording placed around the links, the linked pages are retrieved using HTTP GET and result in an "unsafe" operation, resulting in a violation of the HTTP spec:

If the purpose of such a resource is to perform an unsafe action, then the resource owner MUST disable or disallow that action when it is accessed using a safe request method.

Note use of the word MUST in the wording above (emphasis mine) - this is an absolute requirement and a strict violation of expected web semantics.

The fix seems relatively easy. Show an intermediary page (like Django does when deleting an object), asking the user to confirm their action which should then POST a request that performs the reversion returning a HTTP 301 response the Location header set to either the object history or change page.

[etianen]

Clicking on the link does not revert the version... it shows a preview of the revert, but does not actually perform the revert. The revert is performed when the user clicks "save".

The implementation is that a revert is performed, then the transaction is rolled back. This should undo any effect of rendering the reverted version.

In what was is this unsafe and a violation of HTTP semantics?

[tysonclugg]

From the stack trace that we captured, it appears that revert was called from the revision view and it attempted to delete some of our User instances.

[tysonclugg]

The implementation is that a revert is performed, then the transaction is rolled back. This should undo any effect of rendering the reverted version.

That's an interesting idea, but based on the assumption that there is transaction support in the DB engine being used. There are notes in the Django docs about how particular MySQL configurations don't support transactions.

I'm well aware that #813 has bitten us here and that we're on an older version of django-reversion (3.0.4), but it's still a bad idea to call delete and then assume that a transaction rollback will leave everything in a pristine state. There are other issues that could most certainly bite depending on configuration, such as having the transaction isolation level set to allow dirty reads resulting in 404's as objects disappear momentarily before the rollback kicks in.

My point is you can't know how this behaviour will affect any given application, and you should avoid calls that modify the database in a view that isn't meant to modify the database.

[way-dave]

Right at the top of the admin integration page, it says:

The admin integration requires that your database engine supports transactions. This is the case for PostgreSQL, SQLite and MySQL InnoDB. If you are using MySQL MyISAM, upgrade your database tables to InnoDB!

The semantics of a database transaction are that rolling it back will leave the database exactly as if the transaction has not occured. So from the point of view of the database, it is absolutely fine to assume that a mutating operation, followed by a rollback, leaves the app in a pristine state. The same is true of Django's file storage abstractions.

Other side-effects in other datastores (e.g. redis, queues) triggered by a model save might break this assumption, of course. And that's where the danger is.

But the intention of the revert preview is that it's safe, non-mutating view, and for models that use only the database and file storage abstractions, it is absolutely safe and non-mutating. Just because a user can customize a model to make this not the case by doing some funny business in save(), doesn't make this inherantly unsafe or unsemantic.

Semantically, the revert preview view is supposed to be safe and stateless, so it should be a GET request. And it's implementation for most models is absolutely safe and stateless. Making this a POST request would be unsemantic, as it would imply the view is intended to be mutating.

Instead, I would take the view that the documentation should call out the additional requirements for a model to be used in the admin integration to avoid breaking this assumption.