feat: skill permission levels (read / read-write) + ownership transfer (#1123)#1124
Merged
Merged
Conversation
Introduce the canonical typed access-grant shape that will replace the
legacy read-only `sharedWithUsers` / `sharedWithOrgs` allow-lists: a
`SkillGrant { type, id, level }` pairing a user/org principal with a
`read` or `read_write` level, plus a pure, reusable `grants.ts` helper
module (effective-grant resolution, legacy derive/project, normalize).
The field is added to `SkillDocument` as OPTIONAL on purpose. Skills
predating #1123 carry only the legacy lists, and `effectiveGrants`
derives identical READ-level grants from them when `grants` is absent —
so this commit changes no behaviour and cannot disrupt existing skills.
`legacyListsFromGrants` is the inverse projection used later for the
transitional dual-write that keeps a rolling deploy non-disruptive.
Pure machinery, no wiring yet (authz/scopeFilter/routes follow). Unit
tests pin the grant algebra, especially the legacy round-trip and the
read_write-wins normalization.
Part of #1123
Wire the `grants` field through the skills repository and the detail response: - `create` seeds an empty `grants` array (new skills are born migrated) and keeps the legacy lists in lock-step. - `update` treats `grants` as the canonical ACL write: when provided it dual-writes the legacy `sharedWithUsers` / `sharedWithOrgs` lists via `legacyListsFromGrants` (every grant id → read visibility) so an older pod mid-rolling-deploy still resolves correct read access and nobody is escalated to write. Legacy `sharedWith*` writes only apply when `grants` is absent. - `mapDoc` surfaces stored grants with defensive coercion (drops malformed entries) and leaves the field undefined for un-migrated docs. - the skill detail response now carries `grants`, emitted via `effectiveGrants` so the shape is identical whether or not the skill has been migrated. No authorization behaviour changes yet — `grants` is only read back, the gates still use the legacy lists. Existing CRUD suite (342 tests) stays green. Part of #1123
Mirror the skills grant-persistence onto the skillsets domain, which shares the ownership model verbatim: - `SkillsetDocument` + `SkillsetDetailResponse` gain the optional `grants` field; the detail builder emits `effectiveGrants(skillset)`. - the skillset repository create/update/mapDoc handle `grants` with the same canonical-write + transitional dual-write of the legacy lists. Promote the defensive stored-grant coercion into the shared `grants.ts` as `coerceStoredGrants` and use it from both repositories, removing the duplicate that briefly lived in the skills repo. Unit tests cover the malformed-entry drops and the un-migrated/empty cases. Still no authz behaviour change — grants are only read back. Part of #1123
Backfill the typed `grants` array on every skill / skillset that predates it, deriving one `read`-level grant per legacy `sharedWithUsers` / `sharedWithOrgs` entry. Runs at boot right after the model-catalog migration, before any skill read is served. Non-disruptive by construction — the hard requirement for this feature: the legacy lists are left untouched, public/private flags are untouched, and nobody is escalated to write (every backfilled grant is `read`). Idempotent: only docs missing `grants` are matched, so reruns are no-ops; it runs server-side as one updateMany + aggregation pipeline per collection. Failure is logged non-fatally because `effectiveGrants` still falls back to the legacy lists for any un-migrated doc. Integration tests run the migration against a real Mongo and pin the non-disruption invariant, the empty/ancient-doc cases, idempotency, and that an existing read_write grant is never clobbered. Part of #1123
Make the single source-of-truth `authorize.ts` (shared by skills + skillsets) grant-aware: - `canReadSkill` now resolves access from the typed `grants` ACL — any level confers read, directly or via membership of a granted org. It falls back to `effectiveGrants` so an un-migrated doc reads exactly as before off its legacy lists. - new `canWriteSkill` = author OR platform admin OR a `read_write` grant (direct or via a granted org) — the READ_WRITE tier (content + metadata edits only). - `canManageSkill` is unchanged (author OR platform admin) = the ADMIN tier; a read_write grantee is deliberately never an admin. A shared `actorMatchesGrant` core walks the effective grants once so the read and write gates can never diverge on the matching rules; both fail soft on an unresolved org-membership lookup, matching the existing read behaviour. `SkillOwnership` gains the optional `grants` field. No route yet calls `canWriteSkill` — that re-pointing lands next. Full skills + skillsets suites (758 tests) stay green. Part of #1123
Re-point the content/metadata update paths at the new write gate so a read_write grantee can update a skill without being its owner: - `PUT /skills/:id`: base gate becomes `canWriteSkill`; a request that actually changes `isPrivate` additionally requires `canManageSkill`, so flipping visibility stays an ADMIN-tier action (a read_write grantee can replace content but not toggle public/private). - skillset `publishVersion` gates on `canWriteSkill` (publishing a new version is a content edit); permissions/transfer/delete stay ADMIN-only. - add `canWriteSkillset` to the skillset authorize delegation + `grants` to `SkillsetOwnership`. Route tests cover the new matrix: read_write grantee republishes content (200), read_write grantee flipping visibility (403), read-only grantee republishing content (403). Part of #1123
Extend `PUT /skills/:id/permissions` and `PUT /skillsets/:id/permissions` to carry the canonical typed `grants` ACL, while still accepting the legacy `sharedWithUsers` / `sharedWithOrgs` arrays for backward-compatible callers (they map to READ-level grants). - add the shared `skillGrantSchema` + a `resolvePermissionGrants` helper to `grants.ts` (prefers typed grants, falls back to the legacy lists). - both permission setters now resolve to normalized grants, drop a grant that names the author (implicit ADMIN), and reuse the existing #815/#842 org-membership gate over the resolved org grants — so sharing a read_write grant into a non-member org is rejected exactly like a read share, and an unresolved lookup still 503s. - `skill.permissions_changed` analytics gains a `readWriteGrants` count. The service methods are the back-compat boundary, so the existing org-gate tests pass unchanged; assertions that checked the dual-written legacy lists now assert on the canonical `grants`. New tests cover the typed-grants path (read_write user + org), the non-member gate over an org grant, and author-self-grant pruning. Part of #1123
Add owner-to-owner skill transfer — the first path that mutates the otherwise-immutable `createdBy`. - repository `transferOwnership`: the single explicit `createdBy` write, refreshing the cached owner labels and replacing the ACL (dual-writing legacy lists), kept separate from `update` so the immutability invariant stays obvious. - service `transferSkillOwnership`: rejects a no-op transfer to the current owner (`ownership_conflict`, 409), and recomputes grants so the prior owner is appended as a READ grantee (keeps visibility, loses edit/admin) while the new owner is dropped from any prior grant (they hold implicit ADMIN now). - route `POST /skills/:id/transfer-ownership`: ADMIN-gated (`canManageSkill` — never a read_write grant); validates the target against the injected user directory (`invalid_transfer_target`, 400, when unknown); emits the new `skill.ownership_transferred` analytics event; resyncs the mirror to refresh stale author labels. Immediate/unilateral per the agreed design. - bootstrap wires `resolveUser` from the user directory repo. Service tests cover label refresh + prior-owner read grant + new-owner drop + ownership_conflict; route tests cover the 403/404/409/400 matrix, the owner happy path, and platform-admin force-transfer. Part of #1123
Mirror skill transfer onto skillsets. Because the skillset routes delegate authorization to the service (the route layer has no repo), the service owns the whole flow — ADMIN gate (`canManageSkill`), no-op rejection (`ownership_conflict`), AND target resolution against the injected user directory. Resolving inside the service (rather than the route) means a non-owner can't enumerate users via the 400-vs-403 difference. The repository gets the same dedicated `transferOwnership` write (the one explicit `createdBy` mutation), refreshing owner labels + replacing the ACL with the prior owner kept as a READ grantee. `resolveUser` is injected into `SkillsetService` via `wireSkillsets`, backed by the same user-directory repo as the skills path. Service tests cover owner reassignment + prior-owner read grant, the non-owner 403, the no-op 409, and the unresolvable-target 400; route tests cover the permission gate, delegation, and body validation. Part of #1123
Minor bump for ornn-api + ornn-web (fixed-version pair). The SDKs release on a separate cadence and are not part of the changeset set. Part of #1123
Frontend plumbing for the new API surface, no UI yet: - `SkillPermissionLevel` + `SkillGrant` domain types; `grants?` added to `SkillDetail`. - `permissionsApi`: `SkillPermissionsInput.grants` (legacy `sharedWith*` arrays kept optional for back-compat) + a `transferSkillOwnership` POST. - `useTransferSkillOwnership(skillGuid, idOrName)` mutation following the established #750 two-id split + invalidation (detail, lists, my-skills, shared-with-me) so the new owner redraws and the owner-only UI drops after a transfer. Part of #1123
Add a "Transfer ownership" action above Delete in the owner-only Danger Zone. `TransferOwnershipModal` reuses the directory email-typeahead (the same `searchUsersByEmail` source the PermissionsModal uses, NOT the admin-only picker) for a single-target select, then requires the owner to type the skill name before the danger-styled Transfer button enables. On success the caller is no longer the owner, so the existing list/detail invalidation in `useTransferSkillOwnership` refetches the detail — the new owner renders and the owner-only Danger Zone drops away. Modal state lives in `useSkillDetail` alongside the other modal flags. Tests cover the two-step gate (target + exact name) and that confirming fires the mutation with the selected user id. Part of #1123
#1123) The PermissionsModal now drives the typed `grants` ACL instead of bare id lists: - each granted user (chip) and checked org (row) carries a compact read / read-write toggle (`LevelToggle`); new grants default to read. - form state initializes per-grant levels from `skill.grants` (falling back to read-level from the legacy lists), the resolve-labels effect preserves the level, and Save builds + sends typed `grants` with level-aware change detection (a level flip alone now counts as a change). - unresolved (ghost) user grants hide the toggle — you can only revoke them. - `SkillVisibilityCard` gains a "N can edit" line counting read-write grants. A focused test covers init-from-grants, the read-write→read flip, and that Save sends the canonical typed-grants payload. Full web suite stays green. Part of #1123
Expose the new permission surface in @chronoai/ornn-sdk: - `SkillPermissionLevel` + `SkillGrant` types; `grants` on SkillDetail / SkillsetDetail; a shared `PermissionsInput` (carries `grants`, legacy `sharedWith*` still accepted) re-exported as the skill/skillset aliases. - `setSkillPermissions(id, …)` — the SDK had no skill permissions setter before (only skillsets); this fills the gap. - `transferSkillOwnership(id, newOwnerUserId)` and `transferSkillsetOwnership(id, newOwnerUserId)`. - `setSkillsetPermissions` now accepts `grants`. Transport plumbing (envelope/error/zip helpers) extracted into a new `http.ts` so `client.ts` stays under the 500-line cap after the additions. 8 new tests (transfer success + 403, grants setters + 400 validation); 38 pass; tsc + lint clean. Part of #1123
Mirror the TypeScript SDK additions in the Python client: - `SkillPermissionLevel` literal + `SkillGrant` dataclass (with from_dict/to_json); optional `grants` parsed onto SkillDetail / SkillsetDetail (defaults to [] on pre-#1123 responses). - `set_skill_permissions(...)` — NEW (no skill permissions setter existed before); `set_skillset_permissions(...)` gains a `grants` kwarg; a shared `_permissions_payload` builds both bodies and omits `grants` from the wire when not supplied (so the server's legacy fallback holds). - `transfer_skill_ownership(id, new_owner_user_id)` and `transfer_skillset_ownership(id, new_owner_user_id)`. 45 pytest pass (py3.9 + py3.12); ruff + strict mypy clean. Part of #1123
- CONVENTIONS §5.2: reconcile the documented↔code drift — replace the
fictional `ornn:skill:read/write/admin` catalog with the real route
scopes (`ornn:skill:{create,read,update,delete}`, `ornn:skill:build`,
`ornn:playground:use`, `ornn:admin:skill`), and frame them as route
scopes distinct from object tiers. New §5.4 documents the
READ / READ_WRITE / ADMIN object-permission matrix, the typed `grants`
ACL shape, and both transfer-ownership endpoints (with the
invalid_transfer_target / ownership_conflict behaviors; rides on
ornn:skill:update, no new scope). §2.6 + §12 updated.
- ERRORS: add `invalid_transfer_target` (400), `ownership_conflict` (409),
`invalid_permission_level` (400) with client-action guidance + migration
map rows.
- ARCHITECTURE: add the `skill.ownership_transferred` analytics event and
the `readWriteGrants` count on `skill.permissions_changed`.
Part of #1123
…1123) The KB digest is built from README/CONVENTIONS/ARCHITECTURE etc.; the §5.2 scope reconciliation + §5.4 permission-tier additions changed the CONVENTIONS source, so the committed digest went stale and the assistant-kb-freshness CI gate failed. Regenerated via `bun run build:assistant-kb`. Part of #1123
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds skill & skillset permission levels (read / read-write) and owner-to-owner ownership transfer, end to end: API + both SDKs + web UI + docs.
Closes #1123
What changed
Three-tier object-permission model (skills + skillsets), one source of truth in
authorize.ts:canReadSkillcanWriteSkill(new)read_writegranteecanManageSkillgrantsACL{ type, id, level }is the canonical sharing primitive across DB, API, and both SDKs. LegacysharedWithUsers/sharedWithOrgspayloads are still accepted (→ read-level) for back-compat.POST /skills(/sets)/:id/transfer-ownership { newOwnerUserId }. Immediate; ADMIN-gated; target validated against the user directory (invalid_transfer_target/ownership_conflict); prior owner kept as a read grantee; platform admins can force-transfer.grants, a skill permissions setter (neither had one), and the transfer methods.Non-disruption (load-bearing)
read-level grants; never emitsread_write, never touchesisPrivate, never removes the legacy arrays.grants, so a rolling deploy and any un-migrated doc stay correct;effectiveGrantsprovides a read-time fallback.canWriteSkill≡ the oldcanManageSkill(noread_writegrants exist), so write/admin behavior is unchanged until an owner opts in.grantsis purely additive; the old image ignores it and reads the untouched legacy lists.Deliberate scope call
scopeFilter/ grants-summary Mongo queries were not rewritten — the dual-written legacy lists remain an exact read-visibility index, so they're correct unchanged (this removes the riskiest read-path change). The query rewrite + dropping the legacy fields is a follow-up tied to full rollout (tracked in #1123's checklist).Verification
🤖 Generated with Claude Code