From d79e69aad2025f30e22c4b6116f5e98593a75875 Mon Sep 17 00:00:00 2001 From: RaymondVerhoef Date: Sat, 11 Jul 2026 11:14:09 +0200 Subject: [PATCH] fix: heal migration-ledger mismatch and make Azure SSO deploy-proof (#18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PocketBase on Labs crash-looped since the SSO deploy (PR #17): mounting --migrationsDir for the first time replayed the entire migration history against a database that was provisioned out-of-band (empty _migrations ledger) and died on 1778948471_created_content.js. On top of that the team_members->auth migration had its own crash paths and trapped OAuth2 config inside a one-shot, env-dependent migration. - pb_migrations/1000000000_baseline_ledger_sync.js: detects an out-of-band provisioned DB (schema exists, ledger empty) and marks the 79 historical migrations as applied; no-op on fresh or already-synced DBs - pb_migrations/1781000000_team_members_to_auth.js: idempotency guard, relation->text conversion WITH data preservation (PocketBase diffs fields by id, so the column is backed up and restored via SQL), unique index rebuild, no silent catches, env only as fast-path - pb_hooks/entra_oidc.pb.js + pb_hooks/utils.js: reconcile the Entra OIDC provider from ENTRA_* env on every bootstrap + cron tick (compare-before-save, warn-once); heals environments that migrated without secrets and supports secret rotation without re-apply - pb_hooks/team_members.pb.js: require() pattern — JSVM runs callbacks as isolated programs, top-level helpers are not in scope (adopts Leroy's fix from the fix-sso branch) - infra/*/site/deploy-playbook.yml: health-gate after compose up — the deploy fails loudly with container logs when PocketBase does not become healthy (runs #83-#88 were green while PB crash-looped) - docker-compose.yml: .env.local is optional again - docs/auth-spec.md + AI_AGENT.md: ledger/reconciler documentation, ADRs 006-008, never-rename-applied-migrations warning Verified locally against PocketBase v0.30.4 with a 7-scenario DoD matrix (fresh DB +/- env, out-of-band DB with data incl. data preservation, populated-ledger upgrade, late-secrets healing, re-run guard, hook provisioning): 15/15 pass. npm test 112/112. Closes #18 Co-Authored-By: Claude Fable 5 --- AI_AGENT.md | 3 + docker-compose.yml | 4 +- docs/auth-spec.md | 20 +- infra/development/site/deploy-playbook.yml | 28 +++ infra/production/site/deploy-playbook.yml | 28 +++ pb_hooks/entra_oidc.pb.js | 34 ++++ pb_hooks/team_members.pb.js | 16 +- pb_hooks/utils.js | 104 ++++++++++ .../1000000000_baseline_ledger_sync.js | 138 +++++++++++++ .../1781000000_team_members_to_auth.js | 187 +++++++++++------- 10 files changed, 478 insertions(+), 84 deletions(-) create mode 100644 pb_hooks/entra_oidc.pb.js create mode 100644 pb_hooks/utils.js create mode 100644 pb_migrations/1000000000_baseline_ledger_sync.js diff --git a/AI_AGENT.md b/AI_AGENT.md index a4c2060..76c069c 100644 --- a/AI_AGENT.md +++ b/AI_AGENT.md @@ -137,6 +137,9 @@ The platform ships a global chatbot avatar called **R42**, rendered as the Respe * **PocketBase auto-cancellation is OFF.** Set in `src/lib/pb.js`; never re-enable. * **Go through `callLLM`.** Never call the Anthropic proxy directly; you lose retry, schema validation, and telemetry. * **AI token budget.** Truncation surfaces as `LLMTruncatedError` (`stop_reason: max_tokens`). For extraction, tighten the prompt's topic cap before raising `max_tokens`. +* **PocketBase migrations & the `_migrations` ledger (issue #18).** The deployed Labs/prod databases were originally provisioned WITHOUT `--migrationsDir` (schema came from `scripts/setup-pb-collections.mjs`), so their ledger started empty. `pb_migrations/1000000000_baseline_ledger_sync.js` marks the historical files as applied on such databases — never remove it, never rename an applied migration file (the ledger is filename-based), and give new migrations a timestamp that sorts after the existing ones. A failed migration aborts `pocketbase serve` → container crash-loop → 502 on every `/api/*` call; the deploy playbooks gate on `/api/health` to catch this. +* **OIDC provider config lives in a hook, not a migration.** `pb_hooks/entra_oidc.pb.js` reconciles the Entra provider from `ENTRA_*` env on every bootstrap + cron tick. Don't move provider credentials back into a one-shot migration — an apply while the env is empty would permanently disable OAuth2. +* **JSVM hook scope.** PocketBase runs each hook callback as an isolated program: top-level functions in a `*.pb.js` file are NOT in scope inside callbacks. Shared helpers live in `pb_hooks/utils.js` and are pulled in per-callback via `require(`${__hooks}/utils.js`)`. ## 13. 26-Week Per-User Curriculum System The platform uses a **26-week perpetual curriculum cycle**. Every employee covers the knowledge base in focused, thematic weekly blocks, **starting whenever they enroll**. diff --git a/docker-compose.yml b/docker-compose.yml index dd1eef5..c820ebf 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -13,7 +13,9 @@ services: restart: unless-stopped working_dir: /pb env_file: - - .env.local + # Optional: absent .env.local must not block `docker compose up` (issue #18). + - path: .env.local + required: false command: ["serve", "--http=0.0.0.0:8090", "--dir=/pb/pb_data", "--migrationsDir=/pb/pb_migrations"] ports: - "8090:8090" diff --git a/docs/auth-spec.md b/docs/auth-spec.md index c81fbe3..7785b41 100644 --- a/docs/auth-spec.md +++ b/docs/auth-spec.md @@ -1,6 +1,7 @@ # Authentication — Azure (Entra ID) login > Implemented for issue #16. Replaces the previous client-side PIN login. +> Hardened for issue #18 (migratie-ledger-sync + env-onafhankelijke provider-reconciliatie). ## Doel @@ -40,13 +41,19 @@ Browser (SPA) PocketBase (auth) Entra ID | 003 | Provisioning + admin-rol via `pb_hooks/team_members.pb.js` | Rol-logica server-side, niet in de client | | 004 | Admin-rol via e-mail-allowlist (`ENTRA_ADMIN_EMAILS`), re-synced elke login | Geen handmatig beheer; allowlist-wijziging werkt bij volgende login | | 005 | API-rules → `@request.auth.id != ""` op alle app-collecties | Geen anonieme toegang meer; admins/users zijn beide geauthenticeerd | +| 006 | Baseline-ledger-sync migratie voor out-of-band geprovisionede DB's | Labs/prod draaide historisch zonder `--migrationsDir`; replay van de historie crashte PB (issue #18). De vroegst-sorterende migratie markeert de historie als applied wanneer schema bestaat maar de ledger leeg is | +| 007 | Migratie = structuur, hook = configuratie | De OIDC-provider wordt bij elke start (en per cron-minuut) gereconcilieerd vanuit `ENTRA_*` env door `pb_hooks/entra_oidc.pb.js`; een one-shot migratie die van deploy-time env afhangt kan OAuth2 anders permanent uitschakelen | +| 008 | Hook-helpers via `require(`${__hooks}/utils.js`)` | De JSVM draait elke hook-callback als geïsoleerd programma; top-level functies zijn níet in scope op call-time | ## Bestanden | Bestand | Rol | |---|---| -| `pb_migrations/1781000000_team_members_to_auth.js` | Dropt de oude PIN-collection, maakt `team_members` als auth-collection met de OIDC-provider (creds uit env). | +| `pb_migrations/1000000000_baseline_ledger_sync.js` | Detecteert een out-of-band geprovisionede DB (schema bestaat, `_migrations`-ledger leeg) en markeert de historische migraties als applied, zodat de replay niet crasht (issue #18). | +| `pb_migrations/1781000000_team_members_to_auth.js` | Converteert relation-velden naar text (data blijft behouden), dropt de oude PIN-collection en maakt `team_members` als auth-collection. Idempotent; provider wordt alleen als fast-path meegenomen als de env al aanwezig is. | | `pb_migrations/1781000001_tighten_api_rules.js` | Zet alle niet-systeem-collecties op `@request.auth.id != ""`. | +| `pb_hooks/entra_oidc.pb.js` | Reconcilieert de OIDC-provider vanuit `ENTRA_*` env bij elke start + cron-tick (compare-before-save). | +| `pb_hooks/utils.js` | Gedeelde helpers (`resolveRole`, `reconcileEntraOidc`) — per callback binnenhalen via `require()`. | | `pb_hooks/team_members.pb.js` | Auto-provisioning (`role`, `enrollment_status`, naam-fallback) + admin-rol re-sync. | | `src/lib/azureAuth.js` | Canonieke, unit-geteste helpers (`OIDC_PROVIDER`, `resolveRole`, `parseAdminEmails`, `deriveName`). | | `src/store/AppContext.jsx` | `loginWithAzure()` / `logout()` op basis van `pb.authStore` + `authRefresh()`. | @@ -100,6 +107,11 @@ Ansible-variabelen (vault): `entra_tenant_id`, `entra_client_id`, eerste deploy. - Verweesde voortgangsrecords van verwijderde (pre-Azure) users zijn acceptabel: zonder geldig Entra-account kan niemand inloggen, dus die records zijn onbereikbaar. -- Credential-rotatie: update env + herconfigureer de provider via de PocketBase - admin-UI (Settings → Auth providers) of ship een follow-up migratie — de - create-migratie draait maar één keer. +- Credential-rotatie: update de `ENTRA_*` secrets (Gitea → Ansible `.env`) en + herstart/redeploy — `pb_hooks/entra_oidc.pb.js` reconcilieert de provider + automatisch bij elke start en elke cron-minuut. Geen handmatige re-apply meer. +- **Migratiebestanden nooit hernoemen nadat ze ergens applied zijn** — de + `_migrations`-ledger is filename-based; hernoemen triggert een re-apply. +- De deploy-playbooks bevatten een health-gate: als PocketBase na de deploy niet + healthy wordt, faalt de pipeline zichtbaar en print hij de containerlogs + (issue #18 bleef 2 weken onzichtbaar doordat de deploy "groen" was). diff --git a/infra/development/site/deploy-playbook.yml b/infra/development/site/deploy-playbook.yml index 8d9d8b9..442b8b4 100644 --- a/infra/development/site/deploy-playbook.yml +++ b/infra/development/site/deploy-playbook.yml @@ -72,3 +72,31 @@ state: present files: compose.yml recreate: always + + # A failed migration aborts `pocketbase serve` and the container crash-loops + # while `docker compose up` still reports success (issue #18). Gate the + # deploy on PocketBase actually serving its health endpoint. + - name: Wait for PocketBase to become healthy + ansible.builtin.command: docker exec pocketbase-learning wget -q -O - http://127.0.0.1:8090/api/health + register: pb_health + until: pb_health.rc == 0 + retries: 18 + delay: 5 + changed_when: false + ignore_errors: yes + + - name: Collect PocketBase logs for diagnosis + ansible.builtin.command: docker logs --tail 80 pocketbase-learning + register: pb_logs + when: pb_health is failed + changed_when: false + failed_when: false + + - name: Abort deploy — PocketBase is not healthy + ansible.builtin.fail: + msg: | + PocketBase did not become healthy after the deploy (health endpoint unreachable). + Recent container logs: + {{ pb_logs.stdout | default('') }} + {{ pb_logs.stderr | default('') }} + when: pb_health is failed diff --git a/infra/production/site/deploy-playbook.yml b/infra/production/site/deploy-playbook.yml index 8d9d8b9..442b8b4 100644 --- a/infra/production/site/deploy-playbook.yml +++ b/infra/production/site/deploy-playbook.yml @@ -72,3 +72,31 @@ state: present files: compose.yml recreate: always + + # A failed migration aborts `pocketbase serve` and the container crash-loops + # while `docker compose up` still reports success (issue #18). Gate the + # deploy on PocketBase actually serving its health endpoint. + - name: Wait for PocketBase to become healthy + ansible.builtin.command: docker exec pocketbase-learning wget -q -O - http://127.0.0.1:8090/api/health + register: pb_health + until: pb_health.rc == 0 + retries: 18 + delay: 5 + changed_when: false + ignore_errors: yes + + - name: Collect PocketBase logs for diagnosis + ansible.builtin.command: docker logs --tail 80 pocketbase-learning + register: pb_logs + when: pb_health is failed + changed_when: false + failed_when: false + + - name: Abort deploy — PocketBase is not healthy + ansible.builtin.fail: + msg: | + PocketBase did not become healthy after the deploy (health endpoint unreachable). + Recent container logs: + {{ pb_logs.stdout | default('') }} + {{ pb_logs.stderr | default('') }} + when: pb_health is failed diff --git a/pb_hooks/entra_oidc.pb.js b/pb_hooks/entra_oidc.pb.js new file mode 100644 index 0000000..1747a08 --- /dev/null +++ b/pb_hooks/entra_oidc.pb.js @@ -0,0 +1,34 @@ +/// +// +// Issue #18 — Reconcile the Entra (Azure) OIDC provider from the environment. +// +// The team_members→auth migration (pb_migrations/1781000000) is structure-only +// and runs exactly once. Provider CONFIGURATION lives here instead, so that: +// +// * an environment whose migration applied while the ENTRA_* secrets were +// absent gets its provider enabled on the next startup (no re-apply), +// * rotating ENTRA_CLIENT_SECRET / changing the tenant only requires new env +// values and a container restart, +// * a fresh database gets its provider on the first cron tick right after +// the migrations created the collection (onBootstrap fires BEFORE the +// migrations run — there is no post-migration lifecycle hook in the JSVM, +// hence the cron fallback). +// +// The reconciler compares before saving, so both hooks are cheap no-ops when +// everything is already in sync. + +onBootstrap((e) => { + e.next(); + const { reconcileEntraOidc } = require(`${__hooks}/utils.js`); + // reconcileEntraOidc logs a warn-once itself when the ENTRA_* env is absent. + console.log("entra_oidc reconcile (bootstrap): " + reconcileEntraOidc(e.app)); +}); + +cronAdd("entraOidcReconcile", "* * * * *", () => { + const { reconcileEntraOidc } = require(`${__hooks}/utils.js`); + const result = reconcileEntraOidc($app); + // Only log state changes — this tick runs every minute. + if (result === "updated") { + console.log("entra_oidc reconcile (cron): OIDC provider (re)configured from ENTRA_* env"); + } +}); diff --git a/pb_hooks/team_members.pb.js b/pb_hooks/team_members.pb.js index 4551ddf..4147409 100644 --- a/pb_hooks/team_members.pb.js +++ b/pb_hooks/team_members.pb.js @@ -15,17 +15,16 @@ // The allow-list is a comma-separated list of e-mail addresses, e.g.: // ENTRA_ADMIN_EMAILS=rve@respellion.nl,admin@respellion.nl // -// Keep this logic in sync with src/lib/azureAuth.js (resolveRole / parseAdminEmails), -// which carries the canonical, unit-tested version for the frontend/tests. - -function resolveRole(email) { - const raw = ($os.getenv("ENTRA_ADMIN_EMAILS") || "").toLowerCase(); - const allow = raw.split(",").map(function (s) { return s.trim(); }).filter(Boolean); - return allow.indexOf((email || "").toLowerCase()) !== -1 ? "admin" : "user"; -} +// resolveRole itself lives in pb_hooks/utils.js and is pulled in per-callback +// via require() — PocketBase's JSVM runs each hook callback below as its own +// isolated program, so a shared top-level function here would not be in scope +// at call time. See pb_hooks/utils.js for details. Keep the logic in sync with +// src/lib/azureAuth.js (resolveRole / parseAdminEmails), which carries the +// canonical, unit-tested version for the frontend/tests. // On creation (first login): set defaults before the record is persisted. onRecordCreate(function (e) { + const { resolveRole } = require(`${__hooks}/utils.js`); const email = e.record.get("email") || ""; e.record.set("role", resolveRole(email)); @@ -45,6 +44,7 @@ onRecordCreate(function (e) { // On every successful auth (incl. OIDC): keep the admin role in sync with the // allow-list, so promoting/demoting an admin only requires an env change. onRecordAuthRequest(function (e) { + const { resolveRole } = require(`${__hooks}/utils.js`); const email = e.record.get("email") || ""; const want = resolveRole(email); if (e.record.get("role") !== want) { diff --git a/pb_hooks/utils.js b/pb_hooks/utils.js new file mode 100644 index 0000000..6b0ff31 --- /dev/null +++ b/pb_hooks/utils.js @@ -0,0 +1,104 @@ +/// +// +// Shared helpers for pb_hooks/*.pb.js callbacks. +// +// PocketBase's JSVM serializes and runs each registered hook callback as its +// own isolated program, so a plain top-level function declared in a *.pb.js +// file is NOT in scope inside a callback at call time. Reusable helpers must +// instead live in a plain (non *.pb.js, so it isn't auto-loaded as a hook) +// module and be pulled in per-callback via require(`${__hooks}/utils.js`). +// See: https://github.com/pocketbase/pocketbase/discussions/3599 +// +// Loaded modules use a shared registry across callback invocations, so keep +// this file stateless. (Deliberate exception: the warn-once latch below, which +// exists precisely BECAUSE the registry is shared — it dedupes the env-missing +// warning across the bootstrap hook and the per-minute cron tick.) +// +// Keep resolveRole in sync with src/lib/azureAuth.js's resolveRole (the +// canonical, unit-tested version used by the frontend). + +let warnedEnvMissing = false; + +module.exports = { + /** + * Resolve a user's role from their e-mail and the ENTRA_ADMIN_EMAILS allow-list. + * @param {string} email + * @returns {"admin"|"user"} + */ + resolveRole: function (email) { + const raw = ($os.getenv("ENTRA_ADMIN_EMAILS") || "").toLowerCase(); + const allow = raw.split(",").map(function (s) { return s.trim(); }).filter(Boolean); + return allow.indexOf(String(email || "").toLowerCase()) !== -1 ? "admin" : "user"; + }, + + /** + * Reconcile the Entra (Azure) OIDC provider on the team_members auth + * collection from the ENTRA_* environment variables (issue #18). + * + * Idempotent: compares the desired provider config against the stored one + * and only saves when something actually changed, so it is safe to call on + * every bootstrap and cron tick. Keeping this OUT of the one-shot migration + * means late or rotated secrets are picked up on the next start/tick without + * any manual re-apply. + * + * @param {core.App} app + * @returns {"collection-missing"|"not-auth-yet"|"env-missing"|"in-sync"|"updated"} + */ + reconcileEntraOidc: function (app) { + let col; + try { + col = app.findCollectionByNameOrId("team_members"); + } catch (_) { + return "collection-missing"; // migration has not created it yet + } + if (col.type !== "auth") { + return "not-auth-yet"; // pre-conversion PIN-era collection + } + + const tenant = $os.getenv("ENTRA_TENANT_ID") || "common"; + const clientId = $os.getenv("ENTRA_CLIENT_ID") || ""; + const clientSecret = $os.getenv("ENTRA_CLIENT_SECRET") || ""; + if (!clientId || !clientSecret) { + if (!warnedEnvMissing) { + warnedEnvMissing = true; + console.log( + "WARN entra_oidc: ENTRA_CLIENT_ID / ENTRA_CLIENT_SECRET not set — Microsoft login stays " + + "disabled until the env vars are provided (they are reconciled automatically on startup)." + ); + } + return "env-missing"; + } + warnedEnvMissing = false; // env restored — re-arm the warning for future rotations + + const desired = { + name: "oidc", + clientId: clientId, + clientSecret: clientSecret, + authURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/authorize", + tokenURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/token", + userInfoURL: "https://graph.microsoft.com/oidc/userinfo", + displayName: "Microsoft", + pkce: true, + }; + + const providers = (col.oauth2 && col.oauth2.providers) || []; + const current = providers.length === 1 ? providers[0] : null; + const inSync = !!current && + col.oauth2.enabled === true && + current.name === desired.name && + current.clientId === desired.clientId && + current.clientSecret === desired.clientSecret && + current.authURL === desired.authURL && + current.tokenURL === desired.tokenURL && + current.userInfoURL === desired.userInfoURL && + current.displayName === desired.displayName; + if (inSync) { + return "in-sync"; + } + + col.oauth2.enabled = true; + col.oauth2.providers = [desired]; + app.save(col); + return "updated"; + }, +}; diff --git a/pb_migrations/1000000000_baseline_ledger_sync.js b/pb_migrations/1000000000_baseline_ledger_sync.js new file mode 100644 index 0000000..f31bba6 --- /dev/null +++ b/pb_migrations/1000000000_baseline_ledger_sync.js @@ -0,0 +1,138 @@ +/// +// +// Issue #18 — Baseline ledger sync for out-of-band provisioned databases. +// +// The Labs/production PocketBase originally ran WITHOUT --migrationsDir: its +// schema was created by scripts/setup-pb-collections.mjs / the admin UI, so its +// _migrations ledger is empty. When the migrations dir was mounted for the +// first time (Azure SSO, PR #17), PocketBase tried to replay the entire +// migration history against that existing schema and crash-looped on the very +// first file ("Collection name must be unique"). +// +// This migration sorts before every other file. When it detects that the +// legacy schema already exists but the ledger has never tracked it, it marks +// the historical migrations below as applied so the replay is skipped and the +// chain continues with the genuinely new migrations (team_members_to_auth, +// tighten_api_rules). On fresh databases and on databases that already have a +// populated ledger it is a no-op. +// +// NOTE for future agents: never rename a migration file after it has been +// applied anywhere — the ledger is filename-based. New migrations must sort +// after 1781100001 and be appended to environments through a normal deploy. +const HISTORICAL_MIGRATIONS = [ + "1778948471_created_content.js", + "1778948471_created_quiz_banks.js", + "1778948471_created_relations.js", + "1778948471_created_sources.js", + "1778948471_created_team_members.js", + "1778948471_created_topics.js", + "1778948472_created_leaderboard.js", + "1778948472_created_learn_progress.js", + "1778948472_created_quiz_cache.js", + "1778948472_created_quiz_results.js", + "1778948472_created_settings.js", + "1778954289_created_test_col2.js", + "1778954310_deleted_relations.js", + "1778954310_deleted_topics.js", + "1778954310_deleted_users.js", + "1778954311_deleted_content.js", + "1778954311_deleted_leaderboard.js", + "1778954311_deleted_learn_progress.js", + "1778954311_deleted_quiz_banks.js", + "1778954311_deleted_quiz_cache.js", + "1778954311_deleted_quiz_results.js", + "1778954311_deleted_settings.js", + "1778954311_deleted_sources.js", + "1778954311_deleted_team_members.js", + "1778954311_deleted_test_col2.js", + "1778954317_created_content.js", + "1778954317_created_leaderboard.js", + "1778954317_created_learn_progress.js", + "1778954317_created_quiz_banks.js", + "1778954317_created_quiz_cache.js", + "1778954317_created_quiz_results.js", + "1778954317_created_relations.js", + "1778954317_created_settings.js", + "1778954317_created_sources.js", + "1778954317_created_team_members.js", + "1778954317_created_topics.js", + "1779005271_created_test_col3.js", + "1779005289_created_test_col4.js", + "1779005309_deleted_content.js", + "1779005309_deleted_learn_progress.js", + "1779005309_deleted_quiz_banks.js", + "1779005309_deleted_quiz_cache.js", + "1779005309_deleted_quiz_results.js", + "1779005309_deleted_relations.js", + "1779005309_deleted_sources.js", + "1779005309_deleted_team_members.js", + "1779005309_deleted_topics.js", + "1779005310_deleted_leaderboard.js", + "1779005310_deleted_settings.js", + "1779005310_deleted_test_col3.js", + "1779005310_deleted_test_col4.js", + "1779005316_created_content.js", + "1779005316_created_quiz_banks.js", + "1779005316_created_relations.js", + "1779005316_created_sources.js", + "1779005316_created_topics.js", + "1779005317_created_leaderboard.js", + "1779005317_created_learn_progress.js", + "1779005317_created_quiz_cache.js", + "1779005317_created_quiz_results.js", + "1779005317_created_settings.js", + "1779005317_created_team_members.js", + "1779127586_created_curriculum.js", + "1779127759_collections_snapshot.js", + "1779200000_updated_sources.js", + "1780000001_updated_topics.js", + "1780500000_updated_topics_relevance_locked.js", + "1780500001_normalize_relation_types.js", + "1780500002_created_llm_calls.js", + "1780600000_curriculum_v2.js", + "1780700000_sources_progress.js", + "1780800000_created_micro_learnings.js", + "1780800001_deleted_legacy_collections.js", + "1780800002_update_micro_learnings_rules.js", + "1780900000_team_members_enrollment.js", + "1780900001_created_test_results.js", + "1781000000_created_theme_sessions.js", + "1781100000_created_question_bank.js", + "1781100001_created_on_demand_attempts.js", +]; + +migrate((app) => { + // Fresh database? (no legacy schema) -> let the normal chain run. + try { + app.findCollectionByNameOrId("content"); + } catch (_) { + return; + } + + // Ledger already tracks the history? -> normally migrated DB, nothing to do. + const probe = new DynamicModel({ count: 0 }); + app.db() + .newQuery("SELECT COUNT(*) AS count FROM _migrations WHERE file = {:file}") + .bind({ file: "1778948471_created_content.js" }) + .one(probe); + if (probe.count > 0) { + return; + } + + // Out-of-band provisioned database: schema exists, ledger is empty. + // Mark the historical migrations as applied so they are not replayed. + const applied = Date.now(); + for (const file of HISTORICAL_MIGRATIONS) { + app.db() + .newQuery("INSERT OR IGNORE INTO _migrations (file, applied) VALUES ({:file}, {:applied})") + .bind({ file: file, applied: applied }) + .execute(); + } + console.log( + "baseline_ledger_sync: detected out-of-band provisioned database — marked " + + HISTORICAL_MIGRATIONS.length + " historical migrations as applied" + ); +}, (_app) => { + // Down: intentionally a no-op. The ledger rows describe environment-specific + // bookkeeping; removing them would re-trigger the replay this fix prevents. +}); diff --git a/pb_migrations/1781000000_team_members_to_auth.js b/pb_migrations/1781000000_team_members_to_auth.js index 03badcb..5d98ac9 100644 --- a/pb_migrations/1781000000_team_members_to_auth.js +++ b/pb_migrations/1781000000_team_members_to_auth.js @@ -1,6 +1,6 @@ /// // -// Issue #16 — Azure (Entra ID) login. +// Issue #16 / #18 — Azure (Entra ID) login. // // Replaces the PIN-based `base` collection `team_members` with a PocketBase // `auth` collection that authenticates against Azure Entra ID via OIDC. @@ -10,53 +10,127 @@ // base, generated tests and micro-learnings live in OTHER collections and are // left completely untouched by this migration. // -// OIDC provider credentials are read from the environment at apply time, so no -// secrets are committed to git: -// ENTRA_TENANT_ID, ENTRA_CLIENT_ID, ENTRA_CLIENT_SECRET -// (set via the Ansible-rendered .env, see infra/*/site/compose.yml). -// -// To rotate credentials or change the tenant later, update the env and either -// re-configure the provider from the PocketBase admin UI (Settings → Auth -// providers) or ship a follow-up migration. +// This migration is STRUCTURE ONLY and does not depend on the environment: +// the OIDC provider credentials (ENTRA_TENANT_ID / ENTRA_CLIENT_ID / +// ENTRA_CLIENT_SECRET) are reconciled into the collection on every startup by +// pb_hooks/entra_oidc.pb.js. That keeps the one-shot migration deterministic: +// applying it while the secrets are absent can no longer permanently disable +// OAuth2 (issue #18). As a fast path the provider is also attached here when +// the env vars are already present at apply time. migrate((app) => { - // 1. Drop the old PIN-based collection (takes the PIN records with it). - // Other collections (micro_learning_completions, theme_session_completions) - // have relation fields pointing to the old team_members — PocketBase refuses - // to delete a referenced collection. Remove those relation fields first, - // delete the old collection, create the new auth collection, then re-add - // the relation fields pointing to the new collection. - const relDeps = [ - { collection: "micro_learning_completions", fieldId: "rel_team_member_id", fieldName: "team_member_id" }, - { collection: "theme_session_completions", fieldId: "rel_tsc_team_member", fieldName: "team_member_id" }, - ]; - - // Remove blocking relation fields so the old collection can be deleted. - for (const dep of relDeps) { - try { - const col = app.findCollectionByNameOrId(dep.collection); - col.fields.removeById(dep.fieldId); - app.save(col); - } catch (_) { /* collection doesn't exist yet — skip */ } - } - + // Idempotency guard: if team_members is already an auth collection (e.g. the + // conversion happened through an earlier partial rollout), leave it alone. + let existing = null; try { - const old = app.findCollectionByNameOrId("team_members"); - app.delete(old); + existing = app.findCollectionByNameOrId("team_members"); } catch (_) { - // Collection did not exist — nothing to drop. + existing = null; // collection absent — nothing to convert, only to create + } + if (existing && existing.type === "auth") { + console.log("team_members_to_auth: team_members is already an auth collection — skipping conversion."); + return; } - const tenant = $os.getenv("ENTRA_TENANT_ID") || "common"; - const clientId = $os.getenv("ENTRA_CLIENT_ID"); - const clientSecret = $os.getenv("ENTRA_CLIENT_SECRET"); + // 1. Detach relation fields that point at team_members. PocketBase refuses + // to delete a collection that other collections relate to, so before we + // can drop the old PIN-based team_members we convert those relations into + // plain text fields (the id is stored as text — consistent with how + // user_id is stored in test_results / on_demand_attempts). The column + // values are preserved by the conversion. + const deps = [ + { name: "micro_learning_completions", relId: "rel_team_member_id", textId: "txt_mlc_team_member" }, + { name: "theme_session_completions", relId: "rel_tsc_team_member", textId: "txt_tsc_team_member" }, + ]; + for (const dep of deps) { + let c = null; + try { + c = app.findCollectionByNameOrId(dep.name); + } catch (_) { + console.log("team_members_to_auth: " + dep.name + " does not exist yet — nothing to detach."); + continue; + } + if (!c.fields.getById(dep.relId)) { + console.log("team_members_to_auth: " + dep.name + "." + dep.relId + " already detached — skipping."); + continue; + } + // PocketBase diffs fields by ID, so swapping the relation field for a text + // field drops and recreates the underlying column. Back the values up and + // restore them after the schema save — the ids must survive (issue #18). + const backup = "_issue18_tm_" + dep.name; + app.db().newQuery("DROP TABLE IF EXISTS `" + backup + "`").execute(); + app.db().newQuery( + "CREATE TABLE `" + backup + "` AS SELECT `id`, `team_member_id` AS `v` FROM `" + dep.name + "`" + ).execute(); - const hasOidc = !!(clientId && clientSecret); - if (!hasOidc) { - console.log("WARN: ENTRA_CLIENT_ID / ENTRA_CLIENT_SECRET not set — OIDC provider will not be configured. " + - "Set the env vars and re-apply the migration to enable Azure login."); + // Drop any index that references the team_member_id column (it is rebuilt + // on the text column below). + c.indexes = (c.indexes || []).filter((idx) => idx.indexOf("team_member_id") === -1); + // Replace the relation field with a text field of the same name. + c.fields.removeById(dep.relId); + c.fields.add(new Field({ + type: "text", + id: dep.textId, + name: "team_member_id", + required: false, + min: 0, + max: 0, + })); + app.save(c); + + app.db().newQuery( + "UPDATE `" + dep.name + "` SET `team_member_id` = COALESCE(" + + "(SELECT `v` FROM `" + backup + "` WHERE `" + backup + "`.`id` = `" + dep.name + "`.`id`), '')" + ).execute(); + app.db().newQuery("DROP TABLE `" + backup + "`").execute(); } - // 2. Recreate `team_members` as an auth collection (same name → all existing + // Recreate the unique (team_member_id, session_week) guard on the text column. + try { + const tsc = app.findCollectionByNameOrId("theme_session_completions"); + const idx = "CREATE UNIQUE INDEX `idx_theme_session_completions_user_week` ON `theme_session_completions` (`team_member_id`, `session_week`)"; + if ((tsc.indexes || []).indexOf(idx) === -1) { + tsc.indexes.push(idx); + app.save(tsc); + } + } catch (_) { + console.log("team_members_to_auth: theme_session_completions does not exist yet — no index to rebuild."); + } + + // 2. Drop the old PIN-based team_members (now unreferenced). NOT wrapped in + // a try/catch: if this fails there is an unknown relation left and the + // migration must fail loudly instead of masking the error (issue #18). + if (existing) { + app.delete(existing); + } + + // Fast path: attach the OIDC provider right away when the credentials are + // already present at apply time. When they are absent the collection is + // created without a provider and pb_hooks/entra_oidc.pb.js configures it on + // the next startup / cron tick once the ENTRA_* env vars are supplied. + const tenant = $os.getenv("ENTRA_TENANT_ID") || "common"; + const clientId = $os.getenv("ENTRA_CLIENT_ID") || ""; + const clientSecret = $os.getenv("ENTRA_CLIENT_SECRET") || ""; + const oidcProviders = (clientId && clientSecret) ? [ + { + name: "oidc", + clientId: clientId, + clientSecret: clientSecret, + authURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/authorize", + tokenURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/token", + userInfoURL: "https://graph.microsoft.com/oidc/userinfo", + displayName: "Microsoft", + pkce: true, + }, + ] : []; + if (oidcProviders.length === 0) { + console.log( + "team_members_to_auth: ENTRA_CLIENT_ID / ENTRA_CLIENT_SECRET not set — creating the auth " + + "collection without an OIDC provider. pb_hooks/entra_oidc.pb.js will configure the provider " + + "automatically once the env vars are present (no re-apply needed)." + ); + } + + // 3. Recreate `team_members` as an auth collection (same name → all existing // `user_id` references in test_results, on_demand_attempts, // micro_learning_completions, leaderboard, theme_session_completions keep // working unchanged). @@ -83,7 +157,7 @@ migrate((app) => { mfa: { enabled: false, duration: 600, rule: "" }, oauth2: { - enabled: hasOidc, + enabled: oidcProviders.length > 0, // Map the OIDC userinfo claims onto our fields. mappedFields: { id: "", @@ -91,18 +165,7 @@ migrate((app) => { username: "", avatarURL: "", }, - providers: hasOidc ? [ - { - name: "oidc", - clientId: clientId, - clientSecret: clientSecret, - authURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/authorize", - tokenURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/token", - userInfoURL: "https://graph.microsoft.com/oidc/userinfo", - displayName: "Microsoft", - pkce: true, - }, - ] : [], + providers: oidcProviders, }, fields: [ @@ -258,24 +321,6 @@ migrate((app) => { }); app.save(collection); - - // 3. Re-add the relation fields pointing to the new auth collection. - for (const dep of relDeps) { - try { - const col = app.findCollectionByNameOrId(dep.collection); - const newField = new Field({ - id: dep.fieldId, - name: dep.fieldName, - type: "relation", - required: true, - collectionId: collection.id, - cascadeDelete: true, - maxSelect: 1, - }); - col.fields.add(newField); - app.save(col); - } catch (_) { /* collection doesn't exist — skip */ } - } }, (app) => { // Down: drop the auth collection and recreate the original PIN-based base // collection (without data — the original records are gone).