fix(#3422): give bridge-created identities an email too
#3414 fixed the missing-email defect in swarmctl only, so every identity created by the bridge landed in users.yml with no `email`. A relying party that asks for the claim does not degrade, it fails -- grafana's OIDC login is the measured case (#3393). Two writers of one file disagreeing about a field one of them treats as required is not a difference worth keeping, and with the file shared the result also depended on which tool wrote last. Mutation-checked: removing the fill turns the new test red and nothing else.
This commit is contained in:
parent
af1203c78b
commit
1885022d02
2 changed files with 61 additions and 4 deletions
|
|
@ -278,7 +278,7 @@ async fn handle(state: &AppState, name: String) -> Result<BridgeResponse> {
|
||||||
extra: std::collections::BTreeMap::new(),
|
extra: std::collections::BTreeMap::new(),
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
store::publish(&cfg.users_file, &user_store)?;
|
store::publish(&cfg.users_file, &mut user_store)?;
|
||||||
tracing::info!(agent = %name, "created authelia identity");
|
tracing::info!(agent = %name, "created authelia identity");
|
||||||
Ok(BridgeResponse::Created)
|
Ok(BridgeResponse::Created)
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -169,7 +169,8 @@ pub fn load_store(users_file: &Path) -> Result<UserStore> {
|
||||||
/// against the pinned 4.39.20 build during this design work — a
|
/// against the pinned 4.39.20 build during this design work — a
|
||||||
/// restart would drop every active SSO session, which mara ruled out for
|
/// restart would drop every active SSO session, which mara ruled out for
|
||||||
/// agent creation specifically (not a rare, human-initiated event).
|
/// agent creation specifically (not a rare, human-initiated event).
|
||||||
pub fn publish(users_file: &Path, store: &UserStore) -> Result<()> {
|
pub fn publish(users_file: &Path, store: &mut UserStore) -> Result<()> {
|
||||||
|
fill_missing_emails(store);
|
||||||
// One file, so there is no longer an ordering question between two
|
// One file, so there is no longer an ordering question between two
|
||||||
// writes — the old version wrote the JSON store first so a crash
|
// writes — the old version wrote the JSON store first so a crash
|
||||||
// between them left something to repair from. That whole failure mode
|
// between them left something to repair from. That whole failure mode
|
||||||
|
|
@ -178,6 +179,30 @@ pub fn publish(users_file: &Path, store: &UserStore) -> Result<()> {
|
||||||
write_atomic(users_file, &rendered)
|
write_atomic(users_file, &rendered)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Domain for an address this bridge invents — the same one
|
||||||
|
/// `swarmctl::users` uses, and the same one `hive-c0re` already gives every
|
||||||
|
/// agent's forge account. Never routable, deliberately: nothing sends mail
|
||||||
|
/// here, the address exists so a relying party asking for an `email` claim
|
||||||
|
/// gets one.
|
||||||
|
const SYNTHETIC_EMAIL_DOMAIN: &str = "hyperhive.local";
|
||||||
|
|
||||||
|
/// Give every user an address before the file is written.
|
||||||
|
///
|
||||||
|
/// 🩸 **This bridge did not do it, and `swarmctl` did.** #3414 fixed the
|
||||||
|
/// missing-email defect in `swarmctl` only, so every identity created *here*
|
||||||
|
/// landed in `users.yml` with no `email` — and a relying party that asks for
|
||||||
|
/// the claim does not degrade, it fails (grafana's OIDC login is the
|
||||||
|
/// measured case, #3393). Two writers of one file disagreeing about a
|
||||||
|
/// required field is not a difference worth keeping, so the rule now lives
|
||||||
|
/// on both write paths.
|
||||||
|
fn fill_missing_emails(store: &mut UserStore) {
|
||||||
|
for (name, user) in &mut store.users {
|
||||||
|
if user.email.is_none() {
|
||||||
|
user.email = Some(format!("{name}@{SYNTHETIC_EMAIL_DOMAIN}"));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Replace `path`'s contents atomically. Unlike `swarmctl::write_atomic`,
|
/// Replace `path`'s contents atomically. Unlike `swarmctl::write_atomic`,
|
||||||
/// this does **not** need to preserve a foreign owner via `chown` — this
|
/// this does **not** need to preserve a foreign owner via `chown` — this
|
||||||
/// process runs as `users_file`'s own owning user (see the module doc:
|
/// process runs as `users_file`'s own owning user (see the module doc:
|
||||||
|
|
@ -317,6 +342,38 @@ some_future_top_level_key: 7
|
||||||
|
|
||||||
/// `write_atomic` round-trips through a real temp dir — the property
|
/// `write_atomic` round-trips through a real temp dir — the property
|
||||||
/// under test is the rename-into-place, not just the render.
|
/// under test is the rename-into-place, not just the render.
|
||||||
|
/// 🩸 The asymmetry this fixes: #3414 gave `swarmctl`-created humans a
|
||||||
|
/// synthetic address and left bridge-created **agents** with none, so
|
||||||
|
/// two writers of one file disagreed about a field a relying party
|
||||||
|
/// treats as required — grafana's OIDC login fails outright without it
|
||||||
|
/// (#3393) rather than degrading.
|
||||||
|
///
|
||||||
|
/// Asserted through the real write path, because that is where the rule
|
||||||
|
/// lives: `handle` inserts a user with `email: None` and nothing between
|
||||||
|
/// there and the file would notice.
|
||||||
|
#[test]
|
||||||
|
fn an_agent_identity_reaches_the_file_with_an_email() {
|
||||||
|
let dir = tempdir();
|
||||||
|
let users_file = dir.join("users.yml");
|
||||||
|
|
||||||
|
let mut store = UserStore::default();
|
||||||
|
store.users.insert("atlas".to_owned(), user("$argon2id$x"));
|
||||||
|
assert!(
|
||||||
|
store.users["atlas"].email.is_none(),
|
||||||
|
"the fixture must start with the state `handle` creates"
|
||||||
|
);
|
||||||
|
publish(&users_file, &mut store).expect("publish");
|
||||||
|
|
||||||
|
let reloaded = load_store(&users_file).expect("reload");
|
||||||
|
assert_eq!(
|
||||||
|
reloaded.users["atlas"].email.as_deref(),
|
||||||
|
Some("atlas@hyperhive.local"),
|
||||||
|
"an agent must not land in the file without an email"
|
||||||
|
);
|
||||||
|
|
||||||
|
fs::remove_dir_all(&dir).ok();
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn publish_then_load_round_trips_through_the_real_file() {
|
fn publish_then_load_round_trips_through_the_real_file() {
|
||||||
let dir = tempdir();
|
let dir = tempdir();
|
||||||
|
|
@ -325,7 +382,7 @@ some_future_top_level_key: 7
|
||||||
|
|
||||||
let mut store = UserStore::default();
|
let mut store = UserStore::default();
|
||||||
store.users.insert("atlas".to_owned(), user("$argon2id$x"));
|
store.users.insert("atlas".to_owned(), user("$argon2id$x"));
|
||||||
publish(&users_file, &store).expect("publish");
|
publish(&users_file, &mut store).expect("publish");
|
||||||
|
|
||||||
let reloaded = load_store(&users_file).expect("reload");
|
let reloaded = load_store(&users_file).expect("reload");
|
||||||
assert!(reloaded.users.contains_key("atlas"));
|
assert!(reloaded.users.contains_key("atlas"));
|
||||||
|
|
@ -353,7 +410,7 @@ some_future_top_level_key: 7
|
||||||
store
|
store
|
||||||
.users
|
.users
|
||||||
.insert("atlas".to_owned(), user("$argon2id$second"));
|
.insert("atlas".to_owned(), user("$argon2id$second"));
|
||||||
publish(&users_file, &store).expect("publish");
|
publish(&users_file, &mut store).expect("publish");
|
||||||
let reloaded = load_store(&users_file).expect("reload");
|
let reloaded = load_store(&users_file).expect("reload");
|
||||||
assert!(reloaded.users.contains_key("mara"), "existing user lost");
|
assert!(reloaded.users.contains_key("mara"), "existing user lost");
|
||||||
assert!(reloaded.users.contains_key("atlas"));
|
assert!(reloaded.users.contains_key("atlas"));
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue