Add endpoint to review checks

This endpoint allows check scripts to modify their state, for example
by marking the check as needing manual review. It also allows
reviewers to set the check state.

src/api/build.rs

@@ -1,8 +1,8 @@
 use actix::prelude::*;
 use actix_multipart::Multipart;
-use actix_web::http;
 use actix_web::middleware::BodyEncoding;
 use actix_web::web::{Data, Json, Path, Query};
+use actix_web::{http, web};
 use actix_web::{HttpRequest, HttpResponse, ResponseError, Result};
 
 use chrono::Utc;
@@ -18,8 +18,8 @@ use std::sync::Arc;
 use crate::config::Config;
 use crate::db::*;
 use crate::errors::ApiError;
-use crate::jobs::{JobQueue, ProcessJobs};
-use crate::models::{Build, NewBuild, NewBuildRef};
+use crate::jobs::{update_build_status_after_check, JobQueue, ProcessJobs};
+use crate::models::{Build, CheckStatus, NewBuild, NewBuildRef};
 use crate::ostree::init_ostree_repo;
 use crate::tokens::{self, Claims, ClaimsScope, ClaimsValidator};
 
@@ -56,6 +56,44 @@ async fn get_job_async(
     Ok(HttpResponse::Ok().json(job))
 }
 
+#[derive(Debug, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+pub struct ReviewArgs {
+    new_status: CheckStatus,
+}
+
+pub fn review_check(
+    args: Json<ReviewArgs>,
+    params: Path<JobPathParams>,
+    db: Data<Db>,
+    req: HttpRequest,
+) -> impl Future<Item = HttpResponse, Error = ApiError> {
+    Box::pin(review_check_async(args, params, db, req)).compat()
+}
+
+async fn review_check_async(
+    args: Json<ReviewArgs>,
+    params: Path<JobPathParams>,
+    db: Data<Db>,
+    req: HttpRequest,
+) -> Result<HttpResponse, ApiError> {
+    req.has_token_claims("build", ClaimsScope::ReviewCheck)?;
+
+    db.set_check_status(params.id, args.new_status.clone())
+        .await?;
+
+    let check = db.get_check_by_job_id(params.id).await?;
+    web::block(move || {
+        let conn = db.0.get()?;
+        update_build_status_after_check(check.build_id, &conn)
+            .map_err(|err| ApiError::InternalServerError(err.to_string()))
+    })
+    .compat()
+    .await?;
+
+    Ok(HttpResponse::NoContent().finish())
+}
+
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
 pub struct CreateBuildArgs {

src/app.rs

@@ -96,6 +96,11 @@ pub fn create_app(
                             .name("show_job")
                             .route(web::get().to_async(api::build::get_job)),
                     )
+                    .service(
+                        web::resource("/job/{id}/check/review")
+                            .name("review_check")
+                            .route(web::post().to_async(api::build::review_check)),
+                    )
                     .service(
                         web::resource("/build")
                             .route(web::post().to_async(api::build::create_build))

src/db.rs

@@ -257,6 +257,33 @@ impl Db {
         .await
     }
 
+    /* Checks */
+
+    pub async fn get_check_by_job_id(&self, job: i32) -> Result<Check, ApiError> {
+        self.run(move |conn| {
+            use schema::checks::dsl::*;
+            Ok(checks.filter(job_id.eq(job)).get_result::<Check>(conn)?)
+        })
+        .await
+    }
+
+    pub async fn set_check_status(
+        &self,
+        job: i32,
+        new_status: CheckStatus,
+    ) -> Result<(), ApiError> {
+        self.run(move |conn| {
+            use schema::checks::dsl;
+            let (status, status_reason) = new_status.to_db();
+            diesel::update(dsl::checks)
+                .filter(dsl::job_id.eq(job))
+                .set((dsl::status.eq(status), dsl::status_reason.eq(status_reason)))
+                .execute(conn)?;
+            Ok(())
+        })
+        .await
+    }
+
     /* Builds */
 
     pub async fn new_build(&self, a_build: NewBuild) -> Result<Build, ApiError> {

src/jobs/check_job.rs

@@ -79,30 +79,38 @@ impl JobInstance for CheckJobInstance {
         // Run the hook
         job_log_and_info!(self.job_id, conn, "Running check command");
 
-        let output = if let Some(mut cmd) = check_hook.command.build_command(build_repo_path) {
-            do_command_with_output(&mut cmd)?
+        let new_status = if let Some(mut cmd) = check_hook.command.build_command(build_repo_path) {
+            cmd.env("FLAT_MANAGER_BUILD_ID", self.build_id.to_string())
+                .env("FLAT_MANAGER_JOB_ID", self.job_id.to_string());
+
+            let output = do_command_with_output(&mut cmd)?;
+
+            if output.status.success() {
+                job_log_and_info!(self.job_id, conn, "Check command exited successfully");
+                CheckStatus::Passed
+            } else {
+                let msg = format!(
+                    "Check command {:?} exited unsuccessfully: {}, stderr: {}",
+                    cmd,
+                    output.status,
+                    String::from_utf8_lossy(&output.stderr)
+                );
+
+                job_log_and_info!(self.job_id, conn, &msg);
+
+                if check_hook.reviewable {
+                    CheckStatus::ReviewRequired(msg)
+                } else {
+                    CheckStatus::Failed(msg)
+                }
+            }
         } else {
             return Err(JobError::new(&format!(
                 "The '{}' check hook is defined, but it has no command",
                 self.name
             )));
         };
 
-        let new_status = if output.status.success() {
-            job_log_and_info!(self.job_id, conn, "Check command exited successfully");
-            CheckStatus::Passed
-        } else {
-            let msg = format!("Check command exited with status code {}", output.status);
-
-            job_log_and_info!(self.job_id, conn, &msg);
-
-            if check_hook.reviewable {
-                CheckStatus::ReviewRequired(msg)
-            } else {
-                CheckStatus::Failed(msg)
-            }
-        };
-
         conn.transaction::<_, JobError, _>(|| {
             let check = checks::table
                 .filter(checks::job_id.eq(self.job_id))
@@ -111,7 +119,9 @@ impl JobInstance for CheckJobInstance {
 
             // The command can edit its own check status by calling the API. If that happened and the command exited
             // successfully, don't change it.
-            if !output.status.success() || check.status == CheckStatus::Pending.to_db().0 {
+            if matches!(new_status, CheckStatus::Failed(_))
+                || check.status == CheckStatus::Pending.to_db().0
+            {
                 let (status, status_reason) = new_status.to_db();
                 diesel::update(checks::table)
                     .filter(checks::job_id.eq(self.job_id))
@@ -162,9 +172,9 @@ pub fn update_build_status_after_check(build_id: i32, conn: &PgConnection) -> Re
             .for_update()
             .get_result::<Build>(conn)?;
 
-        // Sanity check--make sure the build is still in Verifying state
+        // Sanity check--make sure the build is still in Validating state
         if !RepoState::from_db(build.repo_state, &build.repo_state_reason)
-            .same_state_as(&RepoState::Committing)
+            .same_state_as(&RepoState::Validating)
         {
             return Err(JobError::new(&format!(
                 "Expected repo to be in {:?} state upon check completion, but it was in {:?}",

src/jobs/mod.rs

@@ -10,6 +10,7 @@ mod publish_job;
 mod republish_job;
 mod update_repo_job;
 
+pub use check_job::update_build_status_after_check;
 pub use job_executor::start_job_executor;
 pub use job_queue::{cleanup_started_jobs, JobQueue, ProcessJobs, StopJobQueue};
 

src/models.rs

@@ -261,7 +261,8 @@ pub struct JobDependencyWithStatus {
     pub dependant_status: i16,
 }
 
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "status", content = "reason")]
 pub enum CheckStatus {
     Pending,
     Passed,

src/tokens.rs

@@ -30,6 +30,9 @@ pub enum ClaimsScope {
     // Permission to republish an app (take it from the repo, re-run the publish hook, and publish it back). Should not
     // be given to untrusted parties.
     Republish,
+    // Permission to change the status of any build check (e.g. mark it as successful, failed, etc.) Should only be
+    // given to reviewers or passed to the check scripts themselves.
+    ReviewCheck,
 
     #[serde(other)]
     Unknown,