Review

3 files changed, 188 insertions, 202 deletions

diff --git a/INSTALL-GITHUB-DEV b/INSTALL-GITHUB-DEV
index 5fe38e6..45f4b9b 100644
--- a/INSTALL-GITHUB-DEV
+++ b/INSTALL-GITHUB-DEV
@@ -1,36 +1,38 @@
-This document explains how to get Github webhooks (a notification that an
+This document explains how to get GitHub webhooks (a notification that an
 event such as a push has occurred on a repository) delivered to a
-locally-running instance of brep.
+locally-running instance of brep (currently to initiate a CI job).
 
-0. Overview of the brep Github CI integration
+0. Overview of the brep GitHub CI integration
 
-First we register our Github CI brep module as a Github app. This Github app
+First we register our GitHub CI brep instance as a GitHub app. This GitHub app
 essentially consists of an app name, the URL we want webhooks to be delivered
 to, permissions required on users' repositories, event subscriptions, and
 various authentication-related settings.
 
-Once registered, Github users can install our Github app on their user or
-organization accounts, optionally restricting its access to specific
+Once registered, GitHub users can install this GitHub app on their user's or
+organization's accounts, optionally restricting its access to specific
 repositories.
 
-Once installed on a repository, Github will send webhook requests to our app's
+Once installed on a repository, GitHub will send webhook requests to our app's
 webhook URL when, for example, code is pushed or a pull request is created
 (the specifics depending on the events our app is subscribed to).
 
 For development we need these webhooks delivered to our locally-running brep
-instance. This is achieved by setting the Github app's webhook URL to that of
-the webhook proxy smee.io and connecting it to our local brep instance via a
-the smee client (also run locally).
+instance. This is achieved by setting the GitHub app's webhook URL to that of
+the webhook proxy smee.io (as recommended by GitHub) and connecting it to our
+local brep instance via the locally-run smee client (a Node application).
 
 1. Follow the instructions in INSTALL-DEV to get brep set up.
 
-2. Register the Github app
+2. Register the GitHub app
 
-Github doc: Registering a GitHub App (note that that doc is a bit out of date)
+GitHub doc: Registering a GitHub App (note that that doc is a bit out of date)
 https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/registering-a-github-app
 
 Skip the steps marked "optional" and leave authorization-related settings at
-their defaults. @@ TODO Update authentication-related info once better understood.
+their defaults.
+
+@@ TODO Update authentication-related info once better understood.
 
 At this stage the only settings important to us are:
 
@@ -46,22 +48,22 @@ At this stage the only settings important to us are:
   - Check run
   - Pull request
 
-3. Install the Github app
+3. Install the GitHub app
 
-Github doc: Installing your own GitHub App
+GitHub doc: Installing your own GitHub App
 https://docs.github.com/en/apps/using-github-apps/installing-your-own-github-app
 
 It would probably make sense to install it to your own user account and
 restrict its access to a test repository.
 
-4. Forward Github webhooks to a local brep instance
+4. Forward GitHub webhooks to a local brep instance
 
 Go to https://smee.io/ and start a new channel. Note the webhook proxy URL,
 which will look something like
 
   https://smee.io/7stvNqVgyQRlIhbY
 
-Set the Github app's webhook URL to this proxy URL.
+Set the GitHub app's webhook URL to this proxy URL.
 
 Install the smee client:
 
@@ -70,19 +72,19 @@ Install the smee client:
 Start brep.
 
 Start the smee client, passing the webhook proxy URL with --url and the brep
-Github CI endpoint's URL with --target:
+GitHub CI endpoint's URL with --target:
 
   $ smee --url https://smee.io/7stvNqVgyQRlIhbY \
          --target http://127.0.0.1/pkg?ci-github
 
-Trigger a webhook delivery from Github by pushing a commit to a repository our
-Github app is installed in. You should see the webhook delivery on the smee.io
-channel page and the smee client will also print something to stdout.
+Trigger a webhook delivery from GitHub by pushing a commit to a repository the
+GitHub app is installed in. You should see the webhook delivery on the smee.io
+channel page and the smee client will also print something to terminal.
 
 Any webhook delivery can be redelivered by clicking a button on the smee.io
-channel page (or the app's advanced settings page on Github) so no need to
+channel page (or the app's advanced settings page on GitHub) so no need to
 repeatedly push to the repository.
 
 You can also see the HTTP headers and JSON payload of delivered webhooks on
-both the Github app's advanced settings page and the smee.io channel page, but
+both the GitHub app's advanced settings page and the smee.io channel page, but
 smee.io's presentation is much better. (There's also wireshark of course.)
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index aee2235..b7bd15f 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -142,13 +142,43 @@ init (scanner& s)
 bool brep::ci_github::
 respond (response& rs, status_code status, const string& message)
 {
-  ostream& os (rs.content (status, "text/manifest;charset=utf-8"));
+  ostream& os (rs.content (status, "text/plain;charset=utf-8"));
 
   os << message;
 
   return true;
 }
 
+// The "check_suite" object within a check_quite webhook request.
+//
+struct check_suite
+{
+  uint64_t id;
+  string head_branch;
+  string head_sha;
+  string before;
+  string after;
+
+  explicit
+  check_suite (json::parser&);
+
+  check_suite () = default;
+};
+
+struct repository
+{
+  string name;
+  string full_name;
+  string default_branch;
+};
+
+struct check_suite_event
+{
+  string        action;
+  ::check_suite check_suite;
+  ::repository  repository;
+};
+
 bool brep::ci_github::
 handle (request& rq, response& rs)
 {
@@ -160,107 +190,111 @@ handle (request& rq, response& rs)
   if (false)
     return respond (rs, 404, "CI request submission disabled");
 
-  enum { unknown, webhook } rq_type (unknown); // Request type.
-
-  const std::optional<std::string>* ghe (nullptr); // Github event.
-
-  // Determine the message type.
+  // Process headers.
   //
-  for (const name_value& h: rq.headers ())
+  string event;
   {
-    if (icasecmp (h.name, "x-github-delivery") == 0)
-    {
-      // @@ TODO Check that delivery UUID has not been received before (replay
-      //         attack).
-    }
-    else if (icasecmp (h.name, "content-type") == 0)
-    {
-      // @@ TODO Safe to assume an empty content-type would have been rejected
-      //         already?
-      //
-      if (icasecmp (*h.value, "application/json") != 0)
-        return respond (rs, 400, "invalid content-type: " + *h.value);
-    }
-    else if (icasecmp (h.name, "x-github-event") == 0)
+    bool content_type (false);
+
+    for (const name_value& h: rq.headers ())
     {
-      rq_type = webhook;
-      ghe = &h.value;
+      if (icasecmp (h.name, "x-github-delivery") == 0)
+      {
+        // @@ TODO Check that delivery UUID has not been received before
+        //         (replay attack).
+      }
+      else if (icasecmp (h.name, "content-type") == 0)
+      {
+        if (!h.value)
+          return respond (rs, 400, "missing content-type value");
+
+        if (icasecmp (*h.value, "application/json") != 0)
+          return respond (rs, 400,
+                          "invalid content-type value: '" + *h.value + '\'');
+      }
+      else if (icasecmp (h.name, "x-github-event") == 0)
+      {
+        if (!h.value)
+          return respond (rs, 400, "missing x-github-event value");
+
+        event = *h.value;
+      }
     }
-  }
 
-  switch (rq_type)
-  {
-  case webhook:
-    return handle_webhook (rq, *ghe, rs);
-    break;
-  default:
-    return respond (rs, 400, "unknown request type");
-    break;
+    if (!content_type)
+      return respond (rs, 400, "missing content-type header");
+
+    if (event.empty ())
+      return respond (rs, 400, "missing x-github-event header");
   }
-}
 
-// The "check_suite" object within a check_quite webhook request.
-//
-struct check_suite_obj
-{
-  uint64_t id;
-  string head_branch;
-  string head_sha;
-  string before;
-  string after;
-};
+  // There is an event (specified in the x-github-event header) and each event
+  // contains a bunch of actions (specified in the JSON request body).
+  //
+  // Note: "GitHub continues to add new event types and new actions to
+  // existing event types." As a result we ignore known actions that we are
+  // not interested in and log and ignore unknown actions. The thinking here
+  // is that we want be "notified" of new actions at which point we can decide
+  // whether to ignore them or to handle.
+  //
+  try
+  {
+    if (event == "check_suite")
+    {
+      json::parser p (rq.content (64 * 1024), "check_suite webhook");
 
-static ostream&
-operator<< (ostream& os, const check_suite_obj& cs)
-{
-  os << "id: " << cs.id << endl
-     << "head_branch: " << cs.head_branch << endl
-     << "head_sha: " << cs.head_sha << endl
-     << "before: " << cs.before << endl
-     << "after: " << cs.after << endl;
+      p.next_expect (event::begin_object); // @@ Let's move into ctor (everywhere).
+      check_suite_event cs (p);
 
-  return os;
-}
+      // @@ TODO: log and ignore unknown.
+      //
+      if (cs.action == "requested")
+      {
+      }
+      else if (cs.action == "rerequested")
+      {
+        // Someone manually requested to re-run the check runs in this check
+        // suite.
+      }
+      else if (cs.action == "completed")
+      {
+        // GitHub thinks that "all the check runs in this check suite have
+        // completed and a conclusion is available". Looks like this one we
+        // ignore?
+      }
+      else
+        return respond (rs, 400, "unsupported action: " + cs.action);
 
-struct repository_obj
-{
-  string name;
-  string full_name;
-  string default_branch;
-};
+      cout << "<check_suite webhook>" << endl << cs << endl;
 
-static ostream&
-operator<< (ostream& os, const repository_obj& rep)
-{
-  os << "name: " << rep.name << endl
-     << "full_name: " << rep.full_name << endl
-     << "default_branch: " << rep.default_branch << endl;
+      return true;
+    }
+    else if (event == "pull_request")
+    {
+      return respond (rs, 501, "pull request events not implemented yet");
+    }
+    else
+      respond (rs, 400, "unexpected event: '" + event + "'");
+  }
+  catch (const json::invalid_json_input& e)
+  {
+    // @@ TODO: should we write more detailed diagnostics to log? Maybe we
+    //    should do this for all unsuccessful calls to respond().
 
-  return os;
+    return respond (rs, 400, "malformed JSON in request body");
+  }
 }
 
-struct check_suite_req
-{
-  string action;
-  check_suite_obj check_suite;
-  repository_obj repository;
-};
+using event = json::event;
 
-static ostream&
-operator<< (ostream& os, const check_suite_req& cs)
-{
-  os << "action: " << cs.action << endl;
-  os << "<check_suite>" << endl << cs.check_suite;
-  os << "<repository>" << endl << cs.repository << endl;
-
-  return os;
-}
+// check_suite_obj
+//
 
+// @@ Let's make then constructors.
+//
 static check_suite_obj
 parse_check_suite_obj (json::parser& p)
 {
-  using event = json::event;
-
   check_suite_obj r;
 
   // Skip unknown/uninteresting members.
@@ -269,28 +303,34 @@ parse_check_suite_obj (json::parser& p)
   {
     const string& n (p.name ());
 
-    if (n == "id")
-      r.id = p.next_expect_number<uint64_t> ();
-    else if (n == "head_branch")
-      r.head_branch = p.next_expect_string ();
-    else if (n == "head_sha")
-      r.head_sha = p.next_expect_string ();
-    else if (n == "before")
-      r.before = p.next_expect_string ();
-    else if (n == "after")
-      r.after = p.next_expect_string ();
-    else
-      p.next_expect_value_skip ();
+    if      (n == "id")          r.id = p.next_expect_number<uint64_t> ();
+    else if (n == "head_branch") r.head_branch = p.next_expect_string ();
+    else if (n == "head_sha")    r.head_sha = p.next_expect_string ();
+    else if (n == "before")      r.before = p.next_expect_string ();
+    else if (n == "after")       r.after = p.next_expect_string ();
+    else p.next_expect_value_skip ();
   }
 
   return r;
 }
 
+static ostream&
+operator<< (ostream& os, const check_suite_obj& cs)
+{
+  os << "id: " << cs.id << endl
+     << "head_branch: " << cs.head_branch << endl
+     << "head_sha: " << cs.head_sha << endl
+     << "before: " << cs.before << endl
+     << "after: " << cs.after << endl;
+
+  return os;
+}
+
+// repository_obj
+//
 static repository_obj
 parse_repository_obj (json::parser& p)
 {
-  using event = json::event;
-
   repository_obj r;
 
   // Skip unknown/uninteresting members.
@@ -299,26 +339,34 @@ parse_repository_obj (json::parser& p)
   {
     const string& n (p.name ());
 
-    if (n == "name")
-      r.name = p.next_expect_string ();
-    else if (n == "full_name")
-      r.full_name = p.next_expect_string ();
-    else if (n == "default_branch")
-      r.default_branch = p.next_expect_string ();
-    else
-      p.next_expect_value_skip ();
+    if      (n == "name")           r.name = p.next_expect_string ();
+    else if (n == "full_name")      r.full_name = p.next_expect_string ();
+    else if (n == "default_branch") r.default_branch = p.next_expect_string ();
+    else p.next_expect_value_skip ();
   }
 
   return r;
 }
 
+static ostream&
+operator<< (ostream& os, const repository_obj& rep)
+{
+  os << "name: " << rep.name << endl
+     << "full_name: " << rep.full_name << endl
+     << "default_branch: " << rep.default_branch << endl;
+
+  return os;
+}
+
+// check_suite_req
+//
 static check_suite_req
 parse_check_suite_webhook (json::parser& p)
 {
-  using event = json::event;
-
   check_suite_req r;
 
+  // @@ Let's not assume order.
+
   r.action = p.next_expect_member_string ("action");
 
   // Parse the check_suite object.
@@ -336,73 +384,12 @@ parse_check_suite_webhook (json::parser& p)
   return r;
 }
 
-bool
-brep::ci_github::handle_webhook (request& rq,
-                                 const std::optional<std::string>& ghe,
-                                 response& rs)
+static ostream&
+operator<< (ostream& os, const check_suite_req& cs)
 {
-  using event = json::event;
-
-  if (!ghe)
-    return respond (rs, 400, "empty Github event type");
-
-  enum class event_type // Github event type.
-  {
-    check_suite,
-    pull_request
-  };
-
-  optional<event_type> evt;
-
-  if (ghe == "check_suite")
-    evt = event_type::check_suite;
-  else if (ghe == "pull_request")
-    evt = event_type::pull_request;
-
-  if (!evt)
-    return respond (rs, 400, "unsupported event type: " + *ghe);
-
-  switch (*evt)
-  {
-  case event_type::pull_request:
-    return respond (rs, 501, "pull request events not implemented yet");
-    break;
-
-  case event_type::check_suite:
-    // Parse the structured result JSON.
-    //
-    try
-    {
-      json::parser p (rq.content (64 * 1024), "check_suite webhook");
-
-      p.next_expect (event::begin_object);
-      check_suite_req cs (parse_check_suite_webhook (p));
-
-      // Note: "GitHub continues to add new event types and new actions to
-      // existing event types."
-      //
-      if (cs.action == "requested")
-      {
-      }
-      else if (cs.action == "rerequested")
-      {
-      }
-      else if (cs.action == "completed")
-      {
-      }
-      else
-        return respond (rs, 400, "unsupported action: " + cs.action);
-
-      cout << "<check_suite webhook>" << endl << cs << endl;
-
-      return true;
-    }
-    catch (const json::invalid_json_input& e)
-    {
-      return respond (rs, 400, "malformed JSON in request body");
-    }
-    break;
-  }
+  os << "action: " << cs.action << endl;
+  os << "<check_suite>" << endl << cs.check_suite;
+  os << "<repository>" << endl << cs.repository << endl;
 
-  return false;
+  return os;
 }
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index c29a967..0896c0b 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -35,14 +35,11 @@ namespace brep
     virtual void
     init (cli::scanner&);
 
+    // @@ Can it be static in .cxx file?
+    //
     bool
     respond (response&, status_code, const string& message);
 
-    bool
-    handle_webhook (request&,
-                    const std::optional<std::string>& github_event,
-                    response&);
-
   private:
     shared_ptr<options::ci> options_;
   };