From f979ffb68bbff234a03878baf04c6f5e99e72466 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Tue, 12 Oct 2021 16:03:56 +0200 Subject: [PATCH] :wastebasket: Tie up various loose ends --- core/src/domain/request/mod.rs | 2 - core/src/domain/request/storage/mod.rs | 5 - core/src/domain/request/storage/read.rs | 42 ------ core/src/domain/request/storage/write.rs | 45 ------ core/src/library/http/responder.rs | 141 +----------------- .../src/module/collector/services/metadata.rs | 1 - core/src/module/gangway/proxy/api.rs | 1 - core/src/module/gangway/proxy/create.rs | 1 - core/src/module/gangway/proxy/mod.rs | 1 - core/src/module/gangway/proxy/storage.rs | 1 - core/src/module/node/mod.rs | 6 +- core/src/module/node/proxy/terminate.rs | 2 - 12 files changed, 2 insertions(+), 246 deletions(-) delete mode 100644 core/src/domain/request/storage/mod.rs delete mode 100644 core/src/domain/request/storage/read.rs delete mode 100644 core/src/domain/request/storage/write.rs diff --git a/core/src/domain/request/mod.rs b/core/src/domain/request/mod.rs index a16a553b..62f7d19d 100644 --- a/core/src/domain/request/mod.rs +++ b/core/src/domain/request/mod.rs @@ -1,7 +1,5 @@ //! Domain specific [`Request`](super::super::library::communication::request::Request) structures mod provisioner; -mod storage; pub use provisioner::*; -pub use storage::*; diff --git a/core/src/domain/request/storage/mod.rs b/core/src/domain/request/storage/mod.rs deleted file mode 100644 index f2537457..00000000 --- a/core/src/domain/request/storage/mod.rs +++ /dev/null @@ -1,5 +0,0 @@ -mod read; -mod write; - -pub use read::{StorageReadRequest, StorageReadResponse}; -pub use write::{StorageWriteRequest, StorageWriteResponse}; diff --git a/core/src/domain/request/storage/read.rs b/core/src/domain/request/storage/read.rs deleted file mode 100644 index 25217a9f..00000000 --- a/core/src/domain/request/storage/read.rs +++ /dev/null @@ -1,42 +0,0 @@ -use crate::library::communication::event::{Notification, QueueDescriptor}; -use crate::library::communication::request::{Request, ResponseLocation}; -use serde::{Deserialize, Serialize}; -use uuid::Uuid; - -const QUEUE_KEY: &str = "storage.read"; -const QUEUE_SIZE: usize = 100; - -/// Request to read an object from persistent storage -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] -pub struct StorageReadRequest { - /// Unique identifier of the session the object is associated with - pub session_id: Uuid, - - /// Object location within the session storage namespace - pub path: String, - - response_location: ResponseLocation, -} - -/// Response to a [`StorageReadRequest`] -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] -pub struct StorageReadResponse { - /// URL from which the object can be fetched using an HTTP GET request - pub location: String, -} - -impl Notification for StorageReadRequest { - fn queue() -> QueueDescriptor { - QueueDescriptor::new(QUEUE_KEY.into(), QUEUE_SIZE) - } -} - -impl Request for StorageReadRequest { - // TODO It might be possible that we have to return an error (e.g. object not found) - type Response = StorageReadResponse; - - fn reply_to(&self) -> ResponseLocation { - // TODO This clone feels unnecessary ... - self.response_location.clone() - } -} diff --git a/core/src/domain/request/storage/write.rs b/core/src/domain/request/storage/write.rs deleted file mode 100644 index e6a0a10c..00000000 --- a/core/src/domain/request/storage/write.rs +++ /dev/null @@ -1,45 +0,0 @@ -use crate::library::communication::event::{Notification, QueueDescriptor}; -use crate::library::communication::request::{Request, ResponseLocation}; -use serde::{Deserialize, Serialize}; -use uuid::Uuid; - -const QUEUE_KEY: &str = "storage.write"; -const QUEUE_SIZE: usize = 10_000; - -/// Request to write an object to persistent storage -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] -pub struct StorageWriteRequest { - /// Unique identifier of the session the object is associated with - pub session_id: Uuid, - - /// Object location within the session storage namespace - pub path: String, - - /// MIME type of the object to be written - pub mime: String, - - response_location: ResponseLocation, -} - -/// Response to a [`StorageWriteRequest`] -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] -pub struct StorageWriteResponse { - /// URL to which the object can be written using an HTTP PUT request - pub location: String, -} - -impl Notification for StorageWriteRequest { - fn queue() -> QueueDescriptor { - QueueDescriptor::new(QUEUE_KEY.into(), QUEUE_SIZE) - } -} - -impl Request for StorageWriteRequest { - // TODO It might be possible that we have to return an error (e.g. permission denied) - type Response = StorageWriteResponse; - - fn reply_to(&self) -> ResponseLocation { - // TODO This clone feels unnecessary ... - self.response_location.clone() - } -} diff --git a/core/src/library/http/responder.rs b/core/src/library/http/responder.rs index 95fb2268..a1189e7f 100644 --- a/core/src/library/http/responder.rs +++ b/core/src/library/http/responder.rs @@ -23,7 +23,7 @@ pub trait Responder { F: FnOnce(Parts, Body, IpAddr) -> Fut + Send; } -/// TODO Apparently this thing has to be document +/// Chains together a number of `Responder` implementations #[macro_export] macro_rules! responder_chain { ($parts:expr, $body:expr, $ip:expr, { $first:ident, $($rest:tt)+ }) => { @@ -46,145 +46,6 @@ macro_rules! responder_chain { }; } -// /// Simplifies chaining of calls to [`Responder::respond`] -// /// -// /// Under the hood, it calls the `.respond` method of each passed responder in order. -// /// If a responder returns [`ResponderResult::Intercepted`], the response is returned -// /// using the `return` statement. Otherwise, the potentially modified request is passed -// /// to the next responder in the chain. -// /// -// /// ```no_run -// ///# use std::net::IpAddr; -// ///# use hyper::{Body, Response, StatusCode}; -// ///# use http::request::Parts; -// ///# use webgrid::{library::http::{responder_chain, ResponderResult, Responder}}; -// ///# use async_trait::async_trait; -// ///# use std::convert::Infallible; -// ///# -// ///# #[derive(Clone)] -// ///# struct StatusCodeResponder(Option); -// ///# -// ///# #[async_trait] -// ///# impl Responder for StatusCodeResponder { -// ///# #[inline] -// ///# async fn respond(&self, parts: Parts, body: Body, client_ip: IpAddr) -> ResponderResult { -// ///# match self.0 { -// ///# Some(status) => { -// ///# let response = Response::builder() -// ///# .status(status) -// ///# .body(Body::empty()) -// ///# .unwrap(); -// ///# -// ///# ResponderResult::Intercepted(Ok(response)) -// ///# } -// ///# None => ResponderResult::Continue(parts, body, client_ip), -// ///# } -// ///# } -// ///# } -// /// async fn respond(input_parts: Parts, input_body: Body, input_client_ip: IpAddr) -> Result, Infallible> { -// /// let a = StatusCodeResponder(None); -// /// let b = StatusCodeResponder(None); -// /// let c = StatusCodeResponder(Some(StatusCode::OK)); -// /// -// /// responder_chain!(input_parts, input_body, input_client_ip, { -// /// a, -// /// b, -// /// c -// /// }); -// /// -// /// Ok(Response::builder() -// /// .status(StatusCode::NOT_FOUND) -// /// .body(Body::empty()) -// /// .unwrap()) -// /// } -// /// ``` -// /// -// /// The above call will result in code similar to the below. -// /// ```no_run -// ///# use std::net::IpAddr; -// ///# use hyper::{Body, Response, StatusCode}; -// ///# use http::request::Parts; -// ///# use webgrid::{library::http::{responder_chain, ResponderResult, Responder}}; -// ///# use async_trait::async_trait; -// ///# use std::convert::Infallible; -// ///# -// ///# #[derive(Clone)] -// ///# struct StatusCodeResponder(Option); -// ///# -// ///# #[async_trait] -// ///# impl Responder for StatusCodeResponder { -// ///# #[inline] -// ///# async fn respond(&self, parts: Parts, body: Body, client_ip: IpAddr) -> ResponderResult { -// ///# match self.0 { -// ///# Some(status) => { -// ///# let response = Response::builder() -// ///# .status(status) -// ///# .body(Body::empty()) -// ///# .unwrap(); -// ///# -// ///# ResponderResult::Intercepted(Ok(response)) -// ///# } -// ///# None => ResponderResult::Continue(parts, body, client_ip), -// ///# } -// ///# } -// ///# } -// /// async fn respond(input_parts: Parts, input_body: Body, input_client_ip: IpAddr) -> Result, Infallible> { -// /// let a = StatusCodeResponder(None); -// /// let b = StatusCodeResponder(None); -// /// let c = StatusCodeResponder(Some(StatusCode::OK)); -// /// -// /// let parts = input_parts; -// /// let body = input_body; -// /// let ip = input_client_ip; -// /// -// /// let (parts, body, ip) = match a.respond(parts, body, ip).await { -// /// ResponderResult::Intercepted(response) => return response, -// /// ResponderResult::Continue(parts, body, ip) => (parts, body, ip), -// /// }; -// /// -// /// let (parts, body, ip) = match b.respond(parts, body, ip).await { -// /// ResponderResult::Intercepted(response) => return response, -// /// ResponderResult::Continue(parts, body, ip) => (parts, body, ip), -// /// }; -// /// -// /// let (parts, body, ip) = match c.respond(parts, body, ip).await { -// /// ResponderResult::Intercepted(response) => return response, -// /// ResponderResult::Continue(parts, body, ip) => (parts, body, ip), -// /// }; -// /// -// /// drop(parts); -// /// drop(body); -// /// drop(ip); -// /// -// /// Ok(Response::builder() -// /// .status(StatusCode::NOT_FOUND) -// /// .body(Body::empty()) -// /// .unwrap()) -// /// } -// /// ``` -// #[macro_export] -// macro_rules! responder_chain { -// ($parts:expr, $body:expr, $ip:expr, { $($responder:ident$(,)? )+ }) => { -// { -// let parts = $parts; -// let body = $body; -// let ip = $ip; - -// $( -// #[allow(unused_variables)] -// let (parts, body, ip) = match $responder.respond(parts, body, ip).await { -// ResponderResult::Intercepted(response) => return response, -// ResponderResult::Continue(parts, body, ip) => (parts, body, ip), -// }; - -// )+ - -// drop(parts); -// drop(body); -// } -// }; -// } - /// Combines a number of responder instances and creates a new hyper service function /// through the use of [`make_service_fn`](hyper::service::make_service_fn). To allow /// for concurrent access to the responders in the chain, they are wrapped in [`Arc`](std::sync::Arc) pointer types. diff --git a/core/src/module/collector/services/metadata.rs b/core/src/module/collector/services/metadata.rs index bacae4b1..d4ab8569 100644 --- a/core/src/module/collector/services/metadata.rs +++ b/core/src/module/collector/services/metadata.rs @@ -36,7 +36,6 @@ impl Consumer for MetadataWatcherService { let mut update = Document::new(); for (key, value) in notification.metadata.iter() { - // TODO Is this susceptible to injection? update.insert(format!("clientMetadata.{}", key), value); } diff --git a/core/src/module/gangway/proxy/api.rs b/core/src/module/gangway/proxy/api.rs index 66dda910..106bc905 100644 --- a/core/src/module/gangway/proxy/api.rs +++ b/core/src/module/gangway/proxy/api.rs @@ -38,7 +38,6 @@ where #[inline] fn new_error_response(&self, message: &str, status: StatusCode) -> Response { - // TODO Wrap the error in a WebDriver protocol compliant JSON error (and stack using the BlackboxError type) let error = format!("unable to forward request to API: {}", message); Response::builder() diff --git a/core/src/module/gangway/proxy/create.rs b/core/src/module/gangway/proxy/create.rs index b60c62a2..7ceac52c 100644 --- a/core/src/module/gangway/proxy/create.rs +++ b/core/src/module/gangway/proxy/create.rs @@ -53,7 +53,6 @@ impl SessionCreationResponder { let response = SessionCreateResponse { value: SessionCreateResponseValue { session_id: notification.id.to_string(), - // TODO Handle this unwrap! capabilities, }, }; diff --git a/core/src/module/gangway/proxy/mod.rs b/core/src/module/gangway/proxy/mod.rs index e13adfbb..f76284b5 100644 --- a/core/src/module/gangway/proxy/mod.rs +++ b/core/src/module/gangway/proxy/mod.rs @@ -46,7 +46,6 @@ impl, S: StorageBackend> ProxyJob #[async_trait] impl Job for ProxyJob where - // TODO Potentially dangerous usage of 'static lifetime S: StorageBackend + Send + Sync + Clone + 'static, D: ServiceDiscoverer + Send + Sync + Clone + 'static, D::I: Send + Sync, diff --git a/core/src/module/gangway/proxy/storage.rs b/core/src/module/gangway/proxy/storage.rs index 85aee909..6e654d66 100644 --- a/core/src/module/gangway/proxy/storage.rs +++ b/core/src/module/gangway/proxy/storage.rs @@ -29,7 +29,6 @@ where #[inline] fn new_error_response(&self, message: &str, status: StatusCode) -> Response { - // TODO Wrap the error in a WebDriver protocol compliant JSON error (and stack using the BlackboxError type) // TODO Add session ID to error message for easier debugging :) let error = format!("unable to serve object: {}", message); diff --git a/core/src/module/node/mod.rs b/core/src/module/node/mod.rs index 299ff21b..1eebb2e2 100644 --- a/core/src/module/node/mod.rs +++ b/core/src/module/node/mod.rs @@ -244,10 +244,6 @@ impl Node { impl Module for Node { async fn pre_startup(&mut self) -> EmptyResult { self.start_driver().await?; - - // TODO Run post-startup bash script - // NOTE: Evaluate if this is really required because it might pose a potential security hazard. - Ok(()) } @@ -256,7 +252,7 @@ impl Module for Node { serde_json::from_str(&self.options.webdriver.capabilities)?; let (heart, stone) = self.build_heart(&capabilities).await; - // TODO Spawn process monitoring for webdriver (todo find a generic solution because it won't be the last one) + // TODO Spawn process monitoring for webdriver (find a generic solution because it won't be the last one) let advertise_job = self.build_advertise_job(); let (metadata_publisher_job, metadata_tx) = self.build_metadata_publisher_job(); diff --git a/core/src/module/node/proxy/terminate.rs b/core/src/module/node/proxy/terminate.rs index bf3034eb..e7d421d2 100644 --- a/core/src/module/node/proxy/terminate.rs +++ b/core/src/module/node/proxy/terminate.rs @@ -87,8 +87,6 @@ impl Responder for TerminationInterceptor { (is_window_delete_request && was_last_window) || is_session_delete_request; if session_closed { - // TODO This is a MAJOR problem as the termination happens even before the request is forwarded to the browser! - // that just ain't gonna work ... on rare occasions (not so rare actually) the whole session tears down before the response is sent. self.heart_stone .lock() .await