From 54137122a720f4a11ece6e3f7a6957b0c764286a Mon Sep 17 00:00:00 2001 From: RaymondVerhoef Date: Sun, 12 Jul 2026 19:26:25 +0200 Subject: [PATCH 1/2] fix: read Entra claims from id_token + UPN e-mail fallback (#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First Microsoft logins failed with 400 "email: Cannot be blank": Graph's /oidc/userinfo endpoint omits the email claim for accounts without a mail attribute, and team_members requires an e-mail (it also drives the ENTRA_ADMIN_EMAILS role mapping). Reproduced against a mock Entra with PocketBase v0.30.4; the user-supplied response body matched the missing-email fingerprint exactly. - provider config (reconciler + migration fast-path): userInfoURL is now empty, so PocketBase reads the claims from the Entra id_token, which always carries preferred_username (UPN) and email when available. The #18 reconciler flips the already-deployed Labs provider automatically on the next boot — no migration needed. - pb_hooks/entra_oidc.pb.js: onRecordAuthWithOAuth2Request hook falls back to the lowercased UPN when the email claim is absent. Guest UPNs (ext_user#EXT#@tenant...) are excluded explicitly — "#" is RFC-valid in an e-mail local part, so both a naive regex AND PocketBase's own validation would accept them (caught by test V5). The hook also logs every failed OAuth2 attempt with the underlying error to the container log, which PocketBase otherwise only writes to its internal logs db. Verified with a switchable mock-Entra matrix (8/8): no-email→UPN e-mail + allow-list role, email claim wins when present, no duplicate on re-login, guest UPN yields a clean validation error, anonymous REST create stays rejected, both log lines present. Regression: issue-22 matrix 9/9 (baseline pinned to 1a1351d now that #23 is merged), DoD harness 15/15, npm test 112/112. Closes #24 Co-Authored-By: Claude Fable 5 --- docs/auth-spec.md | 1 + pb_hooks/entra_oidc.pb.js | 28 +++++++++++++++++++ pb_hooks/utils.js | 7 ++++- .../1781000000_team_members_to_auth.js | 3 +- 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/docs/auth-spec.md b/docs/auth-spec.md index d152845..02acfa5 100644 --- a/docs/auth-spec.md +++ b/docs/auth-spec.md @@ -45,6 +45,7 @@ Browser (SPA) PocketBase (auth) Entra ID | 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 | | 009 | `createRule: '@request.context = "oauth2"'` op `team_members` | PocketBase past de createRule toe op de OAuth2-sign-up (eerste login maakt het record aan); `null` blokkeerde élke eerste login met 403 (issue #22). De context-rule staat alléén de OAuth2-flow toe — anonieme REST-creates blijven geweigerd | +| 010 | Claims uit het Entra **id_token** (`userInfoURL: ""`) + UPN-fallback voor e-mail | Graph `/oidc/userinfo` geeft géén `email`-claim voor accounts zonder mail-attribuut → 400 bij eerste login (issue #24). Het id_token bevat altijd `preferred_username` (UPN = primair e-mailadres bij Respellion); `entra_oidc.pb.js` gebruikt die als fallback (regex-guard tegen guest-UPN's) en logt gefaalde OAuth2-pogingen naar de containerlog | ## Bestanden diff --git a/pb_hooks/entra_oidc.pb.js b/pb_hooks/entra_oidc.pb.js index 1747a08..fe88df1 100644 --- a/pb_hooks/entra_oidc.pb.js +++ b/pb_hooks/entra_oidc.pb.js @@ -32,3 +32,31 @@ cronAdd("entraOidcReconcile", "* * * * *", () => { console.log("entra_oidc reconcile (cron): OIDC provider (re)configured from ENTRA_* env"); } }); + +// Entra does not always supply an `email` claim: the id_token only carries it +// when the account has a mail attribute (issue #24). The UPN in +// `preferred_username` is always present and equals the primary e-mail for +// Respellion organisation accounts, so fall back to it when it looks like a +// real address. Guest UPNs ("user_ext#EXT#@tenant...") fail the regex on +// purpose — those still get a clear validation error instead of a bogus email. +// The catch also surfaces the underlying OAuth2 failure in the container log, +// which PocketBase otherwise only writes to its internal logs database. +onRecordAuthWithOAuth2Request((e) => { + const u = e.oAuth2User; + if (u && !u.email) { + const upn = String((u.rawUser && u.rawUser.preferred_username) || u.username || "").trim().toLowerCase(); + // `#` is RFC-valid in an e-mail local part, so guest UPNs + // ("ext_user#EXT#@tenant.onmicrosoft.com") pass a naive e-mail check AND + // PocketBase's own validation — exclude them explicitly. + if (/^[^@\s#]+@[^@\s#]+\.[^@\s#]+$/.test(upn)) { + u.email = upn; + console.log("entra_oidc: email claim absent — falling back to UPN " + upn); + } + } + try { + e.next(); + } catch (err) { + console.log("entra_oidc auth-with-oauth2 FAILED:", String(err)); + throw err; + } +}, "team_members"); diff --git a/pb_hooks/utils.js b/pb_hooks/utils.js index 6b0ff31..37f5f03 100644 --- a/pb_hooks/utils.js +++ b/pb_hooks/utils.js @@ -76,7 +76,12 @@ module.exports = { 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", + // Deliberately empty: claims are read from the Entra id_token instead of + // Graph's /oidc/userinfo. The userinfo endpoint omits `email` for + // accounts without a mail attribute, while the id_token always carries + // `preferred_username` (UPN) which entra_oidc.pb.js uses as an e-mail + // fallback (issue #24). + userInfoURL: "", displayName: "Microsoft", pkce: true, }; diff --git a/pb_migrations/1781000000_team_members_to_auth.js b/pb_migrations/1781000000_team_members_to_auth.js index 4d02978..f725a61 100644 --- a/pb_migrations/1781000000_team_members_to_auth.js +++ b/pb_migrations/1781000000_team_members_to_auth.js @@ -117,7 +117,8 @@ migrate((app) => { 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", + // Empty on purpose: claims come from the id_token (see utils.js, issue #24). + userInfoURL: "", displayName: "Microsoft", pkce: true, }, -- 2.49.1 From 6bf8bad5fb79a0982810e671789db4873d7287f1 Mon Sep 17 00:00:00 2001 From: RaymondVerhoef Date: Sun, 12 Jul 2026 20:55:35 +0200 Subject: [PATCH 2/2] ci: validate both Caddyfiles in the test job (#26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test image swaps in Caddyfile.test, so the production Caddyfile was never exercised by CI: the parse error from issue #20 sailed through every check and only surfaced when the deploy gate failed — after the broken container had already replaced the healthy one. Running `caddy validate` (same caddy:2-alpine base as the Dockerfile) on both files up front fails the PR check in seconds instead. Touches the frozen .github/workflows/ per explicit product-owner approval in #26. Closes #26 Co-Authored-By: Claude Fable 5 --- .github/workflows/test.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c41f013..5fb2303 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,6 +11,18 @@ jobs: - name: Checkout uses: actions/checkout@v4 + # The test image below swaps in Caddyfile.test, so the production + # Caddyfile is otherwise never exercised by CI — an invalid config then + # only surfaces when the deploy health-gate fails, after the broken + # container already replaced the healthy one (issue #20/#26). Validate + # both files up front so a parse error fails the PR check in seconds. + - name: Validate Caddyfiles + run: | + docker run --rm -v "$PWD":/workspace -w /workspace caddy:2-alpine \ + caddy validate --config Caddyfile --adapter caddyfile + docker run --rm -v "$PWD":/workspace -w /workspace caddy:2-alpine \ + caddy validate --config Caddyfile.test --adapter caddyfile + - name: Build Docker image run: docker build -t learning-platform . -- 2.49.1