Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions crates/assistant/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,22 @@ fn file_access_needs_approval() {
assert!(tool_msg["content"].as_str().is_some_and(|t| t.contains("declined")));
}

#[test]
fn connection_management_requires_fresh_approval_but_can_run_in_app() {
let mut s = Session::new(Project::default());
let mut a = Agent::new(cfg("openai"), &s);
assert!(matches!(a.ask("save a connection", &s), Step::Send(_)));
let args = json!({"command":"connection.new","params":{"kind":"s3","name":"Approved connection"}});
assert_eq!(a.receive(a.generation(), ok(call("first", "run_command", args.clone())), &mut s), Step::Confirm);
assert!(s.connections.find("Approved connection").is_none());
assert!(!a.pending(&s).unwrap().2, "reserved commands cannot be allowed for the whole conversation");
assert!(matches!(a.decide(true, true, &mut s), Step::Send(_)));
assert_eq!(s.connections.find("Approved connection").unwrap().kind, "s3");
assert_eq!(a.receive(a.generation(), ok(call("again", "run_command", args)), &mut s), Step::Confirm);
assert!(matches!(a.decide(false, false, &mut s), Step::Send(_)));
assert_eq!(s.connections.connections.len(), 1, "declining a repeat must leave the approved connection unchanged");
}

#[test]
fn gate_rules() {
let s = sample();
Expand Down
5 changes: 5 additions & 0 deletions crates/data/src/source/duckdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ pub(crate) fn remembered_password(dsn: &str) -> Option<String> {
passwords().lock().unwrap_or_else(|e| e.into_inner()).get(dsn).cloned()
}

/// Whether a password is remembered for a connection this session.
pub fn has_password(dsn: &str) -> bool {
passwords().lock().unwrap_or_else(|e| e.into_inner()).contains_key(dsn)
}

/// The remembered password of a query's database, handed to DuckDB's PostgreSQL client in its
/// environment (`PGPASSWORD`): never in the script, where the process list and DuckDB's errors
/// would show it.
Expand Down
248 changes: 248 additions & 0 deletions crates/engine/src/cmd/connections.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
//! Saved connections and session-only logins. Listing handlers are called directly: these
//! commands never edit the project, create undo snapshots or note recently added sources.

use std::collections::BTreeMap;

use serde_json::{Value, json};

use super::ps;
use crate::connections::{Connection, DATABASE_KINDS, DESKTOP_ONLY, KINDS};
use crate::render::rustgis_tiles::s3;
use crate::{CmdResult, CommandSpec as C, Session};

pub fn commands() -> Vec<C> {
vec![
C::new("connection.list", "List Connections", "Home › Add › Data Sources", list).view().params(
r#"{"kind"?: "postgres"|"mysql"|"sqlserver"|"service"|"wms"|"wmts"|"wfs"|"wcs"|"ogcapi"|"arcgis"|"csw"|"stac"|"s3", "group"?: text}"#,
),
C::new("connection.new", "New Connection", "Home › Add › Data Sources", new).view().params(
r#"{"kind": "postgres"|"mysql"|"sqlserver"|"service"|"wms"|"wmts"|"wfs"|"wcs"|"ogcapi"|"arcgis"|"csw"|"stac"|"s3", "address"?: "as the kind's add command takes it, without its password or token (postgres:host=… dbname=… user=…, mysql://user@host/db, mssql://user@host/db, a service's or STAC API's https:// address, an S3 endpoint: AWS when absent)", "name"?: "unique (default from the address)", "group"?: text, "options"?: {"region"?: text, "buckets"?: "a,b", "credentials"?: "keys"|"profile"|"environment"|"none", "profile"?: text} (S3 only)}"#,
),
C::new("connection.edit", "Edit Connection", "Home › Add › Data Sources", edit).view().params(
r#"{"id": "a connection's id or name", "name"?: text, "group"?: text, "address"?: text, "options"?: "as connection.new (replaces them)", "kind"?: "its kind (unchanged)"}"#,
),
C::new("connection.duplicate", "Duplicate Connection", "Home › Add › Data Sources", duplicate).view().params(r#"{"id": "a connection's id or name", "name"?: text}"#),
C::new("connection.remove", "Remove Connection", "Home › Add › Data Sources", remove).view().params(r#"{"id": "a connection's id or name"}"#),
C::new("connection.login", "Log In to Connection", "Home › Add › Data Sources", login_command).view().params(
r#"{"id": "a connection's id or name", "password"?: "a database connection's, kept for this session only", "accessKeyId"?: text, "secretAccessKey"?: text, "sessionToken"?: "an S3 connection's keys (credentials: keys), kept for this session only"}"#,
),
C::new("connection.test", "Test Connection", "Home › Add › Data Sources", test).view().params(
r#"{"id": "a connection's id or name", "password"?: text, "accessKeyId"?: text, "secretAccessKey"?: text, "sessionToken"?: "as connection.login (logs in first)"}"#,
),
]
}

fn shown(c: &Connection) -> Value {
let database = DATABASE_KINDS.contains(&c.kind.as_str());
let available = !database || !cfg!(target_arch = "wasm32");
let logged_in = if c.kind == "s3" {
s3::connection(&c.name).is_some()
} else if database {
#[cfg(not(target_arch = "wasm32"))]
{
use rustgis_data::source::duckdb::{self, Database};
match Database::parse(&c.address) {
Database::Postgres(d) | Database::Mysql(d) | Database::Sqlserver(d) => duckdb::has_password(&d),
Database::File(_) => false,
}
}
#[cfg(target_arch = "wasm32")]
{
false
}
} else {
false
};
let add_with = match c.kind.as_str() {
"postgres" | "mysql" | "sqlserver" => "info.databaseTables {database: address}, then layer.addDatabase {database: address, layer}",
"service" => "service.layers {url: address}, then service.add {url, layer}",
"wms" | "wmts" | "wfs" | "ogcapi" | "arcgis" => "service.layers {url: address, type: kind}, then service.add {url, type, layer}",
"wcs" => "service.layers {url: address, type: wcs}, then service.coverage {url, layer}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The S3 login state and the session are keyed only by connection name (s3::connection(&c.name), s3::disconnect(&old.name)). The S3 connection registry is shared with s3.connect and the S3 dialog. A connection made there under the same name will show loggedIn: true. connection.remove and connection.edit will also disconnect it, even though the saved connection never created it. Also, connection.duplicate of a logged-in S3 connection gets a different name and so starts logged out, which is fine. Low-medium confidence that this matters in practice. It may be worth mentioning in the docs or namespacing the session name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented explicitly in 0772908. Delivery 1 deliberately reuses the existing name-based S3 session registry so saved connections, s3.connect and the unchanged S3 dialog interoperate. A same-name live entry supplies loggedIn, and editing/removing the saved entry disconnects that live entry regardless of which existing UI/command created it. Introducing a second namespaced registry here would break that approved compatibility path. The user guide now calls out this shared lifetime rather than implying independent ownership.

"csw" => "catalog.search {url: address, query}",
"stac" => "stac.collections {url: address}, then stac.search {url, collections}",
"s3" => "connection.login {id} once, then s3.list {connection: name}, then layer.addUrl {url: s3://…}",
_ => "",
};
let mut out = serde_json::to_value(c).unwrap_or(Value::Null);
out["available"] = json!(available);
if !available {
out["why"] = json!(DESKTOP_ONLY);
}
out["loggedIn"] = json!(logged_in);
out["addWith"] = json!(add_with);
out
}

fn found<'a>(s: &'a Session, p: &Value) -> Result<&'a Connection, String> {
let id = ps(p, "id").unwrap_or("");
s.connections.find(id).ok_or_else(|| format!("no connection '{id}'"))
}

fn options(p: &Value) -> Result<BTreeMap<String, String>, String> {
let object = p.as_object().ok_or("options is an object of texts")?;
object
.iter()
.map(|(k, v)| v.as_str().map(|t| (k.clone(), t.to_owned())).ok_or_else(|| "options is an object of texts".into()))
.collect()
}

fn list(s: &mut Session, p: &Value) -> CmdResult {
let kind = ps(p, "kind");
if let Some(k) = kind
&& !KINDS.contains(&k)
{
return Err(format!("unknown kind '{k}' ({})", KINDS.join(", ")));
}
let group = ps(p, "group");
Ok(json!({"connections": s.connections.connections.iter()
.filter(|c| kind.is_none_or(|k| c.kind == k) && group.is_none_or(|g| c.group.chars().flat_map(char::to_lowercase).eq(g.chars().flat_map(char::to_lowercase))))
.map(shown).collect::<Vec<_>>() }))
}

fn new(s: &mut Session, p: &Value) -> CmdResult {
let c = Connection {
kind: ps(p, "kind").ok_or_else(|| format!("kind is required ({})", KINDS.join(", ")))?.trim().into(),
address: ps(p, "address").unwrap_or("").into(),
name: ps(p, "name").unwrap_or("").into(),
group: ps(p, "group").unwrap_or("").into(),
options: p.get("options").map(options).transpose()?.unwrap_or_default(),
..Connection::default()
};
let explicit = !c.name.trim().is_empty();
let (c, stripped) = s.connections.add(c, explicit)?;
Ok(shown_stripped(&c, stripped))
}

fn shown_stripped(c: &Connection, stripped: bool) -> Value {
let mut out = shown(c);
if stripped {
out["stripped"] = json!(true);
}
out
}

fn edit(s: &mut Session, p: &Value) -> CmdResult {
let old = found(s, p)?.clone();
let mut c = old.clone();
if let Some(kind) = ps(p, "kind")
&& kind.trim() != c.kind
{
return Err("a connection's kind doesn't change: make a new one".into());
}
for (key, field) in [("name", &mut c.name), ("group", &mut c.group), ("address", &mut c.address)] {
if let Some(value) = p.get(key) {
*field = value.as_str().ok_or_else(|| format!("{key} is text"))?.into();
}
}
if let Some(v) = p.get("options") {
c.options = options(v)?;
}
let (c, stripped) = s.connections.update(&old.id, c)?;
if c.kind == "s3" && (old.name != c.name || old.address != c.address || old.options != c.options) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session credentials aren't cleared when a database connection's address is edited. edit only drops the S3 session on change, so a password stored for the old database address stays in the session store, and loggedIn is computed from the address. The old password is orphaned but not revoked. Confidence: low-medium. Consider clearing it on address change for database kinds. Also, remove doesn't forget database session passwords, only S3 ones.

s3::disconnect(&old.name);
if old.name != c.name {
s3::disconnect(&c.name);
}
}
Ok(shown_stripped(&c, stripped))
}

fn duplicate(s: &mut Session, p: &Value) -> CmdResult {
let mut c = found(s, p)?.clone();
c.id.clear();
let explicit = ps(p, "name").is_some_and(|n| !n.trim().is_empty());
if let Some(name) = ps(p, "name") {
c.name = name.into();
}
let (c, _) = s.connections.add(c, explicit)?;
Ok(shown(&c))
}

fn remove(s: &mut Session, p: &Value) -> CmdResult {
Comment thread
deniial00 marked this conversation as resolved.
let c = found(s, p)?.clone();
s.connections.remove(&c.id);
if c.kind == "s3" {
s3::disconnect(&c.name);
}
Ok(json!({"removed": c.id}))
}

fn login(s: &mut Session, c: &Connection, p: &Value) -> Result<(), String> {
if DATABASE_KINDS.contains(&c.kind.as_str()) {
#[cfg(target_arch = "wasm32")]
{
return Err(DESKTOP_ONLY.into());
}
#[cfg(not(target_arch = "wasm32"))]
{
let pw = ps(p, "password").filter(|t| !t.is_empty()).ok_or("password is required")?;
super::layer::database_param(&json!({"database": c.address, "password": pw}));
return Ok(());
}
}
if c.kind != "s3" {
return Err(format!("{} connections don't log in yet", c.kind));
}
let mut q = json!({"name": c.name, "endpoint": c.address});
for key in ["region", "buckets"] {
if let Some(v) = c.options.get(key).filter(|v| !v.is_empty()) {
q[key] = json!(v);
}
}
match c.options.get("credentials").map(String::as_str).unwrap_or("none") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An S3 connection with no credentials option falls through to unwrap_or("none"), so it logs in anonymously. connection.new {kind:"s3"} and edits that clear options therefore silently produce anonymous access. The S3 dialog and the legacy import both default to keys. Consider defaulting to keys, or storing the credentials option explicitly in clean. Confidence: medium.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default is intentional in the approved command contract: an omitted credentials option means none. connection.new with no login method creates a public/anonymous connection; it does not implicitly discover environment or profile credentials. Migration maps an empty legacy source to keys because that older saved format represented typed keys. The dialog may preselect keys, while the command supports explicit anonymous connections. I would keep the specified command behavior rather than silently change the login method.

"keys" => {
if ["accessKeyId", "secretAccessKey"].iter().any(|k| ps(p, k).is_none_or(|v| v.trim().is_empty())) {
return Err("accessKeyId and secretAccessKey are required: an S3 connection's keys are typed each session".into());
}
for key in ["accessKeyId", "secretAccessKey", "sessionToken"] {
if let Some(v) = p.get(key) {
q[key] = v.clone();
}
}
}
"profile" => {
let profile = c.options.get("profile").filter(|v| !v.trim().is_empty()).ok_or("the connection names no AWS profile")?;
q["profile"] = json!(profile);
}
"environment" => q["environment"] = json!(true),
_ => q["anonymous"] = json!(true),
}
super::s3::connect(s, &q)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design concern (medium confidence): S3 sessions are keyed by connection name in the shared runtime registry, so remove, edit and shown().loggedIn can act on a session that s3.connect or the S3 dialog created under the same name. A saved connection called "default", for example, would log out the dialog's default session. The docs mention this, and the tests cover the rename case. Keying the session by connection id, or prefixing the registry name, would remove the coupling. As written, remove also disconnects whenever the name matches, even if the session never came from this connection.

Ok(())
}

fn login_command(s: &mut Session, p: &Value) -> CmdResult {
let c = found(s, p)?.clone();
login(s, &c, p)?;
Ok(shown(&c))
}

fn test(s: &mut Session, p: &Value) -> CmdResult {
let c = found(s, p)?.clone();
if ["password", "accessKeyId", "secretAccessKey", "sessionToken"].iter().any(|k| ps(p, k).is_some_and(|v| !v.is_empty())) {
login(s, &c, p)?;
}
let result = match c.kind.as_str() {
"postgres" | "mysql" | "sqlserver" => {
if cfg!(target_arch = "wasm32") {
return Err(DESKTOP_ONLY.into());
}
super::layer::database_tables(s, &json!({"database": c.address}))?
}
"service" => super::service::layers(s, &json!({"url": c.address}))?,
"wms" | "wmts" | "wfs" | "wcs" | "ogcapi" | "arcgis" => super::service::layers(s, &json!({"url": c.address, "type": c.kind}))?,
"csw" => super::service::catalog_search(s, &json!({"url": c.address}))?,
"stac" => super::stac::collections(s, &json!({"url": c.address}))?,
"s3" => {
if s3::connection(&c.name).is_none() {
if c.options.get("credentials").is_some_and(|v| v == "keys") {
return Err("log in first: connection.login {id, accessKeyId, secretAccessKey}".into());
}
login(s, &c, &Value::Null)?;
}
super::s3::list(s, &json!({"connection": c.name, "limit": 100}))?
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection.test makes connection.login a side effect: the supplied password or keys are stored for the session before the test runs, and they stay stored if the test then fails (wrong password, unreachable host). That is probably intended per the docs, but a "test" that silently persists credentials, and shares a database password by DSN across other layers and connections, can surprise. Consider whether a failed test should leave the login in place. Also, login for non-S3/non-database kinds returns "don't log in yet", so connection.test with a stray password on a wms connection now errors instead of just testing. Confidence: low-medium.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept the planned login-first behavior and made it explicit in the docs in 0772908: when login parameters are supplied, connection.test logs in first, and a failed listing does not undo that session login. A failed listing can mean a temporary network error and does not establish that credentials are invalid; rolling back a shared-DSN login could also affect other connections/layers. A password supplied to a WMS connection intentionally produces the unsupported-login error rather than silently discarding an explicitly supplied secret. The parameter description already says logs in first; the guide now also describes the failure case.

_ => return Err(format!("unknown kind '{}'", c.kind)),
};
Ok(json!({"connection": c.id, "result": result}))
}
2 changes: 1 addition & 1 deletion crates/engine/src/cmd/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ pub fn sql_database_param(p: &Value) -> Result<Option<rustgis_data::source::duck
}

/// The tables of a database (a live connection or a DuckDB file) that have a geometry column.
fn database_tables(_s: &mut Session, p: &Value) -> CmdResult {
pub(super) fn database_tables(_s: &mut Session, p: &Value) -> CmdResult {
#[cfg(target_arch = "wasm32")]
{
let _ = p;
Expand Down
2 changes: 2 additions & 0 deletions crates/engine/src/cmd/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

pub mod catalogs;
pub mod clipboard;
pub mod connections;
pub mod cube;
pub mod deeplearning;
pub mod design;
Expand Down Expand Up @@ -62,6 +63,7 @@ pub fn all() -> Vec<CommandSpec> {
v.extend(service::commands());
v.extend(s3::commands());
v.extend(sources::commands());
v.extend(connections::commands());
v.extend(layer::commands());
v.extend(group::commands());
v.extend(layer_copy::commands());
Expand Down
6 changes: 6 additions & 0 deletions crates/engine/src/cmd/plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,12 @@ const NEVER: &[&str] = &[
"edit.undo",
"edit.redo",
// Credentials: a plugin never sets where the user's are sent.
"connection.new",
"connection.edit",
"connection.duplicate",
"connection.remove",
"connection.login",
"connection.test",
"s3.connect",
"s3.disconnect",
"earthdata.login",
Expand Down
4 changes: 2 additions & 2 deletions crates/engine/src/cmd/s3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ fn credentials(p: &Value) -> Result<(Option<Credentials>, Option<String>, Option
Ok((None, None, None, "anonymous"))
}

fn connect(_s: &mut Session, p: &Value) -> CmdResult {
pub(super) fn connect(_s: &mut Session, p: &Value) -> CmdResult {
let name = ps(p, "name").map(str::trim).filter(|n| !n.is_empty()).unwrap_or("default").to_string();
let endpoint = ps(p, "endpoint").map(str::trim).filter(|e| !e.is_empty()).map(str::to_string);
let (creds, region, profile_endpoint, source) = credentials(p)?;
Expand Down Expand Up @@ -140,7 +140,7 @@ fn target(p: &Value) -> Result<(Option<String>, String), String> {
Ok((bucket, prefix))
}

fn list(_s: &mut Session, p: &Value) -> CmdResult {
pub(super) fn list(_s: &mut Session, p: &Value) -> CmdResult {
let (bucket, prefix) = target(p)?;
let named = match ps(p, "connection").map(str::trim).filter(|n| !n.is_empty()) {
Some(n) => Some(s3::connection(n).ok_or_else(|| format!("no S3 connection named '{n}'"))?),
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/src/cmd/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ pub(crate) fn coverage_check(name: &str, bytes: &[u8]) -> Result<(), String> {

/// `catalog.search`: a page of a catalogue's (CSW) records matching words and an area, with the
/// links RustGIS can follow (those pointing where a document on the web may not send it left out).
fn catalog_search(s: &mut Session, p: &Value) -> CmdResult {
pub(super) fn catalog_search(s: &mut Session, p: &Value) -> CmdResult {
let url = url_of(p)?;
let request = records_request(s, p)?;
let page = match ps(p, "text") {
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/src/cmd/stac.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub(crate) fn add_collection(s: &mut Session, fc: &Value, name: &str) -> Result<
Ok(Some(id))
}

fn collections(_s: &mut Session, p: &Value) -> CmdResult {
pub(super) fn collections(_s: &mut Session, p: &Value) -> CmdResult {
let api = stac::api(ps(p, "url"))?;
#[cfg(target_arch = "wasm32")]
{
Expand Down
Loading
Loading