Compare commits

...

3 Commits

Author SHA1 Message Date
85204938df Merge pull request 'CI: caddy validate van beide Caddyfiles in de test-job' (#28) from fix/issue-26-ci-caddy-validate into main
Some checks failed
On Push to Main / publish (push) Has been cancelled
On Push to Main / deploy-dev (push) Has been cancelled
On Push to Main / test (push) Has been cancelled
Reviewed-on: #28
2026-07-12 19:04:29 +00:00
RaymondVerhoef
6bf8bad5fb ci: validate both Caddyfiles in the test job (#26)
Some checks failed
On Pull Request to Main / test (pull_request) Failing after 26s
On Pull Request to Main / publish (pull_request) Has been skipped
On Pull Request to Main / deploy-dev (pull_request) Has been skipped
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 <noreply@anthropic.com>
2026-07-12 20:55:35 +02:00
RaymondVerhoef
54137122a7 fix: read Entra claims from id_token + UPN e-mail fallback (#24)
All checks were successful
On Pull Request to Main / test (pull_request) Successful in 38s
On Pull Request to Main / publish (pull_request) Successful in 1m10s
On Pull Request to Main / deploy-dev (pull_request) Successful in 3m7s
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 <noreply@anthropic.com>
2026-07-12 19:26:25 +02:00
5 changed files with 49 additions and 2 deletions

View File

@@ -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 .

View File

@@ -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

View File

@@ -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");

View File

@@ -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,
};

View File

@@ -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,
},