diff --git a/crates/didbot-pds/src/names.rs b/crates/didbot-pds/src/names.rs index 7e184b4e..901eb8db 100644 --- a/crates/didbot-pds/src/names.rs +++ b/crates/didbot-pds/src/names.rs @@ -561,9 +561,10 @@ pub struct Naming { /// see the module docs — so a hold on its own does not say which zone a /// name belongs to. This is what lets a zone's teardown gate ask "does /// any unexpired hold belong to *this* zone" rather than "are there any - /// unexpired holds anywhere". Never removed: a released label's zone - /// still matters for exactly as long as its hold does, which - /// [`Naming::labels_held_under`] is what checks. + /// unexpired holds anywhere". A released label's zone still matters for + /// exactly as long as its hold does, which [`Naming::labels_held_under`] + /// is what checks, so [`Naming::release_handle`] drops it on the same + /// sweep that forgets the expired hold. label_zone: Mutex>, } @@ -727,10 +728,23 @@ impl Naming { if let Some(label) = label_of(handle, zone) { self.registry.release(&label, now); let forgotten = self.registry.prune(now); - tracing::info!(name = %label, forgotten, "released the name"); + let unzoned = self.forget_free_labels(now); + tracing::info!(name = %label, forgotten, unzoned, "released the name"); } } + /// Drops the zone of every label the registry no longer holds, and + /// answers how many. A free label belongs to no zone's teardown gate. + fn forget_free_labels(&self, now: OffsetDateTime) -> usize { + let mut label_zone = self + .label_zone + .lock() + .expect("naming's zone index poisoned"); + let before = label_zone.len(); + label_zone.retain(|label, _| !self.registry.is_free(label, now)); + before - label_zone.len() + } + /// Permanently burns the label in `handle`, on a soft or hard delete. /// /// Takes the handle rather than the label for the same reason @@ -871,6 +885,34 @@ mod tests { OffsetDateTime::UNIX_EPOCH + Duration::days(days) } + /// A released label keeps its zone while its hold lasts, because a + /// zone's teardown gate asks for it, and loses it on the first release + /// after the hold expires. + #[test] + fn a_label_leaves_the_zone_index_once_its_hold_expires() { + let zone = "agents.example"; + let naming = + Naming::new(Arc::new(didbot_name::generated::Timestamp)).with_hold(Duration::days(1)); + for hint in ["first-agent", "second-agent"] { + naming + .issue("token", zone, Some(hint), None, at(0)) + .expect("free"); + } + let indexed = |naming: &Naming| { + let mut labels: Vec = + naming.label_zone.lock().unwrap().keys().cloned().collect(); + labels.sort(); + labels + }; + + naming.release_handle("first-agent.agents.example", zone, at(1)); + assert_eq!(indexed(&naming), ["first-agent", "second-agent"]); + + naming.release_handle("second-agent.agents.example", zone, at(3)); + assert_eq!(indexed(&naming), ["second-agent"]); + assert_eq!(naming.labels_held_under(zone, at(3)), ["second-agent"]); + } + #[test] fn a_free_name_can_be_claimed_once() { let names = NameRegistry::default(); diff --git a/crates/didbot-serve/src/oauth/par.rs b/crates/didbot-serve/src/oauth/par.rs index 5d59ee99..5dad4f7b 100644 --- a/crates/didbot-serve/src/oauth/par.rs +++ b/crates/didbot-serve/src/oauth/par.rs @@ -198,6 +198,12 @@ impl MemoryParStore { pub fn new() -> Self { Self::default() } + + /// How many pushed requests this store still holds. Test use only. + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.entries.read().unwrap_or_else(|p| p.into_inner()).len() + } } fn random_request_uri() -> String { @@ -215,10 +221,13 @@ impl ParStore for MemoryParStore { fn push(&self, request: PushedRequest) -> String { let uri = self.mint(); - self.entries - .write() - .unwrap_or_else(|p| p.into_inner()) - .insert(uri.clone(), request); + let now = OffsetDateTime::now_utc(); + let mut entries = self.entries.write().unwrap_or_else(|p| p.into_inner()); + // `take` removes only the request it is handed, so one nobody ever + // takes — an abandoned sign-in — is swept here once it has expired, + // the same way `MemoryConsentStore::mint` sweeps its own map. + entries.retain(|_, pushed| now < pushed.expires_at); + entries.insert(uri.clone(), request); uri } @@ -847,6 +856,31 @@ mod tests { assert!(matches!(err, ParError::RedirectUriNotRegistered(_))); } + /// A pushed request nobody takes is swept by the next push once it has + /// expired, so an abandoned sign-in does not stay until restart. + #[tokio::test] + async fn an_untaken_request_is_swept_once_it_has_expired() { + let fixture = Fixture::new(); + let store = MemoryParStore::new(); + let (abandoned, _) = push(&fixture, &store, &GrantAnyScope, request()) + .await + .unwrap(); + store + .entries + .write() + .unwrap() + .get_mut(&abandoned) + .unwrap() + .expires_at = OffsetDateTime::now_utc() - time::Duration::seconds(1); + + let (fresh, _) = push(&fixture, &store, &GrantAnyScope, request()) + .await + .unwrap(); + assert_eq!(store.len(), 1); + assert!(store.take(&fresh).is_some()); + assert_eq!(store.len(), 0); + } + #[tokio::test] async fn a_valid_admitted_push_mints_a_one_shot_request_uri_and_a_decision() { let fixture = Fixture::new(); diff --git a/crates/didbot-serve/src/oauth/token.rs b/crates/didbot-serve/src/oauth/token.rs index 9dbba5ea..591c1e73 100644 --- a/crates/didbot-serve/src/oauth/token.rs +++ b/crates/didbot-serve/src/oauth/token.rs @@ -139,6 +139,12 @@ impl MemoryAuthorizationCodeStore { pub fn new() -> Self { Self::default() } + + /// How many codes this store still holds. Test use only. + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.entries.read().unwrap_or_else(|p| p.into_inner()).len() + } } fn random_code() -> String { @@ -154,21 +160,23 @@ fn random_code() -> String { impl AuthorizationCodeStore for MemoryAuthorizationCodeStore { fn issue(&self, record: AuthorizationCodeRecord) -> String { let code = random_code(); - self.entries - .write() - .unwrap_or_else(|p| p.into_inner()) - .insert( - code.clone(), - AuthorizationCode { - did: record.did, - client_id: record.client_id, - scope: record.scope, - redirect_uri: record.redirect_uri, - code_challenge: record.code_challenge, - code_challenge_method: record.code_challenge_method, - expires_at: OffsetDateTime::now_utc() + AUTHORIZATION_CODE_TTL, - }, - ); + let now = OffsetDateTime::now_utc(); + let mut entries = self.entries.write().unwrap_or_else(|p| p.into_inner()); + // `redeem` removes only the code it is handed, so one a client never + // exchanges is swept here once it has expired. + entries.retain(|_, entry| now < entry.expires_at); + entries.insert( + code.clone(), + AuthorizationCode { + did: record.did, + client_id: record.client_id, + scope: record.scope, + redirect_uri: record.redirect_uri, + code_challenge: record.code_challenge, + code_challenge_method: record.code_challenge_method, + expires_at: now + AUTHORIZATION_CODE_TTL, + }, + ); code } @@ -750,6 +758,27 @@ mod tests { ); } + /// A code a client never exchanges is swept by the next issue once it + /// has expired. + #[test] + fn an_unredeemed_code_is_swept_once_it_has_expired() { + let (_, challenge) = code_verifier_and_challenge(); + let codes = MemoryAuthorizationCodeStore::new(); + let abandoned = issue_code(&codes, &challenge); + codes + .entries + .write() + .unwrap() + .get_mut(&abandoned) + .unwrap() + .expires_at = OffsetDateTime::now_utc() - time::Duration::seconds(1); + + let fresh = issue_code(&codes, &challenge); + assert_eq!(codes.len(), 1); + assert!(codes.redeem(&fresh).is_some()); + assert_eq!(codes.len(), 0); + } + #[test] fn a_code_is_one_shot() { let (verifier, challenge) = code_verifier_and_challenge();