Skip to content
18 changes: 16 additions & 2 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,7 @@ impl AppBuilder {
tools: &Arc<ToolRegistry>,
hooks: &Arc<HookRegistry>,
settings_store_override: Option<Arc<dyn crate::db::SettingsStore + Send + Sync>>,
ownership_cache: Arc<crate::ownership::OwnershipCache>,
) -> Result<
(
Arc<McpSessionManager>,
Expand Down Expand Up @@ -989,6 +990,13 @@ impl AppBuilder {
if let Some(ref ss) = settings_store_override {
em = em.with_settings_store(Arc::clone(ss));
}
if let Some(ref db) = self.db {
let ps = Arc::new(crate::pairing::PairingStore::new(
Arc::clone(db),
Arc::clone(&ownership_cache),
));
em = em.with_pairing_store(ps);
}
let manager = Arc::new(em);
tools.register_extension_tools(Arc::clone(&manager));

Expand Down Expand Up @@ -1181,6 +1189,7 @@ impl AppBuilder {
_ => (None, None),
};

let ownership_cache = Arc::new(crate::ownership::OwnershipCache::new());
let (
mcp_session_manager,
mcp_process_manager,
Expand All @@ -1189,7 +1198,12 @@ impl AppBuilder {
catalog_entries,
dev_loaded_tool_names,
) = self
.init_extensions(&tools, &hooks, settings_store.clone())
.init_extensions(
&tools,
&hooks,
settings_store.clone(),
Arc::clone(&ownership_cache),
)
.await?;

// Load bootstrap-completed flag from settings so that existing users
Expand Down Expand Up @@ -1333,7 +1347,7 @@ impl AppBuilder {
catalog_entries,
dev_loaded_tool_names,
builder,
ownership_cache: Arc::new(crate::ownership::OwnershipCache::new()),
ownership_cache,
})
}
}
Expand Down
98 changes: 93 additions & 5 deletions src/channels/relay/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//! proxy API (Slack).

use std::collections::HashMap;
use std::sync::Arc;

use async_trait::async_trait;
use tokio::sync::mpsc;
Expand All @@ -15,6 +16,7 @@ use crate::channels::{
Channel, ChatApprovalPrompt, IncomingMessage, MessageStream, OutgoingResponse, StatusUpdate,
};
use crate::error::ChannelError;
use crate::pairing::PairingStore;

/// Default channel name for the Slack relay integration.
pub const DEFAULT_RELAY_NAME: &str = "slack-relay";
Expand Down Expand Up @@ -51,6 +53,8 @@ pub struct RelayChannel {
event_tx: mpsc::Sender<ChannelEvent>,
/// Receiver side — taken once by `start()`.
event_rx: tokio::sync::Mutex<Option<mpsc::Receiver<ChannelEvent>>>,
/// Resolves Slack sender_id → internal UserId for multi-tenant support.
pairing_store: Option<Arc<PairingStore>>,
}

impl RelayChannel {
Expand Down Expand Up @@ -88,9 +92,16 @@ impl RelayChannel {
instance_id,
event_tx,
event_rx: tokio::sync::Mutex::new(Some(event_rx)),
pairing_store: None,
}
}

/// Set the pairing store for multi-tenant identity resolution.
pub fn with_pairing_store(mut self, store: Arc<PairingStore>) -> Self {
self.pairing_store = Some(store);
self
}

/// Get a clone of the event sender for wiring into the webhook endpoint.
pub fn event_sender(&self) -> mpsc::Sender<ChannelEvent> {
self.event_tx.clone()
Expand Down Expand Up @@ -123,9 +134,10 @@ impl RelayChannel {
team_id: &str,
method: &str,
body: serde_json::Value,
slack_user_id: Option<&str>,
) -> Result<serde_json::Value, crate::channels::relay::client::RelayError> {
self.client
.proxy_provider(self.provider.as_str(), team_id, method, body)
.proxy_provider_with_user(self.provider.as_str(), team_id, method, body, slack_user_id)
.await
}

Expand Down Expand Up @@ -205,6 +217,8 @@ impl Channel for RelayChannel {
let (tx, rx) = mpsc::channel(64);
let provider_str = self.provider.as_str().to_string();
let relay_name = channel_name.clone();
let pairing_store = self.pairing_store.clone();
let pairing_client = self.client.clone();

// Spawn a task that reads events from the webhook handler and converts to IncomingMessage
tokio::spawn(async move {
Expand Down Expand Up @@ -240,7 +254,80 @@ impl Channel for RelayChannel {
"Relay: received message from {}", provider_str
);

let mut msg = IncomingMessage::new(&relay_name, &event.sender_id, event.text())
// Resolve sender_id → internal UserId via PairingStore.
// External ID is scoped to workspace: "team_id:sender_id".
let scoped_external_id = format!("{}:{}", event.provider_scope, event.sender_id);
let resolved_user_id: String = if let Some(ref store) = pairing_store {
match store
.resolve_identity(&relay_name, &scoped_external_id)
.await
{
Ok(Some(uid)) => {
let user_str = uid.as_str().to_string();
tracing::debug!(
sender_id = %event.sender_id,
resolved_user = %user_str,
"Relay: resolved sender to internal user"
);
user_str
}
Ok(None) => {
Comment thread
PierreLeGuen marked this conversation as resolved.
tracing::info!(
sender_id = %event.sender_id,
"Relay: sender not paired, sending pairing code"
);
let meta = serde_json::json!({
"sender_name": event.display_name(),
"channel_id": event.channel_id,
});
match store
.upsert_request(&relay_name, &scoped_external_id, Some(meta))
.await
{
Ok(record) => {
let instructions = format!(
"Enter this code in IronClaw to pair your Slack account: `{}`",
record.code
);
let team_id = event.team_id().to_string();
let body = serde_json::json!({
"channel": event.channel_id,
"text": instructions,
"thread_ts": event.thread_id.as_deref().unwrap_or(&event.id),
});
if let Err(e) = pairing_client
.proxy_provider(
&provider_str,
&team_id,
"chat.postMessage",
body,
)
.await
{
tracing::warn!(error = %e, "Relay: failed to send pairing code reply");
}
}
Err(e) => {
tracing::warn!(error = %e, "Relay: failed to create pairing request");
}
}
continue;
}
Err(e) => {
tracing::warn!(
sender_id = %event.sender_id,
error = %e,
"Relay: pairing resolution failed, dropping message"
);
continue;
}
}
} else {
event.sender_id.clone()
};

let mut msg = IncomingMessage::new(&relay_name, &resolved_user_id, event.text())
Comment thread
PierreLeGuen marked this conversation as resolved.
.with_sender_id(event.sender_id.clone())
.with_user_name(event.display_name())
.with_metadata(serde_json::json!({
"team_id": event.team_id(),
Expand Down Expand Up @@ -323,8 +410,9 @@ impl Channel for RelayChannel {
.filter(|s| !s.is_empty());

let (method, body) = self.build_send_body(channel_id, &response.content, thread_id);
let sender_id = metadata.get("sender_id").and_then(|v| v.as_str());

self.proxy_send(team_id, &method, body)
self.proxy_send(team_id, &method, body, sender_id)
.await
.map_err(|e| ChannelError::SendFailed {
name: channel_name,
Expand Down Expand Up @@ -384,7 +472,7 @@ impl Channel for RelayChannel {
})?;
let body = self.build_approval_body(channel_id, thread_id, &prompt, &approval_token);

self.proxy_send(team_id, "chat.postMessage", body)
self.proxy_send(team_id, "chat.postMessage", body, None)
Comment thread
PierreLeGuen marked this conversation as resolved.
.await
.map_err(|e| ChannelError::SendFailed {
name: self.name().to_string(),
Expand All @@ -410,7 +498,7 @@ impl Channel for RelayChannel {

let (method, body) = self.build_send_body(target, &response.content, thread_id);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium Severity

Proactive relay broadcasts do not reverse-map the internal user target back to a Slack identity.

Channel::broadcast() is used for heartbeat/routine/mission-style proactive sends and receives an IronClaw user_id target. This implementation treats that target as the raw Slack channel destination and proxies with slack_user_id = None. The PR fixes inbound sender_id -> UserId resolution and direct replies, but out-of-band notifications still lack the reverse UserId -> paired Slack actor/channel path, so notifications for paired users can target an invalid Slack ID or be sent through the wrong relay connection.

Please use the pairing store (or stored routing metadata) to resolve the target IronClaw user back to the correct Slack/team/user delivery target and pass the corresponding slack_user_id to the relay proxy. Add a regression test for a proactive relay broadcast to a paired non-owner user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged as a design gap for proactive sends. The broadcast() method receives an IronClaw target but lacks the reverse mapping to Slack user/channel. For inbound request-response (the primary multi-tenant flow), the sender_id is preserved in metadata and used for routing. Proactive broadcast reverse-mapping is a follow-up.


self.proxy_send(&self.team_id, &method, body)
self.proxy_send(&self.team_id, &method, body, None)
.await
.map_err(|e| ChannelError::SendFailed {
name: channel_name,
Expand Down
21 changes: 20 additions & 1 deletion src/channels/relay/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,13 @@ impl ChannelEvent {
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Connection {
pub provider: String,
#[serde(alias = "provider_scope")]
pub team_id: String,
#[serde(alias = "provider_scope_name")]
pub team_name: Option<String>,
#[serde(default)]
pub connected: bool,
pub authed_user_id: Option<String>,
}

/// HTTP client for the channel-relay service.
Expand Down Expand Up @@ -237,6 +241,18 @@ impl RelayClient {
team_id: &str,
method: &str,
body: serde_json::Value,
) -> Result<serde_json::Value, RelayError> {
self.proxy_provider_with_user(provider, team_id, method, body, None)
.await
}

pub async fn proxy_provider_with_user(
&self,
provider: &str,
team_id: &str,
method: &str,
body: serde_json::Value,
slack_user_id: Option<&str>,
) -> Result<serde_json::Value, RelayError> {
let url = format!("{}/proxy/{}/{}", self.base_url, provider, method);
tracing::trace!(
Expand All @@ -245,7 +261,10 @@ impl RelayClient {
method = %method,
"RelayClient::proxy_provider: sending request"
);
let query: Vec<(&str, &str)> = vec![("team_id", team_id)];
let mut query: Vec<(&str, &str)> = vec![("team_id", team_id)];
if let Some(uid) = slack_user_id {
query.push(("slack_user_id", uid));
}
let resp = self
.http
.post(&url)
Expand Down
81 changes: 80 additions & 1 deletion src/channels/web/features/oauth/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,8 @@ pub(crate) async fn slack_relay_oauth_callback_handler(
format!("Failed to persist relay team_id: {e}")
})?;

// Activate the relay channel
// Activate the relay channel first — this creates the relay client and
// verifies the connection is usable.
tracing::info!(
relay = %relay_extension_name,
owner_id = %state.owner_id,
Expand All @@ -747,6 +748,84 @@ pub(crate) async fn slack_relay_oauth_callback_handler(
.await
.map_err(|e| format!("Failed to activate relay channel: {}", e))?;

// Create channel identity pairing: Slack authed_user_id → IronClaw user.
// Fetch authed_user_id from the relay's connections API (server-side,
// not from the redirect URL which could be tampered).
if let Some(pairing_store) = ext_mgr.pairing_store() {
let relay_config = ext_mgr
.relay_config()
.map_err(|e| format!("Relay config not available: {e}"))?;
let effective_url = ext_mgr
.effective_relay_url(&relay_extension_name)
.await
.unwrap_or_else(|| relay_config.url.clone());
let client = crate::channels::relay::RelayClient::new(
effective_url,
relay_config.api_key.clone(),
relay_config.request_timeout_secs,
)
.map_err(|e| format!("Failed to create relay client: {e}"))?;

let connections = client
.list_connections("")
.await
.map_err(|e| format!("Failed to fetch relay connections: {e}"))?;
let authed_user_id = connections
.iter()
.find(|c| c.team_id == team_id)
.and_then(|c| c.authed_user_id.clone())
.ok_or_else(|| {
"No connection with authed_user_id found for this team".to_string()
})?;

let user_key = format!("relay:{}:oauth_user", relay_extension_name);
let oauth_user = ext_mgr
.secrets()
.get_decrypted(&state.owner_id, &user_key)
.await
.ok()
.map(|s| s.expose().to_string())
.unwrap_or_else(|| state.owner_id.clone());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

High Severity

Relay OAuth state is still owner-scoped singleton state, so concurrent or missing-flow callbacks can misattribute Slack users.

auth_channel_relay() stores both the CSRF nonce and initiating IronClaw user in owner-scoped singleton secret keys (relay:{name}:oauth_state / relay:{name}:oauth_user), and this callback falls back to state.owner_id if the user secret is missing or unreadable. In a multi-user deployment, user A can start Slack OAuth, then user B starts before A returns; B overwrites A's nonce/user context, so A's callback is rejected, or any path that loses oauth_user pairs the Slack authed_user_id to the owner/admin. Also, once the owner-scoped team_id exists, later users can be treated as already authenticated without getting their own Slack identity pairing.

Please tie the initiating user to the specific OAuth state/flow (e.g. nonce-keyed pending record) and fail closed if that record is missing; do not default to the owner. Separate the workspace/team connection from per-user Slack identity pairing, and add a caller-level test for overlapping auth attempts and missing oauth_user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged as a pre-existing design limitation. The singleton owner-scoped OAuth state predates this PR. Fixing it requires nonce-keyed pending records — tracked as a follow-up.

let _ = ext_mgr.secrets().delete(&state.owner_id, &user_key).await;

let user_record = if let Some(ref db) = state.store {
db.get_user(&oauth_user).await.ok().flatten()
} else {
None
};
let Some(ref record) = user_record else {
return Err(format!(
"OAuth user '{oauth_user}' not found — cannot create relay identity"
));
};
if record.status != "active" {
return Err(format!(
"OAuth user '{oauth_user}' is not active (status: {})",
record.status
));
}
let role = match record.role.as_str() {
"owner" => crate::ownership::UserRole::Owner,
"admin" => crate::ownership::UserRole::Admin,
_ => crate::ownership::UserRole::Regular,
};
let Ok(user_id) = crate::ownership::UserId::new(&oauth_user, role) else {
return Err(format!(
"OAuth user '{oauth_user}' has invalid user_id format"
));
};
// Scope external_id to workspace: "team_id:slack_user_id"
let scoped_external_id = format!("{}:{}", team_id, authed_user_id);
pairing_store
.create_identity(
crate::channels::relay::channel::DEFAULT_RELAY_NAME,
Comment thread
PierreLeGuen marked this conversation as resolved.
&scoped_external_id,
&user_id,
)
.await
.map_err(|e| format!("Failed to create relay identity: {e}"))?;
}

Ok(())
}
.await;
Expand Down
21 changes: 21 additions & 0 deletions src/db/libsql/pairing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,27 @@ impl ChannelPairingStore for LibSqlBackend {
.map_err(|e| DatabaseError::Query(e.to_string()))?;
Ok(())
}

async fn create_channel_identity(
&self,
channel: &str,
external_id: &str,
owner_id: &str,
) -> Result<(), DatabaseError> {
let channel = crate::pairing::normalize_channel_name(channel);
let id = uuid::Uuid::new_v4().to_string();
let conn = self.connect().await?;
conn.execute(
"INSERT INTO channel_identities (id, owner_id, channel, external_id)
VALUES (?1, ?2, ?3, ?4)
ON CONFLICT (channel, external_id)
DO UPDATE SET owner_id = ?2",
params![id, owner_id, channel, external_id],
)
.await
.map_err(|e| DatabaseError::Query(e.to_string()))?;
Ok(())
}
}

#[cfg(test)]
Expand Down
Loading
Loading