From 512d4ebb50a278198a6883c5cebdde47aa1ec551 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:41:53 +0000 Subject: [PATCH] =?UTF-8?q?fix(plugin-auth):=20/sso/register=20=E9=97=A8?= =?UTF-8?q?=E7=A6=81=E6=94=B9=E7=94=A8=E5=94=AF=E4=B8=80=E9=82=A3=E6=8A=8A?= =?UTF-8?q?=E7=AE=A1=E7=90=86=E5=91=98=E7=AD=89=E7=BA=A7=E5=B0=BA=20(#5942?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isOrgOrPlatformAdmin` 的 membership 半边此前手抄了一份判据 (`split(',').map(trim).some(=== 'owner' || === 'admin')`),大小写敏感且只认 字符串。同一个问题在 plugin-auth 内的另一把尺 —— `invitation-role-cap.ts` 的 等级尺(`isOrgAdminGrade`,break-glass ban 守卫在用)—— 会 `.toLowerCase()` 并处理数组拼写。于是 `sys_member.role='Owner'` 被 ban 守卫算作管理员、被 `/sso/register` 门禁算作非管理员,两个方向的错都不出声。 改为直接问 `isOrgAdminGrade(m?.role)`,「哪种 membership 算管理员」在 plugin-auth 内只剩一个答案。 行为变化只有放宽一个方向,且只放宽在此前判错的取值上(大小写非常规值与数组 拼写从误拒变正确放行);无任何收窄 —— 已按 ADR-0108 封闭词表逐值实测。 platform_admin 半边未改动。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- ...sso-register-gate-one-admin-grade-ruler.md | 36 ++++ .../plugin-auth/src/auth-manager.test.ts | 191 ++++++++++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 37 ++-- 3 files changed, 251 insertions(+), 13 deletions(-) create mode 100644 .changeset/sso-register-gate-one-admin-grade-ruler.md diff --git a/.changeset/sso-register-gate-one-admin-grade-ruler.md b/.changeset/sso-register-gate-one-admin-grade-ruler.md new file mode 100644 index 0000000000..8ba70ef51f --- /dev/null +++ b/.changeset/sso-register-gate-one-admin-grade-ruler.md @@ -0,0 +1,36 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): `/sso/register` 的管理员门禁改用唯一那把等级尺,不再手抄一份大小写敏感的判据 (#5942) + +ADR-0024 的 `POST /sso/register` 门禁问的是「这个 membership 是不是本组织的管理员」。 +它此前用的是一份手抄判据: + +```ts +raw.split(',').map((s) => s.trim()).some((r) => r === 'owner' || r === 'admin') +``` + +同一个问题在 plugin-auth 内还有另一把尺 —— `invitation-role-cap.ts` 的等级尺 +(`parseOrgRoles()` 会 `.trim().toLowerCase()`,`isOrgAdminGrade()` 据此评级), +break-glass ban 守卫(`last-admin-ban-guard.ts`,ADR-0024 D5.2)用的就是它。 +两把尺在大小写上不一致:`sys_member.role` 若存成 `Owner` / `ADMIN`,ban 守卫把这一行 +算作**管理员**,而 `/sso/register` 门禁算作**非管理员**。同一条安全路径上的两个答案 +互相矛盾,而且两个方向的错都不出声。 + +现在门禁改问 `isOrgAdminGrade(m.role)` —— 「哪种 membership 算管理员」在 plugin-auth +内只剩一个答案,两处自此同尺。 + +**用户可见的行为变化,只有一个方向:放宽,且只放宽在此前判错的取值上。** +`sys_member.role` 为大小写非常规值(`Owner` / `ADMIN` / ` Admin `,以及 +`member,Owner` 这类逗号拼写)或数组拼写(`['owner']`)的成员,此前会被 +`/sso/register` **误拒**,现在正确判为管理员并放行。**没有任何收窄**:此前被判为管理员 +的取值,换尺后仍然是管理员(已逐值实测,见 PR)。 + +ADR-0108 的封闭词表(`owner` / `admin` / `delegated_admin` / `member`)全为小写,UI 与 +better-auth 写入的也是小写,所以正常部署下答案逐值不变 —— 这也是为什么它此前只是一条 +静默分歧,而不是线上故障。要撞上分歧得有一条绕过表单的写入(导入、外部写入、手工 SQL)。 + +`isOrgOrPlatformAdmin` 名字里的 platform_admin 半边**未改动**,仍由 +`packages/core/src/security/resolve-authz-context.ts` 权威推导;那几处实现的合流是 +另一个决策件。 diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index e6d4b23491..b25e34dcca 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -3659,3 +3659,194 @@ describe('getPublicConfig devSeedAdmin (dev-only login hint)', () => { expect((manager.getPublicConfig() as any).devSeedAdmin).toBeUndefined(); }); }); + +// --------------------------------------------------------------------------- +// [#5942] `isOrgOrPlatformAdmin` — the ADR-0024 `/sso/register` admin gate's +// criterion — asks "does this membership administer the org" through the ONE +// grade ladder (`isOrgAdminGrade`, `invitation-role-cap.ts`), not a hand-copied +// `role === 'owner' || role === 'admin'`. +// +// The hand-copy it replaces did `.split(',').map(trim).some(=== 'owner' || +// === 'admin')` — case-SENSITIVE, and blind to the array spelling. The grade +// ladder additionally `.toLowerCase()`s and joins arrays, so the two answered +// differently on `Owner` / `ADMIN` / `['owner']`: this gate refused a real +// administrator (false negative) while the break-glass ban guard +// (`last-admin-ban-guard.ts`, same ladder) counted the same row AS an +// administrator. Two spellings of one security question, diverging silently. +// +// Direction of the change, measured (see the PR body): every difference is a +// WIDENING, and only over values the old spelling judged wrongly. There is no +// value that was admin before and is not admin now — the closed ADR-0108 +// vocabulary (all lowercase) answers identically on both sides, which is why +// no user could hit this today. +// +// NOTE on `' admin '`: it is a regression pin, NOT a before-red case. The +// hand-copy already trimmed, so it answered `true` before the change too. Only +// the CASE and ARRAY spellings actually move. +// +// The platform-admin half of this method is deliberately untouched (#5942 is +// scoped to the org ruler); the platform-admin cases below pin that. +// --------------------------------------------------------------------------- +describe('isOrgOrPlatformAdmin – one grade ruler for "is this membership an admin" (#5942)', () => { + const SECRET = 'test-secret-at-least-32-chars-long'; + + /** + * Read-only engine stub: `members` are the `sys_member` rows, `platformAdmin` + * controls the org-less `admin_full_access` link. `find` honours the `where` + * the gate actually passes (`user_id`, and `organization_id` when an active + * org is set) so the org-scoping half is the product's, not the fixture's. + */ + const makeEngine = (opts: { members?: any[]; platformAdmin?: boolean; throws?: boolean } = {}) => ({ + find: vi.fn(async (object: string, query?: any) => { + if (opts.throws) throw new Error('db down'); + if (object === 'sys_user_permission_set') { + return opts.platformAdmin + ? [{ user_id: 'u-1', permission_set_id: 'ps-admin', organization_id: null }] + : []; + } + if (object === 'sys_permission_set') return [{ id: 'ps-admin', name: 'admin_full_access' }]; + if (object === 'sys_member') { + const where = query?.where ?? {}; + return (opts.members ?? []).filter((row) => + Object.entries(where).every(([k, v]) => row[k] === v), + ); + } + return []; + }), + findOne: vi.fn(), + }); + + /** The gate's criterion, invoked exactly as the `/sso/register` hook does. */ + const judge = async ( + engine: any, + activeOrgId?: string, + userId = 'u-1', + ): Promise => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const manager = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + }); + warn.mockRestore(); + return (manager as any).isOrgOrPlatformAdmin(userId, activeOrgId); + }; + + const memberRow = (role: unknown) => ({ + id: 'm-1', + user_id: 'u-1', + organization_id: 'org-1', + role, + }); + + // -- (1) the fix itself: values the hand-copy refused, the ladder admits ---- + describe('case-insensitive + array spellings (before: refused, after: admitted)', () => { + it.each([ + ['Owner', 'better-auth owner, capitalized by an import'], + ['ADMIN', 'shout-cased by a hand-written SQL insert'], + [' Admin ', 'padded AND capitalized'], + ['OWNER', 'shout-cased owner'], + ['member,Owner', 'comma-joined with one capitalized administrative role'], + ])('grades %j as an administrator (%s)', async (role) => { + expect(await judge(makeEngine({ members: [memberRow(role)] }), 'org-1')).toBe(true); + }); + + it('grades the ARRAY spelling ["owner"] as an administrator', async () => { + // The hand-copy read `typeof m.role === 'string' ? m.role : ''`, so any + // array-valued role graded as nothing at all. + expect(await judge(makeEngine({ members: [memberRow(['owner'])] }), 'org-1')).toBe(true); + }); + + it('grades the ARRAY spelling ["member","Admin"] as an administrator', async () => { + expect( + await judge(makeEngine({ members: [memberRow(['member', 'Admin'])] }), 'org-1'), + ).toBe(true); + }); + }); + + // -- (2) regression: the closed ADR-0108 vocabulary answers identically ----- + describe('closed membership vocabulary (ADR-0108) — unchanged by the new ruler', () => { + it.each([ + ['owner', true], + ['admin', true], + ['delegated_admin', false], + ['member', false], + ] as const)('grades the built-in %j as admin=%s', async (role, expected) => { + expect(await judge(makeEngine({ members: [memberRow(role)] }), 'org-1')).toBe(expected); + }); + + it.each([ + ['owner,member', true], + ['member,admin', true], + [' admin ', true], + ['member,delegated_admin', false], + ] as const)( + 'grades the comma/whitespace spelling %j as admin=%s (already true before #5942)', + async (role, expected) => { + expect(await judge(makeEngine({ members: [memberRow(role)] }), 'org-1')).toBe(expected); + }, + ); + }); + + // -- (3) non-administrative values still refused (no widening past admin) --- + describe('fail-closed floor — nothing else is admitted', () => { + it.each([ + ['manager', 'an app-registered name that is not an administrative grade'], + ['administrator', 'a near-miss that is not the vocabulary'], + ['adminx', 'a prefix collision'], + ['', 'an empty role'], + ])('refuses %j (%s)', async (role) => { + expect(await judge(makeEngine({ members: [memberRow(role)] }), 'org-1')).toBe(false); + }); + + it.each([ + [null, 'null'], + [undefined, 'undefined'], + [42, 'a number'], + [{ role: 'owner' }, 'an object that merely mentions owner'], + ])('refuses a non-string role (%s: %s)', async (role) => { + expect(await judge(makeEngine({ members: [memberRow(role)] }), 'org-1')).toBe(false); + }); + + it('refuses when the user has no membership row at all', async () => { + expect(await judge(makeEngine({ members: [] }), 'org-1')).toBe(false); + }); + + it('refuses when the engine read throws (fail CLOSED — ADR-0024)', async () => { + expect(await judge(makeEngine({ throws: true }), 'org-1')).toBe(false); + }); + }); + + // -- (4) org scoping and the untouched platform-admin half ----------------- + describe('scoping and the platform-admin half (untouched by #5942)', () => { + it('judges only the ACTIVE org when one is set', async () => { + const engine = makeEngine({ + members: [ + { id: 'm-1', user_id: 'u-1', organization_id: 'org-other', role: 'Owner' }, + { id: 'm-2', user_id: 'u-1', organization_id: 'org-1', role: 'member' }, + ], + }); + // Administrative elsewhere, plain member here → refused for org-1 … + expect(await judge(engine, 'org-1')).toBe(false); + // … and admitted when that other org is the active one. + expect(await judge(engine, 'org-other')).toBe(true); + }); + + it('accepts an administrative membership in ANY org when no active org is set', async () => { + const engine = makeEngine({ + members: [{ id: 'm-1', user_id: 'u-1', organization_id: 'org-other', role: 'ADMIN' }], + }); + expect(await judge(engine, undefined)).toBe(true); + }); + + it('still admits a platform admin whose membership is a plain member', async () => { + const engine = makeEngine({ platformAdmin: true, members: [memberRow('member')] }); + expect(await judge(engine, 'org-1')).toBe(true); + }); + + it('still refuses a non-platform-admin with no administrative membership', async () => { + const engine = makeEngine({ platformAdmin: false, members: [memberRow('member')] }); + expect(await judge(engine, 'org-1')).toBe(false); + }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index c8c7c3dcdf..b814840feb 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -22,7 +22,11 @@ import { postureEnforcesWall, type TenancyPosture } from '@objectstack/spec/secu import { MCP_OAUTH_SCOPES } from '@objectstack/spec/ai'; import { createObjectQLAdapterFactory, withSystemReadContext } from './objectql-adapter.js'; import { runWithAuthActorScope, setAuthActorResolver } from './auth-actor-attribution.js'; -import { invitationRoleCapFailure, isPlainMemberInvitation } from './invitation-role-cap.js'; +import { + invitationRoleCapFailure, + isPlainMemberInvitation, + isOrgAdminGrade, +} from './invitation-role-cap.js'; import { isPlaceholderEmail } from './placeholder-email.js'; import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; import type { TenancyService } from './tenancy-service.js'; @@ -3587,11 +3591,17 @@ export class AuthManager { * True when `userId` is a platform admin (a `sys_user_permission_set` row * pointing at `admin_full_access` with `organization_id = null`) OR an * owner/admin member of `activeOrgId` (any org membership with role - * owner/admin when no active org is set). Mirrors the role-derivation in - * `customSession`; reads through `withSystemReadContext` so the lookups are - * not themselves RLS-scoped to the acting (possibly non-privileged) user. - * Fails CLOSED (returns false) on any lookup error — this backs a security - * gate, so an unverifiable actor must never pass. + * owner/admin when no active org is set). Reads through + * `withSystemReadContext` so the lookups are not themselves RLS-scoped to the + * acting (possibly non-privileged) user. Fails CLOSED (returns false) on any + * lookup error — this backs a security gate, so an unverifiable actor must + * never pass. + * + * [#5942] The membership half asks {@link isOrgAdminGrade} — the single grade + * ladder in `invitation-role-cap.ts`, shared with the break-glass ban guard — + * so "which membership is an administrator" has exactly one answer inside + * plugin-auth. The platform-admin half above is unchanged and still has its + * own derivations elsewhere (`resolve-authz-context.ts` is authoritative). */ private async isOrgOrPlatformAdmin( userId: string, @@ -3623,13 +3633,14 @@ export class AuthManager { if (activeOrgId) where.organization_id = activeOrgId; const members = await sys.find('sys_member', { where, limit: 10 }); for (const m of (Array.isArray(members) ? members : [])) { - const raw = typeof m?.role === 'string' ? m.role : ''; - if ( - raw - .split(',') - .map((s: string) => s.trim()) - .some((r: string) => r === 'owner' || r === 'admin') - ) { + // [#5942] The ONE grade ladder answers "does this membership administer + // the org" — never a hand-copied `role === 'owner' || role === 'admin'`. + // The copy that used to live here was case-SENSITIVE and string-only, so + // a `sys_member.role` of `Owner` / `ADMIN` / `['owner']` was refused + // here while `last-admin-ban-guard.ts` — same question, same ladder — + // counted that row AS an administrator. Two spellings of one security + // question cannot disagree if there is only one spelling. + if (isOrgAdminGrade(m?.role)) { return true; } }