diff --git a/.changeset/read-write-split.md b/.changeset/read-write-split.md new file mode 100644 index 00000000..1c3ed3e0 --- /dev/null +++ b/.changeset/read-write-split.md @@ -0,0 +1,10 @@ +--- +"ornn-api": minor +"ornn-web": minor +--- + +Separate read and write access (drop the `read_write` level) and show edit UI to write-grantees. + +The combined `read_write` grant level is renamed to `write`: each grant is now `read` or `write` (a `write` grant implies read), with no combined label. Existing `read_write` grants migrate to `write` automatically at boot — non-disruptive and rollback-safe. + +This also fixes a bug where a user with **write** access saw no edit controls: the skill detail page (Edit button, inline file editor, Save) and the edit page now reveal content-editing to write-grantees, not just the owner, while admin actions (permissions, transfer, delete, visibility) remain owner/admin-only. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4cd7623f..5fa022e1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -43,7 +43,7 @@ so dashboards can disambiguate from frontend events of the same name): | `api.skill.published` | skill create + version publish | `skillId`, `skillVersion`, `isNewSkill` | | `user.login` / `user.logout` | session open / close | — | | `skill.created` / `.updated` / `.deleted` / `.version_deleted` | mutation routes | `skillId`, `skillName`, `version`, `adminAction?` | -| `skill.visibility_changed` / `.permissions_changed` | visibility + sharing flips | `skillId`, `isPrivate`, `sharedWithUsers`, `sharedWithOrgs`, `readWriteGrants` (count of `read_write` grants, #1123) | +| `skill.visibility_changed` / `.permissions_changed` | visibility + sharing flips | `skillId`, `isPrivate`, `sharedWithUsers`, `sharedWithOrgs`, `writeGrants` (count of `write` grants, #1123) | | `skill.ownership_transferred` | ownership handed to another user (#1123) | `skillId`, `skillName`, `priorOwnerId`, `newOwnerId` | | `skill.refresh` / `.source_linked` / `.source_unlinked` | source-pointer ops | `skillId`, `repo`, `ref`, `commit` | | `skill.nyxid_service_tied` / `.agentseal_rescanned` | tie + admin-rescan | `skillId`, `isSystemSkill`, `score` | diff --git a/docs/CONVENTIONS.md b/docs/CONVENTIONS.md index 76174eb8..fadf84be 100644 --- a/docs/CONVENTIONS.md +++ b/docs/CONVENTIONS.md @@ -346,7 +346,7 @@ Endpoint-specific. Rules: Format: `ornn::`. These are the **request scopes** NyxID mints onto an access token — `requirePermission(...)` middleware checks them per route. They gate *who may call an endpoint at all*; they are **distinct -from** the per-object READ / READ_WRITE / ADMIN tier (§5.4), which decides +from** the per-object READ / WRITE / ADMIN tier (§5.4), which decides *what the caller may do to a specific skill/skillset*. A caller needs both: the route scope to reach the handler, and the object tier to act on the target. @@ -355,7 +355,7 @@ target. |---|---| | `ornn:skill:read` | Read skills (respects visibility) | | `ornn:skill:create` | Create skills (upload, pull from GitHub) | -| `ornn:skill:update` | Update / publish / refresh / change permissions / transfer ownership / toggle deprecation / bind NyxID service (+ object ADMIN/READ_WRITE per §5.4) | +| `ornn:skill:update` | Update / publish / refresh / change permissions / transfer ownership / toggle deprecation / bind NyxID service (+ object ADMIN/WRITE per §5.4) | | `ornn:skill:delete` | Delete a skill or a single version (+ object ADMIN per §5.4) | | `ornn:skill:build` | Invoke skill generation endpoints (high LLM cost) | | `ornn:playground:use` | Invoke playground chat (runs user code) | @@ -396,11 +396,11 @@ tier decides whether they may act on *this specific* skill/skillset. | Tier | Gate | Who qualifies | What it grants | |---|---|---|---| -| **READ** | `canReadSkill` | Public skill → anyone. Private → author, platform admin, or any grantee (`read` **or** `read_write`), directly or via a granted org. | View / pull / execute / list versions. | -| **READ_WRITE** | `canWriteSkill` | Author **OR** platform admin **OR** a `read_write` grantee (direct or via a granted org). | READ, plus update the skill's **content + metadata only** (publish a new version). | +| **READ** | `canReadSkill` | Public skill → anyone. Private → author, platform admin, or any grantee (`read` **or** `write`), directly or via a granted org. | View / pull / execute / list versions. | +| **WRITE** | `canWriteSkill` | Author **OR** platform admin **OR** a `write` grantee (direct or via a granted org). | READ, plus update the skill's **content + metadata only** (publish a new version). | | **ADMIN** | `canManageSkill` | Author **OR** platform admin **only**. | Change permissions, transfer ownership, delete skill/version, toggle deprecation, manage dist-tags, bind a NyxID service. | -A `read_write` grantee is **never** an admin — the danger-zone operations stay +A `write` grantee is **never** an admin — the danger-zone operations stay with the author (`createdBy`) and platform admins (`ornn:admin:skill`). Org grants resolve uniformly: every admin/member of a granted org inherits the grant's level. The org-membership gates fail soft on an unresolved NyxID @@ -416,15 +416,16 @@ skill/skillset detail responses and accepted by the permissions endpoints { "grants": [ { "type": "user", "id": "", "level": "read" }, - { "type": "org", "id": "", "level": "read_write" } + { "type": "org", "id": "", "level": "write" } ] } ``` - `type` — `"user"` (a NyxID person user_id) or `"org"` (a NyxID org user_id). - `id` — the principal's NyxID id (1..128 chars). -- `level` — `"read"` or `"read_write"`. An invalid value is rejected with - `invalid_permission_level` (a `validation_error` subcode — see `docs/ERRORS.md`). +- `level` — `"read"` or `"write"` (a `write` grant implies read). An invalid + value is rejected with `invalid_permission_level` (a `validation_error` + subcode — see `docs/ERRORS.md`). The author (`createdBy`) is never represented in `grants` — they hold implicit ADMIN. The legacy read-only `sharedWithUsers` / `sharedWithOrgs` arrays are @@ -440,7 +441,7 @@ POST /v1/skillsets/:id/transfer-ownership { "newOwnerUserId": "" } ``` - **Auth:** ADMIN tier (`canManageSkill`) — author or platform admin only. A - `read_write` grantee can never transfer. Rides on the existing + `write` grantee can never transfer. Rides on the existing `ornn:skill:update` request scope; **no new scope** was added. - **Behavior:** immediate, synchronous transfer. The target becomes the new owner (`createdBy`); the prior owner is kept as a **READ** grantee (retains @@ -627,7 +628,7 @@ Per-test teardown is the test's responsibility; shared fixtures live in `tests/f - [ ] `X-Request-ID` on every response; `requestId` in every error body - [ ] Query params camelCase; arrays as repeated keys; `q` for search - [ ] Required request scopes from the catalog declared in OpenAPI `security` (§5.2) -- [ ] Object-level authz, where the target is an owned resource, gates on the READ / READ_WRITE / ADMIN tier (§5.4) — distinct from the route scope +- [ ] Object-level authz, where the target is an owned resource, gates on the READ / WRITE / ADMIN tier (§5.4) — distinct from the route scope - [ ] Content negotiation for multi-representation resources - [ ] SSE events named `_` snake_case - [ ] Deprecation uses RFC 8594 headers diff --git a/docs/ERRORS.md b/docs/ERRORS.md index 06193901..1d40a33a 100644 --- a/docs/ERRORS.md +++ b/docs/ERRORS.md @@ -67,9 +67,9 @@ The uploaded payload is not a parseable ZIP — a malformed or unreadable archiv ### invalid_permission_level -A typed `grants` entry on a permissions request (#1123) carried a `level` outside the allowed set. The only accepted values are `read` and `read_write` (see [`docs/CONVENTIONS.md`](CONVENTIONS.md) §5.4). Surfaced from `PUT /api/v1/skills/{id}/permissions` and `PUT /api/v1/skillsets/{id}/permissions`. +A typed `grants` entry on a permissions request (#1123) carried a `level` outside the allowed set. The only accepted values are `read` and `write` (see [`docs/CONVENTIONS.md`](CONVENTIONS.md) §5.4). Surfaced from `PUT /api/v1/skills/{id}/permissions` and `PUT /api/v1/skillsets/{id}/permissions`. -**Client action:** set every grant's `level` to `read` or `read_write` and retry. +**Client action:** set every grant's `level` to `read` or `write` and retry. ### invalid_transfer_target diff --git a/ornn-api/src/bootstrap.ts b/ornn-api/src/bootstrap.ts index 59f6b9eb..034a26ce 100644 --- a/ornn-api/src/bootstrap.ts +++ b/ornn-api/src/bootstrap.ts @@ -146,7 +146,7 @@ import { wireAdmin } from "./domains/admin/bootstrap"; // per-provider arrays). One-time, idempotent, runs before any // LlmProvidersService consumer reads from disk. import { migrateModelCatalogIntoProviders } from "./domains/settings/llmProviders/migration"; -import { backfillTypedGrants } from "./domains/skills/crud/grants.migration"; +import { backfillTypedGrants, renameReadWriteGrantsToWrite } from "./domains/skills/crud/grants.migration"; import { createLlmPickerRoutes } from "./domains/settings/llmProviders/routes"; // OpenAPI spec @@ -690,6 +690,18 @@ export async function bootstrap( ), ); + // ---- read_write → write grant-level rename (#1127) ---- + // The combined `read_write` level was renamed to `write`. Rewrite any + // existing grant carrying the legacy value. Idempotent + non-disruptive + // (write confers what read_write did); `coerceStoredGrants` covers any doc + // not yet rewritten, so failure is non-fatal. + await renameReadWriteGrantsToWrite(db).catch((err) => + logger.error( + { err: err instanceof Error ? err.message : String(err) }, + "read_write→write rename failed — coerceStoredGrants maps legacy values at read time, no data loss", + ), + ); + // The picker route — `GET /me/models?surface=...` — reads from the // per-provider arrays via `LlmProvidersService` (already constructed // upstream as part of `domains/settings/...`). The section-default diff --git a/ornn-api/src/domains/skills/crud/authorize.test.ts b/ornn-api/src/domains/skills/crud/authorize.test.ts index 4ad9c5b1..787f3ba1 100644 --- a/ornn-api/src/domains/skills/crud/authorize.test.ts +++ b/ornn-api/src/domains/skills/crud/authorize.test.ts @@ -161,10 +161,10 @@ describe("canReadSkill (#1123 tiers)", () => { expect(canReadSkill(s, actor("stranger"))).toBe(false); }); - it("any grant level confers read — read AND read_write grantees can read", () => { + it("any grant level confers read — read AND write grantees can read", () => { const s = skill("owner", [ { type: "user", id: "reader", level: "read" }, - { type: "user", id: "editor", level: "read_write" }, + { type: "user", id: "editor", level: "write" }, ]); expect(canReadSkill(s, actor("reader"))).toBe(true); expect(canReadSkill(s, actor("editor"))).toBe(true); @@ -189,24 +189,24 @@ describe("canReadSkill (#1123 tiers)", () => { }); }); -describe("canWriteSkill (#1123 READ_WRITE tier)", () => { +describe("canWriteSkill (#1123 WRITE tier)", () => { it("author + platform admin may write", () => { const s = skill("owner", []); expect(canWriteSkill(s, actor("owner"))).toBe(true); expect(canWriteSkill(s, actor("x", { admin: true }))).toBe(true); }); - it("a read grantee may NOT write; a read_write grantee may", () => { + it("a read grantee may NOT write; a write grantee may", () => { const s = skill("owner", [ { type: "user", id: "reader", level: "read" }, - { type: "user", id: "editor", level: "read_write" }, + { type: "user", id: "editor", level: "write" }, ]); expect(canWriteSkill(s, actor("reader"))).toBe(false); expect(canWriteSkill(s, actor("editor"))).toBe(true); }); - it("a read_write org grant lets every member write", () => { - const s = skill("owner", [{ type: "org", id: "org-a", level: "read_write" }]); + it("a write org grant lets every member write", () => { + const s = skill("owner", [{ type: "org", id: "org-a", level: "write" }]); expect(canWriteSkill(s, actor("u", { orgs: ["org-a"] }))).toBe(true); expect(canWriteSkill(s, actor("u", { orgs: ["org-b"] }))).toBe(false); }); @@ -225,8 +225,8 @@ describe("canWriteSkill (#1123 READ_WRITE tier)", () => { }); describe("canManageSkill (#1123 ADMIN tier)", () => { - it("only author + platform admin administer — a read_write grantee never does", () => { - const s = skill("owner", [{ type: "user", id: "editor", level: "read_write" }]); + it("only author + platform admin administer — a write grantee never does", () => { + const s = skill("owner", [{ type: "user", id: "editor", level: "write" }]); expect(canManageSkill(s, actor("owner"))).toBe(true); expect(canManageSkill(s, actor("x", { admin: true }))).toBe(true); expect(canManageSkill(s, actor("editor"))).toBe(false); diff --git a/ornn-api/src/domains/skills/crud/authorize.ts b/ornn-api/src/domains/skills/crud/authorize.ts index 56cb9488..4d00cd91 100644 --- a/ornn-api/src/domains/skills/crud/authorize.ts +++ b/ornn-api/src/domains/skills/crud/authorize.ts @@ -14,21 +14,21 @@ * - PRIVATE skill: * - author (`createdBy === actor.userId`) → yes * - platform admin (`ornn:admin:skill`) → yes - * - actor holds ANY grant (read or read_write), directly or via + * - actor holds ANY grant (read or write), directly or via * membership of a granted org → yes * - else → no * - * READ_WRITE — `canWriteSkill` (update content + metadata only): + * WRITE — `canWriteSkill` (update content + metadata only): * - author → yes * - platform admin → yes - * - actor holds a `read_write` grant, directly or via a granted org → yes + * - actor holds a `write` grant, directly or via a granted org → yes * - else → no * * ADMIN — `canManageSkill` (change permissions, transfer ownership, delete * skill/version, toggle deprecation, manage dist-tags, bind NyxID service): * - author → yes * - platform admin → yes - * - else → 403. A `read_write` grantee is deliberately NOT an admin. + * - else → 403. A `write` grantee is deliberately NOT an admin. * * Org grants resolve uniformly: every admin/member of a granted org inherits * the grant's level. The org-membership-based gates fail soft on an @@ -117,7 +117,7 @@ export async function buildActorContext( /** * Returns true when `actor` is allowed to READ the skill. Any grant level - * (read or read_write) confers read — a read_write grantee is always also a + * (read or write) confers read — a write grantee is always also a * reader. Public skills are readable by everyone. */ export function canReadSkill(skill: SkillOwnership, actor: ActorContext): boolean { @@ -129,22 +129,22 @@ export function canReadSkill(skill: SkillOwnership, actor: ActorContext): boolea /** * Returns true when `actor` may UPDATE the skill's content + metadata — the - * READ_WRITE tier (#1123). Author + platform admin always qualify; otherwise - * a `read_write` grant (direct, or via membership of a granted org) is + * WRITE tier (#1123). Author + platform admin always qualify; otherwise + * a `write` grant (direct, or via membership of a granted org) is * required. Deliberately NOT sufficient for admin/danger-zone ops — those * gate on `canManageSkill`. */ export function canWriteSkill(skill: SkillOwnership, actor: ActorContext): boolean { if (actor.isPlatformAdmin) return true; if (skill.createdBy === actor.userId) return true; - return actorMatchesGrant(skill, actor, (g) => g.level === "read_write"); + return actorMatchesGrant(skill, actor, (g) => g.level === "write"); } /** * Returns true when `actor` may ADMINISTER the skill — change permissions, * transfer ownership, delete skill/version, toggle deprecation, manage * dist-tags, bind a NyxID service. Author + platform admin only; a - * read_write grantee can never administer. + * write grantee can never administer. */ export function canManageSkill(skill: SkillOwnership, actor: ActorContext): boolean { if (actor.isPlatformAdmin) return true; diff --git a/ornn-api/src/domains/skills/crud/grants.migration.test.ts b/ornn-api/src/domains/skills/crud/grants.migration.test.ts index 3657edee..c1efe1d7 100644 --- a/ornn-api/src/domains/skills/crud/grants.migration.test.ts +++ b/ornn-api/src/domains/skills/crud/grants.migration.test.ts @@ -12,7 +12,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from "bun:test"; import { MongoMemoryServer } from "mongodb-memory-server"; import { MongoClient, type Db } from "mongodb"; -import { backfillTypedGrants } from "./grants.migration"; +import { backfillTypedGrants, renameReadWriteGrantsToWrite } from "./grants.migration"; let mongo: MongoMemoryServer; let client: MongoClient; @@ -92,15 +92,15 @@ describe("backfillTypedGrants", () => { isPrivate: true, sharedWithUsers: ["u1"], sharedWithOrgs: [], - grants: [{ type: "user", id: "u1", level: "read_write" }], + grants: [{ type: "user", id: "u1", level: "write" }], }); const res = await backfillTypedGrants(db); expect(res.skillsBackfilled).toBe(0); const doc = await db.collection("skills").findOne({ _id: "s4" as never }); - // read_write grant preserved — migration must not clobber a richer ACL. - expect(doc?.grants).toEqual([{ type: "user", id: "u1", level: "read_write" }]); + // write grant preserved — migration must not clobber a richer ACL. + expect(doc?.grants).toEqual([{ type: "user", id: "u1", level: "write" }]); }); it("is a no-op on a second run", async () => { @@ -131,3 +131,57 @@ describe("backfillTypedGrants", () => { expect(doc?.grants).toEqual([{ type: "org", id: "o9", level: "read" }]); }); }); + +describe("renameReadWriteGrantsToWrite (#1127)", () => { + it("renames legacy read_write grants to write, leaving read grants + other fields intact", async () => { + await db.collection("skills").insertOne({ + _id: "r1" as never, + name: "legacy-rw", + isPrivate: true, + sharedWithUsers: ["u1", "u2"], + sharedWithOrgs: ["o1"], + grants: [ + { type: "user", id: "u1", level: "read_write" }, + { type: "user", id: "u2", level: "read" }, + { type: "org", id: "o1", level: "read_write" }, + ], + }); + + const res = await renameReadWriteGrantsToWrite(db); + expect(res.skillsRenamed).toBe(1); + + const doc = await db.collection("skills").findOne({ _id: "r1" as never }); + expect(doc?.grants).toEqual([ + { type: "user", id: "u1", level: "write" }, + { type: "user", id: "u2", level: "read" }, + { type: "org", id: "o1", level: "write" }, + ]); + // Non-destructive: legacy lists + privacy untouched. + expect(doc?.sharedWithUsers).toEqual(["u1", "u2"]); + expect(doc?.isPrivate).toBe(true); + }); + + it("is idempotent — a second run renames nothing", async () => { + await db.collection("skills").insertOne({ + _id: "r2" as never, + name: "rw-once", + grants: [{ type: "user", id: "u1", level: "read_write" }], + }); + const first = await renameReadWriteGrantsToWrite(db); + expect(first.skillsRenamed).toBe(1); + const second = await renameReadWriteGrantsToWrite(db); + expect(second.skillsRenamed).toBe(0); + }); + + it("renames skillsets too", async () => { + await db.collection("skillsets").insertOne({ + _id: "rs1" as never, + name: "set-rw", + grants: [{ type: "org", id: "o9", level: "read_write" }], + }); + const res = await renameReadWriteGrantsToWrite(db); + expect(res.skillsetsRenamed).toBe(1); + const doc = await db.collection("skillsets").findOne({ _id: "rs1" as never }); + expect(doc?.grants).toEqual([{ type: "org", id: "o9", level: "write" }]); + }); +}); diff --git a/ornn-api/src/domains/skills/crud/grants.migration.ts b/ornn-api/src/domains/skills/crud/grants.migration.ts index 37c4292b..52d820f2 100644 --- a/ornn-api/src/domains/skills/crud/grants.migration.ts +++ b/ornn-api/src/domains/skills/crud/grants.migration.ts @@ -81,3 +81,57 @@ export async function backfillTypedGrants(db: Db): Promise { + const counts: Record = {}; + for (const coll of GRANTED_COLLECTIONS) { + const res = await db + .collection(coll) + .updateMany({ "grants.level": "read_write" }, [RENAME_LEVEL_STAGE]); + counts[coll] = res.modifiedCount; + } + const result: LevelRenameResult = { + skillsRenamed: counts.skills ?? 0, + skillsetsRenamed: counts.skillsets ?? 0, + }; + if (result.skillsRenamed > 0 || result.skillsetsRenamed > 0) { + logger.info({ ...result }, "read_write → write grant-level rename complete (#1127)"); + } + return result; +} diff --git a/ornn-api/src/domains/skills/crud/grants.test.ts b/ornn-api/src/domains/skills/crud/grants.test.ts index 4f6ec581..db1e7399 100644 --- a/ornn-api/src/domains/skills/crud/grants.test.ts +++ b/ornn-api/src/domains/skills/crud/grants.test.ts @@ -22,7 +22,7 @@ import { describe("effectiveGrants", () => { it("returns the explicit grants array when present", () => { - const grants: SkillGrant[] = [{ type: "user", id: "u1", level: "read_write" }]; + const grants: SkillGrant[] = [{ type: "user", id: "u1", level: "write" }]; expect(effectiveGrants({ grants })).toBe(grants); }); @@ -45,7 +45,7 @@ describe("effectiveGrants", () => { }); describe("deriveGrantsFromLegacy", () => { - it("maps users + orgs to READ grants, never read_write", () => { + it("maps users + orgs to READ grants, never write", () => { const out = deriveGrantsFromLegacy(["u1", "u2"], ["o1"]); expect(out).toEqual([ { type: "user", id: "u1", level: "read" }, @@ -66,8 +66,8 @@ describe("legacyListsFromGrants (dual-write projection)", () => { it("places every grant id in its legacy list regardless of level (read visibility, no escalation)", () => { const grants: SkillGrant[] = [ { type: "user", id: "u1", level: "read" }, - { type: "user", id: "u2", level: "read_write" }, - { type: "org", id: "o1", level: "read_write" }, + { type: "user", id: "u2", level: "write" }, + { type: "org", id: "o1", level: "write" }, ]; expect(legacyListsFromGrants(grants)).toEqual({ sharedWithUsers: ["u1", "u2"], @@ -83,29 +83,29 @@ describe("legacyListsFromGrants (dual-write projection)", () => { }); describe("normalizeGrants", () => { - it("collapses duplicate (type,id) keeping the highest level (read_write wins)", () => { + it("collapses duplicate (type,id) keeping the highest level (write wins)", () => { expect( normalizeGrants([ { type: "user", id: "u1", level: "read" }, - { type: "user", id: "u1", level: "read_write" }, + { type: "user", id: "u1", level: "write" }, ]), - ).toEqual([{ type: "user", id: "u1", level: "read_write" }]); + ).toEqual([{ type: "user", id: "u1", level: "write" }]); }); - it("does not let a later read downgrade an earlier read_write", () => { + it("does not let a later read downgrade an earlier write", () => { expect( normalizeGrants([ - { type: "org", id: "o1", level: "read_write" }, + { type: "org", id: "o1", level: "write" }, { type: "org", id: "o1", level: "read" }, ]), - ).toEqual([{ type: "org", id: "o1", level: "read_write" }]); + ).toEqual([{ type: "org", id: "o1", level: "write" }]); }); it("trims ids and drops empties, preserving first-appearance order", () => { expect( normalizeGrants([ { type: "user", id: " u2 ", level: "read" }, - { type: "user", id: "", level: "read_write" }, + { type: "user", id: "", level: "write" }, { type: "user", id: "u1", level: "read" }, ]), ).toEqual([ @@ -118,18 +118,18 @@ describe("normalizeGrants", () => { expect( normalizeGrants([ { type: "user", id: "x", level: "read" }, - { type: "org", id: "x", level: "read_write" }, + { type: "org", id: "x", level: "write" }, ]), ).toEqual([ { type: "user", id: "x", level: "read" }, - { type: "org", id: "x", level: "read_write" }, + { type: "org", id: "x", level: "write" }, ]); }); }); describe("levelAllowsWrite", () => { - it("only read_write permits writing", () => { - expect(levelAllowsWrite("read_write")).toBe(true); + it("only write permits writing", () => { + expect(levelAllowsWrite("write")).toBe(true); expect(levelAllowsWrite("read")).toBe(false); }); }); @@ -145,11 +145,11 @@ describe("coerceStoredGrants", () => { expect( coerceStoredGrants([ { type: "user", id: " u1 ", level: "read" }, - { type: "org", id: "o1", level: "read_write" }, + { type: "org", id: "o1", level: "write" }, ]), ).toEqual([ { type: "user", id: "u1", level: "read" }, - { type: "org", id: "o1", level: "read_write" }, + { type: "org", id: "o1", level: "write" }, ]); }); @@ -161,12 +161,24 @@ describe("coerceStoredGrants", () => { { type: "user", id: "u1", level: "admin" }, 42, null, - { type: "user", id: "u2", level: "read_write" }, + { type: "user", id: "u2", level: "write" }, ]), - ).toEqual([{ type: "user", id: "u2", level: "read_write" }]); + ).toEqual([{ type: "user", id: "u2", level: "write" }]); }); it("returns an empty array for an empty stored array (explicit no-grants)", () => { expect(coerceStoredGrants([])).toEqual([]); }); + + it("coerces a stored legacy read_write level → write (#1127, no drop)", () => { + expect( + coerceStoredGrants([ + { type: "user", id: "u1", level: "read_write" }, + { type: "org", id: "o1", level: "read" }, + ]), + ).toEqual([ + { type: "user", id: "u1", level: "write" }, + { type: "org", id: "o1", level: "read" }, + ]); + }); }); diff --git a/ornn-api/src/domains/skills/crud/grants.ts b/ornn-api/src/domains/skills/crud/grants.ts index 4743853e..3bcb66d2 100644 --- a/ornn-api/src/domains/skills/crud/grants.ts +++ b/ornn-api/src/domains/skills/crud/grants.ts @@ -31,7 +31,7 @@ import type { export const skillGrantSchema = z.object({ type: z.enum(["user", "org"]), id: z.string().min(1).max(128), - level: z.enum(["read", "read_write"]), + level: z.enum(["read", "write"]), }); /** @@ -88,7 +88,7 @@ export function effectiveGrants(src: GrantSource): SkillGrant[] { /** * Map the legacy read-only allow-lists onto READ-level typed grants. Used by * the boot migration and by the read-time fallback. Never produces a - * `read_write` grant — legacy sharing was read-only, so migrating it must + * `write` grant — legacy sharing was read-only, so migrating it must * not escalate anyone. */ export function deriveGrantsFromLegacy( @@ -106,7 +106,7 @@ export function deriveGrantsFromLegacy( * dual-write (#1123). EVERY grant id — regardless of level — lands in the * matching legacy list so code that only understands the read lists (an * older pod mid-rolling-deploy) still resolves correct READ visibility for - * read_write grantees, and never escalates anyone to write (old code has no + * write grantees, and never escalates anyone to write (old code has no * write concept). Dropped by the post-rollout cleanup once all pods read * `grants`. */ @@ -122,7 +122,7 @@ export function legacyListsFromGrants(grants: readonly SkillGrant[]): LegacyShar /** * Normalize an incoming grants list: trim ids, drop empties, and collapse - * duplicate `(type, id)` pairs keeping the HIGHEST level (`read_write` wins + * duplicate `(type, id)` pairs keeping the HIGHEST level (`write` wins * over `read`) so a principal can never hold two conflicting grants on the * same skill. Order is preserved by first appearance. */ @@ -136,7 +136,7 @@ export function normalizeGrants(grants: readonly SkillGrant[]): SkillGrant[] { const prev = byKey.get(key); if (!prev) order.push(key); const level: SkillPermissionLevel = - prev?.level === "read_write" || g.level === "read_write" ? "read_write" : "read"; + prev?.level === "write" || g.level === "write" ? "write" : "read"; byKey.set(key, { type: g.type, id, level }); } return order.map((k) => byKey.get(k)!); @@ -144,7 +144,7 @@ export function normalizeGrants(grants: readonly SkillGrant[]): SkillGrant[] { /** True when `level` permits writing (updating content / metadata). */ export function levelAllowsWrite(level: SkillPermissionLevel): boolean { - return level === "read_write"; + return level === "write"; } /** @@ -162,9 +162,13 @@ export function coerceStoredGrants(raw: unknown): SkillGrant[] | undefined { const e = entry as Record; const type = e.type; const id = typeof e.id === "string" ? e.id.trim() : ""; - const level = e.level; + // #1127 renamed the level `read_write` → `write`. Defensively coerce a + // stored legacy `read_write` here so a doc the boot migration hasn't + // rewritten yet (or one written by an older pod mid-rolling-deploy) still + // resolves to write — never dropped, never silently downgraded. + const level = e.level === "read_write" ? "write" : e.level; if ((type !== "user" && type !== "org") || !id) continue; - if (level !== "read" && level !== "read_write") continue; + if (level !== "read" && level !== "write") continue; out.push({ type, id, level }); } return out; diff --git a/ornn-api/src/domains/skills/crud/routes.test.ts b/ornn-api/src/domains/skills/crud/routes.test.ts index 8774c9fa..2f2b62e0 100644 --- a/ornn-api/src/domains/skills/crud/routes.test.ts +++ b/ornn-api/src/domains/skills/crud/routes.test.ts @@ -916,7 +916,7 @@ describe("PUT /skills/:id", () => { expect(calls).toEqual(["updateSkill"]); }); - test("200 ZIP republish for a read_write grantee — content edit is the write tier (#1123)", async () => { + test("200 ZIP republish for a write grantee — content edit is the write tier (#1123)", async () => { const calls: string[] = []; const app = buildApp({ userId: "editor", @@ -926,7 +926,7 @@ describe("PUT /skills/:id", () => { skillDoc({ createdBy: OWNER, isPrivate: true, - grants: [{ type: "user", id: "editor", level: "read_write" }], + grants: [{ type: "user", id: "editor", level: "write" }], }), }, service: { @@ -945,7 +945,7 @@ describe("PUT /skills/:id", () => { expect(calls).toEqual(["updateSkill"]); }); - test("403 when a read_write grantee tries to flip visibility — that is admin-only (#1123)", async () => { + test("403 when a write grantee tries to flip visibility — that is admin-only (#1123)", async () => { const app = buildApp({ userId: "editor", permissions: [UPDATE], @@ -954,7 +954,7 @@ describe("PUT /skills/:id", () => { skillDoc({ createdBy: OWNER, isPrivate: true, - grants: [{ type: "user", id: "editor", level: "read_write" }], + grants: [{ type: "user", id: "editor", level: "write" }], }), }, }); diff --git a/ornn-api/src/domains/skills/crud/routes.ts b/ornn-api/src/domains/skills/crud/routes.ts index 517991d2..4ae6c26f 100644 --- a/ornn-api/src/domains/skills/crud/routes.ts +++ b/ornn-api/src/domains/skills/crud/routes.ts @@ -999,8 +999,8 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } const actor = await buildActorContext(c); - // Updating content + metadata is the READ_WRITE tier (#1123): author, - // platform admin, or a read_write grantee. Changing `isPrivate` is a + // Updating content + metadata is the WRITE tier (#1123): author, + // platform admin, or a write grantee. Changing `isPrivate` is a // permission change (ADMIN tier) — gated separately below once parsed. if (!canWriteSkill(existing, actor)) { throw AppError.forbidden( @@ -1065,7 +1065,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.badRequest("no_update", "No update data provided. Send a ZIP file and/or isPrivate field."); } - // Visibility is an ADMIN-tier change (#1123): a read_write grantee may + // Visibility is an ADMIN-tier change (#1123): a write grantee may // replace content but must not flip public/private. Enforce only when // `isPrivate` is actually being changed so a no-op resend by an editor // doesn't 403. @@ -1160,7 +1160,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: const updated = await skillService.getSkill(guid); - const readWriteGrants = (updated.grants ?? []).filter((g) => g.level === "read_write").length; + const writeGrants = (updated.grants ?? []).filter((g) => g.level === "write").length; trackActivity(authCtx.userId, authCtx.email, authCtx.displayName, "skill.permissions_changed", { skillId: guid, skillName: updated.name, @@ -1168,7 +1168,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: sharedWithUsers: updated.sharedWithUsers.length, sharedWithOrgs: updated.sharedWithOrgs.length, // #1123 — richer payload: how many grants confer write. - readWriteGrants, + writeGrants, }); // Permissions change can flip eligibility — sync handles both @@ -1204,7 +1204,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } const actor = await buildActorContext(c); - // ADMIN tier — transfer is a danger-zone op, never a read_write grant. + // ADMIN tier — transfer is a danger-zone op, never a write grant. if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", diff --git a/ornn-api/src/domains/skills/crud/service.test.ts b/ornn-api/src/domains/skills/crud/service.test.ts index 59de2068..ef3cf420 100644 --- a/ornn-api/src/domains/skills/crud/service.test.ts +++ b/ornn-api/src/domains/skills/crud/service.test.ts @@ -478,7 +478,7 @@ describe("SkillService.setSkillPermissions — org-membership gate (#815)", () = expect(state.updateCalled).toBe(false); }); - it("(e) accepts typed grants directly, including read_write (#1123)", async () => { + it("(e) accepts typed grants directly, including write (#1123)", async () => { const { service, state } = makePermissionsService( makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), ); @@ -494,18 +494,18 @@ describe("SkillService.setSkillPermissions — org-membership gate (#815)", () = { isPrivate: true, grants: [ - { type: "user", id: "editor", level: "read_write" }, - { type: "org", id: "org-A", level: "read_write" }, + { type: "user", id: "editor", level: "write" }, + { type: "org", id: "org-A", level: "write" }, ], }, actor, ); expect(state.updateCalled).toBe(true); - expect(result.grants).toContainEqual({ type: "user", id: "editor", level: "read_write" }); - expect(result.grants).toContainEqual({ type: "org", id: "org-A", level: "read_write" }); + expect(result.grants).toContainEqual({ type: "user", id: "editor", level: "write" }); + expect(result.grants).toContainEqual({ type: "org", id: "org-A", level: "write" }); }); - it("(f) a read_write org grant still honours the #815 non-member gate (#1123)", async () => { + it("(f) a write org grant still honours the #815 non-member gate (#1123)", async () => { const { service, state } = makePermissionsService( makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), ); @@ -520,7 +520,7 @@ describe("SkillService.setSkillPermissions — org-membership gate (#815)", () = await service.setSkillPermissions( "guid-1", "owner-1", - { isPrivate: true, grants: [{ type: "org", id: "org-Z", level: "read_write" }] }, + { isPrivate: true, grants: [{ type: "org", id: "org-Z", level: "write" }] }, actor, ); } catch (err) { @@ -543,7 +543,7 @@ describe("SkillService.setSkillPermissions — org-membership gate (#815)", () = const result = await service.setSkillPermissions( "guid-1", "owner-1", - { isPrivate: true, grants: [{ type: "user", id: "owner-1", level: "read_write" }] }, + { isPrivate: true, grants: [{ type: "user", id: "owner-1", level: "write" }] }, actor, ); // The author holds implicit ADMIN — a self-grant is redundant and dropped. @@ -604,7 +604,7 @@ describe("SkillService.transferSkillOwnership (#1123)", () => { guid: "guid-1", createdBy: "owner-1", isPrivate: true, - grants: [{ type: "org", id: "org-A", level: "read_write" }], + grants: [{ type: "org", id: "org-A", level: "write" }], }), ); const result = await service.transferSkillOwnership( @@ -616,7 +616,7 @@ describe("SkillService.transferSkillOwnership (#1123)", () => { expect(result.createdBy).toBe("newbie"); expect(result.createdByEmail).toBe("new@x.io"); // Existing org grant preserved; prior owner added as read. - expect(result.grants).toContainEqual({ type: "org", id: "org-A", level: "read_write" }); + expect(result.grants).toContainEqual({ type: "org", id: "org-A", level: "write" }); expect(result.grants).toContainEqual({ type: "user", id: "owner-1", level: "read" }); }); @@ -626,7 +626,7 @@ describe("SkillService.transferSkillOwnership (#1123)", () => { guid: "guid-1", createdBy: "owner-1", isPrivate: true, - grants: [{ type: "user", id: "newbie", level: "read_write" }], + grants: [{ type: "user", id: "newbie", level: "write" }], }), ); const result = await service.transferSkillOwnership( diff --git a/ornn-api/src/domains/skillsets/authorize.ts b/ornn-api/src/domains/skillsets/authorize.ts index 0e9d4c09..d4333c82 100644 --- a/ornn-api/src/domains/skillsets/authorize.ts +++ b/ornn-api/src/domains/skillsets/authorize.ts @@ -38,7 +38,7 @@ export function canReadSkillset( /** * True when `actor` may UPDATE the skillset's content/metadata (publish a new - * version) — the READ_WRITE tier (#1123). Delegates to the skill gate. + * version) — the WRITE tier (#1123). Delegates to the skill gate. */ export function canWriteSkillset( skillset: SkillsetOwnership, diff --git a/ornn-api/src/domains/skillsets/service.ts b/ornn-api/src/domains/skillsets/service.ts index 23ec65f7..9e9255b3 100644 --- a/ornn-api/src/domains/skillsets/service.ts +++ b/ornn-api/src/domains/skillsets/service.ts @@ -177,7 +177,7 @@ export class SkillsetService { if (!existing) { throw AppError.notFound("skillset_not_found", `Skillset '${guid}' not found`); } - // Publishing a new version is a content/metadata edit — the READ_WRITE + // Publishing a new version is a content/metadata edit — the WRITE // tier (#1123). Permissions/transfer/delete remain ADMIN-only below. if (!canWriteSkill(existing, actor)) { throw AppError.forbidden("forbidden", "You do not have permission to update this skillset"); diff --git a/ornn-api/src/domains/skillsets/types.ts b/ornn-api/src/domains/skillsets/types.ts index 3075c11d..f2d9b0d0 100644 --- a/ornn-api/src/domains/skillsets/types.ts +++ b/ornn-api/src/domains/skillsets/types.ts @@ -211,7 +211,7 @@ export interface SkillsetDocument { /** Explicit per-org grants (NyxID org user_ids). */ sharedWithOrgs: string[]; /** - * Typed access grants (#1123) — canonical read/read-write ACL, mirroring + * Typed access grants (#1123) — canonical read/write ACL, mirroring * `SkillDocument.grants`. Optional for back-compat; readers fall back to * deriving read grants from the legacy lists via `effectiveGrants`. */ diff --git a/ornn-api/src/shared/types/index.ts b/ornn-api/src/shared/types/index.ts index 98f8093e..2943fba5 100644 --- a/ornn-api/src/shared/types/index.ts +++ b/ornn-api/src/shared/types/index.ts @@ -74,13 +74,13 @@ export class AppError extends Error { /** * Permission level a grant confers on a skill / skillset (#1123). * - `read` → view / pull / execute / read versions. - * - `read_write` → READ plus update the skill's content & metadata. Does + * - `write` → READ plus update the skill's content & metadata. Does * NOT include any admin / danger-zone operation (change permissions, * transfer ownership, delete skill/version, toggle deprecation, manage * dist-tags, bind NyxID service) — those stay with the owner (`createdBy`) * and platform admins only. */ -export type SkillPermissionLevel = "read" | "read_write"; +export type SkillPermissionLevel = "read" | "write"; /** The kind of principal a grant targets. */ export type SkillGrantPrincipalType = "user" | "org"; @@ -92,11 +92,11 @@ export type SkillGrantPrincipalType = "user" | "org"; * * - `type` — `user` (NyxID person user_id) or `org` (NyxID org user_id). * - `id` — the principal's NyxID id. - * - `level`— `read` or `read_write`. + * - `level`— `read` or `write`. * * The author (`createdBy`) is never represented here — they hold implicit * ADMIN. Public skills (`isPrivate === false`) are readable by everyone - * regardless of grants; `read_write` only ever applies to explicit grants. + * regardless of grants; `write` only ever applies to explicit grants. */ export interface SkillGrant { type: SkillGrantPrincipalType; @@ -148,7 +148,7 @@ export interface SkillDocument { sharedWithOrgs: string[]; /** * Typed access grants (#1123) — the canonical source of truth for who can - * read vs. read-write the skill, beyond the author + platform admin. + * read vs. write the skill, beyond the author + platform admin. * * Optional for back-compat: skills created before #1123 carry only the * legacy `sharedWithUsers` / `sharedWithOrgs` read lists. Readers MUST fall @@ -373,7 +373,7 @@ export interface SkillDetailResponse { /** Org user_ids granted access — every admin/member of these orgs can read the skill. */ sharedWithOrgs: string[]; /** - * Typed access grants (#1123) — the canonical read/read-write ACL. Always + * Typed access grants (#1123) — the canonical read/write ACL. Always * present in responses (the service emits `effectiveGrants(skill)` so even * an un-migrated skill surfaces its legacy lists as read grants here). */ diff --git a/ornn-web/src/components/permissions/PermissionsEditor.tsx b/ornn-web/src/components/permissions/PermissionsEditor.tsx index eb847cb8..947d9e53 100644 --- a/ornn-web/src/components/permissions/PermissionsEditor.tsx +++ b/ornn-web/src/components/permissions/PermissionsEditor.tsx @@ -44,14 +44,14 @@ const key = (p: { type: string; id: string }) => `${p.type}:${p.id}`; /** * Compose the canonical grants from the two audiences. When public, read * grants are redundant (everyone reads) so only write grants are emitted. - * A principal in both audiences resolves to read_write (write wins). + * A principal in both audiences resolves to write (write wins). */ function buildGrants(read: Principal[], write: Principal[], isPublic: boolean): SkillGrant[] { const map = new Map(); if (!isPublic) { for (const p of read) map.set(key(p), { type: p.type, id: p.id, level: "read" }); } - for (const p of write) map.set(key(p), { type: p.type, id: p.id, level: "read_write" }); + for (const p of write) map.set(key(p), { type: p.type, id: p.id, level: "write" }); return [...map.values()]; } @@ -78,7 +78,7 @@ export function PermissionsEditor({ const [isPublic, setIsPublic] = useState(!initialIsPrivate); const [readGrantees, setReadGrantees] = useState(() => toPrincipals(initialGrants, "read")); const [writeGrantees, setWriteGrantees] = useState(() => - toPrincipals(initialGrants, "read_write"), + toPrincipals(initialGrants, "write"), ); // Resolve user-grant labels once on mount (the typeahead supplies labels @@ -150,7 +150,7 @@ export function PermissionsEditor({ const grants = buildGrants(readGrantees, writeGrantees, isPublic); const initialBuilt = buildGrants( toPrincipals(initialGrants, "read"), - toPrincipals(initialGrants, "read_write"), + toPrincipals(initialGrants, "write"), !initialIsPrivate, ); if (isPrivate === initialIsPrivate && signature(grants) === signature(initialBuilt)) { diff --git a/ornn-web/src/components/skill/PermissionsModal.test.tsx b/ornn-web/src/components/skill/PermissionsModal.test.tsx index 6b8a173c..96ad3e86 100644 --- a/ornn-web/src/components/skill/PermissionsModal.test.tsx +++ b/ornn-web/src/components/skill/PermissionsModal.test.tsx @@ -107,17 +107,17 @@ describe("PermissionsModal — two-tab Read/Write editor (#1125)", () => { fireEvent.click(screen.getByRole("button", { name: /write access/i })); fireEvent.click(screen.getByRole("checkbox", { name: "Org One" })); - // Save → public (isPrivate:false) + a read_write org grant; no read grant + // Save → public (isPrivate:false) + a write org grant; no read grant // (public makes read grants redundant, so they're dropped). fireEvent.click(screen.getByRole("button", { name: /^save$/i })); expect(mutateAsync).toHaveBeenCalledWith({ isPrivate: false, - grants: [{ type: "org", id: "org-1", level: "read_write" }], + grants: [{ type: "org", id: "org-1", level: "write" }], }); }); - it("seeds the Write tab from an existing read_write grant", () => { - renderModal(skill({ grants: [{ type: "org", id: "org-1", level: "read_write" }] })); + it("seeds the Write tab from an existing write grant", () => { + renderModal(skill({ grants: [{ type: "org", id: "org-1", level: "write" }] })); // The Write tab shows a count badge of 1. const writeTab = screen.getByRole("button", { name: /write access/i }); expect(writeTab.textContent).toContain("1"); diff --git a/ornn-web/src/components/skill/SkillHeroStrip.tsx b/ornn-web/src/components/skill/SkillHeroStrip.tsx index 26dd5a73..b87c86c4 100644 --- a/ornn-web/src/components/skill/SkillHeroStrip.tsx +++ b/ornn-web/src/components/skill/SkillHeroStrip.tsx @@ -25,7 +25,6 @@ interface SkillHeroStripProps { pullCount7d?: number | undefined; versionAudit?: AuditRecord | undefined; isAuthenticated: boolean; - isOwner: boolean; ownerDisplayName: string; ownerAvatarUrl?: string | null | undefined; onTryPlayground: () => void; @@ -87,7 +86,6 @@ export function SkillHeroStrip({ pullCount7d, versionAudit, isAuthenticated, - isOwner, ownerDisplayName, ownerAvatarUrl, onTryPlayground, @@ -203,7 +201,7 @@ export function SkillHeroStrip({ replaced the older three-dots menu (#411). Each renders independently when its handler is provided, so the row degrades gracefully (e.g. no edit icon for non-owners). */} - {isOwner && onEditSkill && ( + {onEditSkill && ( + )} - {/* Upload new package */} + {/* Update Package — WRITE tier (owner / admin / write-grantee). */} + {canWrite && (

{t("editSkill.updatePackageHeading", "Update Package")} @@ -220,6 +239,7 @@ export function EditSkillPage() { )} + )} diff --git a/ornn-web/src/pages/skill/SkillDetailPage.tsx b/ornn-web/src/pages/skill/SkillDetailPage.tsx index cce64767..ca2ab1fb 100644 --- a/ornn-web/src/pages/skill/SkillDetailPage.tsx +++ b/ornn-web/src/pages/skill/SkillDetailPage.tsx @@ -75,6 +75,7 @@ export function SkillDetailPage() { isAuthenticated, isOwner, isAdminUser, + canWrite, canManageVersions, auditSummaryByVersion, versionAudit, @@ -167,12 +168,12 @@ export function SkillDetailPage() { pullCount7d={pullCount7d} versionAudit={versionAudit} isAuthenticated={isAuthenticated} - isOwner={isOwner} ownerDisplayName={ownerDisplayName} ownerAvatarUrl={ownerAvatarUrl} onTryPlayground={() => navigate(`/playground?skill=${encodeURIComponent(skill.name)}`)} onDownloadPackage={rawZip ? handleDownloadPackage : undefined} - onEditSkill={isOwner ? () => navigate(`/skills/${skill.guid}/edit`) : undefined} + // Content edit is the write tier (#1127): owner, admin, or write-grantee. + onEditSkill={canWrite ? () => navigate(`/skills/${skill.guid}/edit`) : undefined} /> {/* ── Audit-version banner (yellow/red/missing only; green is silent) ── */} @@ -249,7 +250,7 @@ export function SkillDetailPage() { /> )} - {isOwner && ( + {canWrite && (