diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fa6be2..99cd3a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,23 @@ current as you land changes. ### Changed +- **The setup wizard is two steps.** Blocked countries, then one "Use automatic + VPN detection?" tickbox with the manual fields — tunnel interfaces, self-hosted + config files, endpoints — revealed underneath it when you untick it. The + opening "Configure your VPN now?" question is gone; a run always writes the + VPN keys, so instead the detection answer is **seeded from your config** and a + config with pinned `vpn.tunnelInterfaces` starts on manual, meaning a re-run + clicked straight through preserves your pins — even when that tunnel is down, + which previously dropped it from the pick list and so cleared it. A question + that is not asked still writes no key, so leaving automatic detection on does + not blank endpoints you set by hand; the one deliberate exception is that + *choosing* automatic detection clears pinned interfaces, since a leftover pin + is what stops autodetection happening. Off macOS, where there is no live discovery, the + endpoint question is asked whichever mode you pick. Both wizards read the same + question set, so `dezhban setup` changes with the app — in a terminal, + unticking automatic detection brings the manual fields as a second prompt + rather than revealing them in place, since a form cannot react to an answer + given inside itself. - **Contextual help lands on the key you asked about.** The **?** beside a setting used to open one of four section anchors shared by every key in that section; it now scrolls to that key's own row in the configuration reference. diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 7704b97..7ae80cf 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -66,27 +66,43 @@ func cmdSetup(args []string) int { // Asked a screenful at a time, in Group order, so a gate can be evaluated // against answers already given — which is exactly what makes the VPN // branch a branch. + // + // Within a group, in waves. A huh form binds every field before any of them + // is answered, so a question gated on another question in the SAME group + // would be decided by that question's seeded default rather than by what + // the user just typed. The macOS app has no such problem — it re-evaluates + // gates as answers change and shows the whole step at once, which is what + // makes step 2 a single screen there — so rather than splitting the shared + // question set to suit one renderer, this one asks the ungated questions, + // re-evaluates, and asks whatever that opened up. for _, group := range groupsOf(qs) { - var fields []huh.Field - for _, q := range qs { - if q.Group != group || !answers.ShouldAsk(q) { - continue + asked := map[string]bool{} + for { + wave := nextWave(qs, group, asked, answers) + if len(wave) == 0 { + break + } + var fields []huh.Field + for _, q := range wave { + asked[q.ID] = true + fields = append(fields, field(q, answers)) + } + if err := runForm(huh.NewForm(huh.NewGroup(fields...))); err != nil { + return formExit(err) } - fields = append(fields, field(q, answers)) - } - if len(fields) == 0 { - continue - } - if err := runForm(huh.NewForm(huh.NewGroup(fields...))); err != nil { - return formExit(err) } } // Import any named config files into profiles (best-effort; a bad file is // reported but doesn't abort the wizard). Reading files is the caller's job, // not internal/setup's. + // Only what the user was actually shown: the question is gated behind manual + // mode, and reading it unconditionally would import files from a field this + // run never rendered — the same "unasked means untouched" rule Apply follows + // for keys, and what keeps this in step with the macOS app, whose + // profileFiles is empty for exactly that reason. var profiles []config.Profile - if answers.Bool("configureVPN") { + if q, ok := questionByID(qs, "profileFiles"); ok && answers.ShouldAsk(q) { for _, f := range setup.SplitList(answers.Text("profileFiles")) { eps, format, ierr := vpnimport.Extract(f) if ierr != nil { @@ -112,20 +128,18 @@ func cmdSetup(args []string) int { } // --- lockout guard: warn if an endpoint sits inside a tunnel subnet --- - if answers.Bool("configureVPN") { - if warn := setup.EndpointLockoutWarning(cfg); warn != "" { - var proceed bool - fmt.Fprintln(os.Stderr, warn) - if err := runForm(huh.NewForm(huh.NewGroup( - huh.NewConfirm().Title("Save anyway?"). - Description("The flagged endpoint would very likely lock you out.").Value(&proceed), - ))); err != nil { - return formExit(err) - } - if !proceed { - fmt.Fprintln(os.Stderr, "setup cancelled — fix the endpoint (see 'dezhban doctor').") - return 1 - } + if warn := setup.EndpointLockoutWarning(cfg); warn != "" { + var proceed bool + fmt.Fprintln(os.Stderr, warn) + if err := runForm(huh.NewForm(huh.NewGroup( + huh.NewConfirm().Title("Save anyway?"). + Description("The flagged endpoint would very likely lock you out.").Value(&proceed), + ))); err != nil { + return formExit(err) + } + if !proceed { + fmt.Fprintln(os.Stderr, "setup cancelled — fix the endpoint (see 'dezhban doctor').") + return 1 } } @@ -176,9 +190,7 @@ func cmdSetup(args []string) int { } else { fmt.Println("later, enable it with: sudo dezhban install && sudo dezhban start") } - if answers.Bool("configureVPN") { - fmt.Println("to connect a brand-new VPN whose server isn't known yet: dezhban switch, then connect it.") - } + fmt.Println("to connect a brand-new VPN whose server isn't known yet: dezhban switch, then connect it.") return 0 } @@ -266,11 +278,29 @@ func printQuestions(qs []setup.Question, asJSON bool) int { fmt.Printf(" selected: %s\n", strings.Join(q.Selected, ", ")) } if len(q.Options) > 0 { + // Value is what gets written, but an option whose label says more + // than its value — a tunnel offered because it is configured rather + // than because it was detected — has to show that here too. This is + // the form a human reads to answer "why is that one on the list?"; + // --json already carries both. vals := make([]string, 0, len(q.Options)) for _, o := range q.Options { - vals = append(vals, o.Value) + switch { + case o.Label == "" || o.Label == o.Value: + vals = append(vals, o.Value) + case strings.Contains(o.Label, o.Value): + // The label already carries the value, as the tunnel list's + // "utun9 (configured, not up right now)" does. + vals = append(vals, o.Label) + default: + vals = append(vals, fmt.Sprintf("%s (%s)", o.Value, o.Label)) + } } - fmt.Printf(" options: %s\n", strings.Join(vals, ", ")) + // Semicolons, not commas: labels are prose and may contain a comma + // themselves ("utun9 (configured, not up right now)"), which in a + // comma-joined list reads as two options, one of them an interface + // named "utun9 (configured". + fmt.Printf(" options: %s\n", strings.Join(vals, "; ")) } if q.Gated() { fmt.Printf(" asked only when %s is %s\n", q.RequiresID, q.RequiresValue) @@ -304,3 +334,72 @@ func isInteractive() bool { func isTerminal(f *os.File) bool { return term.IsTerminal(f.Fd()) } + +// questionByID finds a question in the set the wizard is running, so a caller +// can ask whether the user was actually shown it. +func questionByID(qs []setup.Question, id string) (setup.Question, bool) { + for _, q := range qs { + if q.ID == id { + return q, true + } + } + return setup.Question{}, false +} + +// nextWave picks the questions of this group to put on the next form: those +// whose gate is already satisfied, minus any whose gate is about to be answered +// on that very form. +// +// It takes `asked` as read-only and leaves the marking to the caller, which is +// what makes the deferral work at all. Marking inside the selection pass — as +// this did — defeats it silently whenever a gating question appears BEFORE its +// dependents in the question set, which is the normal way to write one: by the +// time the dependent is examined, the gate it is waiting on has been marked +// asked earlier in the same pass, so it is never held back. The visible symptom +// was a re-run on a pinned config, where autoMode seeds to false and so all of +// step 2 satisfied its gate up front and arrived as one form instead of two — +// and ticking automatic detection on that form then retracted the endpoint +// answer the same form had just collected. +// +// Being a plain function over (questions, answers) rather than a loop body is +// also the only reason this is testable without driving a terminal; +// TestStepTwoArrivesInWaves covers it. +func nextWave(qs []setup.Question, group int, asked map[string]bool, a *setup.Answers) []setup.Question { + var wave []setup.Question + for _, q := range qs { + if q.Group != group || asked[q.ID] || !a.ShouldAsk(q) { + continue + } + if q.Gated() && stillToAsk(qs, q.RequiresID, group, asked, a) { + continue + } + wave = append(wave, q) + } + return wave +} + +// stillToAsk reports whether the question a gate points at is in this same group +// and is genuinely still coming — the only case where deferring is right. +// +// A gate pointing at an EARLIER group is already decided by the time this group +// runs. A gate pointing at a question this run will never show — because that +// question's own gate is unmet — is fixed at its seeded default, so deferring +// for it would strand the dependent question forever: the wave it waits for +// never arrives, the loop runs out of fields and breaks, and the question is +// silently never asked. +// +// That second case is only sound while gates are ONE deep. A gate question that +// is itself gated could become askable later, and releasing its dependent now +// would evaluate it against a seed — the very bug this loop was fixed for, one +// level down. Depth 1 is a property of the question set, not of this function, +// so it is pinned there by TestGatesAreShallowAndPointBackwards; make this +// predicate transitive before adding a gated gate. +func stillToAsk(qs []setup.Question, id string, group int, asked map[string]bool, a *setup.Answers) bool { + for _, q := range qs { + if q.ID != id { + continue + } + return q.Group == group && !asked[q.ID] && a.ShouldAsk(q) + } + return false +} diff --git a/cmd/dezhban/setup_wave_test.go b/cmd/dezhban/setup_wave_test.go new file mode 100644 index 0000000..7ae9bd2 --- /dev/null +++ b/cmd/dezhban/setup_wave_test.go @@ -0,0 +1,154 @@ +package main + +import ( + "reflect" + "strconv" + "testing" + + "github.com/behnam-rk/dezhban/internal/config" + "github.com/behnam-rk/dezhban/internal/setup" +) + +// ids of the questions a wave puts on one form. +func waveIDs(qs []setup.Question) []string { + var out []string + for _, q := range qs { + out = append(out, q.ID) + } + return out +} + +// drive runs the wave loop for one group the way cmdSetup does, without a +// terminal: each wave is recorded, then answered by `answer` before the next. +func drive(qs []setup.Question, group int, a *setup.Answers, answer func(id string)) [][]string { + var waves [][]string + asked := map[string]bool{} + for { + wave := nextWave(qs, group, asked, a) + if len(wave) == 0 { + return waves + } + waves = append(waves, waveIDs(wave)) + for _, q := range wave { + asked[q.ID] = true + answer(q.ID) + } + } +} + +// A huh form binds every field before any is answered, so a question gated on +// another question on the SAME form would be decided by that question's seeded +// default. Step 2 must therefore arrive as two waves — and crucially it must do +// so whichever way autoMode is seeded, because a pinned config seeds it to +// false and so satisfies the manual fields' gate before the user touches it. +func TestStepTwoArrivesInWaves(t *testing.T) { + for _, tc := range []struct { + name string + pinned []string + want [][]string + }{ + { + name: "fresh config, automatic seeded on", + want: [][]string{{"autoMode"}}, + }, + { + name: "pinned config, automatic seeded off", + pinned: []string{"utun9"}, + want: [][]string{{"autoMode"}, {"tunnels", "profileFiles", "endpoints"}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = tc.pinned + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + + // The user answers nothing: every question keeps its seeded value. + got := drive(qs, 2, setup.NewAnswers(qs), func(string) {}) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("waves = %v, want %v", got, tc.want) + } + }) + } +} + +// Off macOS there is no live discovery, so the endpoint question is ungated and +// rides on the FIRST prompt beside the tickbox — a Linux user who leaves +// automatic detection on must still be asked for a server, or the config cannot +// enforce. That makes the wave shape genuinely platform-dependent, which is why +// it is pinned rather than left to the darwin cases above. +func TestOffMacOSTheEndpointQuestionRidesTheFirstWave(t *testing.T) { + for _, goos := range []string{"linux", "windows"} { + t.Run(goos, func(t *testing.T) { + cfg := config.Default() + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: goos}) + a := setup.NewAnswers(qs) + + // Automatic detection left on, the recommended answer. + got := drive(qs, 2, a, func(string) {}) + want := [][]string{{"autoMode", "endpoints"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("waves = %v, want %v", got, want) + } + }) + } +} + +// Ticking automatic detection retracts the manual half of step 2. The three +// gated questions must then never be put on a form at all — the wave loop is +// what makes that true, and it is the case the old mark-inside-the-pass loop got +// wrong by shipping all four on one form. +func TestTickingAutomaticRetractsTheManualFields(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = []string{"utun9"} // seeds autoMode false + cfg.VPN.Endpoints = []string{"203.0.113.7"} + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + a := setup.NewAnswers(qs) + + seen := map[string]bool{} + drive(qs, 2, a, func(id string) { + seen[id] = true + if id == "autoMode" { + a.Set("autoMode", "true") // the user ticks it after all + } + }) + + if !seen["autoMode"] { + t.Fatal("autoMode was never asked") + } + for _, id := range []string{"tunnels", "profileFiles", "endpoints"} { + if seen[id] { + t.Errorf("%s was put on a form despite automatic detection being on", id) + } + } + + // And nothing it would have written may reach the config. + after := cfg + in := a.Input(strconv.Itoa(cfg.Hysteresis), nil) + in.MacOS, in.ConfigExisted = true, true + setup.Apply(&after, in) + config.Normalize(&after) + if got := after.VPN.Endpoints; !reflect.DeepEqual(got, []string{"203.0.113.7"}) { + t.Errorf("endpoints = %v, want the configured one untouched", got) + } +} + +// Every question whose gate ends up satisfied must have been put on some form. +func TestEveryGatedQuestionIsReachable(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = []string{"utun9"} + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + a := setup.NewAnswers(qs) + + seen := map[string]bool{} + drive(qs, 2, a, func(id string) { + seen[id] = true + if id == "autoMode" { + a.Set("autoMode", "false") + } + }) + for _, q := range qs { + if q.Group == 2 && a.ShouldAsk(q) && !seen[q.ID] { + t.Errorf("%s has a satisfied gate but was never asked", q.ID) + } + } +} diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index eeb7494..84d23ad 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -724,18 +724,44 @@ macOS only, privileged (`dezhban upgrade download`/`apply`). See forms are bound to the same answers. - [ ] Re-running it **without** naming profile files keeps the profiles already imported (`dezhban vpn list`). +- [ ] **Two steps.** Step 1 is countries. Step 2 opens with "Use automatic VPN + detection?". **On macOS**, leaving it ticked ends the wizard there, and + unticking it asks for tunnel interfaces, self-hosted config files, and + endpoints as a follow-up prompt rather than on the same screen. + **Off macOS** the endpoint question is ungated — there is no live discovery + to find a server — so it rides on that first prompt beside the tickbox, and + unticking brings only tunnel interfaces and config files. +- [ ] **A re-run on a pinned config keeps its pins.** With + `vpn.tunnelInterfaces` set, re-run and press Enter through everything: + automatic detection must arrive **already unticked**, and + `dezhban config show` must still list the same interfaces. This is what + replaced the old "Configure your VPN now?" escape, so it is the check that + matters most in this section. Run it **twice: once with the VPN up and once + with it down.** Detection only sees tunnels that are up, so the down run is + the one where the pinned interface has to appear in the pick list — and stay + ticked — on its own. +- [ ] **A re-run under automatic detection keeps configured endpoints.** With + `vpn.endpoints` set and no pinned interfaces, re-run, leave automatic + detection ticked, finish: the endpoints are unchanged. The question was + never asked, so nothing may have been written. +- [ ] **Off macOS the endpoint question always appears.** On Linux or Windows, + leave automatic detection ticked — you must still be asked for an endpoint, + because there is no live discovery to find one. - [ ] `dezhban setup --questions --json` runs with no TTY, no root, and no config file present, and lists the same questions the wizard asks. ### First-run wizard (macOS app) -- [ ] With no VPN configured and `defaults delete com.dezhban.menu +- [ ] With no VPN configured and `defaults delete com.behnam-rk.dezhban.app dezhban.firstRunCompleted`, launching the app opens the window **and** the wizard. With a VPN already configured from the CLI, it does not — the questions were already answered. - [ ] The questions, their order, and the gating match `dezhban setup` run in a - terminal on the same host. Declining "Configure your VPN now?" skips the - whole VPN branch in both. + terminal on the same host. Unticking "Use automatic VPN detection?" reveals + the same three manual fields in both — in the app they appear **on the same + screen**, without paging forward. +- [ ] **Two steps, labelled as two.** The step counter reads "Step 1 of 2" and + "Step 2 of 2"; unticking automatic detection must not add a third. - [ ] Saving writes through one `config set` (one password prompt, or none with a token enrolled) and the values land in `dezhban config show`. Choosing automatic detection leaves `vpn.tunnelInterfaces` **empty**. diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 4212df9..7bdbebb 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -331,15 +331,43 @@ succeeds and says so; the new values are read the next time it starts. validation, and ruleset preview as `detect-vpn`/`validate`/`print-rules`. Writes to the system path need root (hence `sudo`); a permission error prints a `sudo` hint. -The wizard asks only what has no safe default: blocked countries (plus a -free-text field for other codes), whether to configure the VPN now, automatic -vs. manual detection, and — when configuring — tunnel interfaces (manual mode -only), self-hosted config files to import, and endpoints. Everything it used -to also ask (poll interval, log level, provider quorum, physical DNS, -auto-discovery) ships with a sane default and lives in the app's Settings or -`config set`; a wizard run leaves those keys untouched, so re-running setup -never clobbers a tuned value. The one silent defaulting decision it kept: a -brand-new macOS config gets live endpoint discovery turned on. +The wizard is **two steps**, and asks only what has no safe default: + +1. **Blocked countries** — a checklist of the common ones, plus a free-text + field for any other ISO codes. +2. **Automatic VPN detection?** — on by default. Leave it on and dezhban finds + the tunnel and, on macOS, learns the server address itself. Untick it and the + manual fields appear: tunnel interfaces, self-hosted config files to import, + and endpoints. + +Everything it used to also ask (poll interval, log level, provider quorum, +physical DNS, auto-discovery) ships with a sane default and lives in the app's +Settings or `config set`; a wizard run leaves those keys untouched, so +re-running setup never clobbers a tuned value. **A question that is not asked +writes no key** — so leaving automatic detection on does not blank the endpoints +of someone who set them by hand. The exception is off macOS, where there is no +live discovery: the endpoint question is asked whichever detection mode you +pick, because without it the config cannot enforce. + +Two consequences of there being no "configure your VPN now?" question, which +this wizard used to open with. A run always writes the VPN keys, so the +automatic-detection answer is **seeded from your config**: a config with pinned +`vpn.tunnelInterfaces` starts on manual, and pressing Enter through the wizard +preserves the pins rather than clearing them — including while that tunnel is +down, since detection only sees tunnels that are up and the pick list would +otherwise not contain your own pinned interface. And choosing automatic detection +deliberately clears those pins, because a leftover pin is precisely what stops +autodetection from happening. + +The one silent defaulting decision it kept: a brand-new macOS config gets live +endpoint discovery turned on. + +In a terminal, unticking automatic detection brings the manual fields as a +second prompt rather than revealing them in place — the form library binds every +field before any is answered, so a question that appears only when you untick +another has to come after it. Leave automatic detection on and step 2 is the one +prompt. The macOS app re-evaluates as you type and shows step 2 as a single +screen either way. `setup --questions` is the exception: it prints what the wizard *would* ask — each question, what it writes, its seeded answer, and which earlier answer diff --git a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift index e183bd4..5a0b029 100644 --- a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift +++ b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift @@ -141,14 +141,20 @@ public struct SetupAnswers { /// The `key=value` pairs one batched `config set` should write. /// - /// Three rules are not derivable from a question's `key` alone, and they + /// Two rules are not derivable from a question's `key` alone, and they /// mirror Go's `setup.Apply` exactly: /// - the free-text country codes fold into `blockedCountries`; - /// - answering "no" to the VPN branch writes none of its keys, so a VPN - /// somebody already configured is left alone; /// - choosing automatic detection CLEARS pinned interfaces rather than /// skipping the key, because a leftover pin is what makes autodetect not /// happen. + /// + /// The third rule is `shouldAsk` itself, and it is load-bearing now that + /// there is no "configure your VPN now?" question to skip the branch: a key + /// whose question was never shown must not be written. On macOS the + /// endpoint question is gated behind "not automatic", so a re-run that + /// leaves automatic detection on produces no `vpn.endpoints=` pair at all — + /// which is what keeps it from blanking a configured server. Go's `Apply` + /// achieves the same with a nil `Input.Endpoints`. public func configPairs(for questions: [SetupQuestion]) -> [String] { var pairs: [String] = [] for q in questions where shouldAsk(q) && !q.key.isEmpty { @@ -162,7 +168,7 @@ public struct SetupAnswers { pairs.append("\(q.key)=\(self[q.id])") } } - if bool("configureVPN") && bool("autoMode") { + if bool("autoMode") { pairs.append("vpn.tunnelInterfaces=") } return pairs @@ -170,7 +176,20 @@ public struct SetupAnswers { /// The VPN config files to import, which are not a config key at all — they /// become profiles through `dezhban vpn import`. - public var profileFiles: [String] { list("profileFiles") } + /// + /// Gated, for the same reason `configPairs` skips an unasked key: this step + /// reveals in place and re-evaluates as answers change, so someone can untick + /// automatic detection, choose files, then tick it again — the field goes + /// away but the answer it collected does not. Importing those would enact a + /// choice the user visibly withdrew, and Go's wizard does not (it reads this + /// answer only when `ShouldAsk` holds). Taking the question set rather than + /// reading the stored answer blind is what keeps the two in step. + public func profileFiles(for questions: [SetupQuestion]) -> [String] { + guard let q = questions.first(where: { $0.id == "profileFiles" }), shouldAsk(q) else { + return [] + } + return list("profileFiles") + } } /// When the first-run wizard should be offered. diff --git a/gui/macos/Sources/DezhbanMenu/FirstRunView.swift b/gui/macos/Sources/DezhbanMenu/FirstRunView.swift index 7cf9b99..e85e7ef 100644 --- a/gui/macos/Sources/DezhbanMenu/FirstRunView.swift +++ b/gui/macos/Sources/DezhbanMenu/FirstRunView.swift @@ -248,7 +248,7 @@ struct FirstRunView: View { } return } - let files = answers.profileFiles + let files = answers.profileFiles(for: questions) FirstRun.markComplete() guard !files.isEmpty else { done(true) diff --git a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift index 99f1ada..30cefd9 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift @@ -19,16 +19,15 @@ struct SetupQuestionsTests { "options":[{"label":"Iran (IR)","value":"IR"},{"label":"Russia (RU)","value":"RU"}], "selected":["IR"],"group":1}, {"id":"otherCountries","kind":"list","title":"Other country codes","default":"AQ","group":1}, - {"id":"configureVPN","kind":"bool","title":"Configure your VPN now?","default":"true","group":1}, {"id":"autoMode","kind":"bool","title":"Use automatic VPN detection? (recommended)", - "default":"true","group":2,"requiresId":"configureVPN","requiresValue":"true"}, + "default":"true","group":2}, {"id":"tunnels","key":"vpn.tunnelInterfaces","kind":"multiselect","title":"Tunnel interface(s)", "options":[{"label":"utun4","value":"utun4"},{"label":"utun7","value":"utun7"}], - "selected":["utun4"],"group":3,"requiresId":"autoMode","requiresValue":"false"}, - {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":4, - "requiresId":"configureVPN","requiresValue":"true"}, + "selected":["utun4"],"group":2,"requiresId":"autoMode","requiresValue":"false"}, + {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":2, + "requiresId":"autoMode","requiresValue":"false"}, {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)", - "default":"203.0.113.7","group":4,"requiresId":"configureVPN","requiresValue":"true"} + "default":"203.0.113.7","group":2,"requiresId":"autoMode","requiresValue":"false"} ] """ @@ -38,13 +37,16 @@ struct SetupQuestionsTests { @Test func decodesTheDaemonsQuestions() throws { let qs = try Self.questions() - #expect(qs.count == 8) + #expect(qs.count == 7) let countries = try #require(qs.first { $0.id == "blockedCountries" }) #expect(countries.selected == ["IR"]) #expect(countries.options.map(\.value) == ["IR", "RU"]) // Absent `key` decodes as "no config key", not as a decode failure. #expect(try #require(qs.first { $0.id == "otherCountries" }).key.isEmpty) - #expect(try #require(qs.first { $0.id == "autoMode" }).isGated) + // autoMode is the gate now, not a gated question — everything manual + // hangs off it, and it hangs off nothing. + #expect(!(try #require(qs.first { $0.id == "autoMode" }).isGated)) + #expect(try #require(qs.first { $0.id == "endpoints" }).isGated) #expect(!(try #require(qs.first { $0.id == "pollInterval" }).isGated)) } @@ -52,23 +54,46 @@ struct SetupQuestionsTests { let a = SetupAnswers(questions: try Self.questions()) #expect(a["pollInterval"] == "15s") #expect(a.list("blockedCountries") == ["IR"]) - #expect(a.bool("configureVPN")) + #expect(a.bool("autoMode")) + } + + /// Two steps, matching Go's TestTheWizardIsTwoGroups. The app renders one + /// group per step, so a third group is a third screen. + @Test func theWizardIsTwoSteps() throws { + #expect(Set(try Self.questions().map(\.group)) == [1, 2]) } + /// Step 2 is one automatic-detection tickbox with every manual field hanging + /// off it — the reveal-in-place the app renders, and the same gate the CLI + /// evaluates. @Test func gatingMatchesTheCLI() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "false" - for q in qs where q.requiresID == "configureVPN" { - #expect(!a.shouldAsk(q), "\(q.id) should be hidden when the VPN branch is declined") + a["autoMode"] = "true" + for q in qs where q.requiresID == "autoMode" { + #expect(!a.shouldAsk(q), "\(q.id) should be hidden under automatic detection") } - a["configureVPN"] = "true" - a["autoMode"] = "true" - #expect(!a.shouldAsk(try #require(qs.first { $0.id == "tunnels" }))) a["autoMode"] = "false" - #expect(a.shouldAsk(try #require(qs.first { $0.id == "tunnels" }))) + for id in ["tunnels", "profileFiles", "endpoints"] { + #expect(a.shouldAsk(try #require(qs.first { $0.id == id })), + "\(id) should be asked once automatic detection is unticked") + } + } + + /// The rule that replaced "configure your VPN now?": a question that was + /// never shown writes no key. Under automatic detection on macOS the + /// endpoint question is hidden, so a re-run must produce no + /// `vpn.endpoints=` pair — writing an empty one would delete a configured + /// server. Mirrors Go's TestAnUnaskedEndpointListTouchesNoEndpoint. + @Test func anUnaskedQuestionWritesNoKey() throws { + let qs = try Self.questions() + var a = SetupAnswers(questions: qs) + a["autoMode"] = "true" + + let pairs = a.configPairs(for: qs) + #expect(!pairs.contains { $0.hasPrefix("vpn.endpoints=") }) } /// The free-text codes fold into the same key as the checkboxes — they are @@ -91,7 +116,6 @@ struct SetupQuestionsTests { @Test func automaticDetectionClearsPinnedInterfaces() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "true" a["autoMode"] = "true" let pairs = a.configPairs(for: qs) @@ -101,7 +125,6 @@ struct SetupQuestionsTests { @Test func pinningWritesTheChosenInterfaces() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "true" a["autoMode"] = "false" a["tunnels"] = "utun4,utun7" @@ -111,17 +134,26 @@ struct SetupQuestionsTests { "a pinned config must not also be cleared") } - /// Declining the VPN branch writes none of its keys, so a VPN somebody - /// already configured is left alone — the same rule as Go's setup.Apply. - @Test func decliningTheVPNBranchWritesNoVPNKey() throws { - let qs = try Self.questions() - var a = SetupAnswers(questions: qs) - a["configureVPN"] = "false" + /// A wizard seeded with `autoMode: false` — which the daemon does whenever + /// vpn.tunnelInterfaces is pinned — and clicked straight through must write + /// those pins back, not clear them. This is the consumer side of Go's + /// TestAutoModeSeedsFalseWhenInterfacesArePinned: the app renders whatever + /// default arrives, so the guard only holds if seeding drives it. + @Test func aSeededManualModeReWritesItsPins() throws { + let qs = try Self.questions().map { q -> SetupQuestion in + guard q.id == "autoMode" else { return q } + return SetupQuestion(questionID: q.questionID, key: q.key, kind: q.kind, + title: q.title, description: q.description, + options: q.options, defaultValue: "false", + selected: q.selected, group: q.group, + requiresID: q.requiresID, requiresValue: q.requiresValue) + } + let a = SetupAnswers(questions: qs) + #expect(!a.bool("autoMode"), "the seeded default must drive the answer") let pairs = a.configPairs(for: qs) - #expect(!pairs.contains { $0.hasPrefix("vpn.") }) - // The answers that were given still apply. - #expect(pairs.contains { $0.hasPrefix("pollInterval=") }) + #expect(pairs.contains("vpn.tunnelInterfaces=utun4")) + #expect(!pairs.contains("vpn.tunnelInterfaces=")) } /// Profile files are not a config key: they become profiles through @@ -131,10 +163,25 @@ struct SetupQuestionsTests { var a = SetupAnswers(questions: qs) a["profileFiles"] = "/tmp/home.conf, /tmp/work.ovpn" - #expect(a.profileFiles == ["/tmp/home.conf", "/tmp/work.ovpn"]) + a["autoMode"] = "false" + #expect(a.profileFiles(for: qs) == ["/tmp/home.conf", "/tmp/work.ovpn"]) #expect(!a.configPairs(for: qs).contains { $0.contains("profileFiles") }) } + /// The field is revealed in place and re-evaluated live, so a user can pick + /// files and then tick automatic detection again. The answer survives in the + /// dictionary; acting on it would import files the user visibly withdrew. + @Test func withdrawnProfileFilesAreNotImported() throws { + let qs = try Self.questions() + var a = SetupAnswers(questions: qs) + a["autoMode"] = "false" + a["profileFiles"] = "/tmp/home.conf" + #expect(a.profileFiles(for: qs) == ["/tmp/home.conf"]) + + a["autoMode"] = "true" // the field disappears again + #expect(a.profileFiles(for: qs).isEmpty) + } + @Test func firstRunIsOfferedOnlyWhenNothingIsKnownYet() { // Whether the flag is set is UserDefaults' business; whether the wizard // should be offered given that flag is a rule, and lives in the core. @@ -147,8 +194,7 @@ struct SetupQuestionsTests { // MARK: - the shrunk wizard /// The daemon's question list after the 2026-08 shrink: blocked countries, - /// configure-VPN?, auto-vs-manual, and the gated VPN details — nothing - /// else. Everything above must keep working with this list, because the + /// auto-vs-manual and the gated VPN details — nothing else. Everything above must keep working with this list, because the /// view renders whatever arrives, and the id-keyed special cases /// (blockedCountries+otherCountries fold, autoMode's tunnel clearing) must /// hold with the surrounding questions gone. @@ -158,16 +204,15 @@ struct SetupQuestionsTests { "options":[{"label":"Iran (IR)","value":"IR"},{"label":"Russia (RU)","value":"RU"}], "selected":["IR"],"group":1}, {"id":"otherCountries","kind":"list","title":"Other country codes","default":"AQ","group":1}, - {"id":"configureVPN","kind":"bool","title":"Configure your VPN now?","default":"true","group":1}, {"id":"autoMode","kind":"bool","title":"Use automatic VPN detection? (recommended)", - "default":"true","group":2,"requiresId":"configureVPN","requiresValue":"true"}, + "default":"true","group":2}, {"id":"tunnels","key":"vpn.tunnelInterfaces","kind":"multiselect","title":"Tunnel interface(s)", - "options":[{"label":"utun4","value":"utun4"}],"selected":[],"group":3, + "options":[{"label":"utun4","value":"utun4"}],"selected":[],"group":2, "requiresId":"autoMode","requiresValue":"false"}, - {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":4, - "requiresId":"configureVPN","requiresValue":"true"}, - {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)","group":4, - "requiresId":"configureVPN","requiresValue":"true"} + {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":2, + "requiresId":"autoMode","requiresValue":"false"}, + {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)","group":2, + "requiresId":"autoMode","requiresValue":"false"} ] """ @@ -177,10 +222,10 @@ struct SetupQuestionsTests { @Test func shrunkListDecodesAndGates() throws { let qs = try Self.shrunkQuestions() - #expect(qs.count == 7) + #expect(qs.count == 6) var a = SetupAnswers(questions: qs) - // Default flow: configure yes, automatic yes — the tunnel question is - // never asked. + // Default flow: automatic detection on — the tunnel question is never + // asked. let tunnels = try #require(qs.first { $0.id == "tunnels" }) #expect(!a.shouldAsk(tunnels)) a["autoMode"] = "false" @@ -205,9 +250,15 @@ struct SetupQuestionsTests { let qs = try Self.shrunkQuestions() var a = SetupAnswers(questions: qs) a["endpoints"] = "203.0.113.7" - let pairs = a.configPairs(for: qs) - // configureVPN + autoMode both default true → pinned interfaces cleared. + var pairs = a.configPairs(for: qs) + // autoMode defaults true → pinned interfaces cleared, and the endpoint + // question is not asked, so its answer is not written even though one + // was set. #expect(pairs.contains("vpn.tunnelInterfaces=")) + #expect(!pairs.contains { $0.hasPrefix("vpn.endpoints=") }) + + a["autoMode"] = "false" + pairs = a.configPairs(for: qs) #expect(pairs.contains("vpn.endpoints=203.0.113.7")) } } diff --git a/internal/setup/answers.go b/internal/setup/answers.go index bfecefd..831bb76 100644 --- a/internal/setup/answers.go +++ b/internal/setup/answers.go @@ -143,13 +143,20 @@ func (a *Answers) ShouldAsk(q Question) bool { // Input is the collected answers, in the shape the config wants them. type Input struct { - Hysteresis string - Countries []string - ConfigureVPN bool + Hysteresis string + Countries []string // AutoMode is automatic tunnel detection: no pinned interface names. - AutoMode bool - Tunnels, Endpoints []string - Profiles []config.Profile + AutoMode bool + Tunnels []string + // Endpoints is nil when the wizard never asked — on macOS the question is + // gated behind "not automatic", because live discovery learns the server + // address there. Nil rather than empty for the same reason AutoDiscover is + // a pointer: Apply must leave an unasked key ALONE, and an empty slice is + // indistinguishable from "asked, and cleared on purpose". Writing it + // unconditionally would blank the endpoints of anyone who re-ran setup and + // left automatic detection on. + Endpoints *[]string + Profiles []config.Profile // AutoDiscover is nil when nothing answered it — the wizard no longer asks; // a surface that still collects an explicit answer can set it. Nil rather // than false because Apply must leave an unanswered key ALONE: writing @@ -178,26 +185,51 @@ func (a *Answers) Input(hysteresis string, profiles []config.Profile) Input { // The no-tunnels-detected form of the question is free text. tunnels = SplitList(a.Text("tunnels")) } + var endpoints *[]string + if a.wasAsked("endpoints") { + eps := SplitList(a.Text("endpoints")) + endpoints = &eps + } return Input{ - Hysteresis: hysteresis, - Countries: countries, - ConfigureVPN: a.Bool("configureVPN"), - AutoMode: a.Bool("autoMode"), - Tunnels: tunnels, - Endpoints: SplitList(a.Text("endpoints")), - Profiles: profiles, + Hysteresis: hysteresis, + Countries: countries, + AutoMode: a.Bool("autoMode"), + Tunnels: tunnels, + Endpoints: endpoints, + Profiles: profiles, // Nil unless a surface asked the (no-longer-offered) question anyway; // nil leaves the configured value untouched in Apply. AutoDiscover: a.OptionalBool("autoDiscover"), } } +// wasAsked reports whether the question with this id exists and its gate is +// satisfied by the FINAL answers collected. The question set is retained by +// NewAnswers precisely so this does not have to be re-derived by every caller. +// +// That stands in for "the user saw it", and the two agree only because a gate +// never points forward: every gate names an ungated question in the same or an +// earlier group, so by the time a gated question is put to the user its gate is +// already answered and cannot move afterwards. A gate pointing at a LATER +// group's answer would be read here against that question's seeded default, +// and this would report a question asked that nobody was shown — writing its +// key from a seed. TestGatesAreShallowAndPointBackwards pins the shape. +func (a *Answers) wasAsked(id string) bool { + for _, q := range a.asked { + if q.ID == id { + return a.ShouldAsk(q) + } + } + return false +} + // Apply writes collected answers onto cfg. Validation happens after, by the // caller: this only assembles. // // A question the user never reached leaves its part of the config alone. That -// is why the VPN keys are written only when ConfigureVPN is true — answering -// "no" must not blank out a tunnel someone configured earlier. +// is why Endpoints is a pointer: on macOS the question is gated behind "not +// automatic", and an unasked endpoint list must not blank out a server someone +// configured earlier. // Keys the wizard no longer asks about (pollInterval, logLevel, // providerQuorum, vpn.allowPhysicalDNS) are deliberately not assigned at all: // unasked means untouched, so re-running setup can never clobber a value tuned @@ -208,9 +240,6 @@ func Apply(cfg *config.Config, in Input) { } cfg.BlockedCountries = in.Countries // config.Normalize upper-cases and de-dupes on save - if !in.ConfigureVPN { - return - } if in.AutoMode { // Automatic detection: no pinned interface names (Normalize implies // autodetect), plus live discovery where supported. @@ -218,7 +247,9 @@ func Apply(cfg *config.Config, in Input) { } else { cfg.VPN.TunnelInterfaces = in.Tunnels } - cfg.VPN.Endpoints = in.Endpoints + if in.Endpoints != nil { + cfg.VPN.Endpoints = *in.Endpoints + } cfg.VPN.Profiles = mergeProfiles(cfg.VPN.Profiles, in.Profiles) switch { case in.AutoDiscover != nil: diff --git a/internal/setup/questions.go b/internal/setup/questions.go index a3a9ad7..c83a9b0 100644 --- a/internal/setup/questions.go +++ b/internal/setup/questions.go @@ -51,8 +51,8 @@ type Question struct { // JSON use it, so it is a compatibility surface: do not rename one. ID string `json:"id"` // Key is the dotted config key this answer writes, empty when the question - // only steers the flow (ConfigureVPN, AutoMode) or is folded into another - // key's value (OtherCountries). + // only steers the flow (AutoMode) or is folded into another key's value + // (OtherCountries). Key string `json:"key,omitempty"` Kind string `json:"kind"` Title string `json:"title"` @@ -145,12 +145,38 @@ func Questions(opts Options) []Question { endpointDesc = "Server IP(s)/hostname(s), comma-separated. Required on this platform (no live discovery)." } + // Automatic detection is the recommendation, but never at the cost of + // silently unpinning interfaces someone chose on purpose: a config with + // pinned vpn.tunnelInterfaces seeds this to false, so clicking straight + // through a re-run preserves them. This is load-bearing now that there is + // no "configure your VPN?" question to skip the whole branch — + // TestAutoModeSeedsFalseWhenInterfacesArePinned pins it. + autoModeDefault := "true" + if len(cfg.VPN.TunnelInterfaces) > 0 { + autoModeDefault = "false" + } + + // Endpoints are gated behind "not automatic" on macOS, where live discovery + // learns the server address. Everywhere else there is no discovery, so the + // endpoint is required whichever detection mode is chosen and the question + // is ungated — a Linux host that picked automatic detection and was never + // asked for a server would end up with a config that cannot enforce. + endpointsRequires, endpointsRequiresValue := "autoMode", "false" + if !macOS { + endpointsRequires, endpointsRequiresValue = "", "" + } + // The wizard asks only what has no safe default: what to block, and how to // find the VPN. Everything it used to also ask (poll interval, log level, // provider quorum, physical DNS, auto-discovery) ships with a sane default, // lives in Settings/`config set`, and — critically — is left UNTOUCHED by a // wizard run, so re-running setup never clobbers a tuned value // (TestAnUnaskedQuestionLeavesItsKeyAlone pins this). + // + // Two groups, which is two steps: what to block, then how to find the VPN. + // Everything in group 2 hangs off the one automatic-detection question, so + // unticking it reveals the manual fields in place rather than paging to + // another screen. return []Question{ { ID: "blockedCountries", Key: "blockedCountries", Kind: KindMultiSelect, Group: 1, @@ -165,37 +191,29 @@ func Questions(opts Options) []Question { Description: "Comma-separated ISO codes not listed above (optional).", Default: strings.Join(extra, ","), }, - { - ID: "configureVPN", Kind: KindBool, Group: 1, - Title: "Configure your VPN now?", - Description: "dezhban only enforces once it knows your VPN's tunnel and server. " + - "Say no and it starts in standby — fully open, nothing blocked — until you " + - "run 'dezhban setup' again or edit the config.", - Default: "true", - }, { ID: "autoMode", Kind: KindBool, Group: 2, Title: "Use automatic VPN detection? (recommended)", Description: "dezhban finds your tunnel and, on macOS, learns the server address " + - "itself — works with any VPN and survives redials.", - Default: "true", - RequiresID: "configureVPN", - RequiresValue: "true", + "itself — works with any VPN and survives redials. Untick it to name your " + + "tunnel and server yourself.", + Default: autoModeDefault, }, tunnelQuestion(opts.DetectedTunnels, cfg.VPN.TunnelInterfaces), { - ID: "profileFiles", Kind: KindList, Group: 4, + ID: "profileFiles", Kind: KindList, Group: 2, Title: "Self-hosted VPN config files", Description: "Comma-separated paths to WireGuard/.conf, OpenVPN/.ovpn, or V2Ray " + "JSON to import as profiles (optional).", - RequiresID: "configureVPN", RequiresValue: "true", + RequiresID: "autoMode", RequiresValue: "false", }, { - ID: "endpoints", Key: "vpn.endpoints", Kind: KindList, Group: 4, - Title: "VPN endpoint(s)", - Description: endpointDesc, - Default: strings.Join(cfg.VPN.Endpoints, ","), - RequiresID: "configureVPN", RequiresValue: "true", + ID: "endpoints", Key: "vpn.endpoints", Kind: KindList, Group: 2, + Title: "VPN endpoint(s)", + Description: endpointDesc, + Default: strings.Join(cfg.VPN.Endpoints, ","), + RequiresID: endpointsRequires, + RequiresValue: endpointsRequiresValue, }, } } @@ -204,7 +222,7 @@ func Questions(opts Options) []Question { // none were — the same split the CLI's tunnelSelector used to make on its own. func tunnelQuestion(detected, configured []string) Question { q := Question{ - ID: "tunnels", Key: "vpn.tunnelInterfaces", Group: 3, + ID: "tunnels", Key: "vpn.tunnelInterfaces", Group: 2, Title: "Tunnel interface(s)", RequiresID: "autoMode", RequiresValue: "false", } @@ -214,16 +232,52 @@ func tunnelQuestion(detected, configured []string) Question { q.Default = strings.Join(configured, ",") return q } + q.Kind = KindMultiSelect + + // A pinned interface is an option even when it is not detected right now. + // Detection only sees tunnels that are UP, so a re-run while the VPN is + // down would otherwise offer a list its own configured interface is not in + // — and pressing Enter through that list answers "none of them", which + // Apply writes as an empty vpn.tunnelInterfaces. That silently unpins an + // interface someone chose deliberately, which is the very thing seeding + // autoMode to false from those pins exists to prevent; offering the pins + // but leaving them unselectable would close only half of it. + // TestAPinnedTunnelSurvivesADetectionMiss pins this. + // + // Those extras are labelled, because a list headed "detected" containing a + // ticked interface that is plainly not up reads as a bug in the detector. + // The label carries it; the Value stays the bare interface name, which is + // what gets written. + detectedSet := map[string]bool{} + for _, t := range detected { + detectedSet[t] = true + } + seen := map[string]bool{} + offDuty := false + for _, t := range append(append([]string(nil), detected...), configured...) { + if seen[t] { + continue + } + seen[t] = true + label := t + if !detectedSet[t] { + label = t + " (configured, not up right now)" + offDuty = true + } + q.Options = append(q.Options, Option{Label: label, Value: t}) + } + q.Description = "Detected tunnels — pick the VPN's." + if offDuty { + q.Description = "Pick the VPN's. Interfaces you already configured are listed " + + "and kept ticked even while they are down." + } cfgSet := map[string]bool{} for _, t := range configured { cfgSet[t] = true } - q.Kind = KindMultiSelect - q.Description = "Detected tunnels — pick the VPN's." - for _, t := range detected { - q.Options = append(q.Options, Option{Label: t, Value: t}) - if cfgSet[t] { - q.Selected = append(q.Selected, t) + for _, o := range q.Options { + if cfgSet[o.Value] { + q.Selected = append(q.Selected, o.Value) } } return q diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index d2f1f4b..626747b 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -15,9 +15,9 @@ func TestApplyAutoMode(t *testing.T) { cfg := config.Default() Apply(&cfg, Input{ Hysteresis: "3", - ConfigureVPN: true, AutoMode: true, + AutoMode: true, Tunnels: []string{"utun9"}, // must be ignored in auto mode - Endpoints: []string{"vpn.example.com"}, + Endpoints: eps("vpn.example.com"), Profiles: []config.Profile{{Name: "home", Endpoints: []string{"203.0.113.7"}}}, AutoDiscover: boolPtr(true), }) @@ -93,26 +93,33 @@ func TestAnUnaskedQuestionLeavesItsKeyAlone(t *testing.T) { func TestApplyAdvancedPin(t *testing.T) { cfg := config.Default() Apply(&cfg, Input{ - Hysteresis: "3", - ConfigureVPN: true, AutoMode: false, - Tunnels: []string{"utun4"}, - Endpoints: []string{"203.0.113.7"}, + Hysteresis: "3", + AutoMode: false, + Tunnels: []string{"utun4"}, + Endpoints: eps("203.0.113.7"), }) if len(cfg.VPN.TunnelInterfaces) != 1 || cfg.VPN.TunnelInterfaces[0] != "utun4" { t.Errorf("advanced mode should pin utun4, got %v", cfg.VPN.TunnelInterfaces) } } -// Answering "no" to "configure your VPN now?" must leave a VPN somebody already -// set up completely alone — the wizard is also how people change their -// blocked-country list. -func TestDecliningTheVPNBranchTouchesNoVPNKey(t *testing.T) { +// An UNASKED endpoint question must leave a configured server alone. +// +// This replaced the "configure your VPN now?" question as the thing standing +// between a re-run and someone's working config. On macOS the endpoint question +// is gated behind "not automatic", so a user who re-runs setup to change their +// blocked-country list — and leaves automatic detection on, as recommended — +// reaches Apply with no endpoint answer at all. Writing that as an empty list +// would delete their server. +func TestAnUnaskedEndpointListTouchesNoEndpoint(t *testing.T) { cfg := config.Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} cfg.VPN.Endpoints = []string{"203.0.113.7"} cfg.VPN.AllowPhysicalDNS = true - Apply(&cfg, Input{Countries: []string{"IR", "SY"}, ConfigureVPN: false}) + // Endpoints nil is what Input produces when the question was never shown. + Apply(&cfg, Input{Countries: []string{"IR", "SY"}, AutoMode: false, + Tunnels: []string{"utun4"}}) if !reflect.DeepEqual(cfg.VPN.TunnelInterfaces, []string{"utun4"}) { t.Errorf("tunnels changed: %v", cfg.VPN.TunnelInterfaces) @@ -139,13 +146,13 @@ func TestImportedProfilesAddToTheSavedOnes(t *testing.T) { } // A run that imported nothing keeps both. - Apply(&cfg, Input{ConfigureVPN: true, AutoMode: true}) + Apply(&cfg, Input{AutoMode: true}) if len(cfg.VPN.Profiles) != 2 { t.Fatalf("a run importing nothing must keep saved profiles, got %+v", cfg.VPN.Profiles) } // A run that re-imports one replaces that one and keeps the other. - Apply(&cfg, Input{ConfigureVPN: true, AutoMode: true, + Apply(&cfg, Input{AutoMode: true, Profiles: []config.Profile{{Name: "work", Endpoints: []string{"192.0.2.5"}}}}) if len(cfg.VPN.Profiles) != 2 { t.Fatalf("re-importing a profile must not drop the others, got %+v", cfg.VPN.Profiles) @@ -194,21 +201,21 @@ func TestQuestionsSeedFromTheConfig(t *testing.T) { func TestAutoDiscoverDefaultsOnlyForANewMacConfig(t *testing.T) { fresh := config.Default() fresh.VPN.AutoDiscoverEndpoints = false // Default() has it on; force the observable flip - Apply(&fresh, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: false}) + Apply(&fresh, Input{AutoMode: true, MacOS: true, ConfigExisted: false}) if !fresh.VPN.AutoDiscoverEndpoints { t.Error("a brand-new macOS config should get discovery on") } existing := config.Default() existing.VPN.AutoDiscoverEndpoints = false - Apply(&existing, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: true}) + Apply(&existing, Input{AutoMode: true, MacOS: true, ConfigExisted: true}) if existing.VPN.AutoDiscoverEndpoints { t.Error("an existing config's explicit false must be preserved") } linux := config.Default() linux.VPN.AutoDiscoverEndpoints = false - Apply(&linux, Input{ConfigureVPN: true, AutoMode: true, MacOS: false, ConfigExisted: false}) + Apply(&linux, Input{AutoMode: true, MacOS: false, ConfigExisted: false}) if linux.VPN.AutoDiscoverEndpoints { t.Error("discovery is macOS-only; a new Linux config must not have it defaulted on") } @@ -217,7 +224,7 @@ func TestAutoDiscoverDefaultsOnlyForANewMacConfig(t *testing.T) { answered := config.Default() answered.VPN.AutoDiscoverEndpoints = true off := false - Apply(&answered, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: false, AutoDiscover: &off}) + Apply(&answered, Input{AutoMode: true, MacOS: true, ConfigExisted: false, AutoDiscover: &off}) if answered.VPN.AutoDiscoverEndpoints { t.Error("an explicit false answer must win over the new-config default") } @@ -246,34 +253,149 @@ func TestTunnelQuestionFollowsDetection(t *testing.T) { } } +// Detection only sees tunnels that are UP. A re-run while the VPN is down must +// still offer — and preselect — the interface the config pins, or pressing +// Enter through the pick list answers "none of them" and unpins it. +func TestAPinnedTunnelSurvivesADetectionMiss(t *testing.T) { + cfg := config.Default() + cfg.BlockedCountries = []string{"IR"} + cfg.VPN.Endpoints = []string{"203.0.113.7"} + cfg.VPN.TunnelInterfaces = []string{"utun9"} + config.Normalize(&cfg) + before := config.KeyValues(&cfg) + + // utun9 is absent from the detected set: that tunnel is not up right now. + qs := Questions(Options{Config: &cfg, GOOS: "darwin", DetectedTunnels: []string{"utun0", "utun4"}}) + + q := byID(qs)["tunnels"] + var offered []string + for _, o := range q.Options { + offered = append(offered, o.Value) + } + if !reflect.DeepEqual(offered, []string{"utun0", "utun4", "utun9"}) { + t.Errorf("options = %v, want the detected tunnels plus the pinned one", offered) + } + if !reflect.DeepEqual(q.Selected, []string{"utun9"}) { + t.Errorf("selected = %v, want the pinned tunnel preselected", q.Selected) + } + + // And the whole click-through must be a no-op, as it is when the pin is up. + after := cfg + in := NewAnswers(qs).Input(strconv.Itoa(cfg.Hysteresis), nil) + in.MacOS = true + in.ConfigExisted = true + Apply(&after, in) + config.Normalize(&after) + for key, want := range before { + if got := config.KeyValues(&after)[key]; got != want { + t.Errorf("%s changed by answering nothing: %q -> %q", key, want, got) + } + } +} + // --- gating --- -func TestGatingHidesTheWholeVPNBranch(t *testing.T) { +// Automatic detection is the one gate left, and everything manual hangs off it. +func TestAutomaticDetectionGatesEveryManualField(t *testing.T) { qs := Questions(Options{GOOS: "darwin"}) a := NewAnswers(qs) - a.Set("configureVPN", "false") + a.Set("autoMode", "true") for _, q := range qs { - if q.RequiresID == "configureVPN" && a.ShouldAsk(q) { - t.Errorf("%s should not be asked when the VPN branch was declined", q.ID) + if q.RequiresID == "autoMode" && a.ShouldAsk(q) { + t.Errorf("%s should not be asked under automatic detection", q.ID) + } + } + for _, id := range []string{"tunnels", "endpoints", "profileFiles"} { + if !gatedOnAutoMode(qs, id) { + t.Errorf("%s is not gated on autoMode; on macOS it must be", id) + } + } + + a.Set("autoMode", "false") + for _, id := range []string{"tunnels", "endpoints", "profileFiles"} { + if !asked(qs, a, id) { + t.Errorf("declining automatic detection must ask %s", id) } } +} - a.Set("configureVPN", "true") +// Off macOS there is no live discovery, so the endpoint is required whichever +// detection mode is chosen. Gating it would let a Linux host finish the wizard +// with a config that cannot enforce. +func TestEndpointsAreUngatedWhereThereIsNoDiscovery(t *testing.T) { + qs := Questions(Options{GOOS: "linux"}) + a := NewAnswers(qs) a.Set("autoMode", "true") + if !asked(qs, a, "endpoints") { + t.Error("endpoints must be asked under automatic detection off macOS") + } + if gatedOnAutoMode(qs, "endpoints") { + t.Error("endpoints is gated on autoMode off macOS") + } +} + +// Two steps, which is the whole shape of the wizard: what to block, then how to +// find the VPN. A third group would mean a third screen in the app. +func TestTheWizardIsTwoGroups(t *testing.T) { + for _, goos := range []string{"darwin", "linux", "windows"} { + groups := map[int]bool{} + for _, q := range Questions(Options{GOOS: goos}) { + groups[q.Group] = true + } + if len(groups) != 2 || !groups[1] || !groups[2] { + t.Errorf("%s: groups = %v, want exactly {1, 2}", goos, groups) + } + } +} + +// The guard that replaced "configure your VPN now?". Without it, a re-run on a +// config with pinned interfaces would default to automatic detection, and +// clicking straight through would silently unpin them — Apply clears +// TunnelInterfaces under AutoMode on purpose. +func TestAutoModeSeedsFalseWhenInterfacesArePinned(t *testing.T) { + pinned := config.Default() + pinned.VPN.TunnelInterfaces = []string{"utun4"} + if got := defaultOf(Questions(Options{Config: &pinned, GOOS: "darwin"}), "autoMode"); got != "false" { + t.Errorf("autoMode default with pinned interfaces = %q, want \"false\"", got) + } + + fresh := config.Default() + fresh.VPN.TunnelInterfaces = nil + if got := defaultOf(Questions(Options{Config: &fresh, GOOS: "darwin"}), "autoMode"); got != "true" { + t.Errorf("autoMode default with no pinned interfaces = %q, want \"true\"", got) + } +} + +func gatedOnAutoMode(qs []Question, id string) bool { for _, q := range qs { - if q.ID == "tunnels" && a.ShouldAsk(q) { - t.Error("automatic detection must not ask which interface to pin") + if q.ID == id { + return q.RequiresID == "autoMode" && q.RequiresValue == "false" } } - a.Set("autoMode", "false") + return false +} + +func asked(qs []Question, a *Answers, id string) bool { for _, q := range qs { - if q.ID == "tunnels" && !a.ShouldAsk(q) { - t.Error("declining automatic detection must ask which interface to pin") + if q.ID == id { + return a.ShouldAsk(q) } } + return false } +func defaultOf(qs []Question, id string) string { + for _, q := range qs { + if q.ID == id { + return q.Default + } + } + return "" +} + +func eps(v ...string) *[]string { return &v } + // Walking the wizard and pressing Enter on every question must land on the // config you started with. Anything else means a default is stated in one place // and applied differently in another — the drift Phase M exists to prevent, @@ -289,10 +411,9 @@ func TestAnsweringNothingChangesNothing(t *testing.T) { qs := Questions(Options{Config: &cfg, GOOS: "darwin", DetectedTunnels: []string{"utun4"}}) a := NewAnswers(qs) - // The one answer with no config to seed it: the VPN branch is offered, and - // its own sub-answers are seeded, so accepting them must be a no-op too. - a.Set("configureVPN", "true") - a.Set("autoMode", "false") + // Nothing is Set here on purpose. autoMode seeds itself to false from the + // pinned interfaces above, which is exactly the guard being tested: pressing + // Enter through the whole wizard must not unpin them. after := cfg // Hysteresis has no question; the wizard carries the current value through, @@ -311,6 +432,66 @@ func TestAnsweringNothingChangesNothing(t *testing.T) { } } +// The PR's headline guarantee, driven through Answers rather than a hand-built +// Input: leaving automatic detection on must not blank a server set by hand. +// TestAnUnaskedEndpointListTouchesNoEndpoint pins Apply's half of this with a +// nil Endpoints; this pins that the wizard actually produces that nil. +func TestLeavingAutomaticOnKeepsAConfiguredEndpoint(t *testing.T) { + cfg := config.Default() + cfg.BlockedCountries = []string{"IR"} + cfg.VPN.Endpoints = []string{"203.0.113.7"} + config.Normalize(&cfg) + + qs := Questions(Options{Config: &cfg, GOOS: "darwin"}) + a := NewAnswers(qs) + a.Set("autoMode", "true") // the recommended answer, and the default + + in := a.Input(strconv.Itoa(cfg.Hysteresis), nil) + if in.Endpoints != nil { + t.Errorf("Endpoints = %v, want nil: the question was gated away", *in.Endpoints) + } + after := cfg + in.MacOS, in.ConfigExisted = true, true + Apply(&after, in) + config.Normalize(&after) + if got := after.VPN.Endpoints; !reflect.DeepEqual(got, []string{"203.0.113.7"}) { + t.Errorf("endpoints = %v, want the configured one untouched", got) + } +} + +// Two separate pieces of machinery assume gates are shallow and never point +// forward: the CLI's wave loop (a gate question this run will never show is +// treated as fixed at its seed, which is only safe when gates cannot nest) and +// Answers.wasAsked (which re-evaluates a gate against the FINAL answers, so a +// question gated on a LATER group's answer would be judged against a seed). +// Both are correct only because of the shape pinned here. Adding a gated gate, +// or a gate pointing at a later group, means fixing those two first. +func TestGatesAreShallowAndPointBackwards(t *testing.T) { + for _, goos := range []string{"darwin", "linux", "windows"} { + cfg := config.Default() + qs := Questions(Options{Config: &cfg, GOOS: goos}) + byid := byID(qs) + for _, q := range qs { + if !q.Gated() { + continue + } + gate, ok := byid[q.RequiresID] + if !ok { + t.Errorf("%s/%s: gate points at unknown question %q", goos, q.ID, q.RequiresID) + continue + } + if gate.Gated() { + t.Errorf("%s/%s: gate %q is itself gated; the wave loop assumes depth 1", + goos, q.ID, gate.ID) + } + if gate.Group > q.Group { + t.Errorf("%s/%s: gate %q is in a LATER group (%d > %d); wasAsked would read a seed", + goos, q.ID, gate.ID, gate.Group, q.Group) + } + } + } +} + func TestValidDuration(t *testing.T) { for _, ok := range []string{"30s", "5m", " 1h "} { if err := ValidDuration(ok); err != nil {