diff --git a/src/adm/filter.rs b/src/adm/filter.rs index f5713266..e9734b53 100644 --- a/src/adm/filter.rs +++ b/src/adm/filter.rs @@ -7,7 +7,10 @@ use std::{ use lazy_static::lazy_static; use url::Url; -use super::{tiles::AdmTile, AdmAdvertiserFilterSettings, Tile, DEFAULT}; +use super::{ + tiles::{AdmTile, Tile}, + AdmAdvertiserFilterSettings, DEFAULT, +}; use crate::{ error::{HandlerError, HandlerErrorKind, HandlerResult}, metrics::Metrics, diff --git a/src/adm/mod.rs b/src/adm/mod.rs index 754ff512..c58a06a8 100644 --- a/src/adm/mod.rs +++ b/src/adm/mod.rs @@ -12,4 +12,4 @@ mod tiles; pub use filter::AdmFilter; pub(crate) use settings::{AdmAdvertiserFilterSettings, AdmSettings, DEFAULT}; -pub use tiles::{get_tiles, Tile}; +pub use tiles::{get_tiles, TileResponse}; diff --git a/src/adm/tiles.rs b/src/adm/tiles.rs index 8d15b901..84f67df5 100644 --- a/src/adm/tiles.rs +++ b/src/adm/tiles.rs @@ -11,7 +11,7 @@ use crate::{ server::{location::LocationResult, ServerState}, settings::Settings, tags::Tags, - web::{FormFactor, OsFamily}, + web::DeviceInfo, }; /// The payload provided by ADM @@ -107,32 +107,25 @@ impl Tile { /// Main handler for the User Agent HTTP request /// -#[allow(clippy::too_many_arguments)] pub async fn get_tiles( - reqwest_client: &reqwest::Client, - adm_endpoint_url: &str, - location: &LocationResult, - os_family: OsFamily, - form_factor: FormFactor, state: &ServerState, + location: &LocationResult, + device_info: DeviceInfo, tags: &mut Tags, metrics: &Metrics, headers: Option<&HeaderMap>, ) -> Result { - // XXX: Assumes adm_endpoint_url includes - // ?partner=&sub1= (probably should - // validate this on startup) let settings = &state.settings; let adm_url = Url::parse_with_params( - adm_endpoint_url, + &state.adm_endpoint_url, &[ ("partner", settings.adm_partner_id.clone().unwrap().as_str()), ("sub1", settings.adm_sub1.clone().unwrap().as_str()), ("country-code", &location.country()), ("region-code", &location.region()), // ("dma-code", location.dma), - ("form-factor", &form_factor.to_string()), - ("os-family", &os_family.to_string()), + ("form-factor", &device_info.form_factor.to_string()), + ("os-family", &device_info.os_family.to_string()), ("sub2", "newtab"), ("v", "1.0"), // XXX: some value for results seems required, it defaults to 0 @@ -157,8 +150,8 @@ pub async fn get_tiles( trace!("Getting fake response: {:?}", &test_response); AdmTileResponse::fake_response(&state.settings, test_response)? } else { - // TODO: Add timeout - reqwest_client + state + .reqwest_client .get(adm_url) .timeout(Duration::from_secs(settings.adm_timeout)) .send() diff --git a/src/server/mod.rs b/src/server/mod.rs index 37f6fc86..f2cc38d6 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -1,5 +1,5 @@ //! Main application server -use std::{collections::HashMap, sync::Arc, time::Duration}; +use std::time::Duration; use actix_cors::Cors; use actix_web::{ @@ -33,7 +33,6 @@ pub struct ServerState { /// Metric reporting pub metrics: Box, pub adm_endpoint_url: String, - pub adm_country_ip_map: Arc>, pub reqwest_client: reqwest::Client, pub tiles_cache: cache::TilesCache, pub mmdb: location::Location, @@ -51,7 +50,6 @@ impl std::fmt::Debug for ServerState { fmt.debug_struct("ServerState") .field("metrics", &self.metrics) .field("adm_endpoint_url", &self.adm_endpoint_url) - .field("adm_country_ip_map", &self.adm_country_ip_map) .field("reqwest_client", &self.reqwest_client) .field("tiles_cache", &self.tiles_cache) .field("mmdb", &mmdb_status.to_owned()) @@ -97,7 +95,6 @@ impl Server { let state = ServerState { metrics: Box::new(metrics.clone()), adm_endpoint_url: settings.adm_endpoint_url.clone(), - adm_country_ip_map: Arc::new(settings.build_adm_country_ip_map()), reqwest_client: reqwest::Client::builder() .user_agent(REQWEST_USER_AGENT) .build()?, diff --git a/src/settings.rs b/src/settings.rs index f232ff53..f6bb4041 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1,7 +1,5 @@ //! Application settings objects and initialization -use std::collections::HashMap; - use actix_web::{dev::ServiceRequest, web::Data, HttpRequest}; use config::{Config, ConfigError, Environment, File}; use serde::Deserialize; @@ -13,20 +11,6 @@ static PREFIX: &str = "contile"; static DEFAULT_PORT: u16 = 8000; -// issue11: AdM doesn't return any tiles for any of the example non-finalized -// fake IPs, so replaced "US": "174.245.240.112" with "130.245.32.23" (from -// their provided stage URL params) that does result in tiles -static DEFAULT_ADM_COUNTRY_IP_MAP: &str = r#" -{ - "US": "130.245.32.23", - "UK": "86.164.248.137", - "DE": "87.182.235.159", - "FR": "31.39.255.255", - "IT": "5.62.79.255", - "JP": "27.98.191.255" -} -"#; - // TODO: Call this `EnvSettings` that serializes into // real `Settings`? // @@ -79,8 +63,6 @@ pub struct Settings { pub adm_sub1: Option, /// adm Endpoint URL pub adm_endpoint_url: String, - /// adm country to default IP map (Hash in JSON format) - pub adm_country_ip_map: String, /// max tiles to accept from ADM (default: 2) pub adm_max_tiles: u8, /// number of tiles to query from ADM (default: 10) @@ -126,7 +108,6 @@ impl Default for Settings { adm_endpoint_url: "".to_owned(), adm_partner_id: None, adm_sub1: None, - adm_country_ip_map: DEFAULT_ADM_COUNTRY_IP_MAP.to_owned(), adm_max_tiles: 2, adm_query_tile_count: 10, adm_timeout: 5, @@ -207,20 +188,6 @@ impl Settings { pub fn banner(&self) -> String { format!("http://{}:{}", self.host, self.port) } - - /// convert the `adm_country_ip_map` setting from a string to a hashmap - pub(crate) fn build_adm_country_ip_map(&self) -> HashMap { - let mut map: HashMap = - serde_json::from_str(&self.adm_country_ip_map).expect("Invalid ADM_COUNTRY_IP_MAP"); - map = map - .into_iter() - .map(|(key, val)| (key.to_uppercase(), val)) - .collect(); - if !map.contains_key("US") { - panic!("Invalid ADM_COUNTRY_IP_MAP"); - } - map - } } impl<'a> From<&'a HttpRequest> for &'a Settings { diff --git a/src/web/extractors.rs b/src/web/extractors.rs index f83d16f6..d20a1356 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -2,35 +2,29 @@ //! //! Handles ensuring the header's, body, and query parameters are correct, extraction to //! relevant types, and failing correctly with the appropriate errors if issues arise. +use std::net::{IpAddr, SocketAddr}; + use actix_web::{ - dev::Payload, http::header, http::header::HeaderValue, web, Error, FromRequest, HttpRequest, + dev::Payload, + http::header, + http::{HeaderName, HeaderValue}, + web, Error, FromRequest, HttpRequest, }; use futures::future::{self, FutureExt, LocalBoxFuture}; use lazy_static::lazy_static; -use serde::Deserialize; -use crate::{error::HandlerErrorKind, metrics::Metrics}; +use crate::{ + metrics::Metrics, + server::{location::LocationResult, ServerState}, + web::user_agent::{get_device_info, DeviceInfo}, +}; lazy_static! { static ref EMPTY_HEADER: HeaderValue = HeaderValue::from_static(""); + static ref X_FORWARDED_FOR: HeaderName = HeaderName::from_static("x-forwarded-for"); } -const VALID_PLACEMENTS: &[&str] = &["urlbar", "newtab", "search"]; - -#[derive(Debug, Deserialize)] -pub struct TilesParams { - country: Option, - placement: Option, -} - -#[derive(Debug)] -pub struct TilesRequest { - pub country: Option, - pub placement: Option, - pub ua: String, -} - -impl FromRequest for TilesRequest { +impl FromRequest for DeviceInfo { type Config = (); type Error = Error; type Future = LocalBoxFuture<'static, Result>; @@ -44,25 +38,57 @@ impl FromRequest for TilesRequest { .unwrap_or(&EMPTY_HEADER) .to_str() .unwrap_or_default(); + Ok(get_device_info(&ua)?) + } + .boxed_local() + } +} - let params = web::Query::::from_request(&req, &mut Payload::None).await?; - let placement = match ¶ms.placement { - Some(v) => { - let placement = v.to_lowercase(); - if !validate_placement(&v) { - Err(HandlerErrorKind::Validation( - "Invalid placement parameter".to_owned(), - ))?; - }; - Some(placement) +impl FromRequest for LocationResult { + type Config = (); + type Error = Error; + type Future = LocalBoxFuture<'static, Result>; + + fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future { + let req = req.clone(); + async move { + let state = req.app_data::>().expect("No State!"); + let settings = &state.settings; + let metrics = Metrics::from(&req); + + let mut addr = None; + if let Some(header) = req.headers().get(&*X_FORWARDED_FOR) { + if let Ok(value) = header.to_str() { + // Expect a typical X-Forwarded-For where the first address is the + // client's, the front ends should ensure this + addr = value + .split(',') + .next() + .map(|addr| addr.trim()) + .and_then(|addr| { + // Fallback to parsing as SocketAddr for when a port + // number's included + addr.parse::() + .or_else(|_| addr.parse::().map(|socket| socket.ip())) + .ok() + }); + } + } + + if let Some(addr) = addr { + if state.mmdb.is_available() { + let result = state + .mmdb + .mmdb_locate(addr, &["en".to_owned()], &metrics) + .await?; + if let Some(location) = result { + return Ok(location); + } } - None => None, - }; - Ok(Self { - country: params.country.clone().map(|v| v.to_uppercase()), - placement, - ua: ua.to_owned(), - }) + } else { + metrics.incr("location.unknown.ip"); + } + Ok(LocationResult::from_header(req.head(), settings, &metrics)) } .boxed_local() } @@ -77,7 +103,3 @@ impl FromRequest for Metrics { future::ok(Metrics::from(req)).boxed_local() } } - -fn validate_placement(placement: &str) -> bool { - VALID_PLACEMENTS.contains(&placement) -} diff --git a/src/web/handlers.rs b/src/web/handlers.rs index fb3a1fc5..9786c1d3 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -1,29 +1,22 @@ //! API Handlers -use std::net::{IpAddr, SocketAddr}; +use actix_web::{web, HttpRequest, HttpResponse}; -use actix_web::{http::HeaderName, web, HttpRequest, HttpResponse}; -use lazy_static::lazy_static; - -use super::user_agent; use crate::{ adm, error::{HandlerError, HandlerErrorKind}, metrics::Metrics, server::{cache, location::LocationResult, ServerState}, tags::Tags, - web::{extractors::TilesRequest, middleware::sentry as l_sentry}, + web::{middleware::sentry as l_sentry, DeviceInfo}, }; -lazy_static! { - static ref X_FORWARDED_FOR: HeaderName = HeaderName::from_static("x-forwarded-for"); -} - /// Handler for `.../v1/tiles` endpoint /// /// Normalizes User Agent info and searches cache for possible tile suggestions. /// On a miss, it will attempt to fetch new tiles from ADM. pub async fn get_tiles( - treq: TilesRequest, + location: LocationResult, + device_info: DeviceInfo, metrics: Metrics, state: web::Data, request: HttpRequest, @@ -32,41 +25,6 @@ pub async fn get_tiles( metrics.incr("tiles.get"); let settings = &state.settings; - let mut addr = None; - if let Some(header) = request.headers().get(&*X_FORWARDED_FOR) { - if let Ok(value) = header.to_str() { - // Expect a typical X-Forwarded-For where the first address is the - // client's, the front ends should ensure this - addr = value - .split(',') - .next() - .map(|addr| addr.trim()) - .and_then(|addr| { - // Fallback to parsing as SocketAddr for when a port - // number's included - addr.parse::() - .or_else(|_| addr.parse::().map(|socket| socket.ip())) - .ok() - }); - } - } - if addr.is_none() { - metrics.incr("location.unknown.ip"); - } - let (os_family, form_factor) = user_agent::get_device_info(&treq.ua)?; - - let header = request.head(); - let location = if state.mmdb.is_available() && addr.is_some() { - let addr = addr.unwrap(); - state - .mmdb - .mmdb_locate(addr, &["en".to_owned()], &metrics) - .await? - .unwrap_or_else(|| LocationResult::from_header(header, settings, &metrics)) - } else { - LocationResult::from_header(header, settings, &metrics) - }; - let mut tags = Tags::default(); { tags.add_extra("country", &location.country()); @@ -78,7 +36,7 @@ pub async fn get_tiles( let audience_key = cache::AudienceKey { country_code: location.country(), region_code: location.region(), - form_factor, + form_factor: device_info.form_factor, }; let mut expired = false; if !settings.test_mode { @@ -95,12 +53,9 @@ pub async fn get_tiles( } let result = adm::get_tiles( - &state.reqwest_client, - &state.adm_endpoint_url, - &location, - os_family, - form_factor, &state, + &location, + device_info, &mut tags, &metrics, // be aggressive about not passing headers unless we absolutely need to diff --git a/src/web/mod.rs b/src/web/mod.rs index d0aefef3..1308cebd 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -8,4 +8,4 @@ mod test; mod user_agent; pub use dockerflow::DOCKER_FLOW_ENDPOINTS; -pub use user_agent::{FormFactor, OsFamily}; +pub use user_agent::{DeviceInfo, FormFactor}; diff --git a/src/web/test.rs b/src/web/test.rs index 78b6b6bd..4baf983c 100644 --- a/src/web/test.rs +++ b/src/web/test.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc}; +use std::collections::HashMap; use actix_cors::Cors; use actix_web::{ @@ -53,7 +53,6 @@ macro_rules! init_app { let state = ServerState { metrics: Box::new(Metrics::sink()), adm_endpoint_url: $settings.adm_endpoint_url.clone(), - adm_country_ip_map: Arc::new($settings.build_adm_country_ip_map()), reqwest_client: reqwest::Client::new(), tiles_cache: cache::TilesCache::new(10), mmdb: Location::from(&$settings), @@ -393,37 +392,6 @@ async fn basic_default() { assert!(!names.contains(&"Dunder Mifflin")); } -#[actix_rt::test] -async fn invalid_placement() { - let adm = init_mock_adm(MOCK_RESPONSE1.to_owned()); - let mut settings = Settings { - adm_endpoint_url: adm.endpoint_url, - ..get_test_settings() - }; - let mut app = init_app!(settings).await; - - let req = test::TestRequest::get() - .uri("/v1/tiles?country=US&placement=bus12") - .header(header::USER_AGENT, UA) - .to_request(); - let resp = test::call_service(&mut app, req).await; - assert_eq!(resp.status(), StatusCode::BAD_REQUEST); - - let content_type = resp.headers().get(header::CONTENT_TYPE); - assert!(content_type.is_some()); - assert_eq!( - content_type - .unwrap() - .to_str() - .expect("Couldn't parse Content-Type"), - "application/json" - ); - - let _result: Value = test::read_body_json(resp).await; - // XXX: fixup error json - //assert_eq!(result["code"], 600); -} - #[actix_rt::test] async fn fallback_country() { let mut adm = init_mock_adm(MOCK_RESPONSE1.to_owned()); diff --git a/src/web/user_agent.rs b/src/web/user_agent.rs index a63af9c9..abaddb7c 100644 --- a/src/web/user_agent.rs +++ b/src/web/user_agent.rs @@ -7,7 +7,6 @@ use woothee::parser::Parser; use crate::error::{HandlerError, HandlerErrorKind, HandlerResult}; /// ADM required browser format form -#[allow(dead_code)] #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] pub enum FormFactor { Desktop, @@ -24,7 +23,6 @@ impl fmt::Display for FormFactor { } /// Simplified Operating System Family -#[allow(dead_code)] #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] pub enum OsFamily { Windows, @@ -46,8 +44,14 @@ impl fmt::Display for OsFamily { } } -/// Convert a UserAgent header into a simplified ([OsFamily], [FormFactor]) -pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { +#[derive(Debug, Eq, PartialEq)] +pub struct DeviceInfo { + pub form_factor: FormFactor, + pub os_family: OsFamily, +} + +/// Parse a User-Agent header into a simplified `DeviceInfo` +pub fn get_device_info(ua: &str) -> HandlerResult { let wresult = Parser::new().parse(ua).unwrap_or_default(); // If it's not firefox, it doesn't belong here... @@ -77,20 +81,26 @@ pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { "smartphone" => FormFactor::Phone, _ => FormFactor::Other, }; - Ok((os_family, form_factor)) + Ok(DeviceInfo { + form_factor, + os_family, + }) } #[cfg(test)] mod tests { use crate::error::HandlerErrorKind; - use super::{get_device_info, FormFactor, OsFamily}; + use super::{get_device_info, DeviceInfo, FormFactor, OsFamily}; macro_rules! assert_get_device_info { ($value:expr, $os_family:expr, $form_factor:expr) => { assert_eq!( get_device_info($value).expect("Error"), - ($os_family, $form_factor) + DeviceInfo { + os_family: $os_family, + form_factor: $form_factor + } ); }; }