swarm-controller: refuse a link over any stored object, decodable or not
The existence check read the path as the route's credential type, so an object stored there that no longer decodes as one answered 500 instead of 409. It now reads the path untyped: anything stored holds the name.
This commit is contained in:
parent
8ad2af735e
commit
5971b51e65
2 changed files with 37 additions and 12 deletions
|
|
@ -24,7 +24,7 @@ use std::future::Future;
|
||||||
use axum::Json;
|
use axum::Json;
|
||||||
use axum::extract::State;
|
use axum::extract::State;
|
||||||
use axum::http::StatusCode;
|
use axum::http::StatusCode;
|
||||||
use serde::de::DeserializeOwned;
|
use serde::de::{DeserializeOwned, IgnoredAny};
|
||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
use swarm_secret_client::{Error, SecretStore, forge, github, matrix};
|
use swarm_secret_client::{Error, SecretStore, forge, github, matrix};
|
||||||
use utoipa::{IntoParams, ToSchema};
|
use utoipa::{IntoParams, ToSchema};
|
||||||
|
|
@ -226,12 +226,10 @@ impl Linking {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// [`Linking::Exists`] when an account is stored at `path`.
|
/// [`Linking::Exists`] when any object is stored at `path`, whatever its
|
||||||
pub(crate) async fn refuse_linked<T: DeserializeOwned + Send>(
|
/// shape: one that no longer decodes as an account still holds the name.
|
||||||
store: &impl AccountStore,
|
pub(crate) async fn refuse_linked(store: &impl AccountStore, path: &str) -> Result<(), Linking> {
|
||||||
path: &str,
|
match store.read_optional::<IgnoredAny>(path).await {
|
||||||
) -> Result<(), Linking> {
|
|
||||||
match store.read_optional::<T>(path).await {
|
|
||||||
Ok(None) => Ok(()),
|
Ok(None) => Ok(()),
|
||||||
Ok(Some(_)) => Err(Linking::Exists),
|
Ok(Some(_)) => Err(Linking::Exists),
|
||||||
Err(e) => Err(Linking::Store(e)),
|
Err(e) => Err(Linking::Store(e)),
|
||||||
|
|
@ -242,12 +240,12 @@ pub(crate) async fn refuse_linked<T: DeserializeOwned + Send>(
|
||||||
///
|
///
|
||||||
/// A read then a write, not one atomic step: two links racing for one path can
|
/// A read then a write, not one atomic step: two links racing for one path can
|
||||||
/// both pass the check, and the later write wins.
|
/// both pass the check, and the later write wins.
|
||||||
pub(crate) async fn link<T: Serialize + DeserializeOwned + Send + Sync>(
|
pub(crate) async fn link<T: Serialize + Sync>(
|
||||||
store: &impl AccountStore,
|
store: &impl AccountStore,
|
||||||
path: &str,
|
path: &str,
|
||||||
value: &T,
|
value: &T,
|
||||||
) -> Result<(), Linking> {
|
) -> Result<(), Linking> {
|
||||||
refuse_linked::<T>(store, path).await?;
|
refuse_linked(store, path).await?;
|
||||||
store.write(path, value).await.map_err(Linking::Store)
|
store.write(path, value).await.map_err(Linking::Store)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -846,6 +844,35 @@ pub(crate) mod tests {
|
||||||
assert!(matches!(removal, Removal::Store(_)), "{removal:?}");
|
assert!(matches!(removal, Removal::Store(_)), "{removal:?}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn an_object_that_no_longer_decodes_still_refuses_a_link() {
|
||||||
|
let path = "swarm/agents/atlas/github-token";
|
||||||
|
let stale = json!({"token": 1});
|
||||||
|
// The control: a typed read of this object fails, so the 409 below is
|
||||||
|
// not a decode that happened to succeed.
|
||||||
|
assert!(serde_json::from_value::<github::Credential>(stale.clone()).is_err());
|
||||||
|
let store = FakeStore::default().with(path, stale);
|
||||||
|
|
||||||
|
let linking = super::link(
|
||||||
|
&store,
|
||||||
|
path,
|
||||||
|
&github::Credential {
|
||||||
|
value: "t0k3n-new".to_owned(),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.expect_err("an object is stored");
|
||||||
|
|
||||||
|
assert!(matches!(linking, super::Linking::Exists), "{linking:?}");
|
||||||
|
assert_eq!(
|
||||||
|
linking
|
||||||
|
.problem("agent atlas already has a github token")
|
||||||
|
.status,
|
||||||
|
Some(axum::http::StatusCode::CONFLICT)
|
||||||
|
);
|
||||||
|
assert!(store.written().is_empty());
|
||||||
|
}
|
||||||
|
|
||||||
/// Bare-minimum `AppState`, as `matrix_account`'s tests build it.
|
/// Bare-minimum `AppState`, as `matrix_account`'s tests build it.
|
||||||
fn state() -> super::super::AppState {
|
fn state() -> super::super::AppState {
|
||||||
super::super::AppState {
|
super::super::AppState {
|
||||||
|
|
|
||||||
|
|
@ -200,9 +200,7 @@ async fn link_matrix(
|
||||||
tracing::warn!(path, error = ?e, "linking the matrix account failed");
|
tracing::warn!(path, error = ?e, "linking the matrix account failed");
|
||||||
Box::new(e.problem(existing))
|
Box::new(e.problem(existing))
|
||||||
};
|
};
|
||||||
refuse_linked::<matrix::Credential>(store, path)
|
refuse_linked(store, path).await.map_err(refused)?;
|
||||||
.await
|
|
||||||
.map_err(refused)?;
|
|
||||||
let (token, homeserver, user_id) = resolve_credential(req).await?;
|
let (token, homeserver, user_id) = resolve_credential(req).await?;
|
||||||
link(
|
link(
|
||||||
store,
|
store,
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue