From 041604583b51d292be694ab475712d86bb9af9b5 Mon Sep 17 00:00:00 2001 From: Francois Kritzinger Date: Fri, 6 Dec 2024 11:41:31 +0200 Subject: Post-review changes --- mod/mod-ci-github-gh.cxx | 20 ++++++++++++--- mod/mod-ci-github-gh.hxx | 17 ++++++------- mod/mod-ci-github-service-data.cxx | 21 ++++++++-------- mod/mod-ci-github-service-data.hxx | 12 ++++----- mod/mod-ci-github.cxx | 51 ++++++++++++++++---------------------- mod/mod-ci-github.hxx | 4 +-- 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 (); + 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 (); + 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 (); + 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 conclusion; - // Note: unlike the check_run webhook's app_id this can be null. - // - optional 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 ("app_id"); - installation_id = - p.next_expect_member_number ("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 jwt (generate_jwt (*cs.check_suite.app_id, trace, error)); + optional 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 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& 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 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 - 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 - obtain_installation_access_token (uint64_t install_id, + obtain_installation_access_token (const string& install_id, string jwt, const basic_mark& error) const; -- cgit v1.1