linting

github/src/models/pull_request.rs

@@ -1,7 +1,16 @@
 use std::fmt::{Display, Formatter};
+
 use serde::{Deserialize, Serialize};
 
-use crate::models::{common::Url, DateTime, git::GitReference, labels::Label, links::Links, team::SimpleTeam, user::SimpleUser};
+use crate::models::{
+    common::Url,
+    git::GitReference,
+    labels::Label,
+    links::Links,
+    team::SimpleTeam,
+    user::SimpleUser,
+    DateTime,
+};
 
 #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)]
 pub enum State {

github/src/webhooks/mod.rs

@@ -74,38 +74,44 @@ impl GithubEvent {
 
     pub fn summary(&self) -> String {
         match self {
-            Self::CommitComment(c) =>
-                format!("Commit comment from {}: {}", c.info.sender.login, c.comment.body),
-            Self::IssueComment(c) =>
-                format!("Issue comment from {}: {}", c.info.sender.login, c.comment.body.clone().unwrap_or_default()),
-            Self::Issues(iss) =>
-                format!("Issue {}: {}", iss.action.to_string(), iss.issue.title),
-            Self::Label(lab) =>
-                format!("Label {}: {}", lab.action.to_string(), lab.label.name),
-            Self::Ping(p) =>
-                format!("Ping: {}", p.zen),
-            Self::PullRequest(pr) =>
-                format!("Pull request {}: {}", pr.action.to_string(), pr.pull_request.title),
-            Self::PullRequestReview(r) =>
-                format!("Pull request review {}: {}", r.action.to_string(), r.pull_request.title),
-            Self::PullRequestReviewComment(c) =>
-                format!("PR review comment from {}: {}", c.info.sender.login, c.comment.body),
-            Self::Push(p) =>
-                format!("User {} pushed {} commits to {}", p.pusher.name, p.commits.len(), p.info.repository.name),
-            Self::Status(s) =>
-                format!("Status event ({}) on {}", s.state.to_string(), s.info.repository.name),
-            Self::UnknownEvent { event, .. } =>
-                format!("Unknown event: {}", event),
+            Self::CommitComment(c) => format!("Commit comment from {}: {}", c.info.sender.login, c.comment.body),
+            Self::IssueComment(c) => format!(
+                "Issue comment from {}: {}",
+                c.info.sender.login,
+                c.comment.body.clone().unwrap_or_default()
+            ),
+            Self::Issues(iss) => format!("Issue {}: {}", iss.action.to_string(), iss.issue.title),
+            Self::Label(lab) => format!("Label {}: {}", lab.action.to_string(), lab.label.name),
+            Self::Ping(p) => format!("Ping: {}", p.zen),
+            Self::PullRequest(pr) => format!("Pull request {}: {}", pr.action.to_string(), pr.pull_request.title),
+            Self::PullRequestReview(r) => {
+                format!("Pull request review {}: {}", r.action.to_string(), r.pull_request.title)
+            },
+            Self::PullRequestReviewComment(c) => {
+                format!("PR review comment from {}: {}", c.info.sender.login, c.comment.body)
+            },
+            Self::Push(p) => format!(
+                "User {} pushed {} commits to {}",
+                p.pusher.name,
+                p.commits.len(),
+                p.info.repository.name
+            ),
+            Self::Status(s) => format!("Status event ({}) on {}", s.state.to_string(), s.info.repository.name),
+            Self::UnknownEvent { event, .. } => format!("Unknown event: {}", event),
         }
     }
 }
 
 #[cfg(test)]
 mod test {
-    use crate::{models::static_data::events::PUSH_EVENT, webhooks::GithubEvent};
-    use crate::models::{AuthorAssociation, State};
-    use crate::models::static_data::events::{PR_EDITED_EVENT, PR_EVENT, PR_REVIEW_COMMENT, PR_SYNC_EVENT};
-    use crate::webhooks::{PullRequestAction, PullRequestReviewCommentAction};
+    use crate::{
+        models::{
+            static_data::events::{PR_EDITED_EVENT, PR_EVENT, PR_REVIEW_COMMENT, PR_SYNC_EVENT, PUSH_EVENT},
+            AuthorAssociation,
+            State,
+        },
+        webhooks::{GithubEvent, PullRequestAction, PullRequestReviewCommentAction},
+    };
 
     #[test]
     fn push_event() {
@@ -143,7 +149,10 @@ mod test {
                 assert!(matches!(pr.action, PullRequestAction::Opened));
                 assert_eq!(pr.number, 2);
                 assert_eq!(pr.pull_request.id, 1024763655);
-                assert_eq!(pr.pull_request.created_at.clone().unwrap().to_string(), "2022-08-12T09:55:42Z");
+                assert_eq!(
+                    pr.pull_request.created_at.clone().unwrap().to_string(),
+                    "2022-08-12T09:55:42Z"
+                );
                 assert_eq!(pr.info.repository.name, "tari-dan");
                 assert_eq!(pr.info.organization.clone().unwrap().id, 37560539);
                 assert_eq!(pr.info.sender.node_id, "MDQ6VXNlcjQ3OTE5OTAx");
@@ -158,16 +167,25 @@ mod test {
         match event {
             GithubEvent::PullRequest(pr) => {
                 match pr.action {
-                    PullRequestAction::Edited {changes} => {
+                    PullRequestAction::Edited { changes } => {
                         assert!(changes.body.is_some());
-                        assert_eq!(changes.title.clone().unwrap().from, "[wip] feat!: apply hashing api to the mmr");
+                        assert_eq!(
+                            changes.title.clone().unwrap().from,
+                            "[wip] feat!: apply hashing api to the mmr"
+                        );
                     },
                     _ => panic!("PR event action was not 'edited'"),
                 }
                 assert_eq!(pr.number, 4445);
-                assert_eq!(pr.pull_request.html_url.as_ref(), "https://github.com/tari-project/tari/pull/4445");
+                assert_eq!(
+                    pr.pull_request.html_url.as_ref(),
+                    "https://github.com/tari-project/tari/pull/4445"
+                );
                 assert_eq!(pr.info.repository.node_id, "MDEwOlJlcG9zaXRvcnkxMzY0NTkwOTk=");
-                assert_eq!(pr.info.organization.clone().unwrap().url, "https://api.github.com/orgs/tari-project");
+                assert_eq!(
+                    pr.info.organization.clone().unwrap().url,
+                    "https://api.github.com/orgs/tari-project"
+                );
                 assert_eq!(pr.info.sender.user_type, Some("User".to_string()));
             },
             _ => panic!("Not a pull_request event"),
@@ -180,7 +198,7 @@ mod test {
         match event {
             GithubEvent::PullRequest(pr) => {
                 match pr.action {
-                    PullRequestAction::Synchronize {before, after} => {
+                    PullRequestAction::Synchronize { before, after } => {
                         assert_eq!(before, "4427dabd9c269c60ef9ebf8093d83e28d95dac82");
                         assert_eq!(after, "bdbc85470819181bf9d6243683fb7690959d6a65");
                     },

github/src/webhooks/models.rs

@@ -368,22 +368,22 @@ pub enum PullRequestAction {
 impl ToString for PullRequestAction {
     fn to_string(&self) -> String {
         match self {
-            Self::Assigned{..} => "assigned".into(),
+            Self::Assigned { .. } => "assigned".into(),
             Self::AutoMergeDisabled => "auto_merge_disabled".into(),
             Self::AutoMergeEnabled => "auto_merge_enabled".into(),
             Self::Closed => "closed".into(),
             Self::ConvertedToDraft => "converted_to_draft".into(),
-            Self::Edited{..} => "edited".into(),
-            Self::Labeled{..} => "labeled".into(),
+            Self::Edited { .. } => "edited".into(),
+            Self::Labeled { .. } => "labeled".into(),
             Self::Locked => "locked".into(),
             Self::Opened => "opened".into(),
             Self::ReadyForReview => "ready_for_review".into(),
             Self::Reopened => "reopened".into(),
-            Self::ReviewRequestRemoved{..} => "review_request_removed".into(),
-            Self::ReviewRequested{..} => "review_requested".into(),
-            Self::Synchronize{..} => "synchronize".into(),
-            Self::Unassigned{..} => "unassigned".into(),
-            Self::Unlabeled{..} => "unlabeled".into(),
+            Self::ReviewRequestRemoved { .. } => "review_request_removed".into(),
+            Self::ReviewRequested { .. } => "review_requested".into(),
+            Self::Synchronize { .. } => "synchronize".into(),
+            Self::Unassigned { .. } => "unassigned".into(),
+            Self::Unlabeled { .. } => "unlabeled".into(),
             Self::Unlocked => "unlocked".into(),
         }
     }

lints.toml

@@ -1,68 +1,42 @@
+#
+# For all clippy lints please visit: https://rust-lang.github.io/rust-clippy/master/
+#
 deny = [
-    # Prevent spelling mistakes in lints
-    'unknown_lints',
-    # clippy groups:
-    'clippy::correctness',
-    # All clippy allows must have a reason
-    # TODO: enable lint-reasons feature
-    #    'clippy::allow_attributes_without_reason',
-    # Docs
-    #'missing_docs',
-    #    'clippy::missing_errors_doc',
-    #    'clippy::missing_safety_doc',
-    #    'clippy::missing_panics_doc',
-
-    # Common mistakes
     'clippy::await_holding_lock',
-    'unused_variables',
-    'unused_imports',
-    'dead_code',
-    'unused_extern_crates',
-    'unused_must_use',
-    'unreachable_patterns',
+    'clippy::cast_lossless',
     'clippy::cloned_instead_of_copied',
+    'clippy::correctness',
     'clippy::create_dir',
     'clippy::dbg_macro',
     'clippy::else_if_without_else',
+    'clippy::explicit_into_iter_loop',
+    'clippy::explicit_iter_loop',
+    'clippy::if_not_else',
     'clippy::inline_always',
     'clippy::let_underscore_drop',
     'clippy::let_unit_value',
+    'clippy::match_bool',
     'clippy::match_on_vec_items',
     'clippy::match_wild_err_arm',
-    # In crypto code, it is fairly common to have similar names e.g. `owner_pk` and `owner_k`
-    # 'clippy::similar_names',
     'clippy::needless_borrow',
-    # style
-    'clippy::style',
-    'clippy::explicit_into_iter_loop',
-    'clippy::explicit_iter_loop',
-    'clippy::if_not_else',
-    'clippy::match_bool',
-    # Although generally good practice, this is disabled because the code becomes worse
-    # or needs mod-level exclusion in these cases:
-    # tauri commands, blockchain async db needs owned copy, &Arc<T>, Copy types, T: AsRef<..>, T: ToString
-    # 'clippy::needless_pass_by_value',
+    'clippy::needless_question_mark',
+    'clippy::nonminimal_bool',
     'clippy::range_plus_one',
     'clippy::struct_excessive_bools',
+    'clippy::style',
     'clippy::too_many_lines',
     'clippy::trivially_copy_pass_by_ref',
-    # Highlights potential casting mistakes
-    'clippy::cast_lossless',
-#    'clippy::cast_possible_truncation',
-#    'clippy::cast_possible_wrap',
-    # Precision loss is almost always competely fine and is only useful as a sanity check.
-    # https://rust-lang.github.io/rust-clippy/master/index.html#cast_precision_loss
-    # 'clippy::cast_precision_loss',
-#    'clippy::cast_sign_loss'
     'clippy::unnecessary_to_owned',
-    'clippy::nonminimal_bool',
-    'clippy::needless_question_mark',
+    'dead_code',
+    'unknown_lints',
+    'unreachable_patterns',
+    'unused_extern_crates',
+    'unused_imports',
+    'unused_must_use',
+    'unused_variables',
 ]
-
 allow = [
-    # allow Default::default calls
     'clippy::default_trait_access',
-    # Generally when developers fix this, it can lead to leaky abstractions or worse, so
-    # too many arguments is generally the lesser of two evils
-    'clippy::too_many_arguments'
+    'clippy::too_many_arguments',
 ]
+warn = []

rustfmt.toml

@@ -16,8 +16,6 @@ overflow_delimited_expr = true
 reorder_imports = true
 reorder_modules = true
 reorder_impl_items = true
-report_todo = "Never"
-report_fixme = "Never"
 space_after_colon = true
 space_before_colon = false
 struct_lit_single_line = true

server/src/error.rs

@@ -0,0 +1,43 @@
+use std::fmt::{Display, Formatter};
+
+use actix_web::{
+    error::ResponseError,
+    http::{header::ContentType, StatusCode},
+    HttpResponse,
+};
+use thiserror::Error;
+
+#[derive(Debug, Clone, Error)]
+pub enum ServerError {
+    /// Invalid signature
+    InvalidSignature,
+    /// Payload deserialization error
+    CouldNotDeserializePayload,
+    /// Could not read request body: {0}
+    InvalidRequestBody(String),
+    /// Invalid or missing github event header: {0}
+    InvalidEventHeader(String),
+}
+
+impl ResponseError for ServerError {
+    fn error_response(&self) -> HttpResponse {
+        HttpResponse::build(self.status_code())
+            .insert_header(ContentType::html())
+            .body(self.to_string())
+    }
+
+    fn status_code(&self) -> StatusCode {
+        match *self {
+            Self::InvalidSignature => StatusCode::UNAUTHORIZED,
+            Self::CouldNotDeserializePayload => StatusCode::INTERNAL_SERVER_ERROR,
+            Self::InvalidRequestBody(_) => StatusCode::BAD_REQUEST,
+            Self::InvalidEventHeader(_) => StatusCode::BAD_REQUEST,
+        }
+    }
+}
+
+impl Display for ServerError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.to_string().as_str())
+    }
+}

server/src/lib.rs

@@ -1,3 +1,4 @@
 pub mod config;
+pub mod error;
 pub mod routes;
 pub mod server;

server/src/routes.rs

@@ -19,34 +19,34 @@
 //!         tokio::time::sleep(Duration::from_secs(5)).await; // <-- Ok. Worker thread will handle other requests here
 //!         "response"
 //!     }
-use actix_web::{get, post, web, HttpRequest, HttpResponse, Responder};
+use actix_web::{get, http::header::HeaderMap, post, web, HttpRequest, HttpResponse, Responder};
 use gh_pilot::ghp_api::webhooks::GithubEvent;
 use log::*;
 
+use crate::error::ServerError;
+
 #[get("/health")]
 pub async fn health() -> impl Responder {
     HttpResponse::Ok().body("👍")
 }
 
 #[post("/webhook")]
-pub async fn github_webhook(req: HttpRequest, body: web::Bytes) -> HttpResponse {
-    debug!("Headers: {:?}", req.headers());
-    let payload = match std::str::from_utf8(body.as_ref()) {
-        Ok(text) => text,
-        Err(_) => return HttpResponse::BadRequest().into(),
-    };
-    let event_name = match req
-        .headers()
+pub async fn github_webhook(req: HttpRequest, body: web::Bytes) -> Result<HttpResponse, ServerError> {
+    let headers = req.headers();
+    check_valid_signature(headers)?;
+    let payload = std::str::from_utf8(body.as_ref()).map_err(|e| ServerError::InvalidRequestBody(e.to_string()))?;
+    let event_name = headers
         .get("x-github-event")
-        .map(|v| v.to_str())
-        .and_then(Result::ok)
-    {
-        Some(v) => v,
-        None => return HttpResponse::BadRequest().into(),
-    };
+        .ok_or(ServerError::InvalidEventHeader("x-github-event is missing".into()))?
+        .to_str()
+        .map_err(|_| ServerError::InvalidEventHeader("x-github-event is not a valid string".into()))?;
 
     let event = GithubEvent::from_webhook_info(event_name, payload);
     info!("Github event received: {}, {}", event_name, event.summary());
-    // TODO - set secret on webhook and validate signature
-    HttpResponse::Ok().finish()
+    Ok(HttpResponse::Ok().finish())
+}
+
+fn check_valid_signature(_headers: &HeaderMap) -> Result<(), ServerError> {
+    // TODO
+    Ok(())
 }

server/src/server.rs

@@ -11,9 +11,10 @@ pub async fn run_server(config: ServerConfig) -> std::io::Result<()> {
     HttpServer::new(|| {
         App::new()
             .service(health)
-            .service(web::scope("/github")
-                //.guard(guard::Header("Host", "www.github.com"))
-                .service(github_webhook)
+            .service(
+            web::scope("/github")
+                .guard(guard::Header("Host", "www.github.com"))
+                .service(github_webhook),
             )
     })
     .keep_alive(KeepAlive::Timeout(Duration::from_secs(600)))