diff --git a/Cargo.lock b/Cargo.lock index 2507d69..f2f1815 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28,7 +28,7 @@ checksum = "b4388bee8683e3d04af747c73422af53102d2bd24d9eadb6cbc100baef4b43f8" [[package]] name = "bos-settings" -version = "0.6.0" +version = "0.6.1" dependencies = [ "async-channel", "bread-theme", diff --git a/Cargo.toml b/Cargo.toml index 61bb665..8340ca0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "bos-settings" -version = "0.6.0" +version = "0.6.1" edition = "2021" [dependencies] diff --git a/src/ui/views/firewall.rs b/src/ui/views/firewall.rs index eab6e4c..92d6041 100644 --- a/src/ui/views/firewall.rs +++ b/src/ui/views/firewall.rs @@ -14,7 +14,7 @@ //! action instead of forcing it on app open. use gtk4::prelude::*; -use gtk4::{Box as GBox, Button, Entry, Label, ListBox, ListBoxRow, Orientation, ScrolledWindow, Switch}; +use gtk4::{Box as GBox, Button, Entry, Label, ListBox, ListBoxRow, Orientation, ScrolledWindow, Switch, TextView}; use std::cell::Cell; use std::rc::Rc; @@ -33,13 +33,28 @@ struct Status { /// One `pkexec ufw status numbered` call, parsed for both the active/inactive /// line and the numbered rules — a single privileged read instead of two. -fn fetch_status() -> Option { +/// `Err` carries stderr (or a description of the exec failure) so callers can +/// show *why* it failed instead of leaving the page silently on "Status not +/// loaded" forever — previously this returned `Option`, discarding +/// the reason entirely, so a failed pkexec call (wrong password, no polkit +/// agent running in the session, ufw missing, ...) looked identical to +/// simply never having clicked Refresh. +fn fetch_status() -> Result { let output = std::process::Command::new("pkexec") .args(["ufw", "status", "numbered"]) .output() - .ok()?; + .map_err(|e| format!("couldn't run pkexec: {e}"))?; if !output.status.success() { - return None; + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + return Err(if stderr.is_empty() { + match output.status.code() { + Some(127) => "no polkit authentication agent is available in this session".to_string(), + Some(code) => format!("pkexec exited with status {code}"), + None => "pkexec was terminated by a signal".to_string(), + } + } else { + stderr + }); } let text = String::from_utf8_lossy(&output.stdout); let active = text.lines().next().is_some_and(|l| l.trim() == "Status: active"); @@ -55,10 +70,10 @@ fn fetch_status() -> Option { Some(Rule { number, text: rest.trim().to_string() }) }) .collect(); - Some(Status { active, rules }) + Ok(Status { active, rules }) } -fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc>) { +fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc>, log_buf: >k4::TextBuffer, log_view: &TextView) { while let Some(child) = list.first_child() { list.remove(&child); } @@ -92,14 +107,19 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc>) { let list = list.clone(); let number = rule.number.clone(); let programmatic = programmatic.clone(); + let log_buf = log_buf.clone(); + let log_view = log_view.clone(); remove_btn.connect_clicked(move |_| { - let log_buf = gtk4::TextBuffer::new(None); + log_buf.set_text(""); + log_view.set_visible(true); let list2 = list.clone(); let programmatic2 = programmatic.clone(); + let log_buf2 = log_buf.clone(); + let log_view2 = log_view.clone(); w::stream_command_then( &["pkexec", "ufw", "--force", "delete", &number], - log_buf, - move || refresh(&list2, None, &programmatic2), + log_buf.clone(), + move || refresh(&list2, None, &programmatic2, &log_buf2, &log_view2), ); }); } @@ -111,6 +131,19 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc>) { } } +/// Shows `message` as the list's only row, in place of the rule list — +/// used both for the initial "not loaded yet" state and for a failed +/// Refresh, so a failure looks like a state, not a no-op. +fn render_message(list: &ListBox, icon: &str, title: &str, detail: &str) { + while let Some(child) = list.first_child() { + list.remove(&child); + } + let row = ListBoxRow::new(); + row.set_selectable(false); + row.set_child(Some(&w::empty_state(icon, title, detail))); + list.append(&row); +} + /// Re-fetch status on a background thread and update the list (+ switch, if /// given) on completion. `enabled_sw` is `None` when called from a row /// action (delete/add) where the enabled state can't have changed. @@ -120,22 +153,42 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc>) { /// `disable`) — without it, the very first Refresh after ufw turns out to /// already be active would immediately fire an unwanted `ufw enable` the /// moment `set_active(true)` flips a switch that just became sensitive. -fn refresh(list: &ListBox, enabled_sw: Option<&Switch>, programmatic: &Rc>) { - let (tx, rx) = async_channel::bounded::>(1); +fn refresh(list: &ListBox, enabled_sw: Option<&Switch>, programmatic: &Rc>, log_buf: >k4::TextBuffer, log_view: &TextView) { + let (tx, rx) = async_channel::bounded::>(1); std::thread::spawn(move || { let _ = tx.send_blocking(fetch_status()); }); let list = list.clone(); let enabled_sw = enabled_sw.cloned(); let programmatic = programmatic.clone(); + let log_buf = log_buf.clone(); + let log_view = log_view.clone(); glib::spawn_future_local(async move { - if let Ok(Some(status)) = rx.recv().await { - render_rules(&list, &status.rules, &programmatic); - if let Some(sw) = &enabled_sw { - programmatic.set(true); - sw.set_sensitive(true); - sw.set_active(status.active); - programmatic.set(false); + match rx.recv().await { + Ok(Ok(status)) => { + render_rules(&list, &status.rules, &programmatic, &log_buf, &log_view); + if let Some(sw) = &enabled_sw { + programmatic.set(true); + sw.set_sensitive(true); + sw.set_active(status.active); + programmatic.set(false); + } + } + Ok(Err(reason)) => { + render_message( + &list, + "dialog-warning-symbolic", + "Couldn't read firewall status", + &reason, + ); + } + Err(_) => { + render_message( + &list, + "dialog-warning-symbolic", + "Couldn't read firewall status", + "the background check never reported back", + ); } } }); @@ -161,34 +214,47 @@ pub fn build() -> GBox { let list = ListBox::new(); list.set_selection_mode(gtk4::SelectionMode::None); - { - let row = ListBoxRow::new(); - row.set_selectable(false); - row.set_child(Some(&w::empty_state( - "security-high-symbolic", - "Status not loaded", - "Click Refresh below to check the firewall's current state.", - ))); - list.append(&row); - } + render_message( + &list, + "security-high-symbolic", + "Status not loaded", + "Click Refresh below to check the firewall's current state.", + ); let scroll = ScrolledWindow::new(); scroll.set_vexpand(true); scroll.set_min_content_height(260); scroll.set_child(Some(&list)); content.append(&scroll); + // Shared log view for every pkexec/ufw call on this page (refresh, + // enable/disable, add, remove) — previously each action created its own + // throwaway `TextBuffer::new(None)` that was never attached to any + // visible widget, so stderr/stdout from a failing command (including + // pkexec itself failing) went nowhere the user could see. + let log_buf = gtk4::TextBuffer::new(None); + let log_view = TextView::with_buffer(&log_buf); + log_view.set_editable(false); + log_view.set_monospace(true); + log_view.set_height_request(140); + log_view.set_margin_top(8); + log_view.set_visible(false); + let refresh_btn = Button::with_label("Refresh status"); { let list = list.clone(); let enabled_sw = enabled_sw.clone(); let programmatic = programmatic.clone(); - refresh_btn.connect_clicked(move |_| refresh(&list, Some(&enabled_sw), &programmatic)); + let log_buf = log_buf.clone(); + let log_view = log_view.clone(); + refresh_btn.connect_clicked(move |_| refresh(&list, Some(&enabled_sw), &programmatic, &log_buf, &log_view)); } content.append(&refresh_btn); { let list = list.clone(); let programmatic = programmatic.clone(); + let log_buf = log_buf.clone(); + let log_view = log_view.clone(); enabled_sw.connect_active_notify(move |s| { // Skip both the pre-refresh insensitive state and any // programmatic set_active() from refresh() itself — only a @@ -197,11 +263,14 @@ pub fn build() -> GBox { return; } let verb = if s.is_active() { "enable" } else { "disable" }; - let log_buf = gtk4::TextBuffer::new(None); + log_buf.set_text(""); + log_view.set_visible(true); let list2 = list.clone(); let programmatic2 = programmatic.clone(); - w::stream_command_then(&["pkexec", "ufw", "--force", verb], log_buf, move || { - refresh(&list2, None, &programmatic2); + let log_buf2 = log_buf.clone(); + let log_view2 = log_view.clone(); + w::stream_command_then(&["pkexec", "ufw", "--force", verb], log_buf.clone(), move || { + refresh(&list2, None, &programmatic2, &log_buf2, &log_view2); }); }); } @@ -219,21 +288,26 @@ pub fn build() -> GBox { let list = list.clone(); let add_entry = add_entry.clone(); let programmatic = programmatic.clone(); + let log_buf = log_buf.clone(); + let log_view = log_view.clone(); add_btn.connect_clicked(move |_| { let rule = add_entry.text().to_string(); if rule.trim().is_empty() { return; } - let log_buf = gtk4::TextBuffer::new(None); + log_buf.set_text(""); + log_view.set_visible(true); let list2 = list.clone(); let add_entry2 = add_entry.clone(); let programmatic2 = programmatic.clone(); + let log_buf2 = log_buf.clone(); + let log_view2 = log_view.clone(); w::stream_command_then( &["pkexec", "ufw", "allow", rule.trim()], - log_buf, + log_buf.clone(), move || { add_entry2.set_text(""); - refresh(&list2, None, &programmatic2); + refresh(&list2, None, &programmatic2, &log_buf2, &log_view2); }, ); }); @@ -242,5 +316,7 @@ pub fn build() -> GBox { add_row.append(&add_btn); content.append(&add_row); + content.append(&log_view); + outer } diff --git a/src/ui/views/snapshots.rs b/src/ui/views/snapshots.rs index 76d59b7..435f7ee 100644 --- a/src/ui/views/snapshots.rs +++ b/src/ui/views/snapshots.rs @@ -13,27 +13,28 @@ struct SnapshotRow { description: String, } -fn list_snapshots() -> Vec { +/// `Err` carries snapper's trimmed stderr — distinct from `Ok(vec![])` +/// (snapper works fine, there just aren't any snapshots yet), so the caller +/// can tell a real failure from a legitimately empty list instead of +/// collapsing both into the same generic "not configured yet" message. +fn list_snapshots() -> Result, String> { // NOTE: the real flag is --columns, not --output-cols (which snapper // rejects outright with "Unknown option") — confirmed against snapper // 0.13's own --help. With the wrong flag this always failed and the // panel silently showed "No snapshots found" on every install. - let Ok(output) = Command::new("snapper") + let output = Command::new("snapper") .args(["list", "--columns", "number,date,description"]) .output() - else { - return Vec::new(); - }; + .map_err(|e| e.to_string())?; if !output.status.success() { - eprintln!( - "bos-settings: snapper list failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); - return Vec::new(); + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + eprintln!("bos-settings: snapper list failed: {stderr}"); + return Err(stderr); } let text = String::from_utf8_lossy(&output.stdout); - text.lines() + Ok(text + .lines() .skip(2) // header + separator .filter_map(|line| { let mut cols = line.splitn(3, '|'); @@ -49,7 +50,7 @@ fn list_snapshots() -> Vec { description: cols.next()?.trim().to_string(), }) }) - .collect() + .collect()) } /// Returns whether the list ended up empty, so callers can disable the @@ -58,15 +59,39 @@ fn populate_list(list: &ListBox) -> bool { while let Some(child) = list.first_child() { list.remove(&child); } - let snapshots = list_snapshots(); + let snapshots = match list_snapshots() { + Ok(snapshots) => snapshots, + Err(stderr) => { + let lower = stderr.to_lowercase(); + let (title, detail) = if lower.contains("no permission") { + ( + "No permission to read snapshots", + "This user isn't allowed to run snapper. Check ALLOW_USERS in \ + /etc/snapper/configs/root — it should list your username.", + ) + } else if lower.contains("unknown config") || lower.contains("no such file") { + ( + "Snapper isn't configured", + "No snapper config exists for root yet, so nothing is being \ + snapshotted. This should be set up automatically at install.", + ) + } else { + ("Couldn't read snapshots", stderr.as_str()) + }; + let row = ListBoxRow::new(); + row.set_selectable(false); + row.set_child(Some(&w::empty_state("dialog-warning-symbolic", title, detail))); + list.append(&row); + return true; + } + }; if snapshots.is_empty() { let row = ListBoxRow::new(); row.set_selectable(false); row.set_child(Some(&w::empty_state( "document-open-recent-symbolic", "No snapshots yet", - "Snapshots are created automatically on every pacman transaction \ - (snapper may not be configured yet).", + "Snapshots are created automatically on every pacman transaction.", ))); list.append(&row); return true;