diff --git a/app/.env.example b/app/.env.example index e6cc4d2..2329195 100644 --- a/app/.env.example +++ b/app/.env.example @@ -95,8 +95,37 @@ DISCORD_REDIRECT_URI=https://your-domain/auth/discord/callback # application. Set it to a path OUTSIDE the deployment directory if you move # to a release-directory layout, or a deployment will take the documents with # it (OPS-011, app/storage.py). +# +# IMPORTANT: the backup script reads this same variable. Before wave J it +# did not, and archived `./documents` regardless — so setting this here and +# nowhere else produced empty contract backups that still exited 0. DOCUMENTS_ROOT= +# Where the log files go. Empty means `logs/` beside the application. Both +# defaults are anchored on the application, not on the directory the process +# was started from, which is what they used to be (OBS-006). +LOG_DIR= + +# Where the backup script writes its archives. Empty means `backups/` beside +# the application. +BACKUP_DIR= + +# ============================================================================= +# Optional — rate limiting +# ============================================================================= + +# Where the rate limiter keeps its counters. Empty means `memory://`, which +# is correct for a single Waitress process and is what this deployment runs. +# +# Set it to a shared backend (redis://…) BEFORE running more than one worker: +# in-memory counters are per-process, so N workers let through N times every +# configured limit, with nothing to show for it in the logs. +# +# Note that shared storage does not by itself make the limits sound: they are +# keyed on the client IP, which is forgeable until TRUSTED_PROXY is set +# correctly (SEC-WEB-002 / OPS-002 — see above). +RATELIMIT_STORAGE_URI= + # Tables are created at startup when missing. Set to false once Alembic owns # the schema (DB-002/DB-004): create_all() never ALTERs, so a column added to # a model is silently absent from an existing database. diff --git a/app/logging_config.py b/app/logging_config.py index a126cf9..b221ec2 100644 --- a/app/logging_config.py +++ b/app/logging_config.py @@ -14,6 +14,8 @@ import os import re from logging.handlers import RotatingFileHandler +from app.storage import logs_root + #: Value used when a record is emitted outside a request — startup, the #: Discord bot thread, the scheduler. Short and obviously not an id, so a #: grep for one never matches it by accident. @@ -117,7 +119,12 @@ def configure_logging(app): Args: app: The Flask application instance to configure logging for. """ - log_dir = os.path.join(os.getcwd(), 'logs') + # Anchored on the project, not on the working directory (OBS-006). The + # old form put the logs wherever the process happened to be started + # from, so a service restarted by hand from another directory quietly + # began writing somewhere else — and the file you go looking at when + # something is wrong is the one that must not move. + log_dir = logs_root() os.makedirs(log_dir, exist_ok=True) # Remove default Flask handlers to avoid duplicate logging diff --git a/app/storage.py b/app/storage.py index dc789c9..40d54c9 100644 --- a/app/storage.py +++ b/app/storage.py @@ -1,4 +1,13 @@ -"""Where uploaded documents live, and how the database refers to them. +"""Where the application's files live, and how the database refers to them. + +Three roots: the project itself, the document store, and the log directory. +They are here together because they are the same defect three times over +(OBS-006) — `os.path.join(os.getcwd(), …)`, evaluated at import or upload +time, so every one of them moved with whatever directory the process was +started from. The document store was fixed first, in wave G, because it was +the one that also blocked OPS-011; the other two were left behind, which is +the repeated lesson of this project: a faulty pattern corrected in one layer +stays in the others. Contracts were stored at `os.path.join(os.getcwd(), 'documents', …)`, evaluated at upload time, and the resulting absolute path was written into @@ -26,21 +35,60 @@ import os #: inside whichever one is current. DOCUMENTS_ROOT_ENV = 'DOCUMENTS_ROOT' +#: Environment override for the log directory. Same reasoning: a release +#: directory that carries its own logs loses them at the next switch. +LOG_DIR_ENV = 'LOG_DIR' + +#: Environment override for the backup directory. +BACKUP_DIR_ENV = 'BACKUP_DIR' + #: Sub-directory holding uploaded contracts, under the document root. CONTRACTS_DIR = 'contrats signés' +def project_root(): + """Absolute path of the project, derived from this file's location. + + The anchor every other root falls back to. Not `os.getcwd()`: the + process is started by Waitress under Pterodactyl, by pytest, by a task + scheduler and by a person in a shell, and only one of those four is + reliably in the project directory. + """ + package_dir = os.path.dirname(os.path.abspath(__file__)) + return os.path.dirname(package_dir) + + def documents_root(): """Absolute path of the document store. Falls back to `documents/` beside this package — the project root wherever it is installed, rather than wherever the process was launched. """ - configured = os.getenv(DOCUMENTS_ROOT_ENV) + return _rooted(DOCUMENTS_ROOT_ENV, 'documents') + + +def logs_root(): + """Absolute path of the log directory. + + Was `os.path.join(os.getcwd(), 'logs')`. Starting the server from + another directory sent the logs somewhere new without a word, which is + the worst possible failure mode for the one file you go and read when + something else has gone wrong. + """ + return _rooted(LOG_DIR_ENV, 'logs') + + +def backups_root(): + """Absolute path of the backup directory.""" + return _rooted(BACKUP_DIR_ENV, 'backups') + + +def _rooted(env_name, default_name): + """The configured path, or `default_name` under the project root.""" + configured = os.getenv(env_name) if configured: return os.path.abspath(configured) - package_dir = os.path.dirname(os.path.abspath(__file__)) - return os.path.join(os.path.dirname(package_dir), 'documents') + return os.path.join(project_root(), default_name) def discard_documents(stored_paths): diff --git a/app/supporting_scripts/backup.py b/app/supporting_scripts/backup.py index 53d7475..0b6aa75 100644 --- a/app/supporting_scripts/backup.py +++ b/app/supporting_scripts/backup.py @@ -37,13 +37,33 @@ import sys from datetime import datetime, timedelta from urllib.parse import unquote, urlparse +# Run as `python app/supporting_scripts/backup.py`, sys.path[0] is this +# script's directory, so the application package is not importable. It has +# to be — see DOCUMENTS_DIR below. +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) + +from app.storage import backups_root, documents_root # noqa: E402 — needs the path above + # Configuration -BACKUP_DIR = os.getenv('BACKUP_DIR', os.path.join(os.getcwd(), 'backups')) +BACKUP_DIR = backups_root() BACKUP_RETENTION_DAYS = int(os.getenv('BACKUP_RETENTION_DAYS', 30)) -DOCUMENTS_DIR = os.path.join(os.getcwd(), 'documents') PG_DUMP = os.getenv('PG_DUMP', 'pg_dump') PG_RESTORE = os.getenv('PG_RESTORE', 'pg_restore') +# There is deliberately no DOCUMENTS_DIR constant any more. It held +# `os.path.join(os.getcwd(), 'documents')`, which had stopped being true: +# wave G introduced DOCUMENTS_ROOT so a release-directory deployment could +# keep uploads outside the releases, and docs/deployment.md now tells the +# operator to set it — at which point this script archived a directory the +# application had never written to. It does not fail on a missing directory +# either; it prints "No documents directory found", skips, and exits 0. +# +# So the more correctly an operator followed the deployment documentation, +# the more certainly their contract backups were empty (OBS-006). +# +# backup_documents() now asks app.storage, at call time, the same question +# the upload path asks. One source of truth, and one that a test can move. + class BackupError(Exception): """Raised when a backup step fails in a way that must stop the run.""" @@ -235,20 +255,31 @@ def verify_backup(backup_path): def backup_documents(): """Archive the uploaded contract documents directory. + The directory is resolved through `app.storage.documents_root()` — the + same function the upload path uses — so that setting DOCUMENTS_ROOT + moves both together. Resolved here rather than at import, so that what + is backed up depends on the environment the run has, not on the one the + module happened to be imported with. + Returns: str: Path to the created archive, or None if there is nothing to archive. Signed contracts live only on disk, so losing this directory loses the documents themselves. """ - if not os.path.exists(DOCUMENTS_DIR): - print('[INFO] No documents directory found. Skipping document backup.') + documents_dir = documents_root() + + if not os.path.exists(documents_dir): + # Says where it looked. The previous message named no path, so an + # operator who had moved the documents read it as "there are no + # documents" rather than "I am looking in the wrong place". + print(f'[INFO] No documents directory at {documents_dir}. Skipping document backup.') return None timestamp = datetime.now().strftime('%Y%m%d_%H%M%S') archive_basename = os.path.join(BACKUP_DIR, f'documents_backup_{timestamp}') try: - shutil.make_archive(archive_basename, 'zip', DOCUMENTS_DIR) + shutil.make_archive(archive_basename, 'zip', documents_dir) except Exception as exc: # noqa: BLE001 — a failed document archive must not lose the dump # This runs after the database dump has already succeeded. Letting # anything through here would abort the script with a traceback and @@ -314,6 +345,9 @@ def main(argv=None): print('=== Team Tryouts Backup ===') print(f'Started at: {datetime.now().strftime("%Y-%m-%d %H:%M:%S")}') print(f'Backup directory: {BACKUP_DIR}') + # Printed because it is the value that was wrong for months without + # anyone being able to see it from the output. + print(f'Document source: {documents_root()}') print(f'Retention period: {BACKUP_RETENTION_DAYS} days') print() diff --git a/docs/deployment.md b/docs/deployment.md index 1ce21f6..24329a3 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -270,7 +270,7 @@ Three prerequisites. One is done, two are not: | # | Prerequisite | State | |---|---|---| | 1 | The Pterodactyl startup command must run the app from `current/`, and the server must be restarted on switch | **Panel change.** Cannot be made or verified from the repository | -| 2 | `documents/`, `logs/` and `.env` must sit beside the releases, not inside one. `DOCUMENTS_ROOT` points the document store at a fixed path (`app/storage.py`) | Mechanism ready, **not yet configured on the node** | +| 2 | `documents/`, `logs/` and `.env` must sit beside the releases, not inside one. `DOCUMENTS_ROOT` and `LOG_DIR` point the document store and the logs at fixed paths (`app/storage.py`) | Mechanism ready, **not yet configured on the node** | | 3 | Contract paths must be relative to that root, or the first switch strands every contract ever uploaded | **Done.** New rows store a relative path; rows written earlier keep their absolute one and still resolve, so no data migration is needed | Prerequisite 3 was a defect on its own, not just a blocker: paths were built @@ -278,6 +278,20 @@ from `os.getcwd()`, so starting the server from a different directory would have sent new contracts to a new tree and made the existing ones unreadable — with the database still claiming they were there. +**The same defect was in two more places, and one of them mattered more.** +`logs/` and `backups/` were built from `os.getcwd()` too (OBS-006), and the +backup script kept its own copy of the document path — so it archived +`./documents` no matter what `DOCUMENTS_ROOT` said. Following prerequisite 2 +was therefore enough, on its own, to make every contract backup empty; the +script prints `No documents directory…` and still exits 0, so a scheduled +task watching the exit code would have seen green indefinitely. All three +roots now come from `app/storage.py`, and the backup run prints the document +source it used. + +**After setting `DOCUMENTS_ROOT` on the node, run the backup once by hand** +and check the `Document source:` line and the size of the resulting +`documents_backup_*.zip`. + ## Security Checklist - [ ] `.env` is not committed to repository diff --git a/tests/test_filesystem_roots.py b/tests/test_filesystem_roots.py new file mode 100644 index 0000000..94c6f86 --- /dev/null +++ b/tests/test_filesystem_roots.py @@ -0,0 +1,144 @@ +"""Where the application puts its files, and who agrees about it. + +OBS-006. Three roots were built as `os.path.join(os.getcwd(), …)`: the +document store, the log directory and the backup directory. Each therefore +moved with whatever directory the process was started from — and the +processes that start this application are Waitress under Pterodactyl, a task +scheduler, pytest, and a person in a shell. Only one of the four is reliably +in the project directory. + +The document store was fixed in wave G. The other two were not, and the gap +that opened between them is the reason this file exists: `backup.py` kept +archiving `./documents` while the application wrote to `DOCUMENTS_ROOT`, and +the script's answer to a missing directory is to print a line and exit 0. +Following the deployment documentation was what broke it. +""" + +import os + +import pytest + +from app.storage import backups_root, documents_root, logs_root, project_root + + +class TestTheProjectAnchor: + def test_it_is_the_directory_holding_the_app_package(self): + assert os.path.isdir(os.path.join(project_root(), 'app')) + + def test_it_does_not_follow_the_working_directory(self, tmp_path, monkeypatch): + """The whole point. Every root below inherits this.""" + before = project_root() + monkeypatch.chdir(tmp_path) + + assert project_root() == before + + +@pytest.mark.parametrize( + ('root', 'env_name', 'default_name'), + [ + (documents_root, 'DOCUMENTS_ROOT', 'documents'), + (logs_root, 'LOG_DIR', 'logs'), + (backups_root, 'BACKUP_DIR', 'backups'), + ], +) +class TestEachRoot: + def test_it_defaults_beside_the_project(self, root, env_name, default_name, monkeypatch): + monkeypatch.delenv(env_name, raising=False) + + assert root() == os.path.join(project_root(), default_name) + + def test_it_ignores_the_working_directory( + self, root, env_name, default_name, tmp_path, monkeypatch + ): + monkeypatch.delenv(env_name, raising=False) + monkeypatch.chdir(tmp_path) + + assert root() == os.path.join(project_root(), default_name) + assert not root().startswith(str(tmp_path)) + + def test_the_environment_wins(self, root, env_name, default_name, tmp_path, monkeypatch): + monkeypatch.setenv(env_name, str(tmp_path / 'elsewhere')) + + assert root() == os.path.abspath(str(tmp_path / 'elsewhere')) + + +class TestTheBackupScriptAgreesWithTheApplication: + """The regression this file was written for. + + `backup.py` held its own `os.getcwd()`-based constant, so the two + answers to "where are the contracts" could differ — and did, for anyone + who set DOCUMENTS_ROOT because docs/deployment.md said to. + """ + + def test_it_archives_the_directory_the_application_writes_to(self, tmp_path, monkeypatch): + from app.supporting_scripts import backup + + store = tmp_path / 'somewhere-else' / 'documents' + (store / 'contrats signés').mkdir(parents=True) + (store / 'contrats signés' / 'a.pdf').write_bytes(b'%PDF-1.4 test') + monkeypatch.setenv('DOCUMENTS_ROOT', str(store)) + monkeypatch.setattr(backup, 'BACKUP_DIR', str(tmp_path / 'backups')) + (tmp_path / 'backups').mkdir() + + archive = backup.backup_documents() + + assert archive is not None, ( + 'the script skipped, which is what it did on production while reporting success' + ) + assert os.path.getsize(archive) > 0 + + def test_the_archive_actually_contains_the_contract(self, tmp_path, monkeypatch): + """An empty zip is still a file of non-zero size. This is the + assertion that would have caught the original defect.""" + import zipfile + + from app.supporting_scripts import backup + + store = tmp_path / 'documents' + (store / 'contrats signés').mkdir(parents=True) + (store / 'contrats signés' / 'contrat.pdf').write_bytes(b'%PDF-1.4 test') + monkeypatch.setenv('DOCUMENTS_ROOT', str(store)) + monkeypatch.setattr(backup, 'BACKUP_DIR', str(tmp_path / 'backups')) + (tmp_path / 'backups').mkdir() + + archive = backup.backup_documents() + + with zipfile.ZipFile(archive) as zf: + assert any(name.endswith('contrat.pdf') for name in zf.namelist()) + + def test_a_missing_store_names_the_path_it_looked_in(self, tmp_path, monkeypatch, capsys): + """ "No documents directory found" read as "there are no documents" + rather than "I am looking in the wrong place".""" + from app.supporting_scripts import backup + + missing = tmp_path / 'not-here' + monkeypatch.setenv('DOCUMENTS_ROOT', str(missing)) + + assert backup.backup_documents() is None + assert str(missing) in capsys.readouterr().out + + +class TestLogsFollowTheSameRule: + def test_configure_logging_writes_where_logs_root_says(self, tmp_path, monkeypatch): + from app.app import create_app + + target = tmp_path / 'var' / 'log' + monkeypatch.setenv('LOG_DIR', str(target)) + + create_app( + { + 'SECRET_KEY': 'test-secret-not-used-anywhere-real', + 'SQLALCHEMY_DATABASE_URI': f'sqlite:///{tmp_path / "t.sqlite"}', + 'TESTING': True, + 'WTF_CSRF_ENABLED': False, + 'FORCE_HTTPS': False, + 'SESSION_COOKIE_SECURE': False, + 'ENABLE_DISCORD_BOT': False, + 'AUTO_CREATE_TABLES': True, + 'CORS_ALLOWED_ORIGINS': '', + 'RATELIMIT_ENABLED': False, + } + ) + + assert target.is_dir(), 'the log directory is created where LOG_DIR points' + assert (target / 'app.log').exists()