From cae40c404e5b4d06747fbdc68e5896a3cff07835 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 4 Apr 2023 09:03:34 -0700 Subject: [PATCH 1/5] feat: switch to latest release a2 library * This switches from our semi-vendored a2, back to the official release. This should also clear the warning about the included `nom` library. *Breaking Change* This removes the unused `.aps` field from the router_data options. This field was only used during testing. That test was also removed. --- Cargo.lock | 227 ++++-------------- autoendpoint/Cargo.toml | 4 +- .../src/extractors/router_data_input.rs | 1 - autoendpoint/src/routers/apns/error.rs | 11 +- autoendpoint/src/routers/apns/router.rs | 130 +++------- 5 files changed, 90 insertions(+), 283 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 95d57cf3b..c631b56e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,20 +4,19 @@ version = 3 [[package]] name = "a2" -version = "0.5.2" -source = "git+https://github.com/mozilla-services/a2.git?branch=autoendpoint#74c8588f2cc7d40e9c239897acb99031ceb29117" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "02a1fce6c426a8e26af6f720d0b7fd771fc178d18fe3bd156429356fbe0d6506" dependencies = [ - "base64 0.12.3", + "base64 0.20.0", "erased-serde", - "futures 0.3.27", "http 0.2.9", - "hyper 0.13.10", + "hyper 0.14.25", "hyper-alpn", - "log", "openssl", "serde", - "serde_derive", "serde_json", + "thiserror", ] [[package]] @@ -41,7 +40,7 @@ dependencies = [ "pin-project-lite 0.2.9", "smallvec 1.10.0", "tokio 1.26.0", - "tokio-util 0.7.7", + "tokio-util", ] [[package]] @@ -58,7 +57,7 @@ dependencies = [ "memchr", "pin-project-lite 0.2.9", "tokio 1.26.0", - "tokio-util 0.7.7", + "tokio-util", ] [[package]] @@ -99,7 +98,7 @@ dependencies = [ "h2 0.3.16", "http 0.2.9", "httparse", - "httpdate 1.0.2", + "httpdate", "itoa 1.0.6", "language-tags", "local-channel", @@ -110,7 +109,7 @@ dependencies = [ "sha1", "smallvec 1.10.0", "tokio 1.26.0", - "tokio-util 0.7.7", + "tokio-util", "tracing", "zstd", ] @@ -136,7 +135,7 @@ dependencies = [ "serde_json", "serde_urlencoded 0.7.1", "slab", - "socket2 0.4.9", + "socket2", "tokio 1.26.0", ] @@ -187,7 +186,7 @@ dependencies = [ "futures-util", "mio 0.8.6", "num_cpus", - "socket2 0.4.9", + "socket2", "tokio 1.26.0", "tracing", ] @@ -240,7 +239,7 @@ dependencies = [ "http 0.2.9", "log", "pin-project-lite 0.2.9", - "tokio-util 0.7.7", + "tokio-util", ] [[package]] @@ -289,7 +288,7 @@ dependencies = [ "serde_json", "serde_urlencoded 0.7.1", "smallvec 1.10.0", - "socket2 0.4.9", + "socket2", "time 0.3.20", "url 2.3.1", ] @@ -621,6 +620,7 @@ dependencies = [ "autopush_common", "backtrace", "base64 0.21.0", + "bytebuffer", "cadence", "config", "docopt", @@ -840,15 +840,15 @@ checksum = "b41b7ea54a0c9d92199de89e20e58d49f02f8e699814ef3fdf266f6f748d15c7" [[package]] name = "base64" -version = "0.12.3" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3441f0f7b02788e948e47f457ca01f1d7e6d92c693bc132c22b087d3141c03ff" +checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" [[package]] name = "base64" -version = "0.13.1" +version = "0.20.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" +checksum = "0ea22880d78093b0cbe17c89f64a7d457941e65759157ec6cb31a31d652b05e5" [[package]] name = "base64" @@ -945,6 +945,15 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e3b5ca7a04898ad4bcd41c90c5285445ff5b791899bb1b0abdd2a2aa791211d7" +[[package]] +name = "bytebuffer" +version = "2.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce2468af960a9069ae7f77b6c16836e0ccc13f0caf8c8856b942924241c72f7b" +dependencies = [ + "byteorder", +] + [[package]] name = "byteorder" version = "1.4.3" @@ -1937,26 +1946,6 @@ dependencies = [ "tokio-io", ] -[[package]] -name = "h2" -version = "0.2.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e4728fd124914ad25e99e3d15a9361a879f6620f63cb56bbb08f95abb97a535" -dependencies = [ - "bytes 0.5.6", - "fnv", - "futures-core", - "futures-sink", - "futures-util", - "http 0.2.9", - "indexmap", - "slab", - "tokio 0.2.25", - "tokio-util 0.3.1", - "tracing", - "tracing-futures", -] - [[package]] name = "h2" version = "0.3.16" @@ -1972,7 +1961,7 @@ dependencies = [ "indexmap", "slab", "tokio 1.26.0", - "tokio-util 0.7.7", + "tokio-util", "tracing", ] @@ -2089,16 +2078,6 @@ dependencies = [ "tokio-buf", ] -[[package]] -name = "http-body" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13d5ff830006f7646652e057693569bfe0d51760c0085a071769d142a205111b" -dependencies = [ - "bytes 0.5.6", - "http 0.2.9", -] - [[package]] name = "http-body" version = "0.4.5" @@ -2116,12 +2095,6 @@ version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d897f394bad6a705d5f4104762e116a75639e470d80901eed05a860a95cb1904" -[[package]] -name = "httpdate" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "494b4d60369511e7dea41cf646832512a94e542f68bb9c49e54518e0f468eb47" - [[package]] name = "httpdate" version = "1.0.2" @@ -2164,30 +2137,6 @@ dependencies = [ "want 0.2.0", ] -[[package]] -name = "hyper" -version = "0.13.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a6f157065790a3ed2f88679250419b5cdd96e714a0d65f7797fd337186e96bb" -dependencies = [ - "bytes 0.5.6", - "futures-channel", - "futures-core", - "futures-util", - "h2 0.2.7", - "http 0.2.9", - "http-body 0.3.1", - "httparse", - "httpdate 0.3.2", - "itoa 0.4.8", - "pin-project", - "socket2 0.3.19", - "tokio 0.2.25", - "tower-service", - "tracing", - "want 0.3.0", -] - [[package]] name = "hyper" version = "0.14.25" @@ -2202,10 +2151,10 @@ dependencies = [ "http 0.2.9", "http-body 0.4.5", "httparse", - "httpdate 1.0.2", + "httpdate", "itoa 1.0.6", "pin-project-lite 0.2.9", - "socket2 0.4.9", + "socket2", "tokio 1.26.0", "tower-service", "tracing", @@ -2214,17 +2163,16 @@ dependencies = [ [[package]] name = "hyper-alpn" -version = "0.2.1" +version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a5a8e8e9f05ea8e7d34fd477eab717cdd66391689ea884cf44c5d172c6aff96c" +checksum = "f485f87658b5ac391dbed0379d50928de16b54bffee0a85234dc6a92fbe534f6" dependencies = [ - "futures 0.3.27", - "hyper 0.13.10", + "hyper 0.14.25", "log", - "rustls 0.16.0", - "tokio 0.2.25", - "tokio-rustls 0.12.3", - "webpki 0.21.4", + "rustls 0.20.8", + "rustls-pemfile", + "tokio 1.26.0", + "tokio-rustls 0.23.4", "webpki-roots", ] @@ -3145,26 +3093,6 @@ dependencies = [ "ordermap", ] -[[package]] -name = "pin-project" -version = "1.0.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ad29a609b6bcd67fee905812e544992d216af9d755757c05ed2d0e15a74c6ecc" -dependencies = [ - "pin-project-internal", -] - -[[package]] -name = "pin-project-internal" -version = "1.0.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "069bdb1e05adc7a8990dce9cc75370895fbe4e3d58b9b73bf1aee56359344a55" -dependencies = [ - "proc-macro2 1.0.53", - "quote 1.0.26", - "syn 1.0.109", -] - [[package]] name = "pin-project-lite" version = "0.1.12" @@ -3877,19 +3805,6 @@ dependencies = [ "windows-sys 0.45.0", ] -[[package]] -name = "rustls" -version = "0.16.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b25a18b1bf7387f0145e7f8324e700805aade3842dd3db2e74e4cdeb4677c09e" -dependencies = [ - "base64 0.10.1", - "log", - "ring", - "sct 0.6.1", - "webpki 0.21.4", -] - [[package]] name = "rustls" version = "0.19.1" @@ -4063,7 +3978,7 @@ version = "0.30.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b5ce6d3512e2617c209ec1e86b0ca2fea06454cd34653c91092bf0f3ec41f8e3" dependencies = [ - "httpdate 1.0.2", + "httpdate", "log", "native-tls", "reqwest 0.11.15", @@ -4486,17 +4401,6 @@ version = "1.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a507befe795404456341dfab10cef66ead4c041f62b8b11bbb92bffe5d0953e0" -[[package]] -name = "socket2" -version = "0.3.19" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "122e570113d28d773067fab24266b66753f6ea915758651696b6e35e49f88d6e" -dependencies = [ - "cfg-if 1.0.0", - "libc", - "winapi 0.3.9", -] - [[package]] name = "socket2" version = "0.4.9" @@ -4757,16 +4661,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6703a273949a90131b290be1fe7b039d0fc884aa1935860dfcbe056f28cd8092" dependencies = [ "bytes 0.5.6", - "fnv", - "futures-core", - "iovec", - "lazy_static", - "libc", - "memchr", - "mio 0.6.23", - "mio-uds", "pin-project-lite 0.1.12", - "slab", "tokio-macros 0.2.6", ] @@ -4785,7 +4680,7 @@ dependencies = [ "parking_lot 0.12.1", "pin-project-lite 0.2.9", "signal-hook-registry", - "socket2 0.4.9", + "socket2", "tokio-macros 1.8.2", "windows-sys 0.45.0", ] @@ -4954,18 +4849,6 @@ dependencies = [ "tokio-sync", ] -[[package]] -name = "tokio-rustls" -version = "0.12.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3068d891551949b37681724d6b73666787cc63fa8e255c812a41d2513aff9775" -dependencies = [ - "futures-core", - "rustls 0.16.0", - "tokio 0.2.25", - "webpki 0.21.4", -] - [[package]] name = "tokio-rustls" version = "0.22.0" @@ -5102,20 +4985,6 @@ dependencies = [ "tokio-reactor", ] -[[package]] -name = "tokio-util" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "be8242891f2b6cbef26a2d7e8605133c2c554cd35b3e4948ea892d6d68436499" -dependencies = [ - "bytes 0.5.6", - "futures-core", - "futures-sink", - "log", - "pin-project-lite 0.1.12", - "tokio 0.2.25", -] - [[package]] name = "tokio-util" version = "0.7.7" @@ -5166,16 +5035,6 @@ dependencies = [ "once_cell", ] -[[package]] -name = "tracing-futures" -version = "0.2.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97d095ae15e245a057c8e8451bab9b3ee1e1f68e9ba2b4fbc18d0ac5237835f2" -dependencies = [ - "pin-project", - "tracing", -] - [[package]] name = "try-lock" version = "0.2.4" @@ -5557,11 +5416,11 @@ dependencies = [ [[package]] name = "webpki-roots" -version = "0.17.0" +version = "0.22.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a262ae37dd9d60f60dd473d1158f9fbebf110ba7b6a5051c8160460f6043718b" +checksum = "b6c71e40d7d2c34a5106301fb632274ca37242cd0c9d3e64dbece371a40a2d87" dependencies = [ - "webpki 0.21.4", + "webpki 0.22.0", ] [[package]] diff --git a/autoendpoint/Cargo.toml b/autoendpoint/Cargo.toml index 0f611f55c..c091f7a36 100644 --- a/autoendpoint/Cargo.toml +++ b/autoendpoint/Cargo.toml @@ -51,7 +51,9 @@ uuid.workspace = true # - https://github.com/pimeys/a2/pull/47 # The `autoendpoint` branch merges these three PRs together. # The version of a2 at the time of the fork is v0.5.3. -a2 = { git = "https://github.com/mozilla-services/a2.git", branch = "autoendpoint" } +#a2 = { git = "https://github.com/mozilla-services/a2.git", branch = "autoendpoint" } +a2 = "0.7" +bytebuffer = "2.1" # several of these libraries are pinned due to https://github.com/mozilla-services/autopush-rs/issues/249 again = { version = "0.1.2", default-features = false, features = ["log", "rand"] } async-trait = "0.1" diff --git a/autoendpoint/src/extractors/router_data_input.rs b/autoendpoint/src/extractors/router_data_input.rs index b9099a496..4239ec644 100644 --- a/autoendpoint/src/extractors/router_data_input.rs +++ b/autoendpoint/src/extractors/router_data_input.rs @@ -20,7 +20,6 @@ pub struct RouterDataInput { #[serde(rename = "channelID")] pub channel_id: Option, pub key: Option, - pub aps: Option, } impl FromRequest for RouterDataInput { diff --git a/autoendpoint/src/routers/apns/error.rs b/autoendpoint/src/routers/apns/error.rs index 61a6586af..8e1731380 100644 --- a/autoendpoint/src/routers/apns/error.rs +++ b/autoendpoint/src/routers/apns/error.rs @@ -21,6 +21,9 @@ pub enum ApnsError { #[error("APNS error, {0}")] ApnsUpstream(#[source] a2::Error), + #[error("APNS config, {0}:{1}")] + Config(String, String), + #[error("No device token found for user")] NoDeviceToken, @@ -49,9 +52,10 @@ impl ApnsError { StatusCode::GONE } - ApnsError::ChannelSettingsDecode(_) | ApnsError::Io(_) | ApnsError::ApnsClient(_) => { - StatusCode::INTERNAL_SERVER_ERROR - } + ApnsError::ChannelSettingsDecode(_) + | ApnsError::Io(_) + | ApnsError::ApnsClient(_) + | ApnsError::Config(..) => StatusCode::INTERNAL_SERVER_ERROR, ApnsError::ApnsUpstream(_) => StatusCode::BAD_GATEWAY, } @@ -70,6 +74,7 @@ impl ApnsError { | ApnsError::ApnsUpstream(_) | ApnsError::InvalidReleaseChannel | ApnsError::InvalidApsData + | ApnsError::Config(..) | ApnsError::SizeLimit(_) => None, } } diff --git a/autoendpoint/src/routers/apns/router.rs b/autoendpoint/src/routers/apns/router.rs index e4bd90ee5..1c79392ee 100644 --- a/autoendpoint/src/routers/apns/router.rs +++ b/autoendpoint/src/routers/apns/router.rs @@ -9,15 +9,17 @@ use crate::routers::common::{ build_message_data, incr_error_metric, incr_success_metrics, message_size_check, }; use crate::routers::{Router, RouterError, RouterResponse}; -use a2::request::notification::LocalizedAlert; -use a2::request::payload::{APSAlert, Payload, APS}; -use a2::{self, Endpoint, NotificationOptions, Priority, Response}; +use a2::request::payload::Payload; +use a2::{ + self, DefaultNotificationBuilder, Endpoint, NotificationBuilder, NotificationOptions, Priority, + Response, +}; use actix_web::http::StatusCode; use async_trait::async_trait; +use bytebuffer::ByteBuffer; use cadence::StatsdClient; use futures::{StreamExt, TryStreamExt}; -use serde::Deserialize; -use serde_json::{Number, Value}; +use serde_json::Value; use std::collections::HashMap; use std::sync::Arc; use url::Url; @@ -86,19 +88,20 @@ impl ApnsRouter { } else { Endpoint::Production }; - let cert = if !settings.cert.starts_with('-') { + let mut cert = ByteBuffer::from_vec(if !settings.cert.starts_with('-') { tokio::fs::read(settings.cert).await? } else { settings.cert.as_bytes().to_vec() - }; - let key = if !settings.key.starts_with('-') { + }); + let key = String::from_utf8(if !settings.key.starts_with('-') { tokio::fs::read(settings.key).await? } else { settings.key.as_bytes().to_vec() - }; + }) + .map_err(|e| ApnsError::Config("Bad Key".to_owned(), e.to_string()))?; let client = ApnsClientData { client: Box::new( - a2::Client::certificate_parts(&cert, &key, endpoint) + a2::Client::certificate(&mut cert, &key, endpoint) .map_err(ApnsError::ApnsClient)?, ), topic: settings @@ -110,18 +113,11 @@ impl ApnsRouter { } /// The default APS data for a notification - fn default_aps<'a>() -> APS<'a> { - APS { - alert: Some(APSAlert::Localized({ - LocalizedAlert { - title_loc_key: Some("SentTab.NoTabArrivingNotification.title"), - loc_key: Some("SentTab.NoTabArrivingNotification.body"), - ..Default::default() - } - })), - mutable_content: Some(1), - ..Default::default() - } + fn default_aps<'a>() -> DefaultNotificationBuilder<'a> { + DefaultNotificationBuilder::new() + .set_title_loc_key("SentTab.NoTabArrivingNotification.title") + .set_loc_key("SentTab.NoTabArrivingNotification.body") + .set_mutable_content() } /// Handle an error by logging, updating metrics, etc @@ -149,8 +145,8 @@ impl ApnsRouter { warn!("APNS error: {:?}", response.error); } } - a2::Error::ConnectionError => { - error!("APNS connection error"); + a2::Error::ConnectionError(e) => { + error!("APNS connection error: {:?}", e); incr_error_metric( &self.metrics, "apns", @@ -176,21 +172,6 @@ impl ApnsRouter { ApiError::from(ApnsError::ApnsUpstream(error)) } - /// Convert all of the floats in a JSON value into integers. DynamoDB - /// returns all numbers as floats, but deserializing to `APS` will fail if - /// it expects an integer and gets a float. - fn convert_value_float_to_int(value: &mut Value) { - if let Some(float) = value.as_f64() { - *value = Value::Number(Number::from(float as i64)); - } - - if let Some(object) = value.as_object_mut() { - object - .values_mut() - .for_each(Self::convert_value_float_to_int); - } - } - /// if we have any clients defined, this connection is "active" pub fn active(&self) -> bool { !self.clients.is_empty() @@ -218,14 +199,6 @@ impl Router for ApnsRouter { serde_json::to_value(app_id).unwrap(), ); - if let Some(aps) = &router_input.aps { - if APS::deserialize(aps).is_err() { - return Err(ApnsError::InvalidApsData.into()); - } - - router_data.insert("aps".to_string(), aps.clone()); - } - Ok(router_data) } @@ -251,15 +224,6 @@ impl Router for ApnsRouter { .get("rel_channel") .and_then(Value::as_str) .ok_or(ApnsError::NoReleaseChannel)?; - let aps_json = router_data.get("aps").cloned().map(|mut value| { - Self::convert_value_float_to_int(&mut value); - value - }); - let aps: APS<'_> = aps_json - .as_ref() - .map(|value| APS::deserialize(value).map_err(|_| ApnsError::InvalidApsData)) - .transpose()? - .unwrap_or_else(Self::default_aps); let mut message_data = build_message_data(notification)?; message_data.insert("ver", notification.message_id.clone()); @@ -268,21 +232,20 @@ impl Router for ApnsRouter { .clients .get(channel) .ok_or(ApnsError::InvalidReleaseChannel)?; - let payload = Payload { - aps, - data: message_data - .into_iter() - .map(|(k, v)| (k, Value::String(v))) - .collect(), - device_token: token, - options: NotificationOptions { + let mut payload = Self::default_aps().build( + token, + NotificationOptions { apns_id: None, apns_priority: Some(Priority::High), apns_topic: Some(topic), apns_collapse_id: None, apns_expiration: Some(notification.timestamp + notification.headers.ttl as u64), }, - }; + ); + payload.data = message_data + .into_iter() + .map(|(k, v)| (k, Value::String(v))) + .collect(); // Check size limit let payload_json = payload @@ -408,10 +371,13 @@ mod tests { /// A notification with no data is packaged correctly and sent to APNS #[tokio::test] async fn successful_routing_no_data() { + use a2::NotificationBuilder; + let client = MockApnsClient::new(|payload| { + let built = ApnsRouter::default_aps().build(DEVICE_TOKEN, Default::default()); assert_eq!( serde_json::to_value(payload.aps).unwrap(), - serde_json::to_value(ApnsRouter::default_aps()).unwrap() + serde_json::to_value(built.aps).unwrap() ); assert_eq!(payload.device_token, DEVICE_TOKEN); assert_eq!(payload.options.apns_topic, Some("test-topic")); @@ -440,11 +406,11 @@ mod tests { /// A notification with data is packaged correctly and sent to APNS #[tokio::test] async fn successful_routing_with_data() { + use a2::NotificationBuilder; + let client = MockApnsClient::new(|payload| { - assert_eq!( - serde_json::to_value(payload.aps).unwrap(), - serde_json::to_value(ApnsRouter::default_aps()).unwrap() - ); + let built = ApnsRouter::default_aps().build(DEVICE_TOKEN, Default::default()); + assert_eq!(serde_json::json!(payload.aps), serde_json::json!(built.aps)); assert_eq!(payload.device_token, DEVICE_TOKEN); assert_eq!(payload.options.apns_topic, Some("test-topic")); assert_eq!( @@ -569,28 +535,4 @@ mod tests { "result = {result:?}" ); } - - /// An error is returned if the user's APS data is invalid - #[tokio::test] - async fn invalid_aps_data() { - let client = MockApnsClient::new(|_| panic!("The notification should not be sent")); - let db = MockDbClient::new().into_boxed_arc(); - let router = make_router(client, db); - let mut router_data = default_router_data(); - router_data.insert( - "aps".to_string(), - serde_json::json!({"mutable-content": "should be a number"}), - ); - let notification = make_notification(router_data, None, RouterType::APNS); - - let result = router.route_notification(¬ification).await; - assert!(result.is_err()); - assert!( - matches!( - result.as_ref().unwrap_err().kind, - ApiErrorKind::Router(RouterError::Apns(ApnsError::InvalidApsData)) - ), - "result = {result:?}" - ); - } } From 5c1a8818809c016873e500818dbde0551d7385e8 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 4 Apr 2023 09:11:38 -0700 Subject: [PATCH 2/5] f cleanup comments --- Cargo.toml | 1 - autoendpoint/Cargo.toml | 8 -------- 2 files changed, 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4442a18a9..9ebbec1ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,6 @@ config = "0.13" docopt = "1.1" env_logger = "0.10" fernet = "0.2.0" -# autoendpoint used futures 0.3 futures = {version="0.3", features=["compat"]} futures-util = {version="0.3", features=["async-await", "compat", "sink", "io"]} futures-locks = "0.7" diff --git a/autoendpoint/Cargo.toml b/autoendpoint/Cargo.toml index c091f7a36..be9bde678 100644 --- a/autoendpoint/Cargo.toml +++ b/autoendpoint/Cargo.toml @@ -45,16 +45,8 @@ tokio.workspace = true url.workspace = true uuid.workspace = true -# Using a fork temporarily until these three PRs are merged: -# - https://github.com/pimeys/a2/pull/49 -# - https://github.com/pimeys/a2/pull/48 -# - https://github.com/pimeys/a2/pull/47 -# The `autoendpoint` branch merges these three PRs together. -# The version of a2 at the time of the fork is v0.5.3. -#a2 = { git = "https://github.com/mozilla-services/a2.git", branch = "autoendpoint" } a2 = "0.7" bytebuffer = "2.1" -# several of these libraries are pinned due to https://github.com/mozilla-services/autopush-rs/issues/249 again = { version = "0.1.2", default-features = false, features = ["log", "rand"] } async-trait = "0.1" autopush_common = { path = "../autopush-common" } From fa85fa282be410cc2151e500268c67628fbe1018 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 4 Apr 2023 10:41:26 -0700 Subject: [PATCH 3/5] f Cargo cleanup * note the reason we can't update tungstenite properly --- Cargo.toml | 6 ++---- autopush/Cargo.toml | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9ebbec1ea..e8c513f5c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,6 @@ actix-http = "3.2" actix-rt = "2.7" actix-test = "0.1" actix-web = "4.2" -#actix-web-actors = "4.1" actix-ws = "0.2" backtrace = "0.3" base64 = "0.21" @@ -60,8 +59,7 @@ reqwest = {version="0.11", features = ["json"] } rusoto_core = { version="0.47", default-features=false, features=["rustls"] } # locked by serde_dynamodb 0.9 rusoto_credential = { version="0.47"} # locked by serde_dynamodb 0.9 rusoto_dynamodb = { version="0.47", default-features=false, features=["rustls"]} # locked by serde_dynamodb 0.9 -# Using debug-logs avoids https://github.com/getsentry/sentry-rust/issues/237 -sentry = { version = "0.30", features = ["debug-logs"] } +sentry = { version = "0.30", features = ["debug-logs"] } # Using debug-logs avoids https://github.com/getsentry/sentry-rust/issues/237 sentry-core = {version = "0.30"} sentry-actix = "0.30" sentry-backtrace = "0.30" @@ -82,7 +80,7 @@ tokio-compat-02 = "0.2" tokio-core = "0.1" tokio-io = "0.1" tokio-openssl = "0.6" -tokio-tungstenite = { version = "0.9.0", default-features = false } # 0.10+ requires tokio 0.3+ +# Use older version of tungstenite to support legacy connection server. tungstenite = { version = "0.9.2", default-features = false } # 0.10+ requires tokio 0.3+ uuid = { version = "1.1", features = ["serde", "v4"] } url = "2.2" diff --git a/autopush/Cargo.toml b/autopush/Cargo.toml index 7f65b7136..b9d773724 100644 --- a/autopush/Cargo.toml +++ b/autopush/Cargo.toml @@ -34,6 +34,7 @@ slog-mozlog-json.workspace = true slog-scope.workspace = true slog-stdlog.workspace = true slog-term.workspace = true +tungstenite.workspace = true uuid.workspace = true autopush_common = { path = "../autopush-common" } @@ -56,7 +57,5 @@ state_machine_future = "0.2.0" tokio-core = "0.1" tokio-io = "0.1.13" tokio-openssl = "0.3.0" # XXX: pin to < 0.4 until hyper 0.13 -# XXX: pin tokio-tungstenite & tungstenite until hyper 0.13 tokio-tungstenite = { version = "0.9.0", default-features = false } # 0.10+ requires tokio 0.3+ -tungstenite = { version = "0.9.2", default-features = false } # 0.10+ requires tokio 0.3+ woothee = "0.13" From 8cba21f43d43e3637fa958d9d4fbe824dbdf60dd Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 6 Apr 2023 14:37:08 -0700 Subject: [PATCH 4/5] f add back the custom `aps` override feature. --- .../src/extractors/router_data_input.rs | 1 + autoendpoint/src/routers/apns/error.rs | 1 + autoendpoint/src/routers/apns/router.rs | 229 +++++++++++++++++- 3 files changed, 230 insertions(+), 1 deletion(-) diff --git a/autoendpoint/src/extractors/router_data_input.rs b/autoendpoint/src/extractors/router_data_input.rs index 4239ec644..348af9856 100644 --- a/autoendpoint/src/extractors/router_data_input.rs +++ b/autoendpoint/src/extractors/router_data_input.rs @@ -20,6 +20,7 @@ pub struct RouterDataInput { #[serde(rename = "channelID")] pub channel_id: Option, pub key: Option, + pub aps: Option, } impl FromRequest for RouterDataInput { diff --git a/autoendpoint/src/routers/apns/error.rs b/autoendpoint/src/routers/apns/error.rs index 8e1731380..1aa526f3f 100644 --- a/autoendpoint/src/routers/apns/error.rs +++ b/autoendpoint/src/routers/apns/error.rs @@ -21,6 +21,7 @@ pub enum ApnsError { #[error("APNS error, {0}")] ApnsUpstream(#[source] a2::Error), + /// Configuration error {Type of error}, {Error string} #[error("APNS config, {0}:{1}")] Config(String, String), diff --git a/autoendpoint/src/routers/apns/router.rs b/autoendpoint/src/routers/apns/router.rs index 1c79392ee..0620a874d 100644 --- a/autoendpoint/src/routers/apns/router.rs +++ b/autoendpoint/src/routers/apns/router.rs @@ -19,6 +19,7 @@ use async_trait::async_trait; use bytebuffer::ByteBuffer; use cadence::StatsdClient; use futures::{StreamExt, TryStreamExt}; +use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashMap; use std::sync::Arc; @@ -52,6 +53,60 @@ impl ApnsClient for a2::Client { } } +/// a2 does not allow for Deserialization of the APS structure. +/// this is copied from that library +#[derive(Deserialize, Serialize, Default, Debug, Clone)] +#[serde(rename_all = "kebab-case")] +#[allow(clippy::upper_case_acronyms)] +pub struct ApsDeser<'a> { + // The notification content. Can be empty for silent notifications. + // Note, we overwrite this value, but it's copied and commented here + // so that future development notes the change. + // #[serde(skip_serializing_if = "Option::is_none")] + //pub alert: Option>, + /// A number shown on top of the app icon. + #[serde(skip_serializing_if = "Option::is_none")] + pub badge: Option, + + /// The name of the sound file to play when user receives the notification. + #[serde(skip_serializing_if = "Option::is_none")] + pub sound: Option<&'a str>, + + /// Set to one for silent notifications. + #[serde(skip_serializing_if = "Option::is_none")] + pub content_available: Option, + + /// When a notification includes the category key, the system displays the + /// actions for that category as buttons in the banner or alert interface. + #[serde(skip_serializing_if = "Option::is_none")] + pub category: Option<&'a str>, + + /// If set to one, the app can change the notification content before + /// displaying it to the user. + #[serde(skip_serializing_if = "Option::is_none")] + pub mutable_content: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + // Converted for Deserialization + // pub url_args: Option<&'a [&'a str]>, + pub url_args: Option>, +} + +#[derive(Default)] +// Replicate a2::request::notification::DefaultAlert +// for lifetime reasons. +pub struct ApsAlertHolder { + title: String, + subtitle: String, + body: String, + title_loc_key: String, + title_loc_args: Vec, + action_loc_key: String, + loc_key: String, + loc_args: Vec, + launch_image: String, +} + impl ApnsRouter { /// Create a new APNS router. APNS clients will be initialized for each /// channel listed in the settings. @@ -172,6 +227,21 @@ impl ApnsRouter { ApiError::from(ApnsError::ApnsUpstream(error)) } + /// Convert all of the floats in a JSON value into integers. DynamoDB + /// returns all numbers as floats, but deserializing to `APS` will fail if + /// it expects an integer and gets a float. + fn convert_value_float_to_int(value: &mut Value) { + if let Some(float) = value.as_f64() { + *value = Value::Number(serde_json::Number::from(float as i64)); + } + + if let Some(object) = value.as_object_mut() { + object + .values_mut() + .for_each(Self::convert_value_float_to_int); + } + } + /// if we have any clients defined, this connection is "active" pub fn active(&self) -> bool { !self.clients.is_empty() @@ -199,6 +269,16 @@ impl Router for ApnsRouter { serde_json::to_value(app_id).unwrap(), ); + if let Some(aps) = &router_input.aps { + if serde_json::from_str::>(aps).is_err() { + return Err(ApnsError::InvalidApsData.into()); + } + router_data.insert( + "aps".to_string(), + serde_json::to_value(aps.clone()).unwrap(), + ); + } + Ok(router_data) } @@ -224,6 +304,13 @@ impl Router for ApnsRouter { .get("rel_channel") .and_then(Value::as_str) .ok_or(ApnsError::NoReleaseChannel)?; + // XXX: We don't really use anything that is a numeric here, aside from + // mutable contant, and even there we should just check for presense. + // Once we're off of DynamoDB, we might want to kill the map. + let aps_json = router_data.get("aps").cloned().map(|mut value| { + Self::convert_value_float_to_int(&mut value); + value + }); let mut message_data = build_message_data(notification)?; message_data.insert("ver", notification.message_id.clone()); @@ -232,7 +319,123 @@ impl Router for ApnsRouter { .clients .get(channel) .ok_or(ApnsError::InvalidReleaseChannel)?; - let mut payload = Self::default_aps().build( + let mut aps = Self::default_aps(); + + // A simple bucket variable so that I don't have to deal with fun lifetime issues. + let mut holder = ApsAlertHolder::default(); + if let Some(replacement) = aps_json { + // a2 does not have a way to bulk replace these values, so do them by hand. + // these could probably be turned into a macro, but hopefully, this is + // more one off and I didn't want to fight with the macro generator. + // This whole thing was included as a byproduct of + // https://bugzilla.mozilla.org/show_bug.cgi?id=1364403 which was put + // in place to help debug the iOS build. It was supposed to be temporary, + // but apparently bit-lock set in and now no one is super sure if it's + // still needed or used. (I want to get rid of this.) + if let Some(v) = replacement.get("title") { + if let Some(v) = v.as_str() { + holder.title = v.to_owned(); + aps = aps.set_title(&holder.title); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("subtitle") { + if let Some(v) = v.as_str() { + holder.subtitle = v.to_owned(); + aps = aps.set_subtitle(&holder.subtitle); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("body") { + if let Some(v) = v.as_str() { + holder.body = v.to_owned(); + aps = aps.set_body(&holder.body); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("title_loc_key") { + if let Some(v) = v.as_str() { + holder.title_loc_key = v.to_owned(); + aps = aps.set_title_loc_key(&holder.title_loc_key); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("title_loc_args") { + if let Some(v) = v.as_array() { + let mut args: Vec = Vec::new(); + for val in v { + if let Some(value) = val.as_str() { + args.push(value.to_owned()) + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + holder.title_loc_args = args; + aps = aps.set_title_loc_args(&holder.title_loc_args); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("action_loc_key") { + if let Some(v) = v.as_str() { + holder.action_loc_key = v.to_owned(); + aps = aps.set_action_loc_key(&holder.action_loc_key); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("loc_key") { + if let Some(v) = v.as_str() { + holder.loc_key = v.to_owned(); + aps = aps.set_loc_key(&holder.loc_key); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("loc_args") { + if let Some(v) = v.as_array() { + let mut args: Vec = Vec::new(); + for val in v { + if let Some(value) = val.as_str() { + args.push(value.to_owned()) + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + holder.loc_args = args; + aps = aps.set_loc_args(&holder.loc_args); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + if let Some(v) = replacement.get("launch_image") { + if let Some(v) = v.as_str() { + holder.launch_image = v.to_owned(); + aps = aps.set_launch_image(&holder.launch_image); + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + // Honestly, we should just check to see if this is present + // we don't really care what the value is since we'll never + // use + if let Some(v) = replacement.get("mutable-content") { + if let Some(v) = v.as_i64() { + if v != 0 { + aps = aps.set_mutable_content(); + } + } else { + return Err(ApnsError::InvalidApsData.into()); + } + } + }; + + // Finalize the APS object. + let mut payload = aps.build( token, NotificationOptions { apns_id: None, @@ -535,4 +738,28 @@ mod tests { "result = {result:?}" ); } + + /// An error is returned if the user's APS data is invalid + #[tokio::test] + async fn invalid_aps_data() { + let client = MockApnsClient::new(|_| panic!("The notification should not be sent")); + let db = MockDbClient::new().into_boxed_arc(); + let router = make_router(client, db); + let mut router_data = default_router_data(); + router_data.insert( + "aps".to_string(), + serde_json::json!({"mutable-content": "should be a number"}), + ); + let notification = make_notification(router_data, None, RouterType::APNS); + + let result = router.route_notification(¬ification).await; + assert!(result.is_err()); + assert!( + matches!( + result.as_ref().unwrap_err().kind, + ApiErrorKind::Router(RouterError::Apns(ApnsError::InvalidApsData)) + ), + "result = {result:?}" + ); + } } From 5027227a2a8f51a8abe88129d043e55879ca2b13 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 26 May 2023 14:45:09 -0700 Subject: [PATCH 5/5] f r's --- Cargo.lock | 2 +- autoendpoint/Cargo.toml | 2 +- autoendpoint/src/routers/apns/router.rs | 241 +++++++++++++----------- 3 files changed, 132 insertions(+), 113 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7feb7962..a8fce6349 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,7 +5,7 @@ version = 3 [[package]] name = "a2" version = "0.8.0" -source = "git+https://github.com/mozilla-services/a2#118a7793bb3edfa7f0b362bbe8d7114a161e2a12" +source = "git+https://github.com/mozilla-services/a2?branch=master#118a7793bb3edfa7f0b362bbe8d7114a161e2a12" dependencies = [ "base64 0.20.0", "erased-serde", diff --git a/autoendpoint/Cargo.toml b/autoendpoint/Cargo.toml index b70328bd8..2d7380984 100644 --- a/autoendpoint/Cargo.toml +++ b/autoendpoint/Cargo.toml @@ -45,7 +45,7 @@ tokio.workspace = true url.workspace = true uuid.workspace = true -a2 = {version = "0.8", git = "https://github.com/mozilla-services/a2"} +a2 = {version = "0.8", git = "https://github.com/mozilla-services/a2", branch="master"} bytebuffer = "2.1" again = { version = "0.1.2", default-features = false, features = ["log", "rand"] } async-trait = "0.1" diff --git a/autoendpoint/src/routers/apns/router.rs b/autoendpoint/src/routers/apns/router.rs index 5da46acda..ce7560ec6 100644 --- a/autoendpoint/src/routers/apns/router.rs +++ b/autoendpoint/src/routers/apns/router.rs @@ -244,6 +244,127 @@ impl ApnsRouter { pub fn active(&self) -> bool { !self.clients.is_empty() } + + /// Derive an APS message from the replacement JSON block. + /// + /// This requires an external "holder" that contains the data that APS will refer to. + /// The holder should live in the same context as the `aps.build()` method. + fn derive_aps<'a>( + &self, + replacement: Value, + holder: &'a mut ApsAlertHolder, + ) -> Result, ApnsError> { + let mut aps = Self::default_aps(); + // a2 does not have a way to bulk replace these values, so do them by hand. + // these could probably be turned into a macro, but hopefully, this is + // more one off and I didn't want to fight with the macro generator. + // This whole thing was included as a byproduct of + // https://bugzilla.mozilla.org/show_bug.cgi?id=1364403 which was put + // in place to help debug the iOS build. It was supposed to be temporary, + // but apparently bit-lock set in and now no one is super sure if it's + // still needed or used. (I want to get rid of this.) + if let Some(v) = replacement.get("title") { + if let Some(v) = v.as_str() { + holder.title = v.to_owned(); + aps = aps.set_title(&holder.title); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("subtitle") { + if let Some(v) = v.as_str() { + holder.subtitle = v.to_owned(); + aps = aps.set_subtitle(&holder.subtitle); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("body") { + if let Some(v) = v.as_str() { + holder.body = v.to_owned(); + aps = aps.set_body(&holder.body); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("title_loc_key") { + if let Some(v) = v.as_str() { + holder.title_loc_key = v.to_owned(); + aps = aps.set_title_loc_key(&holder.title_loc_key); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("title_loc_args") { + if let Some(v) = v.as_array() { + let mut args: Vec = Vec::new(); + for val in v { + if let Some(value) = val.as_str() { + args.push(value.to_owned()) + } else { + return Err(ApnsError::InvalidApsData); + } + } + holder.title_loc_args = args; + aps = aps.set_title_loc_args(&holder.title_loc_args); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("action_loc_key") { + if let Some(v) = v.as_str() { + holder.action_loc_key = v.to_owned(); + aps = aps.set_action_loc_key(&holder.action_loc_key); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("loc_key") { + if let Some(v) = v.as_str() { + holder.loc_key = v.to_owned(); + aps = aps.set_loc_key(&holder.loc_key); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("loc_args") { + if let Some(v) = v.as_array() { + let mut args: Vec = Vec::new(); + for val in v { + if let Some(value) = val.as_str() { + args.push(value.to_owned()) + } else { + return Err(ApnsError::InvalidApsData); + } + } + holder.loc_args = args; + aps = aps.set_loc_args(&holder.loc_args); + } else { + return Err(ApnsError::InvalidApsData); + } + } + if let Some(v) = replacement.get("launch_image") { + if let Some(v) = v.as_str() { + holder.launch_image = v.to_owned(); + aps = aps.set_launch_image(&holder.launch_image); + } else { + return Err(ApnsError::InvalidApsData); + } + } + // Honestly, we should just check to see if this is present + // we don't really care what the value is since we'll never + // use + if let Some(v) = replacement.get("mutable-content") { + if let Some(v) = v.as_i64() { + if v != 0 { + aps = aps.set_mutable_content(); + } + } else { + return Err(ApnsError::InvalidApsData); + } + } + Ok(aps) + } } #[async_trait(?Send)] @@ -317,119 +438,17 @@ impl Router for ApnsRouter { .clients .get(channel) .ok_or(ApnsError::InvalidReleaseChannel)?; - let mut aps = Self::default_aps(); - // A simple bucket variable so that I don't have to deal with fun lifetime issues. + // A simple bucket variable so that I don't have to deal with fun lifetime issues if we need + // to derive. let mut holder = ApsAlertHolder::default(); - if let Some(replacement) = aps_json { - // a2 does not have a way to bulk replace these values, so do them by hand. - // these could probably be turned into a macro, but hopefully, this is - // more one off and I didn't want to fight with the macro generator. - // This whole thing was included as a byproduct of - // https://bugzilla.mozilla.org/show_bug.cgi?id=1364403 which was put - // in place to help debug the iOS build. It was supposed to be temporary, - // but apparently bit-lock set in and now no one is super sure if it's - // still needed or used. (I want to get rid of this.) - if let Some(v) = replacement.get("title") { - if let Some(v) = v.as_str() { - holder.title = v.to_owned(); - aps = aps.set_title(&holder.title); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("subtitle") { - if let Some(v) = v.as_str() { - holder.subtitle = v.to_owned(); - aps = aps.set_subtitle(&holder.subtitle); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("body") { - if let Some(v) = v.as_str() { - holder.body = v.to_owned(); - aps = aps.set_body(&holder.body); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("title_loc_key") { - if let Some(v) = v.as_str() { - holder.title_loc_key = v.to_owned(); - aps = aps.set_title_loc_key(&holder.title_loc_key); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("title_loc_args") { - if let Some(v) = v.as_array() { - let mut args: Vec = Vec::new(); - for val in v { - if let Some(value) = val.as_str() { - args.push(value.to_owned()) - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - holder.title_loc_args = args; - aps = aps.set_title_loc_args(&holder.title_loc_args); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("action_loc_key") { - if let Some(v) = v.as_str() { - holder.action_loc_key = v.to_owned(); - aps = aps.set_action_loc_key(&holder.action_loc_key); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("loc_key") { - if let Some(v) = v.as_str() { - holder.loc_key = v.to_owned(); - aps = aps.set_loc_key(&holder.loc_key); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("loc_args") { - if let Some(v) = v.as_array() { - let mut args: Vec = Vec::new(); - for val in v { - if let Some(value) = val.as_str() { - args.push(value.to_owned()) - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - holder.loc_args = args; - aps = aps.set_loc_args(&holder.loc_args); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - if let Some(v) = replacement.get("launch_image") { - if let Some(v) = v.as_str() { - holder.launch_image = v.to_owned(); - aps = aps.set_launch_image(&holder.launch_image); - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } - // Honestly, we should just check to see if this is present - // we don't really care what the value is since we'll never - // use - if let Some(v) = replacement.get("mutable-content") { - if let Some(v) = v.as_i64() { - if v != 0 { - aps = aps.set_mutable_content(); - } - } else { - return Err(ApnsError::InvalidApsData.into()); - } - } + + // If we are provided a replacement APS block, derive an APS message from it, otherwise + // start with a blank APS message. + let aps = if let Some(replacement) = aps_json { + self.derive_aps(replacement, &mut holder)? + } else { + Self::default_aps() }; // Finalize the APS object.