From 5fb145c79ff74e7447a699c6c70c876ee69fad43 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Wed, 19 Aug 2026 20:03:14 +0200 Subject: [PATCH] feat: guided setup wizard for data tables on Cloud (#10584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(frontend): guided setup wizard for data tables On Cloud a data table cannot use the Windmill instance database, so a new workspace hit a dead end: an alert telling the user to go find a PostgreSQL resource somewhere else. Setting one up meant three disconnected places, and the connection could only be tested after the config had already been saved. Adds a three-step wizard (choose a database -> set it up -> name it) reached from the data tables settings page: - Supabase: signs in via the existing supabase_wizard OAuth client and creates the project from inside Windmill. Because db_pass is an input to project creation, Windmill sets the password and the user never visits a dashboard. - Your own database: picks an existing postgresql resource, or adds one with a connection string through the form that already supports it. - Windmill database: hands back to the inline row editor, since instance databases are provisioned by a superadmin. Verifying access is no longer a step the user takes: Continue runs the check and passing it is what advances the wizard, so a database that cannot create tables never reaches the workspace config. Co-Authored-By: Claude Opus 5 (1M context) * chore: pin ee-repo-ref to the Supabase provisioning endpoints Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): do not claim the database is ready when its check failed Co-Authored-By: Claude Opus 5 (1M context) * fix: address review findings on the data table wizard - The Supabase create branch advanced on `provisioning === 4` without consulting the check it had just run, so a role that cannot create tables could reach Finish. It now blocks and offers Try again. - Retrying no longer mints a fresh secret variable + resource each time: the credentials are only re-created when the password actually changed. - The generated password is captured before the create call rather than after, since a throw there can still leave a project behind. - On a failed provision the project list is refreshed, so the just-created project can be picked up from the other tab instead of provisioning a second. - Finish refuses a name that already belongs to another data table, which previously repointed it at the new database. - Secrets go to the acting user's namespace instead of a literal `u/admin/`. - The progress list no longer ticks "Created on Supabase" before the request is sent, and does not claim the database is ready when its check failed. - The wizard's resume state is cleared when it closes, so reopening after an abandoned OAuth round trip is not stuck on step 2. - The OAuth callback shares the session-storage key rather than repeating it. - SupabaseConnect uses the shared provisioning helpers instead of a fork. - Restores the doc comment displaced onto TestDataTableResourceQuery. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): simplify Alert layout and balance its vertical padding The body was rendered by two near-duplicate branches, each wrapping the text in an extra div only to hang a margin on it, and the margins disagreed: the collapsible branch spaced above with mt-2, the static one below with mb-2. Since isCollapsed defaults to true, every non-collapsible alert took the static branch, so titled alerts read as 24px of space below the text against 16px above -- visibly off-centre -- with the title and body flush against each other. Collapse both branches into one and drop the margins; the container's own padding now sets top and bottom equally, with a small gap under the title row. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): only offer Supabase when its OAuth client is configured The wizard offered the Supabase card unconditionally, so on an instance whose superadmin never configured a supabase_wizard client -- or whose backend is built without the oauth2 feature, which compiles the whole /api/oauth router out -- the card dead-ended at a 404. Gate it on listOauthConnects, the same check ApiConnectForm already makes, fetched on open so configuring the client mid-session does not require a reload. Also drop the Supabase project ref from the existing-project cards: it is an opaque identifier that means nothing outside Supabase's own dashboard URLs. Show the region instead, plus a status word when the project is not healthy, since a paused project is the one case where the connection check fails for a reason unrelated to the password. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): run the Supabase OAuth leg in a popup A full-page redirect unmounts the wizard, so anything the user does on Supabase's side -- signing in, confirming an email, browsing their dashboard -- leaves them with nothing pointing back at Windmill, and the wizard had to park its state in sessionStorage to survive the trip. Open the connect endpoint in a popup instead. The modal stays on screen throughout and the callback hands the token back through postMessage rather than navigating. The parked-state path stays as the fallback for browsers that block the popup. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): scope the connection check to the choice that produced it A failed check stayed on screen when the user switched Supabase mode or picked a different provider, so a fresh tab opened showing an error about a database it had nothing to do with. Clear the report and the error on both switches; re-clicking the tab already selected leaves an error the user is reading in place. Also polish the Supabase step: project cards get the provider-card treatment (icon, p-3, flex column) instead of a hand-rolled variant whose block layout left more padding above the name than below; form labels settle on text-emphasis; and the signup link sits under the primary button for anyone who does not have an account yet. Drop the "free" badge and the "Free on Supabase" line -- every option in the wizard is free, so neither told the user anything -- and say what the Supabase card actually does now that connecting an existing project is the default. Co-Authored-By: Claude Opus 5 (1M context) * refactor(frontend): one setup checklist and one Supabase step for every host The data table wizard, the instance database modal and the resource drawer each had their own version of the same two interactions, and they had already begun to drift: the wizard's Supabase resource shape was rebuilt by hand in the drawer, and the instance checks rendered with no notion of a step being in flight. SetupChecklist replaces LoggedWizardResult, whose only consumer was the instance modal. It adds the running state that component lacked, so a list driven by an endpoint that reports nothing until it returns still shows where it is. Both the instance checks and the Supabase provisioning stages render through it. SupabaseProjectStep owns picking or creating a project, and useSupabaseOauth owns the popup leg. Each host keeps only what is genuinely its own: the wizard saves a variable and resource then verifies the connection, the resource drawer fills in its own form. Both trigger authorization themselves, so a host can offer it a screen earlier than the step does. The lists load behind a spinner because which mode to open on depends on whether the account has projects; deciding that after rendering flipped the toggle under the user. Adds a kitchen_sink playground for the checklist so the animation and every failure position can be exercised without a backend, a superadmin, or a Supabase account. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): tidy the resource drawer around the Supabase entry point Connect Supabase was a hand-styled anchor carrying Supabase's brand hex values rather than a Button, and it sat in a row whose other controls had settled on unifiedSize md. Making it a Button meant SupabaseIcon had to satisfy IconType, so it now takes `size` (deriving height/width from it) alongside the string props its other callers pass. The manual resource form spaced every field 32px apart and WhitelistIp added another 16px of its own, which read as a gap rather than a rhythm. One gap of 16px, with the form itself given a little more separation from the description above it. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): stop Supabase resources coming up modified when first opened Resource forms fill in every unset property from the schema as soon as they render, so a postgresql resource saved without region, root_certificate_pem and use_iam_auth was dirty -- and had saved a draft -- the first time anyone looked at it. Write them with the rest of the value. SupabaseConnect also rebuilt the resource shape by hand instead of using the shared helper, which is how the pooler host format ended up in two places. Co-Authored-By: Claude Opus 5 (1M context) * feat(backend): record where a data table came from and whether setup finished edit_datatable_config replaces the whole datatables map and DataTable does not deny unknown fields, so anything the request omits is dropped without a word. origin and setup_incomplete would have been erased by any unrelated save; preserve_unmanaged_datatable_fields carries them -- and migrations_enabled, which had the same problem inline -- forward for entries that already exist, following renames. setup_incomplete is what lets a row be recorded before the resource it points at exists, so the wizard can write nothing until the user finishes. There is deliberately no intermediate state: the setup runs entirely in the browser, so nothing server-side could advance one. datatable_health probes every data table at once for the settings page and skips the incomplete ones, whose resource_path resolves to nothing yet. set_datatable_setup patches a single entry instead of resending the map. test_datatable_connection_value checks a connection the caller has not saved anywhere, which the wizard needs before it has written a resource. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): make destructive default and subtle buttons read red Both variants were neutral until the pointer arrived, then filled solid red: nothing marked the button as destructive until you were already on it. They now carry red text at rest, with a faded red border on default and a light red wash on hover, which is what the legacy red border style in the same file had always done. Three call sites passed color="red" alongside a design-system variant. getStyleClass returns before colour is read for accent, accent-secondary, default and subtle, so the delete-migration control, its modal confirm and the import-database button had all been rendering neutral. They pass destructive now. The dropdown variant strips the button's own border, and matched border-border-light literally -- a class the destructive style no longer contains. Co-Authored-By: Claude Opus 5 (1M context) * feat(frontend): rebuild data table setup around a read-only row The wizard gathers intent over two steps, reviews it on a third and writes nothing until Finish, so a billable Supabase project is created only once the user has seen what will happen. runSetup is also the retry: every step probes for its own result before doing anything, so running it again on a half-finished data table resumes instead of duplicating. Its steps are keyed rather than dispatched on their titles, where rewording one changed what it did. The settings row stops being an editable form with a dirty/save cycle. It carries the name, where the database came from, a health dot and two actions; everything rare moved into the gear panel, which also offers Finish setup for a data table whose wizard never completed. Manage is ExploreAssetButton, the control the ducklake list already uses, and the row and panel both link out to the underlying resource. supabaseResourceValue no longer assembles the pooler host from the region. aws-0-.pooler.supabase.com is wrong for any project Supabase allocated elsewhere, so the host, user and port come from the pooler config endpoint. Two data tables sharing one database also share _wm_migrations, which is probed unqualified, so the review step warns when the database being connected is already behind another data table. SupabaseConnect is deleted. The resource drawer uses the shared project step restricted to existing projects: creating one is a billed action and belongs in the wizard, which has somewhere to report what it did. The kitchen_sink checklist playground goes with it. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): fall back to a direct Supabase connection when the pooler cannot be read Reading a project's Supavisor config needs the database_pooling_config_read scope, which an instance's Supabase OAuth app may never have been granted. No retry recovers from that, and the wizard treated it as fatal: the user was left with an error and no way to finish connecting a project that was otherwise fine. resolveSupabaseConnection replaces the bare pooler read everywhere it happened. Asking for session pooling and failing now yields a direct connection plus the reason, which supabaseResourceValue already knew how to write. Nothing about the fallback is silent -- direct is IPv6-only, which is the whole reason session pooling is the default -- so the wizard warns on its review step and the resource drawer says so in its toast. The row is recorded before credentials are saved, so an origin claiming session pooling has to be corrected once a direct host is what gets written; the run patches it through set_datatable_setup rather than leaving the panel to report a mode nothing uses. Co-Authored-By: Claude Opus 5 (1M context) * feat(frontend): open the database behind a data table, and say when it cannot write Every database in the list now opens the surface that owns its credentials. A postgres one opens its resource in the editor drawer; a Windmill instance one opens the instance modal, which is where its setup checks, password rotation and drop already lived. Both are reachable from the row and from the panel's provenance list, and the provider icon moved inside the button so the whole thing is one target. CustomInstanceDbWizardModal targeted #content unconditionally, which put it underneath the panel drawer that now opens it. It takes a target, and the panel portals it to the body. The status column gains a third state. The probe reports privileges but nothing gated the dot on them, so a data table whose role cannot create tables showed as Connected and only failed when someone ran a migration. It reads "Limited permissions" instead, and opens the panel on the report carrying the GRANTs that fix it -- the settings page has already probed, so the panel takes that report rather than asking the user to run Test connection over work already done. fullyPrivileged is exported from the report component so the dot and the report cannot disagree about what counts as healthy. Co-Authored-By: Claude Opus 5 (1M context) * revert(frontend): keep the data tables settings table as it was The settings table and the setup wizard are two changes that only shared a file. Splitting them makes each reviewable: this branch keeps the wizard, and the read-only row, gear panel, health probe and clickable databases move to their own branch. The rows go back to the editable form with its pickers and save footer, still opening the wizard from Add a database. DataTableSettingsPanel, dataTableHealth and dataTableOrigin had no other consumers and go with them; the connection report stays, because the wizard shows it too. DataTableSettingsType keeps `origin`: the wizard writes it, and the review step reads it back to warn when two data tables would share one database and therefore one _wm_migrations table. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): confirm before dismissing the data table wizard mid-setup Closing was guarded while a run was in flight and unguarded before one, which is backwards: a run leaves a row to resume from, whereas a backdrop click on the review step threw away the project, the pasted password and the folder with nothing to recover them from. Backdrop, Escape and the close button now go through one path that asks first. It only asks when there is something to lose -- no provider chosen yet, or a run that already produced a result, closes immediately -- so the dialog does not become something to click through. Continue in the background still leaves in one click; that exit was always the deliberate one. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): stop the wizard claiming the resource folder controls who can use a data table "Who can use this database" was wrong. Every path that resolves a datatable:// reference -- both executors and the agent-worker endpoint -- reads the resource unchecked, by workspace and name. A resource in u/admin is usable by everyone's scripts. The folder governs who can see and edit the connection, and who can reference the resource directly in a SQL step; neither is who can use the data table. The wizard was contradicting the tab's own description two screens later. The folder select and name field become one Path picker, the same one the resource, variable and script forms use, so the review step reads as a resource path rather than a permission choice. Its initialPath is snapshotted when the step opens: Path seeds itself from it, and a live value fights the typing. Finish now also gates on Path's error, so a taken or malformed path stops the run before it writes anything. The button that opens all this says "Add a data table" -- the data table is what you get; the database is a detail chosen along the way. Co-Authored-By: Claude Opus 5 (1M context) * revert(frontend): move the destructive button restyle out of the wizard PR This reverts 3881e4d8ea. Making default and subtle destructive buttons red at rest changes every existing caller of the prop -- the workspace integrations, AI skills, workspace creation and the instance database drop -- so it is a design-system change, and the call sites it fixed are the migrations list and the database manager. None of that is the setup wizard. Nothing on this branch passes destructive any more, so it leaves with no loose ends. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): make the wizard stepper navigate the steps it already offers Stepper dispatches a click and paints cursor-pointer on every reached step, but the wizard never listened, so the breadcrumbs invited a click and did nothing. They now reach any step already passed, in either direction: going back to check something should not cost the progress, which means tracking the furthest step reached rather than the current one. Forward movement still only happens through the primary action, so a step is never reachable without having been validated -- and changing the intent revokes the steps ahead of it, or Finish could run against a review built from something the user has since edited. The five places that cleared the probe on an edit now do both through one call. During a run nothing is reachable, and the stepper says so rather than showing a pointer over steps that will not respond. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): restore the data tables description lost in the branch split The rewritten description went into DataTableSettings.svelte shortly before that file was restored wholesale to its pre-rebuild state, so it left with the row rework it had nothing to do with. The tab went back to describing the plumbing -- a fully managed PostgreSQL database, reachable from the SDK -- which never answered the question a new user actually has: why this rather than a Postgres resource. It leads with what a data table is, then the two things a resource cannot do -- nobody needs the credentials to query it, and the name can be pointed at another database without editing anything that uses it -- and closes with what Windmill runs on top. Both middle claims are the ones every resolution path backs up: datatable:// resolves by workspace and name, unchecked. Co-Authored-By: Claude Opus 5 (1M context) * fix(backend): say what is missing when a $res: or $var: reference does not resolve Both interpolations fetched with fetch_one and mapped the error through to_anyhow, so a reference to something deleted surfaced as "no rows returned by a query that expected to return at least one row @workspaces.rs:2169". It names neither the kind of thing that was missing nor its path, and it is what a data table pointing at a deleted resource reports. They now fetch_optional and return NotFound naming the path, and datatable resolution adds the data table on the way out: the caller asked for one by name, and a bare "resource f/x/y does not exist" leaves them to work out which of them points at it. The health probe is new, so this string had only just become something users read. Co-Authored-By: Claude Opus 5 (1M context) * feat(frontend): gate the data table wizard behind a dev flag The wizard only appears with `dataTableWizard` set in localStorage; without it the settings page keeps the inline-row flow it had before this branch, down to the empty-state copy and the "New Data Table" button, and the wizard component is not mounted at all. The existing e2e suite drives that button, so the default-off flag is also what keeps it green. Step 2 of "your own database" becomes one list rather than a segmented control: the workspace's Postgres resources, then a New resource card that expands in place. A connection string is not an alternative to a resource, it is how one is written, and the old layout taught otherwise. The card holds the same connection as a string or as fields and carries values across when you switch, so `parse` and `compose` have to be inverses -- hence the percent-encoding on both sides, which also fixes a password containing `@` silently corrupting in the resource form. The Supabase step now uses the same shape. Names and paths are checked as they are typed rather than at the end of a run that may have created a billed project first: the data table name against the charset `edit_datatable_config` enforces, the instance database name against what `setup_custom_instance_db` will accept, and the resource path against both the resource and variable namespaces, since the run writes to both and both writes upsert. `test_datatable_connection_value` refuses `$var:`/`$res:` in its body. It feeds `transform_json_value_unchecked`, which resolves references with no permission check of its own, so an admin could otherwise have had the API server decrypt any workspace secret and hand it to a host the same request chose -- without the audit trail a variable read leaves. Callers testing something unsaved hold the literal value already. Alert, SetupChecklist and postgresConnectionString change for everyone, not just behind the flag: body-only alerts no longer reserve an empty title row, the checklist can nest the checks a step is made of, and the connection-string parser is shared with the resource form. Co-Authored-By: Claude Opus 5 (1M context) * chore: pin ee-repo-ref to the EE branch merged with EE main The Supabase proxies the wizard calls are still unmerged, so the ref cannot be an EE main commit yet; it now names that branch merged with EE main rather than the branch alone, which was nine commits behind and would have been built against a CE main it never saw. Co-Authored-By: Claude Opus 5 (1M context) * feat(frontend): gate the supabase resource path behind the dev flag * test(frontend): pin connection string parsing to libpq behaviour * fix(frontend): keep the supabase resource link off the popup callback path * refactor(frontend): load the supabase resource dialog only behind the flag * fix(frontend): refuse a resource path the wizard run does not own * fix(frontend): let a failed data table setup be corrected without losing what it made * fix(frontend): let a failed setup reuse the resource path it claimed Co-Authored-By: Claude Opus 5 (1M context) * fix(backend): record the two data table connection tests in the audit log Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): use Section for the data table wizard advanced group Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): read connection strings the way libpq does Co-Authored-By: Claude Opus 5 (1M context) * fix(backend): pin the ee ref back to a commit this branch can build Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): keep a failed setup's claims across the redirect and rollback Co-Authored-By: Claude Opus 5 (1M context) * fix(backend): probe a data table with the auth mode the worker will use Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): keep every part of a connection string through the round trip Co-Authored-By: Claude Opus 5 (1M context) * refactor(frontend): give a setup run one record of what it created Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): mark a resource claim by edited_at, not its creator Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): mark every claim by revision, and keep an unconfirmed project's secret Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): refuse to test or save behind a connection string that will not parse Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): refuse a connection string carrying options the resource cannot hold Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): allowlist the connection-string parameters a resource can honour Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): guard every created Supabase project, not just the last one Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): do not warn about renaming an item that does not exist yet Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): make the review step read as one list of what will exist Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): keep the picked Supabase project across the redirect, reject connect_timeout Co-Authored-By: Claude Opus 5 (1M context) * fix: check the data table connection from a worker, not the API server The wizard's connection check ran on the API server through two endpoints added for it. That server is a different machine with a different identity, so the answer was about the API server rather than about the worker that will run the queries: a host reachable from one is not necessarily reachable from the other, and IAM RDS and Azure workload identity authenticate as whichever process opens the connection. Run the privilege query as a preview job instead. A job goes through the worker's Postgres executor, which is where `PgAuthMode::of` already picks the authentication mode, and it takes either a resource value or a `$res:` path exactly as a Postgres step does. Postgres composes the suggested GRANT statements through `format('%I')`, so identifier quoting stays where it is already implemented. Removes `test_datatable_resource_connection` and `test_datatable_connection_value`, and `connect_as_the_worker_would` with them. Co-Authored-By: Claude Opus 5 (1M context) * refactor: fold check_datatable_connection back into its only caller The helper was split out so the two connection-test endpoints could share a body. Those endpoints are gone, leaving one caller. Co-Authored-By: Claude Opus 5 (1M context) * revert: keep the data table connection check schema inline It was lifted into components so three endpoints could share it. Two of those are gone, so it is back to one user and the extraction changes nothing. Co-Authored-By: Claude Opus 5 (1M context) * fix: restore openapi.yaml to the branch point The previous commit restored main's tip rather than the merge base, which carried three unrelated main-only changes into this branch: the resource mcp_tools truncation fields, the execution_mode description, and a version bump. Co-Authored-By: Claude Opus 5 (1M context) * refactor(frontend): drop four effects from the data table wizard Each was doing work a derived, a load callback or a real entry point does better. - The name conflict is kept with the name it was raised for and derived from it. As an effect it was correct only because it never read what it wrote: the pre-flight sets the message and the effect does not re-trigger, so adding a read would have cleared it the instant it appeared. The message now also comes back if the taken name is retyped, which is what the server will say. - The default resource selection is seeded inside the fetcher that loads the list, where "has the fetch settled" cannot be asked wrong. - Reset-on-open becomes an exported open(), called by the settings page, so a fresh run is set up by the act of opening rather than by a flag emulating mount. - The OAuth connects and the folder list become resources; supabaseAvailable and folders are derived from them. defaultFolder takes the list rather than reading it, so the fetch can seed off its own result. Leaves the debounced path check, which is async with an out-of-order guard. Co-Authored-By: Claude Opus 5 (1M context) * refactor(frontend): drop three effects from the Supabase branch - useSupabaseOauth reports success as onAuthed, alongside the failures it already reported. SupabaseResourceConnect was watching `authed` to find out; it takes the callback instead, keeping the guard that stops an authorization started elsewhere on the page from opening its dialog. - SupabaseProjectStep loads its orgs and projects through a resource keyed on the token, so the `loaded` latch goes and re-authorizing reloads rather than keeping the lists from the expired session. - SetupChecklist records what the user toggled and derives the open state from it, a failed step defaulting to open. Recording the open state instead needed an effect to force it, and that effect re-ran on every progress update, so a description closed while anything was still ticking reopened. A close now holds for the life of the checklist, including across Try again. Leaves the message listener, which subscribes to another window. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): confine the modal restyle to the wizard, and trim the comments The wider side padding and lighter dialog heading were changing all 17 Modal2 dialogs to suit this one flow. They move behind an opt-in `formStyling`, taken by the three dialogs this branch owns; every other Modal2 renders as it did. Also drops two comments that cited a design approval rather than a constraint, and shortens the blocks that had grown past the four lines AGENTS.md asks for. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): use the accent token for the wizard's links `text-blue-500` is the marketing blue `#3B82F6`, which brand-guidelines.md rules out in the app interface. Co-Authored-By: Claude Opus 5 (1M context) * chore: point ee-repo-ref at the EE branch head Picks up EE main, which the branch now needs, and the Supabase proxy auth fix. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): read sslmode by name, and stop decrypting a secret to date it - `sslmode` was found by searching the query text, so it also matched inside another parameter's value: `?application_name=sslmode=disable` passed the allowlist on the parameter name and then parsed as a request to turn TLS off, which both the wizard and the resource form saved and probed. Parsed with `URLSearchParams` by exact name, with a test. - `secretMark` read the variable with `decryptSecret` defaulted to true, so every write decrypted a secret nothing reads and recorded the decryption -- including someone else's on the retry about to refuse it. It wants only `edited_at`, which is returned either way. - The probe gave up at 15s while the worker allows its Postgres connect 20s, so a host that accepts the connection and never answers was cancelled and reported as a missing worker rather than a failed connection. - The create-mode region and project name did not report an intent change, so renaming a project after a name collision left the failure naming the old one. - Two comments described the code as it was before the claim mark became a revision, and a doc comment outlived the field it documented. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): read connection parameters the way libpq does One reader for both the parser and the allowlist, since they disagreed about what a string says in two ways that both ended in a weaker connection than was pasted: - `URLSearchParams.get` takes the first of a repeated parameter and libpq takes the last, so `?sslmode=disable&sslmode=require` was read as `disable`. - The allowlist folded the parameter name and the parser did not, so `?SslMode=verify-full` was refused by neither and honoured by neither, and saved as the `require` default. The parked Supabase run is now handed to `open()` rather than read back off the `resume` prop it was just assigned to, so restoring it does not depend on when that prop reaches the component. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): keep connection parameter names case-sensitive libpq does not fold them: `?SslMode=disable` is rejected as an invalid URI query parameter rather than read as `sslmode`, which a local server confirms. Folding made Windmill accept and honour a string Postgres itself refuses; naming the parameter instead tells the user why it cannot be stored. The last-value-wins rule for a repeated parameter is unchanged, and matches what the same server does. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): seed the Supabase organization from the project it selects The loader took `orgs[0]` independently of the project it seeded, so an account whose first project sits outside its first organization had the review step name an organization the database does not belong to. Picking a project by hand already derives it; the seeding now does the same. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): let the probe report an empty search_path instead of failing on it `format('%I', NULL)` raises rather than returning NULL, so a role whose search_path names no valid schema failed the whole privilege query and was reported as an unreachable database. That is the one case `fix_search_path` exists to name, and it never reached the user. Verified against a local server with `SET search_path = ''`. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): say which of the two refusals a connection string hit Making parameter names case-sensitive gave `unsupportedConnectionParam` two reasons to refuse, and the single message explained only one. `?SslMode=` was answered with "Windmill cannot store SslMode on a Postgres resource", which is false twice over: sslmode is exactly what the resource stores, and the string asks for nothing because Postgres rejects the URI. It now names the spelling when the parameter is one we keep, and the storage limit otherwise. The folder-list guard also still read the `resume` prop that `open(parked)` was changed to stop trusting, so the resumed path now comes from whatever `reset` was handed. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): leave the Supabase organization unset when the lookup misses Falling back to the first organization named one the seeded project is not in, since `supabaseSummary` prefers `intent.org` over the project's own. Unset, it falls through to the project's organization identifier — the right one, spelled as a slug rather than a name. Co-Authored-By: Claude Opus 5 (1M context) * test(frontend): pin which refusal a connection string gets The two messages differ in what they ask the user to do, and the condition choosing between them — whether the lowercased name is one the resource keeps — is not visible from either call site. `Connect_Timeout` is the case that keeps them honest: miscased *and* unstorable, so respelling it would not help and the message must not suggest it. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): hand a failed Supabase leg back to the page holding its run Denial, a token error and a malformed callback all sent the user to /resources whether or not a run was parked. Nothing else consumes the park, so the run stayed in sessionStorage and sprang the wizard open on an unrelated later visit instead. A parked run now lands on the data tables tab, where the wizard resumes on the setup step and can authorize again. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): let a run reuse the name of a row it could not take back out `removeRow` reports `kept` when the undo cannot reach the server, so the row this run wrote stays in the workspace config and comes back in `existingNames`. The client-side name check then refused the retry on the run's own name, with no way forward but a rename. The instance database name has carried the same exemption since it was written; this is the data table name catching up. Co-Authored-By: Claude Opus 5 (1M context) * fix(frontend): discard a variable check the wizard has moved on from The post-await guard compared only the path, and the path is built from the review step's fields -- so picking an existing resource stops the wizard minting one without changing it. A check already in flight then answered for a branch nobody was on, and a `true` disabled Finish over a path the run no longer writes. The cleanup cannot help: it cancels a pending timer, not a live request. Both sides of the await now ask the same question. Co-Authored-By: Claude Opus 5 (1M context) * chore: update ee-repo-ref to 483513b70979aa9497cab869837108d948449984 This commit updates the EE repository reference after PR #715 was merged in windmill-ee-private. Previous ee-repo-ref: 8604b30a740c5620069208801a7ae50937b61977 New ee-repo-ref: 483513b70979aa9497cab869837108d948449984 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- backend/windmill-common/src/workspaces.rs | 27 +- .../src/lib/components/ApiConnectForm.svelte | 87 +- .../src/lib/components/AppConnectInner.svelte | 30 +- frontend/src/lib/components/Path.svelte | 12 +- .../src/lib/components/WhitelistIp.svelte | 1 - .../lib/components/common/alert/Alert.svelte | 73 +- .../lib/components/common/modal/Modal2.svelte | 13 +- .../components/common/stepper/Stepper.svelte | 38 +- .../lib/components/copilot/ResourceGen.svelte | 1 + .../lib/components/icons/SupabaseIcon.svelte | 19 +- .../wizards/LoggedWizardResult.svelte | 108 -- .../components/wizards/SetupChecklist.svelte | 117 ++ .../AddDataTableWizard.svelte | 1435 +++++++++++++++++ .../CustomInstanceDbWizardModal.svelte | 70 +- .../DataTableConnectionReport.svelte | 77 + .../DataTableSettings.svelte | 134 +- .../SupabaseConnectionMode.svelte | 62 + .../SupabaseProjectStep.svelte | 290 ++++ .../SupabaseResourceConnect.svelte | 120 ++ .../addDataTableModel.test.ts | 434 +++++ .../workspaceSettings/addDataTableModel.ts | 853 ++++++++++ .../workspaceSettings/datatableProbe.ts | 108 ++ .../workspaceSettings/instanceDbSteps.ts | 89 + .../workspaceSettings/setupClaims.test.ts | 72 + .../workspaceSettings/setupClaims.ts | 87 + .../workspaceSettings/supabaseOauth.svelte.ts | 107 ++ .../workspaceSettings/supabaseProvisioning.ts | 250 +++ .../workspaceSettings/utils.svelte.ts | 12 + .../workspaceSettings/wizardParking.ts | 63 + .../utils/postgresConnectionString.test.ts | 186 +++ .../src/lib/utils/postgresConnectionString.ts | 144 ++ .../oauth/callback_supabase/+page.svelte | 47 +- 33 files changed, 4869 insertions(+), 299 deletions(-) delete mode 100644 frontend/src/lib/components/wizards/LoggedWizardResult.svelte create mode 100644 frontend/src/lib/components/wizards/SetupChecklist.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/DataTableConnectionReport.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts create mode 100644 frontend/src/lib/components/workspaceSettings/addDataTableModel.ts create mode 100644 frontend/src/lib/components/workspaceSettings/datatableProbe.ts create mode 100644 frontend/src/lib/components/workspaceSettings/instanceDbSteps.ts create mode 100644 frontend/src/lib/components/workspaceSettings/setupClaims.test.ts create mode 100644 frontend/src/lib/components/workspaceSettings/setupClaims.ts create mode 100644 frontend/src/lib/components/workspaceSettings/supabaseOauth.svelte.ts create mode 100644 frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts create mode 100644 frontend/src/lib/components/workspaceSettings/wizardParking.ts create mode 100644 frontend/src/lib/utils/postgresConnectionString.test.ts create mode 100644 frontend/src/lib/utils/postgresConnectionString.ts diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index ec7c2ac03e..1b9eb3fe0e 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -bd4de74eb37b32a2b6c7c69f6dedac031ef8436b +483513b70979aa9497cab869837108d948449984 diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 7ba930c3ea..c45d0c79f9 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1123,12 +1123,18 @@ async fn get_datatable_resource_inner( serde_json::to_value(&pg_creds) .map_err(|e| Error::internal_err(format!("Error serializing pg creds: {}", e)))? } else { + // Name the data table too: the caller asked for one by name, and a bare + // "resource f/x/y does not exist" leaves them to work out which one points at it. transform_json_unchecked( &serde_json::Value::String(format!("$res:{}", datatable.database.resource_path)), w_id, db, ) - .await? + .await + .map_err(|e| match e { + Error::NotFound(m) => Error::NotFound(format!("data table {name}: {m}")), + e => e, + })? }; Ok(db_resource) @@ -2105,25 +2111,32 @@ async fn transform_json_unchecked( serde_json::Value::Array(transformed_array) } serde_json::Value::String(s) if s.starts_with("$res:") => { + // A reference to something that was deleted is the common failure here, and + // `fetch_one` reports it as "no rows returned by a query that expected to + // return at least one row" -- which names neither what was missing nor where. + let path = &s[5..]; let resource = sqlx::query_scalar!( "SELECT value AS \"value!: _\" FROM resource WHERE workspace_id = $1 AND path = $2", &w_id, - &s[5..] + path ) - .fetch_one(db) + .fetch_optional(db) .await - .map_err(to_anyhow)?; + .map_err(to_anyhow)? + .ok_or_else(|| Error::NotFound(format!("resource {path} does not exist")))?; transform_json_unchecked(&resource, w_id, db).await? } serde_json::Value::String(s) if s.starts_with("$var:") => { + let path = &s[5..]; let (value, is_secret): (String, bool) = sqlx::query_as( "SELECT value, is_secret FROM variable WHERE workspace_id = $1 AND path = $2", ) .bind(&w_id) - .bind(&s[5..]) - .fetch_one(db) + .bind(path) + .fetch_optional(db) .await - .map_err(to_anyhow)?; + .map_err(to_anyhow)? + .ok_or_else(|| Error::NotFound(format!("variable {path} does not exist")))?; let value = if is_secret { if is_external_stored_value(&value) { get_secret_value(db, w_id, &s[5..], &value).await? diff --git a/frontend/src/lib/components/ApiConnectForm.svelte b/frontend/src/lib/components/ApiConnectForm.svelte index e3927ddc64..e4ee3ca7b9 100644 --- a/frontend/src/lib/components/ApiConnectForm.svelte +++ b/frontend/src/lib/components/ApiConnectForm.svelte @@ -11,12 +11,14 @@ import Button from './common/button/Button.svelte' import { Loader2 } from 'lucide-svelte' import { untrack } from 'svelte' - import { base } from '$lib/base' import GitHubAppIntegration from './GitHubAppIntegration.svelte' import BedrockCredentialsCheck from './BedrockCredentialsCheck.svelte' import { isCloudHosted } from '$lib/cloud' import ResourceGen from './copilot/ResourceGen.svelte' import SyncResourceTypes from './SyncResourceTypes.svelte' + import { base } from '$lib/base' + import { isDataTableWizardEnabled } from './workspaceSettings/utils.svelte' + import { parsePostgresConnectionString } from '$lib/utils/postgresConnectionString' interface Props { resourceType: string @@ -98,35 +100,42 @@ let connectionString = $state('') let validConnectionString = $state(true) function parseConnectionString(close: (_: any) => void) { - const regex = - /postgres(?:ql)?:\/\/(?[^:@]+)(?::(?[^@]+))?@(?[^:\/?]+)(?::(?\d+))?\/(?[^\?]+)?(?:\?.*sslmode=(?[^&]+))?/ - const match = connectionString.match(regex) - if (match) { - validConnectionString = true - const { user, password, host, port, dbname, sslmode } = match.groups! - rawCode = JSON.stringify( - { - ...args, - user, - password: password || args?.password, - host, - port: (port ? Number(port) : undefined) || args?.port, - dbname: dbname || args?.dbname, - sslmode: sslmode || args?.sslmode - }, - null, - 2 - ) - rawCodeEditor?.setCode(rawCode) - close(null) - } else { + const parts = parsePostgresConnectionString(connectionString) + if (!parts) { validConnectionString = false + return } + validConnectionString = true + rawCode = JSON.stringify( + { + ...args, + user: parts.user, + password: parts.password || args?.password, + host: parts.host, + port: parts.port || args?.port, + dbname: parts.dbname || args?.dbname, + sslmode: parts.sslmode || args?.sslmode + }, + null, + 2 + ) + rawCodeEditor?.setCode(rawCode) + close(null) } let rawCodeEditor: { setCode: (code: string) => void } | undefined = $state(undefined) let textFileContent: string | undefined = $state(undefined) + // The wizard's Supabase entry point is opt-in for now; without it the form keeps the link + // that hands the whole leg over to the resources page. + const wizardEnabled = isDataTableWizardEnabled() + + function applySupabasePick(value: Record) { + args = { ...(args ?? {}), ...value } + rawCode = JSON.stringify(args, null, 2) + rawCodeEditor?.setCode(rawCode) + } + function parseTextFileContent() { args = { content: textFileContent @@ -172,7 +181,7 @@ }} > {#snippet trigger()} - {/snippet} @@ -206,14 +215,28 @@ {/if} {#if resourceType == 'postgresql' && supabaseWizard} - - -
Connect Supabase
- + {#if wizardEnabled} + + {#await import('./workspaceSettings/SupabaseResourceConnect.svelte')} + + {:then Module} + + {/await} + {:else} + + + +
Connect Supabase
+ + {/if} {/if} {:else if step == 2 && manual} -
+
{#if !emptyString(resourceTypeInfo?.description)} {/if} @@ -1332,18 +1332,22 @@ Acquire the token automatically via client credentials instead {/if} - {#key resourceTypeInfo} - - {/key} + +
+ {#key resourceTypeInfo} + + {/key} +
{:else if step == 2 && !manual} {#if manual == false && resourceType != ''} diff --git a/frontend/src/lib/components/Path.svelte b/frontend/src/lib/components/Path.svelte index 284677ce7c..639081cc3d 100644 --- a/frontend/src/lib/components/Path.svelte +++ b/frontend/src/lib/components/Path.svelte @@ -85,6 +85,10 @@ * workspace when the editor operates on a workspace other than the one the * top nav points at (see the sessions preview / dev-workspace flows). */ workspaceOverride?: string + /** One path that does not count as taken, for a caller creating something that may + * already have written there itself — a setup flow correcting its own failed attempt. + * Every other existing path is still refused. */ + allowedExistingPath?: string } let { @@ -102,7 +106,8 @@ disableEditing = false, size = 'md', drawerOffset = 0, - workspaceOverride = undefined + workspaceOverride = undefined, + allowedExistingPath = undefined }: Props = $props() let ws = $derived(workspaceOverride ?? $workspaceStore) @@ -240,6 +245,7 @@ } validateTimeout = setTimeout(async () => { if ( + path !== allowedExistingPath && (path == '' || checkInitialPathExistence || path != initialPath) && (await pathExists(path, kind)) ) { @@ -420,8 +426,12 @@ }) } }) + // Nothing depends on an item that does not exist yet, so editing a *suggested* path is not a + // rename. `checkInitialPathExistence` is what callers set when they are creating something, + // which is the same question asked the other way round. let displayPathChangedWarning = $derived( (['flow', 'script', 'resource', 'variable'] as PathKind[]).includes(kind) && + !checkInitialPathExistence && initialPath && initialPath !== path ) diff --git a/frontend/src/lib/components/WhitelistIp.svelte b/frontend/src/lib/components/WhitelistIp.svelte index be2f8f56dd..fbc32d79b6 100644 --- a/frontend/src/lib/components/WhitelistIp.svelte +++ b/frontend/src/lib/components/WhitelistIp.svelte @@ -22,7 +22,6 @@ {#if ips} -
If necessary, the workers IPs to whitelist are: {ips.join(', ')} diff --git a/frontend/src/lib/components/common/alert/Alert.svelte b/frontend/src/lib/components/common/alert/Alert.svelte index 7c03de5dca..c3d86693be 100644 --- a/frontend/src/lib/components/common/alert/Alert.svelte +++ b/frontend/src/lib/components/common/alert/Alert.svelte @@ -54,6 +54,10 @@ } const SvelteComponent = $derived(icons[type]) + + // A blank title would still occupy a text line and push the body down, leaving an alert + // that is visibly top-heavy. Body-only alerts skip the row, and the gap under it, entirely. + const hasTitleRow = $derived(!!title || collapsible || tooltip != '' || !!documentationLink)
-
- - {title} - {#if tooltip != '' || documentationLink} - {tooltip} - {/if} - - {#if collapsible} - - {/if} -
- - {#if children && !isCollapsed} -
-
- {@render children?.()} -
+ + {#if collapsible} + + {/if}
- {:else if children && !collapsible} -
-
- {@render children?.()} -
+ {/if} + + {#if children && (!collapsible || !isCollapsed)} +
+ {@render children?.()}
{/if}
diff --git a/frontend/src/lib/components/common/modal/Modal2.svelte b/frontend/src/lib/components/common/modal/Modal2.svelte index c00bf758d0..f33b1dec04 100644 --- a/frontend/src/lib/components/common/modal/Modal2.svelte +++ b/frontend/src/lib/components/common/modal/Modal2.svelte @@ -26,6 +26,9 @@ * and clicks "outside" the child would otherwise propagate * here and close the underlying modal. */ closeOnOutsideClick?: boolean + /** Wider side padding and a lighter title, for a dialog whose body is a form rather + * than a list. Opt-in: every other Modal2 keeps the padding and heading it had. */ + formStyling?: boolean headerLeft?: import('svelte').Snippet headerRight?: import('svelte').Snippet children?: import('svelte').Snippet @@ -43,6 +46,7 @@ fixedHeight = 'md', contentClasses = '', closeOnOutsideClick = true, + formStyling = false, headerLeft, headerRight, children @@ -91,7 +95,9 @@ // Elevate above the AI chat panel (zIndexes.aiChat) while chat is open so // the dialog isn't hidden behind it; otherwise keep the default modal // stacking just above disposables (zIndexes.disposables). - const overlayZIndex = $derived(chatState.size > 0 ? zIndexes.aiChat + 1 : zIndexes.disposables + 10) + const overlayZIndex = $derived( + chatState.size > 0 ? zIndexes.aiChat + 1 : zIndexes.disposables + 10 + ) @@ -109,7 +115,8 @@ heightMap[fixedHeight] ? `height: ${heightMap[fixedHeight]}; ` : '' }${css?.popup?.style || ''}`} class={twMerge( - 'max-h-screen-80 max-w-screen-80 rounded-lg relative bg-surface p-4', + 'max-h-screen-80 max-w-screen-80 rounded-lg relative bg-surface', + formStyling ? 'py-4 px-6' : 'p-4', css?.popup?.class, 'wm-modal-form-popup' )} @@ -120,7 +127,7 @@
-

{title}

+

{title}

diff --git a/frontend/src/lib/components/common/stepper/Stepper.svelte b/frontend/src/lib/components/common/stepper/Stepper.svelte index bbd6b1afe4..de6303cedc 100644 --- a/frontend/src/lib/components/common/stepper/Stepper.svelte +++ b/frontend/src/lib/components/common/stepper/Stepper.svelte @@ -4,12 +4,14 @@ import { createEventDispatcher } from 'svelte' interface Props { - tabs: string[]; - selectedIndex?: number; - maxReachedIndex?: number; - statusByStep?: Array<'success' | 'error' | 'pending'>; - hasValidations?: boolean; - allowStepNavigation?: boolean; + tabs: string[] + selectedIndex?: number + maxReachedIndex?: number + statusByStep?: Array<'success' | 'error' | 'pending'> + hasValidations?: boolean + allowStepNavigation?: boolean + /** Compact variant, for steering a dialog rather than a full page. */ + small?: boolean } let { @@ -18,8 +20,9 @@ maxReachedIndex = -1, statusByStep = [], hasValidations = false, - allowStepNavigation = false - }: Props = $props(); + allowStepNavigation = false, + small = false + }: Props = $props() const dispatch = createEventDispatcher() @@ -63,13 +66,20 @@
-
    +
      {#each tabs ?? [] as step, index}
    1. { @@ -77,11 +87,13 @@ }} > {#if statusByStep[index] === 'pending'} - + {:else} {#if index !== (tabs ?? []).length - 1}
    2. -
      +
      3. {/if} {/each} diff --git a/frontend/src/lib/components/copilot/ResourceGen.svelte b/frontend/src/lib/components/copilot/ResourceGen.svelte index 42e42fb9fa..34534285b4 100644 --- a/frontend/src/lib/components/copilot/ResourceGen.svelte +++ b/frontend/src/lib/components/copilot/ResourceGen.svelte @@ -124,6 +124,7 @@ + {:else} + {step.title} + {/if} + + {#if descriptionOpened} +
      + {step.description} +
      + {/if} +       +
+
+ {#if step.substeps?.length} +
+ +
+ {/if} +
+ {/each} +
diff --git a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte new file mode 100644 index 0000000000..2e19cbea82 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte @@ -0,0 +1,1435 @@ + + + opened, + (v) => { + if (!v) requestClose() + else opened = v + } + } + target="#content" + formStyling + title="Add a data table" + contentClasses="flex flex-col" + fixedWidth="md" + fixedHeight="lg" +> +
+ goToStep(e.detail.index)} + /> + +
+
+ {#if run.steps.length} + + {#if run.running} +

+ Setting up. This can take a few minutes — leave this open until it finishes. +

+ {/if} + {#if run.result} + {@render poolerWarning()} + + {/if} + {:else if wiz.step === 1} + + A data table runs on a database of your own. It stays yours — you can take it with + you at any time. + +
+ {#if $isCustomInstanceDbEnabled} + {#snippet instanceIcon()} + + {/snippet} + {@render providerCard( + 'instance', + instanceIcon, + 'Windmill database', + 'Windmill creates and manages a database on this instance.' + )} + {/if} + {#if supabaseAvailable} + {#snippet supabaseIcon()} + + {/snippet} + {@render providerCard( + 'supabase', + supabaseIcon, + 'Supabase', + 'Create a project, or connect one you already have. Signing in is required, and connecting an existing project needs its database password.' + )} + {/if} + {#snippet ownIcon()} + + {/snippet} + {@render providerCard( + 'resource', + ownIcon, + 'Your own database', + 'Any Postgres — RDS, Neon, self-hosted. Pick a resource, or paste a connection string.' + )} +
+ {:else if wiz.step === 2} + {#if wiz.provider === 'supabase'} + {#if !supaOauth.authed} + + {#if supaOauth.pending} + Sign in and approve Windmill in the Supabase window, then come back here. + {:else} + Windmill needs your approval on Supabase to see your databases. + {/if} + + {:else} + invalidate()} + /> + {/if} + {:else if wiz.provider === 'instance'} + {@render instanceStep()} + {:else} + {@render ownStep()} + {/if} + + + {:else} +
+ {@render reviewStep()} +
+ {/if} +
+ +
+
+
+ {#if wiz.step > 1 && !run.steps.length} + + {:else if canEditAfterFailure} + + + {/if} +
+ +
+ {#if wiz.provider === 'supabase' && !supaOauth.authed} +

+ If you do not have a Supabase account you can create one for free. +

+ {/if} +
+
+
+ + +{#snippet providerCard(key: Provider, icon: Snippet, title: string, subtitle: string)} + {@const selected = wiz.provider === key} + +{/snippet} + +{#snippet instanceStep()} + {@const instanceDbs = Object.entries(customInstanceDbs.current ?? {}) + .filter(([_, db]) => db.tag === 'datatable') + .map(([name, db]) => ({ name, db }))} + {#if instanceDbs.length} + wiz.instance.mode, + (v) => { + wiz.instance.mode = v + wiz.instance.dbName = v === 'create' ? defaultInstanceDbName() : undefined + } + } + > + {#snippet children({ item })} + + + {/snippet} + + {/if} + {#if wiz.instance.mode === 'existing'} + {@const shared = ( + customInstanceDbs.current?.[wiz.instance.dbName ?? '']?.used_by_workspaces ?? [] + ).filter((w) => w !== $workspaceStore)} + + {#if shared.length} + + This database is also used by workspace{shared.length > 1 ? 's' : ''} + {shared.join(', ')}. Any data written here will be shared + with {shared.length > 1 ? 'them' : 'it'}. + + {/if} +
+ {#each instanceDbs as { name, db } (name)} + {@const selected = wiz.instance.dbName === name} + {@const others = (db.used_by_workspaces ?? []).filter((w) => w !== $workspaceStore)} + + {/each} +
+ {:else} +
+ Database name + wiz.instance.dbName ?? '', (v) => (wiz.instance.dbName = v)} + error={!!instanceNameError} + inputProps={{ placeholder: defaultInstanceDbName() }} + /> + + {#if !instanceNameError} +

+ Created in the Windmill PostgreSQL instance when you finish. Windmill manages its + credentials. +

+ {/if} +
+ {/if} +{/snippet} + +{#snippet ownStep()} + {@const resources = pgResources.loading ? undefined : pgResources.current} + {#if resources === undefined} +

Loading resources...

+ {:else} + {#if resources.length} + Postgres resources in this workspace + {:else} +

A resource is a saved connection your scripts can use.

+ {/if} +
+ {#each resources as r (r.path)} + {@const selected = !wiz.own.creating && wiz.own.resourcePath === r.path} + + {/each} + +
+ + {#if wiz.own.creating} +
{@render newResourceForm()}
+ {/if} +
+
+ {/if} +{/snippet} + +{#snippet newResourceForm()} +
+
+ + {wiz.own.form === 'string' ? 'Connection string' : 'Connection'} + + +
+ {#if wiz.own.form === 'string'} + wiz.own.connectionString, + (v) => { + wiz.own.connectionString = v + absorbConnectionString(v) + invalidate() + } + } + error={!!connectionStringError} + inputProps={{ placeholder: 'postgres://user:password@host:5432/database' }} + /> + + {:else} +
+
+ Host + wiz.own.fields.host, (v) => setField('host', v)} + inputProps={{ placeholder: 'db.example.com' }} + /> +
+
+ Port + wiz.own.fields.port ?? '', + (v) => setField('port', v === '' ? undefined : Number(v)) + } + inputProps={{ placeholder: '5432', type: 'number' }} + /> +
+
+ Database + wiz.own.fields.dbname ?? '', (v) => setField('dbname', v)} + inputProps={{ placeholder: 'postgres' }} + /> +
+
+ SSL mode + certVerification, + (v) => + setAdvanced('accept_invalid_certs', v === 'default' ? undefined : v === 'accept') + } + clearable={false} + /> +
+ setAdvanced('use_iam_auth', e.detail)} + options={{ right: 'Authenticate with AWS IAM' }} + /> + {#if wiz.own.advanced.use_iam_auth} +
+ Region + wiz.own.advanced.region, (v) => setAdvanced('region', v)} + inputProps={{ placeholder: 'us-east-1' }} + /> +
+ {/if} +
+ +
+{/snippet} + +{#snippet poolerWarning()} + {#if poolerUnavailable} + +
+ {poolerUnavailable} + + Windmill connects directly instead, which needs IPv6 from the workers, or the IPv4 add-on + on the project. Granting the Supabase OAuth app + database_pooling_config_read and connecting again restores the + pooler. + +
+ + {/if} +{/snippet} + +{#snippet reviewStep()} + {#if lastFailure} + {lastFailure} + {/if} + + + {#if wiz.provider === 'supabase'} + + + {@render poolerWarning()} + {:else if wiz.provider === 'instance'} + + {:else if !wiz.own.creating} + + {/if} + + {#if mintsResource} + + {/if} + + {#if sharesDatabaseWith} + + {sharesDatabaseWith.name} already uses this database. Both data + tables would write to the same schema, so each one's tables are visible to the other and two tables + of the same name collide. Migrations are tracked per data table, so those stay separate. + + {/if} + + {#if wiz.provider === 'supabase'} + + Your Supabase sign-in is not stored. If the database password ever changes, anyone with access + to the project can sign in and reconnect it. Deleting the data table never deletes the + Supabase project. + + {/if} +{/snippet} diff --git a/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte b/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte index 644f5e2af5..8246118c49 100644 --- a/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte +++ b/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte @@ -8,7 +8,7 @@ import { slide } from 'svelte/transition' import Modal2 from '../common/modal/Modal2.svelte' import Alert from '../common/alert/Alert.svelte' - import LoggedWizardResult, { firstEmptyStepIsError } from '../wizards/LoggedWizardResult.svelte' + import SetupChecklist from '../wizards/SetupChecklist.svelte' import Button from '../common/button/Button.svelte' import { sendUserToast } from '$lib/toast' import { isCustomInstanceDbEnabled } from './utils.svelte' @@ -20,6 +20,7 @@ import { truncate } from '$lib/utils' import Tooltip from '../meltComponents/Tooltip.svelte' import { superadmin } from '$lib/stores' + import { instanceSetupSteps } from './instanceDbSteps' type Props = { customInstanceDbs: ResourceReturn @@ -45,6 +46,7 @@ !!opened, (v) => !v && !preventClose && (opened = undefined)} target="#content" + formStyling title={'Custom Instance Database Setup'} contentClasses="flex flex-col" fixedWidth="md" @@ -59,7 +61,7 @@
{dbname} - + Custom instance databases are databases created in the Windmill PostgreSQL instance. Their credentials are automatically managed by Windmill and are never exposed to users. Only super admins can create them. @@ -127,68 +129,8 @@
{/if} -
{#if $superadmin} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableConnectionReport.svelte b/frontend/src/lib/components/workspaceSettings/DataTableConnectionReport.svelte new file mode 100644 index 0000000000..68984321a3 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/DataTableConnectionReport.svelte @@ -0,0 +1,77 @@ + + +{#if error} + + {error} + +{:else if report} + +
+
+ Connects as {report.user}{#if report.schema}, resolving + unqualified statements to schema {report.schema}{/if}. +
+ {#if report.suggested_search_path} +
+ Its search_path resolves to no schema, so unqualified statements fail with + no schema has been selected to create in whatever + privileges the role holds. Point it at one, e.g. + {report.suggested_search_path}. +
+ {/if} +
    +
  • + Create tables{report.schema ? ` in ${report.schema}` : ''}: + {report.can_create_table ? 'yes' : 'no'} +
    • +
  • + Create schemas: + {report.can_create_schema ? 'yes' : 'no'} +
    • +
  • + Migration bookkeeping table exists: + {report.migrations_table_exists ? 'yes' : 'no'} +
    • +
+ {#if report.suggested_grants.length > 0} +
+ Windmill connects as the role that lacks these privileges, so it cannot grant them itself. + Run as a schema owner or superuser on that database: +
+ 
{report.suggested_grants.map((g) => `${g};`).join('\n')}
+ {#if report.schema && !report.can_create_table && !report.migrations_table_exists} +
+ Alternatively, create the _wm_migrations bookkeeping table + yourself and grant only SELECT, INSERT, UPDATE, DELETE on it. +
+ {/if} + {/if} +
+ +{/if} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index 254a20fe80..bb97e434f6 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -66,7 +66,11 @@ import Row from '../table/Row.svelte' import TextInput from '../text_input/TextInput.svelte' import Tooltip from '../Tooltip.svelte' - import { isCustomInstanceDbEnabled, getUnusedInstanceDbName } from './utils.svelte' + import { + isCustomInstanceDbEnabled, + getUnusedInstanceDbName, + isDataTableWizardEnabled + } from './utils.svelte' import { random_adj } from '../random_positive_adjetive' import { sendUserToast } from '$lib/toast' import { @@ -89,6 +93,10 @@ import Alert from '../common/alert/Alert.svelte' import MissingWorkerTagAlert from '../jobs/MissingWorkerTagAlert.svelte' import { isCloudHosted } from '$lib/cloud' + import AddDataTableWizard from './AddDataTableWizard.svelte' + import { takeParkedWizard, type WizardResume } from './wizardParking' + import { Database } from 'lucide-svelte' + import { onMount } from 'svelte' type Props = { dataTableSettings: DataTableSettingsType @@ -156,6 +164,8 @@ return getUnusedInstanceDbName('dt', $workspaceStore ?? '', usedNames) } + // Kept for the flag-off path: adding a data table is a row in this table that the user + // fills in and saves, rather than a wizard. function onNewDataTable() { const name = tempSettings.dataTables.some((d) => d.name === 'main') ? `${random_adj()}_datatable` @@ -211,6 +221,37 @@ } } + const wizardEnabled = isDataTableWizardEnabled() + let wizardOpen = $state(false) + /** Opened through the wizard's own `open()`, which is what sets a fresh run up. */ + let wizard: { open: (parked?: WizardResume) => void } | undefined = $state(undefined) + let wizardResume: WizardResume | undefined = $state(undefined) + + // Supabase sends the user back here after authorizing; pick the wizard back up where it + // was rather than making them start again. + onMount(() => { + if (!wizardEnabled) return + const parked = takeParkedWizard() + if (parked) { + wizardResume = parked + // Handed in, not left to the `resume` prop: the wizard rebuilds the run synchronously + // inside this call, and a parked run that arrived late would come back as a fresh one. + wizard?.open(parked) + } + }) + + /** + * The wizard persists what it creates, so the server is authoritative afterwards and the + * whole baseline comes from it. `tempSettings` derives from that baseline, so this discards + * uncommitted edits in the table -- which is why the wizard cannot be opened while there + * are any (see the disabled entry points below). + */ + async function reloadAfterWizard() { + const s = await WorkspaceService.getSettings({ workspace: $workspaceStore! }) + dataTableSettings = convertDataTableSettingsFromBackend(s.datatable) + wizardResume = undefined + } + let confirmationModal = createAsyncConfirmationModal() let dirtyMap = $derived.by(() => { const map: Record = {} @@ -241,7 +282,7 @@ @@ -273,9 +314,37 @@ {#if tempSettings.dataTables.length == 0} - - No data table in this workspace yet - + {#if wizardEnabled} + +
+ +
+ No data table yet +

+ Give your scripts a database to store and query data. + {#if isCloudHosted()} + Set one up free in about a minute. + {:else} + Use the Windmill database, or bring your own. + {/if} +

+
+ +
+ + {:else} + + No data table in this workspace yet + + {/if} {/if} {#each tempSettings.dataTables as dataTable, dataTableIndex (dataTable.id)} @@ -383,15 +452,27 @@ {/each} - - -
- -
- - + {#if !wizardEnabled || tempSettings.dataTables.length > 0} + + +
+ +
+ + + {/if} @@ -467,3 +548,28 @@ /> + +{#if wizardEnabled} + wizardOpen, + (v) => { + wizardOpen = v + // Drop the parked run once the wizard closes: leaving it set would force the next + // open straight back to the Supabase setup step. + if (!v) wizardResume = undefined + } + } + existingNames={tempSettings.dataTables.map((d) => d.name)} + existingDataTables={tempSettings.dataTables.map((d) => ({ + name: d.name, + resourcePath: d.database.resource_path + }))} + resume={wizardResume} + onDone={reloadAfterWizard} + {customInstanceDbs} + {confirmationModal} + {defaultInstanceDbName} + /> +{/if} diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte new file mode 100644 index 0000000000..6f2523a729 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte @@ -0,0 +1,62 @@ + + +
+ + {#if open} +
+ + {#each OPTIONS as option (option.value)} + {@const selected = mode === option.value} + + {/each} +
+ {/if} +
diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte new file mode 100644 index 0000000000..0406587ce7 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte @@ -0,0 +1,290 @@ + + +{#if loading} +
+ + Loading your Supabase projects... +
+{:else if (projects ?? []).length === 0 && existingOnly} + + This Supabase account has no projects yet. + +{:else} + {#if (projects ?? []).length} + Projects in your Supabase account + {:else} +

This Supabase account has no projects yet.

+ {/if} +
+ {#each projects ?? [] as p (projectRef(p))} + {@const selected = intent.mode === 'existing' && isSelected(intent.project, p)} + +
+ + {#if selected} +
+
+ Database password + intent.password, (v) => ((intent.password = v ?? ''), onIntentChange?.()) + } + placeholder="••••••••" + /> +

+ Supabase only shows this when the project is created, and never exposes it through + its API. If you no longer have it, set a new one — every existing connection to this project stops working when you do. +

+
+ +
+ {/if} +
+ {/each} + {#if !existingOnly} +
+ + {#if intent.mode === 'create'} +
{@render newProjectFields()}
+ {/if} +
+ {/if} +
+{/if} + +{#snippet newProjectFields()} +
+
+
+ Organization + + ({ label: r.label, value: r.code }))} + bind:value={() => intent.region, (v) => ((intent.region = v), onIntentChange?.())} + placeholder="Region" + /> +
+
+
+ Project name + intent.projectName, (v) => ((intent.projectName = String(v)), onIntentChange?.()) + } + inputProps={{ placeholder: 'windmill-data' }} + /> +
+ + Windmill generates and stores the database password. A new project takes a minute or two to + come up. + + +
+{/snippet} diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte new file mode 100644 index 0000000000..f9f4586503 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte @@ -0,0 +1,120 @@ + + + + + +
+
+ {#if oauth.token} + + {/if} +
+
+ +
+
+ diff --git a/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts b/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts new file mode 100644 index 0000000000..cb42d73075 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts @@ -0,0 +1,434 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const listSupabaseProjectsMock = vi.fn() +const createSupabaseProjectMock = vi.fn() +vi.mock('./supabaseProvisioning', async (importOriginal) => ({ + ...(await importOriginal()), + listSupabaseProjects: (...a: any[]) => listSupabaseProjectsMock(...a), + createSupabaseProject: (...a: any[]) => createSupabaseProjectMock(...a), + generateDbPassword: () => 'generated-password', + // Whatever the run does after creating a project is not what these tests are about, and the + // real ones poll Supabase until it answers. + waitUntilSupabaseHealthy: async (_t: string, _r: string) => ({ id: '2', name: 'later' }), + resolveSupabaseConnection: async () => { + throw new Error('stop the run here') + } +})) + +const existsVariableMock = vi.fn() +const getVariableMock = vi.fn() +const getResourceMock = vi.fn() +const createVariableMock = vi.fn() +const getSettingsMock = vi.fn() +const editDataTableConfigMock = vi.fn() +const testDataTableConnectionMock = vi.fn() +const setupCustomInstanceDbMock = vi.fn() +vi.mock('$lib/gen', () => ({ + VariableService: { + existsVariable: (...a: any[]) => existsVariableMock(...a), + getVariable: (...a: any[]) => getVariableMock(...a), + createVariable: (...a: any[]) => createVariableMock(...a), + updateVariable: vi.fn() + }, + ResourceService: { + existsResource: vi.fn(), + getResource: (...a: any[]) => getResourceMock(...a), + createResource: vi.fn(), + updateResource: vi.fn() + }, + SettingService: { setupCustomInstanceDb: (...a: any[]) => setupCustomInstanceDbMock(...a) }, + WorkspaceService: { + getSettings: (...a: any[]) => getSettingsMock(...a), + editDataTableConfig: (...a: any[]) => editDataTableConfigMock(...a), + testDataTableConnection: (...a: any[]) => testDataTableConnectionMock(...a) + } +})) + +import { + intentComplete, + newResourceParts, + newWizardState, + runSetup, + type WizardState +} from './addDataTableModel' +import { noClaims } from './setupClaims' + +/** Nothing at the path: the reads that answer "is this ours?" find no object. */ +function nothingThere() { + getVariableMock.mockRejectedValue(new Error('not found')) + getResourceMock.mockRejectedValue(new Error('not found')) +} + +/** A resource that exists, with the timestamp the claim is marked by. */ +function resourceEditedAt(at: string) { + getResourceMock.mockResolvedValue({ path: 'p', created_by: 'alice', edited_at: at }) +} + +/** A wizard about to create the Supabase project `later`, in the organization `acme`. */ +function creating(): WizardState { + const state = newWizardState({ name: 'main', projectName: 'later', folder: 'f/team' }) + state.provider = 'supabase' + state.supabase.mode = 'create' + state.supabase.org = 'acme' + state.review.resourceName = 'db' + return state +} + +/** The path `creating()` writes to, and where an earlier attempt's password would sit. */ +const MINTED_PATH = 'f/team/db' + +const deps = (createdProjectName?: string, createdProjectPath = MINTED_PATH) => ({ + workspace: 'w', + supabaseToken: 'token', + onProgress: () => {}, + claims: noClaims, + username: 'alice', + createdProjects: createdProjectName + ? [{ name: createdProjectName, path: createdProjectPath }] + : [] +}) + +// `writeSecret` overwrites in place, and Supabase never shows a project's password twice, so +// minting a second one at the path where an earlier project's is stored destroys the only copy. +describe('runSetup refusing to mint over a project it already created', () => { + beforeEach(() => { + vi.clearAllMocks() + existsVariableMock.mockResolvedValue(false) + nothingThere() + }) + + it('refuses while the earlier project is still there', async () => { + listSupabaseProjectsMock.mockResolvedValue([ + { id: '1', name: 'earlier', organization_id: 'acme' } + ]) + const result = await runSetup(creating(), deps('earlier')) + expect(result.ok).toBe(false) + expect(result.error).toContain('earlier') + expect(createVariableMock).not.toHaveBeenCalled() + expect(createSupabaseProjectMock).not.toHaveBeenCalled() + }) + + // The name is also recorded when a create could not be confirmed -- an expired token answers + // neither the create nor the lookup. Refusing on that forever would strand the session. + it('proceeds when no project by that name exists after all', async () => { + listSupabaseProjectsMock.mockResolvedValue([]) + createSupabaseProjectMock.mockResolvedValue({ id: '2', name: 'later' }) + await runSetup(creating(), deps('earlier')) + expect(createSupabaseProjectMock).toHaveBeenCalled() + }) + + // Connecting the created project as an existing one reaches the same secret by another + // route: the project list on step 2 is where it now appears, so this is the likely move. + it('refuses to write over the secret from the existing-project branch', async () => { + const state = creating() + state.supabase.mode = 'existing' + state.supabase.project = { id: '1', name: 'earlier' } as any + state.supabase.password = 'typed-by-hand' + const result = await runSetup(state, deps('earlier')) + expect(result.ok).toBe(false) + expect(result.error).toContain(MINTED_PATH) + expect(createVariableMock).not.toHaveBeenCalled() + }) + + // Aimed somewhere else, there is nothing to protect -- and over-refusing here would block + // the ordinary way out of every refusal above, which is to choose another path. + it('writes when the run is aimed at a different path', async () => { + const state = creating() + state.supabase.mode = 'existing' + state.supabase.project = { id: '1', name: 'earlier' } as any + state.supabase.password = 'typed-by-hand' + await runSetup(state, deps('earlier', 'f/team/somewhere-else')) + expect(createVariableMock).toHaveBeenCalled() + }) + + // The organization selected now is not the one the earlier project was created under, and + // switching it is one of the ways to arrive here. + it('refuses a project listed under a different organization', async () => { + listSupabaseProjectsMock.mockResolvedValue([ + { id: '1', name: 'earlier', organization_id: 'other-org' } + ]) + const result = await runSetup(creating(), deps('earlier')) + expect(result.ok).toBe(false) + expect(createSupabaseProjectMock).not.toHaveBeenCalled() + }) +}) + +// The instance branch is the one that has to write its row before it can probe it, since the +// probe is by data table name. A database Windmill cannot store data in must not stay in the +// config -- and a probe that throws leaves exactly the same unusable row as one that says no. +describe('runSetup rolling the instance row back', () => { + function usingInstanceDb(): WizardState { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'instance' + state.instance = { mode: 'existing', dbName: 'shared' } + return state + } + + // The rollback reads the config back before deleting, so the config has to behave like one: + // a mock that always answers empty would let a rollback that never finds its own row pass. + let datatables: Record + + beforeEach(() => { + vi.clearAllMocks() + datatables = {} + getSettingsMock.mockImplementation(async () => ({ datatable: { datatables } })) + editDataTableConfigMock.mockImplementation(async ({ requestBody }: any) => { + datatables = { ...requestBody.settings.datatables } + }) + setupCustomInstanceDbMock.mockResolvedValue({ success: true, logs: {} }) + nothingThere() + }) + + // The pre-flight runs once, before a Supabase create that can take minutes, and every + // wizard suggests the same `main` -- so the name can be taken by the time the row is + // written. Repointing it would hand another admin's data table a database nobody chose. + it('refuses a name that was taken while it was running', async () => { + datatables = { main: { database: { resource_path: 'someone-else' } } } + const result = await runSetup(usingInstanceDb(), { + workspace: 'w', + onProgress: () => {}, + claims: noClaims, + username: 'alice', + createdProjects: [] + } as any) + expect(result.ok).toBe(false) + expect(result.error).toContain('main') + expect(editDataTableConfigMock).not.toHaveBeenCalled() + }) + + // Rolling back is just as dangerous once someone else owns the name: the row under it is + // no longer the one this run wrote. + it('leaves a row it no longer recognises alone', async () => { + // Repointed by someone else while this run was probing it. + testDataTableConnectionMock.mockImplementation(async () => { + datatables = { main: { database: { resource_path: 'someone-else' } } } + throw new Error('connection refused') + }) + const result = await runSetup(usingInstanceDb(), { + workspace: 'w', + onProgress: () => {}, + claims: noClaims, + username: 'alice', + createdProjects: [] + } as any) + expect(result.ok).toBe(false) + expect(result.rowRolledBack).toBe(false) + // One call: the write. The rollback found a row it did not write and left it. + expect(editDataTableConfigMock).toHaveBeenCalledTimes(1) + // And the name is not handed back as ours: claiming it would let Try again write over + // the row the other admin now owns. + expect(result.rowWritten).toBe(false) + }) + + it('takes the row back out when the probe never answers', async () => { + testDataTableConnectionMock.mockRejectedValue(new Error('connection refused')) + const result = await runSetup(usingInstanceDb(), { + workspace: 'w', + supabaseToken: undefined, + onProgress: () => {}, + claims: noClaims, + username: 'alice', + createdProjects: [] + } as any) + expect(result.ok).toBe(false) + expect(result.error).toContain('connection refused') + expect(result.rowRolledBack).toBe(true) + expect(result.rowWritten).toBe(false) + const lastWrite = editDataTableConfigMock.mock.calls.at(-1)?.[0] + expect(lastWrite.requestBody.settings.datatables).not.toHaveProperty('main') + }) +}) + +// The fields are the connection; a connection string is a way of writing one down. Reading the +// resource back out of the string is what let a URI grammar gap change what got saved. +describe('newResourceParts', () => { + function typedByHand(): WizardState { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'resource' + state.own.creating = true + state.own.fields = { + host: 'db.example.com', + port: 5432, + dbname: 'mydb', + user: 'u', + password: 'p', + sslmode: 'prefer' + } + return state + } + + it('reads the fields whichever notation is on screen', () => { + const state = typedByHand() + state.own.form = 'string' + state.own.connectionString = 'postgres://u:p@db.example.com:5432/mydb' + // The string names no sslmode. The choice on the fields is what gets saved. + expect(newResourceParts(state)?.sslmode).toBe('prefer') + state.own.form = 'fields' + expect(newResourceParts(state)?.sslmode).toBe('prefer') + }) + + it('is unaffected by a string that cannot be parsed', () => { + const state = typedByHand() + state.own.form = 'string' + state.own.connectionString = 'not a uri' + expect(newResourceParts(state)?.host).toBe('db.example.com') + }) +}) + +// `created_by` survives an update, so it cannot tell an edit by somebody else from no edit at +// all. The claim is marked by `edited_at`, which moves on every write. +describe('runSetup writing over a resource', () => { + function ownResource(): WizardState { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'resource' + state.own.creating = true + state.review.resourceName = 'db' + state.own.fields = { + host: 'h', + port: 5432, + dbname: 'd', + user: 'u', + password: 'p', + sslmode: 'require' + } + return state + } + + beforeEach(() => { + vi.clearAllMocks() + existsVariableMock.mockResolvedValue(false) + getVariableMock.mockRejectedValue(new Error('not found')) + getSettingsMock.mockResolvedValue({ datatable: { datatables: {} } }) + editDataTableConfigMock.mockResolvedValue(undefined) + testDataTableConnectionMock.mockResolvedValue({ can_create_table: true }) + }) + + it('refuses a resource edited since this run claimed it', async () => { + resourceEditedAt('2026-01-02T00:00:00Z') + const result = await runSetup(ownResource(), { + workspace: 'w', + onProgress: () => {}, + // Claimed when it looked like this; someone has written to it since. + claims: [{ kind: 'resource' as const, path: 'f/team/db', mark: '2026-01-01T00:00:00Z' }], + username: 'alice', + createdProjects: [] + } as any) + expect(result.ok).toBe(false) + expect(result.error).toContain('f/team/db') + }) +}) + +describe('runSetup writing over its own secret', () => { + const ownDb = (): WizardState => { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'resource' + state.own.creating = true + state.review.resourceName = 'db' + state.own.fields = { + host: 'h', + port: 5432, + dbname: 'd', + user: 'u', + password: 'p', + sslmode: 'require' + } + return state + } + + beforeEach(() => { + vi.clearAllMocks() + getResourceMock.mockRejectedValue(new Error('not found')) + getSettingsMock.mockResolvedValue({ datatable: { datatables: {} } }) + editDataTableConfigMock.mockResolvedValue(undefined) + testDataTableConnectionMock.mockResolvedValue({ can_create_table: true }) + }) + + // The same person editing the variable in another tab leaves `edited_by` unchanged, so an + // author is not enough to tell that write from none. + it('refuses a secret edited since this run claimed it, even by the same user', async () => { + getVariableMock.mockResolvedValue({ edited_by: 'alice', edited_at: '2026-01-02T00:00:00Z' }) + const result = await runSetup(ownDb(), { + workspace: 'w', + onProgress: () => {}, + claims: [{ kind: 'secret' as const, path: 'f/team/db', mark: '2026-01-01T00:00:00Z' }], + username: 'alice', + createdProjects: [] + } as any) + expect(result.ok).toBe(false) + expect(result.error).toContain('f/team/db') + }) + + // A create whose confirmation also failed records the project name pessimistically. The + // variable it wrote is still its own, and a retry has to be able to reuse the path. + it('reuses the variable a previous attempt wrote when its project was never confirmed', async () => { + getVariableMock.mockResolvedValue({ edited_by: 'alice', edited_at: '2026-01-01T00:00:00Z' }) + listSupabaseProjectsMock.mockResolvedValue([]) + createSupabaseProjectMock.mockResolvedValue({ id: '2', name: 'later' }) + const state = creating() + const result = await runSetup(state, { + ...deps('later'), + claims: [{ kind: 'secret' as const, path: MINTED_PATH, mark: '2026-01-01T00:00:00Z' }] + } as any) + expect(result.error ?? '').not.toContain('was created at') + }) +}) + +// Editing a valid string into an invalid one keeps the fields, so they stay correctable. What +// must not happen is testing or saving those fields while the string on screen says otherwise. +describe('intentComplete with a connection string on screen', () => { + function typed(connectionString: string): WizardState { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'resource' + state.own.creating = true + state.own.form = 'string' + state.own.connectionString = connectionString + state.own.fields = { + host: 'db.example.com', + port: 5432, + dbname: 'mydb', + user: 'u', + password: 'p', + sslmode: 'require' + } + return state + } + + it('refuses a string that will not parse, whatever the fields still hold', () => { + expect(intentComplete(typed('postgres://u:p@db.example.com:5432/mydb'))).toBe(true) + expect(intentComplete(typed('postgres://u:p@db.exa'))).toBe(false) + expect(intentComplete(typed(''))).toBe(false) + }) + + it('is unaffected once the fields are the notation on screen', () => { + const state = typed('nonsense') + state.own.form = 'fields' + expect(intentComplete(state)).toBe(true) + }) +}) + +// Each created project guards its own path. Keeping only the latest let a second attempt at +// another path unlock the first project's password, which Supabase will never show again. +describe('runSetup guarding more than one created project', () => { + beforeEach(() => { + vi.clearAllMocks() + existsVariableMock.mockResolvedValue(false) + nothingThere() + }) + + it('still refuses the first project’s path after a second was created elsewhere', async () => { + listSupabaseProjectsMock.mockResolvedValue([ + { id: '1', name: 'first', organization_id: 'acme' } + ]) + const state = creating() + const result = await runSetup(state, { + ...deps(), + createdProjects: [ + { name: 'first', path: MINTED_PATH }, + { name: 'second', path: 'f/team/other' } + ] + } as any) + expect(result.ok).toBe(false) + expect(result.error).toContain('first') + expect(createVariableMock).not.toHaveBeenCalled() + }) +}) diff --git a/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts new file mode 100644 index 0000000000..e01987c3ad --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts @@ -0,0 +1,853 @@ +/** + * Everything the "add a data table" wizard collects, and the one function that acts on it. + * + * The wizard writes nothing until the user finishes: steps 1 and 2 gather intent, step 3 + * reviews it, and `runSetup` performs it. That ordering is what lets the review step show + * the resource path before the resource exists. + * + * `runSetup` is also what Try again calls, so every step has to tolerate the results of a + * previous attempt still being there. + */ + +import { + ResourceService, + SettingService, + VariableService, + WorkspaceService, + type TestDataTableConnectionResponse +} from '$lib/gen' +import type { SetupStep } from '../wizards/SetupChecklist.svelte' +import { instanceSetupSteps } from './instanceDbSteps' +import { claim, stillOurs, type Claims } from './setupClaims' +import { probeDatatableConnection } from './datatableProbe' +import { + DEFAULT_SSLMODE, + parsePostgresConnectionString, + unsupportedConnectionParam, + type PostgresConnectionParts +} from '$lib/utils/postgresConnectionString' +import { + createSupabaseProject, + generateDbPassword, + resolveSupabaseConnection, + listSupabaseProjects, + projectOrg, + projectRef, + orgSlug, + supabaseResourceValue, + waitUntilSupabaseHealthy, + DEFAULT_SUPABASE_REGION, + type SupabaseConnectionMode, + type SupabaseOrg, + type SupabaseProject +} from './supabaseProvisioning' + +export type Provider = 'supabase' | 'instance' | 'resource' + +export type WizardState = { + step: 1 | 2 | 3 + provider: Provider | undefined + supabase: { + mode: 'existing' | 'create' + project: SupabaseProject | undefined + password: string + /** + * The whole organization, not its slug: the API is called with the slug, but a slug is a + * random string and the review step has a person reading it. + */ + org: SupabaseOrg | undefined + region: string + projectName: string + connectionMode: SupabaseConnectionMode + } + instance: { mode: 'existing' | 'create'; dbName: string | undefined } + /** + * One list: the workspace's Postgres resources, plus the one about to exist. A + * connection string is not an alternative to a resource, it is how one is written -- + * so `creating` and `resourcePath` are the two ways of answering the same question and + * are never both set. + */ + own: { + resourcePath: string | undefined + creating: boolean + /** Which notation the new resource is being entered in. Same object either way. */ + form: 'string' | 'fields' + connectionString: string + fields: PostgresConnectionParts + /** The resource fields no URI can carry, so they belong to neither notation. */ + advanced: PostgresAdvanced + } + review: { name: string; folder: string; resourceName: string } + /** Result of validating what step 2 collected. Cleared whenever its input changes. */ + probe: { + checking: boolean + report: TestDataTableConnectionResponse | undefined + error: string | undefined + } +} + +export function newWizardState(defaults: { + name: string + projectName: string + folder: string +}): WizardState { + return { + step: 1, + provider: undefined, + supabase: { + // Nothing is chosen yet; the step decides between the two once it knows whether the + // account has any projects. `create` here would be indistinguishable from the user + // having picked "New project", which is what survives a Back out of the step. + mode: 'existing', + project: undefined, + password: '', + org: undefined, + region: DEFAULT_SUPABASE_REGION, + projectName: defaults.projectName, + connectionMode: 'session' + }, + instance: { mode: 'create', dbName: undefined }, + own: { + resourcePath: undefined, + creating: false, + form: 'string', + connectionString: '', + fields: emptyFields(), + advanced: emptyAdvanced() + }, + review: { name: defaults.name, folder: defaults.folder, resourceName: '' }, + probe: { checking: false, report: undefined, error: undefined } + } +} + +export function clearProbe(state: WizardState) { + state.probe = { checking: false, report: undefined, error: undefined } +} + +/** Path of the resource and secret variable the run will write. They share one. */ +export function resourcePathOf(state: WizardState): string { + return `${state.review.folder}/${state.review.resourceName}` +} + +/** True once the branch has everything `runSetup` needs. */ +export function intentComplete(state: WizardState): boolean { + if (state.provider === 'supabase') { + return state.supabase.mode === 'create' + ? !!state.supabase.projectName.trim() && !!state.supabase.org + : !!state.supabase.project && !!state.supabase.password + } + if (state.provider === 'instance') return !!state.instance.dbName?.trim() + if (!state.own.creating) return !!state.own.resourcePath + // Text that will not parse leaves the fields on their last good values, which is what makes + // it correctable -- but the connection on screen is then not the one they describe, and + // testing or saving the old one behind an unparseable string points the data table + // somewhere nobody asked for. + if ( + state.own.form === 'string' && + (!parsePostgresConnectionString(state.own.connectionString) || + unsupportedConnectionParam(state.own.connectionString)) + ) + return false + return !!newResourceParts(state) +} + +/** + * The `postgresql` fields outside the connection-string vocabulary: TLS verification and + * AWS IAM auth. Kept apart from the parts so composing a string cannot appear to drop them. + */ +export type PostgresAdvanced = { + root_certificate_pem: string + /** + * Undefined is meaningful: the backend then verifies only when a root certificate is + * present. Only ever set by an explicit choice. + */ + accept_invalid_certs: boolean | undefined + use_iam_auth: boolean + region: string +} + +function emptyAdvanced(): PostgresAdvanced { + return { + root_certificate_pem: '', + accept_invalid_certs: undefined, + use_iam_auth: false, + region: '' + } +} + +/** Whether anything was set, so a notation that cannot show them can say they apply. */ +export function hasAdvanced(advanced: PostgresAdvanced): boolean { + return ( + !!advanced.root_certificate_pem.trim() || + advanced.accept_invalid_certs !== undefined || + advanced.use_iam_auth || + !!advanced.region.trim() + ) +} + +function emptyFields(): PostgresConnectionParts { + return { + host: '', + port: 5432, + dbname: 'postgres', + user: '', + password: '', + sslmode: DEFAULT_SSLMODE + } +} + +const RESERVED_DB_NAMES = ['template0', 'template1', 'postgres'] +const VALID_DB_NAME = /^[a-zA-Z][a-zA-Z0-9_-]*$/ + +/** + * Why `setup_custom_instance_db` would refuse this name, checked as it is typed. Deliberately + * not exhaustive -- the backend stays the authority, this only catches what the browser + * already knows. Empty is incomplete rather than wrong. + */ +export function instanceDbNameError(name: string, existing: Iterable): string | undefined { + const trimmed = name.trim() + if (!trimmed) return undefined + if (trimmed.length > 63) return 'A database name cannot exceed 63 characters.' + if (!VALID_DB_NAME.test(trimmed)) + return 'Start with a letter, then letters, digits, underscores or hyphens only.' + if (RESERVED_DB_NAMES.includes(trimmed.toLowerCase())) + return `${trimmed} is a reserved PostgreSQL database name.` + if (new Set(existing).has(trimmed)) + return `A database called ${trimmed} already exists on this instance.` + return undefined +} + +const VALID_DATATABLE_NAME = /^[a-zA-Z0-9][a-zA-Z0-9_\-.]*$/ + +/** + * Why `edit_datatable_config` would refuse this name, checked as it is typed because the write + * is the *last* step of the run: by the time the backend rejects it a Supabase project may + * have been billed. `existing` are the names already in the workspace. + */ +export function datatableNameError(name: string, existing: Iterable): string | undefined { + const trimmed = name.trim() + if (!trimmed) return undefined + if (new Set(existing).has(trimmed)) + return `A data table called ${trimmed} already exists in this workspace.` + // `validate_datatable_path_segment` runs first on the backend and rejects `..` outright, + // before the charset check the regex below mirrors. + if (trimmed.includes('..')) return "A data table name cannot contain '..'." + if (!VALID_DATATABLE_NAME.test(trimmed)) + return "Start with a letter or digit, then letters, digits, '_', '-' and '.' only — the name has to survive being synced to a git repository." + return undefined +} + +/** + * What the new resource describes. The fields are the connection; a connection string is a way + * of writing one down, parsed into the fields as it is typed. Reading it back out here instead + * would put every gap in the URI grammar between the user and what gets saved. + */ +export function newResourceParts(state: WizardState): PostgresConnectionParts | undefined { + const fields = state.own.fields + return fields.host.trim() && fields.user.trim() ? fields : undefined +} + +/** + * Those parts as a `postgresql` resource value -- the one shape everything downstream sees, + * so nothing after this point knows which notation produced it. The password is the + * caller's: the literal one when testing before anything is saved, a `$var:` reference once + * it has somewhere to live. + */ +export function postgresResourceValue( + parts: PostgresConnectionParts, + password: string, + advanced: PostgresAdvanced +): Record { + return { + host: parts.host, + user: parts.user, + port: parts.port ?? 5432, + dbname: parts.dbname || 'postgres', + sslmode: parts.sslmode || DEFAULT_SSLMODE, + password, + region: advanced.region, + root_certificate_pem: advanced.root_certificate_pem, + use_iam_auth: advanced.use_iam_auth, + // Omitted rather than sent as false: absent is its own state, and the one every + // resource that predates the flag is in. + ...(advanced.accept_invalid_certs !== undefined + ? { accept_invalid_certs: advanced.accept_invalid_certs } + : {}) + } +} + +/** + * The connection value a branch can be validated against before anything is saved. + * Undefined for branches with nothing to validate yet: creating a Supabase project has no + * database to reach, and an instance database does not exist until setup runs. + */ +export function probeValue(state: WizardState): Record | undefined { + if (state.provider !== 'resource' || !state.own.creating) return undefined + const parts = newResourceParts(state) + return parts ? postgresResourceValue(parts, parts.password ?? '', state.own.advanced) : undefined +} + +/** + * Where the Supabase project will live, for the review step to state plainly. Read off + * the project when it already exists, off what was picked when it is about to be created. + */ +export function supabaseSummary(state: WizardState): { org?: string; region?: string } { + if (state.supabase.mode === 'create') + return { org: state.supabase.org?.name, region: state.supabase.region } + const project = state.supabase.project + return { + // The name when the organization is known, its identifier only as a last resort. + org: state.supabase.org?.name ?? (project ? projectOrg(project) : undefined), + region: project?.region + } +} + +export type RunStepKey = + | 'create_project' + | 'wait_healthy' + | 'save_credentials' + | 'setup_instance' + | 'check' + +/** + * The steps this branch will run, in order. The key drives the runner and the title only + * the display, so rewording a step cannot change what it does. + */ +export function plan(state: WizardState): { key: RunStepKey; title: string }[] { + const path = resourcePathOf(state) + const steps: { key: RunStepKey; title: string }[] = [] + if (state.provider === 'supabase') { + if (state.supabase.mode === 'create') { + steps.push({ + key: 'create_project', + title: `Creating ${state.supabase.projectName.trim()} on Supabase` + }) + steps.push({ key: 'wait_healthy', title: 'Waiting for the database to start' }) + } + steps.push({ key: 'save_credentials', title: `Saving credentials to ${path}` }) + } else if (state.provider === 'instance') { + steps.push({ + key: 'setup_instance', + title: `Setting up ${state.instance.dbName} in the Windmill database` + }) + } else if (state.own.creating) { + steps.push({ key: 'save_credentials', title: `Saving the connection to ${path}` }) + } + steps.push({ key: 'check', title: 'Checking Windmill can store data' }) + return steps +} + +/** The same plan as a checklist, all pending. */ +export function planSteps(state: WizardState): SetupStep[] { + return plan(state).map((s) => ({ title: s.title, status: 'pending' })) +} + +/** A Supabase project this session created, and the path holding its only password. */ +export type CreatedProject = { name: string; path: string } + +export type RunDeps = { + workspace: string + /** Required for the Supabase branch. */ + supabaseToken?: string + /** So the settings page's pool reflects a database this run created. */ + onInstanceDbsChanged?: () => Promise + onProgress: (steps: SetupStep[]) => void + /** Session pooling was asked for but could not be read; a direct host was written. */ + onPoolerUnavailable?: (reason: string) => void + /** + * The Supabase project an earlier attempt in this session created. Minting a second password + * over the first one's variable would lose the only copy of credentials Supabase will not + * repeat, so a run that would do that refuses -- but only once it has seen that the project + * is really there, since the name is also recorded when a create could not be confirmed. + */ + createdProjects: CreatedProject[] + /** + * What earlier attempts in this session wrote, and this one may therefore write over again. + * The pre-flight checks the names are free, but the Supabase branch then spends minutes + * provisioning, and every wizard suggests the same `main` -- so a second admin can take the + * name or the path in between. + */ + claims: Claims + /** Stands in as the mark where the object was written but its timestamp could not be read back. */ + username: string +} + +export type RunResult = { + ok: boolean + report?: TestDataTableConnectionResponse + error?: string + /** + * The workspace config still holds this data table. False when the run never got that far, + * and when a refused instance database was taken back out again -- so the name is free and + * the caller must not claim it. + */ + rowWritten?: boolean + /** A row this run had written is gone again, so a claim on the name has to go with it. */ + rowRolledBack?: boolean + /** + * Every project created this session, each guarding the path holding its only password. + * Supabase never shows that password again, so the variable there is the only copy and no + * later attempt may write over it. + */ + createdProjects: CreatedProject[] + /** What this run holds now, for the next attempt to be given back. */ + claims: Claims +} + +/** + * Why a run will not write at a path that already holds a created project's password. Names + * the path the password is actually at, which is not always the one the wizard is pointing at + * now -- the review step can be edited after a failure. + */ +function createdSecretRefusal(projectName: string, passwordPath: string): string { + return `The password of the Supabase project ${projectName}, which this setup created, is stored at ${passwordPath}. Writing here would replace it and Supabase cannot show that password again. Name the project ${projectName} again to carry on with it, or use a different path.` +} + +async function exists(kind: 'variable' | 'resource', workspace: string, path: string) { + return kind === 'variable' + ? VariableService.existsVariable({ workspace, path }) + : ResourceService.existsResource({ workspace, path }) +} + +/** + * Adds the data table to the workspace config, once everything it points at exists. + * `edit_datatable_config` replaces the whole map, so the rest is read back and sent with + * it. Re-runnable: a second attempt overwrites the entry it wrote. + */ +async function writeRow( + deps: RunDeps, + claims: Claims, + name: string, + database: { resource_type: 'postgresql' | 'instance'; resource_path: string } +): Promise { + const settings = await WorkspaceService.getSettings({ workspace: deps.workspace }) + const datatables: Record = { ...(settings.datatable?.datatables ?? {}) } + // Free when the pre-flight looked, taken by the time we write: repointing it here would + // silently hand another admin's data table a database they never chose. + if ( + datatables[name] && + !stillOurs(claims, 'row', name, datatables[name]?.database?.resource_path) + ) { + throw new Error( + `A data table called ${name} was created while this setup was running. Choose another name and try again.` + ) + } + datatables[name] = { ...(datatables[name] ?? {}), database } + await WorkspaceService.editDataTableConfig({ + workspace: deps.workspace, + requestBody: { settings: { datatables }, renames: [], deleted_datatables: [] } + }) + return claim(claims, 'row', name, database.resource_path) +} + +/** + * `removed` — the row this run wrote is gone. `kept` — the undo could not reach the server, so + * it is still there and the caller has to keep saying so. `foreign` — the name now points + * somewhere this run never wrote, so there is nothing of ours to take back. + */ +type Rollback = 'removed' | 'kept' | 'foreign' + +async function removeRow(deps: RunDeps, claims: Claims, name: string): Promise { + try { + const settings = await WorkspaceService.getSettings({ workspace: deps.workspace }) + const datatables: Record = { ...(settings.datatable?.datatables ?? {}) } + // Only take back the row this run put there. Between writing it and probing it, another + // admin can have pointed the same name somewhere else, and deleting that is worse than + // leaving ours behind. + if (!stillOurs(claims, 'row', name, datatables[name]?.database?.resource_path)) return 'foreign' + delete datatables[name] + // Not `deleted_datatables`: that exists to cascade migration bookkeeping and deployment + // records for a data table that was really in use, and this one never got that far. + await WorkspaceService.editDataTableConfig({ + workspace: deps.workspace, + requestBody: { settings: { datatables }, renames: [], deleted_datatables: [] } + }) + return 'removed' + } catch { + return 'kept' + } +} + +/** + * The read answers both questions at once: whether anything is there, and who last wrote it. + * Replacing this run's own work is required for Try again; replacing anyone else's loses a + * generated Supabase password, which Supabase never shows twice. + */ +async function writeSecret( + deps: RunDeps, + claims: Claims, + path: string, + value: string, + description: string +): Promise { + const held = await secretMark(deps, path) + if (held) { + if (!stillOurs(claims, 'secret', path, held)) throw new Error(pathTakenLate('variable', path)) + await VariableService.updateVariable({ + workspace: deps.workspace, + path, + requestBody: { value, is_secret: true } + }) + } else { + await VariableService.createVariable({ + workspace: deps.workspace, + requestBody: { path, value, is_secret: true, description, is_oauth: false } + }) + } + return claim(claims, 'secret', path, (await secretMark(deps, path)) ?? deps.username) +} + +/** + * A revision, not an author: the same person editing the variable in another tab leaves + * `edited_by` unchanged, and that write is no more ours to discard than a stranger's. + * `undefined` when nothing is there. + */ +async function secretMark(deps: RunDeps, path: string): Promise { + // `decryptSecret` defaults to true, and the handler audit-logs a decryption when it does. + // Only the timestamp is wanted, and it is on the response either way -- asking for the + // plaintext records decrypting a secret nothing reads, including someone else's on the + // retry that is about to refuse it. + const held = await VariableService.getVariable({ + workspace: deps.workspace, + path, + decryptSecret: false + }).catch(() => undefined) + return held ? (held.edited_at ?? held.edited_by ?? '') : undefined +} + +function pathTakenLate(kind: 'variable' | 'resource', path: string): string { + return `A ${kind} was created at ${path} while this setup was running. Choose another path and try again.` +} + +async function writeResource( + deps: RunDeps, + claims: Claims, + path: string, + value: Record, + description: string +): Promise { + const held = await resourceMark(deps, path) + if (held) { + if (!stillOurs(claims, 'resource', path, held)) throw new Error(pathTakenLate('resource', path)) + await ResourceService.updateResource({ + workspace: deps.workspace, + path, + requestBody: { value, description } + }) + } else { + await ResourceService.createResource({ + workspace: deps.workspace, + requestBody: { resource_type: 'postgresql', path, value, description } + }) + } + // Read back rather than claim the username: `created_by` survives an update, so it cannot + // tell an edit by somebody else from no edit at all. `edited_at` moves on every write, which + // is what makes the next attempt able to see one that happened in between. + return claim(claims, 'resource', path, (await resourceMark(deps, path)) ?? deps.username) +} + +/** `undefined` when nothing is there. */ +async function resourceMark(deps: RunDeps, path: string): Promise { + const held = await ResourceService.getResource({ workspace: deps.workspace, path }).catch( + () => undefined + ) + return held ? (held.edited_at ?? held.created_by ?? '') : undefined +} + +/** + * Performs what the wizard collected, reporting each step as it goes. + * + * Every step is safe to re-run, because Try again runs the whole plan a second time: + * each one upserts rather than assuming what it creates is absent. + */ +export async function runSetup(state: WizardState, deps: RunDeps): Promise { + const planned = plan(state) + const steps: SetupStep[] = planned.map((s) => ({ title: s.title, status: 'pending' })) + let index = 0 + const advance = ( + status: 'running' | 'done' | 'failed', + description?: string, + substeps?: SetupStep[] + ) => { + steps[index] = { + ...steps[index], + status, + description, + substeps: substeps ?? steps[index].substeps + } + deps.onProgress([...steps]) + } + let rowWritten = false + let rowRolledBack = false + let claims = deps.claims + let createdProjects: CreatedProject[] = [...deps.createdProjects] + /** Records a created project once, so a second attempt cannot displace the first one's guard. */ + const rememberProject = (name: string, at: string) => { + if (!createdProjects.some((p) => p.path === at)) + createdProjects = [...createdProjects, { name, path: at }] + } + const fail = (message: string): RunResult => { + advance('failed', message) + return { + ok: false, + error: message, + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } + + const path = resourcePathOf(state) + const name = state.review.name.trim() + /** + * An earlier attempt stored a created project's password here. Supabase hands that out once + * and every write upserts, so every route back to this path refuses. Each created project + * guards its own path -- checking only the latest unlocked the earlier one's password. + */ + const guardedHere = deps.createdProjects.find((p) => p.path === path) + const instanceName = state.instance.dbName?.trim() ?? '' + + let project = state.supabase.project + let resourcePath = + state.provider === 'resource' && !state.own.creating ? state.own.resourcePath! : path + + for (; index < planned.length; index++) { + advance('running') + try { + if (planned[index].key === 'create_project') { + // The password is generated here and can never be read back from Supabase, so it + // is written to the secret variable before the project that uses it exists. A run + // that dies right after creation is then still repairable; the reverse order + // would strand a billed project nobody holds the password to. + const wanted = state.supabase.projectName.trim() + const inOrg = (name: string) => (p: SupabaseProject) => + p.name === name && (!state.supabase.org || projectOrg(p) === orgSlug(state.supabase.org)) + const projects = await listSupabaseProjects(deps.supabaseToken!) + const existing = projects.find(inOrg(wanted)) + if (existing) { + if (!(await exists('variable', deps.workspace, path))) { + // A project this same session created is the one case where the password is + // held after all, just not here: the path has been edited since. Saying so + // beats telling someone to reset or delete a project that is working. + const elsewhere = deps.createdProjects.find((p) => p.name === wanted) + if (elsewhere) + return fail( + `The password for ${wanted}, which this setup created, is stored at ${elsewhere.path}, not at ${path}. Set the path back to ${elsewhere.path} to carry on with that project.` + ) + return fail( + `A Supabase project called ${wanted} already exists, but Windmill does not hold its password and Supabase cannot return it. Reset the password in Supabase and connect it as an existing project, or delete the project and retry.` + ) + } + project = existing + } else { + // The project has to still exist for its password to be worth protecting: a name + // recorded from a create that could not be confirmed is a false alarm, and + // refusing on it leaves the session with nothing it can do. Matched by name + // across every organization -- a namesake costs a rename, a miss costs the + // password. + const earlier = guardedHere?.name + if (earlier && projects.some((p) => p.name === earlier)) { + return fail(createdSecretRefusal(earlier, guardedHere!.path)) + } + const password = generateDbPassword() + claims = await writeSecret( + deps, + claims, + path, + password, + `Password for the ${wanted} Supabase database` + ) + try { + project = await createSupabaseProject(deps.supabaseToken!, { + name: wanted, + organizationSlug: orgSlug(state.supabase.org!), + region: state.supabase.region, + dbPass: password + }) + // From here the password in `path` is the only copy of a billed project's + // credentials, and every later write to that path upserts. + rememberProject(wanted, path) + } catch (err) { + // A refusal and a lost response look the same from here, and only one of them + // bills. Ask Supabase which it was: a project that turned up is ours, holds the + // password just written, and is what the rest of the run is for. If even that + // cannot be answered -- an expired token answers nothing -- record the name + // anyway, and let the next attempt's own lookup decide whether it was real. + const appeared = await listSupabaseProjects(deps.supabaseToken!).then( + (after) => after.find(inOrg(wanted)), + () => { + rememberProject(wanted, path) + return undefined + } + ) + if (!appeared) throw err + rememberProject(wanted, path) + project = appeared + } + } + } else if (planned[index].key === 'wait_healthy') { + // Minutes of polling with nothing else to show: hang what Supabase reports off the + // step, so the longest wait in the wizard has something behind its chevron. + project = await waitUntilSupabaseHealthy( + deps.supabaseToken!, + projectRef(project!), + (status) => advance('running', status) + ) + } else if (planned[index].key === 'save_credentials') { + if (state.provider === 'supabase') { + if (state.supabase.mode === 'existing') { + if (guardedHere) return fail(createdSecretRefusal(guardedHere.name, path)) + claims = await writeSecret( + deps, + claims, + path, + state.supabase.password, + `Password for the ${project!.name} Supabase database` + ) + } + const connection = await resolveSupabaseConnection( + deps.supabaseToken!, + project!, + state.supabase.connectionMode + ) + if (connection.mode !== state.supabase.connectionMode) + state.supabase.connectionMode = connection.mode + if (connection.unavailable) deps.onPoolerUnavailable?.(connection.unavailable) + claims = await writeResource( + deps, + claims, + path, + supabaseResourceValue(project!, path, connection), + `Supabase project ${project!.name}` + ) + } else { + if (guardedHere) return fail(createdSecretRefusal(guardedHere.name, path)) + const parts = newResourceParts(state)! + claims = await writeSecret( + deps, + claims, + path, + parts.password ?? '', + `Password for the ${parts.host} database` + ) + claims = await writeResource( + deps, + claims, + path, + postgresResourceValue(parts, `$var:${path}`, state.own.advanced), + `Database for the ${name} data table` + ) + } + } else if (planned[index].key === 'setup_instance') { + // The call reports nothing until it returns, so name the checks it is about to run + // with the first one marked in flight; its answer replaces them when it lands. + // Otherwise the longest step in the wizard is a single line that sits there. + advance('running', undefined, instanceSetupSteps(instanceName, undefined, true)) + const status = await SettingService.setupCustomInstanceDb({ + name: instanceName, + requestBody: { tag: 'datatable' } + }) + await deps.onInstanceDbsChanged?.() + const checks = instanceSetupSteps(instanceName, status, false) + if (!status.success) { + advance('failed', status.error ?? 'Setup failed', checks) + return { + ok: false, + error: status.error ?? 'Setup failed', + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } + advance('running', undefined, checks) + } else if (state.provider === 'instance') { + // An instance data table is probed by name, through the very entry being written + // here, so this is the one branch that cannot check first. A database Windmill + // cannot store data in must not stay in the config, so a refusal takes the row + // back out -- leaving it would also block retrying under the same name. + const database = { resource_type: 'instance' as const, resource_path: instanceName } + claims = await writeRow(deps, claims, name, database) + rowWritten = true + const report = await WorkspaceService.testDataTableConnection({ + workspace: deps.workspace, + datatableName: name + }).catch(async (err) => { + // A probe that never answered leaves the same unusable row behind as one that + // answered no -- an unreachable database or a timeout lands here -- so it takes + // the same way out rather than the bare outer catch. + const rollback = await removeRow(deps, claims, name) + rowRolledBack = rollback === 'removed' + // `foreign` means the name is somebody else's now: our row is not there to + // hand back to the collision checks, and a retry must not write over theirs. + rowWritten = rollback === 'kept' + throw err + }) + if (!report.can_create_table) { + const rollback = await removeRow(deps, claims, name) + rowRolledBack = rollback === 'removed' + rowWritten = rollback === 'kept' + advance('failed', 'The database is reachable but its user cannot create tables.') + return { + ok: false, + report, + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } + advance('done') + return { + ok: true, + report, + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } else { + // Checked through the resource, so nothing is written until the database has proved + // it can hold a data table. + const report = await probeDatatableConnection(deps.workspace, `$res:${resourcePath}`) + if (!report.can_create_table) { + advance('failed', 'The database is reachable but its user cannot create tables.') + return { + ok: false, + report, + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } + claims = await writeRow(deps, claims, name, { + resource_type: 'postgresql', + resource_path: resourcePath + }) + rowWritten = true + advance('done') + return { + ok: true, + report, + rowWritten, + rowRolledBack, + claims, + createdProjects + } + } + advance('done') + } catch (err: any) { + return fail(err?.body ?? err?.message ?? String(err)) + } + } + + return { + ok: true, + rowWritten, + rowRolledBack, + claims, + createdProjects + } +} diff --git a/frontend/src/lib/components/workspaceSettings/datatableProbe.ts b/frontend/src/lib/components/workspaceSettings/datatableProbe.ts new file mode 100644 index 0000000000..38368c4b7f --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/datatableProbe.ts @@ -0,0 +1,108 @@ +/** + * What a database lets the data table's role do, answered by the worker rather than by the API + * server. + * + * It runs as a preview job for the same reason `TestConnection` does: a job goes through the + * worker's Postgres executor, so IAM and Azure workload identity authenticate as the worker + * will when a real query runs. A connection opened from the API server proves something about + * the API server, which is a different machine with a different identity. + * + * Postgres composes the suggested statements itself through `format('%I')`, so identifier + * quoting stays where it is already implemented. + */ + +import { JobService, type Preview, type TestDataTableConnectionResponse } from '$lib/gen' +import { tryEvery } from '$lib/utils' + +const PRIVILEGES = `SELECT current_user AS usr, + current_schema() AS sch, + has_schema_privilege(current_schema(), 'CREATE') AS can_create_table, + has_database_privilege(current_database(), 'CREATE') AS can_create_schema, + to_regclass('_wm_migrations') IS NOT NULL AS has_migrations_table, + -- A role whose search_path names no valid schema has a NULL current_schema(), and + -- format('%I', NULL) raises rather than returning NULL, which would fail the whole + -- query on the one case fix_search_path exists to report. + CASE WHEN current_schema() IS NULL THEN NULL + ELSE format('GRANT CREATE ON SCHEMA %I TO %I', current_schema(), current_user) + END AS grant_schema, + format('GRANT CREATE ON DATABASE %I TO %I', current_database(), current_user) AS grant_database, + format('ALTER ROLE %I SET search_path = public', current_user) AS fix_search_path` + +type Row = { + usr?: string + sch?: string | null + can_create_table?: boolean + can_create_schema?: boolean + has_migrations_table?: boolean + grant_schema?: string | null + grant_database?: string | null + fix_search_path?: string | null +} + +/** + * `database` is whatever a Postgres step takes: the resource value, or a `$res:` path the + * worker resolves. Throws with the database's own message when the query fails, and after + * `timeout` when no worker picks the job up. + */ +export async function probeDatatableConnection( + workspace: string, + database: Record | string, + // Longer than the 20s the worker allows its own Postgres connect, or a host that accepts + // the connection and never answers -- a firewall with no rule for the workers, which this + // check exists to catch -- is cancelled first and reported as a missing worker. + timeout = 30000 +): Promise { + const job = await JobService.runScriptPreview({ + workspace, + requestBody: { + path: 'testConnection: datatable', + language: 'postgresql' as Preview['language'], + content: PRIVILEGES, + args: { database } + } + }) + + let completed: Awaited> | undefined = undefined + await tryEvery({ + tryCode: async () => { + completed = await JobService.getCompletedJob({ workspace, id: job }) + }, + timeoutCode: async () => { + await JobService.cancelQueuedJob({ + workspace, + id: job, + requestBody: { reason: 'The connection check did not start' } + }).catch(() => {}) + }, + interval: 500, + timeout + }) + + if (!completed) { + throw new Error( + 'The connection check did not run. Is a worker listening to the postgresql tag available?' + ) + } + const done = completed as { success: boolean; result?: any } + if (!done.success) { + throw new Error(done.result?.error?.message ?? 'Could not connect to the database') + } + + const row: Row = (Array.isArray(done.result) ? done.result[0] : done.result) ?? {} + // Suggested only where the privilege is actually missing; Postgres returns NULL for a + // statement it could not name, which is the case where no grant would help anyway. + const suggested_grants = [ + row.can_create_table ? undefined : (row.grant_schema ?? undefined), + row.can_create_schema ? undefined : (row.grant_database ?? undefined) + ].filter((s): s is string => !!s) + + return { + user: row.usr ?? '', + schema: row.sch ?? null, + can_create_table: !!row.can_create_table, + can_create_schema: !!row.can_create_schema, + migrations_table_exists: !!row.has_migrations_table, + suggested_grants, + suggested_search_path: row.sch ? undefined : (row.fix_search_path ?? undefined) + } +} diff --git a/frontend/src/lib/components/workspaceSettings/instanceDbSteps.ts b/frontend/src/lib/components/workspaceSettings/instanceDbSteps.ts new file mode 100644 index 0000000000..dd465626f4 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/instanceDbSteps.ts @@ -0,0 +1,89 @@ +import type { CustomInstanceDb } from '$lib/gen' +import { runningFrom, type SetupStep } from '../wizards/SetupChecklist.svelte' + +/** + * The same checks as [`instanceDbSteps`], in the vocabulary the wizard's checklist speaks. + * Nothing is reported until the call returns, so an unreported step is either the failure + * (when the call errored) or simply not reached yet. + */ +export function instanceSetupSteps( + dbname: string, + status: CustomInstanceDb | undefined, + running: boolean +): SetupStep[] { + let firstUnreported = true + const steps = instanceDbSteps(dbname, status).map((step): SetupStep => { + if (step.status === 'OK') return { ...step, status: 'done' } + if (step.status === 'FAIL') return { ...step, status: 'failed' } + if (step.status === 'SKIP') return { ...step, status: 'skipped' } + const failed = firstUnreported && !!status?.error + firstUnreported = false + return { ...step, status: failed ? 'failed' : 'pending' } + }) + return runningFrom(steps, running) +} + +/** + * The checks `setup_custom_instance_db` reports, in the order it runs them. Shared so the + * setup modal and the data table wizard describe the same failure the same way. + */ +export function instanceDbSteps(dbname: string, status: CustomInstanceDb | undefined) { + return [ + { + title: 'Super admin required', + status: status?.logs.super_admin, + description: + 'You need to be a super admin to create a new database in the Windmill PostgreSQL instance' + }, + { + title: 'Retrieve and parse database credentials', + status: status?.logs.database_credentials, + description: + 'Windmill uses the DATABASE_URL or DATABASE_URL_FILE environment variable to connect to the PostgreSQL instance. Make sure it is correctly set' + }, + { + title: 'Database name is valid', + status: status?.logs.valid_dbname, + description: + 'The database name must be alphanumeric (underscores and hyphens allowed) and cannot be named the same as the Windmill database (usually "windmill")' + }, + { + title: + 'Create database' + + (status?.logs.created_database === 'SKIP' ? ' (already exists, skipped)' : ''), + status: status?.logs.created_database, + description: `In the Windmill PostgreSQL instance, run: CREATE DATABASE "${dbname}".` + }, + { + title: `Connect to the ${dbname} database`, + status: status?.logs.db_connect, + description: + "Connect to the newly created database with the default admin user (the one in DATABASE_URL, usually 'postgres') to run the next commands" + }, + { + title: 'Grant permissions to custom_instance_user', + status: status?.logs.grant_permissions, + description: + 'Gives custom_instance_user the required permissions to use the database. custom_instance_user is already created during a migration and has an auto-generated password stored in global_settings.custom_instance_pg_databases.user_pwd. These are the commands : \n\n' + + `GRANT CONNECT ON DATABASE "${dbname}" TO custom_instance_user;\n` + + 'GRANT USAGE ON SCHEMA public TO custom_instance_user;\n' + + 'GRANT CREATE ON SCHEMA public TO custom_instance_user;\n' + + `GRANT CREATE ON DATABASE "${dbname}" TO custom_instance_user;\n` + + 'ALTER DEFAULT PRIVILEGES IN SCHEMA public \n' + + ' GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES\n TO custom_instance_user;\n' + + 'ALTER ROLE custom_instance_user CREATEROLE;' + }, + { + title: 'Grant replication to custom_instance_replication_user', + status: status?.logs.replication_user, + description: + 'Postgres triggers on custom-instance datatables connect as custom_instance_replication_user, whose password is stored in global_settings.custom_instance_replication_pwd. The role is cluster-wide, so it is created on the Windmill PostgreSQL instance rather than on this database : \n\n' + + 'ALTER ROLE custom_instance_replication_user REPLICATION;\n' + + 'GRANT custom_instance_user TO custom_instance_replication_user;\n\n' + + 'Setting REPLICATION requires a superuser on PostgreSQL 15 and older. Managed instances never grant one, so on AWS RDS Windmill falls back to GRANT rds_replication TO custom_instance_replication_user. The database stays usable for datatables if this step fails, but postgres triggers on them do not.' + + (status?.logs.replication_user_error + ? `\n\nError: ${status.logs.replication_user_error}` + : '') + } + ] +} diff --git a/frontend/src/lib/components/workspaceSettings/setupClaims.test.ts b/frontend/src/lib/components/workspaceSettings/setupClaims.test.ts new file mode 100644 index 0000000000..766d2b5281 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/setupClaims.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from 'vitest' +import { + anythingClaimed, + claim, + claimsFromJSON, + claimsToJSON, + noClaims, + release, + stillOurs +} from './setupClaims' + +describe('stillOurs', () => { + it('honours a claim whose object has not moved', () => { + const claims = claim(noClaims, 'secret', 'f/team/db', 'alice') + expect(stillOurs(claims, 'secret', 'f/team/db', 'alice')).toBe(true) + }) + + it('refuses when the object was last written by somebody else', () => { + const claims = claim(noClaims, 'secret', 'f/team/db', 'alice') + expect(stillOurs(claims, 'secret', 'f/team/db', 'bob')).toBe(false) + }) + + // Deleted and recreated between two attempts: something is there, it is not ours. + it('refuses when the object is gone', () => { + const claims = claim(noClaims, 'resource', 'f/team/db', 'alice') + expect(stillOurs(claims, 'resource', 'f/team/db', undefined)).toBe(false) + }) + + it('refuses a path this run never claimed', () => { + expect(stillOurs(noClaims, 'secret', 'f/team/db', 'alice')).toBe(false) + }) + + // The secret and the resource are separate objects at one path. + it('keeps the two objects at one path apart', () => { + const claims = claim(noClaims, 'secret', 'f/team/db', 'alice') + expect(stillOurs(claims, 'secret', 'f/team/db', 'alice')).toBe(true) + expect(stillOurs(claims, 'resource', 'f/team/db', 'alice')).toBe(false) + }) + + it('refuses a row repointed since it was written', () => { + const claims = claim(noClaims, 'row', 'main', 'f/team/db') + expect(stillOurs(claims, 'row', 'main', 'f/team/db')).toBe(true) + expect(stillOurs(claims, 'row', 'main', 'someone-elses-db')).toBe(false) + }) +}) + +describe('claims as a set', () => { + it('replaces the mark when the same object is claimed again', () => { + let claims = claim(noClaims, 'row', 'main', 'first') + claims = claim(claims, 'row', 'main', 'second') + expect(claims).toHaveLength(1) + expect(stillOurs(claims, 'row', 'main', 'second')).toBe(true) + }) + + it('gives a claim up so the name is free again', () => { + const claims = release(claim(noClaims, 'row', 'main', 'x'), 'row', 'main') + expect(anythingClaimed(claims)).toBe(false) + }) + + it('carries every claim across the redirect, whatever kinds are held', () => { + let claims = claim(noClaims, 'secret', 'f/team/db', 'alice') + claims = claim(claims, 'resource', 'f/team/db', 'alice') + claims = claim(claims, 'row', 'main', 'f/team/db') + const restored = claimsFromJSON(JSON.parse(JSON.stringify(claimsToJSON(claims)))) + expect(restored).toEqual(claims) + }) + + it('survives a payload that is not claims at all', () => { + expect(claimsFromJSON(undefined)).toEqual(noClaims) + expect(claimsFromJSON([{ kind: 'nonsense', path: 'p', mark: 'm' }])).toEqual(noClaims) + }) +}) diff --git a/frontend/src/lib/components/workspaceSettings/setupClaims.ts b/frontend/src/lib/components/workspaceSettings/setupClaims.ts new file mode 100644 index 0000000000..48f844d6f9 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/setupClaims.ts @@ -0,0 +1,87 @@ +/** + * What a setup run created, and whether it is still there. + * + * Try again re-runs the whole plan, so every write meets what the previous attempt left behind + * and has to answer one question: is the thing at this path the thing I made? Writing over its + * own work is required; writing over another admin's destroys a password Supabase shows once. + * + * A claim therefore carries a **mark** — the discriminator to compare against the object as it + * is now, rather than trusting that whatever sits at a remembered path is ours. + * + * Values, not runes, so the ownership matrix is testable without mounting a component. + */ + +export type ClaimKind = 'secret' | 'resource' | 'row' + +export type Claim = { + kind: ClaimKind + path: string + /** + * Compared against the live object. It has to move whenever anyone else writes: `edited_at` + * for a secret and a resource — an author survives an edit and so cannot tell one from no + * edit at all — and the target for a row. + */ + mark: string +} + +export type Claims = readonly Claim[] + +export const noClaims: Claims = [] + +function sameObject(a: Claim, kind: ClaimKind, path: string): boolean { + return a.kind === kind && a.path === path +} + +/** Re-claiming an object replaces its mark. */ +export function claim(claims: Claims, kind: ClaimKind, path: string, mark: string): Claims { + return [...claims.filter((c) => !sameObject(c, kind, path)), { kind, path, mark }] +} + +export function claimOf(claims: Claims, kind: ClaimKind, path: string): Claim | undefined { + return claims.find((c) => sameObject(c, kind, path)) +} + +/** Given up when a run takes its own object back out, so the path is free again. */ +export function release(claims: Claims, kind: ClaimKind, path: string): Claims { + return claims.filter((c) => !sameObject(c, kind, path)) +} + +/** + * Whether the object now at `path` is the one this run claimed. `observed` is the mark read back + * from the live object; `undefined` means nothing is there. + */ +export function stillOurs( + claims: Claims, + kind: ClaimKind, + path: string, + observed: string | undefined +): boolean { + const held = claimOf(claims, kind, path) + return !!held && observed !== undefined && held.mark === observed +} + +export function anythingClaimed(claims: Claims): boolean { + return claims.length > 0 +} + +/** + * Carried across the full-page redirect the blocked-popup Supabase leg falls back to. No secret + * travels: a mark is a timestamp or a resource path. + */ +export function claimsToJSON(claims: Claims): Claim[] { + return [...claims] +} + +const KINDS: ClaimKind[] = ['secret', 'resource', 'row'] + +export function claimsFromJSON(value: unknown): Claims { + if (!Array.isArray(value)) return noClaims + return value.filter( + (c): c is Claim => + !!c && + typeof c === 'object' && + typeof (c as Claim).path === 'string' && + typeof (c as Claim).mark === 'string' && + KINDS.includes((c as Claim).kind) + ) +} diff --git a/frontend/src/lib/components/workspaceSettings/supabaseOauth.svelte.ts b/frontend/src/lib/components/workspaceSettings/supabaseOauth.svelte.ts new file mode 100644 index 0000000000..3e11ec2b9d --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/supabaseOauth.svelte.ts @@ -0,0 +1,107 @@ +import { fromStore } from 'svelte/store' +import { base } from '$lib/base' +import { oauthStore } from '$lib/stores' + +const OAUTH_WINDOW = 'windmill_supabase_oauth' +const CONNECT_URL = `${base}/api/oauth/connect/supabase_wizard` + +/** + * The Supabase authorization leg, driven from a popup. + * + * A full-page redirect unmounts whatever opened it, so a user who stops to create a Supabase + * account lands on their dashboard with nothing left pointing back. Keeping the flow in a + * popup keeps the host on screen, and keeps the window ours to steer: after they sign up we + * send the same popup back through the connect endpoint and consent follows. + */ +export function useSupabaseOauth( + opts: { + onPopupBlocked?: () => void + /** + * Where popups are blocked, navigate this tab instead of opening a new one. Only for + * hosts that can be resumed afterwards -- a caller whose state dies with the page (a + * half-filled form) must leave this off and keep the user where they are. + */ + redirectIfBlocked?: boolean + /** Even the new tab was refused, so the caller has to say so rather than sit loading. */ + onFallbackBlocked?: () => void + /** The window went away without authorizing; the caller can drop its own waiting state. */ + onAbandoned?: () => void + /** + * Authorization came back and the token is in the store. Reported like the failures + * above so a caller does not have to watch `authed` to find out. Fires on any successful + * authorization, this caller's or another's -- every instance listens on the same window + * -- so a caller that acts on it has to know it was the one waiting. + */ + onAuthed?: () => void + } = {} +) { + const oauth = fromStore(oauthStore) + let pending = $state(false) + let win: Window | null = null + let abandonWatch: ReturnType | undefined = undefined + + $effect(() => { + function onMessage(e: MessageEvent) { + if (e.origin !== window.location.origin || e.data?.type !== 'supabase_oauth') return + oauthStore.set(e.data.res) + pending = false + clearInterval(abandonWatch) + win?.close() + opts.onAuthed?.() + } + window.addEventListener('message', onMessage) + return () => { + window.removeEventListener('message', onMessage) + clearInterval(abandonWatch) + } + }) + + /** + * Nothing arrives if the user closes the window, denies consent, or wanders off to create + * an account first -- which is a link this flow deliberately offers. Watch for the window + * going away, so the button comes back instead of staying disabled until a page reload. + */ + function watchForAbandon() { + clearInterval(abandonWatch) + abandonWatch = setInterval(() => { + if (!win || win.closed) { + clearInterval(abandonWatch) + pending = false + opts.onAbandoned?.() + } + }, 500) + } + + return { + get token(): string | undefined { + return oauth.current?.access_token + }, + get authed(): boolean { + return !!oauth.current?.access_token + }, + get pending(): boolean { + return pending + }, + /** Opens (or re-points) the popup, falling back to a new tab where popups are blocked. */ + connect() { + win = window.open(CONNECT_URL, OAUTH_WINDOW, 'width=600,height=820') + if (!win) { + opts.onPopupBlocked?.() + if (opts.redirectIfBlocked) { + window.location.href = CONNECT_URL + return + } + // No `noopener`: the callback hands the token back through `window.opener`, and + // severing that is what would leave the host waiting forever. The URL is our own + // origin, so there is nothing to protect against here. + win = window.open(CONNECT_URL, '_blank') + if (!win) { + opts.onFallbackBlocked?.() + return + } + } + pending = true + watchForAbandon() + } + } +} diff --git a/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts b/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts new file mode 100644 index 0000000000..5040b5ab38 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts @@ -0,0 +1,250 @@ +/** + * Supabase Management API calls, proxied through Windmill's backend. + * + * The Management API sends no access-control-allow-origin, so the browser cannot call it + * directly -- every request below goes through /api/oauth/*, which forwards the user's OAuth + * access token. + */ + +import { DEFAULT_SSLMODE } from '$lib/utils/postgresConnectionString' +import { base } from '$lib/base' +import { oauthStore } from '$lib/stores' +import { get } from 'svelte/store' + +export type SupabaseOrg = { id: string; slug?: string; name: string } + +export type SupabaseProject = { + /** `id` is Supabase's deprecated spelling of `ref`; both are sent today. */ + id?: string + ref?: string + name: string + region: string + status?: string + organization_slug?: string + organization_id?: string + database?: { host: string } +} + +/** One Supavisor endpoint of a project. A project has one per mode and replica. */ +export type SupabasePooler = { + database_type: 'PRIMARY' | 'READ_REPLICA' + pool_mode: 'transaction' | 'session' + db_user: string + db_host: string + db_port: number + db_name: string +} + +export type SupabaseConnectionMode = 'session' | 'direct' + +/** Supabase deprecated `id` in favour of `ref`, and still sends both. */ +export function projectRef(project: SupabaseProject): string { + return project.ref ?? project.id ?? '' +} + +export function projectOrg(project: SupabaseProject): string | undefined { + return project.organization_slug ?? project.organization_id +} + +/** Region codes accepted by region_selection, with the names Supabase shows for them. */ +export const SUPABASE_REGIONS: { code: string; label: string }[] = [ + { code: 'us-east-1', label: 'East US (N. Virginia)' }, + { code: 'us-west-1', label: 'West US (N. California)' }, + { code: 'eu-central-1', label: 'Central EU (Frankfurt)' }, + { code: 'eu-west-1', label: 'West EU (Ireland)' }, + { code: 'eu-west-3', label: 'West EU (Paris)' }, + { code: 'ap-southeast-1', label: 'Southeast Asia (Singapore)' }, + { code: 'ap-northeast-1', label: 'Northeast Asia (Tokyo)' } +] + +export const DEFAULT_SUPABASE_REGION = 'eu-central-1' + +function headers(token: string): HeadersInit { + return { 'Content-Type': 'application/json', 'X-Supabase-Token': token } +} + +async function unwrap(res: Response, what: string): Promise { + if (!res.ok) { + // Supabase access tokens are short-lived while `oauthStore` lasts as long as the tab, so + // a stale one otherwise leaves every caller "authorized" and unable to reach the button + // that would fix it. Forgetting it here is what puts Connect back on screen. + if (res.status === 401) oauthStore.set(undefined) + const body = await res.text() + throw new Error(`${what}: ${supabaseErrorMessage(body) || res.statusText}`) + } + return res.json() +} + +/** + * Supabase answers with `{ message }` or `{ error }` and occasionally plain text. + * Surfacing the raw body puts a JSON blob in front of the user, so unwrap it to + * the sentence inside. + */ +export function supabaseErrorMessage(body: string): string { + try { + const parsed = JSON.parse(body) + return parsed?.message ?? parsed?.error ?? parsed?.msg ?? body + } catch { + return body + } +} + +export async function listSupabaseOrgs(token: string): Promise { + const res = await fetch(`${base}/api/oauth/list_supabase_orgs`, { headers: headers(token) }) + return unwrap(res, 'Could not list your Supabase organizations') +} + +export async function listSupabaseProjects(token: string): Promise { + const res = await fetch(`${base}/api/oauth/list_supabase`, { headers: headers(token) }) + return unwrap(res, 'Could not list your Supabase projects') +} + +/** Plan of one organization, which the list endpoint does not carry. */ +export async function getSupabaseOrgPlan(token: string, slug: string): Promise { + try { + const res = await fetch(`${base}/api/oauth/get_supabase_org/${slug}`, { + headers: headers(token) + }) + if (!res.ok) return undefined + return (await res.json())?.plan + } catch { + return undefined + } +} + +/** organization_slug is what create takes; older payloads only carry an id. */ +export function orgSlug(org: SupabaseOrg): string { + return org.slug ?? org.id +} + +/** + * Supabase never lets a database password be read back, so the only way to know it is to be + * the one who set it: db_pass is an input to project creation. + */ +export function generateDbPassword(): string { + const charset = 'abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789' + const values = new Uint32Array(32) + crypto.getRandomValues(values) + return Array.from(values, (v) => charset[v % charset.length]).join('') +} + +export async function createSupabaseProject( + token: string, + args: { name: string; organizationSlug: string; region: string; dbPass: string } +): Promise { + const res = await fetch(`${base}/api/oauth/create_supabase_project`, { + method: 'POST', + headers: headers(token), + body: JSON.stringify({ + name: args.name, + organization_slug: args.organizationSlug, + db_pass: args.dbPass, + // region_selection is { type: 'specific' | 'smartGroup', code }. Neither the published + // docs nor the OpenAPI spec describe it correctly (they give `kind`/`region` and + // `primary`) -- this shape comes from the API's own validation errors, so do not + // "correct" it against the documentation. + region_selection: { type: 'specific', code: args.region } + }) + }) + return unwrap(res, 'Supabase refused to create the project') +} + +/** + * Creation returns immediately with the project still coming up, so the pooler is not + * reachable yet. Poll until Supabase reports it healthy before trying to connect. + */ +export async function waitUntilSupabaseHealthy( + token: string, + projectId: string, + onStatus?: (status: string | undefined) => void, + attempts = 60 +): Promise { + for (let i = 0; i < attempts; i++) { + await new Promise((r) => setTimeout(r, 5000)) + let list: SupabaseProject[] + try { + list = await listSupabaseProjects(token) + } catch (err) { + // A transient failure is worth another poll; an expired token is not -- retrying it + // burns five minutes and then reports a timeout, which names the wrong problem. + if (!get(oauthStore)?.access_token) throw err + continue + } + const project = list?.find?.((p) => projectRef(p) === projectId) + if (project?.status === 'ACTIVE_HEALTHY') return project + onStatus?.(project?.status) + } + throw new Error('Timed out waiting for the project to become reachable') +} + +/** + * The session-mode Supavisor endpoint of the project's primary database. + * + * Which pooler a project sits behind is assigned by Supabase, not derived from its + * region: constructing `aws-0-.pooler.supabase.com` is wrong for every project + * that landed on another one, and the resulting resource never connects. + */ +export async function getSupabasePooler(token: string, projectId: string): Promise { + const res = await fetch(`${base}/api/oauth/get_supabase_pooler/${projectId}`, { + headers: headers(token) + }) + const configs: SupabasePooler[] = await unwrap(res, 'Could not read the connection details') + const primary = configs.filter((c) => c.database_type === 'PRIMARY') + const pooler = primary.find((c) => c.pool_mode === 'session') ?? primary[0] ?? configs[0] + if (!pooler) throw new Error('Supabase returned no connection details for this project') + return pooler +} + +export type SupabaseConnection = { + mode: SupabaseConnectionMode + pooler?: SupabasePooler + /** Why session pooling was asked for and not used. Absent when nothing was given up. */ + unavailable?: string +} + +/** + * The endpoint a project should be reached through, degrading rather than failing. Reading the + * pooler config needs the `database_pooling_config_read` scope, which an instance's OAuth app + * may not have. A direct connection still works where the workers have IPv6, so fall back to + * it and say so. + */ +export async function resolveSupabaseConnection( + token: string, + project: SupabaseProject, + mode: SupabaseConnectionMode +): Promise { + if (mode !== 'session') return { mode } + try { + return { mode, pooler: await getSupabasePooler(token, projectRef(project)) } + } catch (err) { + return { mode: 'direct', unavailable: err instanceof Error ? err.message : String(err) } + } +} + +/** The resource value for a project, given the endpoint it should connect through. */ +export function supabaseResourceValue( + project: SupabaseProject, + passwordVarPath: string, + connection: { mode: SupabaseConnectionMode; pooler?: SupabasePooler } +) { + const direct = connection.mode === 'direct' || !connection.pooler + return { + host: direct + ? (project.database?.host ?? `db.${projectRef(project)}.supabase.co`) + : connection.pooler!.db_host, + user: direct ? 'postgres' : connection.pooler!.db_user, + port: direct ? 5432 : connection.pooler!.db_port, + dbname: direct ? 'postgres' : connection.pooler!.db_name, + // Supabase terminates TLS on every endpoint it hands out, and this connection carries a + // generated password, so there is no reason to leave a plaintext fallback open. + sslmode: DEFAULT_SSLMODE, + password: `$var:${passwordVarPath}`, + // Resource forms fill in every unset property from the schema as soon as they render, + // so a postgresql resource saved without these comes up already modified -- and saves a + // draft -- the first time anyone opens it. Write them here so opening one is a no-op. + // (accept_invalid_certs renders conditionally and is not seeded, so it stays out.) + region: '', + root_certificate_pem: '', + use_iam_auth: false + } +} diff --git a/frontend/src/lib/components/workspaceSettings/utils.svelte.ts b/frontend/src/lib/components/workspaceSettings/utils.svelte.ts index 6925be9c04..55dc531182 100644 --- a/frontend/src/lib/components/workspaceSettings/utils.svelte.ts +++ b/frontend/src/lib/components/workspaceSettings/utils.svelte.ts @@ -1,8 +1,20 @@ import { isCloudHosted } from '$lib/cloud' import { superadmin } from '$lib/stores' +import { getLocalSetting } from '$lib/utils' import { derived } from 'svelte/store' +/** + * Opt-in for the data table setup wizard while it is being tested. Browser-local and read + * once per page: `localStorage.setItem('dataTableWizard', 'true')`, then reload. With it + * off, adding a data table falls back to the inline row in the settings table. + */ +export const DATATABLE_WIZARD_SETTING_NAME = 'dataTableWizard' + +export function isDataTableWizardEnabled(): boolean { + return getLocalSetting(DATATABLE_WIZARD_SETTING_NAME) === 'true' +} + export let isCustomInstanceDbEnabled = derived( [superadmin], ([superadmin_]) => superadmin_ && !isCloudHosted() diff --git a/frontend/src/lib/components/workspaceSettings/wizardParking.ts b/frontend/src/lib/components/workspaceSettings/wizardParking.ts new file mode 100644 index 0000000000..37d031fa3d --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/wizardParking.ts @@ -0,0 +1,63 @@ +/** + * Where popups are blocked the Supabase leg falls back to a full-page redirect, which + * unmounts the wizard. What the user had chosen is parked here and picked back up by the + * settings page when Supabase sends them home. + * + * Kept out of the wizard component so the OAuth callback route can ask whether anything is + * parked without pulling the whole wizard into that page's bundle. + */ + +import type { SupabaseConnectionMode, SupabaseOrg, SupabaseProject } from './supabaseProvisioning' +import type { Claim } from './setupClaims' +import type { CreatedProject } from './addDataTableModel' + +const RESUME_KEY = 'datatable_wizard_resume' + +export type WizardResume = { + name: string + region: string + projectName: string + /** + * What the interrupted run had already created. Without these the resumed run meets its + * own secret variable and resource as somebody else's and refuses to write over them, + * which strands the Supabase project it just paid for. No secret is parked -- these are + * paths, and the password they name is already in the workspace. + */ + resourcePath?: string + /** Everything the run holds, serialised whole so a newly added kind cannot be left behind. */ + claims?: Claim[] + /** Every project created before the redirect, each still guarding its password's path. */ + createdProjects?: CreatedProject[] + /** + * Which side of the step-2 toggle the run was on, and where it was pointed. A run that + * died mid-create otherwise comes back on `existing`, is asked for the password it + * generated and never showed anyone, and looks for its project in whichever organization + * happens to be first. + */ + mode?: 'existing' | 'create' + org?: SupabaseOrg + /** The project that was picked. Without it a resume selects the first in the list, which is + * a different database from the one whose password the user had already typed. */ + project?: SupabaseProject + connectionMode?: SupabaseConnectionMode +} + +/** True while a wizard run is waiting on the Supabase redirect to come back. */ +export function hasParkedWizard(): boolean { + return sessionStorage.getItem(RESUME_KEY) != null +} + +export function parkWizard(state: WizardResume) { + sessionStorage.setItem(RESUME_KEY, JSON.stringify(state)) +} + +export function takeParkedWizard(): WizardResume | undefined { + const raw = sessionStorage.getItem(RESUME_KEY) + sessionStorage.removeItem(RESUME_KEY) + if (!raw) return undefined + try { + return JSON.parse(raw) + } catch { + return undefined + } +} diff --git a/frontend/src/lib/utils/postgresConnectionString.test.ts b/frontend/src/lib/utils/postgresConnectionString.test.ts new file mode 100644 index 0000000000..ae5fc19195 --- /dev/null +++ b/frontend/src/lib/utils/postgresConnectionString.test.ts @@ -0,0 +1,186 @@ +import { describe, expect, it } from 'vitest' +import { + composePostgresConnectionString, + connectionParamRefusal, + parsePostgresConnectionString, + unsupportedConnectionParam +} from './postgresConnectionString' + +// Two callers depend on this producing the same resource value from the same string: +// the resource form's "From connection string", and the data table wizard. +describe('parsePostgresConnectionString', () => { + it('reads every part of a full URI', () => { + expect( + parsePostgresConnectionString('postgres://u:p@db.example.com:6543/mydb?sslmode=require') + ).toEqual({ + user: 'u', + password: 'p', + host: 'db.example.com', + port: 6543, + dbname: 'mydb', + sslmode: 'require' + }) + }) + + it('leaves optional parts undefined rather than empty', () => { + expect(parsePostgresConnectionString('postgresql://u@host/')).toEqual({ + user: 'u', + password: undefined, + host: 'host', + port: undefined, + dbname: undefined, + sslmode: undefined + }) + }) + + it('returns undefined for anything that is not a postgres URI', () => { + expect(parsePostgresConnectionString('mysql://u:p@host/db')).toBeUndefined() + expect(parsePostgresConnectionString('')).toBeUndefined() + }) + + // Verified against psql: `postgres://role:p%40ss@host/db` authenticates as `p@ss`, and an + // unencoded `@` puts the rest of the password in libpq's host too. Reading these any other + // way would make the same string mean something here that it means nowhere else. + it('decodes percent escapes in credentials, as libpq does', () => { + expect(parsePostgresConnectionString('postgres://u:p%40ss@host/db')?.password).toBe('p@ss') + expect(parsePostgresConnectionString('postgres://u%40corp:p@host/db')?.user).toBe('u@corp') + }) +}) + +// The wizard offers the same connection as a string or as fields and switches between them +// by composing and reparsing. A password holding a character the URI reserves is the case +// that breaks silently: it comes back wrong rather than failing to parse. +describe('composePostgresConnectionString', () => { + // `prefer` is libpq's default, so it is the one a composer is tempted to leave out -- and + // the one that silently becomes `require` when the wizard reparses the string and falls + // back to its own default. It is a weaker TLS setting chosen on purpose; it has to survive. + it('keeps an explicit prefer through the round trip', () => { + const parts = { user: 'u', host: 'h', port: undefined, dbname: 'db', sslmode: 'prefer' } + const composed = composePostgresConnectionString(parts) + expect(composed).toContain('sslmode=prefer') + expect(parsePostgresConnectionString(composed)?.sslmode).toBe('prefer') + }) + + // The wizard composes this from fields, so a database name holding a character the URI + // reserves has to survive the toggle. `?` is the one that truncates silently: the parser + // reads everything after it as the query string. + it('round-trips a database name holding reserved characters', () => { + const parts = { user: 'u', host: 'h', dbname: 'sales?archive', sslmode: 'require' } + expect(parsePostgresConnectionString(composePostgresConnectionString(parts))?.dbname).toBe( + 'sales?archive' + ) + }) + + // A literal IPv6 address is all colons, so the URI brackets it and the resource stores it + // bare. Both halves have to agree or the wizard's own toggle produces a string it rejects. + it('brackets an IPv6 host and reads it back bare', () => { + const composed = composePostgresConnectionString({ + user: 'u', + host: '2001:db8::1', + port: 5432, + dbname: 'db' + }) + expect(composed).toContain('@[2001:db8::1]:5432/') + expect(parsePostgresConnectionString(composed)?.host).toBe('2001:db8::1') + expect(parsePostgresConnectionString('postgres://u:p@[2001:db8::1]/db')?.host).toBe( + '2001:db8::1' + ) + }) + + it('round-trips through parse', () => { + const parts = { + user: 'u@corp', + password: 'p@ss/w:rd', + host: 'db.example.com', + port: 6543, + dbname: 'mydb', + sslmode: 'require' + } + expect(parsePostgresConnectionString(composePostgresConnectionString(parts))).toEqual(parts) + }) +}) + +// A parameter the resource has no field for is not a preference that can be dropped: it decides +// where data lands, or how the connection is verified. The check is an allowlist because the +// dangerous ones are precisely the ones a hand-written denylist would miss. +describe('unsupportedConnectionParam', () => { + it('names a parameter that decides where data lands', () => { + expect(unsupportedConnectionParam('postgres://u:p@h/db?options=-csearch_path%3Dtenant')).toBe( + 'options' + ) + expect(unsupportedConnectionParam('postgres://u:p@h/db?search_path=tenant')).toBe('search_path') + }) + + // Dropping these saves a *weaker* connection than the one pasted. + it('names a parameter that decides how the connection is secured or routed', () => { + expect(unsupportedConnectionParam('postgres://u:p@h/db?sslrootcert=system')).toBe('sslrootcert') + expect(unsupportedConnectionParam('postgres://u:p@h/db?channel_binding=require')).toBe( + 'channel_binding' + ) + expect(unsupportedConnectionParam('postgres://u:p@h/db?target_session_attrs=read-write')).toBe( + 'target_session_attrs' + ) + }) + + // The backend applies its own connect timeout, so accepting one and dropping it would make + // `connect_timeout=1` mean a twenty-second wait. + it('names a parameter whose behaviour the backend overrides', () => { + expect(unsupportedConnectionParam('postgres://u:p@h/db?connect_timeout=1')).toBe( + 'connect_timeout' + ) + }) + + // `sslmode=` also occurs inside another parameter's value, and reading it there turns TLS + // off behind a string that never asked for it -- past the allowlist, since the parameter + // actually carrying it is one we accept. + it('reads sslmode by name, not from anywhere it appears in the query', () => { + const disguised = 'postgres://u:p@h/db?application_name=sslmode=disable' + expect(unsupportedConnectionParam(disguised)).toBeUndefined() + expect(parsePostgresConnectionString(disguised)?.sslmode).toBeUndefined() + }) + + // libpq rejects `?SslMode=` as an invalid URI query parameter rather than folding it, so a + // string carrying one does not connect anywhere. Naming it is the honest answer; honouring + // it would save a resource from a URI Postgres itself refuses. + it('refuses a parameter whose name is not the one libpq accepts', () => { + const shouted = 'postgres://u:p@h/db?SslMode=verify-full' + expect(unsupportedConnectionParam(shouted)).toBe('SslMode') + expect(parsePostgresConnectionString(shouted)?.sslmode).toBeUndefined() + }) + + // libpq takes the last of a repeated parameter. Taking the first reads a weaker mode than + // the string actually asks for. + it('takes the last value of a repeated parameter', () => { + expect( + parsePostgresConnectionString('postgres://u:p@h/db?sslmode=disable&sslmode=require')?.sslmode + ).toBe('require') + }) + + it('ignores the one it can store, and the ones that cost nothing', () => { + expect(unsupportedConnectionParam('postgres://u:p@h/db?sslmode=require')).toBeUndefined() + expect(unsupportedConnectionParam('postgres://u:p@h/db?application_name=wm')).toBeUndefined() + expect(unsupportedConnectionParam('postgres://u:p@h/db')).toBeUndefined() + }) +}) + +// One refusal reached the user through two very different causes, and the wrong explanation +// sends them to fix the wrong thing: respelling a parameter this resource cannot store changes +// nothing, and removing one it can store loses what the string asked for. +describe('connectionParamRefusal', () => { + it('blames the spelling only when the parameter is one the resource keeps', () => { + expect(connectionParamRefusal('postgres://u:p@h/db?SslMode=verify-full')).toContain( + 'case-sensitive' + ) + expect(connectionParamRefusal('postgres://u:p@h/db?SslMode=verify-full')).toContain('sslmode') + }) + + it('blames the resource when respelling would not help', () => { + const refusal = connectionParamRefusal('postgres://u:p@h/db?Connect_Timeout=1') + expect(refusal).toContain('cannot store') + expect(refusal).not.toContain('case-sensitive') + }) + + it('says nothing about a string it can save', () => { + expect(connectionParamRefusal('postgres://u:p@h/db?sslmode=require')).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/utils/postgresConnectionString.ts b/frontend/src/lib/utils/postgresConnectionString.ts new file mode 100644 index 0000000000..d684739904 --- /dev/null +++ b/frontend/src/lib/utils/postgresConnectionString.ts @@ -0,0 +1,144 @@ +/** + * `postgres://user:password@host:5432/dbname?sslmode=require` in both directions. + * + * Shared by the resource form and the data table wizard: both turn a pasted + * connection string into a `postgresql` resource value, and the two drifting + * apart would mean the same string produced two different resources. + * + * The wizard offers the same connection as a string or as fields and lets the + * user switch, so parse and compose have to be inverses: whatever one produces, + * the other must read back unchanged. + * + * libpq is the arbiter of what a connection string means, so this follows it rather than + * RFC 3986 where they differ: credentials are split at the *first* `@` -- an unencoded one + * lands in the host for libpq too -- and percent escapes in them are decoded, so `p%40ss` + * authenticates as `p@ss`. + */ + +/** + * The host alternation is what admits IPv6: a literal address is full of colons, so a URI + * has to bracket it (`@[2001:db8::1]:5432/`) and the brackets are what tell the port apart + * from the address. Brackets are stripped on the way in and added back on the way out, so + * what is stored is the bare address a Postgres client wants. + */ +const CONNECTION_STRING = + /postgres(?:ql)?:\/\/(?[^:@]+)(?::(?[^@]+))?@(?\[[^\]]+\]|[^:\/?]+)(?::(?\d+))?\/(?[^\?]+)?/ + +/** + * The query parameters, read the way libpq reads them: names are case-sensitive — `SslMode` is + * rejected outright as an invalid URI query parameter, not folded to `sslmode` — and a name + * repeated takes its last value. One reader for both the parser and the allowlist below, or + * they disagree about what a string says and a name is refused by neither and honoured by + * neither. + */ +function paramsOf(connectionString: string): Map { + const query = connectionString.split('?').slice(1).join('?') + const params = new Map() + if (!query) return params + new URLSearchParams(query).forEach((value, name) => params.set(name, value)) + return params +} + +/** + * A database someone types into Windmill is almost never localhost, so callers ask for TLS + * where libpq would settle for `prefer`. A string that names its own `sslmode` keeps it. + */ +export const DEFAULT_SSLMODE = 'require' + +export type PostgresConnectionParts = { + user: string + password?: string + host: string + port?: number + dbname?: string + sslmode?: string +} + +/** A lone `%` is not an escape, and a password is free to contain one. */ +function decode(value: string): string { + try { + return decodeURIComponent(value) + } catch { + return value + } +} + +/** Undefined when the string is not a postgres URI. */ +export function parsePostgresConnectionString( + connectionString: string +): PostgresConnectionParts | undefined { + const match = connectionString.match(CONNECTION_STRING) + if (!match?.groups) return undefined + const { user, password, host, port, dbname } = match.groups + // By parameter name, never by searching the query text: `sslmode=` also occurs inside + // another parameter's *value*, and a substring match there reads someone's + // `application_name=sslmode=disable` as a request to turn TLS off. + const sslmode = paramsOf(connectionString).get('sslmode') + return { + user: decode(user), + password: password ? decode(password) : undefined, + host: host.startsWith('[') ? host.slice(1, -1) : host, + port: port ? Number(port) : undefined, + dbname: dbname ? decode(dbname) : undefined, + sslmode: sslmode || undefined + } +} + +/** The only query parameter the `postgresql` resource has a field for. */ +const REPRESENTABLE_PARAMS = ['sslmode'] + +/** + * Parameters that change nothing about what the connection reaches, how it is secured, or how + * it behaves, so losing them costs the user nothing. `connect_timeout` is deliberately not one + * of them: the backend applies its own fixed timeout, so honouring it is not on offer. + */ +const COSMETIC_PARAMS = ['application_name'] + +/** + * The name of a parameter this string carries that the resource cannot honour. An allowlist, + * not a list of known-bad names: libpq keeps adding parameters, and the ones that matter are + * the ones that would be missed. Dropping one silently saves a connection weaker or simply + * other than the one pasted, behind a probe that reports success. + */ +export function unsupportedConnectionParam(connectionString: string): string | undefined { + for (const name of paramsOf(connectionString).keys()) { + if (!REPRESENTABLE_PARAMS.includes(name) && !COSMETIC_PARAMS.includes(name)) return name + } + return undefined +} + +/** + * Why the string cannot be saved, in the terms the reader needs. Two refusals come out of the + * check above and they call for opposite fixes: a name Postgres does not accept at all, where + * the parameter itself is fine and only its spelling is wrong, and a parameter this resource + * has no field for, where respelling it changes nothing. + */ +export function connectionParamRefusal(connectionString: string): string | undefined { + const name = unsupportedConnectionParam(connectionString) + if (!name) return undefined + const lower = name.toLowerCase() + const storableWhenSpelledRight = + REPRESENTABLE_PARAMS.includes(lower) || COSMETIC_PARAMS.includes(lower) + return storableWhenSpelledRight + ? `Postgres does not accept ${name}: connection parameter names are case-sensitive. Write it as ${lower}.` + : `Windmill cannot store ${name} on a Postgres resource, and ignoring it would connect differently from what this string asks for. Remove it, or set the connection with the fields.` +} + +/** + * Every part that was set is emitted, `sslmode` included. Leaving `prefer` out because it is + * libpq's own default would be shorter, but it does not survive the trip: a caller that + * reparses this string gets `undefined` back and substitutes its own default, which is how an + * explicit `prefer` silently became `require`. Whatever this produces, `parse` must read back. + */ +export function composePostgresConnectionString(parts: PostgresConnectionParts): string { + const credentials = parts.password + ? `${encodeURIComponent(parts.user)}:${encodeURIComponent(parts.password)}` + : encodeURIComponent(parts.user) + const port = parts.port ? `:${parts.port}` : '' + const query = parts.sslmode ? `?sslmode=${parts.sslmode}` : '' + const dbname = parts.dbname ? encodeURIComponent(parts.dbname) : '' + // A bare IPv6 address would put its own colons where the port separator goes. + const host = + parts.host.includes(':') && !parts.host.startsWith('[') ? `[${parts.host}]` : parts.host + return `postgres://${credentials}@${host}${port}/${dbname}${query}` +} diff --git a/frontend/src/routes/oauth/callback_supabase/+page.svelte b/frontend/src/routes/oauth/callback_supabase/+page.svelte index 83bd448a13..ac6776804d 100644 --- a/frontend/src/routes/oauth/callback_supabase/+page.svelte +++ b/frontend/src/routes/oauth/callback_supabase/+page.svelte @@ -5,6 +5,7 @@ import { onMount } from 'svelte' import { OauthService } from '$lib/gen' import { oauthStore } from '$lib/stores' + import { hasParkedWizard } from '$lib/components/workspaceSettings/wizardParking' import CenteredPage from '$lib/components/CenteredPage.svelte' import PageHeader from '$lib/components/PageHeader.svelte' import { Loader2 } from 'lucide-svelte' @@ -15,25 +16,63 @@ let code = page.url.searchParams.get('code') ?? undefined let state = page.url.searchParams.get('state') ?? undefined + /** + * As the wizard's popup there is no page to land on: the tab behind us is still showing the + * flow and is watching for this window to go away. Leaving it open on a full Windmill page + * is what strands the caller's button spinning, and declining consent is a normal outcome, + * not an edge case. + */ + function closeIfPopup(): boolean { + if (!window.opener) return false + window.close() + return true + } + + /** + * Where a failed leg lands when this is not a popup. A parked run has to be handed back its + * own page: nothing else consumes the park, so sending it to `/resources` leaves the run in + * `sessionStorage` to spring the wizard open on some unrelated later visit. + */ + function failureDestination(): string { + return hasParkedWizard() ? '/workspace_settings?tab=windmill_data_tables' : '/resources' + } + onMount(async () => { if (error) { + if (closeIfPopup()) return sendUserToast(`Error trying to fetch projects from windmill: ${error}`, true) - goto('/resources') + goto(failureDestination()) } else if (code && state) { try { const res = await OauthService.connectCallback({ clientName: client_name, requestBody: { code, state } }) + // Opened as the data table wizard's popup: hand the token to the tab that is still + // sitting on the wizard and get out of the way, so nothing has to be resumed. + if (window.opener) { + window.opener.postMessage({ type: 'supabase_oauth', res }, window.location.origin) + window.close() + return + } $oauthStore = res - goto(`/resources?callback=${client_name}`) + // The data table wizard parks its state before redirecting, so it can be resumed + // where it left off. Everything else lands on the resources page, which opens the + // Supabase drawer for this callback. + if (hasParkedWizard()) { + goto(`/workspace_settings?tab=windmill_data_tables&callback=${client_name}`) + } else { + goto(`/resources?callback=${client_name}`) + } } catch (e) { + if (closeIfPopup()) return sendUserToast(`Error parsing the response token, ${e.body}`, true) - goto('/resources') + goto(failureDestination()) } } else { + if (closeIfPopup()) return sendUserToast('Missing code or state as query params', true) - goto('/resources') + goto(failureDestination()) } })