From 709e8a5d519b020e1b3e4ce6b96bf80967ad6312 Mon Sep 17 00:00:00 2001 From: GGThed Date: Tue, 11 Aug 2026 15:34:33 -0400 Subject: [PATCH] fix(data): supprimer un compte emporte ses contrats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 17 ++++++++ app/storage.py | 42 +++++++++++++++++++ tests/test_storage.py | 79 ++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/app/routes/users/accounts.py b/app/routes/users/accounts.py index 1d431f8..9b4751d 100644 --- a/app/routes/users/accounts.py +++ b/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), diff --git a/app/storage.py b/app/storage.py index b3bbbca..dc789c9 100644 --- a/app/storage.py +++ b/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. diff --git a/tests/test_storage.py b/tests/test_storage.py index aacd4c5..434c2be 100644 --- a/tests/test_storage.py +++ b/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