diff --git a/apps/fabro-web/app/api.ts b/apps/fabro-web/app/api.ts index d7171fbdc..4e9cbb9d0 100644 --- a/apps/fabro-web/app/api.ts +++ b/apps/fabro-web/app/api.ts @@ -48,14 +48,6 @@ export async function apiJsonOrNull( return response.json() as Promise; } -export async function getSetupStatus(): Promise<{ configured: boolean }> { - const response = await fetch("/api/v1/setup/status", { credentials: "include" }); - if (!response.ok) { - throw new Response(null, { status: response.status, statusText: response.statusText }); - } - return response.json(); -} - export async function getAuthConfig(): Promise<{ methods: string[] }> { const response = await fetch("/api/v1/auth/config", { credentials: "include" }); if (!response.ok) { diff --git a/apps/fabro-web/app/router.test.tsx b/apps/fabro-web/app/router.test.tsx index e409f73e7..70007f8fd 100644 --- a/apps/fabro-web/app/router.test.tsx +++ b/apps/fabro-web/app/router.test.tsx @@ -25,4 +25,11 @@ describe("browser router", () => { expect(paths).toContain("/login"); expect(paths).not.toContain("/auth/login"); }); + + test("exposes /setup but not the removed /setup/complete route", () => { + const paths = collectPaths(routes); + + expect(paths).toContain("/setup"); + expect(paths).not.toContain("/setup/complete"); + }); }); diff --git a/apps/fabro-web/app/router.tsx b/apps/fabro-web/app/router.tsx index d4ceb937d..8773d8b69 100644 --- a/apps/fabro-web/app/router.tsx +++ b/apps/fabro-web/app/router.tsx @@ -9,7 +9,6 @@ import { import Root, { ErrorBoundary as RootErrorBoundary } from "./root"; import * as RedirectHome from "./routes/redirect-home"; import * as Setup from "./routes/setup"; -import * as SetupComplete from "./routes/setup-complete"; import * as AuthLogin from "./routes/auth-login"; import * as Start from "./routes/start"; import * as Workflows from "./routes/workflows"; @@ -81,7 +80,6 @@ export const routes: RouteObject[] = [ children: [ indexRoute(RedirectHome), route("setup", Setup), - route("setup/complete", SetupComplete), route("login", AuthLogin), { loader: appShellLoader, diff --git a/apps/fabro-web/app/routes/redirect-home.tsx b/apps/fabro-web/app/routes/redirect-home.tsx index 1a372ec02..83a523945 100644 --- a/apps/fabro-web/app/routes/redirect-home.tsx +++ b/apps/fabro-web/app/routes/redirect-home.tsx @@ -1,12 +1,7 @@ import { redirect } from "react-router"; -import { getAuthMe, getSetupStatus } from "../api"; +import { getAuthMe } from "../api"; export async function loader() { - const setup = await getSetupStatus(); - if (!setup.configured) { - return redirect("/setup"); - } - try { await getAuthMe(); } catch (error) { diff --git a/apps/fabro-web/app/routes/setup-complete.tsx b/apps/fabro-web/app/routes/setup-complete.tsx deleted file mode 100644 index 566e43e3d..000000000 --- a/apps/fabro-web/app/routes/setup-complete.tsx +++ /dev/null @@ -1,105 +0,0 @@ -import { useEffect, useMemo, useRef, useState } from "react"; -import { AuthLayout } from "../components/auth-layout"; - -type SetupState = "registering" | "done" | "error"; - -export default function SetupComplete() { - const code = useMemo(() => new URLSearchParams(window.location.search).get("code"), []); - const [state, setState] = useState(code ? "registering" : "done"); - const [error, setError] = useState(null); - const [restartRequired, setRestartRequired] = useState(false); - const registeredRef = useRef(false); - - useEffect(() => { - if (registeredRef.current) return; - registeredRef.current = true; - - async function register() { - if (!code) { - setState("done"); - return; - } - - const response = await fetch("/api/v1/setup/register", { - method: "POST", - credentials: "include", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ code }), - }); - - if (!response.ok) { - const payload = await response.json().catch(() => ({})); - setError(payload.error ?? "Setup registration failed."); - setState("error"); - return; - } - - const payload = await response.json().catch(() => ({})); - setRestartRequired(payload.restart_required === true); - setState("done"); - window.history.replaceState({}, "", "/setup/complete"); - } - - void register(); - }, [code]); - - return ( - - {state === "registering" && ( - <> -

- Finishing setup -

-

- Registering your GitHub App and writing local configuration. -

- - )} - - {state === "done" && ( - <> -

- Setup complete -

-

- {restartRequired - ? "Your GitHub App is configured. Restart the Fabro server before attempting login." - : "Your GitHub App has been registered and configured."} -

- {restartRequired ? ( - - Back to setup - - ) : ( - - Continue to sign in - - )} - - )} - - {state === "error" && ( - <> -

- Setup failed -

-

- {error} -

- - Try again - - - )} - - ); -} diff --git a/apps/fabro-web/app/routes/setup.tsx b/apps/fabro-web/app/routes/setup.tsx index 168fbd5be..a159ddb76 100644 --- a/apps/fabro-web/app/routes/setup.tsx +++ b/apps/fabro-web/app/routes/setup.tsx @@ -1,58 +1,45 @@ -import { useMemo } from "react"; import { AuthLayout } from "../components/auth-layout"; -export default function Setup() { - const manifest = useMemo(() => { - const baseUrl = window.location.origin; - const suffix = Math.random().toString(16).slice(2, 8); - return JSON.stringify({ - name: `Fabro-${suffix}`, - url: "https://fabro.sh", - redirect_url: `${baseUrl}/setup/complete`, - callback_urls: [`${baseUrl}/auth/callback/github`], - setup_url: `${baseUrl}/setup/complete`, - public: false, - default_permissions: { - contents: "write", - metadata: "read", - pull_requests: "write", - checks: "write", - issues: "write", - emails: "read", - }, - default_events: [], - }); - }, []); +export default function Setup() { return ( - +

Set up Fabro

- Register a GitHub App to enable OAuth login and repository access. + Run the installer on the same host that runs the Fabro server to + register a GitHub App and write local configuration.

-
- - -
+
+
+

+ 1. Open a terminal on the server host +

+ 
+            fabro install
+
+
+
+

+ 2. Choose GitHub App setup +

+

+ The CLI opens GitHub, exchanges the manifest code, and writes the + required settings and secrets locally. +

+
+
+

+ 3. Restart the server, then return to sign in +

+ + Continue to sign in + +
+
); } - -function GitHubMark() { - return ( - - - - ); -} diff --git a/docs/integrations/github.mdx b/docs/integrations/github.mdx index ff9507c16..039d67d8e 100644 --- a/docs/integrations/github.mdx +++ b/docs/integrations/github.mdx @@ -39,15 +39,18 @@ The rest of this page describes the `app` strategy, which is required for browse ### Prerequisites - A GitHub account (personal or organization) -- The Fabro web app running (`cd apps/fabro-web && bun run dev`) +- Shell access to the host that runs the Fabro server +- The Fabro web app running (`cd apps/fabro-web && bun run dev`) if you want browser sign-in after setup -### Register the GitHub App +### Register the GitHub App with `fabro install` -1. Navigate to the web app (default `http://localhost:3000`). If no GitHub App is configured, you'll be redirected to the setup page automatically. +Run the installer on the same machine that will run the Fabro server: -2. Choose where to register the app. If you have the `gh` CLI installed, Fabro detects your GitHub username and any organizations you administer and lets you pick. For organization-owned apps, the app is registered under that org's settings. If `gh` is not available, the app is registered under your personal account. +```bash +fabro install +``` -3. Click **Register GitHub App**. This takes you to GitHub with a pre-filled [App Manifest](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest) containing: +When you choose the GitHub App strategy, the CLI opens GitHub with a pre-filled [App Manifest](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest) containing: | Permission | Level | Purpose | |---|---|---| @@ -58,15 +61,20 @@ The rest of this page describes the `app` strategy, which is required for browse | Issues | Write | Create issues from workflows | | Emails | Read | Read verified email for OAuth login | -4. Review the permissions on GitHub and click **Create GitHub App**. +The installer: -5. GitHub redirects back to Fabro, which automatically: - - Exchanges the temporary code for permanent app credentials - - Writes `app_id`, `client_id`, and `slug` to `~/.fabro/settings.toml` - - Stores `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`, and `GITHUB_APP_PRIVATE_KEY` in `/server.env` - - Marks the change as restart-bound so the server must be restarted before login +1. Lets you choose where to register the app. If you have the `gh` CLI installed, Fabro detects your GitHub username and any organizations you administer and lets you pick. If `gh` is not available, the app is registered under your personal account. +2. Opens GitHub in your browser so you can review the manifest and click **Create GitHub App**. +3. Receives GitHub's temporary callback on localhost, exchanges it for permanent app credentials, and writes: + - `app_id`, `client_id`, and `slug` to `~/.fabro/settings.toml` + - `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`, and `GITHUB_APP_PRIVATE_KEY` to `/server.env` +4. Prints the resulting app slug so you can install the app on the repositories Fabro should access. -6. **Install the app** on your GitHub account or organization. Go to `https://github.com/settings/apps//installations` and install it on the repositories Fabro should access. +After setup completes, restart the Fabro server before attempting browser login. + +### Install the app on GitHub + +Go to `https://github.com/settings/apps//installations` and install it on the repositories Fabro should access. ### Verify the configuration @@ -143,8 +151,7 @@ The web app uses the GitHub App's OAuth credentials to authenticate users: Configure allowed users in `settings.toml`: ```toml title="settings.toml" -[web.auth] -provider = "github" +[server.auth.github] allowed_usernames = ["alice", "bob"] ``` @@ -218,8 +225,8 @@ The app is installed but doesn't have access to this specific repository. Update ### "GitHub App authentication failed" -The `app_id` in `settings.toml` or the `GITHUB_APP_PRIVATE_KEY` environment variable is incorrect. Re-run the setup flow or verify the values match your GitHub App. +The `app_id` in `settings.toml` or the `GITHUB_APP_PRIVATE_KEY` environment variable is incorrect. Re-run `fabro install` on the server host or verify the values match your GitHub App. ### Clone fails for private repositories -If you see `Git clone failed ... If this is a private repository, configure a GitHub App`, the GitHub App credentials are not configured. Run the setup flow through the web UI or verify with `fabro doctor`. +If you see `Git clone failed ... If this is a private repository, configure a GitHub App`, the GitHub App credentials are not configured. Run `fabro install` on the server host or verify with `fabro doctor`. diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 49ed39a30..c0faefc24 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -760,10 +760,10 @@ struct CallbackParams { fn build_github_app_manifest(app_name: &str, port: u16, web_url: &str) -> serde_json::Value { serde_json::json!({ "name": app_name, - "url": "https://github.com/apps/arc", + "url": "https://fabro.sh", "redirect_url": format!("http://127.0.0.1:{port}/callback"), - "callback_urls": [format!("{web_url}/auth/callback")], - "setup_url": format!("{web_url}/setup/callback"), + "callback_urls": [format!("{web_url}/auth/callback/github")], + "setup_url": format!("{web_url}/setup"), "public": false, "default_permissions": { "contents": "write", @@ -1680,13 +1680,14 @@ client_id = "client-id" let web_url = "https://app.example.com"; let manifest = build_github_app_manifest("Fabro-test", 12345, web_url); + assert_eq!(manifest["url"], serde_json::json!("https://fabro.sh"),); assert_eq!( manifest["callback_urls"], - serde_json::json!(["https://app.example.com/auth/callback"]), + serde_json::json!(["https://app.example.com/auth/callback/github"]), ); assert_eq!( manifest["setup_url"], - serde_json::json!("https://app.example.com/setup/callback"), + serde_json::json!("https://app.example.com/setup"), ); } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index de2308b01..5b4c6c6e9 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -4,7 +4,7 @@ use std::time::Duration; use anyhow::Context; use clap::Args; -use fabro_config::user::{active_settings_path, load_settings_config}; +use fabro_config::user::load_settings_config; use fabro_config::{Storage, resolve_server_from_file}; use fabro_sandbox::SandboxProvider; use fabro_types::settings::server::GithubIntegrationStrategy; @@ -88,10 +88,6 @@ fn load_settings(path: Option<&Path>) -> anyhow::Result { Ok(load_settings_config(path)?) } -fn resolved_config_path(path: Option<&Path>) -> PathBuf { - active_settings_path(path) -} - fn apply_serve_overrides( base: &SettingsLayer, args: &ServeArgs, @@ -258,7 +254,6 @@ where let config_path = args.config.clone(); let disk_settings = load_settings(config_path.as_deref())?; let disk_server_settings = resolve_server_settings(&disk_settings)?; - let active_config_path = resolved_config_path(config_path.as_deref()); let data_dir = match storage_dir_override { Some(path) => path, None => resolve_interp_path(&disk_server_settings.storage.root)?, @@ -329,7 +324,6 @@ where store, artifact_store, &vault_path, - active_config_path, true, )?; let reconciled = reconcile_incomplete_runs_on_startup(&state).await?; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 3f7fde061..aaa0d0725 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -524,7 +524,6 @@ pub struct AppState { pub(crate) provider_credentials: ProviderCredentials, pub(crate) settings: Arc>, pub(crate) server_settings: RwLock>, - pub(crate) config_path: PathBuf, pub(crate) local_daemon_mode: bool, shutting_down: AtomicBool, registry_factory_override: Option>, @@ -712,11 +711,6 @@ impl AppState { .expect("server settings lock poisoned") = resolved; Ok(()) } - - pub(crate) fn reload_settings_from_disk(&self) -> anyhow::Result<()> { - let reloaded = fabro_config::load_settings_path(&self.config_path)?; - self.replace_settings(reloaded) - } } fn artifact_upload_token_keys() -> ArtifactUploadTokenKeys { @@ -858,6 +852,10 @@ impl Default for RouterOptions { } } +fn removed_web_route(path: &str) -> bool { + matches!(path, "/setup/complete") +} + /// Build the axum Router with configurable web surface routing. pub fn build_router_with_options( state: Arc, @@ -931,6 +929,8 @@ pub fn build_router_with_options( || (options.web_enabled && path.starts_with("/auth/")); if dispatch_path { dispatch.oneshot(req).await + } else if options.web_enabled && removed_web_route(&path) { + Ok::<_, std::convert::Infallible>(StatusCode::NOT_FOUND.into_response()) } else if options.web_enabled && matches!(req.method(), &Method::GET | &Method::HEAD) { @@ -2155,7 +2155,6 @@ pub fn create_app_state_with_settings_and_registry_factory( store, artifact_store, &test_secret_store_path(), - test_config_path(), false, ) .expect("test app state should build") @@ -2201,7 +2200,6 @@ pub(crate) fn create_test_app_state_with_session_key( store, artifact_store, &secrets_path, - test_config_path(), local_daemon_mode, ) .expect("test app state should build") @@ -2231,7 +2229,6 @@ pub fn create_app_state_with_store( store, artifact_store, &test_secret_store_path(), - test_config_path(), false, ) .expect("test app state should build") @@ -2244,7 +2241,6 @@ pub(crate) fn build_app_state_with_path( store: Arc, artifact_store: ArtifactStore, vault_path: &std::path::Path, - config_path: PathBuf, local_daemon_mode: bool, ) -> anyhow::Result> { let vault = Arc::new(AsyncRwLock::new(Vault::load(vault_path.to_path_buf())?)); @@ -2306,7 +2302,6 @@ pub(crate) fn build_app_state_with_path( provider_credentials, settings, server_settings: RwLock::new(resolved_server_settings), - config_path, local_daemon_mode, shutting_down: AtomicBool::new(false), registry_factory_override, @@ -2319,10 +2314,6 @@ fn test_secret_store_path() -> PathBuf { std::env::temp_dir().join(format!("fabro-test-secrets-{}.json", Ulid::new())) } -fn test_config_path() -> PathBuf { - std::env::temp_dir().join(format!("fabro-test-settings-{}.toml", Ulid::new())) -} - fn board_column(status: RunStatus) -> Option<&'static str> { match status { RunStatus::Running => Some("working"), diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index 232f96d92..16b630766 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -42,16 +42,6 @@ impl ServerSecrets { pub(crate) fn get(&self, name: &str) -> Option { (self.env_lookup)(name).or_else(|| self.file_entries.get(name).cloned()) } - - pub(crate) fn persist_updates(&mut self, updates: I) -> Result<(), Error> - where - I: IntoIterator, - K: Into, - V: Into, - { - self.file_entries = envfile::merge_env_file(&self.path, updates)?; - Ok(()) - } } impl std::fmt::Debug for ServerSecrets { diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index cc2978bf5..65ef92bd7 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -1,16 +1,12 @@ use std::sync::Arc; -use anyhow::{Context, anyhow}; use axum::extract::{Query, State}; use axum::http::{HeaderMap, HeaderValue, StatusCode, header}; use axum::response::{IntoResponse, Redirect, Response}; use axum::routing::{get, post}; use axum::{Extension, Json, Router}; -use base64::Engine as _; -use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use cookie::time::Duration; use cookie::{Cookie, CookieJar, Key, SameSite}; -use fabro_config::Storage; use fabro_types::RunAuthMethod; use fabro_types::settings::{InterpString, ServerAuthMethod, SettingsLayer}; use fabro_util::dev_token::validate_dev_token_format; @@ -18,10 +14,8 @@ use serde::{Deserialize, Serialize}; use serde_json::json; use tracing::{debug, error, info, warn}; -use crate::error::ApiError; -use crate::jwt_auth::{AuthMode, AuthenticatedSubject, auth_method_name, dev_token_matches}; +use crate::jwt_auth::{AuthMode, auth_method_name, dev_token_matches}; use crate::server::AppState; -use crate::server_secrets::ServerSecrets; pub const SESSION_COOKIE_NAME: &str = "__fabro_session"; const OAUTH_STATE_COOKIE_NAME: &str = "fabro_oauth_state"; @@ -51,21 +45,11 @@ struct DevTokenLoginRequest { token: String, } -#[derive(Deserialize)] -struct SetupRegisterRequest { - code: String, -} - #[derive(Deserialize)] struct DemoToggleRequest { enabled: bool, } -#[derive(Serialize)] -struct SetupStatusResponse { - configured: bool, -} - #[derive(Serialize)] struct AuthConfigResponse { methods: Vec, @@ -111,16 +95,6 @@ struct GitHubEmail { verified: bool, } -#[derive(Deserialize)] -struct GitHubManifestConversion { - id: i64, - slug: String, - client_id: String, - client_secret: String, - webhook_secret: Option, - pem: String, -} - pub fn routes() -> Router> { Router::new() .route("/login/dev-token", post(login_dev_token)) @@ -133,8 +107,6 @@ pub fn api_routes() -> Router> { Router::new() .route("/auth/config", get(auth_config)) .route("/auth/me", get(auth_me)) - .route("/setup/register", post(setup_register)) - .route("/setup/status", get(setup_status)) .route("/demo/toggle", post(toggle_demo)) } @@ -669,20 +641,6 @@ async fn auth_me(State(state): State>, headers: HeaderMap) -> Resp .into_response() } -async fn setup_status( - State(state): State>, - Extension(auth_mode): Extension, -) -> Response { - let configured = state - .server_settings() - .integrations - .github - .client_id - .is_some() - || auth_method_enabled(&auth_mode, ServerAuthMethod::DevToken); - Json(SetupStatusResponse { configured }).into_response() -} - async fn toggle_demo(Json(payload): Json) -> Response { let mut jar = CookieJar::new(); jar.add( @@ -697,277 +655,6 @@ async fn toggle_demo(Json(payload): Json) -> Response { response } -async fn setup_register( - subject: AuthenticatedSubject, - State(state): State>, - headers: HeaderMap, - Json(payload): Json, -) -> Response { - if subject.auth_method != RunAuthMethod::DevToken { - return ApiError::forbidden().into_response(); - } - - let origin = headers - .get(header::ORIGIN) - .or_else(|| headers.get(header::REFERER)) - .and_then(|v| v.to_str().ok()) - .and_then(|s| fabro_http::Url::parse(s).ok()) - .map(|url| format!("{}://{}", url.scheme(), url.authority())); - - let http = match fabro_http::http_client() { - Ok(http) => http, - Err(err) => { - error!(error = %err, "Setup register failed: could not build GitHub HTTP client"); - return json_response( - StatusCode::SERVICE_UNAVAILABLE, - json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), - ); - } - }; - let response = match http - .post(format!( - "https://api.github.com/app-manifests/{}/conversions", - payload.code - )) - .header(header::ACCEPT, "application/vnd.github+json") - .header(header::USER_AGENT, "fabro-server") - .send() - .await - { - Ok(response) => response, - Err(err) => { - error!(error = %err, "Setup register failed: GitHub manifest conversion request failed"); - return json_response( - StatusCode::BAD_GATEWAY, - json!({"error": format!("GitHub manifest conversion failed: {err}")}), - ); - } - }; - - if !response.status().is_success() { - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - error!(status = %status, body = %body, "Setup register failed: GitHub manifest conversion returned error"); - return json_response( - StatusCode::BAD_GATEWAY, - json!({"error": format!("GitHub manifest conversion failed: {status}")}), - ); - } - - let body = match response.text().await { - Ok(body) => body, - Err(err) => { - error!(error = %err, "Setup register failed: could not read conversion response body"); - return json_response( - StatusCode::BAD_GATEWAY, - json!({"error": "Failed to read GitHub manifest conversion response"}), - ); - } - }; - let data = match serde_json::from_str::(&body) { - Ok(data) => data, - Err(err) => { - error!(error = %err, body = %body, "Setup register failed: could not parse conversion response"); - return json_response( - StatusCode::BAD_GATEWAY, - json!({"error": format!("Failed to parse GitHub manifest conversion response: {err}")}), - ); - } - }; - - let settings_path = state.config_path.clone(); - - // Edit the settings file in place via `toml_edit::DocumentMut`, which - // preserves existing comments, whitespace, and key ordering. The value- - // tree parser (`toml::Value`) would strip all of that on round-trip. - if let Some(parent) = settings_path.parent() { - if let Err(err) = std::fs::create_dir_all(parent) { - error!(error = %err, path = %parent.display(), "Setup register failed: could not create settings parent directory"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to create settings directory: {err}")}), - ); - } - } - let existing = std::fs::read_to_string(&settings_path).unwrap_or_default(); - let mut doc: toml_edit::DocumentMut = if existing.is_empty() { - toml_edit::DocumentMut::new() - } else { - match existing - .parse::() - .context("failed to parse existing settings config") - { - Ok(doc) => doc, - Err(err) => { - error!(error = %err, path = %settings_path.display(), "Setup register failed: could not parse settings config"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to parse settings config: {err}")}), - ); - } - } - }; - if let Err(err) = merge_settings_keys(&mut doc, &data, origin.as_deref()) { - error!(error = %err, "Setup register failed: could not merge settings"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to update settings config: {err}")}), - ); - } - if let Err(err) = std::fs::write(&settings_path, doc.to_string()) { - error!(error = %err, path = %settings_path.display(), "Setup register failed: could not write settings config"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to write settings config: {err}")}), - ); - } - - let mut secret_updates = vec![ - ("GITHUB_APP_CLIENT_SECRET", data.client_secret.clone()), - ( - "GITHUB_APP_PRIVATE_KEY", - BASE64_STANDARD.encode(data.pem.as_bytes()), - ), - ]; - if let Some(ref webhook_secret) = data.webhook_secret { - secret_updates.push(("GITHUB_APP_WEBHOOK_SECRET", webhook_secret.clone())); - } - - let server_env_path = Storage::new(state.server_storage_dir()) - .server_state() - .env_path(); - let mut server_secrets = match ServerSecrets::load(server_env_path.clone()) { - Ok(server_secrets) => server_secrets, - Err(err) => { - error!(error = %err, path = %server_env_path.display(), "Setup register failed: could not load server env"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to load server env: {err}")}), - ); - } - }; - if let Err(err) = server_secrets.persist_updates(secret_updates) { - error!(error = %err, path = %server_env_path.display(), "Setup register failed: could not write server env"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to write server env: {err}")}), - ); - } - - // Re-parse the freshly-written settings file and swap it into the - // in-memory state so the non-secret GitHub App config becomes visible - // immediately. Secret material remains restart-bound through server.env. - match state.reload_settings_from_disk() { - Ok(()) => {} - Err(err) => { - error!(error = %err, path = %settings_path.display(), "Setup register failed: could not reload written settings config"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to reload settings config after write: {err}")}), - ); - } - } - - info!(slug = %data.slug, app_id = %data.id, "GitHub App registered successfully"); - Json(json!({"ok": true, "restart_required": true})).into_response() -} - -/// Walk dotted `path` into `doc`, creating missing intermediate tables, -/// and return a mutable reference to the terminal table. -/// -/// Uses `toml_edit`'s [`toml_edit::Entry::or_insert`] so existing tables -/// keep their comments, ordering, and any sibling keys untouched. -fn ensure_nested_table<'a>( - doc: &'a mut toml_edit::DocumentMut, - path: &[&str], -) -> anyhow::Result<&'a mut toml_edit::Table> { - let mut current: &mut toml_edit::Table = doc.as_table_mut(); - for segment in path { - let next = current - .entry(segment) - .or_insert(toml_edit::Item::Table(toml_edit::Table::new())); - current = next - .as_table_mut() - .ok_or_else(|| anyhow!("settings config [{segment}] is not a table"))?; - } - Ok(current) -} - -/// Set a scalar value on `table[key]`, preserving any key-level decor -/// (leading comments, blank lines) that was attached to the existing entry. -/// -/// `toml_edit`'s default `Table::insert` replaces the entry wholesale and -/// drops its prefix decoration, which would strip a top-of-file comment -/// attached to a key we're updating. Copying the decor forward keeps the -/// user's formatting intact. -fn set_preserving_decor(table: &mut toml_edit::Table, key: &str, value: toml_edit::Item) { - let preserved_decor = table.key(key).map(|existing| existing.leaf_decor().clone()); - table.insert(key, value); - if let (Some(decor), Some(mut updated)) = (preserved_decor, table.key_mut(key)) { - *updated.leaf_decor_mut() = decor; - } -} - -fn merge_settings_keys( - doc: &mut toml_edit::DocumentMut, - data: &GitHubManifestConversion, - origin: Option<&str>, -) -> anyhow::Result<()> { - let web_url = origin.map_or_else(|| "http://localhost:3000".to_string(), str::to_string); - - // Make sure the freshly-written file is a valid v2 file. `_version` is - // always `1` at the moment, so skip the write entirely if it's already - // there -- otherwise we'd trample any top-of-file comment attached to - // the key. - let root = doc.as_table_mut(); - if !root.contains_key("_version") { - root.insert("_version", toml_edit::value(1_i64)); - } - - let web = ensure_nested_table(doc, &["server", "web"])?; - set_preserving_decor(web, "enabled", toml_edit::value(true)); - set_preserving_decor(web, "url", toml_edit::value(web_url)); - - let auth = ensure_nested_table(doc, &["server", "auth"])?; - let mut methods = auth - .get("methods") - .and_then(|item| item.as_array()) - .map(|array| { - array - .iter() - .filter_map(|value| value.as_str().map(str::to_string)) - .collect::>() - }) - .unwrap_or_default(); - if !methods.iter().any(|method| method == "github") { - methods.push("github".to_string()); - } - if methods.is_empty() { - methods.push("github".to_string()); - } - set_preserving_decor( - auth, - "methods", - toml_edit::value( - methods - .into_iter() - .map(toml_edit::Value::from) - .collect::(), - ), - ); - - let github = ensure_nested_table(doc, &["server", "integrations", "github"])?; - set_preserving_decor(github, "app_id", toml_edit::value(data.id.to_string())); - set_preserving_decor( - github, - "client_id", - toml_edit::value(data.client_id.clone()), - ); - set_preserving_decor(github, "slug", toml_edit::value(data.slug.clone())); - - Ok(()) -} - #[cfg(test)] mod tests { use std::sync::Arc; @@ -986,10 +673,7 @@ mod tests { use serde_json::{Value, json}; use tower::ServiceExt; - use super::{ - GitHubManifestConversion, SessionCookie, api_routes, merge_settings_keys, - read_private_session, routes, - }; + use super::{SessionCookie, api_routes, read_private_session, routes}; use crate::jwt_auth::{AuthMode, ConfiguredAuth}; use crate::server; @@ -1036,19 +720,6 @@ mod tests { } } - fn encode_session_cookie(key: &Key, session: &SessionCookie) -> String { - let mut jar = cookie::CookieJar::new(); - jar.private_mut(key).add(cookie::Cookie::new( - super::SESSION_COOKIE_NAME, - serde_json::to_string(session).unwrap(), - )); - jar.delta() - .next() - .expect("private cookie should exist") - .encoded() - .to_string() - } - fn test_auth_router_with_settings( settings: SettingsLayer, auth_mode: AuthMode, @@ -1087,143 +758,6 @@ mod tests { serde_json::from_slice(&to_bytes(response.into_body(), usize::MAX).await.unwrap()).unwrap() } - fn sample_conversion() -> GitHubManifestConversion { - GitHubManifestConversion { - id: 123, - slug: "fabro".to_string(), - client_id: "abc".to_string(), - client_secret: "shh".to_string(), - pem: String::new(), - webhook_secret: None, - } - } - - fn parse_doc(source: &str) -> toml_edit::DocumentMut { - source - .parse::() - .expect("fixture should parse as TOML") - } - - #[test] - fn merge_settings_keys_writes_v2_server_integrations_github() { - let mut doc = parse_doc("_version = 1\n"); - merge_settings_keys(&mut doc, &sample_conversion(), Some("https://example.test")).unwrap(); - - let methods = doc["server"]["auth"]["methods"] - .as_array() - .expect("server.auth.methods should exist"); - assert_eq!( - methods.iter().next().and_then(|value| value.as_str()), - Some("github") - ); - - let github = doc["server"]["integrations"]["github"] - .as_table() - .expect("server.integrations.github should exist"); - assert_eq!(github["app_id"].as_str(), Some("123")); - assert_eq!(github["slug"].as_str(), Some("fabro")); - assert_eq!(github["client_id"].as_str(), Some("abc")); - - let web = doc["server"]["web"] - .as_table() - .expect("server.web should exist"); - assert_eq!(web["url"].as_str(), Some("https://example.test")); - assert_eq!(web["enabled"].as_bool(), Some(true)); - - // Re-parse the emitted document to prove it round-trips into a - // valid v2 `SettingsLayer`. - let emitted = doc.to_string(); - let file = fabro_config::parse_settings_layer(&emitted) - .expect("merged output should parse as a v2 SettingsLayer"); - let server = file.server.as_ref().expect("[server] should be present"); - let integrations = server - .integrations - .as_ref() - .expect("[server.integrations] should be present"); - let github = integrations - .github - .as_ref() - .expect("[server.integrations.github] should be present"); - assert_eq!( - github - .app_id - .as_ref() - .map(fabro_types::settings::InterpString::as_source), - Some("123".to_string()) - ); - } - - #[test] - fn merge_settings_keys_preserves_comments_and_unrelated_keys() { - let existing = r##"# Top-of-file comment explaining the settings layout. -_version = 1 - -# Storage root comment — should survive the edit. -[server.storage] -root = "/srv/fabro-data" - -# A pre-existing integration that is NOT github. -[server.integrations.slack] -default_channel = "#ops" - -[run.model] -provider = "anthropic" -name = "claude-sonnet" -"##; - let mut doc = parse_doc(existing); - merge_settings_keys( - &mut doc, - &sample_conversion(), - Some("https://fabro.example"), - ) - .unwrap(); - - let emitted = doc.to_string(); - - // Comments must survive the round-trip. - assert!( - emitted.contains("# Top-of-file comment explaining the settings layout."), - "top-of-file comment was stripped:\n{emitted}" - ); - assert!( - emitted.contains("# Storage root comment — should survive the edit."), - "inline table comment was stripped:\n{emitted}" - ); - assert!( - emitted.contains("# A pre-existing integration that is NOT github."), - "sibling-table comment was stripped:\n{emitted}" - ); - - // Unrelated keys must still be intact. - assert!( - emitted.contains(r#"root = "/srv/fabro-data""#), - "server.storage.root was lost:\n{emitted}" - ); - assert!( - emitted.contains(r##"default_channel = "#ops""##), - "server.integrations.slack.default_channel was lost:\n{emitted}" - ); - assert!( - emitted.contains(r#"provider = "anthropic""#), - "run.model.provider was lost:\n{emitted}" - ); - - // And the new keys must be present. - assert!( - emitted.contains(r#"app_id = "123""#), - "server.integrations.github.app_id missing:\n{emitted}" - ); - assert!( - emitted.contains(r#"url = "https://fabro.example""#), - "server.web.url missing:\n{emitted}" - ); - - // Finally, the whole thing must still parse as a valid v2 - // SettingsLayer. - fabro_config::parse_settings_layer(&emitted) - .expect("merged output should still parse as v2 after the edit"); - } - #[tokio::test] async fn login_dev_token_mints_session_with_dev_token_provider() { let key = Key::derive_from(b"web-auth-test-key-material-0123456789"); @@ -1317,61 +851,6 @@ name = "claude-sonnet" assert_eq!(body, json!({ "methods": ["dev-token"] })); } - #[tokio::test] - async fn setup_register_requires_authentication() { - let app = test_auth_router_with_settings(SettingsLayer::default(), dev_token_auth_mode()); - - let response = app - .oneshot( - Request::builder() - .method("POST") - .uri("/api/v1/setup/register") - .body(Body::from("not json")) - .unwrap(), - ) - .await - .unwrap(); - - assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - } - - #[tokio::test] - async fn setup_register_forbids_github_sessions() { - let key = Key::derive_from(b"web-auth-test-key-material-0123456789"); - let app = test_auth_router_with_settings( - github_settings("https://fabro.example"), - github_auth_mode(), - ); - let now = chrono::Utc::now().timestamp(); - let session_cookie = encode_session_cookie(&key, &SessionCookie { - v: 1, - login: "octocat".to_string(), - auth_method: RunAuthMethod::Github, - provider_id: Some(1), - name: "Octocat".to_string(), - email: "octocat@example.com".to_string(), - avatar_url: "https://avatars.example/octocat".to_string(), - user_url: "https://github.com/octocat".to_string(), - iat: now, - exp: now + 60, - }); - - let response = app - .oneshot( - Request::builder() - .method("POST") - .uri("/api/v1/setup/register") - .header(header::CONTENT_TYPE, "application/json") - .header(header::COOKIE, session_cookie) - .body(Body::from(json!({ "code": "fake-code" }).to_string())) - .unwrap(), - ) - .await - .unwrap(); - - assert_eq!(response.status(), StatusCode::FORBIDDEN); - } - #[tokio::test] async fn login_github_sets_secure_state_cookie_for_https_web_url() { let app = test_auth_router_with_settings( diff --git a/lib/crates/fabro-server/tests/it/api/routing.rs b/lib/crates/fabro-server/tests/it/api/routing.rs index f4d3c5e9c..b0ee0c210 100644 --- a/lib/crates/fabro-server/tests/it/api/routing.rs +++ b/lib/crates/fabro-server/tests/it/api/routing.rs @@ -97,13 +97,29 @@ async fn web_enabled_serves_web_only_routes() { let auth_me_response = app.clone().oneshot(auth_me_request).await.unwrap(); assert_eq!(auth_me_response.status(), StatusCode::UNAUTHORIZED); + let setup_request = Request::builder() + .method("GET") + .uri("/setup") + .body(Body::empty()) + .unwrap(); + let setup_response = app.clone().oneshot(setup_request).await.unwrap(); + assert_eq!(setup_response.status(), StatusCode::OK); + let setup_status_request = Request::builder() .method("GET") .uri("/api/v1/setup/status") .body(Body::empty()) .unwrap(); let setup_status_response = app.clone().oneshot(setup_status_request).await.unwrap(); - assert_eq!(setup_status_response.status(), StatusCode::OK); + assert_eq!(setup_status_response.status(), StatusCode::NOT_FOUND); + + let setup_complete_request = Request::builder() + .method("GET") + .uri("/setup/complete") + .body(Body::empty()) + .unwrap(); + let setup_complete_response = app.clone().oneshot(setup_complete_request).await.unwrap(); + assert_eq!(setup_complete_response.status(), StatusCode::NOT_FOUND); let demo_toggle_request = Request::builder() .method("POST") @@ -138,6 +154,7 @@ enabled = false for (method, path, body) in [ ("GET", "/", Body::empty()), + ("GET", "/setup", Body::empty()), ("GET", "/runs/abc", Body::empty()), ("GET", "/auth/login/github", Body::empty()), ("GET", "/api/v1/auth/me", Body::empty()),