diff --git a/.changeset/sso-verify-domain-disabled-answers-disabled-code.md b/.changeset/sso-verify-domain-disabled-answers-disabled-code.md new file mode 100644 index 0000000000..77f100b382 --- /dev/null +++ b/.changeset/sso-verify-domain-disabled-answers-disabled-code.md @@ -0,0 +1,56 @@ +--- +"@objectstack/plugin-auth": minor +--- + +`POST /admin/sso/verify-domain` now answers the DISABLED condition the way its +sibling always has. When SSO domain verification is off for an environment, +`@better-auth/sso` never mounts the inner endpoint and answers `404` with no +code. Both bridge routes recognise that shape, and they used to answer it +differently (#10859): + +| route | answered | answers instead | +| --- | --- | --- | +| `POST /admin/sso/request-domain-verification` | `400` `DOMAIN_VERIFICATION_DISABLED` | unchanged | +| `POST /admin/sso/verify-domain` | `404` `DOMAIN_VERIFICATION_FAILED` | `400` `DOMAIN_VERIFICATION_DISABLED` | + +`verify-domain` rewrote only the `message` for that branch and let the code fall +through to its generic failure default, so the response carried "the feature is +off" copy under a code that means "verification failed". A caller can only act +on the machine-readable half, and the two halves disagreed. The status moves +with the code: the inner `404` describes the INNER endpoint, which is unmounted, +whereas this bridge route is mounted unconditionally — passing that status +through said "no such endpoint" about a resource that exists. + +If you match on `DOMAIN_VERIFICATION_FAILED` (or on `404`) to detect the +disabled case on `verify-domain`, match on `DOMAIN_VERIFICATION_DISABLED` (or on +`400`) instead — the same pair `request-domain-verification` has always +answered. The distinction is worth having: `DISABLED` means "turn on +`OS_SSO_DOMAIN_VERIFICATION`", `FAILED` means "the DNS TXT record is not visible +yet, retry". + +**No `packages/spec` change, and the emitted vocabulary gains no member.** Both +codes are already registered for `@objectstack/plugin-auth` in the error-code +ledger, with exactly these meanings (`DOMAIN_VERIFICATION_DISABLED` — "domain +verification is off on this deployment"). This route was emitting a *declared* +code whose registered meaning is a different condition, so this is +declared-vs-enforced restoration rather than a new contract decision. + +**A genuine verification failure still answers the failure code, and the vendor +pass-through arm is untouched on both routes.** The rewrite is keyed to the +disabled shape specifically — `404` *without* a code. A `404` that carries +`@better-auth/sso`'s own code is the vendor's diagnosis and reaches the caller +verbatim, status included, as does every non-404 failure. That direction is the +load-bearing one — an implementation keyed to "any 404", or to `!resp.ok`, would +satisfy the disabled case while destroying the diagnosis a caller acts on — and +it is pinned in both directions in +`packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts`. + +Shipped as `minor`, following the same call the casing rename on these two +routes made (#10716). The argument for it: the vocabulary is unchanged, and the +old pairing was self-contradictory rather than a contract anyone could have +relied on deliberately. The argument against it, stated here rather than +settled: unlike that rename — whose old spellings were undeclared values no +schema admitted — `DOMAIN_VERIFICATION_FAILED` *is* a declared, registered code, +so a client keyed to it for this case was keyed to something the published +contract admitted, and both halves of the answer change. A reviewer who reads +that as `major` is not reading it wrong; this PR does not decide it silently. diff --git a/packages/plugins/plugin-auth/src/register-sso-provider.ts b/packages/plugins/plugin-auth/src/register-sso-provider.ts index 62e381c3ab..429a1399ba 100644 --- a/packages/plugins/plugin-auth/src/register-sso-provider.ts +++ b/packages/plugins/plugin-auth/src/register-sso-provider.ts @@ -457,11 +457,21 @@ export async function runVerifyDomain( if (resp.ok) { return { status: 200, body: { success: true, data: { providerId, verified: true, message: 'Domain ownership verified — this provider can now sign users in.' } } }; } + // The feature is OFF for this env: `@better-auth/sso` never mounted the inner + // endpoint, so its `404` describes the INNER route. THIS route is mounted + // unconditionally, so passing that status through says "no such endpoint" + // about a resource that exists. Answer the sibling's answer instead — same + // condition, same code, same status (#10859). Before this, the branch + // rewrote only the `message` and let the code fall through to the generic + // default below, so the machine-readable field said "verification failed" + // while the human-readable one said "the feature is off"; a caller can only + // act on the first. + if (resp.status === 404 && !parsed?.code) { + return { status: 400, body: { success: false, error: { code: 'DOMAIN_VERIFICATION_DISABLED', message: 'Domain verification is not enabled for this environment (set OS_SSO_DOMAIN_VERIFICATION).' } } }; + } // Friendlier copy for the expected failure modes. let message = parsed?.message || 'Domain verification failed'; - if (resp.status === 404 && !parsed?.code) { - message = 'Domain verification is not enabled for this environment (set OS_SSO_DOMAIN_VERIFICATION).'; - } else if (parsed?.code === 'NO_PENDING_VERIFICATION') { + if (parsed?.code === 'NO_PENDING_VERIFICATION') { message = 'No pending verification — click “Request Domain Verification” first to get the DNS record.'; } else if (parsed?.code === 'DOMAIN_VERIFICATION_FAILED') { message = 'DNS TXT record not found yet. Add the record shown when you requested verification, allow time for DNS to propagate, then retry.'; diff --git a/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts b/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts index fadee75d94..7213137978 100644 --- a/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts +++ b/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts @@ -64,13 +64,18 @@ describe('#10716 SSO domain verification — our default is a registered ADR-011 }); it('verify-domain: an uncoded vendor failure answers our SCREAMING default, status passed through', async () => { - // The 404-without-a-code shape: SSO domain verification is off for this env. - // This is the response the dogfood admin-route probe observes. - const res = await runVerifyDomain(fakeHandle(404, undefined), post(VERIFY_URL)); + // RE-FIXTURED by #10859. This case used to be driven by `fakeHandle(404, + // undefined)` — which is the DISABLED shape, not a verification failure, and + // now answers `DOMAIN_VERIFICATION_DISABLED` (see the #10859 describe below). + // Pointing the generic-default leg at that shape would have left the default + // arm of `verify-domain` with no coverage at all once the disabled branch + // returned early, so it is driven by a real uncoded failure instead — the + // same `502` its sibling above uses. + const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL)); expect(res.body.success).toBe(false); expect(res.body.error?.code).toBe(OUR_DEFAULT); - expect(res.status).toBe(404); + expect(res.status).toBe(502); }); it('both defaults are SCREAMING_SNAKE and registered for this package — reused, not invented', () => { @@ -116,3 +121,83 @@ describe('#10716 the vendor pass-through arm is untouched', () => { expect(res.status).toBe(400); }); }); + +describe('#10859 the DISABLED condition gets ONE answer across both routes', () => { + /** + * The feature being off is a different condition from a verification failing, + * and the two SSO domain-verification routes used to answer it differently: + * the sibling rewrote to `400 DOMAIN_VERIFICATION_DISABLED`, while + * `verify-domain` rewrote only the MESSAGE and let the code fall through to + * the generic failure default — so its machine-readable field said + * "verification failed" while its human-readable one said "the feature is + * off". A caller can only act on the first. + * + * Both halves are asserted per ADR-0112 (`code` AND `status`), and the + * counter-direction below is the load-bearing half: stamping `DISABLED` on + * every failure would pass a one-directional suite while destroying exactly + * the diagnosis the caller acts on. + */ + const DISABLED = 'DOMAIN_VERIFICATION_DISABLED'; + /** The inner endpoint is unmounted when the feature is off: 404, no code. */ + const disabledInner = () => fakeHandle(404, undefined); + + it('verify-domain: the disabled condition answers the dedicated code at the sibling’s status', async () => { + const res = await runVerifyDomain(disabledInner(), post(VERIFY_URL)); + + expect(res.body.success).toBe(false); + expect(res.body.error?.code).toBe(DISABLED); + expect(res.status).toBe(400); + expect(res.body.error?.message).toContain('OS_SSO_DOMAIN_VERIFICATION'); + }); + + it('both routes answer the SAME code and the SAME status for the SAME condition', async () => { + // The card's governing invariant, asserted directly rather than inferred + // from the two per-route cases above: one answer for one condition. + const verify = await runVerifyDomain(disabledInner(), post(VERIFY_URL)); + const request = await runRequestDomainVerification(disabledInner(), post(REQUEST_URL)); + + expect(verify.body.error?.code).toBe(request.body.error?.code); + expect(verify.status).toBe(request.status); + expect(verify.body.error?.code).toBe(DISABLED); + expect(verify.status).toBe(400); + }); + + it('DOMAIN_VERIFICATION_DISABLED is registered for this package — reused, not invented', () => { + expect(DISABLED).toMatch(/^[A-Z][A-Z0-9_]*$/); + expect(ERROR_CODE_LEDGER['@objectstack/plugin-auth']).toContain(DISABLED); + }); + + // ── the load-bearing direction: DISABLED is NOT stamped on every failure ── + + it('verify-domain: a genuine verification failure still answers the FAILURE code', async () => { + // Uncoded, but not the disabled shape. An implementation that keyed on + // `!resp.ok` instead of the 404-without-a-code shape would answer DISABLED + // here and tell the admin to flip an env var that is already on. + const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL)); + + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(502); + }); + + it('verify-domain: a 404 that CARRIES a vendor code is the vendor’s diagnosis, not DISABLED', async () => { + // The disabled shape is 404 *without* a code. A 404 that carries one is the + // vendor answering, and both its code and its status pass through untouched + // — the arm #10716 pinned, re-pinned here at the status this branch tests. + const res = await runVerifyDomain(fakeHandle(404, { code: VENDOR_CODE, message: 'vendor copy' }), post(VERIFY_URL)); + + expect(res.body.error?.code).toBe(VENDOR_CODE); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(404); + }); + + it('request-domain-verification: a genuine failure still answers the FAILURE code', async () => { + // The sibling's counter-direction, so the parity assertion above cannot be + // satisfied by both routes collapsing onto DISABLED. + const res = await runRequestDomainVerification(fakeHandle(502, { message: 'upstream exploded' }), post(REQUEST_URL)); + + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(502); + }); +}); diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index abd2e29f86..90539d6f52 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -258,7 +258,7 @@ function expectationsFor(targetUserId: string): Record 'POST /api/v1/auth/admin/sso/verify-domain': { bucket: 'objectstack-gate', body: { providerId: 'refusal-probe-oidc' }, - note: 'admin passes the gate and lands on 404 DOMAIN_VERIFICATION_FAILED while SSO is off', + note: 'admin passes the gate and lands on 400 DOMAIN_VERIFICATION_DISABLED while SSO is off (#10859 — it answered 404 DOMAIN_VERIFICATION_FAILED before, out of step with the sibling above)', }, // ── #9652: ban / unban moved from the vendor to an ObjectStack mount ────