diff --git a/README.md b/README.md index e3e334f..526e261 100644 --- a/README.md +++ b/README.md @@ -344,8 +344,16 @@ is filled in for you and their slugs are `google` and `microsoft`: | Variable | Description | |----------|-------------| | `GOOGLE_CLIENT_ID` | OAuth 2.0 client ID from [Google Cloud Console](https://console.cloud.google.com) | +| `GOOGLE_CLIENT_SECRET` | Optional — PKCE means a public client works | | `MICROSOFT_CLIENT_ID` | Application (client) ID from the [Azure portal](https://portal.azure.com) | | `MICROSOFT_TENANT_ID` | **Your tenant GUID — required.** `common`/`organizations` are refused | +| `MICROSOFT_CLIENT_SECRET` | Required in practice — register the redirect URI under the **Web** platform, which Entra treats as a confidential client. A **SPA** registration is rejected at the token endpoint, because this exchange runs server-side and sends no browser `Origin` | + +Register the redirect URI under **Web**, add the **`email`** optional claim under *Token configuration → +ID*, and note that **Entra ID v2 does not send `email_verified`** — ScreenTinker treats a +tenant-pinned Microsoft entry as vouching for the address rather than demanding a claim Microsoft +never emits. An explicit `email_verified: false` is still refused, and an organization's own provider +can never make that assumption. ⚠️ **Multi-tenant Microsoft (`common`) is deliberately refused, and Microsoft sign-in stays disabled until you set a tenant GUID.** Two reasons that point the same way. It cannot work: Microsoft's @@ -366,6 +374,7 @@ OIDC_OKTA_CLIENT_ID=0oa... OIDC_OKTA_NAME=Okta # optional button label OIDC_OKTA_CLIENT_SECRET=... # optional — PKCE means a public client works OIDC_OKTA_SCOPES=openid email profile # optional +OIDC_OKTA_ASSUME_EMAIL_VERIFIED=true # only if the IdP verifies addresses but omits the claim ``` The issuer is the base URL whose `/.well-known/openid-configuration` describes the provider; endpoints diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index dd789b2..b41ed9d 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -52,10 +52,41 @@ function fromEnv(env, slug) { clientId, clientSecret: (env[envKey(slug, 'CLIENT_SECRET')] || '').trim() || null, scopes: (env[envKey(slug, 'SCOPES')] || '').trim() || DEFAULT_SCOPES, + // Escape hatch for an IdP that verifies addresses but does not say so in the token. Off unless + // the operator sets it, and it only ever covers an ABSENT claim — see emailIsVerified(). + assumeEmailVerified: /^(1|true|yes)$/i.test((env[envKey(slug, 'ASSUME_EMAIL_VERIFIED')] || '').trim()), source: 'env', }; } +/** + * May this provider's assertion of `email` be treated as verified? + * + * The login callback used to demand `claims.email_verified === true` outright. That is correct for a + * provider a CUSTOMER configured — such a provider is chosen by the party it vouches for, so + * anything it merely asserts is worth nothing — but it made Microsoft sign-in impossible, because + * **Entra ID v2 does not emit the claim at all**. Every Entra login authenticated successfully and + * was then refused with `email_unverified`. + * + * The distinction that resolves it: `users.email_verified` is OUR state and this is the IdP's claim. + * Whether an address is trustworthy is a decision about WHO WE TRUSTED, not a field we can insist a + * provider populate. An instance-wide provider was chosen by the operator — the same trust that + * 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 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. + */ +function emailIsVerified(claims, provider) { + const asserted = (claims || {}).email_verified; + if (asserted === true) return true; + if (asserted === undefined || asserted === null) return !!(provider && provider.assumeEmailVerified); + return false; // explicit false, or anything else the provider chose to send +} + /** * Every provider this instance offers, in a stable order. * @@ -75,6 +106,8 @@ function list(env = process.env) { clientId: googleId, clientSecret: (env.GOOGLE_CLIENT_SECRET || '').trim() || null, scopes: DEFAULT_SCOPES, + // Google DOES send email_verified. Nothing to assume, so it stays strict. + assumeEmailVerified: false, source: 'env', }); seen.add('google'); @@ -117,6 +150,13 @@ function list(env = process.env) { clientId: msId, clientSecret: (env.MICROSOFT_CLIENT_SECRET || '').trim() || null, scopes: DEFAULT_SCOPES, + /* + * Entra ID v2 never sends email_verified, so demanding it refused every Microsoft login. + * Safe here specifically because this entry is operator-chosen AND pinned to one tenant + * GUID above: only that directory can issue a token whose `iss` matches. An explicit + * email_verified:false is still refused — see emailIsVerified(). + */ + assumeEmailVerified: true, source: 'env', }); seen.add('microsoft'); @@ -185,6 +225,15 @@ function rowToProvider(row, secretbox) { ? (secretbox.decrypt(row.client_secret_enc) ?? (() => { throw new Error('client secret could not be decrypted — re-enter it'); })()) : null, scopes: row.scopes || DEFAULT_SCOPES, + /* + * ⚠️ NEVER settable for an organization's provider, and deliberately not read from the row. + * + * 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. + */ + assumeEmailVerified: false, source: 'org', organizationId: row.organization_id, /* @@ -396,5 +445,5 @@ function ssoOnlyForUser(user) { module.exports = { list, get, publicList, getOrgProvider, ownerOf, forEmail, - ssoOnlyForEmail, ssoOnlyForUser, DEFAULT_SCOPES, SLUG_RE, + ssoOnlyForEmail, ssoOnlyForUser, emailIsVerified, DEFAULT_SCOPES, SLUG_RE, }; diff --git a/server/routes/auth.js b/server/routes/auth.js index 714d9fe..6b7fa53 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -1381,9 +1381,14 @@ router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => { * anyone who can type an address into a sloppy IdP arrive as its owner. Providers that omit the * claim entirely are treated as "not asserted", which is the same answer. */ - // `=== false` accepted an OMITTED claim, which is the opposite of what the comment above says and - // what Azure AD v2 actually sends (it omits it). Absent means not asserted, which is not verified. - if (claims.email_verified !== true) return backToApp(res, { sso_error: 'email_unverified' }); + // `=== false` accepted an OMITTED claim, which is the opposite of what the comment above says. + // But requiring `=== true` refused every Microsoft login, because Azure AD v2 omits the claim + // entirely — so the policy now depends on WHO the provider is, not only on what it sent. An + // explicit false is still refused, and an org-configured provider still cannot assume anything. + // See oidcProviders.emailIsVerified() for why that division is the safe one. + if (!oidcProviders.emailIsVerified(claims, provider)) { + return backToApp(res, { sso_error: 'email_unverified' }); + } try { const result = upsertFederatedUser({ claims, email, provider, req }); diff --git a/server/test/oidc-email-verified-policy.test.js b/server/test/oidc-email-verified-policy.test.js new file mode 100644 index 0000000..7bdbeda --- /dev/null +++ b/server/test/oidc-email-verified-policy.test.js @@ -0,0 +1,77 @@ +'use strict'; + +/* + * Who may be believed about an email address. + * + * Regression: requiring `claims.email_verified === true` made Microsoft sign-in impossible, because + * Entra ID v2 does not send the claim. Every Entra login authenticated and was then refused with + * `email_unverified`. The previous test suite asserted how the Microsoft ISSUER string is built but + * 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 + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +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 }; + +test('an explicit true is believed from any provider', () => { + for (const p of [microsoft(), google(), orgProvider]) { + assert.equal(emailIsVerified({ email_verified: true }, p), true, `${p.slug} should accept an explicit true`); + } +}); + +test('Microsoft omits the claim and is still believed (the regression)', () => { + const ms = microsoft(); + assert.equal(ms.assumeEmailVerified, true, 'the tenant-pinned Microsoft entry must assume verification'); + assert.equal(emailIsVerified({ email: 'someone@example.com' }, ms), true); +}); + +test('Google stays strict — it does send the claim, so there is nothing to assume', () => { + const g = google(); + assert.equal(g.assumeEmailVerified, false); + 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. + 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.doesNotMatch(src, /assumeEmailVerified: *row\./, 'must never be read from the org row'); +}); + +test('an EXPLICIT false is refused even where absence would be assumed', () => { + assert.equal(emailIsVerified({ email_verified: false }, microsoft()), false); + assert.equal(emailIsVerified({ email_verified: 'false' }, microsoft()), false, 'a string is not a true'); + assert.equal(emailIsVerified({ email_verified: 0 }, microsoft()), false); +}); + +test('a generic provider can opt in by env, and is strict without it', () => { + const base = { OIDC_PROVIDERS: 'keycloak', OIDC_KEYCLOAK_ISSUER: 'https://kc.example.com', OIDC_KEYCLOAK_CLIENT_ID: 'kc' }; + const strict = list(base).find((p) => p.slug === 'keycloak'); + assert.equal(strict.assumeEmailVerified, false); + assert.equal(emailIsVerified({}, strict), false); + + const opted = list({ ...base, OIDC_KEYCLOAK_ASSUME_EMAIL_VERIFIED: 'true' }).find((p) => p.slug === 'keycloak'); + assert.equal(opted.assumeEmailVerified, true); + assert.equal(emailIsVerified({}, opted), true); + assert.equal(emailIsVerified({ email_verified: false }, opted), false, 'opting in never overrides an explicit false'); +}); + +test('missing claims object or provider does not throw and does not pass', () => { + assert.equal(emailIsVerified(null, microsoft()), true, 'no claims at all still consults the provider policy'); + assert.equal(emailIsVerified({}, null), false); + assert.equal(emailIsVerified({}, undefined), false); +});