diff --git a/.gitea/workflows/git-to-ptero.yaml b/.gitea/workflows/git-to-ptero.yaml index 8c6cdd2..3489c64 100644 --- a/.gitea/workflows/git-to-ptero.yaml +++ b/.gitea/workflows/git-to-ptero.yaml @@ -6,6 +6,34 @@ on: # branches: # - main # Optional: Run automatically on pushes to the main branch +# OPS-011 — what this workflow now guarantees, and what it still does not. +# +# Guaranteed: +# - nothing is uploaded unless the test suite and the linters pass; +# - only files on an explicit allowlist are uploaded, so a new file at the +# repository root does not reach production by default. That is how +# clear_db.py — a script that DROPs every table and recreates +# admin/password — got there in the first place; +# - after the upload, /health is polled until it answers healthy, and the +# job fails loudly if it does not. Before, a half-uploaded tree was a +# green deployment. +# +# NOT guaranteed — the switch is not atomic. Files are mirrored in place, so +# for the length of the transfer production runs a mixture of two versions. +# Closing that needs a release-directory layout, which has three +# prerequisites, two of which cannot be done from here: +# +# 1. the Pterodactyl startup command must run the app from `current/` +# rather than from the server root, and the server must be restarted on +# switch — a panel change; +# 2. `documents/`, `logs/` and `.env` must live beside the releases, not +# inside one. DOCUMENTS_ROOT exists for this (app/storage.py); +# 3. contract paths must be relative to that root, so the switch does not +# strand them. Done: new rows are relative, old absolute ones still +# resolve. +# +# docs/deployment.md carries the design and the rollback procedure. + jobs: deploy-to-sftp: runs-on: ubuntu-latest @@ -16,51 +44,98 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 - - name: Install lftp and ssh - run: sudo apt-get update && sudo apt-get install -y lftp openssh-client + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Install dependencies + run: pip install -r requirements.txt -r requirements-dev.txt + + # The gate. This runner is not the GitHub one, so a green CI over + # there proves nothing about what is about to be shipped from here: + # the deploy is triggered by hand, on whatever the branch holds. + - name: Refuse to deploy a broken tree + run: | + python -m pytest -q + python -m ruff check . + python -m ruff format --check . + + # An allowlist, not a list of exclusions. The previous form mirrored + # the whole working tree minus nine globs, so every file added to the + # repository shipped to production unless someone remembered to + # exclude it. This inverts the default: a new top-level file has to be + # named here to reach the server. + - name: Assemble the release payload + run: | + set -euo pipefail + mkdir -p payload + cp -r app payload/ + cp requirements.txt wsgi.py payload/ + # Compiled catalogues are versioned deliberately: the deployment is + # a file mirror with no build step (docs/translations.md). + find payload -name '__pycache__' -type d -prune -exec rm -rf {} + + find payload -name '*.pyc' -delete + echo "Shipping $(find payload -type f | wc -l) files:" + find payload -maxdepth 2 -type d | sort - name: Set up SSH Private Key env: # Binds the secret to a secure environment variable - SSH_PRIVATE_KEY: ${{ secrets.SSH }} + SSH_PRIVATE_KEY: ${{ secrets.SSH }} run: | mkdir -p ~/.ssh # Uses the environment variable, so the raw key is never printed in the execution log - echo "$SSH_PRIVATE_KEY" > ~/.ssh/id_rsa + echo "$SSH_PRIVATE_KEY" > ~/.ssh/id_rsa chmod 600 ~/.ssh/id_rsa - name: Push files via SFTP with progress run: | - # The mirror command below uses the -R (reverse) flag - # to push from local './' to remote './' - # Connection is made using 'open' inside the execution block to enforce SSH key usage - # - # --exclude-glob entries: the previous command mirrored the entire - # working tree, so CI definitions, the test suite and clear_db.py -- - # a script that DELETEs every table and recreates admin/password -- - # were all shipped to the production node. - # # --delete is deliberately NOT used. Uploaded contracts, logs and the # server's own .env live under the deployment root and are absent # from the repository; deleting anything not present locally would - # destroy them. Stale files therefore accumulate: switching to an - # atomic timestamped-directory deploy is tracked as OPS-011. + # destroy them. Stale files therefore still accumulate — that is the + # other half of what the release-directory layout would fix. lftp -e "set sftp:connect-program 'ssh -a -x -i ~/.ssh/id_rsa -o StrictHostKeyChecking=no -o BatchMode=yes -o PasswordAuthentication=no'; \ set sftp:auto-confirm yes; \ set net:max-retries 5; \ set net:timeout 30; \ set cmd:fail-exit yes; \ open -u ${{ secrets.SSH_USER }}, sftp://sftp.node4.immortal.host:2022; \ - mirror -R --verbose --parallel=4 \ - --exclude-glob .git/ \ - --exclude-glob .github/ \ - --exclude-glob .gitea/ \ - --exclude-glob .venv/ \ - --exclude-glob venv/ \ - --exclude-glob tests/ \ - --exclude-glob audit/ \ - --exclude-glob .ai/ \ - --exclude-glob __pycache__/ \ - --exclude-glob clear_db.py \ - ./ ./; \ - quit" \ No newline at end of file + mirror -R --verbose --parallel=4 ./payload/ ./; \ + quit" + + # Without this a deployment that left the site 500ing reported success, + # and the first person to hear about it was a user. /health checks the + # database connection and reports whether the Discord bot thread is + # alive (OPS-012). + # HEALTH_URL is carried as a secret rather than as a variable. It is + # not secret — it is the public site — but `secrets` is the context + # this runner is already known to support, and a smoke test that fails + # to run because of an unsupported expression is worse than none. + - name: Smoke test + if: ${{ secrets.HEALTH_URL != '' }} + env: + HEALTH_URL: ${{ secrets.HEALTH_URL }} + run: | + set -euo pipefail + # The app is restarted by the panel, not by this workflow, so the + # first few probes are expected to fail or answer from the old + # process. Two minutes, then give up. + for attempt in $(seq 1 24); do + body=$(curl -fsS --max-time 10 "$HEALTH_URL" 2>/dev/null) || body='' + if echo "$body" | grep -q '"status": *"healthy"'; then + echo "Healthy after ${attempt} attempt(s):" + echo "$body" + exit 0 + fi + echo "attempt ${attempt}: not healthy yet" + sleep 5 + done + echo "::error::/health never reported healthy. The deployment is live and may be broken — see the rollback procedure in docs/deployment.md." + exit 1 + + - name: Warn when no health check is configured + if: ${{ secrets.HEALTH_URL == '' }} + run: | + echo "::warning::HEALTH_URL is not set, so this deployment was not verified. Set it to https:///health in the repository secrets." diff --git a/app/routes/users/contracts.py b/app/routes/users/contracts.py index 31888f6..865bc8f 100644 --- a/app/routes/users/contracts.py +++ b/app/routes/users/contracts.py @@ -19,6 +19,7 @@ from app.routes.users._shared import ( pdf_upload_error, ) from app.routes.users.blueprint import users_bp +from app.storage import CONTRACTS_DIR, document_path from app.validators import UploadContractSchema @@ -108,25 +109,24 @@ def upload_contract(): flash(error, 'danger') return redirect(url_for('users.upload_contract')) - upload_dir = os.path.join(os.getcwd(), 'documents', 'contrats signés') - os.makedirs(upload_dir, exist_ok=True) - player = User.query.get_or_404(player_id) player_teams = player.get_org_teams() team = player_teams[0] if player_teams else None - if team: - team_folder = os.path.join(upload_dir, secure_filename(team.name)) - os.makedirs(team_folder, exist_ok=True) - final_dir = team_folder - else: - final_dir = upload_dir - original_filename = secure_filename(file.filename) - file_uuid = str(uuid.uuid4()) - stored_filename = f"{file_uuid}.pdf" - file_path = os.path.join(final_dir, stored_filename) - file.save(file_path) + stored_filename = f"{uuid.uuid4()}.pdf" + + # Kept relative to the document root, not absolute (see app/storage.py): + # an absolute path pins the file to the directory the process was + # started from, which is the one thing a release-directory deploy + # changes. + relative_path = os.path.join(CONTRACTS_DIR, stored_filename) + if team: + relative_path = os.path.join(CONTRACTS_DIR, secure_filename(team.name), stored_filename) + + absolute_path = document_path(relative_path) + os.makedirs(os.path.dirname(absolute_path), exist_ok=True) + file.save(absolute_path) contract = Contract( player_id=player_id, @@ -134,7 +134,7 @@ def upload_contract(): uploaded_by_id=current_user.id, original_filename=original_filename, stored_filename=stored_filename, - file_path=file_path, + file_path=relative_path, notes=notes if notes else None, ) db.session.add(contract) @@ -164,12 +164,11 @@ def upload_signed_contract(contract_id): return redirect(url_for('users.list_contracts')) signed_filename = f"signed_{contract.stored_filename}" - file.save(contract.file_path.replace(contract.stored_filename, signed_filename)) + signed_path = contract.file_path.replace(contract.stored_filename, signed_filename) + file.save(document_path(signed_path)) contract.signed_filename = signed_filename - contract.signed_file_path = contract.file_path.replace( - contract.stored_filename, signed_filename - ) + contract.signed_file_path = signed_path contract.status = 'signed' contract.signed_at = datetime.utcnow() db.session.commit() @@ -186,7 +185,9 @@ def download_contract(contract_id): flash(_('You do not have permission to download this contract.'), 'danger') return redirect(url_for('users.list_contracts')) return send_file( - contract.file_path, as_attachment=True, download_name=contract.original_filename + document_path(contract.file_path), + as_attachment=True, + download_name=contract.original_filename, ) @@ -202,5 +203,7 @@ def download_signed_contract(contract_id): flash(_('No signed contract available.'), 'danger') return redirect(url_for('users.list_contracts')) return send_file( - contract.signed_file_path, as_attachment=True, download_name=contract.signed_filename + document_path(contract.signed_file_path), + as_attachment=True, + download_name=contract.signed_filename, ) diff --git a/app/storage.py b/app/storage.py new file mode 100644 index 0000000..b3bbbca --- /dev/null +++ b/app/storage.py @@ -0,0 +1,59 @@ +"""Where uploaded documents live, and how the database refers to them. + +Contracts were stored at `os.path.join(os.getcwd(), 'documents', …)`, +evaluated at upload time, and the resulting absolute path was written into +`Contract.file_path`. The storage root therefore moved with whatever +directory the process happened to be started from. Two consequences: + + - one latent: start the server from elsewhere and new contracts land in a + new tree while the old ones become unreadable — with the database still + saying they are there, so the failure surfaces as a 500 on download + rather than as anything a person could act on; + - one blocking: it rules out a release-directory deployment (OPS-011) + outright. Every stored path would point inside a release that is about + to be replaced, so the first switch would take every contract ever + uploaded with it. + +New rows keep a path *relative* to the document root. Old rows keep their +absolute path and are returned untouched, so this change needs no data +migration and can ship before Alembic does (DB-002). +""" + +import os + +#: Environment override for the document root. What a release-directory +#: deployment sets, to a path outside the releases — alongside them, not +#: inside whichever one is current. +DOCUMENTS_ROOT_ENV = 'DOCUMENTS_ROOT' + +#: Sub-directory holding uploaded contracts, under the document root. +CONTRACTS_DIR = 'contrats signés' + + +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) + 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') + + +def document_path(stored_path): + """Absolute path of a document, from what the database holds. + + Args: + stored_path: The value of Contract.file_path or signed_file_path. + Relative for rows written since this module existed, absolute + for the ones written before. + + Returns: + str: An absolute path. + """ + if os.path.isabs(stored_path): + return stored_path + return os.path.join(documents_root(), stored_path) diff --git a/docs/deployment.md b/docs/deployment.md index a1e6f6e..72648b4 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -145,6 +145,93 @@ python security_scan.py --url https://yourdomain.com All checks must pass before deployment. +## Deploying, and undoing a deployment (OPS-011) + +Deployment is the Gitea workflow `.gitea/workflows/git-to-ptero.yaml`, run by +hand (`workflow_dispatch`). It mirrors files over SFTP to the Pterodactyl +node. + +### What the workflow guarantees + +1. **Nothing ships from a broken tree.** The suite, `ruff check` and + `ruff format --check` all run on the deploy runner first. A green CI on + GitHub proves nothing here: the deploy is triggered by hand, on whatever + the branch currently holds. +2. **Only named files ship.** The payload is an allowlist — `app/`, + `wsgi.py`, `requirements.txt` — not the working tree minus exclusions. + A new file at the repository root does not reach production unless + somebody adds it. The old form is how `clear_db.py`, the test suite and + the CI definitions got onto the production node. +3. **The deployment is verified.** `/health` is polled for two minutes after + the upload and the job fails if it never reports healthy. Set the + `HEALTH_URL` repository secret to `https:///health`; without it the + workflow warns that the deployment went out unverified. + +Deliberately *not* shipped: `run.py` (development entry point), `tests/`, +`migrations/add_tryout_coaches.py` (a one-off, already applied — run it by +hand if a fresh database ever needs it), `docs/`, `audit/`, `clear_db.py`. + +### What it does not guarantee + +**The switch is not atomic.** Files are mirrored in place, so for the length +of the transfer the node runs a mixture of two versions. And because +`--delete` is off — uploaded contracts, logs and the server's `.env` live +under the deployment root and are absent from the repository — a file +removed from the repository stays on the server for ever. + +### Rolling back + +There is no previous release on the node to switch back to, so a rollback is +a forward deployment of a known-good commit: + +```bash +# 1. Find the last deployment that was verified healthy — the workflow run +# log names the commit. +git log --oneline + +# 2. Deploy that commit. In the Gitea UI, run the "Push to SFTP" workflow +# against the tag or branch pointing at it. Tag known-good releases so +# this step does not depend on reading a log: +git tag -a deploy-2026-08-11 -m "verified healthy" +git push origin deploy-2026-08-11 # requires the push freeze to be lifted + +# 3. Confirm. +curl -fsS https:///health +``` + +A rollback does **not** undo a database migration. If the deployment that +broke production also changed the schema, restore from backup first — +`docs/database-restore.md`. + +### Making the switch atomic + +The remaining work, and why it is not done here. A release-directory layout +looks like this on the node: + +``` +/home/container/ +├── releases/ +│ ├── 2026-08-11-a1b2c3/ +│ └── 2026-08-10-9f8e7d/ +├── current -> releases/2026-08-11-a1b2c3 +├── documents/ # shared, never inside a release +├── logs/ # shared +└── .env # shared +``` + +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** | +| 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 +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. + ## Security Checklist - [ ] `.env` is not committed to repository diff --git a/tests/conftest.py b/tests/conftest.py index 3183d80..969727e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -70,6 +70,18 @@ def _base_config(db_path, csrf=False): } +@pytest.fixture(autouse=True) +def documents_in_a_throwaway_directory(tmp_path, monkeypatch): + """Keep uploads out of the repository. + + Contract uploads land under app.storage.documents_root(), which without + this is `documents/` beside the package — i.e. inside the checkout. No + test writes a file there today; this is so that the first one that does + cannot leave a PDF in someone's working tree. + """ + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'documents')) + + @pytest.fixture def app(): """A fully configured application backed by a throwaway SQLite file. diff --git a/tests/test_storage.py b/tests/test_storage.py new file mode 100644 index 0000000..aacd4c5 --- /dev/null +++ b/tests/test_storage.py @@ -0,0 +1,146 @@ +"""Where uploaded documents live (OPS-011). + +Contract paths were absolute and built from `os.getcwd()`, so the storage +root followed whatever directory the process was started from. That is a +defect on its own — a server restarted from elsewhere writes new contracts +into a new tree and cannot read the old ones, while the database goes on +saying they are there — and it is what made a release-directory deployment +impossible: every stored path would point inside a release about to be +replaced. + +The compatibility case is the one that matters most here. Rows written +before this change hold absolute paths, and they have to keep working +without a data migration, because the migration tooling does not exist yet +(DB-002, blocked). +""" + +import os + +import pytest + +from app.storage import CONTRACTS_DIR, document_path, documents_root + + +class TestDocumentsRoot: + def test_the_environment_wins(self, tmp_path, monkeypatch): + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'elsewhere')) + + assert documents_root() == str(tmp_path / 'elsewhere') + + def test_a_relative_override_is_made_absolute(self, monkeypatch): + monkeypatch.setenv('DOCUMENTS_ROOT', 'docs-here') + + assert os.path.isabs(documents_root()) + + def test_the_default_does_not_follow_the_working_directory(self, tmp_path, monkeypatch): + """The whole point. `os.getcwd()` made this move; the package + location does not.""" + monkeypatch.delenv('DOCUMENTS_ROOT', raising=False) + before = documents_root() + + monkeypatch.chdir(tmp_path) + after = documents_root() + + assert before == after + + def test_the_default_sits_beside_the_package(self, monkeypatch): + monkeypatch.delenv('DOCUMENTS_ROOT', raising=False) + + # From the module file, not from `app.__file__`: `app` has no + # __init__.py, so it is a namespace package and __file__ is None. + from app import storage + + package_dir = os.path.dirname(os.path.abspath(storage.__file__)) + expected = os.path.join(os.path.dirname(package_dir), 'documents') + assert documents_root() == expected + + +class TestDocumentPath: + def test_a_relative_path_is_resolved_against_the_root(self, tmp_path, monkeypatch): + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path)) + + resolved = document_path(os.path.join(CONTRACTS_DIR, 'abc.pdf')) + + assert resolved == str(tmp_path / CONTRACTS_DIR / 'abc.pdf') + + def test_an_absolute_path_is_left_alone(self, tmp_path, monkeypatch): + """Rows written before this module existed. They must keep resolving + to where the file actually is, or every contract uploaded so far + becomes a 500 on download the day this ships.""" + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'new-root')) + legacy = os.path.abspath(os.path.join('C:' + os.sep, 'old', 'place', 'abc.pdf')) + + assert document_path(legacy) == legacy + + def test_moving_the_root_moves_new_documents_and_not_old_ones(self, tmp_path, monkeypatch): + relative = os.path.join(CONTRACTS_DIR, 'abc.pdf') + legacy = os.path.abspath(os.path.join(str(tmp_path), 'legacy', 'abc.pdf')) + + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'one')) + first_new, first_old = document_path(relative), document_path(legacy) + + monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'two')) + second_new, second_old = document_path(relative), document_path(legacy) + + assert first_new != second_new, 'a release switch has to move new documents' + assert first_old == second_old, 'and must not move the ones already filed' + + +class TestThroughTheUploadRoute: + @pytest.fixture + def uploaded(self, app, client, as_role, make_user): + """A contract filed by an admin for a player.""" + as_role('admin') + player_id = make_user('player') + + response = client.post( + '/users/contracts/upload', + data={ + 'player_id': str(player_id), + 'notes': 'Season contract', + 'contract_file': (_pdf(), 'contract.pdf'), + }, + content_type='multipart/form-data', + follow_redirects=True, + ) + assert response.status_code == 200 + return player_id + + def test_the_stored_path_is_relative(self, app, uploaded): + from app.models import Contract + + with app.app_context(): + contract = Contract.query.one() + assert not os.path.isabs(contract.file_path), ( + 'an absolute path pins the file to the directory the process ' + 'was started from — the one thing a release switch changes' + ) + assert contract.file_path.startswith(CONTRACTS_DIR) + + def test_the_file_lands_under_the_configured_root(self, app, uploaded): + from app.models import Contract + + with app.app_context(): + contract = Contract.query.one() + resolved = document_path(contract.file_path) + + assert os.path.exists(resolved) + assert resolved.startswith(documents_root()) + + def test_it_can_be_downloaded_back(self, app, client, uploaded): + from app.models import Contract + + with app.app_context(): + contract_id = Contract.query.one().id + + response = client.get(f'/users/contracts/{contract_id}/download') + + assert response.status_code == 200 + assert response.data.startswith(b'%PDF-') + + +def _pdf(): + """The smallest thing pdf_upload_error accepts.""" + import io + + return io.BytesIO(b'%PDF-1.4\n%%EOF\n')