feat: Normalize and explain TTLs maximums (#742)

This normalizes to a max channel TTL of 30 days and a max router TTL of 2 * max channel TTL. See doc comment in autopush-common for details. This also adds two convenience functions to convert SystemTime and UTC millisecond offsets into formatted DateTime strings. These were useful to identify the bug and may also be useful for various logging statements. There was also a bug in one of the unit tests, where we were taking the cell timestamp for `connected_at` instead of the cell value. How this did not fail before is a mystery. --------- Co-authored-by: Philip Jenvey <[email protected]>
mozilla-services · Oct 2, 2024 · b8e524f · b8e524f
1 parent 3cc5a56
commit b8e524f
Show file tree

Hide file tree

Showing 9 changed files with 94 additions and 35 deletions.
diff --git a/autoendpoint/src/extractors/notification_headers.rs b/autoendpoint/src/extractors/notification_headers.rs
@@ -2,7 +2,7 @@ use crate::error::{ApiError, ApiErrorKind, ApiResult};
 use crate::headers::crypto_key::CryptoKeyHeader;
 use crate::headers::util::{get_header, get_owned_header};
 use actix_web::HttpRequest;
-use autopush_common::util::InsertOpt;
+use autopush_common::{util::InsertOpt, MAX_NOTIFICATION_TTL};
 use lazy_static::lazy_static;
 use regex::Regex;
 use std::cmp::min;
@@ -16,9 +16,6 @@ lazy_static! {
         Regex::new(r"(?P<head>[0-9A-Za-z\-_]+)=+(?P<tail>[,;]|$)").unwrap();
 }
 
-/// 60 days
-const MAX_TTL: i64 = 60 * 60 * 24 * 60;
-
 /// Extractor and validator for notification headers
 #[derive(Clone, Debug, Eq, PartialEq, Validate)]
 pub struct NotificationHeaders {
@@ -71,7 +68,9 @@ impl NotificationHeaders {
         let ttl = get_header(req, "ttl")
             .and_then(|ttl| ttl.parse().ok())
             // Enforce a maximum TTL, but don't error
-            .map(|ttl| min(ttl, MAX_TTL))
+            // NOTE: In order to trap for negative TTLs, this should be a
+            // signed value, otherwise we will error out with NO_TTL.
+            .map(|ttl| min(ttl, MAX_NOTIFICATION_TTL as i64))
             .ok_or(ApiErrorKind::NoTTL)?;
         let topic = get_owned_header(req, "topic");
 
@@ -216,9 +215,9 @@ impl NotificationHeaders {
 #[cfg(test)]
 mod tests {
     use super::NotificationHeaders;
-    use super::MAX_TTL;
     use crate::error::{ApiErrorKind, ApiResult};
     use actix_web::test::TestRequest;
+    use autopush_common::MAX_NOTIFICATION_TTL;
 
     /// Assert that a result is a validation error and check its serialization
     /// against the JSON value.
@@ -265,7 +264,6 @@ mod tests {
             .insert_header(("TTL", "-1"))
             .to_http_request();
         let result = NotificationHeaders::from_request(&req, false);
-
         assert_validation_error(
             result,
             serde_json::json!({
@@ -285,12 +283,12 @@ mod tests {
     #[test]
     fn maximum_ttl() {
         let req = TestRequest::post()
-            .insert_header(("TTL", (MAX_TTL + 1).to_string()))
+            .insert_header(("TTL", (MAX_NOTIFICATION_TTL + 1).to_string()))
             .to_http_request();
         let result = NotificationHeaders::from_request(&req, false);
 
         assert!(result.is_ok());
-        assert_eq!(result.unwrap().ttl, MAX_TTL);
+        assert_eq!(result.unwrap().ttl, MAX_NOTIFICATION_TTL as i64);
     }
 
     /// A valid topic results in no errors

diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs
@@ -85,7 +85,7 @@ impl FcmClient {
         &self,
         data: HashMap<&'static str, String>,
         routing_token: String,
-        ttl: usize,
+        ttl: u64,
     ) -> Result<(), RouterError> {
         // Check the payload size. FCM only cares about the `data` field when
         // checking size.

diff --git a/autoendpoint/src/routers/fcm/router.rs b/autoendpoint/src/routers/fcm/router.rs
@@ -1,4 +1,4 @@
-use autopush_common::db::client::DbClient;
+use autopush_common::{db::client::DbClient, MAX_NOTIFICATION_TTL};
 
 use crate::error::ApiResult;
 use crate::extractors::notification::Notification;
@@ -16,9 +16,6 @@ use std::sync::Arc;
 use url::Url;
 use uuid::Uuid;
 
-/// 28 days
-const MAX_TTL: usize = 28 * 24 * 60 * 60;
-
 /// Firebase Cloud Messaging router
 pub struct FcmRouter {
     settings: FcmSettings,
@@ -156,7 +153,8 @@ impl Router for FcmRouter {
 
         let (routing_token, app_id) =
             self.routing_info(router_data, &notification.subscription.user.uaid)?;
-        let ttl = MAX_TTL.min(self.settings.min_ttl.max(notification.headers.ttl as usize));
+        let ttl =
+            MAX_NOTIFICATION_TTL.min(self.settings.min_ttl.max(notification.headers.ttl as u64));
 
         // Send the notification to FCM
         let client = self

diff --git a/autoendpoint/src/routers/fcm/settings.rs b/autoendpoint/src/routers/fcm/settings.rs
@@ -8,7 +8,7 @@ use url::Url;
 #[serde(deny_unknown_fields)]
 pub struct FcmSettings {
     /// The minimum TTL to use for FCM notifications
-    pub min_ttl: usize,
+    pub min_ttl: u64,
     /// A JSON dict of `FcmCredential`s. This must be a `String` because
     /// environment variables cannot encode a `HashMap<String, FcmCredential>`
     /// This contains both GCM and FCM credentials.

diff --git a/autopush-common/Cargo.toml b/autopush-common/Cargo.toml
@@ -63,12 +63,13 @@ form_urlencoded = { version = "1.2", optional = true }
 [dev-dependencies]
 mockito = "0.31"
 tempfile = "3.2.0"
-tokio = { workspace=true, features = ["macros"] }
+tokio = { workspace = true, features = ["macros"] }
 actix-rt = "2.8"
 
 [features]
 # NOTE: Do not set a `default` here, rather specify them in the calling library.
 # This is to reduce complexity around feature specification.
+
 bigtable = [
     "dep:google-cloud-rust-raw",
     "dep:grpcio",

diff --git a/autopush-common/src/db/bigtable/bigtable_client/mod.rs b/autopush-common/src/db/bigtable/bigtable_client/mod.rs
@@ -24,8 +24,7 @@ use uuid::Uuid;
 use crate::db::{
     client::{DbClient, FetchMessageResponse},
     error::{DbError, DbResult},
-    DbSettings, Notification, NotificationRecord, User, MAX_CHANNEL_TTL, MAX_ROUTER_TTL,
-    USER_RECORD_VERSION,
+    DbSettings, Notification, NotificationRecord, User, MAX_ROUTER_TTL, USER_RECORD_VERSION,
 };
 
 pub use self::metadata::MetadataBuilder;
@@ -900,6 +899,18 @@ impl DbClient for BigTableClientImpl {
     /// BigTable doesn't really have the concept of an "update". You simply write the data and
     /// the individual cells create a new version. Depending on the garbage collection rules for
     /// the family, these can either persist or be automatically deleted.
+    ///
+    /// NOTE: This function updates the key ROUTER records for a given UAID. It does this by
+    /// calling [BigTableClientImpl::user_to_row] which creates a new row with new `cell.timestamp` values set
+    /// to now + `MAX_ROUTER_TTL`. This function is called by mobile during the daily
+    /// [autoendpoint::routes::update_token_route] handling, and by desktop
+    /// [autoconnect-ws-sm::get_or_create_user]` which is called
+    /// during the `HELLO` handler. This should be enough to ensure that the ROUTER records
+    /// are properly refreshed for "lively" clients.
+    ///
+    /// NOTE: There is some, very small, potential risk that a desktop client that can
+    /// somehow remain connected the duration of MAX_ROUTER_TTL, may be dropped as not being
+    /// "lively".
     async fn update_user(&self, user: &mut User) -> DbResult<bool> {
         let Some(ref version) = user.version else {
             return Err(DbError::General(
@@ -1023,7 +1034,7 @@ impl DbClient for BigTableClientImpl {
         // easy/efficient
         let row_key = uaid.simple().to_string();
         let mut row = Row::new(row_key);
-        let expiry = std::time::SystemTime::now() + Duration::from_secs(MAX_CHANNEL_TTL);
+        let expiry = std::time::SystemTime::now() + Duration::from_secs(MAX_ROUTER_TTL);
 
         // Note: updating the version column isn't necessary here because this
         // write only adds a new (or updates an existing) column with a 0 byte
@@ -1215,8 +1226,9 @@ impl DbClient for BigTableClientImpl {
             &row_key,
             timestamp.to_be_bytes().to_vec()
         );
-        let mut row = Row::new(row_key);
         let expiry = std::time::SystemTime::now() + Duration::from_secs(MAX_ROUTER_TTL);
+        let mut row = Row::new(row_key.clone());
+
         row.cells.insert(
             ROUTER_FAMILY.to_owned(),
             vec![
@@ -1229,7 +1241,9 @@ impl DbClient for BigTableClientImpl {
                 new_version_cell(expiry),
             ],
         );
+
         self.write_row(row).await?;
+
         Ok(())
     }
 
@@ -1831,20 +1845,24 @@ mod tests {
             panic!("Expected row");
         };
 
-        // Ensure the initial expiry (timestamp) of all the cells in the row
-        let expiry = row.take_required_cell("connected_at").unwrap().timestamp;
+        // Ensure the initial cell expiry (timestamp) of all the cells
+        // in the row has been updated
+        let ca_expiry = row.take_required_cell("connected_at").unwrap().timestamp;
         for mut cells in row.cells.into_values() {
             let Some(cell) = cells.pop() else {
                 continue;
             };
             assert!(
-                cell.timestamp >= expiry,
+                cell.timestamp >= ca_expiry,
                 "{} cell timestamp should >= connected_at's",
                 cell.qualifier
             );
         }
 
         let mut user = client.get_user(&uaid).await.unwrap().unwrap();
+
+        // Quick nap to make sure that the ca_expiry values are different.
+        tokio::time::sleep(Duration::from_secs_f32(0.2)).await;
         client.update_user(&mut user).await.unwrap();
 
         // Ensure update_user updated the expiry (timestamp) of every cell in the row
@@ -1853,16 +1871,17 @@ mod tests {
             panic!("Expected row");
         };
 
-        let expiry2 = row.take_required_cell("connected_at").unwrap().timestamp;
-        assert!(expiry2 > expiry);
+        let ca_expiry2 = row.take_required_cell("connected_at").unwrap().timestamp;
+
+        assert!(ca_expiry2 > ca_expiry);
 
         for mut cells in row.cells.into_values() {
             let Some(cell) = cells.pop() else {
                 continue;
             };
-            assert_eq!(
-                cell.timestamp, expiry2,
-                "{} cell timestamp should match connected_at's",
+            assert!(
+                cell.timestamp >= ca_expiry2,
+                "{} cell timestamp expiry should exceed connected_at's",
                 cell.qualifier
             );
         }

diff --git a/autopush-common/src/db/mod.rs b/autopush-common/src/db/mod.rs
@@ -35,14 +35,10 @@ pub use reporter::spawn_pool_periodic_reporter;
 use crate::errors::{ApcErrorKind, Result};
 use crate::notification::{Notification, STANDARD_NOTIFICATION_PREFIX, TOPIC_NOTIFICATION_PREFIX};
 use crate::util::timing::{ms_since_epoch, sec_since_epoch};
+use crate::{MAX_NOTIFICATION_TTL, MAX_ROUTER_TTL};
 use models::{NotificationHeaders, RangeKey};
 
-const MAX_EXPIRY: u64 = 2_592_000;
 pub const USER_RECORD_VERSION: u64 = 1;
-/// The maximum TTL for channels, 30 days
-pub const MAX_CHANNEL_TTL: u64 = 30 * 24 * 60 * 60;
-/// The maximum TTL for router records, 30 days
-pub const MAX_ROUTER_TTL: u64 = MAX_CHANNEL_TTL;
 
 #[derive(Eq, Debug, PartialEq)]
 pub enum StorageType {
@@ -346,7 +342,7 @@ impl NotificationRecord {
             uaid: *uaid,
             chidmessageid: val.chidmessageid(),
             timestamp: Some(val.timestamp),
-            expiry: sec_since_epoch() + min(val.ttl, MAX_EXPIRY),
+            expiry: sec_since_epoch() + min(val.ttl, MAX_NOTIFICATION_TTL),
             ttl: Some(val.ttl),
             data: val.data,
             headers: val.headers.map(|h| h.into()),

diff --git a/autopush-common/src/lib.rs b/autopush-common/src/lib.rs
@@ -19,3 +19,32 @@ pub mod test_support;
 
 #[macro_use]
 pub mod util;
+
+/// Define some global TTLs.
+///
+/// [RFC8030 notes](https://datatracker.ietf.org/doc/html/rfc8030#section-5.2) that
+/// technically these are u32 values, but we should be kind and use u64. The RFC also
+/// does not define a maximum TTL duration. Traditionally, Autopush has capped this
+/// to 60 days, partly because we used to require monthly message table rotation.
+/// (The TTL was given an extra 30 days grace in order to handle dates near the
+/// turn of the month, when we might need to look in two tables for the data.)
+/// Since we now have automatically applied garbage collection, we are at a bit of
+/// liberty about how long these should be.
+///
+/// That gets back to the concept that Push messages are supposed to be "timely".
+/// A user may not appreciate that they have an undelivered calendar reminder from
+/// 58 days ago, nor should they be interested in a meeting alert that happened last
+/// month. When a User Agent (UA) connects, it recieves all pending messages. If
+/// a user has not used the User Agent in more than
+/// [60 days](https://searchfox.org/mozilla-central/search?q=OFFER_PROFILE_RESET_INTERVAL_MS),
+/// the User Agent suggest "refreshing Firefox", which essentially throws away one's
+/// current profile. This would include all subscriptions a user may have had.
+///
+/// To that end, messages left unread for more than 30 days should be considered
+/// "abandoned" and any router info assigned to a User Agent that has not contacted
+/// Autopush in 60 days can be discarded.
+///
+/// The maximum TTL for notifications, 30 days in seconds
+pub const MAX_NOTIFICATION_TTL: u64 = 30 * 24 * 60 * 60;
+/// The maximum TTL for router records, 60 days in seconds
+pub const MAX_ROUTER_TTL: u64 = 2 * MAX_NOTIFICATION_TTL;
diff --git a/autopush-common/src/util/timing.rs b/autopush-common/src/util/timing.rs
@@ -25,3 +25,21 @@ pub fn us_since_epoch() -> u64 {
     let now = Utc::now();
     (now.timestamp() as u64) * 1_000_000 + (now.timestamp_subsec_micros() as u64)
 }
+
+/// Display a formatted date-time string from a SystemTime
+///
+/// (This is useful in dev/debugging)
+#[allow(dead_code)]
+pub fn date_string_from_systemtime(ts: std::time::SystemTime) -> String {
+    let dt: chrono::DateTime<chrono::Utc> = ts.into();
+    dt.format("%Y-%m-%d %H:%M:%S.%f").to_string()
+}
+
+/// Display a formatted date-time string from a UTC offset in millis
+///
+/// (This is useful in dev/debugging)
+#[allow(dead_code)]
+pub fn date_string_from_utc_ms(offset: u64) -> String {
+    let utc = std::time::SystemTime::UNIX_EPOCH + std::time::Duration::from_millis(offset);
+    date_string_from_systemtime(utc)
+}