Post-review changesci-github-2

6 files changed, 64 insertions, 61 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index 9eebf34..021ff6b 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -205,7 +205,19 @@ namespace brep
         //
         while (p.next_expect (event::name, event::end_object))
         {
-          if (c (ai, "id")) app_id = p.next_expect_number<uint64_t> ();
+          if (c (ai, "id"))
+          {
+            // Note: unlike the check_run webhook's app.id, the check_suite
+            // one can be null. It's unclear under what circumstances, but it
+            // shouldn't happen unless something is broken.
+            //
+            string* v (p.next_expect_number_null ());
+
+            if (v == nullptr)
+              throw_json (p, "check_suite.app.id is null");
+
+            app_id = *v;
+          }
           else p.next_expect_value_skip ();
         }
 
@@ -230,7 +242,7 @@ namespace brep
        << ", head_sha: " << cs.head_sha
        << ", latest_check_runs_count: " << cs.check_runs_count
        << ", conclusion: " << (cs.conclusion ? *cs.conclusion : "null")
-       << ", app_id: " << (cs.app_id ? to_string (*cs.app_id) : "null");
+       << ", app_id: " << cs.app_id;
 
     return os;
   }
@@ -298,7 +310,7 @@ namespace brep
         //
         while (p.next_expect (event::name, event::end_object))
         {
-          if (c (ai, "id")) app_id = p.next_expect_number<uint64_t> ();
+          if (c (ai, "id")) app_id = p.next_expect_number ();
           else p.next_expect_value_skip ();
         }
 
@@ -501,7 +513,7 @@ namespace brep
         return p.name () == s ? (v = true) : false;
       };
 
-      if (c (i, "id")) id = p.next_expect_number<uint64_t> ();
+      if (c (i, "id")) id = p.next_expect_number ();
       else p.next_expect_value_skip ();
     }
 
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index b34dfd1..ab6dbaa 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -26,9 +26,10 @@ namespace brep
   // GitHub request/response types (all start with gh_).
   //
   // Note that the GitHub REST and GraphQL APIs use different id types and
-  // values. In the REST API they are usually integers (but sometimes
-  // strings!) whereas in GraphQL they are always strings (note:
-  // base64-encoded and opaque, not just the REST id value as a string).
+  // values. In the REST API they are usually integers (but check the API
+  // reference for the object in question) whereas in GraphQL they are always
+  // strings (note: base64-encoded and opaque, not just the REST id value as a
+  // string).
   //
   // In both APIs the id field is called `id`, but REST responses and webhook
   // events also contain the corresponding GraphQL object's id in the
@@ -65,9 +66,7 @@ namespace brep
     size_t check_runs_count;
     optional<string> conclusion;
 
-    // Note: unlike the check_run webhook's app_id this can be null.
-    //
-    optional<uint64_t> app_id;
+    string app_id;
 
     explicit
     gh_check_suite_ex (json::parser&);
@@ -98,7 +97,7 @@ namespace brep
     string details_url;
     gh_check_suite check_suite;
 
-    uint64_t app_id;
+    string app_id;
 
     explicit
     gh_check_run_ex (json::parser&);
@@ -130,7 +129,7 @@ namespace brep
     // simplicity we emulate check_suite and check_run by storing the app-id
     // webhook query parameter here.
     //
-    uint64_t app_id;
+    string app_id;
 
     explicit
     gh_pull_request (json::parser&);
@@ -156,7 +155,7 @@ namespace brep
   //
   struct gh_installation
   {
-    uint64_t id; // Note: used for installation access token (REST API).
+    string id; // Note: used for installation access token (REST API).
 
     explicit
     gh_installation (json::parser&);
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 4c6c9fe..9f66a6c 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -54,9 +54,8 @@ namespace brep
     p.next_expect_name ("installation_access");
     installation_access = gh_installation_access_token (p);
 
-    app_id = p.next_expect_member_number<uint64_t> ("app_id");
-    installation_id =
-        p.next_expect_member_number<uint64_t> ("installation_id");
+    app_id = p.next_expect_member_string ("app_id");
+    installation_id = p.next_expect_member_string ("installation_id");
 
     repository_node_id = p.next_expect_member_string ("repository_node_id");
     repository_clone_url = p.next_expect_member_string ("repository_clone_url");
@@ -136,8 +135,8 @@ namespace brep
   service_data (bool ws,
                 string iat_tok,
                 timestamp iat_ea,
-                uint64_t aid,
-                uint64_t iid,
+                string aid,
+                string iid,
                 string rid,
                 string rcu,
                 kind_type k,
@@ -148,8 +147,8 @@ namespace brep
       : kind (k), pre_check (pc), re_request (rr),
         warning_success (ws),
         installation_access (move (iat_tok), iat_ea),
-        app_id (aid),
-        installation_id (iid),
+        app_id (move (aid)),
+        installation_id (move (iid)),
         repository_node_id (move (rid)),
         repository_clone_url (move (rcu)),
         check_sha (move (cs)),
@@ -164,8 +163,8 @@ namespace brep
   service_data (bool ws,
                 string iat_tok,
                 timestamp iat_ea,
-                uint64_t aid,
-                uint64_t iid,
+                string aid,
+                string iid,
                 string rid,
                 string rcu,
                 kind_type k,
@@ -178,8 +177,8 @@ namespace brep
       : kind (k), pre_check (pc), re_request (rr),
         warning_success (ws),
         installation_access (move (iat_tok), iat_ea),
-        app_id (aid),
-        installation_id (iid),
+        app_id (move (aid)),
+        installation_id (move (iid)),
         repository_node_id (move (rid)),
         repository_clone_url (move (rcu)),
         pr_node_id (move (pid)),
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index fc97cfa..50bb49d 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -89,8 +89,8 @@ namespace brep
     //
     gh_installation_access_token installation_access;
 
-    uint64_t app_id;
-    uint64_t installation_id;
+    string app_id;
+    string installation_id;
 
     string repository_node_id; // GitHub-internal opaque repository id.
 
@@ -152,8 +152,8 @@ namespace brep
     service_data (bool warning_success,
                   string iat_token,
                   timestamp iat_expires_at,
-                  uint64_t app_id,
-                  uint64_t installation_id,
+                  string app_id,
+                  string installation_id,
                   string repository_node_id,
                   string repository_clone_url,
                   kind_type kind,
@@ -167,8 +167,8 @@ namespace brep
     service_data (bool warning_success,
                   string iat_token,
                   timestamp iat_expires_at,
-                  uint64_t app_id,
-                  uint64_t installation_id,
+                  string app_id,
+                  string installation_id,
                   string repository_node_id,
                   string repository_clone_url,
                   kind_type kind,
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 65f8c94..5bcec98 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -228,7 +228,7 @@ namespace brep
 
     // Process the `app-id` and `warning` webhook request query parameters.
     //
-    uint64_t app_id; // @@ string
+    string app_id;
     bool warning_success;
     {
       const name_values& rps (rq.parameters (1024, true /* url_only */));
@@ -248,16 +248,7 @@ namespace brep
             badreq ("missing 'app-id' webhook query parameter value");
 
           ai = true;
-
-          // Parse the value.
-          //
-          char* e (nullptr);
-          app_id = strtoull (rp.value->c_str (), &e, 10);
-          if (*e != '\0' || app_id == 0 || app_id == ULLONG_MAX)
-          {
-            badreq ("invalid 'app-id' webhook query parameter value: '" +
-                    *rp.value + '\'');
-          }
+          app_id = *rp.value;
         }
         else if (rp.name == "warning")
         {
@@ -308,17 +299,9 @@ namespace brep
         throw invalid_request (400, move (m));
       }
 
-      // It's unclear under what circumstances the app_id can be null but it
-      // shouldn't happen unless something is broken.
-      //
-      // @@ Move to parsing, make non-optional.
-      //
-      if (!cs.check_suite.app_id)
-        throw invalid_request (400, "absent app.id in check_suite webhook");
-
-      if (*cs.check_suite.app_id != app_id)
+      if (cs.check_suite.app_id != app_id)
       {
-        fail << "webhook check_suite app.id " << *cs.check_suite.app_id
+        fail << "webhook check_suite app.id " << cs.check_suite.app_id
              << " does not match app-id query parameter " << app_id;
       }
 
@@ -576,7 +559,7 @@ namespace brep
     // let's obtain it to flush out any permission issues early. Also, it is
     // valid for an hour so we will most likely make use of it.
     //
-    optional<string> jwt (generate_jwt (*cs.check_suite.app_id, trace, error));
+    optional<string> jwt (generate_jwt (cs.check_suite.app_id, trace, error));
     if (!jwt)
       throw server_error ();
 
@@ -660,7 +643,7 @@ namespace brep
     service_data sd (warning_success,
                      iat->token,
                      iat->expires_at,
-                     *cs.check_suite.app_id,
+                     cs.check_suite.app_id,
                      cs.installation.id,
                      move (cs.repository.node_id),
                      move (cs.repository.clone_url),
@@ -2940,22 +2923,32 @@ namespace brep
   }
 
   optional<string> ci_github::
-  generate_jwt (uint64_t app_id,
+  generate_jwt (const string& app_id,
                 const basic_mark& trace,
                 const basic_mark& error) const
   {
     string jwt;
     try
     {
-      string ai (to_string (app_id));
+      // Look up the private key path for the app id and fail if not found.
+      //
+      const map<string, dir_path>& pks (
+        options_->ci_github_app_id_private_key ());
+
+      auto pk (pks.find (app_id));
+      if (pk == pks.end ())
+      {
+        error << "unable to generate JWT: "
+              << "no private key configured for app id " << app_id;
+        return nullopt;
+      }
 
       // Set token's "issued at" time 60 seconds in the past to combat clock
       // drift (as recommended by GitHub).
       //
       jwt = brep::generate_jwt (
           *options_,
-          options_->ci_github_app_id_private_key ()[ai], // @@ Lookup and fail.
-          ai,
+          pk->second, app_id,
           chrono::seconds (options_->ci_github_jwt_validity_period ()),
           chrono::seconds (60));
 
@@ -3011,7 +3004,7 @@ namespace brep
   //   example.
   //
   optional<gh_installation_access_token> ci_github::
-  obtain_installation_access_token (uint64_t iid,
+  obtain_installation_access_token (const string& iid,
                                     string jwt,
                                     const basic_mark& error) const
   {
@@ -3020,7 +3013,7 @@ namespace brep
     {
       // API endpoint.
       //
-      string ep ("app/installations/" + to_string (iid) + "/access_tokens");
+      string ep ("app/installations/" + iid + "/access_tokens");
 
       uint16_t sc (
           github_post (iat, ep, strings {"Authorization: Bearer " + jwt}));
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index a9617fa..059801a 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -120,7 +120,7 @@ namespace brep
     details_url (const build&) const;
 
     optional<string>
-    generate_jwt (uint64_t app_id,
+    generate_jwt (const string& app_id,
                   const basic_mark& trace,
                   const basic_mark& error) const;
 
@@ -129,7 +129,7 @@ namespace brep
     // goes wrong.
     //
     optional<gh_installation_access_token>
-    obtain_installation_access_token (uint64_t install_id,
+    obtain_installation_access_token (const string& install_id,
                                       string jwt,
                                       const basic_mark& error) const;