From e5e5b75b8509003fddfd155ef2012ed067fb315d Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Tue, 11 Aug 2026 23:05:25 -0500 Subject: [PATCH] Customer Entra tenants: verify the domain, then be believed The previous fix let the instance-wide Microsoft button work and left the customer-facing path broken, which is the worst way round. An organization that brings its own Entra tenant would publish the TXT record, watch its domain go green, and still be refused at login with `email_unverified` -- because Entra sends no such claim and rowToProvider pinned the assumption off for every org provider. Requiring a claim Microsoft does not emit is not a security control, it is an outage. What makes it safe to stop requiring it is the proof that already gates these providers: the callback confines an org provider to its DNS-verified domains, and an address only reaches the check after passing that. Whoever controls a domain's DNS controls its mail, which is the same trust that makes a verification link meaningful. So the assumption is DERIVED from proof -- `verified.length > 0` -- rather than pinned off. A provider that has verified nothing still assumes nothing, which is belt and braces: emailAllowedForProvider already refuses it, since an empty allow-list matches no domain, but deriving it here means a future reordering of those checks cannot silently widen it. It is never a column, and there is no column for it to be read from. An organization must not be able to switch this on for itself; it is a consequence of DNS proof, not a setting. A test asserts both -- that the value is derived next to `source: 'org'`, and that no `assume_email_verified` exists in the schema. Domain confinement is untouched. An explicit `email_verified: false` is still refused from anyone. Mutations all fail the tests: assuming unconditionally, never assuming, and reading it from the row. --- README.md | 7 +++ server/lib/oidc-providers.js | 42 ++++++++++++---- .../test/oidc-email-verified-policy.test.js | 48 ++++++++++++++----- 3 files changed, 75 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 526e261..8fbbe8f 100644 --- a/README.md +++ b/README.md @@ -425,6 +425,13 @@ https://yourdomain.com/api/auth/oidc//callback The slug is generated rather than chosen so two customers cannot collide on — or guess — each other's. A domain may be claimed by only one organization; a second claim is refused. +A customer bringing **Microsoft/Entra** registers a single-tenant application in their own directory +and uses `https://login.microsoftonline.com//v2.0` as the issuer. Because Entra +does not send `email_verified`, an organization's provider is trusted to assert addresses **once it +has verified a domain** — the DNS proof is what stands in for the claim, and the provider is confined +to those domains regardless. A provider that has verified nothing assumes nothing, and an explicit +`email_verified: false` is refused whoever sends it. + ⚠️ **A provider may only authenticate emails inside the domains it has VERIFIED.** An organization supplies its own issuer and client ID, so it controls that identity provider completely and could otherwise assert any address at all — including another company's, or an administrator's. Confining diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index b41ed9d..b9b171c 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -74,11 +74,18 @@ function fromEnv(env, slug) { * already exempts it from domain confinement — and the Microsoft entry is additionally pinned to one * tenant GUID, so only that directory can issue tokens for it. * - * Two limits keep this from becoming the hole the strict check was closing: + * An organization's own provider may assume too, but only once it has DNS-verified a domain — the + * callback confines it to those domains, so it can only ever speak for names it proved it controls. + * Requiring the claim from it as well meant a customer's Entra tenant went green on domain + * verification and then failed the login anyway. + * + * Three limits keep this from becoming the hole the strict check was closing: * - an EXPLICIT `email_verified: false` is always refused. Assuming only ever covers an omitted * claim, never a provider actively saying the address is unverified; - * - org-configured providers can never set it (rowToProvider pins it false, and nothing reads it - * from the database), so the account-takeover path stays shut. + * - for an org provider it is DERIVED from proof (`verified.length > 0`) and is never a column, so + * a customer cannot switch it on for themselves; + * - domain confinement is unchanged and still runs first, so an org provider that assumes still + * cannot assert an address outside a domain it has proven. */ function emailIsVerified(claims, provider) { const asserted = (claims || {}).email_verified; @@ -211,6 +218,9 @@ function db() { } function rowToProvider(row, secretbox) { + // Resolved once: it decides both which addresses this provider may assert and, below, whether it + // has proven anything at all. + const verified = verifiedDomainsFor(row.id); return { slug: row.slug, name: row.name, @@ -226,14 +236,26 @@ function rowToProvider(row, secretbox) { : null, scopes: row.scopes || DEFAULT_SCOPES, /* - * ⚠️ NEVER settable for an organization's provider, and deliberately not read from the row. + * DERIVED from proof, never read from a column. * - * A customer chooses this provider, so it speaks for the party it vouches for. Letting an org - * assume verification would hand back exactly the takeover primitive the strict check exists to - * stop. Domain confinement narrows WHICH addresses it may assert; this keeps the assertion - * itself honest. + * Entra ID v2 omits email_verified, so demanding it refused every customer who brought their own + * Microsoft tenant — the domain went green and the login still failed. Requiring a claim + * Microsoft does not send is not a security control, it is an outage. + * + * What makes it safe to stop requiring it is the proof that already gates this provider: the + * callback confines it to DNS-verified domains, and an address is only reached here after + * passing that. Whoever controls a domain's DNS controls its mail, which is the same trust that + * makes a verification link meaningful in the first place. + * + * So the assumption is tied to having proven SOMETHING. A provider with no verified domain + * assumes nothing — belt and braces, because emailAllowedForProvider already refuses it (an + * empty allow-list matches no domain), and this way a future refactor that reorders those checks + * cannot silently widen it. + * + * ⚠️ Still never a column. An org must not be able to switch this on for itself; it is a + * consequence of DNS proof, not a setting. */ - assumeEmailVerified: false, + assumeEmailVerified: verified.length > 0, source: 'org', organizationId: row.organization_id, /* @@ -244,7 +266,7 @@ function rowToProvider(row, secretbox) { * Reading the typed column here would reduce the whole verification feature to a decoration: * a tenant could type any company's domain and immediately assert addresses in it. */ - emailDomains: verifiedDomainsFor(row.id).join(','), + emailDomains: verified.join(','), }; } diff --git a/server/test/oidc-email-verified-policy.test.js b/server/test/oidc-email-verified-policy.test.js index 7bdbeda..ca8ed46 100644 --- a/server/test/oidc-email-verified-policy.test.js +++ b/server/test/oidc-email-verified-policy.test.js @@ -9,10 +9,14 @@ * never pushed a Microsoft-shaped token through the policy, so nothing failed. * * The rule these tests pin down: - * - `email_verified: true` -> believed, always - * - claim ABSENT + operator-chosen -> believed (Microsoft, or an opted-in generic provider) - * - claim ABSENT + org-configured -> refused - * - `email_verified: false` -> refused, whoever asked + * - `email_verified: true` -> believed, always + * - claim ABSENT + operator-chosen -> believed (Microsoft, or an opted-in provider) + * - claim ABSENT + org with a VERIFIED domain -> believed (it proved DNS control) + * - claim ABSENT + org with NO verified domain -> refused (it has proven nothing) + * - `email_verified: false` -> refused, whoever asked + * + * The org case is a consequence of DNS proof, never a setting: an organization must not be able to + * turn it on for itself, so it is derived and never read from a column. */ const { test } = require('node:test'); @@ -22,10 +26,18 @@ const { emailIsVerified, list } = require('../lib/oidc-providers'); const MS_ENV = { MICROSOFT_CLIENT_ID: 'client-abc', MICROSOFT_TENANT_ID: 'ffffffff-1111-2222-3333-444444444444' }; const microsoft = () => list(MS_ENV).find((p) => p.slug === 'microsoft'); const google = () => list({ GOOGLE_CLIENT_ID: 'g-abc' }).find((p) => p.slug === 'google'); -const orgProvider = { slug: 'acme7f3', source: 'org', organizationId: 'org-1', assumeEmailVerified: false }; +// An org provider as rowToProvider builds it: `assumeEmailVerified` follows from whether any domain +// has actually been DNS-verified. +const orgProvider = (verifiedDomains = []) => ({ + slug: 'acme7f3', source: 'org', organizationId: 'org-1', + emailDomains: verifiedDomains.join(','), + assumeEmailVerified: verifiedDomains.length > 0, +}); +const orgUnproven = orgProvider(); // configured, nothing verified yet +const orgProven = orgProvider(['bytetinker.net']); // TXT published, domain green test('an explicit true is believed from any provider', () => { - for (const p of [microsoft(), google(), orgProvider]) { + for (const p of [microsoft(), google(), orgUnproven, orgProven]) { assert.equal(emailIsVerified({ email_verified: true }, p), true, `${p.slug} should accept an explicit true`); } }); @@ -42,14 +54,26 @@ test('Google stays strict — it does send the claim, so there is nothing to ass assert.equal(emailIsVerified({ email: 'someone@example.com' }, g), false); }); -test('an ORG-configured provider may never assume, even if the object claims it can', () => { - assert.equal(emailIsVerified({ email: 'a@b.c' }, orgProvider), false); - // Belt and braces: a tampered/hand-built org object must not be able to opt itself in through - // the database, which is why rowToProvider pins the field rather than reading a column. +test('an org provider that has proven a domain may assume — the customer-Entra case', () => { + // Entra sends no email_verified. Before this, the domain went green and the login still failed. + assert.equal(emailIsVerified({ email: 'dan@bytetinker.net' }, orgProven), true); +}); + +test('an org provider that has proven NOTHING assumes nothing', () => { + assert.equal(emailIsVerified({ email: 'dan@bytetinker.net' }, orgUnproven), false); +}); + +test('the org assumption is DERIVED from proof, never readable from the row', () => { const src = require('fs').readFileSync(require.resolve('../lib/oidc-providers'), 'utf8'); - assert.match(src, /assumeEmailVerified: false,\s*\n\s*source: 'org'/, - 'rowToProvider must hard-code assumeEmailVerified:false next to source:org'); + assert.match(src, /assumeEmailVerified: verified\.length > 0,\s*\n\s*source: 'org'/, + 'rowToProvider must derive it from the verified-domain list, next to source:org'); + // The whole point: an organization must not be able to switch this on for itself. assert.doesNotMatch(src, /assumeEmailVerified: *row\./, 'must never be read from the org row'); + // ...and there must be no column for it to be read FROM. Checked against the schema rather than + // this file, where `OIDC__ASSUME_EMAIL_VERIFIED` is a legitimate operator-set env var. + const schema = require('fs').readFileSync(require.resolve('../db/database'), 'utf8'); + assert.doesNotMatch(schema, /assume_email_verified/i, + 'org_sso_providers must have no assume_email_verified column'); }); test('an EXPLICIT false is refused even where absence would be assumed', () => {