diff --git a/.changeset/quiet-lions-retire.md b/.changeset/quiet-lions-retire.md new file mode 100644 index 00000000..e6a82b2d --- /dev/null +++ b/.changeset/quiet-lions-retire.md @@ -0,0 +1,15 @@ +--- +"@buildinternet/uploads": minor +--- + +`uploads config init` with no flags no longer seeds `UPLOADS_WORKSPACE=default` +into the config file. That entry outranked the workspace encoded in your token, +so it pinned every later `uploads login` to `default` no matter which workspace +the token was actually minted for. It now seeds only `UPLOADS_API_URL`; pass +`--workspace ` to set one explicitly. + +`uploads login` on an account with no workspace yet now offers a name derived +from your GitHub login as a bracketed default (`… (lowercase, hyphens) +[octocat]:`). Press Enter to accept it, or type anything else to override. When +no name can be derived — no linked GitHub account, or the derived name is +reserved or already taken — the prompt is exactly as before. diff --git a/apps/api/package.json b/apps/api/package.json index ea7b7284..e1b53db3 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -40,7 +40,6 @@ "@uploads/email": "workspace:^", "@uploads/errors": "workspace:^", "@uploads/storage": "workspace:^", - "@uploads/workspace": "workspace:^", "hono": "^4.12.28", "mediabunny": "^1.50.9" }, diff --git a/apps/api/src/content-hash.ts b/apps/api/src/content-hash.ts index dfb41318..e0d1c12a 100644 --- a/apps/api/src/content-hash.ts +++ b/apps/api/src/content-hash.ts @@ -13,7 +13,6 @@ * Design: `.context/479-content-hash-inheritance.md`. */ -import { isCommunalWorkspace } from "@uploads/workspace"; import { addMissingFileMetadata, getFileMetadata } from "./file-metadata"; /** @@ -22,8 +21,8 @@ import { addMissingFileMetadata, getFileMetadata } from "./file-metadata"; * * Restated rather than imported, and deliberately so. The vocabulary lives in * `@buildinternet/uploads`, which is **published** and carries no `@uploads/*` - * workspace dependencies — extracting the list to a shared private package (the - * `@uploads/workspace` pattern from #504) would make the published package + * workspace dependencies — extracting the list to a shared private package + * would make the published package * depend on a private one, and importing the CLI package here would invert the * dependency and drag CLI deps toward a Worker bundle. This follows the other * in-repo precedent instead: `packages/billing/src/workspace-cap.ts`, which @@ -88,14 +87,14 @@ export async function recordContentHash( * Derived metadata to inherit for `contentSha256`, or `{}` when there is * nothing to inherit. * - * Returns `{}` unconditionally for the communal workspace. `default` is the one - * shared tenant every account belongs to (`packages/workspace`), so - * same-workspace — the boundary issue #479 originally proposed — would let one - * user's `path=/admin/billing` land on an unrelated user's upload of the same - * bytes. Applied in both cloud and self-hosted: inheriting across unrelated - * members of a shared tenant is surprising in either deployment, and a - * trust rule that differs per deployment is one every future cross-tenant - * feature would have to reason about twice. + * Workspace membership is the trust boundary, with no workspace excluded. + * Issue #479 originally carved out `default` because it was the communal + * tenant every account belonged to, where "same workspace" said nothing about + * whether two uploaders were related. That concept is retired (#505): every + * workspace is now an ordinary one whose members are there deliberately. Since + * members can already read each other's files, inheriting metadata from a file + * the caller could simply open discloses nothing new — so the rule is uniform, + * in both cloud and self-hosted. */ export async function inheritableMetaForHash( db: D1Database, @@ -103,8 +102,6 @@ export async function inheritableMetaForHash( contentSha256: string, selfKey: string, ): Promise> { - if (isCommunalWorkspace(workspace)) return {}; - // Fail-soft, deliberately. This runs *after* the bytes are durably stored, so // a D1 blip here must cost the convenience (metadata the caller can re-state) // rather than the upload (bytes the caller would have to re-send). The same diff --git a/apps/api/src/routes/internal-billing.test.ts b/apps/api/src/routes/internal-billing.test.ts index 4dee59be..d62b37bd 100644 --- a/apps/api/src/routes/internal-billing.test.ts +++ b/apps/api/src/routes/internal-billing.test.ts @@ -1,4 +1,3 @@ -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { Hono } from "hono"; import { beforeAll, describe, expect, it } from "vitest"; import { fakeRegistry } from "../../test/fake-kv"; @@ -281,12 +280,25 @@ describe("GET /internal/billing/member-cap", () => { expect(body.message).toBeNull(); }); - it("exempts the communal workspace even if a plan is stamped on it", async () => { + // `default` used to be exempt from the member cap as the communal tenant. + // That exemption is gone (#505): it is capped by whatever plan it carries, + // like any other workspace. Its live record carries no plan and no + // `selfServe`, so it still resolves to unlimited on its own — but a plan + // stamped on it now applies, which is the behavior change this pins. + it("applies the cap to `default` when a plan is stamped on it — no exemption", async () => { const registry = fakeRegistry({ - [COMMUNAL_WORKSPACE]: { provider: "r2", bucket: "b", plan: "free", selfServe: true }, + default: { provider: "r2", bucket: "b", plan: "free", selfServe: true }, }); const env = { REGISTRY: registry, BILLING_INTERNAL_KEY: SECRET } as unknown as Env; - const res = await get(COMMUNAL_WORKSPACE, { "x-internal-billing-key": SECRET }, env); + const res = await get("default", { "x-internal-billing-key": SECRET }, env); + expect(res.status).toBe(200); + expect((await res.json()) as { cap: number | null }).toMatchObject({ cap: 3 }); + }); + + it("leaves `default` unlimited with no plan stamped — matching its live record", async () => { + const registry = fakeRegistry({ default: { provider: "r2", bucket: "b" } }); + const env = { REGISTRY: registry, BILLING_INTERNAL_KEY: SECRET } as unknown as Env; + const res = await get("default", { "x-internal-billing-key": SECRET }, env); expect(res.status).toBe(200); expect((await res.json()) as { cap: number | null }).toMatchObject({ cap: null }); }); diff --git a/apps/api/src/routes/internal-billing.ts b/apps/api/src/routes/internal-billing.ts index d04f3a6f..7df1a4e4 100644 --- a/apps/api/src/routes/internal-billing.ts +++ b/apps/api/src/routes/internal-billing.ts @@ -39,7 +39,6 @@ import { type PlanId, } from "@uploads/billing"; import { UnauthorizedError, ValidationError } from "@uploads/errors"; -import { isCommunalWorkspace } from "@uploads/workspace"; import { Hono } from "hono"; import type { Context } from "hono"; import { jsonBody } from "./json-body"; @@ -143,15 +142,7 @@ internalBilling.get("/member-cap", async (c) => { throw new ValidationError("workspace is required", { code: "invalid_workspace" }); } - // The communal workspace is exempt from the member cap, the same way it's - // already exempt from the other member flows (see slug-policy.ts). Today it - // also resolves to unlimited on its own — it's operator-provisioned, so it - // has neither a `plan` nor `selfServe` — but skipping the lookup means an - // operator stamping a plan on it can't accidentally cap the shared - // workspace. - const record = isCommunalWorkspace(workspace) - ? null - : await loadWorkspaceRecord(c.env, workspace); + const record = await loadWorkspaceRecord(c.env, workspace); const cap = record ? resolveMemberCap(record) : null; return c.json({ diff --git a/apps/api/src/routes/me.test.ts b/apps/api/src/routes/me.test.ts index 74297d16..b2f50d52 100644 --- a/apps/api/src/routes/me.test.ts +++ b/apps/api/src/routes/me.test.ts @@ -1,7 +1,6 @@ import { readFileSync } from "node:fs"; import { DatabaseSync, type SQLInputValue } from "node:sqlite"; import { fileURLToPath, URL as NodeURL } from "node:url"; -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { Hono } from "hono"; import { describe, expect, it } from "vitest"; import { respondError } from "../error-response"; @@ -78,33 +77,12 @@ describe("GET /me/workspaces", () => { workspace: "acme", organization: { id: "org1", slug: "acme", name: "Acme Inc" }, role: "owner", - communal: false, hasPublicUrl: false, plan: "free", }, ]); }); - it("marks the communal workspace so the account UI doesn't infer it from the slug", async () => { - const env = stubEnv(USER, (path) => { - if (path === "/internal/memberships") { - return Response.json( - ownedMemberships([ - [COMMUNAL_WORKSPACE, "owner"], - ["acme", "owner"], - ]), - ); - } - return new Response(null, { status: 404 }); - }); - const res = await app().request("/me/workspaces", {}, env); - const body = (await res.json()) as { workspaces: { workspace: string; communal: boolean }[] }; - expect(body.workspaces.map((ws) => [ws.workspace, ws.communal])).toEqual([ - [COMMUNAL_WORKSPACE, true], - ["acme", false], - ]); - }); - it("flags hasPublicUrl for a workspace whose storage record has a publicBaseUrl", async () => { const env = stubEnv(USER, (path) => { if (path === "/internal/memberships") { @@ -204,7 +182,7 @@ describe("GET /me/workspaces", () => { expect(await quotaFor(env)).toEqual({ used: 2, cap: 3, allowed: true }); }); - it("loads the communal workspace's record like any other — the flag is not a short-circuit", async () => { + it("loads `default`'s record like any other workspace — no name-based short-circuit", async () => { const env = stubEnv(USER, (path) => { if (path === "/internal/memberships") { return Response.json([ @@ -228,7 +206,6 @@ describe("GET /me/workspaces", () => { workspace: "default", organization: { id: "org2", slug: "default", name: "Default" }, role: "member", - communal: true, hasPublicUrl: false, plan: "free", }, diff --git a/apps/api/src/routes/me.ts b/apps/api/src/routes/me.ts index 1234943e..982ee94c 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -12,7 +12,6 @@ import { resolveWorkspaceCreateQuota } from "@uploads/billing"; import { ForbiddenError, NotFoundError, RateLimitedError, ValidationError } from "@uploads/errors"; import { createFilesRouter, signedDownloadUrl } from "@uploads/storage"; -import { isCommunalWorkspace } from "@uploads/workspace"; import { Hono, type Context } from "hono"; import { usageWithLimits } from "../budget"; import { throwForInviteError } from "../invite-error"; @@ -53,20 +52,11 @@ interface MyWorkspace { workspace: string; organization: { id: string; slug: string; name: string }; role: string; - /** - * True for the shared tenant every account belongs to. Sent so clients - * don't have to re-derive a privileged-workspace rule from the slug: the - * account UI skips the communal workspace when picking which workspace to - * auto-open, and that is a server-owned fact, not a naming convention the - * browser should be pattern-matching. Free to compute — no extra lookup. - */ - communal: boolean; } function myWorkspaceFromMembership(membership: Membership, workspace: string): MyWorkspace { return { workspace, - communal: isCommunalWorkspace(workspace), organization: { id: membership.organizationId, slug: membership.organizationSlug, @@ -191,9 +181,7 @@ export const me = new Hono() // without a per-workspace billing round trip. Both fields are loaded here // (not in `myWorkspaces`, which every member-gated route calls for // authorization) so the extra KV read per workspace stays confined to this - // one listing endpoint. `communal` rides along from `myWorkspaces` — it - // costs no lookup, and it's what keeps the account UI from inferring the - // shared-tenant rule from the slug. + // one listing endpoint. .get("/workspaces", async (c) => { const userId = c.get("sessionUser")?.id; if (!userId) throw new NotFoundError("no session user", { code: "workspace_not_found" }); @@ -262,7 +250,6 @@ export const me = new Hono() workspace: ws.workspace, organization: ws.organization, role: ws.role, - communal: ws.communal, hasPublicUrl: Boolean(publicBaseUrl), publicBaseUrl, usage, diff --git a/apps/api/src/routes/tokens.test.ts b/apps/api/src/routes/tokens.test.ts index 43f67852..f1a1e0be 100644 --- a/apps/api/src/routes/tokens.test.ts +++ b/apps/api/src/routes/tokens.test.ts @@ -43,6 +43,8 @@ interface EnvOpts { db?: D1Database; /** When set, the WRITE_LIMITER binding reports this success value. */ writeLimitOk?: boolean; + /** Seeds the `ghlogin:` cache the workspace suggestion reads. */ + githubLogin?: string; } function stubEnv(opts: EnvOpts = {}): Env { @@ -53,6 +55,7 @@ function stubEnv(opts: EnvOpts = {}): Env { workspaces = { acme: { provider: "r2", bucket: "b" } }, db = captureDb().db, writeLimitOk, + githubLogin, } = opts; const auth = stubAuth((req) => { @@ -80,7 +83,18 @@ function stubEnv(opts: EnvOpts = {}): Env { const WRITE_LIMITER = writeLimitOk === undefined ? undefined : { limit: async () => ({ success: writeLimitOk }) }; - return { AUTH: auth, REGISTRY: registry, DB: db, WRITE_LIMITER } as unknown as Env; + const GITHUB_CACHE = { + get: async (key: string) => (githubLogin && key === `ghlogin:${USER.id}` ? githubLogin : null), + put: async () => {}, + }; + + return { + AUTH: auth, + REGISTRY: registry, + DB: db, + WRITE_LIMITER, + GITHUB_CACHE, + } as unknown as Env; } function app() { @@ -216,6 +230,37 @@ describe("GET /v1/tokens (workspace listing)", () => { expect(res.status).toBe(401); }); + // Issue #506: a name to prefill, offered only to an account with nothing to + // pick from. It is a hint — nothing is created and no name is claimed. + it("suggests a workspace derived from the GitHub login when the account has none", async () => { + const env = stubEnv({ memberships: [], workspaces: {}, githubLogin: "Octocat" }); + const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env); + expect(await res.json()).toEqual({ workspaces: [], suggestedWorkspace: "octocat" }); + }); + + it("omits the suggestion when the derived name is already taken", async () => { + const env = stubEnv({ + memberships: [], + workspaces: { octocat: { provider: "r2", bucket: "b" } }, + githubLogin: "Octocat", + }); + const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env); + expect(await res.json()).toEqual({ workspaces: [] }); + }); + + it("omits the suggestion when no GitHub login resolves", async () => { + const env = stubEnv({ memberships: [], workspaces: {} }); + const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env); + expect(await res.json()).toEqual({ workspaces: [] }); + }); + + it("does not suggest for an account that already has a workspace", async () => { + const env = stubEnv({ githubLogin: "Octocat" }); + const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env); + const body = (await res.json()) as { suggestedWorkspace?: string }; + expect(body.suggestedWorkspace).toBeUndefined(); + }); + it("lists memberships whose workspace still exists in KV", async () => { const env = stubEnv({ memberships: [ diff --git a/apps/api/src/routes/tokens.ts b/apps/api/src/routes/tokens.ts index 8fc08132..65ce85c7 100644 --- a/apps/api/src/routes/tokens.ts +++ b/apps/api/src/routes/tokens.ts @@ -31,6 +31,7 @@ import { type SessionVars, } from "../session-auth"; import { loadWorkspaceRecord, WS_NAME_RE } from "../workspace"; +import { suggestWorkspaceName } from "../workspace-suggestion"; const MAX_BODY_BYTES = 4096; const MAX_LABEL_LEN = 200; @@ -139,7 +140,20 @@ export const tokens = new Hono() }), ) ).filter((w): w is { workspace: string; role: string } => w !== null); - return c.json({ workspaces }); + + // Name to prefill when this account is about to create its first + // workspace (#506). Only computed when there is nothing to pick from — + // a user who already has a workspace is not being asked to name one, so + // the GitHub round-trip would be wasted. Omitted entirely when no clean + // candidate exists; the field being absent means "offer nothing", which + // is the pre-#506 behavior. + const suggestedWorkspace = + workspaces.length === 0 ? await suggestWorkspaceName(c.env, user.id) : null; + + return c.json({ + workspaces, + ...(suggestedWorkspace ? { suggestedWorkspace } : {}), + }); }) .post("/", sessionAuth, requireSessionUser, async (c) => { const contentLength = Number(c.req.header("Content-Length") ?? 0); diff --git a/apps/api/src/routes/workspaces.ts b/apps/api/src/routes/workspaces.ts index 611f9f63..4c6c7de9 100644 --- a/apps/api/src/routes/workspaces.ts +++ b/apps/api/src/routes/workspaces.ts @@ -39,6 +39,7 @@ import { sendWelcomeEmail } from "../welcome-email"; import { isPastGrace, isPurgedTombstone, + isWorkspaceNameTaken, loadWorkspaceRecord, loadWorkspaceRecordRaw, stampRestore, @@ -191,11 +192,10 @@ export const workspaces = new Hono().post( }); } - // Direct KV read (no cacheTtl) — a 60s-stale cached miss here could let a - // just-taken name through to the org 409 instead, which is fine, but a - // stale HIT must not block a genuinely free name. - const existing = await c.env.REGISTRY.get(`ws:${name}`); - if (existing !== null) { + // Raw occupancy, not `loadWorkspaceRecord` — soft-deleted records and + // purged tombstones still hold the key. Shared with the name-suggestion + // prefill so the two can't disagree about what "available" means. + if (await isWorkspaceNameTaken(c.env, name)) { throw new ConflictError("workspace name is taken", { code: "workspace_name_taken" }); } diff --git a/apps/api/src/slug-policy.ts b/apps/api/src/slug-policy.ts index ebd3b07e..54d0b06e 100644 --- a/apps/api/src/slug-policy.ts +++ b/apps/api/src/slug-policy.ts @@ -1,25 +1,26 @@ -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { WS_NAME_RE } from "./workspace"; import { SLUG_BLOCKLIST, SLUG_BLOCKLIST_ALLOW } from "./slug-blocklist"; /** * Names that collide with routes or subdomains. * - * `COMMUNAL_WORKSPACE` is redundant with the existing-name check today (the - * built-in communal workspace is always registered, so self-serve - * registration would already 409 `workspace_name_taken` without this entry) - * — but it stays reserved deliberately, not just for symmetry. Hard-delete is - * the only path that ever frees a slug (see docs/deletion.md), and the - * communal workspace is uniquely privileged: it's the communal/agent - * workspace exempted from the reaper and referenced by ops tooling, docs, and - * bootstrap scripts by name. If it were ever hard-deleted (accidentally or - * deliberately) and this entry were removed, self-serve registration would - * let any signed-in user reclaim the slug for an unrelated, non-communal - * workspace — a name-squatting/confusion risk this list exists to close for - * the other entries. Keeping it here costs nothing and closes that latent gap. + * `"default"` is redundant with the existing-name check today — it is a + * registered workspace, so self-serve registration would already 409 + * `workspace_name_taken` without this entry. It stays reserved anyway. + * Hard-delete is the only path that ever frees a slug (see docs/deletion.md), + * and `default` is the oldest workspace: years of public + * `storage.uploads.sh/default/…` URLs are embedded in merged PRs, issues, and + * chat history. If it were ever hard-deleted and this entry were gone, + * self-serve registration would let any signed-in user claim the slug and + * start serving their own bytes at paths those links still point at. Keeping + * it costs nothing and closes that gap. + * + * This is the only thing left that treats `default` differently, and it is a + * squatting guard on a name that served public traffic — not a claim that the + * workspace itself is special. It isn't; see the retired communal concept. */ export const RESERVED_WORKSPACE_NAMES: ReadonlySet = new Set([ - COMMUNAL_WORKSPACE, + "default", "admin", "api", "www", diff --git a/apps/api/src/workspace-suggestion.test.ts b/apps/api/src/workspace-suggestion.test.ts new file mode 100644 index 00000000..ac4b78be --- /dev/null +++ b/apps/api/src/workspace-suggestion.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "vitest"; +import { slugFromGithubLogin, suggestWorkspaceName } from "./workspace-suggestion"; + +/** + * `resolveUploaderLogin` short-circuits on a `ghlogin:` cache hit, so + * seeding that key exercises the whole suggestion path with no GitHub or AUTH + * round-trip. `login: null` instead leaves the cache empty and has AUTH report + * no linked account — the real "nothing to suggest from" path, rather than + * depending on the exact bytes of that module's private miss sentinel. + */ +function stubEnv( + opts: { login?: string | null; taken?: Record | string[] } = {}, +): Env { + const { login = "Octocat", taken = [] } = opts; + // An array is shorthand for "occupied by an ordinary record"; an object maps + // name → whatever blob sits at `ws:`, so tests can seed a soft-deleted + // record or a purged tombstone. + const occupied: Record = Array.isArray(taken) + ? Object.fromEntries(taken.map((n) => [n, { provider: "r2", bucket: "b" }])) + : taken; + return { + GITHUB_CACHE: { + get: async (key: string) => (login !== null && key === "ghlogin:u-1" ? login : null), + put: async () => {}, + }, + AUTH: { + fetch: async () => Response.json({ githubAccountId: null }), + }, + REGISTRY: { + get: async (key: string) => { + const name = key.startsWith("ws:") ? key.slice(3) : key; + // Mirrors a raw `REGISTRY.get` with no type option: the stored blob if + // the key exists, null otherwise. Occupancy is what matters, not shape. + return name in occupied ? JSON.stringify(occupied[name]) : null; + }, + }, + } as unknown as Env; +} + +describe("slugFromGithubLogin", () => { + it("lowercases a normal login", () => { + expect(slugFromGithubLogin("Octocat")).toBe("octocat"); + }); + + it("keeps internal hyphens", () => { + expect(slugFromGithubLogin("build-internet")).toBe("build-internet"); + }); + + it("trims leading and trailing hyphens so the slug starts alphanumeric", () => { + expect(slugFromGithubLogin("-edge-")).toBe("edge"); + }); + + it("collapses runs of hyphens", () => { + expect(slugFromGithubLogin("a--b")).toBe("a-b"); + }); + + // GitHub allows 1-character logins; our slugs require 2-63. Not representable. + it("returns null for a 1-char login", () => { + expect(slugFromGithubLogin("t")).toBeNull(); + }); + + it("returns null when nothing survives normalization", () => { + expect(slugFromGithubLogin("---")).toBeNull(); + }); +}); + +describe("suggestWorkspaceName", () => { + it("suggests the derived slug when it is valid and free", async () => { + expect(await suggestWorkspaceName(stubEnv(), "u-1")).toBe("octocat"); + }); + + it("offers nothing when no GitHub account is linked", async () => { + expect(await suggestWorkspaceName(stubEnv({ login: null }), "u-1")).toBeNull(); + }); + + it("offers nothing when the login is already a workspace", async () => { + expect(await suggestWorkspaceName(stubEnv({ taken: ["octocat"] }), "u-1")).toBeNull(); + }); + + // `loadWorkspaceRecord` hides these two, but they still hold the KV key and + // `POST /v1/workspaces` still 409s on them — so suggesting one would prefill + // a name that fails the moment the user submits it. + it("offers nothing when the name is held by a soft-deleted workspace", async () => { + const env = stubEnv({ + taken: { octocat: { provider: "r2", bucket: "b", deletedAt: "2026-07-01T00:00:00.000Z" } }, + }); + expect(await suggestWorkspaceName(env, "u-1")).toBeNull(); + }); + + it("offers nothing when the name is held by a purged tombstone", async () => { + const env = stubEnv({ taken: { octocat: { status: "purged" } } }); + expect(await suggestWorkspaceName(env, "u-1")).toBeNull(); + }); + + it("offers nothing for a reserved name", async () => { + // `admin` is a real GitHub login shape and a reserved slug. + expect(await suggestWorkspaceName(stubEnv({ login: "admin" }), "u-1")).toBeNull(); + }); + + it("offers nothing for a login that cannot become a valid slug", async () => { + expect(await suggestWorkspaceName(stubEnv({ login: "t" }), "u-1")).toBeNull(); + }); + + it("offers nothing rather than throwing when the login lookup fails", async () => { + const env = { + GITHUB_CACHE: { + get: async () => { + throw new Error("KV down"); + }, + put: async () => {}, + }, + REGISTRY: { get: async () => null }, + } as unknown as Env; + expect(await suggestWorkspaceName(env, "u-1")).toBeNull(); + }); +}); diff --git a/apps/api/src/workspace-suggestion.ts b/apps/api/src/workspace-suggestion.ts new file mode 100644 index 00000000..e8207eac --- /dev/null +++ b/apps/api/src/workspace-suggestion.ts @@ -0,0 +1,70 @@ +/** + * A suggested workspace name for a user who is about to create their first + * one (issue #506), derived from their linked GitHub login. + * + * This is a *hint*, never a decision. Nothing is provisioned here and no name + * is claimed — the caller renders the result into an editable field and the + * user confirms it. That framing is what makes the derivation tractable: the + * mapping from GitHub login to workspace slug is lossy in several ways (case, + * a 1-char length floor, reserved names, the profanity blocklist, collisions + * with existing workspaces), and every one of those is allowed to resolve to + * "no suggestion" here. An empty field is exactly today's behavior, so a + * failure at any step costs the user a prefill, never an error. + * + * Auto-*provisioning* from the same signal was considered and rejected: it + * would have to invent an answer for each of those cases (a suffix policy, a + * silent fallback ladder) and would claim a slug on every signup whether or + * not the account was ever used. + */ +import { validateSlug } from "./slug-policy"; +import { resolveUploaderLogin } from "./uploader-identity"; +import { isWorkspaceNameTaken } from "./workspace"; + +/** + * Reduce a GitHub login to a candidate slug, or null when it cannot become + * one. GitHub allows alphanumerics and single hyphens, 1–39 chars; our slugs + * are lowercase, 2–63, and must start alphanumeric — so lowercasing plus a + * defensive scrub covers the shape, and the length floor is a real rejection + * (1-char logins like `t` exist and cannot be represented). + */ +export function slugFromGithubLogin(login: string): string | null { + const candidate = login + .toLowerCase() + .replace(/[^a-z0-9-]/g, "-") + .replace(/-+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 63); + return candidate.length >= 2 ? candidate : null; +} + +/** + * The name to prefill for `userId`, or null for "offer nothing". + * + * Null on every one of: no linked GitHub account, a login lookup that failed + * or was rate-limited, a login that cannot become a valid slug, a slug that is + * reserved or blocklisted, and a slug already taken. + * + * "Taken" goes through `isWorkspaceNameTaken`, the same raw-occupancy check + * `POST /v1/workspaces` uses — NOT `loadWorkspaceRecord`, which hides + * soft-deleted records and purged tombstones even though they still hold the + * slug. Using the lenient lookup here would prefill names that creation then + * rejects with `workspace_name_taken`. + * + * That check is deliberately the *only* availability lookup exposed here, and + * it runs against a server-derived candidate, never a client-supplied string — + * so this cannot be turned into a name-enumeration oracle. Learning that your + * own login is taken is no more than you'd learn by trying to create it. + */ +export async function suggestWorkspaceName(env: Env, userId: string): Promise { + // No repo hint: `resolveUploaderLogin` falls through to the unauthenticated + // GitHub endpoint, which is IP-rate-limited. Acceptable because the result + // is cached per user in KV and a miss simply means no suggestion. + const login = await resolveUploaderLogin(env, userId, undefined); + if (!login) return null; + + const candidate = slugFromGithubLogin(login); + if (!candidate) return null; + if (!validateSlug(candidate).ok) return null; + + return (await isWorkspaceNameTaken(env, candidate)) ? null : candidate; +} diff --git a/apps/api/src/workspace.ts b/apps/api/src/workspace.ts index 0aaff823..273a67d8 100644 --- a/apps/api/src/workspace.ts +++ b/apps/api/src/workspace.ts @@ -249,6 +249,28 @@ export async function loadWorkspaceRecord( return { ...record, name }; } +/** + * Whether `ws:` is occupied, for the purposes of registering that name. + * + * Deliberately NOT `loadWorkspaceRecord(...) !== null`. That function collapses + * soft-deleted records and purged tombstones to "not found" so every auth and + * serving path 404s uniformly — but those records still hold the KV key, and + * the slug stays unregistrable (see docs/deletion.md). Anything deciding + * whether a name can be *taken* must ask this question instead, or it will + * disagree with the creation path. + * + * Both `POST /v1/workspaces` and the workspace-name suggestion (#506) gate on + * this, so a prefilled name can never be one creation would reject with + * `workspace_name_taken`. + * + * No `cacheTtl`: a 60s-stale cached miss would let a just-taken name through to + * the org's own uniqueness check, which is fine, but a stale HIT must not block + * a genuinely free name. + */ +export async function isWorkspaceNameTaken(env: Env, name: string): Promise { + return (await env.REGISTRY.get(`ws:${name}`)) !== null; +} + /** * Unfiltered read of `ws:` — used by admin routes and the retention * sweep, which need to see soft-deleted records and purged tombstones rather diff --git a/apps/api/test/content-hash.test.ts b/apps/api/test/content-hash.test.ts index 7b9632e2..4d8927f4 100644 --- a/apps/api/test/content-hash.test.ts +++ b/apps/api/test/content-hash.test.ts @@ -1,4 +1,3 @@ -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { describe, expect, it } from "vitest"; import { applyInheritedMetaAdditively, @@ -68,18 +67,17 @@ describe("inheritableMetaForHash", () => { } }); - it("never inherits in the communal workspace, even on an exact byte match", async () => { + it("inherits in `default` like any other workspace", async () => { const sqlite = new SqliteD1(MIGRATIONS); try { - // `default` is the one shared tenant every account belongs to, so a byte - // match there says nothing about the two uploads being related. - await seedDonor(sqlite, COMMUNAL_WORKSPACE, "f/one.png", HASH_A, { - path: "/admin/billing", - }); + // `default` was excluded while it was the communal tenant every account + // belonged to. That concept is retired (#505) — it is an ordinary + // workspace, so the ordinary rule applies. + await seedDonor(sqlite, "default", "f/one.png", HASH_A, { path: "/admin/billing" }); expect( - await inheritableMetaForHash(database(sqlite), COMMUNAL_WORKSPACE, HASH_A, "f/two.png"), - ).toEqual({}); + await inheritableMetaForHash(database(sqlite), "default", HASH_A, "f/two.png"), + ).toEqual({ path: "/admin/billing" }); } finally { sqlite.close(); } diff --git a/apps/web/package.json b/apps/web/package.json index 986e0c42..5544c849 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -21,7 +21,6 @@ "@astrojs/react": "^6.0.1", "@uploads/billing": "workspace:^", "@uploads/ui": "workspace:*", - "@uploads/workspace": "workspace:^", "astro": "^7.0.6", "files-sdk": "^2.1.0", "markdown-for-agents": "^1.3.4", diff --git a/apps/web/src/lib/api-client.test.ts b/apps/web/src/lib/api-client.test.ts index b0ff90a2..eec84f77 100644 --- a/apps/web/src/lib/api-client.test.ts +++ b/apps/web/src/lib/api-client.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { getGithubInstalled, getMyWorkspaces, + getSuggestedWorkspaceName, getWorkspaceInvites, getWorkspaceMembers, inviteToWorkspace, @@ -160,48 +161,6 @@ describe("getMyWorkspaces", () => { ["byo", undefined], ]); }); - - it("trusts the api's communal flag and falls back to the slug only when it is absent", async () => { - vi.stubGlobal( - "fetch", - vi.fn(async () => - Response.json({ - workspaces: [ - { - workspace: "acme", - organization: { id: "org1", slug: "acme", name: "Acme Inc" }, - role: "owner", - communal: true, - }, - { - // The api's own answer wins even for the communal slug. - workspace: "default", - organization: { id: "org2", slug: "default", name: "default" }, - role: "member", - communal: false, - }, - { - // Older api response, no communal field: fall back to the slug - // so the auto-open skip can't silently stop working. - workspace: "default", - organization: { id: "org3", slug: "default", name: "default" }, - role: "member", - }, - { - workspace: "byo", - organization: { id: "org4", slug: "byo", name: "BYO Inc" }, - role: "member", - }, - ], - }), - ), - ); - - const result = await getMyWorkspaces("http://127.0.0.1:8787"); - expect(result.kind).toBe("success"); - if (result.kind !== "success") throw new Error("expected success"); - expect(result.workspaces.map((ws) => ws.communal)).toEqual([true, false, true, false]); - }); }); describe("setFileVisibility", () => { @@ -720,3 +679,41 @@ describe("getGithubInstalled", () => { } }); }); + +describe("getSuggestedWorkspaceName", () => { + it("returns the api's suggestion", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => Response.json({ workspaces: [], suggestedWorkspace: "octocat" })), + ); + expect(await getSuggestedWorkspaceName("http://127.0.0.1:8787")).toBe("octocat"); + }); + + it("returns an empty string when the api offers nothing", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => Response.json({ workspaces: [] })), + ); + expect(await getSuggestedWorkspaceName("http://127.0.0.1:8787")).toBe(""); + }); + + // Every failure collapses to "no prefill" — an empty field is the behavior + // this feature replaced, so it is always a safe answer. + it("returns an empty string on a non-ok response", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response(null, { status: 401 })), + ); + expect(await getSuggestedWorkspaceName("http://127.0.0.1:8787")).toBe(""); + }); + + it("returns an empty string when the api is unreachable", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("network down"); + }), + ); + expect(await getSuggestedWorkspaceName("http://127.0.0.1:8787")).toBe(""); + }); +}); diff --git a/apps/web/src/lib/api-client.ts b/apps/web/src/lib/api-client.ts index 90982e94..af6c43bc 100644 --- a/apps/web/src/lib/api-client.ts +++ b/apps/web/src/lib/api-client.ts @@ -6,7 +6,6 @@ * rather than rendering an outage as an empty account; less central detail * helpers retain their defensive null/[] fallbacks. */ -import { isCommunalWorkspace } from "@uploads/workspace"; import { fetchWithTimeout, type RequestFailure } from "./request"; import { buildSearchQuery, type MetaFilter } from "./workspace-search-url"; @@ -34,22 +33,15 @@ export interface MyWorkspace { * "pro". Undefined only against an older api build that omits the field. */ plan?: string; - /** - * True for the shared/communal tenant every account belongs to. Server- - * supplied (`/me/workspaces`) so the UI reads a business rule rather than - * matching a slug; `mapMyWorkspace` falls back to the slug only for an - * older api build that predates the field. - */ - communal: boolean; } /** - * The fields every api build has always sent. `hasPublicUrl`/`plan`/`communal` + * The fields every api build has always sent. `hasPublicUrl`/`plan` * are deliberately excluded: web and api deploy independently, so an older api * may omit them — the entry is still accepted and `mapMyWorkspace` coerces each * missing value to a safe default. */ -type MyWorkspaceCore = Omit; +type MyWorkspaceCore = Omit; function isMyWorkspaceCore(value: unknown): value is MyWorkspaceCore { if (!value || typeof value !== "object") return false; @@ -65,23 +57,12 @@ function isMyWorkspaceCore(value: unknown): value is MyWorkspaceCore { ); } -/** - * Coerce optional/legacy fields; shared by list + summary mappers. - * - * `communal` is the one field with a non-constant fallback: it is a rule the - * api owns, but an api build older than the field would omit it, and reading - * that absence as "not communal" would silently un-skip the shared workspace - * in `resolveDefaultWorkspace`. So a missing flag — and only a missing flag — - * falls back to the shared slug constant. This is the sole place in the web - * app that knows the communal slug at all; everything downstream reads the - * boolean. - */ +/** Coerce optional/legacy fields; shared by list + summary mappers. */ function mapMyWorkspace(ws: MyWorkspaceCore): MyWorkspace { const raw = ws as MyWorkspaceCore & { hasPublicUrl?: unknown; publicBaseUrl?: unknown; plan?: unknown; - communal?: unknown; }; return { workspace: raw.workspace, @@ -90,7 +71,6 @@ function mapMyWorkspace(ws: MyWorkspaceCore): MyWorkspace { hasPublicUrl: raw.hasPublicUrl === true, publicBaseUrl: typeof raw.publicBaseUrl === "string" ? raw.publicBaseUrl : undefined, plan: typeof raw.plan === "string" ? raw.plan : undefined, - communal: typeof raw.communal === "boolean" ? raw.communal : isCommunalWorkspace(raw.workspace), }; } @@ -876,6 +856,26 @@ export async function getGithubInstalled(apiOrigin: string, name: string): Promi return body?.installed === true; } +/** + * A workspace name to prefill for an account that has none yet (issue #506), + * derived server-side from the user's GitHub login. Empty string for "offer + * nothing" — no linked GitHub account, a login that can't become a valid slug, + * a reserved or blocklisted name, or one already taken. Every failure mode is + * indistinguishable here on purpose: the caller either has a name to prefill + * or doesn't, and an empty field is the pre-#506 behavior. + */ +export async function getSuggestedWorkspaceName(apiOrigin: string): Promise { + const result = await fetchWithTimeout(`${trimOrigin(apiOrigin)}/v1/tokens`, { + credentials: "include", + cache: "no-store", + }); + if (result.kind === "unavailable" || !result.response.ok) return ""; + const body = (await result.response.json().catch(() => null)) as { + suggestedWorkspace?: unknown; + } | null; + return typeof body?.suggestedWorkspace === "string" ? body.suggestedWorkspace : ""; +} + export interface WorkspaceFolderFile { key: string; url: string | null; diff --git a/apps/web/src/lib/workspace-file-row.test.ts b/apps/web/src/lib/workspace-file-row.test.ts index 048b706c..ba432383 100644 --- a/apps/web/src/lib/workspace-file-row.test.ts +++ b/apps/web/src/lib/workspace-file-row.test.ts @@ -170,7 +170,6 @@ function mkWorkspace(workspace: string, overrides: Partial = {}): M organization: { id: "1", slug: workspace, name: workspace }, role: "member", hasPublicUrl: false, - communal: false, ...overrides, }; } diff --git a/apps/web/src/lib/workspaces-nav.test.ts b/apps/web/src/lib/workspaces-nav.test.ts index 49f90835..57fbb554 100644 --- a/apps/web/src/lib/workspaces-nav.test.ts +++ b/apps/web/src/lib/workspaces-nav.test.ts @@ -55,14 +55,12 @@ const sample: MyWorkspace[] = [ organization: { id: "1", slug: "buildinternet", name: "buildinternet" }, role: "admin", hasPublicUrl: true, - communal: false, }, { workspace: "side", organization: { id: "2", slug: "side", name: "Side Project" }, role: "member", hasPublicUrl: false, - communal: false, }, ]; @@ -228,21 +226,18 @@ describe("isManageRequest", () => { }); describe("resolveDefaultWorkspace", () => { - const multi = [ - { workspace: "buildinternet", communal: false }, - { workspace: "side", communal: false }, - ]; + const multi = [{ workspace: "buildinternet" }, { workspace: "side" }]; it("picks the only membership, else a valid last-used", () => { - expect(resolveDefaultWorkspace([{ workspace: "solo", communal: false }], "other")).toBe("solo"); + expect(resolveDefaultWorkspace([{ workspace: "solo" }], "other")).toBe("solo"); expect(resolveDefaultWorkspace(multi, "side")).toBe("side"); }); it("falls back to the first owned workspace when no last-used is stored", () => { const roles = [ - { workspace: "invited", role: "member", communal: false }, - { workspace: "mine", role: "owner", communal: false }, - { workspace: "other", role: "owner", communal: false }, + { workspace: "invited", role: "member" }, + { workspace: "mine", role: "owner" }, + { workspace: "other", role: "owner" }, ]; expect(resolveDefaultWorkspace(roles, "")).toBe("mine"); // A stale slug the user is no longer a member of takes the same path. @@ -251,8 +246,8 @@ describe("resolveDefaultWorkspace", () => { it("prefers an administered workspace when none is owned", () => { const roles = [ - { workspace: "invited", role: "member", communal: false }, - { workspace: "runs-it", role: "admin", communal: false }, + { workspace: "invited", role: "member" }, + { workspace: "runs-it", role: "admin" }, ]; expect(resolveDefaultWorkspace(roles, "")).toBe("runs-it"); }); @@ -262,43 +257,32 @@ describe("resolveDefaultWorkspace", () => { expect( resolveDefaultWorkspace( [ - { workspace: "one", role: "member", communal: false }, - { workspace: "two", role: "member", communal: false }, + { workspace: "one", role: "member" }, + { workspace: "two", role: "member" }, ], "", ), ).toBe("one"); }); - // The skip keys off the server-supplied `communal` flag, never the slug — - // these fixtures name the workspace "shared" precisely to prove that. - const communalPair = [ - { workspace: "shared", role: "owner", communal: true }, - { workspace: "buildinternet", role: "admin", communal: false }, - ]; - - it("skips a communal workspace so an owner there still lands in their own workspace", () => { - expect(resolveDefaultWorkspace(communalPair, "")).toBe("buildinternet"); - }); - - it("still opens the communal workspace when it is the only membership", () => { - expect( - resolveDefaultWorkspace([{ workspace: "shared", role: "owner", communal: true }], ""), - ).toBe("shared"); - }); - - it("honours an explicit last-used communal workspace over the skip", () => { - expect(resolveDefaultWorkspace(communalPair, "shared")).toBe("shared"); + // `default` used to be skipped here as the communal tenant. That concept is + // retired (#505) — no workspace is special, including this one. Role order + // decides, so an owner of `default` and admin elsewhere now lands in + // `default`. This is the user-visible change the retirement makes. + it("treats `default` as an ordinary workspace, ranked by role like any other", () => { + const pair = [ + { workspace: "default", role: "owner" }, + { workspace: "buildinternet", role: "admin" }, + ]; + expect(resolveDefaultWorkspace(pair, "")).toBe("default"); }); - it("does not skip the communal slug when the flag says otherwise", () => { - // An entry named `default` that the api did not mark communal is an - // ordinary workspace here — the client no longer reads the slug. - const roles = [ - { workspace: "default", role: "owner", communal: false }, - { workspace: "buildinternet", role: "member", communal: false }, + it("still honours an explicit last-used workspace over role order", () => { + const pair = [ + { workspace: "default", role: "owner" }, + { workspace: "buildinternet", role: "admin" }, ]; - expect(resolveDefaultWorkspace(roles, "")).toBe("default"); + expect(resolveDefaultWorkspace(pair, "buildinternet")).toBe("buildinternet"); }); it("returns null only when there are no memberships", () => { diff --git a/apps/web/src/lib/workspaces-nav.ts b/apps/web/src/lib/workspaces-nav.ts index 35aa61aa..0f0cc1f6 100644 --- a/apps/web/src/lib/workspaces-nav.ts +++ b/apps/web/src/lib/workspaces-nav.ts @@ -199,32 +199,25 @@ const FALLBACK_ROLES = ["owner", "admin"]; * workspace rather than being asked to choose every time. The switcher, not * this page, is how you change workspaces. * - * A communal workspace (`communal`, set by the api — most accounts belong to - * the shared one and many are `owner` there, so a naive role-first fallback - * would land almost everyone in it) is skipped at every step of that fallback - * and only used when it is the sole membership, so a user who happens to be - * `owner` there still lands in their own workspace. The flag comes from the - * server precisely so this file doesn't encode which slug that is. + * No workspace is treated specially here. `default` used to be skipped at + * every step as the shared/communal tenant; that concept is retired and it is + * now an ordinary workspace like any other. * * Null only when there are no memberships at all. The index suppresses the * auto-open entirely when it was reached deliberately (`?manage=1`). */ export function resolveDefaultWorkspace( - // `communal` is required, not optional: an optional flag would let a future - // caller omit it and silently un-skip the shared workspace with no type - // error — exactly the drift this stopped inferring from the slug to avoid. - workspaces: readonly { workspace: string; role?: string; communal: boolean }[], + workspaces: readonly { workspace: string; role?: string }[], lastActive = "", ): string | null { if (workspaces.length === 1) return workspaces[0]!.workspace; if (lastActive && workspaces.some((ws) => ws.workspace === lastActive)) return lastActive; - const own = workspaces.filter((ws) => !ws.communal); for (const role of FALLBACK_ROLES) { - const match = own.find((ws) => ws.role === role); + const match = workspaces.find((ws) => ws.role === role); if (match) return match.workspace; } - return own[0]?.workspace ?? workspaces[0]?.workspace ?? null; + return workspaces[0]?.workspace ?? null; } /** diff --git a/apps/web/src/pages/account/workspaces/new.astro b/apps/web/src/pages/account/workspaces/new.astro index 182ce20f..263d6905 100644 --- a/apps/web/src/pages/account/workspaces/new.astro +++ b/apps/web/src/pages/account/workspaces/new.astro @@ -75,7 +75,11 @@ export const prerender = false; diff --git a/apps/web/src/pages/device.astro b/apps/web/src/pages/device.astro index 8a9a27aa..44daff1e 100644 --- a/apps/web/src/pages/device.astro +++ b/apps/web/src/pages/device.astro @@ -215,7 +215,7 @@ applyAuthSecurityHeaders(Astro.response.headers, devicePageCsp(authOrigin, apiOr setDeviceWorkspace, signInWithGitHub, } from "../lib/auth-client"; - import { createWorkspace } from "../lib/api-client"; + import { createWorkspace, getSuggestedWorkspaceName } from "../lib/api-client"; import { resolveDeviceWorkspaceState, type DeviceWorkspaceState } from "../lib/device-workspace"; import { createErrorCopy } from "../lib/workspace-ui"; import { clearCachedWorkspaces } from "../lib/workspaces-nav"; @@ -393,6 +393,7 @@ applyAuthSecurityHeaders(Astro.response.headers, devicePageCsp(authOrigin, apiOr wsNew.hidden = false; wsNote.textContent = "Your account doesn't have a workspace yet — name one to create it."; wsNote.hidden = false; + prefillWorkspaceName(); return; } wsSelect.replaceChildren(); @@ -502,6 +503,22 @@ applyAuthSecurityHeaders(Astro.response.headers, devicePageCsp(authOrigin, apiOr })(); }); + /** + * Prefill the new-workspace field with the server's GitHub-derived + * suggestion (issue #506). Fetched at most once, and only applied while the + * field is still untouched, so a user who starts typing is never overwritten + * by a late response. Any failure leaves the field empty — the pre-#506 + * behavior — so this never blocks or delays the approval. + */ + let prefillRequested = false; + function prefillWorkspaceName(): void { + if (prefillRequested) return; + prefillRequested = true; + void getSuggestedWorkspaceName(apiOrigin).then((suggestion) => { + if (suggestion && !wsName.value) wsName.value = suggestion; + }); + } + /** * Resolve the workspace this approval should mint for, creating one first * when the user asked for that. Returns the slug, `null` when there's diff --git a/docs/admin-tokens.md b/docs/admin-tokens.md index afadd88a..8abf4c5b 100644 --- a/docs/admin-tokens.md +++ b/docs/admin-tokens.md @@ -39,8 +39,8 @@ curl -XPOST https://api.uploads.sh/admin/tokens \ ``` Omitting it is a `400 workspace_required` rather than a fallback. These routes -used to default to the communal `default` workspace, which meant a forgotten -field silently issued a credential against the shared tenant — and since +used to default to the shared `default` workspace, which meant a forgotten +field silently issued a credential against that workspace — and since enrollment redemption mints a token without creating an org membership, the recipient would not show up in any member list while still reading and writing that workspace's files. Naming `default` explicitly is still allowed. diff --git a/docs/enrollment.md b/docs/enrollment.md index 9add1b82..16ed68dd 100644 --- a/docs/enrollment.md +++ b/docs/enrollment.md @@ -59,7 +59,10 @@ admins a workspace, or an **enrollment code** shared out-of-band. Any signed-in user with a **GitHub-linked account** can create a workspace without an invitation or `ADMIN_TOKEN` — `/account/workspaces` has a "Create a workspace" form, and `uploads login` offers the same prompt when your account -has no workspaces yet. Scripted or agent logins can skip the prompt with +has no workspaces yet. Both suggest a name based on your GitHub login when that +makes a valid, unclaimed workspace name; edit or replace it freely. When no name +can be derived — no linked GitHub account, or the name is reserved or already +taken — nothing is prefilled. Scripted or agent logins can skip the prompt with `uploads login --workspace --create`, which provisions the workspace during login when the account doesn't already have it (browser device approval is still required once). You become the owner of a new organization and a diff --git a/packages/email/package.json b/packages/email/package.json index 7cb5542e..64d030cc 100644 --- a/packages/email/package.json +++ b/packages/email/package.json @@ -7,9 +7,6 @@ "exports": { ".": "./src/index.ts" }, - "dependencies": { - "@uploads/workspace": "workspace:^" - }, "scripts": { "typecheck": "tsc --noEmit", "test": "vitest run" diff --git a/packages/email/src/invites.ts b/packages/email/src/invites.ts index 77327952..6b6ac344 100644 --- a/packages/email/src/invites.ts +++ b/packages/email/src/invites.ts @@ -1,4 +1,3 @@ -import { isCommunalWorkspace } from "@uploads/workspace"; import { escapeHtml, renderEmailCard, strong, type RenderedEmail } from "./card"; const CTA = "Accept invitation →"; @@ -37,10 +36,7 @@ export function renderOrgInvitationEmail(ctx: { }); } -/** - * Token enrollment invite (console / CLI → /invite#code). - * The communal workspace is framed as access to uploads.sh itself. - */ +/** Token enrollment invite (console / CLI → /invite#code). */ export function renderEnrollmentInvitationEmail(ctx: { workspaceName: string; link: string; @@ -55,10 +51,7 @@ export function renderEnrollmentInvitationEmail(ctx: { timeZone: "UTC", timeZoneName: "short", }); - const isDefault = isCommunalWorkspace(ctx.workspaceName); - const invitedTo = isDefault - ? "You've been given access to uploads.sh" - : `You've been invited to the ${ctx.workspaceName} workspace on uploads.sh`; + const invitedTo = `You've been invited to the ${ctx.workspaceName} workspace on uploads.sh`; let webOrigin = "https://uploads.sh"; try { webOrigin = new URL(ctx.link).origin; @@ -67,15 +60,11 @@ export function renderEnrollmentInvitationEmail(ctx: { } return renderEmailCard({ - subject: isDefault - ? "You've been given access to uploads.sh" - : `You're invited to ${ctx.workspaceName} on uploads.sh`, + subject: `You're invited to ${ctx.workspaceName} on uploads.sh`, preheader: `${invitedTo} — one click to accept, link expires ${expires}.`, eyebrow: "Invitation", title: "You're invited", - bodyHtml: isDefault - ? `You've been given access to ${strong("uploads.sh")} — ${PITCH}.` - : `You've been invited to the ${strong(ctx.workspaceName)} workspace on uploads.sh — ${PITCH}.`, + bodyHtml: `You've been invited to the ${strong(ctx.workspaceName)} workspace on uploads.sh — ${PITCH}.`, text: [ `${invitedTo} — ${PITCH}.`, "", diff --git a/packages/email/test/invites.test.ts b/packages/email/test/invites.test.ts index aa0d2ca5..34a8267a 100644 --- a/packages/email/test/invites.test.ts +++ b/packages/email/test/invites.test.ts @@ -160,16 +160,16 @@ describe("renderOrgInvitationEmail", () => { }); describe("renderEnrollmentInvitationEmail", () => { - it("special-cases the default workspace subject and body", () => { + it("names `default` like any other workspace — no special casing", () => { const link = "https://uploads.sh/invite?id=upi_x#code=secret"; const out = renderEnrollmentInvitationEmail({ workspaceName: "default", link, expiresAt: "2030-01-15T12:00:00.000Z", }); - expect(out.subject).toBe("You've been given access to uploads.sh"); + expect(out.subject).toBe("You're invited to default on uploads.sh"); expect(out.text).toContain("#code=secret"); - expect(out.html).toContain("You've been given access"); + expect(out.text).toContain("You've been invited to the default workspace on uploads.sh"); expect(out.html).toContain("laptop"); expect(parseJsonLd(out.html)?.potentialAction).toEqual({ "@type": "ViewAction", diff --git a/packages/uploads/src/client.ts b/packages/uploads/src/client.ts index 8889d494..780b93b8 100644 --- a/packages/uploads/src/client.ts +++ b/packages/uploads/src/client.ts @@ -603,7 +603,7 @@ export interface MintWorkspaceSummary { export function listMintWorkspaces( apiUrl: string, accessToken: string, -): Promise<{ workspaces: MintWorkspaceSummary[] }> { +): Promise<{ workspaces: MintWorkspaceSummary[]; suggestedWorkspace?: string }> { return jsonRequest(`${apiUrl.replace(/\/$/, "")}/v1/tokens`, { headers: { Authorization: `Bearer ${accessToken}` }, }); diff --git a/packages/uploads/src/commands/config.ts b/packages/uploads/src/commands/config.ts index 4ee59bfe..10b0343b 100644 --- a/packages/uploads/src/commands/config.ts +++ b/packages/uploads/src/commands/config.ts @@ -182,9 +182,13 @@ Examples: if (workspace) keys.UPLOADS_WORKSPACE = workspace; if (token) keys.UPLOADS_TOKEN = token; + // Seed only the API URL. A `UPLOADS_WORKSPACE` written here outranks the + // workspace encoded in the token itself (see `resolveConfig`'s precedence), + // so seeding one would pin every later `uploads login` to whatever this + // wrote — historically `default` — no matter which workspace the user's + // token was actually minted for. if (Object.keys(keys).length === 0) { keys.UPLOADS_API_URL = DEFAULT_API_URL; - keys.UPLOADS_WORKSPACE = DEFAULT_WORKSPACE; } try { diff --git a/packages/uploads/src/commands/login.ts b/packages/uploads/src/commands/login.ts index 40ef7ff7..3e94ec5d 100644 --- a/packages/uploads/src/commands/login.ts +++ b/packages/uploads/src/commands/login.ts @@ -216,16 +216,27 @@ export interface DeviceLoginIo { write: (text: string) => void; /** Whether the CLI can prompt the user (a real TTY, not a script/CI pipe). */ isTTY: boolean; - /** Prompt for a new workspace name when the account has zero. */ - promptWorkspaceName: () => Promise; + /** + * Prompt for a new workspace name when the account has zero. `suggestion` + * is a server-derived default (the user's GitHub login, when it makes a + * valid and available slug) offered as a bracketed default that Enter + * accepts; it is always overridable and never auto-submitted. + */ + promptWorkspaceName: (suggestion?: string) => Promise; } -async function promptWorkspaceName(): Promise { +async function promptWorkspaceName(suggestion?: string): Promise { const rl = createInterface({ input: stdin, output: process.stderr }); + const hint = suggestion ? ` [${suggestion}]` : ""; try { - return ( - await rl.question("no workspaces yet — enter a name to create one (lowercase, hyphens): ") + const answer = ( + await rl.question( + `no workspaces yet — enter a name to create one (lowercase, hyphens)${hint}: `, + ) ).trim(); + // Empty input accepts the offered default; with no suggestion it stays + // empty and the caller's "cancelled" guard fires as before. + return answer || (suggestion ?? ""); } finally { rl.close(); } @@ -449,7 +460,7 @@ async function resolveMintWorkspace( ); return created.name; } - const { workspaces } = await listMintWorkspaces(apiUrl, accessToken); + const { workspaces, suggestedWorkspace } = await listMintWorkspaces(apiUrl, accessToken); if (workspaces.length === 1) return workspaces[0]!.workspace; if (workspaces.length === 0) { if (!io.isTTY) { @@ -457,7 +468,7 @@ async function resolveMintWorkspace( "your account has no workspace access yet — pass `--workspace --create` to provision one, run `uploads login` interactively, or ask an administrator for an invitation", ); } - const name = (await io.promptWorkspaceName()).trim(); + const name = (await io.promptWorkspaceName(suggestedWorkspace)).trim(); if (!name) throw new UsageError("workspace creation cancelled"); const created = await createWorkspaceRequest(apiUrl, accessToken, name); io.write( diff --git a/packages/uploads/test/commands-login.test.ts b/packages/uploads/test/commands-login.test.ts index ec0f0704..91fd25e5 100644 --- a/packages/uploads/test/commands-login.test.ts +++ b/packages/uploads/test/commands-login.test.ts @@ -470,6 +470,80 @@ describe("runLogin device flow", () => { expect(loadConfigFile(path).UPLOADS_TOKEN).toBe(token); }); + // Issue #506: the API offers a name derived from the user's GitHub login + // when the account has no workspace yet. It is a prefill, never a decision — + // the prompt still runs and the user can override or ignore it. + it("passes the api's suggested workspace to the prompt as a default", async () => { + const path = join(mkdtempSync(join(tmpdir(), "uploads-login-")), "config"); + const token = "up_octocat_abcdefghijklmnopqrstuvwxyz"; + vi.spyOn(globalThis, "fetch") + .mockResolvedValueOnce(deviceCode()) + .mockResolvedValueOnce( + response({ access_token: "sess-tok", token_type: "Bearer", expires_in: 3600, scope: "" }), + ) + .mockResolvedValueOnce(response({ workspaces: [], suggestedWorkspace: "octocat" })) + .mockResolvedValueOnce( + response( + { + workspace: { + name: "octocat", + publicBaseUrl: "https://storage.uploads.sh/octocat", + selfServe: true, + }, + }, + 201, + ), + ) + .mockResolvedValueOnce( + response( + { token, workspace: "octocat", scopes: ["files:read"], label: "host", expiresAt: null }, + 201, + ), + ); + captureOutput(); + + let offered: string | undefined = "NOT CALLED"; + expect( + await runLogin(["--path", path, "--no-check"], { json: true }, false, { + ...silentIo, + isTTY: true, + promptWorkspaceName: async (suggestion?: string) => { + offered = suggestion; + // Simulate the user accepting the offered default. + return suggestion ?? ""; + }, + }), + ).toBe(0); + + expect(offered).toBe("octocat"); + expect(loadConfigFile(path).UPLOADS_WORKSPACE).toBe("octocat"); + }); + + it("offers no default when the api sends no suggestion", async () => { + const path = join(mkdtempSync(join(tmpdir(), "uploads-login-")), "config"); + vi.spyOn(globalThis, "fetch") + .mockResolvedValueOnce(deviceCode()) + .mockResolvedValueOnce( + response({ access_token: "sess-tok", token_type: "Bearer", expires_in: 3600, scope: "" }), + ) + .mockResolvedValueOnce(response({ workspaces: [] })); + captureOutput(); + + let offered: string | undefined = "NOT CALLED"; + // Accepting nothing with no suggestion must still cancel, exactly as before. + await expect( + runLogin(["--path", path, "--no-check"], { json: true }, false, { + ...silentIo, + isTTY: true, + promptWorkspaceName: async (suggestion?: string) => { + offered = suggestion; + return ""; + }, + }), + ).rejects.toThrow(/workspace creation cancelled/); + expect(offered).toBeUndefined(); + }); + it("points at account/profile when workspace creation requires GitHub", async () => { const path = join(mkdtempSync(join(tmpdir(), "uploads-login-")), "config"); vi.spyOn(globalThis, "fetch") diff --git a/packages/uploads/test/config-init.test.ts b/packages/uploads/test/config-init.test.ts new file mode 100644 index 00000000..97518817 --- /dev/null +++ b/packages/uploads/test/config-init.test.ts @@ -0,0 +1,54 @@ +/** + * `uploads config init` with no flags used to seed `UPLOADS_WORKSPACE=default` + * into the config file. That entry outranks the workspace encoded in the token + * itself (see `resolveConfig`'s precedence: flag → env → env-file → config file + * → token), so it pinned every later login to `default` regardless of which + * workspace the user's token was actually minted for. Seeding the API URL has + * no such conflict — there is no "API URL embedded in the token" concept. + */ +import { mkdtempSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runConfig } from "../src/commands/config.js"; +import { loadConfigFile } from "../src/config-file.js"; + +afterEach(() => vi.restoreAllMocks()); + +function silence() { + vi.spyOn(process.stdout, "write").mockImplementation((() => true) as typeof process.stdout.write); + vi.spyOn(process.stderr, "write").mockImplementation((() => true) as typeof process.stderr.write); +} + +function freshPath() { + return join(mkdtempSync(join(tmpdir(), "uploads-config-init-")), "config"); +} + +describe("uploads config init", () => { + it("seeds the API URL but never a workspace when no flags are passed", async () => { + const path = freshPath(); + silence(); + expect(await runConfig(["init", "--path", path], {})).toBe(0); + const written = loadConfigFile(path); + expect(written.UPLOADS_API_URL).toBe("https://api.uploads.sh"); + expect(written.UPLOADS_WORKSPACE).toBeUndefined(); + }); + + it("still writes a workspace when one is passed explicitly", async () => { + const path = freshPath(); + silence(); + expect(await runConfig(["init", "--workspace", "acme", "--path", path], {})).toBe(0); + expect(loadConfigFile(path).UPLOADS_WORKSPACE).toBe("acme"); + }); + + it("leaves the token's own workspace free to win after a bare init", async () => { + const path = freshPath(); + silence(); + await runConfig(["init", "--path", path], {}); + // No config-file workspace means `resolveConfig` falls through to the + // workspace encoded in the token (`up__…`) rather than being + // overridden by a seeded value. + expect(loadConfigFile(path).UPLOADS_WORKSPACE).toBeUndefined(); + expect(existsSync(path)).toBe(true); + }); +}); diff --git a/packages/workspace/README.md b/packages/workspace/README.md deleted file mode 100644 index b9d8456f..00000000 --- a/packages/workspace/README.md +++ /dev/null @@ -1,14 +0,0 @@ -# @uploads/workspace - -Workspace identity facts that more than one worker has to agree on — today, -which slug is the communal workspace (`COMMUNAL_WORKSPACE`, -`isCommunalWorkspace`). Enforcement lives with the rules it belongs to -(slug reservation in `apps/api/src/slug-policy.ts`, the member-cap exemption in -`apps/api/src/routes/internal-billing.ts`); this package only says which -workspace they are all talking about. - -Clients are told, not left to infer: `/me/workspaces` stamps `communal` on each -membership entry, so `apps/web` reads a flag instead of matching a slug. - -Private workspace package — not published, excluded from Changesets like -`@uploads/api` / `@uploads/billing` / `@uploads/web` / `@uploads/auth`. diff --git a/packages/workspace/package.json b/packages/workspace/package.json deleted file mode 100644 index aa4a7ef4..00000000 --- a/packages/workspace/package.json +++ /dev/null @@ -1,17 +0,0 @@ -{ - "name": "@uploads/workspace", - "version": "0.1.0", - "private": true, - "type": "module", - "exports": { - ".": "./src/index.ts" - }, - "scripts": { - "typecheck": "tsc --noEmit", - "test": "vitest run" - }, - "devDependencies": { - "typescript": "^6.0.3", - "vitest": "^4.1.10" - } -} diff --git a/packages/workspace/src/communal.ts b/packages/workspace/src/communal.ts deleted file mode 100644 index 9bbaf49c..00000000 --- a/packages/workspace/src/communal.ts +++ /dev/null @@ -1,27 +0,0 @@ -/** - * Identity of the communal workspace — the one shared tenant every account - * belongs to, as opposed to the workspaces a user creates or is invited to. - * - * It is privileged in several unrelated places: its slug is reserved against - * self-serve registration (`slug-policy.ts`), it is exempt from the free-plan - * member cap (`routes/internal-billing.ts`) and from the workspace reaper, and - * the account UI skips it when choosing which workspace to open. Those rules - * are enforced in different workers, so the slug itself lives here — one home - * that both `apps/api` and `apps/web` can import — rather than being spelled - * out at each site where a rename would have to be applied in lockstep with no - * compiler help. - * - * Note the distinction this package does *not* erase: server code may compare - * against the slug, but clients should be *told* whether a workspace is - * communal (`/me/workspaces` stamps `communal` on each entry) instead of - * re-deriving a business rule from a naming convention. The one client-side - * comparison left is `apps/web`'s `mapMyWorkspace`, which falls back to the - * slug only when the field is absent — i.e. against an api build older than - * the flag, since web and api deploy independently. - */ -export const COMMUNAL_WORKSPACE = "default"; - -/** Whether `name` is the communal workspace. */ -export function isCommunalWorkspace(name: string): boolean { - return name === COMMUNAL_WORKSPACE; -} diff --git a/packages/workspace/src/index.ts b/packages/workspace/src/index.ts deleted file mode 100644 index 10a5e1ef..00000000 --- a/packages/workspace/src/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from "./communal"; diff --git a/packages/workspace/tsconfig.json b/packages/workspace/tsconfig.json deleted file mode 100644 index f320e242..00000000 --- a/packages/workspace/tsconfig.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "compilerOptions": { - "target": "ES2022", - "lib": ["ES2022"], - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true - }, - "include": ["src"] -} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 60d14035..d3ef00db 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -56,9 +56,6 @@ importers: '@uploads/storage': specifier: workspace:^ version: link:../../packages/storage - '@uploads/workspace': - specifier: workspace:^ - version: link:../../packages/workspace hono: specifier: ^4.12.28 version: 4.12.28 @@ -170,9 +167,6 @@ importers: '@uploads/ui': specifier: workspace:* version: link:../../packages/ui - '@uploads/workspace': - specifier: workspace:^ - version: link:../../packages/workspace astro: specifier: ^7.0.6 version: 7.0.6(@emnapi/core@1.11.1)(@emnapi/runtime@1.11.2)(@types/node@26.1.0)(rollup@4.62.2)(terser@5.49.0)(yaml@2.9.0) @@ -221,10 +215,6 @@ importers: version: 4.1.10(@opentelemetry/api@1.9.1)(@types/node@26.1.0)(vite@8.1.3(@types/node@26.1.0)(esbuild@0.28.1)(terser@5.49.0)(yaml@2.9.0)) packages/email: - dependencies: - '@uploads/workspace': - specifier: workspace:^ - version: link:../workspace devDependencies: typescript: specifier: ^6.0.3 @@ -333,15 +323,6 @@ importers: specifier: ^1.61.1 version: 1.61.1 - packages/workspace: - devDependencies: - typescript: - specifier: ^6.0.3 - version: 6.0.3 - vitest: - specifier: ^4.1.10 - version: 4.1.10(@opentelemetry/api@1.9.1)(@types/node@26.1.0)(vite@8.1.3(@types/node@26.1.0)(esbuild@0.28.1)(terser@5.49.0)(yaml@2.9.0)) - scripts/demos: dependencies: '@remotion/cli': diff --git a/skills/uploads-cli/SKILL.md b/skills/uploads-cli/SKILL.md index b869770f..1d660733 100644 --- a/skills/uploads-cli/SKILL.md +++ b/skills/uploads-cli/SKILL.md @@ -197,6 +197,12 @@ uploads login # opens a browser to approve sign-in, saves config, runs uploads login --workspace acme # only needed if your account can access more than one ``` +If the account has no workspace yet, `login` prompts for a name and offers one +derived from your GitHub login as a bracketed default — press Enter to take it, +or type your own. Nothing is prefilled when no valid, unclaimed name can be +derived. `--workspace --create` skips the prompt entirely, which is the +form to use in scripts. + That's a one-time, human-in-the-loop step (device sign-in needs a browser); once the config file is written, every later `uploads` invocation — including from a non-interactive agent — just reads the saved token. Routine agents never need @@ -792,9 +798,14 @@ uploads config show # effective settings (token red uploads config path # resolved config file path uploads config set UPLOADS_DEFAULT_REPO myorg/myapp uploads config set UPLOADS_DEFAULT_WIDTH 700 -uploads config init --api-url http://localhost:8787 --workspace default --token up_default_… +uploads config init --api-url http://localhost:8787 --workspace acme --token up_acme_… ``` +`init` writes only the keys you pass. With no flags it seeds `UPLOADS_API_URL` +alone and deliberately sets no workspace: a `UPLOADS_WORKSPACE` in the config +file outranks the workspace encoded in your token, so seeding one would pin +every later `uploads login` to it. Pass `--workspace` when you want it fixed. + Recognized keys: `UPLOADS_API_URL`, `UPLOADS_WORKSPACE`, `UPLOADS_TOKEN`, `UPLOADS_DEFAULT_PREFIX`, `UPLOADS_DEFAULT_REPO`, `UPLOADS_DEFAULT_REF`, `UPLOADS_DEFAULT_WIDTH`, `UPLOADS_NO_GIT`, `UPLOADS_NO_OPTIMIZE`, `UPLOADS_KEEP_EXIF`,