fix(data): supprimer un compte emporte ses contrats

DATA-012. delete_user supprimait les lignes Contract et laissait les PDF.
Des contrats nominatifs signes restaient donc sur le serveur apres la
suppression du compte, sans plus aucune reference en base : invisibles pour
l application, ingerables par elle, et toujours des donnees personnelles.

Les chemins sont lus **avant** que les lignes partent — apres, plus rien ne
dit ou sont les fichiers — et les fichiers sont retires **apres** le commit.
L ordre compte dans ce sens et pas dans l autre : un echec entre les deux
doit laisser un fichier sans ligne, ce qui est recuperable et correspond
exactement a l etat precedent, plutot qu une ligne sans fichier, qui est un
telechargement en 500 pour toujours.

Un fichier deja absent est journalise en info et ignore ; un fichier
impossible a retirer est journalise en erreur avec ce que ca implique — il
devient orphelin, donc plus rien dans l application ne proposera jamais de
le supprimer. Rien ici ne peut faire echouer la suppression du compte : le
compte est la partie que quelqu un a demandee.

Le nombre de fichiers retires part dans le journal d authentification, a
cote de account.deleted.

554 tests.

app/routes/users/accounts.py

@@ -42,6 +42,7 @@ from app.routes.users._shared import (
     update_user_gamertags,
 )
 from app.routes.users.blueprint import users_bp
+from app.storage import discard_documents
 from app.validators import CreateUserSchema, EditUserSchema
 
 
@@ -249,6 +250,15 @@ def delete_user(user_id):
         db.or_(OneOnOneRequest.player_id == user_id, OneOnOneRequest.coach_id == user_id),
     ).delete(synchronize_session=False)
     UserGamertag.query.filter_by(user_id=user_id).delete()
+
+    # Read the file paths before the rows go: afterwards there is nothing
+    # left to say where the PDFs are (DATA-012). The files themselves are
+    # removed after the commit, below.
+    contract_files = [
+        path
+        for contract in Contract.query.filter_by(player_id=user_id).all()
+        for path in (contract.file_path, contract.signed_file_path)
+    ]
     Contract.query.filter_by(player_id=user_id).delete()
     TryoutRegistration.query.filter_by(player_id=user_id).delete()
     TeamPlayer.query.filter_by(player_id=user_id).delete()
@@ -265,6 +275,12 @@ def delete_user(user_id):
     deleted_username, deleted_role = user.username, user.role
     db.session.delete(user)
     db.session.commit()
+
+    # After the commit, deliberately. A failure here leaves a file with no
+    # row — recoverable, and exactly what happened before this existed —
+    # rather than a row with no file, which is a download that 500s for ever.
+    discarded = discard_documents(contract_files)
+
     log_auth_event(
         'account.deleted',
         actor=current_user.username,
@@ -272,6 +288,7 @@ def delete_user(user_id):
         target=deleted_username,
         target_id=user_id,
         role=deleted_role,
+        contract_files_removed=discarded,
     )
     flash(
         _('User %(deleted_username)s has been removed.', deleted_username=deleted_username),

app/storage.py

@@ -43,6 +43,48 @@ def documents_root():
     return os.path.join(os.path.dirname(package_dir), 'documents')
 
 
+def discard_documents(stored_paths):
+    """Remove these documents from disk. Returns how many went (DATA-012).
+
+    `delete_user` removed the Contract rows and left the PDFs. Signed,
+    named contracts therefore stayed on the server after the account was
+    deleted, with nothing in the database pointing at them — invisible to
+    the application, unmanageable through it, and still personal data.
+
+    Call this **after** the commit that removed the rows, never before: a
+    failure between the two should leave a file with no row (recoverable,
+    and what the previous behaviour produced anyway) rather than a row with
+    no file (a download that 500s for ever).
+
+    A path that cannot be removed is logged and skipped. Nothing here should
+    be able to abort the deletion of an account.
+    """
+    import logging
+
+    logger = logging.getLogger(__name__)
+    removed = 0
+    for stored_path in stored_paths:
+        if not stored_path:
+            continue
+        target = document_path(stored_path)
+        try:
+            os.remove(target)
+            removed += 1
+        except FileNotFoundError:
+            # Already gone. Two contracts sharing a stem, or a previous
+            # attempt: not a problem, and not worth an error line.
+            logger.info('Document already absent: %s', target)
+        except OSError as exc:
+            logger.error(
+                'Could not remove %s (%s). It is now an orphan: no database row '
+                'refers to it, so nothing in the application will ever offer to '
+                'delete it again.',
+                target,
+                exc,
+            )
+    return removed
+
+
 def document_path(stored_path):
     """Absolute path of a document, from what the database holds.
 

tests/test_storage.py

@@ -18,6 +18,7 @@ import os
 
 import pytest
 
+from app.extensions import db
 from app.storage import CONTRACTS_DIR, document_path, documents_root
 
 
@@ -144,3 +145,81 @@ def _pdf():
     import io
 
     return io.BytesIO(b'%PDF-1.4\n%%EOF\n')
+
+
+class TestDeletingAnAccountTakesItsDocuments:
+    """DATA-012. `delete_user` removed the Contract rows and left the PDFs.
+
+    Signed, named contracts stayed on the server after the account was
+    deleted, with nothing in the database pointing at them: invisible to the
+    application, unmanageable through it, and still personal data.
+    """
+
+    @pytest.fixture
+    def player_with_contract(self, app, client, as_role, make_user):
+        as_role('admin')
+        player_id = make_user('player')
+
+        client.post(
+            '/users/contracts/upload',
+            data={
+                'player_id': str(player_id),
+                'contract_file': (_pdf(), 'contract.pdf'),
+            },
+            content_type='multipart/form-data',
+            follow_redirects=True,
+        )
+
+        from app.models import Contract
+
+        with app.app_context():
+            contract = Contract.query.one()
+            return player_id, document_path(contract.file_path)
+
+    def test_the_file_goes_with_the_account(self, app, client, player_with_contract):
+        player_id, path = player_with_contract
+        assert os.path.exists(path), 'the fixture never wrote the file'
+
+        client.post(f'/users/{player_id}/delete', follow_redirects=True)
+
+        from app.models import Contract
+
+        with app.app_context():
+            assert Contract.query.count() == 0
+        assert not os.path.exists(path), 'the row went and the PDF stayed'
+
+    def test_a_file_already_gone_does_not_stop_the_deletion(
+        self, app, client, player_with_contract
+    ):
+        """Removing a file must never be able to abort the removal of an
+        account: the account is the part somebody asked for."""
+        player_id, path = player_with_contract
+        os.remove(path)
+
+        response = client.post(f'/users/{player_id}/delete', follow_redirects=True)
+
+        assert response.status_code == 200
+        from app.models import User
+
+        with app.app_context():
+            assert db.session.get(User, player_id) is None
+
+
+class TestDiscardDocuments:
+    def test_it_reports_how_many_it_removed(self, tmp_path, monkeypatch):
+        from app.storage import discard_documents
+
+        monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path))
+        (tmp_path / 'a.pdf').write_bytes(b'%PDF-')
+        (tmp_path / 'b.pdf').write_bytes(b'%PDF-')
+
+        assert discard_documents(['a.pdf', 'b.pdf', 'never-existed.pdf']) == 2
+
+    def test_a_none_path_is_skipped(self, tmp_path, monkeypatch):
+        """signed_file_path is NULL until the player signs, and both paths of
+        every contract are passed in together."""
+        from app.storage import discard_documents
+
+        monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path))
+
+        assert discard_documents([None, '']) == 0