fix: harden skill owner migration
Some checks failed
CI / static (push) Has been cancelled
CI / unit (push) Has been cancelled
CI / packages (push) Has been cancelled
CI / types-build (push) Has been cancelled
CI / e2e-http (push) Has been cancelled
CI / playwright-smoke (push) Has been cancelled
OpenClaw Docs Sync Dispatch / dispatch-openclaw-docs-sync (push) Has been cancelled
Security Gate: Secret Scanning / Scan for Verified Secrets (push) Has been cancelled
Some checks failed
CI / static (push) Has been cancelled
CI / unit (push) Has been cancelled
CI / packages (push) Has been cancelled
CI / types-build (push) Has been cancelled
CI / e2e-http (push) Has been cancelled
CI / playwright-smoke (push) Has been cancelled
OpenClaw Docs Sync Dispatch / dispatch-openclaw-docs-sync (push) Has been cancelled
Security Gate: Secret Scanning / Scan for Verified Secrets (push) Has been cancelled
Harden skill owner migration across UI, CLI, API, stats, backups, and docs. Co-authored-by: momothemage <niuzhengnan@163.com>
This commit is contained in:
parent
38c2134590
commit
5b63d5df60
@ -5,8 +5,9 @@
|
||||
### Changes
|
||||
|
||||
### Fixes
|
||||
- Security: block owner delete/undelete paths from overriding moderator or scanner hides, and return explicit 403 authz responses for owner restore denials (#2078) (thanks @momothemage).
|
||||
|
||||
- Skills: allow confirmed owner migration when republishing an existing skill to another publisher, preserving versions, stats, aliases, and audit history (#1998, #2102) (thanks @momothemage).
|
||||
- Security: block owner delete/undelete paths from overriding moderator or scanner hides, and return explicit 403 authz responses for owner restore denials (#2078) (thanks @momothemage).
|
||||
|
||||
## 0.12.3 - 2026-05-06
|
||||
|
||||
|
||||
4
convex/_generated/api.d.ts
vendored
4
convex/_generated/api.d.ts
vendored
@ -74,6 +74,7 @@ import type * as lib_packageRegistry from "../lib/packageRegistry.js";
|
||||
import type * as lib_packageSearchDigest from "../lib/packageSearchDigest.js";
|
||||
import type * as lib_packageSecurity from "../lib/packageSecurity.js";
|
||||
import type * as lib_public from "../lib/public.js";
|
||||
import type * as lib_publicRouteReservations from "../lib/publicRouteReservations.js";
|
||||
import type * as lib_publishLimits from "../lib/publishLimits.js";
|
||||
import type * as lib_publishers from "../lib/publishers.js";
|
||||
import type * as lib_reporting from "../lib/reporting.js";
|
||||
@ -87,6 +88,7 @@ import type * as lib_skillPublish from "../lib/skillPublish.js";
|
||||
import type * as lib_skillQuality from "../lib/skillQuality.js";
|
||||
import type * as lib_skillSafety from "../lib/skillSafety.js";
|
||||
import type * as lib_skillSearchDigest from "../lib/skillSearchDigest.js";
|
||||
import type * as lib_skillSlugValidator from "../lib/skillSlugValidator.js";
|
||||
import type * as lib_skillStats from "../lib/skillStats.js";
|
||||
import type * as lib_skillSummary from "../lib/skillSummary.js";
|
||||
import type * as lib_skillZip from "../lib/skillZip.js";
|
||||
@ -202,6 +204,7 @@ declare const fullApi: ApiFromModules<{
|
||||
"lib/packageSearchDigest": typeof lib_packageSearchDigest;
|
||||
"lib/packageSecurity": typeof lib_packageSecurity;
|
||||
"lib/public": typeof lib_public;
|
||||
"lib/publicRouteReservations": typeof lib_publicRouteReservations;
|
||||
"lib/publishLimits": typeof lib_publishLimits;
|
||||
"lib/publishers": typeof lib_publishers;
|
||||
"lib/reporting": typeof lib_reporting;
|
||||
@ -215,6 +218,7 @@ declare const fullApi: ApiFromModules<{
|
||||
"lib/skillQuality": typeof lib_skillQuality;
|
||||
"lib/skillSafety": typeof lib_skillSafety;
|
||||
"lib/skillSearchDigest": typeof lib_skillSearchDigest;
|
||||
"lib/skillSlugValidator": typeof lib_skillSlugValidator;
|
||||
"lib/skillStats": typeof lib_skillStats;
|
||||
"lib/skillSummary": typeof lib_skillSummary;
|
||||
"lib/skillZip": typeof lib_skillZip;
|
||||
|
||||
@ -2416,6 +2416,7 @@ describe("httpApiV1 handlers", () => {
|
||||
slug: "demo",
|
||||
displayName: "Demo",
|
||||
ownerHandle: "@openclaw",
|
||||
migrateOwner: true,
|
||||
version: "1.0.0",
|
||||
changelog: "c",
|
||||
acceptLicenseTerms: true,
|
||||
@ -2450,7 +2451,7 @@ describe("httpApiV1 handlers", () => {
|
||||
expect.anything(),
|
||||
"users:1",
|
||||
expect.not.objectContaining({ ownerHandle: expect.anything() }),
|
||||
{ ownerPublisherId: "publishers:openclaw" },
|
||||
{ ownerPublisherId: "publishers:openclaw", migrateOwner: true },
|
||||
);
|
||||
});
|
||||
|
||||
@ -2586,6 +2587,7 @@ describe("httpApiV1 handlers", () => {
|
||||
slug: "demo",
|
||||
displayName: "Demo",
|
||||
ownerHandle: "@openclaw",
|
||||
migrateOwner: true,
|
||||
version: "1.0.0",
|
||||
changelog: "",
|
||||
acceptLicenseTerms: true,
|
||||
@ -2606,7 +2608,7 @@ describe("httpApiV1 handlers", () => {
|
||||
expect.anything(),
|
||||
"users:1",
|
||||
expect.not.objectContaining({ ownerHandle: expect.anything() }),
|
||||
{ ownerPublisherId: "publishers:openclaw" },
|
||||
{ ownerPublisherId: "publishers:openclaw", migrateOwner: true },
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@ -311,6 +311,7 @@ export async function parseMultipartPublish(
|
||||
slug: payload.slug,
|
||||
displayName: payload.displayName,
|
||||
...(typeof payload.ownerHandle === "string" ? { ownerHandle: payload.ownerHandle } : {}),
|
||||
...(typeof payload.migrateOwner === "boolean" ? { migrateOwner: payload.migrateOwner } : {}),
|
||||
version: payload.version,
|
||||
changelog: typeof payload.changelog === "string" ? payload.changelog : "",
|
||||
...(hasAcceptLicenseTerms ? { acceptLicenseTerms: payload.acceptLicenseTerms } : {}),
|
||||
@ -331,6 +332,7 @@ export function parsePublishBody(body: unknown) {
|
||||
slug: parsed.slug,
|
||||
displayName: parsed.displayName,
|
||||
ownerHandle: parsed.ownerHandle?.trim().replace(/^@+/, "") || undefined,
|
||||
migrateOwner: parsed.migrateOwner === true ? true : undefined,
|
||||
version: parsed.version,
|
||||
changelog: parsed.changelog,
|
||||
acceptLicenseTerms: parsed.acceptLicenseTerms,
|
||||
|
||||
@ -1038,7 +1038,7 @@ async function publishSkillPayloadForApiUser(
|
||||
userId: Id<"users">,
|
||||
payload: ReturnType<typeof parsePublishBody>,
|
||||
) {
|
||||
const { ownerHandle, ...publishPayload } = payload;
|
||||
const { ownerHandle, migrateOwner, ...publishPayload } = payload;
|
||||
if (!ownerHandle) {
|
||||
return await publishVersionForUser(ctx, userId, publishPayload);
|
||||
}
|
||||
@ -1049,6 +1049,7 @@ async function publishSkillPayloadForApiUser(
|
||||
})) as { publisherId: Id<"publishers"> };
|
||||
return await publishVersionForUser(ctx, userId, publishPayload, {
|
||||
ownerPublisherId: target.publisherId,
|
||||
migrateOwner: migrateOwner === true ? true : undefined,
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@ -82,6 +82,11 @@ export type PublishOptions = {
|
||||
skipBackup?: boolean;
|
||||
skipWebhook?: boolean;
|
||||
ownerPublisherId?: Id<"publishers">;
|
||||
// Explicit opt-in to owner migration. The `insertVersion` mutation refuses
|
||||
// to rewrite a skill's `ownerPublisherId` unless this is `true`, so default
|
||||
// publishes (including older CLIs that never pass this flag) can never
|
||||
// accidentally transfer ownership.
|
||||
migrateOwner?: boolean;
|
||||
};
|
||||
|
||||
export async function publishVersionForUser(
|
||||
@ -299,6 +304,7 @@ export async function publishVersionForUser(
|
||||
const publishResult = (await ctx.runMutation(internal.skills.insertVersion, {
|
||||
userId,
|
||||
ownerPublisherId: options.ownerPublisherId,
|
||||
migrateOwner: options.migrateOwner,
|
||||
slug,
|
||||
displayName,
|
||||
version,
|
||||
@ -351,7 +357,14 @@ export async function publishVersionForUser(
|
||||
versionId: publishResult.versionId,
|
||||
});
|
||||
|
||||
const ownerHandle = owner?.handle ?? owner?.displayName ?? owner?.name ?? "unknown";
|
||||
const targetPublisher =
|
||||
options.ownerPublisherId !== undefined
|
||||
? ((await ctx.runQuery(internal.publishers.getByIdInternal, {
|
||||
publisherId: options.ownerPublisherId,
|
||||
})) as Doc<"publishers"> | null)
|
||||
: null;
|
||||
const ownerHandle =
|
||||
targetPublisher?.handle ?? owner?.handle ?? owner?.displayName ?? owner?.name ?? "unknown";
|
||||
|
||||
if (!options.skipBackup) {
|
||||
void ctx.scheduler
|
||||
|
||||
@ -1,5 +1,6 @@
|
||||
import type { Doc, Id } from "../_generated/dataModel";
|
||||
import type { MutationCtx } from "../_generated/server";
|
||||
import { readCanonicalStat } from "./skillStats";
|
||||
|
||||
function getSkillContribution(skill: Doc<"skills">) {
|
||||
if (skill.softDeletedAt) {
|
||||
@ -8,8 +9,8 @@ function getSkillContribution(skill: Doc<"skills">) {
|
||||
|
||||
return {
|
||||
publishedSkills: 1,
|
||||
totalStars: skill.stats?.stars ?? 0,
|
||||
totalDownloads: skill.stats?.downloads ?? 0,
|
||||
totalStars: readCanonicalStat(skill, "stars"),
|
||||
totalDownloads: readCanonicalStat(skill, "downloads"),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
691
convex/skills.ownerMigration.test.ts
Normal file
691
convex/skills.ownerMigration.test.ts
Normal file
@ -0,0 +1,691 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("@convex-dev/auth/server", () => ({
|
||||
getAuthUserId: vi.fn(),
|
||||
authTables: {},
|
||||
}));
|
||||
|
||||
import { insertVersion } from "./skills";
|
||||
|
||||
type WrappedHandler<TArgs> = {
|
||||
_handler: (ctx: unknown, args: TArgs) => Promise<unknown>;
|
||||
};
|
||||
|
||||
const insertVersionHandler = (insertVersion as unknown as WrappedHandler<Record<string, unknown>>)
|
||||
._handler;
|
||||
|
||||
const SENTINEL_BAIL_MESSAGE = "__owner_migration_sentinel_stop__";
|
||||
|
||||
function buildPublishArgs(overrides?: Partial<Record<string, unknown>>) {
|
||||
return {
|
||||
userId: "users:caller",
|
||||
ownerPublisherId: "publishers:org",
|
||||
slug: "nano",
|
||||
displayName: "Nano",
|
||||
version: "1.0.0",
|
||||
changelog: "Initial release",
|
||||
changelogSource: "user",
|
||||
tags: ["latest"],
|
||||
fingerprint: "f".repeat(64),
|
||||
files: [
|
||||
{
|
||||
path: "SKILL.md",
|
||||
size: 128,
|
||||
storageId: "_storage:1",
|
||||
sha256: "a".repeat(64),
|
||||
contentType: "text/markdown",
|
||||
},
|
||||
],
|
||||
parsed: {
|
||||
frontmatter: { description: "test" },
|
||||
metadata: {},
|
||||
clawdis: {},
|
||||
},
|
||||
embedding: [0.1, 0.2],
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
type PublisherMemberRecord = {
|
||||
_id: string;
|
||||
publisherId: string;
|
||||
userId: string;
|
||||
role: "owner" | "admin" | "publisher";
|
||||
};
|
||||
|
||||
type OrgMigrationFixture = {
|
||||
db: {
|
||||
get: ReturnType<typeof vi.fn>;
|
||||
query: ReturnType<typeof vi.fn>;
|
||||
patch: ReturnType<typeof vi.fn>;
|
||||
insert: ReturnType<typeof vi.fn>;
|
||||
normalizeId: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
patchCalls: Array<{ id: string; value: Record<string, unknown> }>;
|
||||
insertCalls: Array<{ table: string; value: Record<string, unknown> }>;
|
||||
};
|
||||
|
||||
type SkillSourceMode = "other-personal" | "caller-personal" | "source-org";
|
||||
|
||||
function createMigrationFixture(params: {
|
||||
sourceMemberships: PublisherMemberRecord[];
|
||||
/**
|
||||
* Which publisher owns the existing `skills:1` row in this fixture:
|
||||
* - "other-personal": `publishers:personalSource` (linkedUser = users:sourceOwner),
|
||||
* used to simulate an attacker publishing into someone else's slug.
|
||||
* - "caller-personal": `publishers:personalCaller` (linkedUser = users:caller),
|
||||
* used to simulate the real issue scenario: moving your own personal skill
|
||||
* into an org you belong to.
|
||||
* - "source-org": `publishers:sourceOrg` (kind = "org"), used to simulate
|
||||
* moving a skill OUT of one org and into another publisher. The caller's
|
||||
* authority on `publishers:sourceOrg` is parameterized via `sourceMemberships`.
|
||||
*/
|
||||
skillSource?: SkillSourceMode;
|
||||
sourcePersonalLinkedUserId?: string | null;
|
||||
}): OrgMigrationFixture {
|
||||
const now = Date.now();
|
||||
const patchCalls: Array<{ id: string; value: Record<string, unknown> }> = [];
|
||||
const insertCalls: Array<{ table: string; value: Record<string, unknown> }> = [];
|
||||
|
||||
const db = {
|
||||
get: vi.fn(async (id: string) => {
|
||||
if (id === "users:caller") {
|
||||
return {
|
||||
_id: "users:caller",
|
||||
handle: "caller",
|
||||
name: "caller",
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
personalPublisherId: "publishers:personalCaller",
|
||||
_creationTime: now,
|
||||
};
|
||||
}
|
||||
if (id === "publishers:personalCaller") {
|
||||
return {
|
||||
_id: "publishers:personalCaller",
|
||||
kind: "user",
|
||||
handle: "caller",
|
||||
displayName: "caller",
|
||||
linkedUserId:
|
||||
params.sourcePersonalLinkedUserId === undefined
|
||||
? "users:caller"
|
||||
: params.sourcePersonalLinkedUserId,
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
};
|
||||
}
|
||||
if (id === "publishers:org") {
|
||||
return {
|
||||
_id: "publishers:org",
|
||||
kind: "org",
|
||||
handle: "casualsecurityinc",
|
||||
displayName: "Casual Security",
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
};
|
||||
}
|
||||
if (id === "users:sourceOwner") {
|
||||
return {
|
||||
_id: "users:sourceOwner",
|
||||
handle: "cbrunnkvist",
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
};
|
||||
}
|
||||
if (id === "publishers:personalSource") {
|
||||
return {
|
||||
_id: "publishers:personalSource",
|
||||
kind: "user",
|
||||
handle: "cbrunnkvist",
|
||||
displayName: "cbrunnkvist",
|
||||
linkedUserId: "users:sourceOwner",
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
};
|
||||
}
|
||||
if (id === "publishers:sourceOrg") {
|
||||
return {
|
||||
_id: "publishers:sourceOrg",
|
||||
kind: "org",
|
||||
handle: "sourceorg",
|
||||
displayName: "Source Org",
|
||||
deletedAt: undefined,
|
||||
deactivatedAt: undefined,
|
||||
};
|
||||
}
|
||||
return null;
|
||||
}),
|
||||
query: vi.fn((table: string) => {
|
||||
if (table === "publishers") {
|
||||
return {
|
||||
withIndex: (_name: string, build: (q: unknown) => unknown) => {
|
||||
// Handle any publisher-handle lookup by returning the caller/source/org
|
||||
// as inert (not present) to keep ensurePersonalPublisherForUser happy.
|
||||
const q: Record<string, unknown> = {
|
||||
eq: (_field: string, _value: unknown) => q,
|
||||
};
|
||||
build?.(q);
|
||||
return { unique: async () => null };
|
||||
},
|
||||
};
|
||||
}
|
||||
if (table === "publisherMembers") {
|
||||
return {
|
||||
withIndex: (
|
||||
name: string,
|
||||
build: (q: { eq: (field: string, value: string) => unknown }) => unknown,
|
||||
) => {
|
||||
if (name !== "by_publisher_user") {
|
||||
throw new Error(`unexpected publisherMembers index ${name}`);
|
||||
}
|
||||
let publisherId = "";
|
||||
let userId = "";
|
||||
const q = {
|
||||
eq: (field: string, value: string) => {
|
||||
if (field === "publisherId") publisherId = value;
|
||||
if (field === "userId") userId = value;
|
||||
return q;
|
||||
},
|
||||
};
|
||||
build(q);
|
||||
return {
|
||||
unique: async () => {
|
||||
// Caller's role on the target org defaults to "publisher" in
|
||||
// these tests (that's the precondition the first-pass
|
||||
// `requirePublisherRole(..., ["publisher"])` check expects).
|
||||
// Individual tests can upgrade this to admin/owner by passing
|
||||
// a matching entry in `sourceMemberships`, which we consult
|
||||
// FIRST so it can override the default. Source-publisher
|
||||
// membership is parameterized per-test the same way.
|
||||
const override = params.sourceMemberships.find(
|
||||
(m) => m.publisherId === publisherId && m.userId === userId,
|
||||
);
|
||||
if (override) return override;
|
||||
if (publisherId === "publishers:personalCaller" && userId === "users:caller") {
|
||||
return {
|
||||
_id: "publisherMembers:personalCaller",
|
||||
publisherId,
|
||||
userId,
|
||||
role: "owner",
|
||||
};
|
||||
}
|
||||
if (publisherId === "publishers:org" && userId === "users:caller") {
|
||||
return {
|
||||
_id: "publisherMembers:orgCaller",
|
||||
publisherId,
|
||||
userId,
|
||||
role: "publisher",
|
||||
};
|
||||
}
|
||||
return null;
|
||||
},
|
||||
};
|
||||
},
|
||||
};
|
||||
}
|
||||
if (table === "skills") {
|
||||
return {
|
||||
withIndex: (
|
||||
name: string,
|
||||
build: ((q: { eq: (field: string, value: string) => unknown }) => unknown) | undefined,
|
||||
) => {
|
||||
if (name === "by_slug") {
|
||||
const q = {
|
||||
eq: (_field: string, _value: string) => q,
|
||||
};
|
||||
build?.(q);
|
||||
const mode: SkillSourceMode = params.skillSource ?? "other-personal";
|
||||
const ownerPublisherId =
|
||||
mode === "caller-personal"
|
||||
? "publishers:personalCaller"
|
||||
: mode === "source-org"
|
||||
? "publishers:sourceOrg"
|
||||
: "publishers:personalSource";
|
||||
const ownerUserId = mode === "caller-personal" ? "users:caller" : "users:sourceOwner";
|
||||
return {
|
||||
unique: async () => ({
|
||||
_id: "skills:1",
|
||||
slug: "nano",
|
||||
ownerUserId,
|
||||
ownerPublisherId,
|
||||
softDeletedAt: undefined,
|
||||
moderationStatus: "active",
|
||||
moderationFlags: undefined,
|
||||
statsDownloads: 42,
|
||||
statsStars: 7,
|
||||
stats: {
|
||||
downloads: 1,
|
||||
stars: 2,
|
||||
installsCurrent: 0,
|
||||
installsAllTime: 0,
|
||||
comments: 0,
|
||||
versions: 1,
|
||||
},
|
||||
}),
|
||||
};
|
||||
}
|
||||
// Any subsequent skill-table access means migration was allowed and
|
||||
// insertVersion proceeded to the "brand new skill" path. Bail out
|
||||
// with a sentinel so the test can assert patch/insert calls without
|
||||
// having to mock the entire downstream pipeline.
|
||||
throw new Error(SENTINEL_BAIL_MESSAGE);
|
||||
},
|
||||
};
|
||||
}
|
||||
if (table === "skillSlugAliases") {
|
||||
return {
|
||||
withIndex: (name: string) => {
|
||||
if (name === "by_skill") {
|
||||
return { collect: async () => [] };
|
||||
}
|
||||
if (name === "by_slug") {
|
||||
return { unique: async () => null };
|
||||
}
|
||||
throw new Error(`unexpected skillSlugAliases index ${name}`);
|
||||
},
|
||||
};
|
||||
}
|
||||
if (table === "skillEmbeddings") {
|
||||
return {
|
||||
withIndex: (name: string) => {
|
||||
if (name === "by_skill") {
|
||||
// Single mock embedding for `skills:1` so the migration branch
|
||||
// exercises the embedding-ownerId reassignment loop. Older
|
||||
// `ownerId` is the source owner derived from the skill source
|
||||
// mode, mirroring how embeddings get written at publish time.
|
||||
const mode: SkillSourceMode = params.skillSource ?? "other-personal";
|
||||
const ownerId = mode === "caller-personal" ? "users:caller" : "users:sourceOwner";
|
||||
return {
|
||||
collect: async () => [
|
||||
{
|
||||
_id: "skillEmbeddings:1",
|
||||
skillId: "skills:1",
|
||||
ownerId,
|
||||
},
|
||||
],
|
||||
};
|
||||
}
|
||||
throw new Error(`unexpected skillEmbeddings index ${name}`);
|
||||
},
|
||||
};
|
||||
}
|
||||
if (table === "authAccounts") {
|
||||
return {
|
||||
withIndex: () => ({
|
||||
unique: async () => null,
|
||||
}),
|
||||
};
|
||||
}
|
||||
// Any access to a downstream table means the migration branch completed
|
||||
// and insertVersion proceeded into the publish pipeline. Bail out with a
|
||||
// sentinel so the test can assert patch/insert side-effects without
|
||||
// having to mock the entire pipeline.
|
||||
throw new Error(SENTINEL_BAIL_MESSAGE);
|
||||
}),
|
||||
patch: vi.fn(async (id: string, value: Record<string, unknown>) => {
|
||||
patchCalls.push({ id, value });
|
||||
}),
|
||||
insert: vi.fn(async (table: string, value: Record<string, unknown>) => {
|
||||
insertCalls.push({ table, value });
|
||||
return `${table}:inserted`;
|
||||
}),
|
||||
normalizeId: vi.fn(),
|
||||
};
|
||||
|
||||
return { db, patchCalls, insertCalls };
|
||||
}
|
||||
|
||||
describe("skills.insertVersion owner migration", () => {
|
||||
it("rejects slug migration when caller has no membership on the source publisher", async () => {
|
||||
const fixture = createMigrationFixture({ sourceMemberships: [] });
|
||||
|
||||
// Pass `migrateOwner: true` so this test actually exercises the
|
||||
// source-authority check. Without the opt-in, the request would be
|
||||
// rejected by the explicit-intent gate instead and the authority check
|
||||
// below would never run.
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(/Slug is already taken/);
|
||||
|
||||
// The skill row must NOT be patched when the caller is not a source member.
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
|
||||
// No migration audit log should be written on the rejection path.
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("rejects slug migration when caller is only a 'publisher' (not admin/owner) on the source org", async () => {
|
||||
// Regression guard for the privilege-escalation path: a plain publisher-role
|
||||
// member of the source org must NOT be able to walk skills out of that org
|
||||
// via a republish. Transferring ownership requires admin/owner-level
|
||||
// authority on the source, aligned with `transferPackage` in packages.ts.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "source-org",
|
||||
sourceMemberships: [
|
||||
{
|
||||
_id: "publisherMembers:sourcePublisher",
|
||||
publisherId: "publishers:sourceOrg",
|
||||
userId: "users:caller",
|
||||
role: "publisher",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(/Slug is already taken/);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("migrates ownership when caller is an admin on the source org", async () => {
|
||||
// Positive counterpart to the publisher-only rejection above: admin/owner
|
||||
// authority on the source publisher IS sufficient to move the skill, which
|
||||
// matches the transfer semantics in convex/packages.ts.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "source-org",
|
||||
sourceMemberships: [
|
||||
{
|
||||
_id: "publisherMembers:sourceAdmin",
|
||||
publisherId: "publishers:sourceOrg",
|
||||
userId: "users:caller",
|
||||
role: "admin",
|
||||
},
|
||||
// Destination admin role is also required on the migration path now
|
||||
// (matching transferPackage). Without this the migration would be
|
||||
// rejected even though the source side is satisfied.
|
||||
{
|
||||
_id: "publisherMembers:orgAdminCaller",
|
||||
publisherId: "publishers:org",
|
||||
userId: "users:caller",
|
||||
role: "admin",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(SENTINEL_BAIL_MESSAGE);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(1);
|
||||
expect(skillPatches[0]?.value).toMatchObject({
|
||||
ownerPublisherId: "publishers:org",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(1);
|
||||
const auditMetadata = migrationAudits[0]?.value.metadata as {
|
||||
from?: { ownerPublisherId?: string; ownerUserId?: string };
|
||||
to?: { ownerPublisherId?: string; ownerUserId?: string };
|
||||
};
|
||||
expect(auditMetadata.from).toEqual({
|
||||
ownerPublisherId: "publishers:sourceOrg",
|
||||
ownerUserId: "users:sourceOwner",
|
||||
});
|
||||
expect(auditMetadata.to).toEqual({
|
||||
ownerPublisherId: "publishers:org",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
|
||||
// Cross-owner migration must also rebalance the per-user skill counters
|
||||
// (publishedSkills / totalStars / totalDownloads) so the previous owner
|
||||
// stops being credited for the skill. This mirrors the maintenance the
|
||||
// moderator `changeOwner` path performs via
|
||||
// `adjustUserSkillStatsForSkillChange`.
|
||||
const prevOwnerStatsPatch = fixture.patchCalls.find((p) => p.id === "users:sourceOwner");
|
||||
expect(prevOwnerStatsPatch?.value).toMatchObject({
|
||||
publishedSkills: 0,
|
||||
totalStars: 0,
|
||||
totalDownloads: 0,
|
||||
});
|
||||
const nextOwnerStatsPatch = fixture.patchCalls.find((p) => p.id === "users:caller");
|
||||
expect(nextOwnerStatsPatch?.value).toMatchObject({
|
||||
publishedSkills: 1,
|
||||
totalStars: 7,
|
||||
totalDownloads: 42,
|
||||
});
|
||||
|
||||
// And the skill's embedding must be re-homed to the new owner so that
|
||||
// "authored by" queries don't keep resolving to the previous owner.
|
||||
const embeddingPatches = fixture.patchCalls.filter((p) => p.id === "skillEmbeddings:1");
|
||||
expect(embeddingPatches).toHaveLength(1);
|
||||
expect(embeddingPatches[0]?.value).toMatchObject({ ownerId: "users:caller" });
|
||||
});
|
||||
|
||||
it("migrates ownership when caller moves their OWN personal skill into an org they belong to", async () => {
|
||||
// Real issue scenario: @cbrunnkvist owns `nano` under their personal
|
||||
// publisher and wants to republish under `@casualsecurityinc`.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "caller-personal",
|
||||
sourceMemberships: [
|
||||
// ensurePersonalPublisherForUser already grants the caller an "owner"
|
||||
// membership on publishers:personalCaller, so the source side is
|
||||
// covered. The destination (the org) needs admin-level rights now
|
||||
// (matching transferPackage), so we grant that explicitly here.
|
||||
{
|
||||
_id: "publisherMembers:orgAdminCaller",
|
||||
publisherId: "publishers:org",
|
||||
userId: "users:caller",
|
||||
role: "admin",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
// After the migration branch succeeds we bail out via a sentinel so we can
|
||||
// assert on the side-effects without fully mocking downstream pipeline.
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(SENTINEL_BAIL_MESSAGE);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(1);
|
||||
expect(skillPatches[0]?.value).toMatchObject({
|
||||
ownerPublisherId: "publishers:org",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(1);
|
||||
const auditMetadata = migrationAudits[0]?.value.metadata as {
|
||||
from?: { ownerPublisherId?: string; ownerUserId?: string };
|
||||
to?: { ownerPublisherId?: string; ownerUserId?: string };
|
||||
};
|
||||
expect(auditMetadata.from).toEqual({
|
||||
ownerPublisherId: "publishers:personalCaller",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
expect(auditMetadata.to).toEqual({
|
||||
ownerPublisherId: "publishers:org",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
|
||||
// When the skill's `ownerUserId` doesn't actually change (personal → org
|
||||
// owned by the same user), the embedding's `ownerId` was already correct
|
||||
// and must not be rewritten. This exercises the early-continue branch in
|
||||
// the embedding reassignment loop and avoids a no-op patch storm.
|
||||
const embeddingPatches = fixture.patchCalls.filter((p) => p.id === "skillEmbeddings:1");
|
||||
expect(embeddingPatches).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("migrates caller-owned personal skills even when the legacy personal publisher link is missing", async () => {
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "caller-personal",
|
||||
sourcePersonalLinkedUserId: null,
|
||||
sourceMemberships: [
|
||||
{
|
||||
_id: "publisherMembers:orgAdminCaller",
|
||||
publisherId: "publishers:org",
|
||||
userId: "users:caller",
|
||||
role: "admin",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(SENTINEL_BAIL_MESSAGE);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(1);
|
||||
expect(skillPatches[0]?.value).toMatchObject({
|
||||
ownerPublisherId: "publishers:org",
|
||||
ownerUserId: "users:caller",
|
||||
});
|
||||
});
|
||||
|
||||
it("refuses to migrate a skill out of SOMEONE ELSE'S personal publisher even if caller happens to be a member", async () => {
|
||||
// Defense-in-depth: addMember currently doesn't forbid adding extra
|
||||
// members to a user-kind publisher. We must still refuse to let the
|
||||
// extra member move that user's skills away from them.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "other-personal",
|
||||
sourceMemberships: [
|
||||
{
|
||||
_id: "publisherMembers:sourceCaller",
|
||||
publisherId: "publishers:personalSource",
|
||||
userId: "users:caller",
|
||||
role: "publisher",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow(/Slug is already taken/);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("does NOT migrate ownership when caller omits ownerPublisherId (prevents silent re-ownership)", async () => {
|
||||
const fixture = createMigrationFixture({
|
||||
sourceMemberships: [
|
||||
// Caller happens to be a publisher on the source org — but has NOT
|
||||
// explicitly asked for any particular target publisher. Without the
|
||||
// explicit opt-in, we must fall through to the "Slug is already taken"
|
||||
// error instead of silently migrating the org-owned skill back into
|
||||
// the caller's personal namespace.
|
||||
{
|
||||
_id: "publisherMembers:sourceCaller",
|
||||
publisherId: "publishers:personalSource",
|
||||
userId: "users:caller",
|
||||
role: "publisher",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const argsWithoutOwner = buildPublishArgs({ migrateOwner: true });
|
||||
delete (argsWithoutOwner as Record<string, unknown>).ownerPublisherId;
|
||||
|
||||
await expect(
|
||||
insertVersionHandler({ db: fixture.db } as never, argsWithoutOwner as never),
|
||||
).rejects.toThrow(/Slug is already taken/);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("rejects migration when caller does NOT pass migrateOwner:true even with full source+destination authority", async () => {
|
||||
// Explicit-intent gate: even if the caller has all the authority needed
|
||||
// on both sides, refusing to pass `migrateOwner: true` must be treated as
|
||||
// "not trying to move the skill" and fall through to the slug-collision
|
||||
// error. This is what protects the New Version form from silently
|
||||
// re-owning a skill whose Owner selector happens to default to the
|
||||
// caller's personal publisher.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "caller-personal",
|
||||
sourceMemberships: [
|
||||
{
|
||||
_id: "publisherMembers:orgAdminCaller",
|
||||
publisherId: "publishers:org",
|
||||
userId: "users:caller",
|
||||
role: "admin",
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler({ db: fixture.db } as never, buildPublishArgs() as never),
|
||||
).rejects.toThrow(/Slug is already taken/);
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("rejects migration when caller is only 'publisher' (not admin) on the DESTINATION org", async () => {
|
||||
// Destination authority check (aligned with transferPackage in
|
||||
// convex/packages.ts): publishing into an org only requires publisher
|
||||
// role, but *transferring ownership into* the org requires admin-level
|
||||
// rights on that destination too. A plain publisher on the destination
|
||||
// org must not be able to pull a skill into the org namespace via a
|
||||
// republish even if source-side authority is fully satisfied.
|
||||
const fixture = createMigrationFixture({
|
||||
skillSource: "caller-personal",
|
||||
// Grant NO admin role on publishers:org — the default fixture wiring
|
||||
// already gives the caller "publisher" role there, which is what we
|
||||
// want to test against.
|
||||
sourceMemberships: [],
|
||||
});
|
||||
|
||||
await expect(
|
||||
insertVersionHandler(
|
||||
{ db: fixture.db } as never,
|
||||
buildPublishArgs({ migrateOwner: true }) as never,
|
||||
),
|
||||
).rejects.toThrow();
|
||||
|
||||
const skillPatches = fixture.patchCalls.filter((p) => p.id === "skills:1");
|
||||
expect(skillPatches).toHaveLength(0);
|
||||
const migrationAudits = fixture.insertCalls.filter(
|
||||
(call) => call.table === "auditLogs" && call.value.action === "skill.ownership.migrate",
|
||||
);
|
||||
expect(migrationAudits).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
170
convex/skills.ts
170
convex/skills.ts
@ -74,6 +74,8 @@ import {
|
||||
assertCanManageOwnedResource,
|
||||
ensurePersonalPublisherForUser,
|
||||
getOwnerPublisher,
|
||||
getPublisherMembership,
|
||||
isPublisherRoleAllowed,
|
||||
requirePublisherRole,
|
||||
} from "./lib/publishers";
|
||||
import {
|
||||
@ -6218,6 +6220,11 @@ export const getVersionBySkillAndVersion = query({
|
||||
export const publishVersion: ReturnType<typeof action> = action({
|
||||
args: {
|
||||
ownerHandle: v.optional(v.string()),
|
||||
// Explicit opt-in from the client to migrate an existing skill's owner
|
||||
// when `ownerHandle` differs from the skill's current owner. Without this
|
||||
// flag, a mismatching Owner selector is treated as a slug collision so
|
||||
// re-publishes cannot silently transfer ownership.
|
||||
migrateOwner: v.optional(v.boolean()),
|
||||
slug: v.string(),
|
||||
displayName: v.string(),
|
||||
version: v.string(),
|
||||
@ -6252,6 +6259,7 @@ export const publishVersion: ReturnType<typeof action> = action({
|
||||
})) as { publisherId: Id<"publishers"> };
|
||||
return publishVersionForUser(ctx, userId, args, {
|
||||
ownerPublisherId: target.publisherId,
|
||||
migrateOwner: args.migrateOwner,
|
||||
});
|
||||
},
|
||||
});
|
||||
@ -7578,6 +7586,13 @@ export const insertVersion = internalMutation({
|
||||
args: {
|
||||
userId: v.id("users"),
|
||||
ownerPublisherId: v.optional(v.id("publishers")),
|
||||
// Explicit opt-in to owner migration. When an existing skill row already has
|
||||
// a different `ownerPublisherId` than the one supplied above, the mutation
|
||||
// only rewrites ownership if `migrateOwner === true`. Without this flag the
|
||||
// mismatch is surfaced as a slug-collision error (the pre-org-migration
|
||||
// behaviour), so a silently-different Owner value in an older CLI or a
|
||||
// wrongly-defaulted form cannot re-own an org-owned skill by accident.
|
||||
migrateOwner: v.optional(v.boolean()),
|
||||
slug: v.string(),
|
||||
displayName: v.string(),
|
||||
version: v.string(),
|
||||
@ -7663,6 +7678,15 @@ export const insertVersion = internalMutation({
|
||||
if (!user || user.deletedAt || user.deactivatedAt) throw new Error("User not found");
|
||||
const personalPublisher = await ensurePersonalPublisherForUser(ctx, user);
|
||||
if (!personalPublisher) throw new ConvexError("Personal publisher not found");
|
||||
// `callerExplicitlySpecifiedOwner` distinguishes the two semantically
|
||||
// different reasons we end up with `ownerPublisherId === personalPublisher._id`:
|
||||
// 1. the caller explicitly asked to publish under their own personal
|
||||
// publisher (we still allow migration in that case — moving from an
|
||||
// org back to personal is symmetric to the org-migration flow), or
|
||||
// 2. the caller simply didn't pass the field (e.g. older CLI builds).
|
||||
// We only treat case (2) as "no migration intent", so that a silent client
|
||||
// upgrade can never re-own an org-owned skill into a personal namespace.
|
||||
const callerExplicitlySpecifiedOwner = args.ownerPublisherId !== undefined;
|
||||
const ownerPublisherId = args.ownerPublisherId ?? personalPublisher._id;
|
||||
if (ownerPublisherId !== personalPublisher._id) {
|
||||
await requirePublisherRole(ctx, {
|
||||
@ -7703,11 +7727,149 @@ export const insertVersion = internalMutation({
|
||||
}
|
||||
|
||||
if (skill && skill.ownerPublisherId && skill.ownerPublisherId !== ownerPublisherId) {
|
||||
const owner = await getOwnerPublisher(ctx, {
|
||||
ownerPublisherId: skill.ownerPublisherId,
|
||||
ownerUserId: skill.ownerUserId,
|
||||
// Owner migration: allow publishing under a different publisher (e.g. moving
|
||||
// a skill from a personal publisher into an org, or between orgs) only when
|
||||
// the caller has sufficient authority on BOTH sides AND has explicitly
|
||||
// opted into a migration.
|
||||
//
|
||||
// Authority model — aligned with `transferPackage` in convex/packages.ts:
|
||||
// * destination side — publisher-level rights were already enforced above
|
||||
// (`requirePublisherRole(..., ["publisher"])`) when the caller is
|
||||
// publishing into an org. That is enough for *publishing* into the
|
||||
// destination, but *transferring ownership into* it is a stronger
|
||||
// operation, so on the migration path we additionally require ADMIN
|
||||
// rights on the destination publisher. Moving a skill into the
|
||||
// caller's own personal publisher is still allowed because
|
||||
// `ensurePersonalPublisherForUser` guarantees the caller is the
|
||||
// publisher's `linkedUser` with role `owner` (>= admin).
|
||||
// * source side — must be ADMIN on the source publisher (or the linked
|
||||
// personal-publisher user themselves). This matches the transfer spec:
|
||||
// moving a skill *out* of an org is an ownership change, so a plain
|
||||
// "publisher" role member must not be able to trigger it by republishing.
|
||||
//
|
||||
// We also require the caller to have *explicitly* asked to publish under
|
||||
// a specific publisher (`args.ownerPublisherId !== undefined`) AND to
|
||||
// have explicitly signalled migration intent (`args.migrateOwner === true`).
|
||||
// Older clients that just call `publishVersion` without an owner param, or
|
||||
// newer clients where the Owner selector defaulted to the caller's
|
||||
// personal publisher, would otherwise accidentally migrate org-owned
|
||||
// skills on every publish.
|
||||
//
|
||||
// Defense in depth: `addMember` does not currently require publisher.kind ===
|
||||
// "org", so in principle a user-kind ("personal") publisher can end up with
|
||||
// extra members beyond its linkedUser. We refuse migration *out* of a
|
||||
// user-kind publisher unless the caller IS its linkedUser, so the only
|
||||
// way to move a personal skill is "the owner themselves decides to move
|
||||
// it" — never "a third party who happens to share a publisher row".
|
||||
// Legacy personal publisher rows may be missing `linkedUserId`, so the
|
||||
// persisted skill owner is accepted as the compatibility fallback.
|
||||
const callerRequestedMigration = args.migrateOwner === true;
|
||||
const sourcePublisher = await ctx.db.get(skill.ownerPublisherId);
|
||||
const callerOwnsSourceViaPersonalLink =
|
||||
sourcePublisher?.kind === "user" &&
|
||||
(sourcePublisher.linkedUserId === userId || skill.ownerUserId === userId);
|
||||
const sourceIsOrg = sourcePublisher?.kind === "org";
|
||||
|
||||
const sourceMembership =
|
||||
callerExplicitlySpecifiedOwner && callerRequestedMigration && sourceIsOrg
|
||||
? await getPublisherMembership(ctx, skill.ownerPublisherId, userId)
|
||||
: null;
|
||||
const callerHasSourceAdminRole = Boolean(
|
||||
sourceMembership && isPublisherRoleAllowed(sourceMembership.role, ["admin"]),
|
||||
);
|
||||
const callerCanPublishFromSource =
|
||||
callerExplicitlySpecifiedOwner &&
|
||||
callerRequestedMigration &&
|
||||
(callerOwnsSourceViaPersonalLink || callerHasSourceAdminRole);
|
||||
|
||||
if (!callerCanPublishFromSource) {
|
||||
const owner = await getOwnerPublisher(ctx, {
|
||||
ownerPublisherId: skill.ownerPublisherId,
|
||||
ownerUserId: skill.ownerUserId,
|
||||
});
|
||||
throw new ConvexError(buildSlugTakenErrorMessage(skill, owner));
|
||||
}
|
||||
|
||||
// Destination admin check: publishing into a publisher only requires
|
||||
// publisher-level rights, but *transferring ownership into* a publisher
|
||||
// requires admin-level rights on that destination too. For the caller's
|
||||
// own personal publisher this is trivially satisfied (linkedUser ===
|
||||
// role "owner"); for an org destination this rejects plain publishers.
|
||||
await requirePublisherRole(ctx, {
|
||||
publisherId: ownerPublisherId,
|
||||
userId,
|
||||
allowed: ["admin"],
|
||||
});
|
||||
throw new ConvexError(buildSlugTakenErrorMessage(skill, owner));
|
||||
|
||||
const previousOwnerPublisherId = skill.ownerPublisherId;
|
||||
const previousOwnerUserId = skill.ownerUserId;
|
||||
|
||||
const nextSkill: Doc<"skills"> = {
|
||||
...skill,
|
||||
ownerPublisherId,
|
||||
ownerUserId: userId,
|
||||
updatedAt: now,
|
||||
};
|
||||
|
||||
await ctx.db.patch(skill._id, {
|
||||
ownerPublisherId,
|
||||
ownerUserId: userId,
|
||||
updatedAt: now,
|
||||
});
|
||||
|
||||
// Reassign per-user counters from the previous owner to the new one.
|
||||
// Without this, `users.publishedSkills / totalStars / totalDownloads`
|
||||
// would still credit the source owner after an org→org or
|
||||
// personal→org migration (and double-count once the new owner
|
||||
// publishes anything else). `adjustUserSkillStatsForSkillChange`
|
||||
// already handles the cross-owner move cleanly — this mirrors the
|
||||
// moderator `changeOwner` path above.
|
||||
await adjustUserSkillStatsForSkillChange(ctx, skill, nextSkill);
|
||||
|
||||
// Keep `skillEmbeddings.ownerId` in sync with the skill's owner so
|
||||
// "authored by" queries/filters and embedding-side access checks
|
||||
// don't keep resolving to the previous owner after the migration.
|
||||
const embeddings = await listSkillEmbeddingsForSkill(ctx, skill._id);
|
||||
for (const embedding of embeddings) {
|
||||
if (embedding.ownerId === userId) continue;
|
||||
await ctx.db.patch(embedding._id, {
|
||||
ownerId: userId,
|
||||
updatedAt: now,
|
||||
});
|
||||
}
|
||||
|
||||
// Keep existing slug aliases pointed at the new owner so old URLs still
|
||||
// resolve correctly while the canonical page moves (the `$owner/$slug`
|
||||
// loader already redirects to the canonical owner handle on read).
|
||||
const aliases = await listSkillSlugAliasesForSkill(ctx, skill._id);
|
||||
for (const alias of aliases) {
|
||||
await ctx.db.patch(alias._id, {
|
||||
ownerPublisherId,
|
||||
ownerUserId: userId,
|
||||
updatedAt: now,
|
||||
});
|
||||
}
|
||||
|
||||
await ctx.db.insert("auditLogs", {
|
||||
actorUserId: userId,
|
||||
action: "skill.ownership.migrate",
|
||||
targetType: "skill",
|
||||
targetId: skill._id,
|
||||
metadata: {
|
||||
reason: "publishVersion.ownerMigration",
|
||||
from: {
|
||||
ownerPublisherId: previousOwnerPublisherId,
|
||||
ownerUserId: previousOwnerUserId,
|
||||
},
|
||||
to: {
|
||||
ownerPublisherId,
|
||||
ownerUserId: userId,
|
||||
},
|
||||
},
|
||||
createdAt: now,
|
||||
});
|
||||
|
||||
skill = nextSkill;
|
||||
}
|
||||
|
||||
if (skill && !skill.ownerPublisherId && skill.ownerUserId !== userId) {
|
||||
|
||||
@ -154,6 +154,8 @@ Stores your API token + cached registry URL.
|
||||
- Requires semver: `--version 1.2.3`.
|
||||
- `--owner <handle>` publishes under an org/user publisher handle when the
|
||||
actor has publisher access.
|
||||
- `--migrate-owner` moves an existing skill to `--owner` while publishing a new
|
||||
version. Requires admin/owner access on both publishers.
|
||||
- Owner and review behavior is explained in `docs/publishing.md`.
|
||||
- Publishing a skill means it is released under `MIT-0` on ClawHub.
|
||||
- Published skills are free to use, modify, and redistribute without attribution.
|
||||
|
||||
@ -1138,6 +1138,10 @@ Publishes a new version.
|
||||
- JSON body with `files` (storageId-based) is also accepted.
|
||||
- Optional payload field: `ownerHandle`. When present, the API resolves that
|
||||
publisher server-side and requires the actor to have publisher access.
|
||||
- Optional payload field: `migrateOwner`. When `true` with `ownerHandle`, an
|
||||
existing skill may move to that owner if the actor is an admin/owner on both
|
||||
the current and target publishers. Without this opt-in, owner changes are
|
||||
rejected.
|
||||
|
||||
### `POST /api/v1/packages`
|
||||
|
||||
|
||||
@ -37,6 +37,19 @@ The publish request includes the selected owner, slug, version, changelog, and
|
||||
files. The server verifies that the actor can publish as that owner before it
|
||||
creates the release.
|
||||
|
||||
To move an existing skill to another owner while publishing a new version, choose
|
||||
the new owner and explicitly confirm the ownership move. In the CLI/API, pass the
|
||||
target owner plus the migration opt-in:
|
||||
|
||||
```sh
|
||||
clawhub skill publish ./review-helper --owner openclaw --migrate-owner --version 1.2.0
|
||||
```
|
||||
|
||||
Skill owner migration requires admin or owner access on both the current owner
|
||||
and the destination owner. It preserves the skill, version history, stats,
|
||||
comments, forks, aliases, and audit trail; old owner URLs continue through the
|
||||
alias/redirect path.
|
||||
|
||||
## Plugins
|
||||
|
||||
Plugins use npm-style package names. Scoped package names include the owner in
|
||||
|
||||
@ -285,6 +285,7 @@ registerCommand(program, ["publish"])
|
||||
.option("--slug <slug>", "Skill slug")
|
||||
.option("--name <name>", "Display name")
|
||||
.option("--owner <handle>", "Publish under an org/user publisher handle")
|
||||
.option("--migrate-owner", "Move an existing skill to the selected owner when republishing")
|
||||
.option("--version <version>", "Version (semver)")
|
||||
.option("--fork-of <slug[@version]>", "Mark as a fork of an existing skill")
|
||||
.option("--changelog <text>", "Changelog text")
|
||||
@ -345,6 +346,7 @@ registerCommand(skill, ["skill", "publish"])
|
||||
.option("--slug <slug>", "Skill slug")
|
||||
.option("--name <name>", "Display name")
|
||||
.option("--owner <handle>", "Publish under an org/user publisher handle")
|
||||
.option("--migrate-owner", "Move an existing skill to the selected owner when republishing")
|
||||
.option("--version <version>", "Version (semver)")
|
||||
.option("--fork-of <slug[@version]>", "Mark as a fork of an existing skill")
|
||||
.option("--changelog <text>", "Changelog text")
|
||||
|
||||
@ -165,6 +165,7 @@ describe("cmdPublish", () => {
|
||||
|
||||
await cmdPublish(makeOpts(workdir), "org-skill", {
|
||||
owner: "@openclaw",
|
||||
migrateOwner: true,
|
||||
version: "1.0.1",
|
||||
changelog: "",
|
||||
tags: "latest",
|
||||
@ -180,6 +181,7 @@ describe("cmdPublish", () => {
|
||||
if (typeof payloadEntry !== "string") throw new Error("Missing publish payload");
|
||||
const payload = JSON.parse(payloadEntry);
|
||||
expect(payload.ownerHandle).toBe("openclaw");
|
||||
expect(payload.migrateOwner).toBe(true);
|
||||
} finally {
|
||||
await rm(workdir, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
@ -21,6 +21,7 @@ export async function cmdPublish(
|
||||
changelog?: string;
|
||||
tags?: string;
|
||||
forkOf?: string;
|
||||
migrateOwner?: boolean;
|
||||
},
|
||||
) {
|
||||
const folder = folderArg ? resolve(opts.workdir, folderArg) : null;
|
||||
@ -72,6 +73,7 @@ export async function cmdPublish(
|
||||
slug,
|
||||
displayName,
|
||||
...(ownerHandle ? { ownerHandle } : {}),
|
||||
...(options.migrateOwner ? { migrateOwner: true } : {}),
|
||||
version,
|
||||
changelog,
|
||||
acceptLicenseTerms: true,
|
||||
|
||||
@ -81,6 +81,7 @@ export const CliPublishRequestSchema = type({
|
||||
slug: "string",
|
||||
displayName: "string",
|
||||
ownerHandle: "string?",
|
||||
migrateOwner: "boolean?",
|
||||
version: "string",
|
||||
changelog: "string",
|
||||
acceptLicenseTerms: "boolean?",
|
||||
|
||||
1
packages/schema/dist/schemas.d.ts
vendored
1
packages/schema/dist/schemas.d.ts
vendored
@ -79,6 +79,7 @@ export declare const CliPublishRequestSchema: import("arktype/internal/variants/
|
||||
contentType?: string | undefined;
|
||||
}[];
|
||||
ownerHandle?: string | undefined;
|
||||
migrateOwner?: boolean | undefined;
|
||||
acceptLicenseTerms?: boolean | undefined;
|
||||
tags?: string[] | undefined;
|
||||
source?: {
|
||||
|
||||
1
packages/schema/dist/schemas.js
vendored
1
packages/schema/dist/schemas.js
vendored
@ -67,6 +67,7 @@ export const CliPublishRequestSchema = type({
|
||||
slug: "string",
|
||||
displayName: "string",
|
||||
ownerHandle: "string?",
|
||||
migrateOwner: "boolean?",
|
||||
version: "string",
|
||||
changelog: "string",
|
||||
acceptLicenseTerms: "boolean?",
|
||||
|
||||
2
packages/schema/dist/schemas.js.map
vendored
2
packages/schema/dist/schemas.js.map
vendored
File diff suppressed because one or more lines are too long
@ -69,6 +69,7 @@ describe("clawhub-schema", () => {
|
||||
slug: "demo",
|
||||
displayName: "Demo",
|
||||
ownerHandle: "openclaw",
|
||||
migrateOwner: true,
|
||||
version: "1.0.0",
|
||||
changelog: "",
|
||||
files: [{ path: "SKILL.md", size: 1, storageId: "s", sha256: "x" }],
|
||||
@ -76,6 +77,7 @@ describe("clawhub-schema", () => {
|
||||
"Publish payload",
|
||||
);
|
||||
expect(payload.ownerHandle).toBe("openclaw");
|
||||
expect(payload.migrateOwner).toBe(true);
|
||||
});
|
||||
|
||||
it("reports scoped package names that do not match the selected owner", () => {
|
||||
|
||||
@ -82,6 +82,7 @@ export const CliPublishRequestSchema = type({
|
||||
slug: "string",
|
||||
displayName: "string",
|
||||
ownerHandle: "string?",
|
||||
migrateOwner: "boolean?",
|
||||
version: "string",
|
||||
changelog: "string",
|
||||
acceptLicenseTerms: "boolean?",
|
||||
|
||||
@ -240,14 +240,21 @@ Moderators/admins keep global override powers as they do today.
|
||||
|
||||
### Skills
|
||||
|
||||
Skill publishing currently assumes the actor is the owner.
|
||||
|
||||
Target behavior:
|
||||
Skill publishing is publisher-aware for normal publishes and explicit owner
|
||||
migration.
|
||||
|
||||
- actor selects publisher in UI/CLI
|
||||
- publish mutation validates publisher membership
|
||||
- resource stores `ownerPublisherId`
|
||||
- version keeps `createdBy`
|
||||
- if the selected publisher differs from the existing skill owner, the request
|
||||
must include `migrateOwner: true`
|
||||
- owner migration requires admin or owner access on both the current publisher
|
||||
and the destination publisher
|
||||
- migration preserves the skill id, versions, stats, comments, forks, aliases,
|
||||
search digest identity, and audit history
|
||||
- migration writes a `skill.ownership.migrate` audit event; the new version's
|
||||
`createdBy` remains the publishing actor
|
||||
|
||||
### Packages
|
||||
|
||||
@ -315,6 +322,7 @@ Suggested fields:
|
||||
Publish payloads should take:
|
||||
|
||||
- `ownerHandle`
|
||||
- `migrateOwner`
|
||||
|
||||
Semantics:
|
||||
|
||||
@ -322,6 +330,7 @@ Semantics:
|
||||
- validate membership
|
||||
- reject unknown publishers
|
||||
- reject insufficient role
|
||||
- reject owner changes unless `migrateOwner === true`
|
||||
|
||||
## Transfer Model
|
||||
|
||||
|
||||
@ -71,6 +71,10 @@ export function Upload() {
|
||||
skill?: { slug: string; displayName: string };
|
||||
soul?: { slug: string; displayName: string };
|
||||
latestVersion?: { version: string };
|
||||
// Present on skills.getBySlug; absent on souls.getBySlug. Used to
|
||||
// default the Owner selector to the skill's current owner in update
|
||||
// mode so a New Version publish does not silently re-own the skill.
|
||||
owner?: { handle: string; displayName?: string };
|
||||
}
|
||||
| null
|
||||
| undefined;
|
||||
@ -106,6 +110,12 @@ export function Upload() {
|
||||
}>
|
||||
| undefined;
|
||||
const [ownerHandle, setOwnerHandle] = useState("");
|
||||
// Owner migration opt-in: when updating an existing skill under a different
|
||||
// publisher than its current owner, the backend requires an explicit
|
||||
// `migrateOwner: true` signal. We only send it when the user ticks this box,
|
||||
// so a wrong default in the Owner selector cannot silently transfer
|
||||
// ownership.
|
||||
const [confirmMigrateOwner, setConfirmMigrateOwner] = useState(false);
|
||||
const [isDragging, setIsDragging] = useState(false);
|
||||
const fileInputRef = useRef<HTMLInputElement | null>(null);
|
||||
const setFileInputRef = (node: HTMLInputElement | null) => {
|
||||
@ -204,13 +214,21 @@ export function Upload() {
|
||||
|
||||
useEffect(() => {
|
||||
if (ownerHandle) return;
|
||||
// In update mode, default the Owner selector to the skill's current owner
|
||||
// so the New Version flow is a same-owner republish by default and does
|
||||
// not require an ownership-migration opt-in for the common case.
|
||||
const existingOwnerHandle = !isSoulMode ? existing?.owner?.handle : undefined;
|
||||
if (existingOwnerHandle) {
|
||||
setOwnerHandle(existingOwnerHandle);
|
||||
return;
|
||||
}
|
||||
const personalPublisher = publisherMemberships?.find(
|
||||
(entry) => entry.publisher.kind === "user",
|
||||
);
|
||||
if (personalPublisher?.publisher.handle) {
|
||||
setOwnerHandle(personalPublisher.publisher.handle);
|
||||
}
|
||||
}, [ownerHandle, publisherMemberships]);
|
||||
}, [ownerHandle, publisherMemberships, existing, isSoulMode]);
|
||||
|
||||
useEffect(() => {
|
||||
if (changelogTouchedRef.current) return;
|
||||
@ -267,6 +285,22 @@ export function Upload() {
|
||||
trimmedSlug,
|
||||
trimmedVersion,
|
||||
]);
|
||||
// Detect ownership migration intent. We only treat it as a migration when:
|
||||
// * updating an existing skill (`updateSlug` + loaded existing),
|
||||
// * the caller has picked a different Owner than the skill currently has,
|
||||
// * not in soul mode (souls don't carry a publisher owner).
|
||||
// The submit button is disabled until the user ticks the explicit
|
||||
// `confirmMigrateOwner` checkbox, mirroring the backend's `migrateOwner`
|
||||
// contract.
|
||||
const existingOwnerHandle = !isSoulMode ? (existing?.owner?.handle ?? null) : null;
|
||||
const isOwnerMigration = Boolean(
|
||||
!isSoulMode &&
|
||||
updateSlug &&
|
||||
existingOwnerHandle &&
|
||||
ownerHandle &&
|
||||
ownerHandle !== existingOwnerHandle,
|
||||
);
|
||||
const effectiveSlugCollision = isOwnerMigration && confirmMigrateOwner ? null : slugCollision;
|
||||
const parsedTags = useMemo(
|
||||
() =>
|
||||
tags
|
||||
@ -300,6 +334,11 @@ export function Upload() {
|
||||
if (!hasRequiredFile) {
|
||||
issues.push(`${requiredFileLabel} is required.`);
|
||||
}
|
||||
if (isOwnerMigration && !confirmMigrateOwner) {
|
||||
issues.push(
|
||||
`Confirm the ownership move from @${existingOwnerHandle} to @${ownerHandle} to publish.`,
|
||||
);
|
||||
}
|
||||
const invalidFiles = files.filter((file) => !isTextFile(file));
|
||||
if (invalidFiles.length > 0) {
|
||||
issues.push(
|
||||
@ -315,8 +354,8 @@ export function Upload() {
|
||||
if (totalBytes > MAX_PUBLISH_TOTAL_BYTES) {
|
||||
issues.push("Total file size exceeds 50MB.");
|
||||
}
|
||||
if (slugCollision) {
|
||||
issues.push(slugCollision.message);
|
||||
if (effectiveSlugCollision) {
|
||||
issues.push(effectiveSlugCollision.message);
|
||||
}
|
||||
return {
|
||||
issues,
|
||||
@ -335,7 +374,11 @@ export function Upload() {
|
||||
oversizedFiles.length,
|
||||
oversizedFileNames,
|
||||
requiredFileLabel,
|
||||
slugCollision,
|
||||
effectiveSlugCollision,
|
||||
isOwnerMigration,
|
||||
confirmMigrateOwner,
|
||||
existingOwnerHandle,
|
||||
ownerHandle,
|
||||
]);
|
||||
|
||||
// webkitdirectory/directory attributes are set via the ref callback (setFileInputRef)
|
||||
@ -371,9 +414,9 @@ export function Upload() {
|
||||
}
|
||||
return;
|
||||
}
|
||||
if (slugCollision) {
|
||||
setError(slugCollision.message);
|
||||
toast.error(slugCollision.message);
|
||||
if (effectiveSlugCollision) {
|
||||
setError(effectiveSlugCollision.message);
|
||||
toast.error(effectiveSlugCollision.message);
|
||||
return;
|
||||
}
|
||||
if (!isSoulMode && !acceptedLicenseTerms) {
|
||||
@ -433,6 +476,10 @@ export function Upload() {
|
||||
try {
|
||||
const result = await publishVersion({
|
||||
ownerHandle: isSoulMode ? undefined : ownerHandle || undefined,
|
||||
// Only propagate the migration opt-in when the user is actually
|
||||
// changing the skill's owner AND has explicitly confirmed the move.
|
||||
// Same-owner republishes must never carry `migrateOwner: true`.
|
||||
migrateOwner: !isSoulMode && isOwnerMigration && confirmMigrateOwner ? true : undefined,
|
||||
slug: trimmedSlug,
|
||||
displayName: trimmedName,
|
||||
version: trimmedVersion,
|
||||
@ -509,7 +556,14 @@ export function Upload() {
|
||||
className="w-full min-h-[44px] rounded-[var(--radius-sm)] border px-3.5 py-[13px] text-[color:var(--ink)] transition-all duration-[180ms] ease-out border-[rgba(29,59,78,0.22)] bg-[rgba(255,255,255,0.94)] focus:outline-none focus:border-[color-mix(in_srgb,var(--accent)_70%,white)] focus:shadow-[0_0_0_3px_color-mix(in_srgb,var(--accent)_22%,transparent)] dark:border-[rgba(255,255,255,0.12)] dark:bg-[rgba(14,28,37,0.84)]"
|
||||
id="ownerHandle"
|
||||
value={ownerHandle}
|
||||
onChange={(event) => setOwnerHandle(event.target.value)}
|
||||
onChange={(event) => {
|
||||
setOwnerHandle(event.target.value);
|
||||
// Reset the migration confirmation any time the Owner
|
||||
// selector changes; the user must re-acknowledge the move
|
||||
// after picking a different target to avoid a stale tick
|
||||
// turning into a silent transfer.
|
||||
setConfirmMigrateOwner(false);
|
||||
}}
|
||||
>
|
||||
{(publisherMemberships ?? []).map((entry) => (
|
||||
<option key={entry.publisher._id} value={entry.publisher.handle}>
|
||||
@ -517,6 +571,22 @@ export function Upload() {
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
{isOwnerMigration ? (
|
||||
<label className="flex items-start gap-2 text-sm cursor-pointer mt-2">
|
||||
<input
|
||||
type="checkbox"
|
||||
className="mt-0.5"
|
||||
checked={confirmMigrateOwner}
|
||||
onChange={(event) => setConfirmMigrateOwner(event.target.checked)}
|
||||
/>
|
||||
<span>
|
||||
Move ownership of <strong>{trimmedSlug || "this skill"}</strong> from{" "}
|
||||
<strong>@{existingOwnerHandle}</strong> to <strong>@{ownerHandle}</strong>.
|
||||
Versions, tags, stats, comments and stars are preserved; the old URL
|
||||
redirects to the new one.
|
||||
</span>
|
||||
</label>
|
||||
) : null}
|
||||
</>
|
||||
) : null}
|
||||
|
||||
@ -631,14 +701,14 @@ export function Upload() {
|
||||
))}
|
||||
</ul>
|
||||
)}
|
||||
{slugCollision?.url ? (
|
||||
{effectiveSlugCollision?.url ? (
|
||||
<div className="text-sm text-[color:var(--ink-soft)]">
|
||||
Existing skill:{" "}
|
||||
<a
|
||||
href={slugCollision.url}
|
||||
href={effectiveSlugCollision.url}
|
||||
className="text-[color:var(--accent)] hover:underline"
|
||||
>
|
||||
{slugCollision.url}
|
||||
{effectiveSlugCollision.url}
|
||||
</a>
|
||||
</div>
|
||||
) : null}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user