feat(policy-server): Don't fail-closed & refactor references

src/service/rooms/event_handler/mod.rs

@@ -1,5 +1,4 @@
 mod acl_check;
-mod call_policyserv;
 mod fetch_and_handle_outliers;
 mod fetch_prev;
 mod fetch_state;
@@ -7,6 +6,7 @@ mod handle_incoming_pdu;
 mod handle_outlier_pdu;
 mod handle_prev_pdu;
 mod parse_incoming_pdu;
+mod policy_server;
 mod resolve_state;
 mod state_at_incoming;
 mod upgrade_outlier_pdu;

src/service/rooms/event_handler/policy_server.rs

@@ -15,14 +15,14 @@ use ruma::{
 /// Returns Ok if the policy server allows the event
 #[implement(super::Service)]
 #[tracing::instrument(skip_all, level = "debug")]
-pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result {
+pub async fn ask_policy_server(&self, pdu: &PduEvent, room_id: &RoomId) -> Result<bool> {
 	if *pdu.event_type() == StateEventType::RoomPolicy.into() {
 		debug!(
 			room_id = %room_id,
 			event_type = ?pdu.event_type(),
 			"Skipping spam check for policy server meta-event"
 		);
-		return Ok(());
+		return Ok(true);
 	}
 	let Ok(policyserver) = self
 		.services
@@ -31,19 +31,19 @@ pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result
 		.await
 		.map(|c: RoomPolicyEventContent| c)
 	else {
-		return Ok(());
+		return Ok(true);
 	};
 
 	let via = match policyserver.via {
 		| Some(ref via) => ServerName::parse(via)?,
 		| None => {
 			debug!("No policy server configured for room {room_id}");
-			return Ok(());
+			return Ok(true);
 		},
 	};
 	if via.is_empty() {
 		debug!("Policy server is empty for room {room_id}, skipping spam check");
-		return Ok(());
+		return Ok(true);
 	}
 	if !self.services.state_cache.server_in_room(via, room_id).await {
 		debug!(
@@ -51,7 +51,7 @@ pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result
 			via = %via,
 			"Policy server is not in the room, skipping spam check"
 		);
-		return Ok(());
+		return Ok(true);
 	}
 	let outgoing = self
 		.services
@@ -85,7 +85,7 @@ pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result
 			);
 			// Network or policy server errors are treated as non-fatal: event is allowed by
 			// default.
-			return Ok(());
+			return Err(e);
 		},
 		| Err(_) => {
 			warn!(
@@ -94,7 +94,7 @@ pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result
 				room_id = %room_id,
 				"Policy server request timed out after 10 seconds"
 			);
-			return Ok(());
+			return Err!("Request to policy server timed out");
 		},
 	};
 	if response.recommendation == "spam" {
@@ -107,5 +107,5 @@ pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result
 		return Err!(Request(Forbidden("Event was marked as spam by policy server")));
 	}
 
-	Ok(())
+	Ok(true)
 }

src/service/rooms/event_handler/upgrade_outlier_pdu.rs

@@ -245,46 +245,49 @@ where
 			.await?;
 	}
 
-	// 14-pre. If the event is not a state event, ask the policy server about it
-	if incoming_pdu.state_key.is_none() {
-		debug!(
-			event_id = %incoming_pdu.event_id,"Checking policy server for event");
-		let policy = self.policyserv_check(&incoming_pdu, room_id);
-		if let Err(e) = policy.await {
-			warn!(
-				event_id = %incoming_pdu.event_id,
-				error = ?e,
-				"Policy server check failed for event"
+	if !soft_fail {
+		// Don't call the below checks on events that have already soft-failed, there's
+		// no reason to re-calculate that.
+		// 14-pre. If the event is not a state event, ask the policy server about it
+		if incoming_pdu.state_key.is_none() {
+			debug!(event_id = %incoming_pdu.event_id, "Checking policy server for event");
+			match self.ask_policy_server(&incoming_pdu, room_id).await {
+				| Ok(false) => {
+					warn!(
+						event_id = %incoming_pdu.event_id,
+						"Event has been marked as spam by policy server"
+					);
+					soft_fail = true;
+				},
+				| _ => {
+					debug!(
+						event_id = %incoming_pdu.event_id,
+						"Event has passed policy server check or the policy server was unavailable."
+					);
+				},
+			};
+		}
+
+		// Additionally, if this is a redaction for a soft-failed event, we soft-fail it
+		// also
+		if let Some(redact_id) = incoming_pdu.redacts_id(&room_version_id) {
+			debug!(
+				redact_id = %redact_id,
+				"Checking if redaction is for a soft-failed event"
 			);
-			if !soft_fail {
+			if self
+				.services
+				.pdu_metadata
+				.is_event_soft_failed(&redact_id)
+				.await
+			{
+				warn!(
+					redact_id = %redact_id,
+					"Redaction is for a soft-failed event, soft failing the redaction"
+				);
 				soft_fail = true;
 			}
 		}
-		debug!(
-			event_id = %incoming_pdu.event_id,
-			"Policy server check passed for event"
-		);
-	}
-
-	// Additionally, if this is a redaction for a soft-failed event, we soft-fail it
-	// also
-	if let Some(redact_id) = incoming_pdu.redacts_id(&room_version_id) {
-		debug!(
-			redact_id = %redact_id,
-			"Checking if redaction is for a soft-failed event"
-		);
-		if self
-			.services
-			.pdu_metadata
-			.is_event_soft_failed(&redact_id)
-			.await
-		{
-			warn!(
-				redact_id = %redact_id,
-				"Redaction is for a soft-failed event, soft failing the redaction"
-			);
-			soft_fail = true;
-		}
 	}
 
 	// 14. Check if the event passes auth based on the "current state" of the room,