diff --git a/CHANGELOG.md b/CHANGELOG.md index c76c2075..3da23cbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ ### Fixes - API: fix `GET /api/v1/skills` pagination so `cursor` advances to the next page instead of repeating the first page for supported non-trending sorts (#2275) (thanks @vyctorbrzezowski, @enerj). +- Web: block collaborative membership on personal publishers while allowing the linked owner to clean up stale extra membership rows (thanks @vyctorbrzezowski). - Security/API: hide owned package/plugin catalog entries, revoke package publish tokens, and restore only matching ban-hidden packages on user unban (thanks @vyctorbrzezowski). - API: block public raw skill files when moderation already blocks downloads and reject skill tags that point at another skill's version (thanks @vyctorbrzezowski). - Web: stop stale unban restore batches from reactivating skills after the owner is banned again or deactivated (thanks @vyctorbrzezowski). diff --git a/convex/lib/publishers.ts b/convex/lib/publishers.ts index fc401ee4..c1e4e06b 100644 --- a/convex/lib/publishers.ts +++ b/convex/lib/publishers.ts @@ -127,7 +127,16 @@ export async function assertCanManageOwnedResource( } const publisher = await ctx.db.get(params.ownerPublisherId); - if (publisher?.kind === "user" && publisher.linkedUserId === params.actor._id) return; + if (publisher?.kind === "user") { + if (publisher.linkedUserId) { + if (publisher.linkedUserId === params.actor._id) return; + throw new ConvexError("Forbidden"); + } + // Compatibility for legacy personal publishers created before linkedUserId. + // Only fall back to resource ownership while the publisher has no link. + if (params.ownerUserId === params.actor._id) return; + throw new ConvexError("Forbidden"); + } const membership = await getPublisherMembership(ctx, params.ownerPublisherId, params.actor._id); if ( @@ -475,6 +484,28 @@ export async function getPublisherMembership( } } +export async function canAccessPublisherOwnerScope( + ctx: DbCtx, + params: { + publisher: Doc<"publishers"> | null | undefined; + userId: Id<"users">; + allowedPublisherRoles?: PublisherRole[]; + legacyOwnerUserId?: Id<"users">; + }, +) { + const publisher = params.publisher; + if (!publisher || !isPublisherActive(publisher)) return false; + if (publisher.kind === "user") { + if (publisher.linkedUserId) return publisher.linkedUserId === params.userId; + return params.legacyOwnerUserId === params.userId; + } + const membership = await getPublisherMembership(ctx, publisher._id, params.userId); + return Boolean( + membership && + isPublisherRoleAllowed(membership.role, params.allowedPublisherRoles ?? ["publisher"]), + ); +} + export async function requirePublisherRole( ctx: DbCtx, params: { @@ -484,7 +515,14 @@ export async function requirePublisherRole( }, ) { const publisher = await ctx.db.get(params.publisherId); - if (!isPublisherActive(publisher)) throw new ConvexError("Publisher not found"); + if (!publisher || !isPublisherActive(publisher)) throw new ConvexError("Publisher not found"); + if (publisher.kind === "user") { + if (publisher.linkedUserId !== params.userId) { + throw new ConvexError("Forbidden"); + } + const membership = await getPublisherMembership(ctx, params.publisherId, params.userId); + return { publisher, membership }; + } const membership = await getPublisherMembership(ctx, params.publisherId, params.userId); if (!membership || !isPublisherRoleAllowed(membership.role, params.allowed)) { throw new ConvexError("Forbidden"); @@ -514,6 +552,10 @@ export async function resolvePublisherForActor( if (!publisher || !isPublisherActive(publisher)) { throw new ConvexError(`Publisher "@${requestedHandle}" not found`); } + if (publisher.kind === "user") { + if (publisher.linkedUserId === params.actor._id) return publisher; + throw new ConvexError(`You do not have publish access for "@${requestedHandle}"`); + } const membership = await getPublisherMembership(ctx, publisher._id, params.actor._id); if (!membership || !isPublisherRoleAllowed(membership.role, params.allowed)) { throw new ConvexError(`You do not have publish access for "@${requestedHandle}"`); diff --git a/convex/packages.public.test.ts b/convex/packages.public.test.ts index 5f9de9c2..52963b25 100644 --- a/convex/packages.public.test.ts +++ b/convex/packages.public.test.ts @@ -14,6 +14,7 @@ import { publishPackageForUserInternal, listPackageReportsInternal, getPackageModerationStatusForUserInternal, + getClawScanNoteSettings, reportPackageForUserInternal, triagePackageReportForUserInternal, submitPackageAppealForUserInternal, @@ -343,6 +344,12 @@ const getPackageModerationStatusForUserInternalHandler = ( } > )._handler; +const getClawScanNoteSettingsHandler = ( + getClawScanNoteSettings as unknown as WrappedHandler< + { name: string; candidateNames?: string[] }, + { package: { name: string }; latestRelease: { version: string } } | null + > +)._handler; const submitPackageAppealForUserInternalHandler = ( submitPackageAppealForUserInternal as unknown as WrappedHandler< { @@ -733,6 +740,7 @@ function makeDigestCtx(options: { }>; exactPackages?: Array>; exactDigests?: Array>; + publisherDocs?: Record>; publisherMemberships?: Record; }) { const pageByTable = new Map< @@ -839,6 +847,11 @@ function makeDigestCtx(options: { take, ctx: { db: { + get: vi.fn(async (id: string) => { + if (options.publisherDocs?.[id]) return options.publisherDocs[id]; + if (options.publisherMemberships?.[id]) return { _id: id, kind: "org" }; + return null; + }), query: vi.fn((table: string) => { if (table === "packages") { return { @@ -1277,6 +1290,7 @@ function makeTransferPackageOwnerCtx(options?: { function makeUserTransferPackageOwnerCtx(options?: { pkg?: Record | null; actor?: Record | null; + sourcePublisher?: Record | null; destinationPublisher?: Record | null; sourceMembershipRole?: "owner" | "admin" | "publisher" | null; destinationMembershipRole?: "owner" | "admin" | "publisher" | null; @@ -1315,6 +1329,7 @@ function makeUserTransferPackageOwnerCtx(options?: { get: vi.fn(async (id: string) => { if (id === "users:vincent") return actor; if (id === "publishers:vincent") { + if (options?.sourcePublisher !== undefined) return options.sourcePublisher; return { _id: id, kind: "user", @@ -1717,6 +1732,148 @@ describe("packages public queries", () => { expect(result.page.map((entry) => entry.name)).toEqual(["secret-plugin", "public-plugin"]); }); + it("does not let stale personal ownerUserId expose private package digests", async () => { + const { ctx } = makeDigestCtx({ + pages: [ + { + page: [ + makeDigest("stale-personal-secret", { + channel: "private", + ownerKind: "user", + ownerUserId: "users:viewer", + ownerPublisherId: "publishers:other-personal", + }), + makeDigest("public-plugin"), + ], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await listPageForViewerInternalHandler(ctx, { + paginationOpts: { cursor: null, numItems: 10 }, + viewerUserId: "users:viewer", + }); + + expect(result.page.map((entry) => entry.name)).toEqual(["public-plugin"]); + }); + + it("lets legacy no-link personal package owners list private package digests", async () => { + const { ctx } = makeDigestCtx({ + publisherDocs: { + "publishers:legacy-personal": { + _id: "publishers:legacy-personal", + kind: "user", + handle: "viewer", + linkedUserId: undefined, + }, + }, + pages: [ + { + page: [ + makeDigest("legacy-personal-secret", { + channel: "private", + ownerKind: "user", + ownerUserId: "users:viewer", + ownerPublisherId: "publishers:legacy-personal", + }), + makeDigest("public-plugin"), + ], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await listPageForViewerInternalHandler(ctx, { + paginationOpts: { cursor: null, numItems: 10 }, + viewerUserId: "users:viewer", + }); + + expect(result.page.map((entry) => entry.name)).toEqual([ + "legacy-personal-secret", + "public-plugin", + ]); + }); + + it("does not let inactive no-link personal publishers expose private package digests", async () => { + const { ctx } = makeDigestCtx({ + publisherDocs: { + "publishers:legacy-personal": { + _id: "publishers:legacy-personal", + kind: "user", + handle: "viewer", + linkedUserId: undefined, + deactivatedAt: 123, + }, + }, + pages: [ + { + page: [ + makeDigest("legacy-personal-secret", { + channel: "private", + ownerKind: "user", + ownerUserId: "users:viewer", + ownerPublisherId: "publishers:legacy-personal", + }), + makeDigest("public-plugin"), + ], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await listPageForViewerInternalHandler(ctx, { + paginationOpts: { cursor: null, numItems: 10 }, + viewerUserId: "users:viewer", + }); + + expect(result.page.map((entry) => entry.name)).toEqual(["public-plugin"]); + }); + + it("does not reuse legacy no-link personal access across package owners", async () => { + const { ctx } = makeDigestCtx({ + publisherDocs: { + "publishers:legacy-personal": { + _id: "publishers:legacy-personal", + kind: "user", + handle: "viewer", + linkedUserId: undefined, + }, + }, + pages: [ + { + page: [ + makeDigest("own-legacy-secret", { + channel: "private", + ownerKind: "user", + ownerUserId: "users:viewer", + ownerPublisherId: "publishers:legacy-personal", + }), + makeDigest("stale-legacy-secret", { + channel: "private", + ownerKind: "user", + ownerUserId: "users:other", + ownerPublisherId: "publishers:legacy-personal", + }), + makeDigest("public-plugin"), + ], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await listPageForViewerInternalHandler(ctx, { + paginationOpts: { cursor: null, numItems: 10 }, + viewerUserId: "users:viewer", + }); + + expect(result.page.map((entry) => entry.name)).toEqual(["own-legacy-secret", "public-plugin"]); + }); + it("allows owners to filter to only their private packages", async () => { const { ctx, indexNames } = makeDigestCtx({ pages: [ @@ -3378,6 +3535,105 @@ describe("packages public queries", () => { ).rejects.toThrow("Forbidden"); }); + it("rejects user package transfers through stale personal-publisher memberships", async () => { + const { ctx } = makeUserTransferPackageOwnerCtx({ + pkg: makePackageDoc({ + name: "@owner/demo", + normalizedName: "@owner/demo", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:vincent", + }), + sourcePublisher: { + _id: "publishers:vincent", + kind: "user", + handle: "owner", + linkedUserId: "users:owner", + }, + sourceMembershipRole: "admin", + destinationMembershipRole: "owner", + }); + + await expect( + transferPackageOwnerForUserInternalHandler(ctx, { + actorUserId: "users:vincent", + name: "demo-plugin", + toOwner: "opik", + }), + ).rejects.toThrow("Forbidden"); + }); + + it("lets owners transfer packages from legacy personal publishers without linked users", async () => { + const { ctx } = makeUserTransferPackageOwnerCtx({ + sourcePublisher: { + _id: "publishers:vincent", + kind: "user", + handle: "vincentkoc", + linkedUserId: undefined, + }, + sourceMembershipRole: null, + destinationMembershipRole: "owner", + }); + + await expect( + transferPackageOwnerForUserInternalHandler(ctx, { + actorUserId: "users:vincent", + name: "@opik/opik-openclaw", + toOwner: "opik", + }), + ).resolves.toMatchObject({ + ok: true, + ownerUserId: "users:vincent", + ownerPublisherId: "publishers:opik", + }); + }); + + it("rejects package transfers through stale no-link personal memberships", async () => { + const { ctx } = makeUserTransferPackageOwnerCtx({ + pkg: makePackageDoc({ + name: "@owner/demo", + normalizedName: "@owner/demo", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:vincent", + }), + sourcePublisher: { + _id: "publishers:vincent", + kind: "user", + handle: "owner", + linkedUserId: undefined, + }, + sourceMembershipRole: "admin", + destinationMembershipRole: "owner", + }); + + await expect( + transferPackageOwnerForUserInternalHandler(ctx, { + actorUserId: "users:vincent", + name: "demo-plugin", + toOwner: "opik", + }), + ).rejects.toThrow("Forbidden"); + }); + + it("rejects package transfer destinations through stale no-link personal memberships", async () => { + const { ctx } = makeUserTransferPackageOwnerCtx({ + destinationPublisher: { + _id: "publishers:opik", + kind: "user", + handle: "opik", + linkedUserId: undefined, + }, + destinationMembershipRole: "admin", + }); + + await expect( + transferPackageOwnerForUserInternalHandler(ctx, { + actorUserId: "users:vincent", + name: "@opik/opik-openclaw", + toOwner: "opik", + }), + ).rejects.toThrow('admin access for "@opik"'); + }); + it("rejects user package transfers without destination admin access", async () => { const { ctx } = makeUserTransferPackageOwnerCtx({ destinationMembershipRole: "publisher" }); @@ -5599,12 +5855,176 @@ describe("packages public queries", () => { ]); }); + it("lists packages for the viewer's legacy no-link personal publisher dashboard", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const result = await listHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "packageReleases:demo-1") return makeReleaseDoc({ version: "1.0.0" }); + if (id === "packageReleases:legacy-1") { + return makeReleaseDoc({ + _id: "packageReleases:legacy-1", + packageId: "packages:legacy-direct", + version: "1.0.0", + }); + } + if (id === "users:owner") { + return { + _id: "users:owner", + handle: "owner", + personalPublisherId: "publishers:owner", + }; + } + if (id === "publishers:owner") { + return { + _id: "publishers:owner", + kind: "user", + linkedUserId: undefined, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn((indexName: string) => { + if (indexName === "by_owner_publisher") { + return { + order: vi.fn(() => ({ + take: vi + .fn() + .mockResolvedValue([ + makePackageDoc({ ownerPublisherId: "publishers:owner" }), + ]), + })), + }; + } + if (indexName === "by_owner") { + return { + order: vi.fn(() => ({ + take: vi.fn().mockResolvedValue([ + makePackageDoc({ + _id: "packages:legacy-direct", + name: "legacy-direct-plugin", + normalizedName: "legacy-direct-plugin", + displayName: "Legacy Direct Plugin", + ownerPublisherId: undefined, + latestReleaseId: "packageReleases:legacy-1", + }), + ]), + })), + }; + } + throw new Error(`Unexpected index ${indexName}`); + }), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(null), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { ownerPublisherId: "publishers:owner", limit: 20 }, + ); + + expect(result).toEqual([ + expect.objectContaining({ + name: "demo-plugin", + ownerPublisherId: "publishers:owner", + }), + expect.objectContaining({ + name: "legacy-direct-plugin", + ownerPublisherId: undefined, + }), + ]); + }); + + it("keeps stale publisher-owned package rows out of owner-user dashboards", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const result = await listHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") { + return { _id: "users:owner", handle: "owner" }; + } + if (id === "publishers:other-personal") { + return { + _id: "publishers:other-personal", + kind: "user", + linkedUserId: "users:other", + }; + } + if (id === "publishers:org") { + return { _id: "publishers:org", kind: "org" }; + } + if (id === "packageReleases:legacy-1") { + return makeReleaseDoc({ + _id: "packageReleases:legacy-1", + packageId: "packages:legacy-direct", + version: "1.0.0", + }); + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn((indexName: string) => { + if (indexName !== "by_owner") throw new Error(`Unexpected index ${indexName}`); + return { + order: vi.fn(() => ({ + take: vi.fn().mockResolvedValue([ + makePackageDoc({ + _id: "packages:other-personal", + name: "other-personal-plugin", + ownerPublisherId: "publishers:other-personal", + }), + makePackageDoc({ + _id: "packages:org", + name: "org-plugin", + ownerPublisherId: "publishers:org", + }), + makePackageDoc({ + _id: "packages:legacy-direct", + name: "legacy-direct-plugin", + normalizedName: "legacy-direct-plugin", + displayName: "Legacy Direct Plugin", + ownerPublisherId: undefined, + latestReleaseId: "packageReleases:legacy-1", + }), + ]), + })), + }; + }), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { ownerUserId: "users:owner", limit: 20 }, + ); + + expect(result.map((entry) => entry.name)).toEqual(["legacy-direct-plugin"]); + }); + it("returns no owner packages when the viewer lacks access", async () => { vi.mocked(getAuthUserId).mockResolvedValue("users:stranger" as never); const result = await listHandler( { db: { get: vi.fn(async (id: string) => { + if (id === "users:stranger") { + return { _id: "users:stranger", handle: "stranger", displayName: "Stranger" }; + } if (id === "publishers:owner") { return { _id: "publishers:owner", @@ -5632,6 +6052,110 @@ describe("packages public queries", () => { expect(result).toEqual([]); }); + it("ignores stale personal memberships for package dashboards", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:stranger" as never); + const result = await listHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "publishers:owner") { + return { + _id: "publishers:owner", + kind: "user", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn(() => ({ + order: vi.fn(() => ({ + take: vi + .fn() + .mockResolvedValue([ + makePackageDoc({ ownerPublisherId: "publishers:owner" }), + ]), + })), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:stranger", + role: "owner", + }), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { ownerPublisherId: "publishers:owner", limit: 20 }, + ); + + expect(result).toEqual([]); + }); + + it("keeps org memberships authorized for package dashboards", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:member" as never); + const result = await listHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:member") { + return { _id: "users:member", handle: "member", displayName: "Member" }; + } + if (id === "publishers:org") { + return { + _id: "publishers:org", + kind: "org", + handle: "team", + displayName: "Team", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn(() => ({ + order: vi.fn(() => ({ + take: vi + .fn() + .mockResolvedValue([makePackageDoc({ ownerPublisherId: "publishers:org" })]), + })), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:member", + publisherId: "publishers:org", + userId: "users:member", + role: "publisher", + }), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { ownerPublisherId: "publishers:org", limit: 20 }, + ); + + expect(result).toEqual([expect.objectContaining({ name: "demo-plugin" })]); + }); + it("requires auth inside the public publish action", async () => { await expect( publishPackageHandler({ runQuery: vi.fn(), runMutation: vi.fn() } as never, { @@ -6085,6 +6609,46 @@ describe("packages public queries", () => { }); }); + it("does not let stale personal ownerUserId read package moderation status", async () => { + await expect( + getPackageModerationStatusForUserInternalHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:viewer") return { _id: id, role: "user" }; + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue( + makePackageDoc({ + name: "@scope/demo", + ownerKind: "user", + ownerUserId: "users:viewer", + ownerPublisherId: "publishers:other-personal", + }), + ), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(null), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { actorUserId: "users:viewer", name: "@scope/demo" }, + ), + ).rejects.toThrow("Unauthorized"); + }); + it("submits owner appeals for quarantined package releases", async () => { const insert = vi.fn(async (table: string) => table === "packageAppeals" ? "packageAppeals:1" : "auditLogs:1", @@ -6177,6 +6741,126 @@ describe("packages public queries", () => { ); }); + it("does not let stale personal-publisher memberships submit package appeals", async () => { + const insert = vi.fn(); + + await expect( + submitPackageAppealForUserInternalHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:stale-member") return { _id: id, role: "user" }; + if (id === "publishers:owner") { + return { + _id: id, + kind: "user", + handle: "owner", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue( + makePackageDoc({ + name: "@scope/demo", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:owner", + }), + ), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:stale-member", + role: "admin", + }), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + insert, + patch: vi.fn(), + replace: vi.fn(), + delete: vi.fn(), + normalizeId: vi.fn(), + }, + } as never, + { + actorUserId: "users:stale-member", + name: "@scope/demo", + version: "1.2.3", + message: "please review", + }, + ), + ).rejects.toThrow("Unauthorized"); + + expect(insert).not.toHaveBeenCalled(); + }); + + it("does not let stale personal-publisher memberships read owner-only scan settings", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:stale-member" as never); + + const result = await getClawScanNoteSettingsHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:stale-member") return { _id: id, role: "user" }; + if (id === "publishers:owner") { + return { + _id: id, + kind: "user", + handle: "owner", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "packages") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue( + makePackageDoc({ + name: "@scope/demo", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:owner", + }), + ), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:stale-member", + role: "admin", + }), + })), + }; + } + throw new Error(`Unexpected table ${table}`); + }), + }, + } as never, + { name: "@scope/demo" }, + ); + + expect(result).toBeNull(); + }); + it("lists package appeals for moderators", async () => { const result = await listPackageAppealsInternalHandler( { diff --git a/convex/packages.ts b/convex/packages.ts index 9fe2e029..8f5e2004 100644 --- a/convex/packages.ts +++ b/convex/packages.ts @@ -68,10 +68,12 @@ import { import { toPublicPublisher } from "./lib/public"; import { assertCanManageOwnedResource, + canAccessPublisherOwnerScope, getPublisherByHandle, getPersonalPublisherForUser, getOwnerPublisher, getPublisherMembership, + isPublisherActive, isPublisherRoleAllowed, normalizePublisherHandle, } from "./lib/publishers"; @@ -587,6 +589,8 @@ type PackageDigestLike = Pick< capabilityTag?: string; pluginCategory?: string; }; +type PackageOwnerAccessRef = Pick & + Partial>; type PublicPageCursorState = { cursor: string | null; offset: number; @@ -690,46 +694,51 @@ function requiresPrivilegedPackageAccess( async function viewerCanAccessPackageOwner( ctx: DbReaderCtx, - digest: Pick, + digest: PackageOwnerAccessRef, viewerUserId: Id<"users"> | undefined, membershipCache?: Map>, ) { if (!viewerUserId) return false; if (!digest.ownerPublisherId) return digest.ownerUserId === viewerUserId; - const cacheKey = String(digest.ownerPublisherId); + const ownerPublisherId = digest.ownerPublisherId; + const cacheKey = `${ownerPublisherId}:${digest.ownerUserId}`; const cached = membershipCache?.get(cacheKey); if (cached) return await cached; - const membershipPromise = getPublisherMembership(ctx, digest.ownerPublisherId, viewerUserId).then( - Boolean, - ); + const membershipPromise = (async () => { + const ownerPublisher = await ctx.db.get(ownerPublisherId); + return await canAccessPublisherOwnerScope(ctx, { + publisher: ownerPublisher, + userId: viewerUserId, + legacyOwnerUserId: digest.ownerUserId, + }); + })(); membershipCache?.set(cacheKey, membershipPromise); - if (await membershipPromise) return true; - - if (digest.ownerUserId !== viewerUserId) return false; - const ownerPublisher = await ctx.db.get(digest.ownerPublisherId); - return ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId === viewerUserId; + return await membershipPromise; } async function viewerCanManagePackageOwner( ctx: DbReaderCtx, - digest: Pick, + digest: PackageOwnerAccessRef, viewerUserId: Id<"users"> | undefined, ) { if (!viewerUserId) return false; if (!digest.ownerPublisherId) return digest.ownerUserId === viewerUserId; const ownerPublisher = await ctx.db.get(digest.ownerPublisherId); - if (ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId === viewerUserId) return true; - - const membership = await getPublisherMembership(ctx, digest.ownerPublisherId, viewerUserId); - return Boolean(membership && isPublisherRoleAllowed(membership.role, ["admin"])); + return await canAccessPublisherOwnerScope(ctx, { + publisher: ownerPublisher, + userId: viewerUserId, + allowedPublisherRoles: ["admin"], + legacyOwnerUserId: digest.ownerUserId, + }); } async function canViewerReadPackage( ctx: DbReaderCtx, - digest: Pick, + digest: Pick & + Partial>, viewerUserId: Id<"users"> | undefined, membershipCache?: Map>, ) { @@ -1023,16 +1032,19 @@ async function listDashboardPackagesForOwnerPublisher( ) { const takeLimit = Math.min(limit * 5, 500); const ownerPublisher = await ctx.db.get(ownerPublisherId); - const membership = - (await ctx.db - .query("publisherMembers") - .withIndex("by_publisher_user", (q) => - q.eq("publisherId", ownerPublisherId).eq("userId", viewerUserId), - ) - .unique()) ?? null; - const isOwnDashboard = Boolean( - membership || (ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId === viewerUserId), - ); + const owner = + ownerPublisher?.kind === "user" && !ownerPublisher.linkedUserId + ? await ctx.db.get(viewerUserId) + : null; + const isOwnDashboard = + (await canAccessPublisherOwnerScope(ctx, { + publisher: ownerPublisher, + userId: viewerUserId, + })) || + (ownerPublisher?.kind === "user" && + isPublisherActive(ownerPublisher) && + !ownerPublisher.linkedUserId && + owner?.personalPublisherId === ownerPublisherId); if (!isOwnDashboard) return []; const scopedEntries = await ctx.db @@ -1040,14 +1052,17 @@ async function listDashboardPackagesForOwnerPublisher( .withIndex("by_owner_publisher", (q) => q.eq("ownerPublisherId", ownerPublisherId)) .order("desc") .take(takeLimit); - const legacyEntries = - ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId - ? await ctx.db - .query("packages") - .withIndex("by_owner", (q) => q.eq("ownerUserId", ownerPublisher.linkedUserId!)) - .order("desc") - .take(takeLimit) - : []; + const legacyPersonalOwnerUserId = + ownerPublisher?.kind === "user" + ? (ownerPublisher.linkedUserId ?? (isOwnDashboard ? viewerUserId : undefined)) + : undefined; + const legacyEntries = legacyPersonalOwnerUserId + ? await ctx.db + .query("packages") + .withIndex("by_owner", (q) => q.eq("ownerUserId", legacyPersonalOwnerUserId)) + .order("desc") + .take(takeLimit) + : []; const combined = [...scopedEntries, ...legacyEntries].filter( (pkg, index, all) => @@ -1061,6 +1076,33 @@ async function listDashboardPackagesForOwnerPublisher( ).filter((pkg): pkg is DashboardPackageListItem => Boolean(pkg)); } +async function packageBelongsToOwnerUserDashboardScope( + ctx: Pick, + pkg: Pick, "ownerUserId" | "ownerPublisherId">, + ownerUserId: Id<"users">, +) { + if (pkg.ownerUserId !== ownerUserId) return false; + if (!pkg.ownerPublisherId) return true; + const ownerPublisher = await ctx.db.get(pkg.ownerPublisherId); + if (!ownerPublisher || !isPublisherActive(ownerPublisher) || ownerPublisher.kind !== "user") { + return false; + } + return ownerPublisher.linkedUserId ? ownerPublisher.linkedUserId === ownerUserId : true; +} + +async function filterPackagesForOwnerUserDashboard( + ctx: Pick, + packages: Doc<"packages">[], + ownerUserId: Id<"users">, +) { + const scoped = await Promise.all( + packages.map(async (pkg) => + (await packageBelongsToOwnerUserDashboardScope(ctx, pkg, ownerUserId)) ? pkg : null, + ), + ); + return scoped.filter((pkg): pkg is Doc<"packages"> => Boolean(pkg)); +} + async function listDashboardPackagesForOwnerUser( ctx: QueryCtx, ownerUserId: Id<"users">, @@ -1074,7 +1116,8 @@ async function listDashboardPackagesForOwnerUser( .withIndex("by_owner", (q) => q.eq("ownerUserId", ownerUserId)) .order("desc") .take(takeLimit); - const filtered = entries.filter((pkg) => !pkg.softDeletedAt).slice(0, limit); + const scoped = await filterPackagesForOwnerUserDashboard(ctx, entries, ownerUserId); + const filtered = scoped.filter((pkg) => !pkg.softDeletedAt).slice(0, limit); return ( await Promise.all(filtered.map(async (pkg) => await toDashboardPackageListItem(ctx, pkg))) ).filter((pkg): pkg is DashboardPackageListItem => Boolean(pkg)); @@ -5656,10 +5699,16 @@ async function transferPackageOwnerForUser( if (pkg.ownerPublisherId) { const sourcePublisher = await ctx.db.get(pkg.ownerPublisherId); const sourceMembership = await getPublisherMembership(ctx, pkg.ownerPublisherId, actor._id); + const canManagePersonalSource = + sourcePublisher?.kind === "user" && + (sourcePublisher.linkedUserId + ? sourcePublisher.linkedUserId === actor._id + : pkg.ownerUserId === actor._id); const canManageSource = actor.role === "admin" || - sourcePublisher?.linkedUserId === actor._id || - Boolean(sourceMembership && isPublisherRoleAllowed(sourceMembership.role, ["admin"])); + (sourcePublisher?.kind === "user" + ? canManagePersonalSource + : Boolean(sourceMembership && isPublisherRoleAllowed(sourceMembership.role, ["admin"]))); if (!canManageSource) { throw new ConvexError("Forbidden"); } @@ -5689,10 +5738,18 @@ async function transferPackageOwnerForUser( destinationPublisher._id, actor._id, ); + const canManagePersonalDestination = + destinationPublisher.kind === "user" && + (destinationPublisher.linkedUserId + ? destinationPublisher.linkedUserId === actor._id + : actor.personalPublisherId === destinationPublisher._id); const canManageDestination = actor.role === "admin" || - destinationPublisher.linkedUserId === actor._id || - Boolean(destinationMembership && isPublisherRoleAllowed(destinationMembership.role, ["admin"])); + (destinationPublisher.kind === "user" + ? canManagePersonalDestination + : Boolean( + destinationMembership && isPublisherRoleAllowed(destinationMembership.role, ["admin"]), + )); if (!canManageDestination) { throw new ConvexError( `You do not have admin access for "@${destinationHandle}". Ask an owner or admin to add you before transferring this package.`, diff --git a/convex/publishers.test.ts b/convex/publishers.test.ts index 23006cf8..b305c51f 100644 --- a/convex/publishers.test.ts +++ b/convex/publishers.test.ts @@ -1,6 +1,6 @@ import { getAuthUserId } from "@convex-dev/auth/server"; import { describe, expect, it, vi } from "vitest"; -import { assertCanManageOwnedResource } from "./lib/publishers"; +import { assertCanManageOwnedResource, requirePublisherRole } from "./lib/publishers"; import { addMember, listPublicPage, @@ -13,6 +13,7 @@ import { createOrg, removeMember, createOrgPublisherForUserInternal, + resolvePublishTargetForUserInternal, setTrustedPublisherInternal, updateProfile, } from "./publishers"; @@ -194,6 +195,22 @@ const createOrgHandler = ( > )._handler; +const resolvePublishTargetForUserInternalHandler = ( + resolvePublishTargetForUserInternal as unknown as WrappedHandler< + { + actorUserId: string; + ownerHandle?: string; + minimumRole?: "owner" | "admin" | "publisher"; + }, + { + publisherId: string; + handle: string; + kind: "user" | "org"; + linkedUserId?: string; + } | null + > +)._handler; + function indexedRows(rows: T[]) { return { collect: vi.fn(async () => rows), @@ -201,7 +218,165 @@ function indexedRows(rows: T[]) { }; } +function makeResolvePublishTargetCtx(options: { + targetPublisher: Record; + targetMembership?: Record | null; +}) { + const actor = { + _id: "users:vincent", + handle: "vincent", + name: "Vincent", + displayName: "Vincent", + image: null, + trustedPublisher: false, + personalPublisherId: "publishers:vincent", + }; + const actorPersonalPublisher = { + _id: "publishers:vincent", + kind: "user", + handle: "vincent", + displayName: "Vincent", + linkedUserId: "users:vincent", + }; + const actorOwnerMembership = { + _id: "publisherMembers:vincent-owner", + publisherId: "publishers:vincent", + userId: "users:vincent", + role: "owner", + }; + const queryValues = ( + builder: (q: { eq: (field: string, value: unknown) => unknown }) => unknown, + ) => { + const values = new Map(); + const q = { + eq(field: string, value: unknown) { + values.set(field, value); + return q; + }, + }; + builder(q); + return values; + }; + const query = vi.fn((table: string) => { + if (table === "publishers") { + return { + withIndex: vi.fn((_indexName: string, builder) => { + const values = queryValues(builder); + return { + unique: vi.fn(async () => { + if (values.get("linkedUserId") === "users:vincent") return actorPersonalPublisher; + if (values.get("handle") === "vincent") return actorPersonalPublisher; + if (values.get("handle") === options.targetPublisher.handle) { + return options.targetPublisher; + } + return null; + }), + }; + }), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn((_indexName: string, builder) => { + const values = queryValues(builder); + return { + unique: vi.fn(async () => { + if ( + values.get("publisherId") === "publishers:vincent" && + values.get("userId") === "users:vincent" + ) { + return actorOwnerMembership; + } + if ( + values.get("publisherId") === options.targetPublisher._id && + values.get("userId") === "users:vincent" + ) { + return options.targetMembership ?? null; + } + return null; + }), + }; + }), + }; + } + throw new Error(`unexpected table ${table}`); + }); + return { + db: { + get: vi.fn(async (tableOrId: string, maybeId?: string) => { + const id = maybeId ?? tableOrId; + if (id === "users:vincent") return actor; + if (id === "publishers:vincent") return actorPersonalPublisher; + if (id === options.targetPublisher._id) return options.targetPublisher; + return null; + }), + query, + patch: vi.fn(), + insert: vi.fn(async () => "auditLogs:resolve"), + delete: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn((table: string, id: string) => (id.startsWith(`${table}:`) ? id : null)), + system: {}, + }, + }; +} + describe("publishers membership controls", () => { + it("does not resolve another personal publisher through a stale membership", async () => { + const ctx = makeResolvePublishTargetCtx({ + targetPublisher: { + _id: "publishers:owner", + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:owner", + }, + targetMembership: { + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:vincent", + role: "publisher", + }, + }); + + await expect( + resolvePublishTargetForUserInternalHandler(ctx as never, { + actorUserId: "users:vincent", + ownerHandle: "owner", + minimumRole: "publisher", + }), + ).rejects.toThrow('publish access for "@owner"'); + }); + + it("keeps org publisher memberships valid for publish target resolution", async () => { + const ctx = makeResolvePublishTargetCtx({ + targetPublisher: { + _id: "publishers:openclaw", + kind: "org", + handle: "openclaw", + displayName: "OpenClaw", + }, + targetMembership: { + _id: "publisherMembers:openclaw", + publisherId: "publishers:openclaw", + userId: "users:vincent", + role: "publisher", + }, + }); + + await expect( + resolvePublishTargetForUserInternalHandler(ctx as never, { + actorUserId: "users:vincent", + ownerHandle: "openclaw", + minimumRole: "publisher", + }), + ).resolves.toMatchObject({ + publisherId: "publishers:openclaw", + handle: "openclaw", + kind: "org", + }); + }); + it("rejects org handles reserved for public routes", async () => { const ctx = { db: { @@ -982,6 +1157,308 @@ describe("publishers membership controls", () => { ).rejects.toThrow("Only org owners can promote members to owner"); }); + it("prevents adding members to personal publishers", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") return { _id: id }; + if (id === "publishers:personal") { + return { + _id: id, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:owner", + publisherId: "publishers:personal", + userId: "users:owner", + role: "owner", + }), + })), + }; + } + throw new Error(`unexpected table ${table}`); + }), + insert: vi.fn(), + patch: vi.fn(), + delete: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + }; + + await expect( + addMemberHandler( + ctx as never, + { publisherId: "publishers:personal", userHandle: "friend", role: "admin" } as never, + ), + ).rejects.toThrow("Personal publishers do not support member management"); + + expect(ctx.db.insert).not.toHaveBeenCalled(); + expect(ctx.db.patch).not.toHaveBeenCalled(); + }); + + it("lets linked owners remove stale members from personal publishers", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") return { _id: id }; + if (id === "publishers:personal") { + return { + _id: id, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:friend", + publisherId: "publishers:personal", + userId: "users:friend", + role: "admin", + }), + })), + }; + } + throw new Error(`unexpected table ${table}`); + }), + delete: vi.fn(), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + }; + + await expect( + removeMemberHandler( + ctx as never, + { publisherId: "publishers:personal", userId: "users:friend" } as never, + ), + ).resolves.toEqual({ ok: true }); + + expect(ctx.db.delete).toHaveBeenCalledWith("publisherMembers:friend"); + expect(ctx.db.insert).toHaveBeenCalledWith( + "auditLogs", + expect.objectContaining({ + action: "publisher.member.remove", + targetId: "publishers:personal", + metadata: { memberUserId: "users:friend" }, + }), + ); + }); + + it("lets legacy no-link personal owners remove stale members by personal publisher link", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const memberships: Record> = { + "users:friend": { + _id: "publisherMembers:friend", + publisherId: "publishers:personal", + userId: "users:friend", + role: "admin", + }, + }; + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") return { _id: id, personalPublisherId: "publishers:personal" }; + if (id === "publishers:personal") { + return { + _id: id, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: undefined, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn( + ( + _indexName: string, + builder: (q: { eq: (field: string, value: string) => unknown }) => unknown, + ) => { + let userId = ""; + const q = { + eq: (field: string, value: string) => { + if (field === "userId") userId = value; + return q; + }, + }; + builder(q); + return { + unique: vi.fn().mockResolvedValue(memberships[userId] ?? null), + }; + }, + ), + }; + } + throw new Error(`unexpected table ${table}`); + }), + delete: vi.fn(), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + }; + + await expect( + removeMemberHandler( + ctx as never, + { publisherId: "publishers:personal", userId: "users:friend" } as never, + ), + ).resolves.toEqual({ ok: true }); + + expect(ctx.db.delete).toHaveBeenCalledWith("publisherMembers:friend"); + }); + + it("lets legacy no-link personal owners remove stale members by owner membership", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const memberships: Record> = { + "users:owner": { + _id: "publisherMembers:owner", + publisherId: "publishers:personal", + userId: "users:owner", + role: "owner", + }, + "users:friend": { + _id: "publisherMembers:friend", + publisherId: "publishers:personal", + userId: "users:friend", + role: "admin", + }, + }; + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") return { _id: id }; + if (id === "publishers:personal") { + return { + _id: id, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: undefined, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn( + ( + _indexName: string, + builder: (q: { eq: (field: string, value: string) => unknown }) => unknown, + ) => { + let userId = ""; + const q = { + eq: (field: string, value: string) => { + if (field === "userId") userId = value; + return q; + }, + }; + builder(q); + return { + unique: vi.fn().mockResolvedValue(memberships[userId] ?? null), + }; + }, + ), + }; + } + throw new Error(`unexpected table ${table}`); + }), + delete: vi.fn(), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + }; + + await expect( + removeMemberHandler( + ctx as never, + { publisherId: "publishers:personal", userId: "users:friend" } as never, + ), + ).resolves.toEqual({ ok: true }); + + expect(ctx.db.delete).toHaveBeenCalledWith("publisherMembers:friend"); + }); + + it("prevents removing the linked owner from personal publishers", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:owner") return { _id: id }; + if (id === "publishers:personal") { + return { + _id: id, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:owner", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:owner", + publisherId: "publishers:personal", + userId: "users:owner", + role: "owner", + }), + })), + }; + } + throw new Error(`unexpected table ${table}`); + }), + delete: vi.fn(), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + }; + + await expect( + removeMemberHandler( + ctx as never, + { publisherId: "publishers:personal", userId: "users:owner" } as never, + ), + ).rejects.toThrow("Personal publisher owner membership cannot be removed"); + + expect(ctx.db.delete).not.toHaveBeenCalled(); + expect(ctx.db.insert).not.toHaveBeenCalled(); + }); + it("prevents removing the last remaining owner", async () => { vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); const ctx = { @@ -1501,6 +1978,112 @@ describe("publisher-owned resource authorization", () => { ), ).resolves.toBeUndefined(); }); + + it("keeps legacy personal publishers without linked users manageable by the resource owner", async () => { + const ctx = makeOwnerResourceCtx({ + publisher: { + _id: "publishers:owner", + kind: "user", + handle: "vincentkoc", + linkedUserId: undefined, + }, + }); + + await expect( + assertCanManageOwnedResource( + ctx as never, + { + actor: { _id: "users:vincent" }, + ownerUserId: "users:vincent", + ownerPublisherId: "publishers:owner", + } as never, + ), + ).resolves.toBeUndefined(); + }); + + it("does not honor extra memberships on personal publishers", async () => { + const ctx = makeOwnerResourceCtx({ + publisher: { + _id: "publishers:owner", + kind: "user", + handle: "vincentkoc", + linkedUserId: "users:vincent", + }, + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:friend", + role: "owner", + }, + }); + + await expect( + assertCanManageOwnedResource( + ctx as never, + { + actor: { _id: "users:friend" }, + ownerUserId: "users:vincent", + ownerPublisherId: "publishers:owner", + } as never, + ), + ).rejects.toThrow("Forbidden"); + }); + + it("does not authorize personal publisher roles for non-linked members", async () => { + const ctx = makeOwnerResourceCtx({ + publisher: { + _id: "publishers:owner", + kind: "user", + handle: "vincentkoc", + linkedUserId: "users:vincent", + }, + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:friend", + role: "owner", + }, + }); + + await expect( + requirePublisherRole( + ctx as never, + { + publisherId: "publishers:owner", + userId: "users:friend", + allowed: ["owner"], + } as never, + ), + ).rejects.toThrow("Forbidden"); + }); + + it("treats linked users as personal publisher owners even with stale membership roles", async () => { + const ctx = makeOwnerResourceCtx({ + publisher: { + _id: "publishers:owner", + kind: "user", + handle: "vincentkoc", + linkedUserId: "users:vincent", + }, + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:vincent", + role: "publisher", + }, + }); + + await expect( + requirePublisherRole( + ctx as never, + { + publisherId: "publishers:owner", + userId: "users:vincent", + allowed: ["admin"], + } as never, + ), + ).resolves.toBeDefined(); + }); }); describe("publisher bootstrap", () => { @@ -1671,6 +2254,127 @@ describe("publisher bootstrap", () => { }), ]); }); + + it("filters stale personal memberships from mine listings", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:friend" as never); + const memberships = [ + { + _id: "publisherMembers:stale-personal", + publisherId: "publishers:owner", + userId: "users:friend", + role: "owner", + }, + { + _id: "publisherMembers:own-personal", + publisherId: "publishers:friend", + userId: "users:friend", + role: "publisher", + }, + { + _id: "publisherMembers:team", + publisherId: "publishers:team", + userId: "users:friend", + role: "admin", + }, + ]; + const publishers = { + "publishers:owner": { + _id: "publishers:owner", + _creationTime: 1, + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:owner", + trustedPublisher: false, + createdAt: 1, + updatedAt: 1, + }, + "publishers:friend": { + _id: "publishers:friend", + _creationTime: 1, + kind: "user", + handle: "friend", + displayName: "Friend", + linkedUserId: "users:friend", + trustedPublisher: false, + createdAt: 1, + updatedAt: 1, + }, + "publishers:team": { + _id: "publishers:team", + _creationTime: 1, + kind: "org", + handle: "team", + displayName: "Team", + trustedPublisher: false, + createdAt: 1, + updatedAt: 1, + }, + }; + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:friend") { + return { + _id: id, + _creationTime: 1, + handle: "friend", + displayName: "Friend", + personalPublisherId: "publishers:friend", + trustedPublisher: false, + createdAt: 1, + updatedAt: 1, + }; + } + return publishers[id as keyof typeof publishers] ?? null; + }), + query: vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn((indexName: string) => { + if (indexName !== "by_user") throw new Error(`unexpected index ${indexName}`); + return { collect: vi.fn().mockResolvedValue(memberships) }; + }), + }; + } + if (table === "publishers") { + return { + withIndex: vi.fn((indexName: string) => { + if (indexName === "by_handle") return { unique: vi.fn().mockResolvedValue(null) }; + if (indexName !== "by_linked_user") { + throw new Error(`unexpected index ${indexName}`); + } + return { unique: vi.fn().mockResolvedValue(null) }; + }), + }; + } + throw new Error(`unexpected table ${table}`); + }), + }, + }; + + const result = await listMineHandler(ctx as never, {} as never); + + expect(result).toEqual([ + expect.objectContaining({ + role: "owner", + publisher: expect.objectContaining({ + _id: "publishers:friend", + handle: "friend", + kind: "user", + linkedUserId: "users:friend", + }), + }), + expect.objectContaining({ + role: "admin", + publisher: expect.objectContaining({ + _id: "publishers:team", + handle: "team", + kind: "org", + }), + }), + ]); + }); }); describe("self-serve org publisher creation", () => { diff --git a/convex/publishers.ts b/convex/publishers.ts index 5dea7b61..75327bf4 100644 --- a/convex/publishers.ts +++ b/convex/publishers.ts @@ -11,6 +11,7 @@ import { isReservedPublicOwnerHandle, } from "./lib/publicRouteReservations"; import { + canAccessPublisherOwnerScope, ensurePersonalPublisherForUser, getActiveUserByHandleOrPersonalPublisher, getPublisherByHandle, @@ -26,6 +27,11 @@ import { readCanonicalStat } from "./lib/skillStats"; const PUBLISHER_HANDLE_PATTERN = /^[a-z0-9](?:[a-z0-9-]{0,38}[a-z0-9])?$/; const MAX_PUBLIC_PUBLISHER_LIST_LIMIT = 500; const PUBLISHER_LIST_PREVIEW_LIMIT = 3; +const publisherRoleValidator = v.union( + v.literal("owner"), + v.literal("admin"), + v.literal("publisher"), +); type PublisherListStats = { skills: number; @@ -94,6 +100,12 @@ function validateHandle(rawHandle: string) { return handle; } +function assertOrgPublisherMembershipManagement(publisher: Doc<"publishers">) { + if (publisher.kind !== "org") { + throw new ConvexError("Personal publishers do not support member management"); + } +} + async function getUserByHandle(ctx: Pick, handle: string) { return await ctx.db .query("users") @@ -888,6 +900,24 @@ export const getMemberRoleInternal = internalQuery({ (await getPublisherMembership(ctx, args.publisherId, args.userId))?.role ?? null, }); +export const canAccessOwnerScopeInternal = internalQuery({ + args: { + publisherId: v.id("publishers"), + userId: v.id("users"), + allowedPublisherRoles: v.optional(v.array(publisherRoleValidator)), + legacyOwnerUserId: v.optional(v.id("users")), + }, + handler: async (ctx, args) => { + const publisher = await ctx.db.get(args.publisherId); + return await canAccessPublisherOwnerScope(ctx, { + publisher, + userId: args.userId, + allowedPublisherRoles: args.allowedPublisherRoles, + legacyOwnerUserId: args.legacyOwnerUserId, + }); + }, +}); + export const ensurePersonalPublisherInternal = internalMutation({ args: { userId: v.id("users") }, handler: async (ctx, args) => { @@ -942,6 +972,19 @@ export const resolvePublishTargetForUserInternal = internalMutation({ `Publisher "@${requestedHandle}" not found. Create the "@${requestedHandle}" organization on ClawHub or choose a different owner.`, ); } + if (publisher.kind === "user") { + if (publisher.linkedUserId !== actor._id) { + throw new ConvexError( + `You do not have publish access for "@${requestedHandle}". Ask an owner or admin of "@${requestedHandle}" to add you.`, + ); + } + return { + publisherId: publisher._id, + handle: publisher.handle, + kind: publisher.kind, + linkedUserId: publisher.linkedUserId, + }; + } const membership = await getPublisherMembership(ctx, publisher._id, actor._id); if (!membership || !isPublisherRoleAllowed(membership.role, [minimumRole])) { throw new ConvexError( @@ -971,11 +1014,17 @@ export const listMine = query({ const publishers = await Promise.all( memberships.map(async (membership) => { const publisher = await ctx.db.get(membership.publisherId); + if (publisher?.kind === "user") { + const isLinkedPersonal = publisher.linkedUserId === userId; + const isLegacyPersonal = + !publisher.linkedUserId && user.personalPublisherId === publisher._id; + if (!isLinkedPersonal && !isLegacyPersonal) return null; + } const publicPublisher = await toPublicPublisherWithOfficial(ctx, publisher); if (!publicPublisher) return null; return { publisher: publicPublisher, - role: membership.role, + role: publisher?.kind === "user" ? "owner" : membership.role, }; }), ); @@ -1499,6 +1548,7 @@ export const addMember = mutation({ if (!membership || !isPublisherRoleAllowed(membership.role, ["admin"])) { throw new ConvexError("Forbidden"); } + assertOrgPublisherMembershipManagement(publisher); if (args.role === "owner" && membership.role !== "owner") { throw new ConvexError("Only org owners can promote members to owner"); } @@ -1547,15 +1597,39 @@ export const removeMember = mutation({ userId: v.id("users"), }, handler: async (ctx, args) => { - const { userId } = await requireUser(ctx); + const { user, userId } = await requireUser(ctx); const publisher = await ctx.db.get(args.publisherId); if (!publisher || publisher.deletedAt || publisher.deactivatedAt) { throw new ConvexError("Publisher not found"); } + if (publisher.kind === "user") { + const actorMembership = await getPublisherMembership(ctx, publisher._id, userId); + const isPersonalOwner = + publisher.linkedUserId === userId || + (!publisher.linkedUserId && + (user.personalPublisherId === publisher._id || actorMembership?.role === "owner")); + if (!isPersonalOwner) throw new ConvexError("Forbidden"); + const targetMembership = await getPublisherMembership(ctx, publisher._id, args.userId); + if (!targetMembership) return { ok: true }; + if (args.userId === (publisher.linkedUserId ?? userId)) { + throw new ConvexError("Personal publisher owner membership cannot be removed"); + } + await ctx.db.delete(targetMembership._id); + await ctx.db.insert("auditLogs", { + actorUserId: userId, + action: "publisher.member.remove", + targetType: "publisher", + targetId: publisher._id, + metadata: { memberUserId: args.userId }, + createdAt: Date.now(), + }); + return { ok: true }; + } const actorMembership = await getPublisherMembership(ctx, publisher._id, userId); if (!actorMembership || !isPublisherRoleAllowed(actorMembership.role, ["admin"])) { throw new ConvexError("Forbidden"); } + assertOrgPublisherMembershipManagement(publisher); const targetMembership = await getPublisherMembership(ctx, publisher._id, args.userId); if (!targetMembership) return { ok: true }; if (targetMembership.role === "owner" && actorMembership.role !== "owner") { diff --git a/convex/skills.dashboard.test.ts b/convex/skills.dashboard.test.ts index 989b5b0d..f6f25b57 100644 --- a/convex/skills.dashboard.test.ts +++ b/convex/skills.dashboard.test.ts @@ -6,7 +6,7 @@ vi.mock("@convex-dev/auth/server", () => ({ authTables: {}, })); -import { listDashboardPaginated } from "./skills"; +import { list, listDashboardPaginated } from "./skills"; type WrappedHandler = { _handler: (ctx: unknown, args: TArgs) => Promise; @@ -22,6 +22,16 @@ const handler = ( { page: Array<{ slug: string }>; isDone: boolean; continueCursor: string } > )._handler; +const listHandler = ( + list as unknown as WrappedHandler< + { + ownerUserId?: string; + ownerPublisherId?: string; + limit?: number; + }, + Array<{ slug: string }> + > +)._handler; function makeSkill(slug: string, overrides: Record = {}) { return { @@ -61,13 +71,39 @@ function makeSkill(slug: string, overrides: Record = {}) { }; } -function makeCtx(indexPages: Record[]>) { +type SkillTestDoc = ReturnType; +type IndexPage = + | SkillTestDoc[] + | Array<{ page: SkillTestDoc[]; isDone: boolean; continueCursor: string }>; + +function isPaginatedIndexPage( + page: IndexPage | undefined, +): page is Array<{ page: SkillTestDoc[]; isDone: boolean; continueCursor: string }> { + return Array.isArray(page) && page.length > 0 && "page" in page[0]; +} + +function makeCtx( + indexPages: Record, + options: { membership?: Record | null; legacyPersonalPublisher?: boolean } = {}, +) { const indexCalls: string[] = []; const ctx = { db: { get: vi.fn(async (id: string) => { if (id === "users:owner") { - return { _id: "users:owner", _creationTime: 1, handle: "owner", displayName: "Owner" }; + return { + _id: "users:owner", + _creationTime: 1, + handle: "owner", + displayName: "Owner", + personalPublisherId: options.legacyPersonalPublisher ? "publishers:self" : undefined, + }; + } + if (id === "users:other") { + return { _id: "users:other", _creationTime: 1, handle: "other", displayName: "Other" }; + } + if (id === "users:member") { + return { _id: "users:member", _creationTime: 1, handle: "member", displayName: "Member" }; } if (id === "publishers:self") { return { @@ -76,7 +112,7 @@ function makeCtx(indexPages: Record[]>) { kind: "user", handle: "owner", displayName: "Owner", - linkedUserId: "users:owner", + linkedUserId: options.legacyPersonalPublisher ? undefined : "users:owner", }; } if (id === "publishers:org") { @@ -88,13 +124,23 @@ function makeCtx(indexPages: Record[]>) { displayName: "Team", }; } + if (id === "publishers:other-personal") { + return { + _id: "publishers:other-personal", + _creationTime: 1, + kind: "user", + handle: "other", + displayName: "Other", + linkedUserId: "users:other", + }; + } return null; }), query: vi.fn((table: string) => { if (table === "publisherMembers") { return { withIndex: vi.fn(() => ({ - unique: vi.fn().mockResolvedValue(null), + unique: vi.fn().mockResolvedValue(options.membership ?? null), })), }; } @@ -109,12 +155,27 @@ function makeCtx(indexPages: Record[]>) { return { withIndex: vi.fn((indexName: string) => { indexCalls.push(indexName); + const indexPage = indexPages[indexName] ?? []; + const takeRows = isPaginatedIndexPage(indexPage) + ? indexPage.flatMap((entry) => entry.page) + : indexPage; return { order: vi.fn(() => ({ - paginate: vi.fn().mockResolvedValue({ - page: indexPages[indexName] ?? [], - isDone: true, - continueCursor: "", + take: vi.fn().mockResolvedValue(takeRows), + paginate: vi.fn((paginationOpts: { cursor: string | null }) => { + if (isPaginatedIndexPage(indexPage)) { + const pageIndex = paginationOpts.cursor + ? Number(paginationOpts.cursor.replace("cursor:", "")) + : 0; + return Promise.resolve( + indexPage[pageIndex] ?? { page: [], isDone: true, continueCursor: "" }, + ); + } + return Promise.resolve({ + page: indexPage, + isDone: true, + continueCursor: "", + }); }), })), }; @@ -167,11 +228,40 @@ describe("skills.listDashboardPaginated", () => { expect(result.page).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); }); - it("keeps non-owner personal publisher reads scoped to publisher-owned skills", async () => { - vi.mocked(getAuthUserId).mockResolvedValue("users:other" as never); + it("includes legacy no-link personal publisher skills when paginating", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const { ctx, indexCalls } = makeCtx( + { + by_owner_active_updated: [makeSkill("legacy-skill")], + }, + { legacyPersonalPublisher: true }, + ); + + const result = await handler( + ctx as never, + { + ownerPublisherId: "publishers:self", + paginationOpts, + } as never, + ); + + expect(indexCalls).toContain("by_owner_active_updated"); + expect(result.page).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + }); + + it("excludes other publisher-owned skills from personal publisher dashboards", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); const { ctx, indexCalls } = makeCtx({ - by_owner_publisher_active_updated: [ - makeSkill("published-skill", { ownerPublisherId: "publishers:self" }), + by_owner_active_updated: [ + makeSkill("team-hidden", { + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + }), + makeSkill("personal-published", { + ownerPublisherId: "publishers:self", + moderationStatus: "hidden", + }), + makeSkill("legacy-skill"), ], }); @@ -183,9 +273,110 @@ describe("skills.listDashboardPaginated", () => { } as never, ); - expect(indexCalls).toContain("by_owner_publisher_active_updated"); - expect(indexCalls).not.toContain("by_owner_active_updated"); - expect(result.page).toEqual([expect.objectContaining({ slug: "published-skill" })]); + expect(indexCalls).toContain("by_owner_active_updated"); + expect(result.page).toEqual([ + expect.objectContaining({ slug: "personal-published" }), + expect.objectContaining({ slug: "legacy-skill" }), + ]); + }); + + it("continues personal dashboard pagination past other publisher-owned rows", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const { ctx } = makeCtx({ + by_owner_active_updated: [ + { + page: [ + makeSkill("team-hidden", { + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + }), + ], + isDone: false, + continueCursor: "cursor:1", + }, + { + page: [makeSkill("legacy-skill")], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await handler( + ctx as never, + { + ownerPublisherId: "publishers:self", + paginationOpts: { cursor: null, numItems: 1 }, + } as never, + ); + + expect(result.page).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + expect(result.isDone).toBe(true); + expect(result.continueCursor).toBe(""); + }); + + it("continues owner-user dashboard pagination past stale publisher-owned rows", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const { ctx } = makeCtx({ + by_owner_active_updated: [ + { + page: [ + makeSkill("other-personal-hidden", { + ownerPublisherId: "publishers:other-personal", + moderationStatus: "hidden", + }), + makeSkill("org-hidden", { + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + }), + ], + isDone: false, + continueCursor: "cursor:1", + }, + { + page: [makeSkill("legacy-skill", { moderationStatus: "hidden" })], + isDone: true, + continueCursor: "", + }, + ], + }); + + const result = await handler( + ctx as never, + { + ownerUserId: "users:owner", + paginationOpts: { cursor: null, numItems: 1 }, + } as never, + ); + + expect(result.page).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + expect(result.isDone).toBe(true); + expect(result.continueCursor).toBe(""); + }); + + it("includes linked-user legacy skills in non-owner personal publisher reads", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:other" as never); + const { ctx, indexCalls } = makeCtx({ + by_owner_active_updated: [ + makeSkill("published-skill", { ownerPublisherId: "publishers:self" }), + makeSkill("legacy-skill"), + ], + }); + + const result = await handler( + ctx as never, + { + ownerPublisherId: "publishers:self", + paginationOpts, + } as never, + ); + + expect(indexCalls).toContain("by_owner_active_updated"); + expect(indexCalls).not.toContain("by_owner_publisher_active_updated"); + expect(result.page).toEqual([ + expect.objectContaining({ slug: "published-skill" }), + expect.objectContaining({ slug: "legacy-skill" }), + ]); }); it("paginates org publisher skills through an active publisher index", async () => { @@ -207,4 +398,160 @@ describe("skills.listDashboardPaginated", () => { expect(indexCalls).toContain("by_owner_publisher_active_updated"); expect(result.page).toEqual([expect.objectContaining({ slug: "team-skill" })]); }); + + it("ignores stale personal memberships for hidden dashboard skills", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:other" as never); + const { ctx, indexCalls } = makeCtx( + { + by_owner_publisher_active_updated: [ + makeSkill("hidden-personal", { + ownerPublisherId: "publishers:self", + moderationStatus: "hidden", + }), + ], + }, + { + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:self", + userId: "users:other", + role: "owner", + }, + legacyPersonalPublisher: true, + }, + ); + + const result = await handler( + ctx as never, + { + ownerPublisherId: "publishers:self", + paginationOpts, + } as never, + ); + + expect(indexCalls).toContain("by_owner_publisher_active_updated"); + expect(indexCalls).not.toContain("by_owner_active_updated"); + expect(result.page).toEqual([]); + }); + + it("keeps org members authorized for hidden dashboard skills", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:member" as never); + const { ctx } = makeCtx( + { + by_owner_publisher_active_updated: [ + makeSkill("hidden-team", { + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + }), + ], + }, + { + membership: { + _id: "publisherMembers:member", + publisherId: "publishers:org", + userId: "users:member", + role: "publisher", + }, + }, + ); + + const result = await handler( + ctx as never, + { + ownerPublisherId: "publishers:org", + paginationOpts, + } as never, + ); + + expect(result.page).toEqual([expect.objectContaining({ slug: "hidden-team" })]); + }); + + it("ignores stale personal memberships in the non-paginated skill list", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:other" as never); + const { ctx, indexCalls } = makeCtx( + { + by_owner_publisher: [ + makeSkill("hidden-personal", { + ownerPublisherId: "publishers:self", + moderationStatus: "hidden", + }), + ], + }, + { + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:self", + userId: "users:other", + role: "owner", + }, + legacyPersonalPublisher: true, + }, + ); + + const result = await listHandler( + ctx as never, + { ownerPublisherId: "publishers:self", limit: 20 } as never, + ); + + expect(indexCalls).toContain("by_owner_publisher"); + expect(result).toEqual([]); + }); + + it("includes linked-user legacy personal skills in public non-paginated lists", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:other" as never); + const { ctx, indexCalls } = makeCtx({ + by_owner: [makeSkill("legacy-skill")], + }); + + const result = await listHandler( + ctx as never, + { ownerPublisherId: "publishers:self", limit: 20 } as never, + ); + + expect(indexCalls).toContain("by_owner"); + expect(result).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + }); + + it("includes legacy no-link personal publisher skills in the non-paginated list", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const { ctx, indexCalls } = makeCtx( + { + by_owner: [makeSkill("legacy-skill")], + }, + { legacyPersonalPublisher: true }, + ); + + const result = await listHandler( + ctx as never, + { ownerPublisherId: "publishers:self", limit: 20 } as never, + ); + + expect(indexCalls).toContain("by_owner"); + expect(result).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + }); + + it("keeps stale publisher-owned rows out of owner-user non-paginated dashboards", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const { ctx, indexCalls } = makeCtx({ + by_owner: [ + makeSkill("other-personal-hidden", { + ownerPublisherId: "publishers:other-personal", + moderationStatus: "hidden", + }), + makeSkill("org-hidden", { + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + }), + makeSkill("legacy-skill", { moderationStatus: "hidden" }), + ], + }); + + const result = await listHandler( + ctx as never, + { ownerUserId: "users:owner", limit: 20 } as never, + ); + + expect(indexCalls).toContain("by_owner"); + expect(result).toEqual([expect.objectContaining({ slug: "legacy-skill" })]); + }); }); diff --git a/convex/skills.ownerMigration.test.ts b/convex/skills.ownerMigration.test.ts index 44486a1a..d08bcc36 100644 --- a/convex/skills.ownerMigration.test.ts +++ b/convex/skills.ownerMigration.test.ts @@ -363,6 +363,38 @@ describe("skills.insertVersion owner migration", () => { expect(migrationAudits).toHaveLength(0); }); + it("rejects migration from a linked personal publisher when ownerUserId is stale", async () => { + const fixture = createMigrationFixture({ + skillSource: "other-personal", + sourceMemberships: [ + { + _id: "publisherMembers:orgAdminCaller", + publisherId: "publishers:org", + userId: "users:caller", + role: "admin", + }, + ], + skillOverrides: { + ownerUserId: "users:caller", + }, + }); + + 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("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 diff --git a/convex/skills.ownership.test.ts b/convex/skills.ownership.test.ts index 2cb4513a..5e63a485 100644 --- a/convex/skills.ownership.test.ts +++ b/convex/skills.ownership.test.ts @@ -575,6 +575,239 @@ describe("skills ownership", () => { ); }); + it("rejects stale personal publisher memberships as skill transfer destinations", async () => { + const patch = vi.fn(async () => {}); + const skill = { + _id: "skills:source", + slug: "portable", + displayName: "Portable", + ownerUserId: "users:actor", + ownerPublisherId: "publishers:actor", + softDeletedAt: undefined, + }; + + await expect( + transferSkillOwnerForUserInternalHandler( + { + db: { + normalizeId: vi.fn(() => null), + get: vi.fn(async (id: string) => { + if (id === "users:actor") return { _id: "users:actor", role: "user" }; + if (id === "users:owner") return { _id: "users:owner", role: "user" }; + if (id === "publishers:actor") { + return { + _id: "publishers:actor", + kind: "user", + handle: "actor", + linkedUserId: "users:actor", + }; + } + if (id === "publishers:owner") { + return { + _id: "publishers:owner", + kind: "user", + handle: "owner", + linkedUserId: "users:owner", + deletedAt: undefined, + deactivatedAt: undefined, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: (name: string, build: (q: ReturnType) => unknown) => { + const constraints: Record = {}; + build(chainEq(constraints)); + if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`); + return { unique: async () => (constraints.slug === "portable" ? skill : null) }; + }, + }; + } + if (table === "publishers") { + return { + withIndex: (name: string) => { + if (name !== "by_handle") { + throw new Error(`unexpected publishers index ${name}`); + } + return { + unique: async () => ({ + _id: "publishers:owner", + kind: "user", + handle: "owner", + linkedUserId: "users:owner", + deletedAt: undefined, + deactivatedAt: undefined, + }), + }; + }, + }; + } + if (table === "publisherMembers") { + return { + withIndex: (name: string) => { + if (name !== "by_publisher_user") { + throw new Error(`unexpected publisherMembers index ${name}`); + } + return { + unique: async () => ({ + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:actor", + role: "admin", + }), + }; + }, + }; + } + throw new Error(`unexpected table ${table}`); + }), + patch, + insert: vi.fn(), + }, + } as never, + { + actorUserId: "users:actor", + slug: "portable", + toOwner: "owner", + }, + ), + ).rejects.toThrow('admin access for "@owner"'); + + expect(patch).not.toHaveBeenCalled(); + }); + + it("allows transfers into the actor's legacy no-link personal publisher", async () => { + const patch = vi.fn(async () => {}); + const insert = vi.fn(async () => "auditLogs:1"); + const skill = { + _id: "skills:source", + slug: "portable", + displayName: "Portable", + ownerUserId: "users:actor", + ownerPublisherId: "publishers:actor", + softDeletedAt: undefined, + }; + const aliases = [ + { + _id: "skillSlugAliases:old", + slug: "portable-old", + skillId: "skills:source", + ownerUserId: "users:actor", + ownerPublisherId: "publishers:actor", + }, + ]; + + const result = await transferSkillOwnerForUserInternalHandler( + { + db: { + normalizeId: vi.fn(() => null), + get: vi.fn(async (id: string) => { + if (id === "users:actor") { + return { + _id: "users:actor", + role: "user", + personalPublisherId: "publishers:actor-legacy", + }; + } + if (id === "publishers:actor") { + return { + _id: "publishers:actor", + kind: "user", + handle: "actor", + linkedUserId: "users:actor", + }; + } + if (id === "publishers:actor-legacy") { + return { + _id: "publishers:actor-legacy", + kind: "user", + handle: "actor-legacy", + linkedUserId: undefined, + deletedAt: undefined, + deactivatedAt: undefined, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: (name: string, build: (q: ReturnType) => unknown) => { + const constraints: Record = {}; + build(chainEq(constraints)); + if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`); + return { unique: async () => (constraints.slug === "portable" ? skill : null) }; + }, + }; + } + if (table === "publishers") { + return { + withIndex: (name: string) => { + if (name !== "by_handle") { + throw new Error(`unexpected publishers index ${name}`); + } + return { + unique: async () => ({ + _id: "publishers:actor-legacy", + kind: "user", + handle: "actor-legacy", + linkedUserId: undefined, + deletedAt: undefined, + deactivatedAt: undefined, + }), + }; + }, + }; + } + if (table === "skillSlugAliases") { + return { + withIndex: (name: string) => { + if (name !== "by_skill") { + throw new Error(`unexpected skillSlugAliases index ${name}`); + } + return { collect: async () => aliases }; + }, + }; + } + if (table === "skillSearchDigest") { + return { + withIndex: (name: string) => { + if (name !== "by_skill") { + throw new Error(`unexpected skillSearchDigest index ${name}`); + } + return { unique: async () => ({ _id: "skillSearchDigest:source" }) }; + }, + }; + } + throw new Error(`unexpected table ${table}`); + }), + patch, + insert, + }, + } as never, + { + actorUserId: "users:actor", + slug: "portable", + toOwner: "actor-legacy", + }, + ); + + expect(result).toMatchObject({ + ok: true, + transferred: true, + toPublisherHandle: "actor-legacy", + }); + expect(patch).toHaveBeenCalledWith( + "skills:source", + expect.objectContaining({ + ownerUserId: "users:actor", + ownerPublisherId: "publishers:actor-legacy", + }), + ); + }); + it("rejects direct owner transfers for skills under moderation", async () => { const moderationStates = [ { moderationStatus: "hidden" }, diff --git a/convex/skills.public.test.ts b/convex/skills.public.test.ts index d152cb95..9daa51c2 100644 --- a/convex/skills.public.test.ts +++ b/convex/skills.public.test.ts @@ -166,6 +166,8 @@ const resolveSkillAppealForUserInternalHandler = ( function makeCtx(args: { skill: Record | null; owner: Record | null; + ownerPublisher?: Record | null; + membership?: Record | null; latestVersion?: Record | null; skillsById?: Record>; ownersById?: Record>; @@ -173,6 +175,13 @@ function makeCtx(args: { const unique = vi.fn().mockResolvedValue(args.skill); const withIndex = vi.fn(() => ({ unique })); const query = vi.fn((table: string) => { + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(args.membership ?? null), + })), + }; + } if (table !== "skills") throw new Error(`Unexpected query table: ${table}`); return { withIndex }; }); @@ -181,6 +190,7 @@ function makeCtx(args: { if (id === args.skill._id) return args.skill; if (args.skillsById?.[id]) return args.skillsById[id]; if (args.ownersById?.[id]) return args.ownersById[id]; + if (id === args.skill.ownerPublisherId) return args.ownerPublisher ?? null; if (id === args.skill.ownerUserId) return args.owner; if (id === args.skill.latestVersionId) return args.latestVersion ?? null; return null; @@ -375,6 +385,114 @@ describe("skills.getBySlug", () => { expect(result).toBeNull(); }); + it("does not honor stale personal memberships for hidden skill owner views", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:stranger" as never); + const ctx = makeCtx({ + skill: makeSkill({ + ownerPublisherId: "publishers:owner", + moderationStatus: "hidden", + moderationReason: "manual.review", + }), + owner: makeOwner("users:1", "owner"), + ownerPublisher: { + _id: "publishers:owner", + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:1", + }, + ownersById: { + "users:stranger": makeOwner("users:stranger", "stranger"), + }, + membership: { + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:stranger", + role: "owner", + }, + }); + + const result = await getBySlugHandler(ctx, { slug: "demo" } as never); + + expect(result).toBeNull(); + }); + + it("does not treat stale ownerUserId as owner for hidden publisher-owned skills", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:1" as never); + const ctx = makeCtx({ + skill: makeSkill({ + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + moderationReason: "manual.review", + }), + owner: makeOwner("users:1", "owner"), + ownerPublisher: { + _id: "publishers:org", + kind: "org", + handle: "team", + displayName: "Team", + }, + }); + + const result = await getBySlugHandler(ctx, { slug: "demo" } as never); + + expect(result).toBeNull(); + }); + + it("keeps legacy no-link personal publisher owners authorized for hidden skill views", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:1" as never); + const ctx = makeCtx({ + skill: makeSkill({ + ownerPublisherId: "publishers:owner", + moderationStatus: "hidden", + moderationReason: "manual.review", + }), + owner: makeOwner("users:1", "owner"), + ownerPublisher: { + _id: "publishers:owner", + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: undefined, + }, + }); + + const result = await getBySlugHandler(ctx, { slug: "demo" } as never); + + expect(result?.skill).toMatchObject({ slug: "demo" }); + }); + + it("keeps org memberships authorized for hidden skill owner views", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:member" as never); + const ctx = makeCtx({ + skill: makeSkill({ + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + moderationReason: "manual.review", + }), + owner: makeOwner("users:1", "owner"), + ownerPublisher: { + _id: "publishers:org", + kind: "org", + handle: "team", + displayName: "Team", + }, + ownersById: { + "users:member": makeOwner("users:member", "member"), + }, + membership: { + _id: "publisherMembers:member", + publisherId: "publishers:org", + userId: "users:member", + role: "publisher", + }, + }); + + const result = await getBySlugHandler(ctx, { slug: "demo" } as never); + + expect(result?.skill).toMatchObject({ slug: "demo" }); + }); + it("omits duplicate references to nonpublic skills", async () => { const ctx = makeCtx({ skill: makeSkill({ @@ -726,6 +844,211 @@ describe("skill artifact moderation", () => { ); }); + it("does not let stale personal memberships submit skill appeals", async () => { + const skill = makeSkill({ + softDeletedAt: 123, + ownerPublisherId: "publishers:owner", + moderationStatus: "hidden", + moderationReason: "scanner.llm.suspicious", + latestVersionId: undefined, + }); + + await expect( + submitSkillAppealForUserInternalHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:stranger") return makeOwner("users:stranger", "stranger"); + if (id === "publishers:owner") { + return { + _id: "publishers:owner", + kind: "user", + handle: "owner", + displayName: "Owner", + linkedUserId: "users:1", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(skill), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:stale", + publisherId: "publishers:owner", + userId: "users:stranger", + role: "owner", + }), + })), + }; + } + throw new Error(`Unexpected query table: ${table}`); + }), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + delete: vi.fn(), + normalizeId: vi.fn(), + }, + } as never, + { + actorUserId: "users:stranger", + slug: "demo", + message: "please review", + }, + ), + ).rejects.toThrow("Unauthorized"); + }); + + it("does not let stale ownerUserId submit skill appeals for publisher-owned skills", async () => { + const skill = makeSkill({ + softDeletedAt: 123, + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + moderationReason: "scanner.llm.suspicious", + latestVersionId: undefined, + }); + + await expect( + submitSkillAppealForUserInternalHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:1") return makeOwner("users:1", "owner"); + if (id === "publishers:org") { + return { + _id: "publishers:org", + kind: "org", + handle: "team", + displayName: "Team", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(skill), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(null), + })), + }; + } + throw new Error(`Unexpected query table: ${table}`); + }), + insert: vi.fn(), + patch: vi.fn(), + replace: vi.fn(), + delete: vi.fn(), + normalizeId: vi.fn(), + }, + } as never, + { + actorUserId: "users:1", + slug: "demo", + message: "please review", + }, + ), + ).rejects.toThrow("Unauthorized"); + }); + + it("lets org members submit skill appeals", async () => { + const skill = makeSkill({ + softDeletedAt: 123, + ownerPublisherId: "publishers:org", + moderationStatus: "hidden", + moderationReason: "scanner.llm.suspicious", + latestVersionId: undefined, + }); + const insert = vi.fn(async (table: string) => { + if (table === "skillAppeals") return "skillAppeals:1"; + if (table === "skillModerationEventLogs") return "skillModerationEventLogs:1"; + if (table === "auditLogs") return "auditLogs:1"; + throw new Error(`Unexpected insert table: ${table}`); + }); + + const result = await submitSkillAppealForUserInternalHandler( + { + db: { + get: vi.fn(async (id: string) => { + if (id === "users:member") return makeOwner("users:member", "member"); + if (id === "publishers:org") { + return { + _id: "publishers:org", + kind: "org", + handle: "team", + displayName: "Team", + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skills") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue(skill), + })), + }; + } + if (table === "publisherMembers") { + return { + withIndex: vi.fn(() => ({ + unique: vi.fn().mockResolvedValue({ + _id: "publisherMembers:member", + publisherId: "publishers:org", + userId: "users:member", + role: "publisher", + }), + })), + }; + } + if (table === "skillAppeals") { + return { + withIndex: vi.fn(() => ({ + order: vi.fn(() => ({ + first: vi.fn().mockResolvedValue(null), + })), + })), + }; + } + throw new Error(`Unexpected query table: ${table}`); + }), + insert, + patch: vi.fn(), + replace: vi.fn(), + delete: vi.fn(), + normalizeId: vi.fn(), + }, + } as never, + { + actorUserId: "users:member", + slug: "demo", + message: "please review", + }, + ); + + expect(result).toMatchObject({ + ok: true, + submitted: true, + appealId: "skillAppeals:1", + skillId: "skills:1", + }); + }); + it("keeps hidden skill reports visible in the moderator queue", async () => { const result = await listSkillReportsInternalHandler( { diff --git a/convex/skills.ts b/convex/skills.ts index 48eafba0..2676cd27 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -73,6 +73,7 @@ import { } from "./lib/public"; import { assertCanManageOwnedResource, + canAccessPublisherOwnerScope, ensurePersonalPublisherForUser, getActiveUserByHandleOrPersonalPublisher, getOwnerPublisher, @@ -1919,6 +1920,33 @@ async function filterSkillsByActiveOwner(ctx: Pick, skills: Doc< return filtered.filter((skill): skill is Doc<"skills"> => skill !== null); } +async function skillBelongsToOwnerUserDashboardScope( + ctx: Pick, + skill: Pick, "ownerUserId" | "ownerPublisherId">, + ownerUserId: Id<"users">, +) { + if (skill.ownerUserId !== ownerUserId) return false; + if (!skill.ownerPublisherId) return true; + const ownerPublisher = await ctx.db.get(skill.ownerPublisherId); + if (!ownerPublisher || !isPublisherActive(ownerPublisher) || ownerPublisher.kind !== "user") { + return false; + } + return ownerPublisher.linkedUserId ? ownerPublisher.linkedUserId === ownerUserId : true; +} + +async function filterSkillsForOwnerUserDashboard( + ctx: Pick, + skills: Doc<"skills">[], + ownerUserId: Id<"users">, +) { + const scoped = await Promise.all( + skills.map(async (skill) => + (await skillBelongsToOwnerUserDashboardScope(ctx, skill, ownerUserId)) ? skill : null, + ), + ); + return scoped.filter((skill): skill is Doc<"skills"> => Boolean(skill)); +} + async function loadPublicLatestVersionForSkill( ctx: Pick, skill: Pick, "_id" | "latestVersionId">, @@ -2217,6 +2245,13 @@ async function removeSkillBadge(ctx: MutationCtx, skillId: Id<"skills">, kind: B } } +function isDirectSkillOwner( + skill: Pick, "ownerUserId" | "ownerPublisherId">, + userId: Id<"users">, +) { + return !skill.ownerPublisherId && skill.ownerUserId === userId; +} + export const getBySlug = query({ args: { slug: v.string() }, handler: async (ctx, args) => { @@ -2229,16 +2264,18 @@ export const getBySlug = query({ ownerPublisherId: skill.ownerPublisherId, ownerUserId: skill.ownerUserId, }); - const membership = - userId && skill.ownerPublisherId - ? await ctx.db - .query("publisherMembers") - .withIndex("by_publisher_user", (q) => - q.eq("publisherId", skill.ownerPublisherId!).eq("userId", userId), - ) - .unique() - : null; - const isOwner = Boolean(userId && (userId === skill.ownerUserId || membership)); + const skillOwnerPublisher = skill.ownerPublisherId + ? await ctx.db.get(skill.ownerPublisherId) + : null; + const publisherOwner = + userId && skillOwnerPublisher + ? await canAccessPublisherOwnerScope(ctx, { + publisher: skillOwnerPublisher, + userId, + legacyOwnerUserId: skill.ownerUserId, + }) + : false; + const isOwner = Boolean(userId && (isDirectSkillOwner(skill, userId) || publisherOwner)); const latestVersionDoc = skill.latestVersionId ? await ctx.db.get(skill.latestVersionId) : null; const publicLatestVersionDoc = isPublicSkillVersionAvailableForSkill( @@ -3156,31 +3193,37 @@ export const list = query({ if (ownerPublisherId) { const userId = await getOptionalActiveAuthUserId(ctx); const ownerPublisher = await ctx.db.get(ownerPublisherId); - const membership = - userId && - (await ctx.db - .query("publisherMembers") - .withIndex("by_publisher_user", (q) => - q.eq("publisherId", ownerPublisherId).eq("userId", userId), - ) - .unique()); + const owner = + userId && ownerPublisher?.kind === "user" && !ownerPublisher.linkedUserId + ? await ctx.db.get(userId) + : null; const isOwnDashboard = Boolean( - membership || - (userId && ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId === userId), + userId && + ((await canAccessPublisherOwnerScope(ctx, { + publisher: ownerPublisher, + userId, + })) || + (ownerPublisher?.kind === "user" && + isPublisherActive(ownerPublisher) && + !ownerPublisher.linkedUserId && + owner?.personalPublisherId === ownerPublisherId)), ); const scopedEntries = await ctx.db .query("skills") .withIndex("by_owner_publisher", (q) => q.eq("ownerPublisherId", ownerPublisherId)) .order("desc") .take(takeLimit); - const legacyEntries = - ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId - ? await ctx.db - .query("skills") - .withIndex("by_owner", (q) => q.eq("ownerUserId", ownerPublisher.linkedUserId!)) - .order("desc") - .take(takeLimit) - : []; + const legacyPersonalOwnerUserId = + ownerPublisher?.kind === "user" + ? (ownerPublisher.linkedUserId ?? (isOwnDashboard ? userId : undefined)) + : undefined; + const legacyEntries = legacyPersonalOwnerUserId + ? await ctx.db + .query("skills") + .withIndex("by_owner", (q) => q.eq("ownerUserId", legacyPersonalOwnerUserId)) + .order("desc") + .take(takeLimit) + : []; const combined = [...scopedEntries, ...legacyEntries].filter( (skill, index, all) => !skill.softDeletedAt && @@ -3210,7 +3253,8 @@ export const list = query({ .withIndex("by_owner", (q) => q.eq("ownerUserId", ownerUserId)) .order("desc") .take(takeLimit); - const filtered = entries.filter((skill) => !skill.softDeletedAt).slice(0, limit); + const scoped = await filterSkillsForOwnerUserDashboard(ctx, entries, ownerUserId); + const filtered = scoped.filter((skill) => !skill.softDeletedAt).slice(0, limit); const withBadges = await attachBadgesToSkills(ctx, filtered); if (isOwnDashboard) { @@ -3264,36 +3308,62 @@ export const listDashboardPaginated = query({ if (ownerPublisherId) { const userId = await getOptionalActiveAuthUserId(ctx); const ownerPublisher = await ctx.db.get(ownerPublisherId); - const membership = - userId && - (await ctx.db - .query("publisherMembers") - .withIndex("by_publisher_user", (q) => - q.eq("publisherId", ownerPublisherId).eq("userId", userId), - ) - .unique()); + const owner = + userId && ownerPublisher?.kind === "user" && !ownerPublisher.linkedUserId + ? await ctx.db.get(userId) + : null; const isOwnDashboard = Boolean( - membership || - (userId && ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId === userId), + userId && + ((await canAccessPublisherOwnerScope(ctx, { + publisher: ownerPublisher, + userId, + })) || + (ownerPublisher?.kind === "user" && + isPublisherActive(ownerPublisher) && + !ownerPublisher.linkedUserId && + owner?.personalPublisherId === ownerPublisherId)), ); - const result = - isOwnDashboard && ownerPublisher?.kind === "user" && ownerPublisher.linkedUserId + const legacyPersonalOwnerUserId = + ownerPublisher?.kind === "user" + ? (ownerPublisher.linkedUserId ?? (isOwnDashboard ? userId : undefined)) + : undefined; + const shouldIncludeLegacyPersonalSkills = Boolean(legacyPersonalOwnerUserId); + const paginateOwnerSkills = async (paginationOpts: typeof args.paginationOpts) => + shouldIncludeLegacyPersonalSkills ? await ctx.db .query("skills") .withIndex("by_owner_active_updated", (q) => - q.eq("ownerUserId", ownerPublisher.linkedUserId!).eq("softDeletedAt", undefined), + q.eq("ownerUserId", legacyPersonalOwnerUserId!).eq("softDeletedAt", undefined), ) .order("desc") - .paginate(args.paginationOpts) + .paginate(paginationOpts) : await ctx.db .query("skills") .withIndex("by_owner_publisher_active_updated", (q) => q.eq("ownerPublisherId", ownerPublisherId).eq("softDeletedAt", undefined), ) .order("desc") - .paginate(args.paginationOpts); - const page = await mapDashboardSkillPage(ctx, result.page, isOwnDashboard); + .paginate(paginationOpts); + let result = await paginateOwnerSkills(args.paginationOpts); + const scopePersonalDashboardPage = (page: Doc<"skills">[]) => + shouldIncludeLegacyPersonalSkills + ? page.filter( + (skill) => !skill.ownerPublisherId || skill.ownerPublisherId === ownerPublisherId, + ) + : page; + const scopedPage = scopePersonalDashboardPage(result.page); + if (shouldIncludeLegacyPersonalSkills) { + while (!result.isDone && scopedPage.length < args.paginationOpts.numItems) { + result = await paginateOwnerSkills({ + ...args.paginationOpts, + cursor: result.continueCursor, + numItems: args.paginationOpts.numItems - scopedPage.length, + }); + scopedPage.push(...scopePersonalDashboardPage(result.page)); + } + } + const page = await mapDashboardSkillPage(ctx, scopedPage, isOwnDashboard); return { ...result, page }; } @@ -3301,14 +3371,27 @@ export const listDashboardPaginated = query({ if (ownerUserId) { const userId = await getOptionalActiveAuthUserId(ctx); const isOwnDashboard = Boolean(userId && userId === ownerUserId); - const result = await ctx.db - .query("skills") - .withIndex("by_owner_active_updated", (q) => - q.eq("ownerUserId", ownerUserId).eq("softDeletedAt", undefined), - ) - .order("desc") - .paginate(args.paginationOpts); - const page = await mapDashboardSkillPage(ctx, result.page, isOwnDashboard); + const paginateOwnerSkills = async (paginationOpts: typeof args.paginationOpts) => + await ctx.db + .query("skills") + .withIndex("by_owner_active_updated", (q) => + q.eq("ownerUserId", ownerUserId).eq("softDeletedAt", undefined), + ) + .order("desc") + .paginate(paginationOpts); + let result = await paginateOwnerSkills(args.paginationOpts); + const scopedPage = await filterSkillsForOwnerUserDashboard(ctx, result.page, ownerUserId); + while (!result.isDone && scopedPage.length < args.paginationOpts.numItems) { + result = await paginateOwnerSkills({ + ...args.paginationOpts, + cursor: result.continueCursor, + numItems: args.paginationOpts.numItems - scopedPage.length, + }); + scopedPage.push( + ...(await filterSkillsForOwnerUserDashboard(ctx, result.page, ownerUserId)), + ); + } + const page = await mapDashboardSkillPage(ctx, scopedPage, isOwnDashboard); return { ...result, page }; } @@ -4016,15 +4099,14 @@ async function applySkillAppealFinalAction( } async function canUserAppealSkill(ctx: MutationCtx, skill: Doc<"skills">, userId: Id<"users">) { - if (skill.ownerUserId === userId) return true; + if (isDirectSkillOwner(skill, userId)) return true; if (!skill.ownerPublisherId) return false; - const member = await ctx.db - .query("publisherMembers") - .withIndex("by_publisher_user", (q) => - q.eq("publisherId", skill.ownerPublisherId!).eq("userId", userId), - ) - .unique(); - return Boolean(member); + const publisher = await ctx.db.get(skill.ownerPublisherId); + return await canAccessPublisherOwnerScope(ctx, { + publisher, + userId, + legacyOwnerUserId: skill.ownerUserId, + }); } async function getActiveSkillVersionForAppeal( @@ -8129,15 +8211,20 @@ async function canReadSkillVersionFiles(ctx: ActionCtx, version: Doc<"skillVersi const authUserId = await getOptionalActiveAuthUserIdFromAction(ctx); if (authUserId) { - if (authUserId === skill.ownerUserId && !skill.softDeletedAt && !version.softDeletedAt) { + if (isDirectSkillOwner(skill, authUserId) && !skill.softDeletedAt && !version.softDeletedAt) { return true; } if (skill.ownerPublisherId && !skill.softDeletedAt && !version.softDeletedAt) { - const memberRole = (await ctx.runQuery(internal.publishers.getMemberRoleInternal, { - publisherId: skill.ownerPublisherId, - userId: authUserId, - })) as "owner" | "admin" | "publisher" | null; - if (memberRole) { + const canAccessOwnerScope = (await ctx.runQuery( + internal.publishers.canAccessOwnerScopeInternal, + { + publisherId: skill.ownerPublisherId, + userId: authUserId, + allowedPublisherRoles: ["publisher"], + legacyOwnerUserId: skill.ownerUserId, + }, + )) as boolean; + if (canAccessOwnerScope) { return true; } } @@ -9138,7 +9225,11 @@ async function canManagePublisherDestination( publisher: Doc<"publishers">, ) { if (actor.role === "admin") return true; - if (publisher.kind === "user" && publisher.linkedUserId === actor._id) return true; + if (publisher.kind === "user") { + return publisher.linkedUserId + ? publisher.linkedUserId === actor._id + : actor.personalPublisherId === publisher._id; + } const membership = await getPublisherMembership(ctx, publisher._id, actor._id); return Boolean(membership && isPublisherRoleAllowed(membership.role, ["admin"])); } @@ -9958,8 +10049,11 @@ export const insertVersion = internalMutation({ const sourcePublisher = await ctx.db.get(skill.ownerPublisherId); const callerOwnsSourceViaPersonalLink = sourcePublisher?.kind === "user" && - (sourcePublisher.linkedUserId === userId || skill.ownerUserId === userId); - const sourceIsOrg = sourcePublisher?.kind === "org"; + isPublisherActive(sourcePublisher) && + (sourcePublisher.linkedUserId + ? sourcePublisher.linkedUserId === userId + : skill.ownerUserId === userId); + const sourceIsOrg = sourcePublisher?.kind === "org" && isPublisherActive(sourcePublisher); const sourceMembership = callerExplicitlySpecifiedOwner && callerRequestedMigration && sourceIsOrg diff --git a/convex/versionFileAccess.test.ts b/convex/versionFileAccess.test.ts index 2bb6e330..a2fbb36f 100644 --- a/convex/versionFileAccess.test.ts +++ b/convex/versionFileAccess.test.ts @@ -48,6 +48,7 @@ function makeActionCtx(args: { version?: Record | null; actor?: Record | null; publisherMemberRole?: "owner" | "admin" | "publisher" | null; + publisherAccess?: boolean; }) { return { runQuery: vi.fn(async (_endpoint: unknown, payload: Record) => { @@ -55,6 +56,11 @@ function makeActionCtx(args: { if (payload.skillId && args.skill) return args.skill ?? null; if (payload.soulId && args.soul) return args.soul ?? null; if (payload.publisherId && payload.userId === args.actor?._id) { + if (Array.isArray(payload.allowedPublisherRoles)) { + if (args.publisherAccess !== undefined) return args.publisherAccess; + if (payload.legacyOwnerUserId) return payload.legacyOwnerUserId === args.actor?._id; + return Boolean(args.publisherMemberRole); + } return args.publisherMemberRole ?? null; } if (payload.userId === args.actor?._id) { @@ -109,11 +115,35 @@ describe("version file access actions", () => { ).resolves.toEqual({ path: "SKILL.md", text: "# skill" }); }); + it("does not let stale ownerUserId read publisher-owned hidden skill versions", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = makeActionCtx({ + actor: { _id: "users:owner", role: "user" }, + publisherMemberRole: null, + publisherAccess: false, + version: makeSkillVersion(), + skill: { + _id: "skills:1", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:org", + softDeletedAt: undefined, + moderationStatus: "hidden", + moderationReason: "pending.scan", + moderationFlags: [], + }, + }); + + await expect( + getSkillReadmeHandler._handler(ctx, { versionId: "skillVersions:1" } as never), + ).rejects.toThrow("Version not available"); + }); + it("allows org collaborators to read hidden skill versions", async () => { vi.mocked(getAuthUserId).mockResolvedValue("users:member" as never); const ctx = makeActionCtx({ actor: { _id: "users:member", role: "user" }, publisherMemberRole: "publisher", + publisherAccess: true, version: makeSkillVersion(), skill: { _id: "skills:1", @@ -131,6 +161,74 @@ describe("version file access actions", () => { ).resolves.toEqual({ path: "SKILL.md", text: "# skill" }); }); + it("allows linked personal publisher users to read hidden skill versions", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = makeActionCtx({ + actor: { _id: "users:owner", role: "user" }, + publisherMemberRole: null, + publisherAccess: true, + version: makeSkillVersion(), + skill: { + _id: "skills:1", + ownerUserId: "users:legacy-owner", + ownerPublisherId: "publishers:owner", + softDeletedAt: undefined, + moderationStatus: "hidden", + moderationReason: "pending.scan", + moderationFlags: [], + }, + }); + + await expect( + getSkillReadmeHandler._handler(ctx, { versionId: "skillVersions:1" } as never), + ).resolves.toEqual({ path: "SKILL.md", text: "# skill" }); + }); + + it("allows legacy no-link personal publisher owners to read hidden skill versions", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); + const ctx = makeActionCtx({ + actor: { _id: "users:owner", role: "user" }, + publisherMemberRole: null, + version: makeSkillVersion(), + skill: { + _id: "skills:1", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:owner", + softDeletedAt: undefined, + moderationStatus: "hidden", + moderationReason: "pending.scan", + moderationFlags: [], + }, + }); + + await expect( + getSkillReadmeHandler._handler(ctx, { versionId: "skillVersions:1" } as never), + ).resolves.toEqual({ path: "SKILL.md", text: "# skill" }); + }); + + it("does not honor stale personal publisher memberships for hidden skill versions", async () => { + vi.mocked(getAuthUserId).mockResolvedValue("users:friend" as never); + const ctx = makeActionCtx({ + actor: { _id: "users:friend", role: "user" }, + publisherMemberRole: "owner", + publisherAccess: false, + version: makeSkillVersion(), + skill: { + _id: "skills:1", + ownerUserId: "users:owner", + ownerPublisherId: "publishers:owner", + softDeletedAt: undefined, + moderationStatus: "hidden", + moderationReason: "pending.scan", + moderationFlags: [], + }, + }); + + await expect( + getSkillReadmeHandler._handler(ctx, { versionId: "skillVersions:1" } as never), + ).rejects.toThrow("Version not available"); + }); + it("allows owners to read hidden skill files", async () => { vi.mocked(getAuthUserId).mockResolvedValue("users:owner" as never); const ctx = makeActionCtx({ diff --git a/specs/orgs.md b/specs/orgs.md index edbfa90f..1f0ee39e 100644 --- a/specs/orgs.md +++ b/specs/orgs.md @@ -252,6 +252,14 @@ Role semantics: Moderators/admins keep global override powers as they do today. +Membership management is only valid for `kind: "org"` publishers. Personal +publishers (`kind: "user"`) are identity aliases for one linked user; they keep +a single owner membership row for compatibility with publisher-aware ownership +checks, but authorization must key off `linkedUserId`, not extra membership +rows. Public member add mutations must not treat personal publishers as +organizations; remove mutations may only let the linked user clean up stale +extra membership rows. + Skill slug merges are content-management operations. They must authorize through publisher ownership, not only `ownerUserId`, so org owners/admins can merge two skills owned by the same manageable publisher. Merge aliases must keep both