From 83b9f4134ffd0dabab431f36b4a4e0ce12c37d3c Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Mon, 21 Mar 2022 21:35:52 +0000 Subject: [PATCH 1/6] stuff --- syncstorage/src/db/transaction.rs | 6 ++-- syncstorage/src/web/middleware/mod.rs | 41 +++++++-------------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/syncstorage/src/db/transaction.rs b/syncstorage/src/db/transaction.rs index 9763d0e6..d2c6f1d5 100644 --- a/syncstorage/src/db/transaction.rs +++ b/syncstorage/src/db/transaction.rs @@ -1,4 +1,5 @@ use std::cell::RefMut; +use std::convert::TryFrom; use std::future::Future; use actix_http::http::{HeaderValue, Method, StatusCode}; @@ -16,9 +17,8 @@ use crate::error::{ApiError, ApiErrorKind}; use crate::server::metrics::Metrics; use crate::server::ServerState; use crate::web::extractors::{ - BsoParam, CollectionParam, PreConditionHeader, PreConditionHeaderOpt, + BsoParam, CollectionParam, HawkIdentifier, PreConditionHeader, PreConditionHeaderOpt, }; -use crate::web::middleware::SyncServerRequest; use crate::web::tags::Tags; use crate::web::X_LAST_MODIFIED; @@ -241,7 +241,7 @@ impl FromRequest for DbTransactionPool { } }; let method = req.method().clone(); - let user_id = match req.get_hawk_id() { + let user_id = match HawkIdentifier::try_from(&req) { Ok(v) => v, Err(e) => { warn!("⚠️ Bad Hawk Id: {:?}", e; "user_agent"=> useragent); diff --git a/syncstorage/src/web/middleware/mod.rs b/syncstorage/src/web/middleware/mod.rs index 4f4b1274..4ff32932 100644 --- a/syncstorage/src/web/middleware/mod.rs +++ b/syncstorage/src/web/middleware/mod.rs @@ -10,7 +10,8 @@ use std::{future::Future, sync::Arc}; use actix_web::{ dev::{Service, ServiceRequest, ServiceResponse}, - Error, HttpRequest, + web::Data, + HttpRequest, }; use syncstorage_db_common::util::SyncTimestamp; @@ -19,48 +20,28 @@ use crate::server::{metrics::Metrics, ServerState}; use crate::settings::Secrets; use crate::tokenserver::auth::TokenserverOrigin; use crate::web::{extractors::HawkIdentifier, tags::Tags, DOCKER_FLOW_ENDPOINTS}; -use actix_web::web::Data; +use std::convert::TryFrom; /// The resource in question's Timestamp pub struct ResourceTimestamp(SyncTimestamp); -pub trait SyncServerRequest { - fn get_hawk_id(&self) -> Result; -} +impl TryFrom<&HttpRequest> for HawkIdentifier { + type Error = actix_web::Error; -impl SyncServerRequest for ServiceRequest { - fn get_hawk_id(&self) -> Result { - if DOCKER_FLOW_ENDPOINTS.contains(&self.uri().path().to_lowercase().as_str()) { + fn try_from(req: &HttpRequest) -> Result { + if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { return Ok(HawkIdentifier::cmd_dummy()); } - let method = self.method().clone(); + let method = req.method().clone(); // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so // it must be cloned - let ci = &self.connection_info().clone(); - let secrets = &self + let ci = req.connection_info().clone(); + let secrets = req .app_data::>>() .ok_or_else(|| -> ApiError { ApiErrorKind::Internal("No app_data Secrets".to_owned()).into() })?; - HawkIdentifier::extrude(self, method.as_str(), self.uri(), ci, secrets) - } -} - -impl SyncServerRequest for HttpRequest { - fn get_hawk_id(&self) -> Result { - if DOCKER_FLOW_ENDPOINTS.contains(&self.uri().path().to_lowercase().as_str()) { - return Ok(HawkIdentifier::cmd_dummy()); - } - let method = self.method().clone(); - // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so - // it must be cloned - let ci = &self.connection_info().clone(); - let secrets = &self - .app_data::>>() - .ok_or_else(|| -> ApiError { - ApiErrorKind::Internal("No app_data Secrets".to_owned()).into() - })?; - HawkIdentifier::extrude(self, method.as_str(), self.uri(), ci, secrets) + HawkIdentifier::extrude(req, method.as_str(), req.uri(), &ci, secrets) } } From e90cde11d02877f14905fddf1998cc1848e9813f Mon Sep 17 00:00:00 2001 From: Taddes Korris Date: Thu, 28 Apr 2022 13:41:55 -0400 Subject: [PATCH 2/6] moved TryFrom implementation on HawkIdentifier to extractors.rs and removed use statements in middleware --- syncstorage/src/web/extractors.rs | 26 ++++++++++++++++++++-- syncstorage/src/web/middleware/mod.rs | 31 ++------------------------- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/syncstorage/src/web/extractors.rs b/syncstorage/src/web/extractors.rs index 9d00378f..21af5f23 100644 --- a/syncstorage/src/web/extractors.rs +++ b/syncstorage/src/web/extractors.rs @@ -3,7 +3,8 @@ //! 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::{ - self, collections::HashMap, collections::HashSet, num::ParseIntError, str::FromStr, sync::Arc, + self, collections::HashMap, collections::HashSet, convert::TryFrom, num::ParseIntError, + str::FromStr, sync::Arc, }; use actix_web::{ @@ -42,7 +43,7 @@ use crate::tokenserver::auth::TokenserverOrigin; use crate::web::{ auth::HawkPayload, error::{HawkErrorKind, ValidationErrorKind}, - X_WEAVE_RECORDS, + DOCKER_FLOW_ENDPOINTS, X_WEAVE_RECORDS, }; const BATCH_MAX_IDS: usize = 100; @@ -1134,6 +1135,27 @@ impl From for UserIdentifier { } } +impl TryFrom<&HttpRequest> for HawkIdentifier { + type Error = actix_web::Error; + // type Error = ActixError; + + fn try_from(req: &HttpRequest) -> Result { + if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { + return Ok(HawkIdentifier::cmd_dummy()); + } + let method = req.method().clone(); + // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so + // it must be cloned + let ci = req.connection_info().clone(); + let secrets = req + .app_data::>>() + .ok_or_else(|| -> ApiError { + ApiErrorKind::Internal("No app_data Secrets".to_owned()).into() + })?; + HawkIdentifier::extrude(req, method.as_str(), req.uri(), &ci, secrets) + } +} + impl FromRequest for HawkIdentifier { type Config = (); type Error = Error; diff --git a/syncstorage/src/web/middleware/mod.rs b/syncstorage/src/web/middleware/mod.rs index 4ff32932..f300c935 100644 --- a/syncstorage/src/web/middleware/mod.rs +++ b/syncstorage/src/web/middleware/mod.rs @@ -6,44 +6,17 @@ pub mod weave; // // Matches the [Sync Storage middleware](https://github.com/mozilla-services/server-syncstorage/blob/master/syncstorage/tweens.py) (tweens). -use std::{future::Future, sync::Arc}; +use std::future::Future; use actix_web::{ dev::{Service, ServiceRequest, ServiceResponse}, web::Data, - HttpRequest, }; -use syncstorage_db_common::util::SyncTimestamp; use crate::error::{ApiError, ApiErrorKind}; use crate::server::{metrics::Metrics, ServerState}; -use crate::settings::Secrets; use crate::tokenserver::auth::TokenserverOrigin; -use crate::web::{extractors::HawkIdentifier, tags::Tags, DOCKER_FLOW_ENDPOINTS}; -use std::convert::TryFrom; - -/// The resource in question's Timestamp -pub struct ResourceTimestamp(SyncTimestamp); - -impl TryFrom<&HttpRequest> for HawkIdentifier { - type Error = actix_web::Error; - - fn try_from(req: &HttpRequest) -> Result { - if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { - return Ok(HawkIdentifier::cmd_dummy()); - } - let method = req.method().clone(); - // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so - // it must be cloned - let ci = req.connection_info().clone(); - let secrets = req - .app_data::>>() - .ok_or_else(|| -> ApiError { - ApiErrorKind::Internal("No app_data Secrets".to_owned()).into() - })?; - HawkIdentifier::extrude(req, method.as_str(), req.uri(), &ci, secrets) - } -} +use crate::web::tags::Tags; pub fn emit_http_status_with_tokenserver_origin( req: ServiceRequest, From dd90d309b2c62ce846e8e930817f751409b8f07e Mon Sep 17 00:00:00 2001 From: Taddes Korris Date: Thu, 28 Apr 2022 19:16:09 -0400 Subject: [PATCH 3/6] format check --- syncstorage/src/web/extractors.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/syncstorage/src/web/extractors.rs b/syncstorage/src/web/extractors.rs index 21af5f23..625b2367 100644 --- a/syncstorage/src/web/extractors.rs +++ b/syncstorage/src/web/extractors.rs @@ -1136,8 +1136,7 @@ impl From for UserIdentifier { } impl TryFrom<&HttpRequest> for HawkIdentifier { - type Error = actix_web::Error; - // type Error = ActixError; + type Error = Error; fn try_from(req: &HttpRequest) -> Result { if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { From e1783f03ecb5b3b2c3f5ce7465db9ba1e4a892fe Mon Sep 17 00:00:00 2001 From: Taddes Korris Date: Fri, 29 Apr 2022 17:35:11 -0400 Subject: [PATCH 4/6] map_err refactor --- syncstorage/src/db/transaction.rs | 11 ++++------- syncstorage/src/web/extractors.rs | 4 +--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/syncstorage/src/db/transaction.rs b/syncstorage/src/db/transaction.rs index d2c6f1d5..501525c0 100644 --- a/syncstorage/src/db/transaction.rs +++ b/syncstorage/src/db/transaction.rs @@ -241,13 +241,10 @@ impl FromRequest for DbTransactionPool { } }; let method = req.method().clone(); - let user_id = match HawkIdentifier::try_from(&req) { - Ok(v) => v, - Err(e) => { - warn!("⚠️ Bad Hawk Id: {:?}", e; "user_agent"=> useragent); - return Err(e); - } - }; + let user_id = HawkIdentifier::try_from(&req).map_err(|e| { + warn!("⚠️ Bad Hawk Id: {:?}", e; "user_agent"=> useragent); + e + })?; let bso = BsoParam::extrude(req.head(), &mut req.extensions_mut()).ok(); let bso_opt = bso.map(|b| b.bso); diff --git a/syncstorage/src/web/extractors.rs b/syncstorage/src/web/extractors.rs index 625b2367..11c3f710 100644 --- a/syncstorage/src/web/extractors.rs +++ b/syncstorage/src/web/extractors.rs @@ -1143,9 +1143,7 @@ impl TryFrom<&HttpRequest> for HawkIdentifier { return Ok(HawkIdentifier::cmd_dummy()); } let method = req.method().clone(); - // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so - // it must be cloned - let ci = req.connection_info().clone(); + let ci = req.connection_info(); let secrets = req .app_data::>>() .ok_or_else(|| -> ApiError { From 822bb6b6c6ce60f10449ab3068e70fd3491f47b0 Mon Sep 17 00:00:00 2001 From: Taddes Korris Date: Fri, 6 May 2022 12:37:52 -0400 Subject: [PATCH 5/6] WIP incorporated TryFrom into From request --- syncstorage/src/web/extractors.rs | 68 +++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/syncstorage/src/web/extractors.rs b/syncstorage/src/web/extractors.rs index 11c3f710..5a8d09ed 100644 --- a/syncstorage/src/web/extractors.rs +++ b/syncstorage/src/web/extractors.rs @@ -1139,6 +1139,7 @@ impl TryFrom<&HttpRequest> for HawkIdentifier { type Error = Error; fn try_from(req: &HttpRequest) -> Result { + // Dummy token if a Docker Flow request is detected. if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { return Ok(HawkIdentifier::cmd_dummy()); } @@ -1156,32 +1157,57 @@ impl TryFrom<&HttpRequest> for HawkIdentifier { impl FromRequest for HawkIdentifier { type Config = (); type Error = Error; - type Future = LocalBoxFuture<'static, Result>; + type Future = Ready>; /// Use HawkPayload extraction and format as HawkIdentifier. fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { + // Dummy token if a Docker Flow request is detected. + if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { + return future::ready(Ok(HawkIdentifier::cmd_dummy())); + } let req = req.clone(); + let uri = req.uri(); + // NOTE: `connection_info()` will get a mutable reference lock on `extensions()` + let connection_info = req.connection_info().clone(); + let method = req.method().clone(); + // Tried collapsing this to a `.or_else` and hit problems with the retun resolving + // to an appropriate error state. + let secrets = match req.app_data::>>() { + Some(v) => v, + None => { + let err: ApiError = ApiErrorKind::Internal("No app_data Secrets".to_owned()).into(); + return future::ready(Err(err.into())); + } + }; - Box::pin(async move { - let secrets = match req.app_data::>>() { - Some(s) => s, - None => { - error!("⚠️ Could not load the app secrets"); - return Err(ValidationErrorKind::FromDetails( - "Internal error".to_owned(), - RequestErrorLocation::Unknown, - Some("secrets".to_owned()), - None, - ) - .into()); - } - }; - // NOTE: `connection_info()` will get a mutable reference lock on `extensions()` - let connection_info = req.connection_info().clone(); - let method = req.method().as_str(); - let uri = req.uri(); - Self::extrude(&req, method, uri, &connection_info, secrets) - }) + future::ready(Self::extrude( + &req, + method.as_str(), + uri, + &connection_info, + secrets, + )) + + // Box::pin(async move { + // let secrets = match req.app_data::>>() { + // Some(s) => s, + // None => { + // error!("⚠️ Could not load the app secrets"); + // return Err(ValidationErrorKind::FromDetails( + // "Internal error".to_owned(), + // RequestErrorLocation::Unknown, + // Some("secrets".to_owned()), + // None, + // ) + // .into()); + // } + // }; + // NOTE: `connection_info()` will get a mutable reference lock on `extensions()` + // let connection_info = req.connection_info().clone(); + // let method = req.method().as_str(); + // let uri = req.uri(); + // future::ready(Self::extrude(&req, method, uri, &connection_info, secrets)) + // }) } } From a21073d0f3ae7417fb66d40c9a4a6e088f67d48f Mon Sep 17 00:00:00 2001 From: Taddes Korris Date: Fri, 6 May 2022 17:14:49 -0400 Subject: [PATCH 6/6] refactor to remove comments and old methods, unused imports, post Ethan review --- syncstorage/src/db/transaction.rs | 3 +- syncstorage/src/web/extractors.rs | 47 ++----------------------------- 2 files changed, 4 insertions(+), 46 deletions(-) diff --git a/syncstorage/src/db/transaction.rs b/syncstorage/src/db/transaction.rs index 501525c0..de43c0f6 100644 --- a/syncstorage/src/db/transaction.rs +++ b/syncstorage/src/db/transaction.rs @@ -1,5 +1,4 @@ use std::cell::RefMut; -use std::convert::TryFrom; use std::future::Future; use actix_http::http::{HeaderValue, Method, StatusCode}; @@ -241,7 +240,7 @@ impl FromRequest for DbTransactionPool { } }; let method = req.method().clone(); - let user_id = HawkIdentifier::try_from(&req).map_err(|e| { + let user_id = HawkIdentifier::extract(&req).await.map_err(|e| { warn!("⚠️ Bad Hawk Id: {:?}", e; "user_agent"=> useragent); e })?; diff --git a/syncstorage/src/web/extractors.rs b/syncstorage/src/web/extractors.rs index 5a8d09ed..41725a43 100644 --- a/syncstorage/src/web/extractors.rs +++ b/syncstorage/src/web/extractors.rs @@ -3,8 +3,7 @@ //! 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::{ - self, collections::HashMap, collections::HashSet, convert::TryFrom, num::ParseIntError, - str::FromStr, sync::Arc, + self, collections::HashMap, collections::HashSet, num::ParseIntError, str::FromStr, sync::Arc, }; use actix_web::{ @@ -1135,25 +1134,6 @@ impl From for UserIdentifier { } } -impl TryFrom<&HttpRequest> for HawkIdentifier { - type Error = Error; - - fn try_from(req: &HttpRequest) -> Result { - // Dummy token if a Docker Flow request is detected. - if DOCKER_FLOW_ENDPOINTS.contains(&req.uri().path().to_lowercase().as_str()) { - return Ok(HawkIdentifier::cmd_dummy()); - } - let method = req.method().clone(); - let ci = req.connection_info(); - let secrets = req - .app_data::>>() - .ok_or_else(|| -> ApiError { - ApiErrorKind::Internal("No app_data Secrets".to_owned()).into() - })?; - HawkIdentifier::extrude(req, method.as_str(), req.uri(), &ci, secrets) - } -} - impl FromRequest for HawkIdentifier { type Config = (); type Error = Error; @@ -1170,8 +1150,8 @@ impl FromRequest for HawkIdentifier { // NOTE: `connection_info()` will get a mutable reference lock on `extensions()` let connection_info = req.connection_info().clone(); let method = req.method().clone(); - // Tried collapsing this to a `.or_else` and hit problems with the retun resolving - // to an appropriate error state. + // Tried collapsing this to a `.or_else` and hit problems with the return resolving + // to an appropriate error state. Can't use `?` since the function does not return a result. let secrets = match req.app_data::>>() { Some(v) => v, None => { @@ -1187,27 +1167,6 @@ impl FromRequest for HawkIdentifier { &connection_info, secrets, )) - - // Box::pin(async move { - // let secrets = match req.app_data::>>() { - // Some(s) => s, - // None => { - // error!("⚠️ Could not load the app secrets"); - // return Err(ValidationErrorKind::FromDetails( - // "Internal error".to_owned(), - // RequestErrorLocation::Unknown, - // Some("secrets".to_owned()), - // None, - // ) - // .into()); - // } - // }; - // NOTE: `connection_info()` will get a mutable reference lock on `extensions()` - // let connection_info = req.connection_info().clone(); - // let method = req.method().as_str(); - // let uri = req.uri(); - // future::ready(Self::extrude(&req, method, uri, &connection_info, secrets)) - // }) } }