From b192cf2cc9a8e3db526c5f4b05d294040b7fc26b Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sun, 7 Jun 2026 19:44:23 +0200 Subject: [PATCH] =?UTF-8?q?fix(shared):=20F-Q-004=20unify=20error=20varian?= =?UTF-8?q?ts=20=E2=80=94=20Forbidden=20on=20QueueError,=20rename=20Unavai?= =?UTF-8?q?lable=E2=86=92Backend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings QueueError / PubsubError / InvokeError in line with sibling shape used by KvError / DocsError / FilesError / SecretsError: - QueueError gains an explicit Forbidden variant (previously authz denial was squashed into Rejected("forbidden") in queue_service.rs:68, losing the structured variant a 403-translation layer would need). - QueueError::Unavailable renamed → Backend. - PubsubError::Unavailable renamed → Backend. - InvokeError::Unavailable renamed → Backend. - Call sites updated (queue_service, pubsub_service, invoke_service, Noop* stubs, From impl, one test assertion). - New unit test verifying authed-denied → QueueError::Forbidden. AUDIT.md anchor: F-Q-004. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/manager-core/src/invoke_service.rs | 6 +-- crates/manager-core/src/pubsub_service.rs | 10 ++--- crates/manager-core/src/queue_service.rs | 51 +++++++++++++++++++++-- crates/shared/src/invoke.rs | 9 ++-- crates/shared/src/pubsub.rs | 9 ++-- crates/shared/src/queue.rs | 18 +++++--- 6 files changed, 78 insertions(+), 25 deletions(-) diff --git a/crates/manager-core/src/invoke_service.rs b/crates/manager-core/src/invoke_service.rs index fe6d0b1..6dfb88f 100644 --- a/crates/manager-core/src/invoke_service.rs +++ b/crates/manager-core/src/invoke_service.rs @@ -49,7 +49,7 @@ impl InvokeServiceImpl { .scripts .get(script_id) .await - .map_err(|e| InvokeError::Unavailable(e.to_string()))? + .map_err(|e| InvokeError::Backend(e.to_string()))? .ok_or_else(|| InvokeError::NotFound(format!("id {script_id}")))?; if script.app_id != cx.app_id { return Err(InvokeError::CrossApp); @@ -72,7 +72,7 @@ impl InvokeServiceImpl { .scripts .get_by_name(cx.app_id, name) .await - .map_err(|e| InvokeError::Unavailable(e.to_string()))? + .map_err(|e| InvokeError::Backend(e.to_string()))? .ok_or_else(|| InvokeError::NotFound(format!("name {name:?}")))?; Ok(ResolvedScript { script_id: script.id, @@ -156,7 +156,7 @@ impl InvokeService for InvokeServiceImpl { root_execution_id: Some(cx.root_execution_id), }) .await - .map_err(|e| InvokeError::Unavailable(e.to_string()))?; + .map_err(|e| InvokeError::Backend(e.to_string()))?; Ok(execution_id) } } diff --git a/crates/manager-core/src/pubsub_service.rs b/crates/manager-core/src/pubsub_service.rs index 2f5658c..13c6451 100644 --- a/crates/manager-core/src/pubsub_service.rs +++ b/crates/manager-core/src/pubsub_service.rs @@ -129,7 +129,7 @@ impl PubsubServiceImpl { impl From for PubsubError { fn from(e: PubsubRepoError) -> Self { - Self::Unavailable(e.to_string()) + Self::Backend(e.to_string()) } } @@ -227,7 +227,7 @@ impl PubsubService for PubsubServiceImpl { let (Some(topic_repo), Some(secrets)) = (self.topics.as_ref(), self.secrets.as_ref()) else { - return Err(PubsubError::Unavailable( + return Err(PubsubError::Backend( "subscriber tokens are not wired in".into(), )); }; @@ -253,7 +253,7 @@ impl PubsubService for PubsubServiceImpl { let registered = topic_repo .get(cx.app_id, name) .await - .map_err(|e| PubsubError::Unavailable(e.to_string()))?; + .map_err(|e| PubsubError::Backend(e.to_string()))?; if !registered.is_some_and(|t| t.external_subscribable) { return Err(PubsubError::SubscriberToken(format!( "pubsub::subscriber_token: topic {name} is not externally subscribable" @@ -264,7 +264,7 @@ impl PubsubService for PubsubServiceImpl { let key = secrets .get_or_create_signing_key(cx.app_id) .await - .map_err(|e| PubsubError::Unavailable(e.to_string()))?; + .map_err(|e| PubsubError::Backend(e.to_string()))?; let now = chrono::Utc::now().timestamp(); let claims = TokenClaims { app_id: cx.app_id, @@ -472,7 +472,7 @@ mod tests { .publish_durable(&anon_cx(app), "user.created", serde_json::json!(1)) .await .unwrap_err(); - assert!(matches!(err, PubsubError::Unavailable(_))); + assert!(matches!(err, PubsubError::Backend(_))); // Rollback: no partial fan-out survived. assert_eq!(repo.written_count(), 0); } diff --git a/crates/manager-core/src/queue_service.rs b/crates/manager-core/src/queue_service.rs index c21e7f5..5558dec 100644 --- a/crates/manager-core/src/queue_service.rs +++ b/crates/manager-core/src/queue_service.rs @@ -65,7 +65,7 @@ impl QueueService for QueueServiceImpl { Capability::AppQueueEnqueue(cx.app_id), ) .await - .map_err(|_| QueueError::Rejected("forbidden".into()))?; + .map_err(|_| QueueError::Forbidden)?; } let deliver_after = opts @@ -88,7 +88,7 @@ impl QueueService for QueueServiceImpl { enqueued_by_principal, }) .await - .map_err(|e| QueueError::Unavailable(e.to_string())) + .map_err(|e| QueueError::Backend(e.to_string())) } async fn depth(&self, cx: &SdkCallCx, queue_name: &str) -> Result { @@ -98,7 +98,7 @@ impl QueueService for QueueServiceImpl { self.repo .depth(cx.app_id, queue_name) .await - .map_err(|e| QueueError::Unavailable(e.to_string())) + .map_err(|e| QueueError::Backend(e.to_string())) } async fn depth_pending(&self, cx: &SdkCallCx, queue_name: &str) -> Result { @@ -108,7 +108,7 @@ impl QueueService for QueueServiceImpl { self.repo .depth_pending(cx.app_id, queue_name) .await - .map_err(|e| QueueError::Unavailable(e.to_string())) + .map_err(|e| QueueError::Backend(e.to_string())) } } @@ -312,6 +312,49 @@ mod tests { assert!(captured.deliver_after.is_none()); } + struct DenyAllAuthz; + #[async_trait] + impl AuthzRepo for DenyAllAuthz { + async fn membership( + &self, + _user_id: picloud_shared::UserId, + _app_id: AppId, + ) -> Result, AuthzError> { + Ok(None) + } + } + + #[tokio::test] + async fn authed_principal_denied_returns_forbidden_variant() { + use picloud_shared::{InstanceRole, Principal, UserId}; + let repo = Arc::new(CapturingRepo { + last: tokio::sync::Mutex::new(None), + }); + let svc = QueueServiceImpl::new(repo, Arc::new(DenyAllAuthz)); + let exec = ExecutionId::new(); + let cx = SdkCallCx { + app_id: AppId::new(), + script_id: ScriptId::new(), + principal: Some(Principal { + user_id: UserId::new(), + instance_role: InstanceRole::Member, + scopes: None, + app_binding: None, + }), + execution_id: exec, + request_id: RequestId::new(), + trigger_depth: 0, + root_execution_id: exec, + is_dead_letter_handler: false, + event: None, + }; + let err = svc + .enqueue(&cx, "x", serde_json::Value::Null, EnqueueOpts::default()) + .await + .unwrap_err(); + assert!(matches!(err, QueueError::Forbidden)); + } + #[tokio::test] async fn depth_passes_through() { let repo = Arc::new(CapturingRepo { diff --git a/crates/shared/src/invoke.rs b/crates/shared/src/invoke.rs index 78e9740..3fab55e 100644 --- a/crates/shared/src/invoke.rs +++ b/crates/shared/src/invoke.rs @@ -105,9 +105,10 @@ pub enum InvokeError { #[error("invoke rejected: {0}")] Rejected(String), - /// Backend / database error. + /// Backend / database error. Named `Backend` to align with KvError / + /// DocsError / FilesError / SecretsError / QueueError / PubsubError. #[error("invoke backend error: {0}")] - Unavailable(String), + Backend(String), } /// Test-only stub: every call errors. Lets harnesses build a `Services` @@ -122,7 +123,7 @@ impl InvokeService for NoopInvokeService { _cx: &SdkCallCx, _target: InvokeTarget, ) -> Result { - Err(InvokeError::Unavailable("invoke is not wired in".into())) + Err(InvokeError::Backend("invoke is not wired in".into())) } async fn enqueue_async( @@ -131,6 +132,6 @@ impl InvokeService for NoopInvokeService { _target: InvokeTarget, _args: serde_json::Value, ) -> Result { - Err(InvokeError::Unavailable("invoke is not wired in".into())) + Err(InvokeError::Backend("invoke is not wired in".into())) } } diff --git a/crates/shared/src/pubsub.rs b/crates/shared/src/pubsub.rs index 68ff3b6..4cce6d1 100644 --- a/crates/shared/src/pubsub.rs +++ b/crates/shared/src/pubsub.rs @@ -52,7 +52,7 @@ pub trait PubsubService: Send + Sync { ttl_seconds: Option, ) -> Result { let _ = (cx, topics, ttl_seconds); - Err(PubsubError::Unavailable( + Err(PubsubError::Backend( "subscriber tokens are not wired in".into(), )) } @@ -80,9 +80,10 @@ pub enum PubsubError { #[error("{0}")] SubscriberToken(String), - /// Anything else — Postgres unavailable, etc. + /// Anything else — Postgres unavailable, etc. Named `Backend` to + /// align with KvError / DocsError / FilesError / SecretsError. #[error("pubsub backend error: {0}")] - Unavailable(String), + Backend(String), } /// Match a stored `topic_pattern` against a published `topic`. @@ -145,7 +146,7 @@ impl PubsubService for NoopPubsubService { _topic: &str, _message: serde_json::Value, ) -> Result<(), PubsubError> { - Err(PubsubError::Unavailable("pubsub is not wired in".into())) + Err(PubsubError::Backend("pubsub is not wired in".into())) } } diff --git a/crates/shared/src/queue.rs b/crates/shared/src/queue.rs index a4c1c2c..f621a04 100644 --- a/crates/shared/src/queue.rs +++ b/crates/shared/src/queue.rs @@ -61,6 +61,13 @@ pub enum QueueError { #[error("queue_name must not be empty")] EmptyName, + /// Caller principal lacked the required capability. Only raised when + /// `cx.principal.is_some()` (script-as-gate; public HTTP skips it). + /// Sibling shape across KvError / DocsError / PubsubError — uniform + /// for 403 translation at the HTTP boundary. + #[error("forbidden")] + Forbidden, + /// Payload could not be serialized (e.g. a Rhai FnPtr / closure /// captured into the message). Rejected at the SDK boundary; surface /// the wording verbatim so scripts see the documented message. @@ -71,9 +78,10 @@ pub enum QueueError { #[error("{0}")] InvalidOpts(String), - /// Backend unavailable (Postgres down, etc.). + /// Backend failure (Postgres unavailable, etc.). Named `Backend` to + /// align with KvError / DocsError / FilesError / SecretsError. #[error("queue backend error: {0}")] - Unavailable(String), + Backend(String), } /// Test-only stub: every call errors `Unavailable`. Used by harnesses @@ -90,14 +98,14 @@ impl QueueService for NoopQueueService { _payload: serde_json::Value, _opts: EnqueueOpts, ) -> Result { - Err(QueueError::Unavailable("queue is not wired in".into())) + Err(QueueError::Backend("queue is not wired in".into())) } async fn depth(&self, _cx: &SdkCallCx, _queue_name: &str) -> Result { - Err(QueueError::Unavailable("queue is not wired in".into())) + Err(QueueError::Backend("queue is not wired in".into())) } async fn depth_pending(&self, _cx: &SdkCallCx, _queue_name: &str) -> Result { - Err(QueueError::Unavailable("queue is not wired in".into())) + Err(QueueError::Backend("queue is not wired in".into())) } }