diff --git a/crates/manager-core/src/queues_api.rs b/crates/manager-core/src/queues_api.rs index 8dc670b..64807d2 100644 --- a/crates/manager-core/src/queues_api.rs +++ b/crates/manager-core/src/queues_api.rs @@ -18,6 +18,7 @@ use axum::{ }; use picloud_shared::{AppId, Principal}; use serde::Serialize; +use serde_json::json; use crate::app_repo::AppRepository; use crate::authz::{self, AuthzRepo, Capability}; @@ -46,12 +47,27 @@ pub enum QueuesApiError { impl axum::response::IntoResponse for QueuesApiError { fn into_response(self) -> axum::response::Response { - let status = match self { - Self::Forbidden => axum::http::StatusCode::FORBIDDEN, - Self::AppNotFound => axum::http::StatusCode::NOT_FOUND, - Self::Repo(_) => axum::http::StatusCode::INTERNAL_SERVER_ERROR, + // Match the JSON envelope shape used by every other admin api + // file (`{"error": "..."}`); previously this returned a + // plain-text body inconsistent with siblings. + let (status, body) = match &self { + Self::Forbidden => ( + axum::http::StatusCode::FORBIDDEN, + json!({ "error": self.to_string() }), + ), + Self::AppNotFound => ( + axum::http::StatusCode::NOT_FOUND, + json!({ "error": self.to_string() }), + ), + Self::Repo(e) => { + tracing::error!(error = %e, "queues admin backend error"); + ( + axum::http::StatusCode::INTERNAL_SERVER_ERROR, + json!({ "error": "internal error" }), + ) + } }; - (status, self.to_string()).into_response() + (status, Json(body)).into_response() } } @@ -203,3 +219,15 @@ async fn require_log_read( .map_err(|_| QueuesApiError::Forbidden)?; Ok(()) } + +// In-process unit tests for queues_api are intentionally not added. +// Constructing a `QueuesState` requires mocking five traits +// (`AppRepository`, `QueueRepo`, `TriggerRepo`, `ScriptRepository`, +// `AuthzRepo`) totalling 30+ methods just to exercise the resolver +// path. Integration tests in `crates/picloud/tests/` and the +// dashboard's `tests/e2e/navigation/tabs.spec.ts` already cover both +// the UUID and slug entry points end-to-end. The same applies to +// `files_api.rs`, `secrets_api.rs`, and `dead_letters_api.rs` — all +// three have zero in-process tests and would each need a similarly +// wide mock surface. Adding a shared mock-repo crate is the right +// long-term fix. diff --git a/crates/manager-core/src/topics_api.rs b/crates/manager-core/src/topics_api.rs index 39c5086..580c049 100644 --- a/crates/manager-core/src/topics_api.rs +++ b/crates/manager-core/src/topics_api.rs @@ -350,7 +350,44 @@ mod tests { } } - struct InMemoryAppRepo(AppId); + /// In-memory `AppRepository` for handler-level tests. Backs one + /// app at `(id, slug="test")` plus an optional history map so tests + /// can exercise slug-history redirect behavior. + struct InMemoryAppRepo { + id: AppId, + slug: String, + /// Historical slugs → current app id. Empty unless the test + /// explicitly registers one. + history: std::sync::Mutex>, + } + impl InMemoryAppRepo { + fn new(id: AppId) -> Self { + Self { + id, + slug: "test".into(), + history: std::sync::Mutex::new(std::collections::HashMap::new()), + } + } + #[allow(dead_code)] + fn with_history(self, old_slug: &str) -> Self { + self.history + .lock() + .unwrap() + .insert(old_slug.to_string(), self.id); + self + } + fn current_app(&self) -> App { + let now = Utc::now(); + App { + id: self.id, + slug: self.slug.clone(), + name: "test".into(), + description: None, + created_at: now, + updated_at: now, + } + } + } #[async_trait] impl AppRepository for InMemoryAppRepo { async fn list(&self) -> Result, ScriptRepositoryError> { @@ -360,27 +397,36 @@ mod tests { unimplemented!() } async fn get_by_id(&self, id: AppId) -> Result, ScriptRepositoryError> { - if id != self.0 { + if id != self.id { return Ok(None); } - let now = Utc::now(); - Ok(Some(App { - id, - slug: "test".into(), - name: "test".into(), - description: None, - created_at: now, - updated_at: now, - })) + Ok(Some(self.current_app())) } - async fn get_by_slug(&self, _: &str) -> Result, ScriptRepositoryError> { - unimplemented!() + async fn get_by_slug(&self, slug: &str) -> Result, ScriptRepositoryError> { + if slug == self.slug { + Ok(Some(self.current_app())) + } else { + Ok(None) + } } async fn get_by_slug_or_history( &self, - _: &str, + slug: &str, ) -> Result, ScriptRepositoryError> { - unimplemented!() + if slug == self.slug { + return Ok(Some(crate::app_repo::AppLookup { + app: self.current_app(), + redirected: false, + })); + } + let history = self.history.lock().unwrap(); + if history.contains_key(slug) { + return Ok(Some(crate::app_repo::AppLookup { + app: self.current_app(), + redirected: true, + })); + } + Ok(None) } async fn slug_in_history(&self, _: &str) -> Result, ScriptRepositoryError> { unimplemented!() @@ -487,7 +533,7 @@ mod tests { let bc = Arc::new(RecordingBroadcaster::default()); let state = TopicsState { topics: Arc::new(InMemoryTopicRepo::default()), - apps: Arc::new(InMemoryAppRepo(app_id)), + apps: Arc::new(InMemoryAppRepo::new(app_id)), authz, broadcaster: bc.clone(), }; @@ -626,4 +672,70 @@ mod tests { .unwrap_err(); assert!(matches!(err, TopicsApiError::Invalid(_))); } + + // ---------------------------------------------------------------- + // Slug-or-UUID path acceptance (post-aa493b9 refactor). + // Mirrors the same set of tests added across every refactored + // admin api file. + // ---------------------------------------------------------------- + + #[tokio::test] + async fn list_accepts_slug_path() { + let app = AppId::new(); + let (s, _) = state(app, Arc::new(PerAppAuthzRepo { granted_app: app })); + let Json(resp) = list_topics(State(s), Extension(member()), Path("test".into())) + .await + .expect("slug path should resolve"); + assert_eq!(resp.topics.len(), 0); + } + + #[tokio::test] + async fn list_unknown_slug_returns_app_not_found() { + let app = AppId::new(); + let (s, _) = state(app, Arc::new(PerAppAuthzRepo { granted_app: app })); + let err = list_topics(State(s), Extension(member()), Path("does-not-exist".into())) + .await + .expect_err("unknown slug should 404"); + assert!(matches!(err, TopicsApiError::AppNotFound)); + } + + #[tokio::test] + async fn list_historical_slug_resolves_silently() { + // Locks in current behavior: a request via an old slug after a + // rename returns OK with the renamed app's data, no signal back + // to the caller. The dashboard side handles the URL update via + // `redirect_to` on `GET /admin/apps/{slug}` separately. + let app = AppId::new(); + let bc = Arc::new(RecordingBroadcaster::default()); + let state = TopicsState { + topics: Arc::new(InMemoryTopicRepo::default()), + apps: Arc::new(InMemoryAppRepo::new(app).with_history("old-slug")), + authz: Arc::new(PerAppAuthzRepo { granted_app: app }), + broadcaster: bc, + }; + let Json(resp) = list_topics(State(state), Extension(member()), Path("old-slug".into())) + .await + .expect("historical slug should resolve to the renamed app"); + assert_eq!(resp.topics.len(), 0); + } + + #[tokio::test] + async fn create_accepts_slug_path() { + let app = AppId::new(); + let (s, _) = state(app, Arc::new(PerAppAuthzRepo { granted_app: app })); + let (status, Json(topic)) = create_topic( + State(s), + Extension(member()), + Path("test".into()), + Json(CreateTopicRequest { + name: "chat".into(), + external_subscribable: false, + auth_mode: TopicAuthMode::Public, + }), + ) + .await + .expect("slug-pathed mutation should succeed"); + assert_eq!(status, StatusCode::CREATED); + assert_eq!(topic.name, "chat"); + } } diff --git a/crates/manager-core/src/triggers_api.rs b/crates/manager-core/src/triggers_api.rs index 05c860f..cf07147 100644 --- a/crates/manager-core/src/triggers_api.rs +++ b/crates/manager-core/src/triggers_api.rs @@ -593,9 +593,16 @@ async fn delete_trigger( Path((id_or_slug, trigger_id)): Path<(String, TriggerId)>, ) -> Result { let app_id = resolve_app(&*s.apps, &id_or_slug).await?; - // Load the trigger so we can confirm it belongs to the right - // app; this prevents a caller from deleting a trigger by id alone - // when their capability is bound to a different app. + // Cap check first so an unauthorized caller can't probe trigger + // existence via 404-vs-403 discrimination. Only after we know the + // caller may manage triggers in this app do we load the row to + // verify the cross-app guard. + require( + s.authz.as_ref(), + &principal, + Capability::AppManageTriggers(app_id), + ) + .await?; let trigger = s .triggers .get(trigger_id) @@ -604,12 +611,6 @@ async fn delete_trigger( if trigger.app_id != app_id { return Err(TriggersApiError::NotFound(trigger_id)); } - require( - s.authz.as_ref(), - &principal, - Capability::AppManageTriggers(app_id), - ) - .await?; if !s.triggers.delete(trigger_id).await? { return Err(TriggersApiError::NotFound(trigger_id)); } @@ -1106,15 +1107,24 @@ mod tests { } async fn get_by_slug( &self, - _slug: &str, + slug: &str, ) -> Result, crate::repo::ScriptRepositoryError> { - unimplemented!() + let map = self.existing.lock().await; + Ok(map.values().find(|a| a.slug == slug).cloned()) } async fn get_by_slug_or_history( &self, - _slug: &str, + slug: &str, ) -> Result, crate::repo::ScriptRepositoryError> { - unimplemented!() + let map = self.existing.lock().await; + Ok(map + .values() + .find(|a| a.slug == slug) + .cloned() + .map(|app| AppLookup { + app, + redirected: false, + })) } async fn update( &self, @@ -1593,6 +1603,27 @@ mod tests { assert!(matches!(err, TriggersApiError::NotFound(_))); } + #[tokio::test] + async fn delete_without_manage_cap_is_forbidden_not_notfound() { + // Confirms the cap check fires before the trigger load — an + // unauthorized caller can't probe trigger existence by trying + // to delete one and reading 404 vs 403. + let app_id = AppId::new(); + let state = state_with(Arc::new(AlwaysDenyAuthzRepo), app_id); + let unknown_trigger = TriggerId::new(); + let res = delete_trigger( + State(state), + Extension(member_principal()), + Path((app_id.to_string(), unknown_trigger)), + ) + .await; + let err = res.expect_err("denied cap should surface as Forbidden"); + assert!( + matches!(err, TriggersApiError::Forbidden), + "got {err:?}; expected Forbidden so unauthorized callers can't probe existence" + ); + } + // ---------------------------------------------------------------- // v1.1.3: kind + cross-app target validation on trigger create. // ---------------------------------------------------------------- @@ -2220,4 +2251,36 @@ mod tests { TriggersApiError::Forbidden )); } + + // ---------------------------------------------------------------- + // Slug-or-UUID path acceptance (post-aa493b9 refactor). + // ---------------------------------------------------------------- + + #[tokio::test] + async fn list_accepts_slug_path() { + let app_id = AppId::new(); + let state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id); + let Json(resp) = list_triggers( + State(state), + Extension(member_principal()), + Path("test".into()), + ) + .await + .expect("slug path should resolve"); + assert!(resp.triggers.is_empty()); + } + + #[tokio::test] + async fn list_unknown_slug_returns_404() { + let app_id = AppId::new(); + let state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id); + let err = list_triggers( + State(state), + Extension(member_principal()), + Path("does-not-exist".into()), + ) + .await + .expect_err("unknown slug should 404"); + assert!(matches!(err, TriggersApiError::AppNotFound(_))); + } } diff --git a/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte b/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte index 18a40aa..32d02dd 100644 --- a/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte +++ b/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte @@ -1,5 +1,6 @@