From 0a3a9d63b5cb861b40dbc6a053529c728ae8551c Mon Sep 17 00:00:00 2001
From: Phoebe Goldman <phoebe@clockworklabs.io>
Date: Thu, 28 Sep 2023 17:06:13 -0400
Subject: [PATCH] Return the formatted error from `log_and_500`, so the CLI can
 report it (#338)

In the client API's log_and_500,
return an ErrorResponse with a body derived from the error,
rather than a bare 500 code.

Note that this exposes previously-internal error messages to users.
---
 crates/client-api/src/lib.rs             |  5 +++--
 crates/client-api/src/routes/database.rs | 17 ++++++-----------
 crates/standalone/src/lib.rs             |  3 +++
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/crates/client-api/src/lib.rs b/crates/client-api/src/lib.rs
index e42e63835f9..8afd1dae11f 100644
--- a/crates/client-api/src/lib.rs
+++ b/crates/client-api/src/lib.rs
@@ -1,6 +1,7 @@
 use std::sync::Arc;
 
 use async_trait::async_trait;
+use axum::response::ErrorResponse;
 use http::StatusCode;
 
 use spacetimedb::address::Address;
@@ -477,7 +478,7 @@ impl<T: NodeDelegate + ?Sized> NodeDelegate for Arc<T> {
     }
 }
 
-pub fn log_and_500(e: impl std::fmt::Display) -> StatusCode {
+pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse {
     log::error!("internal error: {e:#}");
-    StatusCode::INTERNAL_SERVER_ERROR
+    (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into()
 }
diff --git a/crates/client-api/src/routes/database.rs b/crates/client-api/src/routes/database.rs
index 3df57913170..ca70a4cc0e5 100644
--- a/crates/client-api/src/routes/database.rs
+++ b/crates/client-api/src/routes/database.rs
@@ -473,7 +473,7 @@ fn mime_ndjson() -> mime::Mime {
 async fn worker_ctx_find_database(
     worker_ctx: &(impl ControlStateDelegate + ?Sized),
     address: &Address,
-) -> Result<Option<Database>, StatusCode> {
+) -> axum::response::Result<Option<Database>> {
     worker_ctx.get_database_by_address(address).map_err(log_and_500)
 }
 
@@ -603,11 +603,6 @@ pub struct RegisterTldParams {
     tld: String,
 }
 
-fn auth_or_bad_request(auth: SpacetimeAuthHeader) -> axum::response::Result<SpacetimeAuth> {
-    auth.get()
-        .ok_or((StatusCode::BAD_REQUEST, "Invalid credentials.").into())
-}
-
 pub async fn register_tld<S: ControlStateDelegate>(
     State(ctx): State<S>,
     Query(RegisterTldParams { tld }): Query<RegisterTldParams>,
@@ -615,7 +610,7 @@ pub async fn register_tld<S: ControlStateDelegate>(
 ) -> axum::response::Result<impl IntoResponse> {
     // You should not be able to publish to a database that you do not own
     // so, unless you are the owner, this will fail, hence not using get_or_create
-    let auth = auth_or_bad_request(auth)?;
+    let auth = auth_or_unauth(auth)?;
 
     let tld = tld.parse::<DomainName>().map_err(DomainParsingRejection)?.into();
     let result = ctx.register_tld(&auth.identity, tld).await.map_err(log_and_500)?;
@@ -750,7 +745,7 @@ pub async fn publish<S: NodeDelegate + ControlStateDelegate>(
 
     // You should not be able to publish to a database that you do not own
     // so, unless you are the owner, this will fail.
-    let auth = auth_or_bad_request(auth)?;
+    let auth = auth_or_unauth(auth)?;
 
     let (db_addr, db_name) = match name_or_address {
         Some(noa) => match noa.try_resolve(&ctx).await? {
@@ -838,7 +833,7 @@ pub async fn delete_database<S: ControlStateDelegate>(
     Path(DeleteDatabaseParams { address }): Path<DeleteDatabaseParams>,
     auth: SpacetimeAuthHeader,
 ) -> axum::response::Result<impl IntoResponse> {
-    let auth = auth_or_bad_request(auth)?;
+    let auth = auth_or_unauth(auth)?;
 
     ctx.delete_database(&auth.identity, &address)
         .await
@@ -858,7 +853,7 @@ pub async fn set_name<S: ControlStateDelegate>(
     Query(SetNameQueryParams { domain, address }): Query<SetNameQueryParams>,
     auth: SpacetimeAuthHeader,
 ) -> axum::response::Result<impl IntoResponse> {
-    let auth = auth_or_bad_request(auth)?;
+    let auth = auth_or_unauth(auth)?;
 
     let database = ctx
         .get_database_by_address(&address)
@@ -874,7 +869,7 @@ pub async fn set_name<S: ControlStateDelegate>(
         .create_dns_record(&auth.identity, &domain, &address)
         .await
         .map_err(|err| match err {
-            spacetimedb::control_db::Error::RecordAlreadyExists(_) => StatusCode::CONFLICT,
+            spacetimedb::control_db::Error::RecordAlreadyExists(_) => StatusCode::CONFLICT.into(),
             _ => log_and_500(err),
         })?;
 
diff --git a/crates/standalone/src/lib.rs b/crates/standalone/src/lib.rs
index 0bb619ddec3..77811c4f949 100644
--- a/crates/standalone/src/lib.rs
+++ b/crates/standalone/src/lib.rs
@@ -331,6 +331,9 @@ impl spacetimedb_client_api::ControlStateWriteAccess for StandaloneEnv {
             return Ok(());
         };
         if &database.identity != identity {
+            // TODO: `PermissionDenied` should be a variant of `Error`,
+            //       so we can match on it and return better error responses
+            //       from HTTP endpoints.
             return Err(anyhow!(
                 "Permission denied: `{}` does not own database `{}`",
                 identity.to_hex(),