From 7a6192b2bd2152fb82c59ecf3f53ce0affe06342 Mon Sep 17 00:00:00 2001 From: Eddie Kohler Date: Sun, 15 Sep 2024 10:25:24 -0400 Subject: [PATCH] Improve plan for resolving qualified extensions We want "merge" extensions to apply differently to "get" and "post" for API. Old plan didn't allow that. New plan does. Also, new plan checks "get"/"post" on aliases too. And more API spec. --- etc/apifunctions.json | 73 +++++++++++++++++++----------------------- etc/listactions.json | 8 ++--- src/conference.php | 6 +++- src/listaction.php | 42 ++++++++++++++---------- src/paperlist.php | 12 +++---- src/xtparams.php | 74 ++++++++++++++----------------------------- 6 files changed, 96 insertions(+), 119 deletions(-) diff --git a/etc/apifunctions.json b/etc/apifunctions.json index d6a74386b3..e65e2484f0 100644 --- a/etc/apifunctions.json +++ b/etc/apifunctions.json @@ -28,7 +28,7 @@ "parameters": "?p =assignments" }, { - "name": "claimreview", "paper": true, "post": true, "redirect": true, + "name": "claimreview", "post": true, "paper": true, "redirect": true, "function": "RequestReview_API::claimreview", "parameters": "r email", "response": "action review_site_relative" @@ -39,14 +39,14 @@ "parameters": "?p ?=accept =clickthrough_id =clickthrough_type =clickthrough_time" }, { - "name": "comment", "paper": true, "get": true, + "name": "comment", "get": true, "paper": true, "function": "Comment_API::run", "parameters": "c", "response": "?comment", "response_deprecated": "cmt" }, { - "name": "comment", "paper": true, "post": true, + "name": "comment", "post": true, "paper": true, "function": "Comment_API::run", "parameters": "c ?override ?=text ?=response ?=ready ?=topic ?=draft ?=blind ?=tags ?=visibility ?=:attachment ?=by_author ?=review_token ?delete", "response": "?comment ?conflict", @@ -59,12 +59,12 @@ "response": "action ?reason review_site_relative" }, { - "name": "decision", "paper": true, "get": true, + "name": "decision", "get": true, "paper": true, "function": "Decision_API::run", "response": "decision decision_html ?editable" }, { - "name": "decision", "paper": true, "post": true, + "name": "decision", "post": true, "paper": true, "function": "Decision_API::run", "parameters": "=decision", "response": "decision decision_html ?editable", @@ -87,13 +87,13 @@ "response": "fields data ?stat ?classes ?attr" }, { - "name": "follow", "paper": true, "post": true, "xxx": true, + "name": "follow", "post": true, "paper": true, "xxx": true, "function": "Follow_API::run", "parameters": "?u =following", "response": "following" }, { - "name": "formatcheck", "get": true, "post": true, + "name": "formatcheck", "get": true, "function": "FormatCheck_API::run", "parameters": "?p ?doc ?soft ?dt ?docid", "response": "npages nwords problem_fields has_error docid", @@ -110,42 +110,42 @@ "response": "update_at ?status *" }, { - "name": "jserror", "get": true, + "name": "jserror", "post": true, "function": "Error_API::jserror", "parameters": "=error ?=url ?=lineno ?=colno ?=stack" }, { - "name": "lead", "paper": true, "get": true, + "name": "lead", "get": true, "paper": true, "function": "PaperPC_API::lead_api", "response": "lead lead_html ?color_classes" }, { - "name": "lead", "paper": true, "post": true, + "name": "lead", "post": true, "paper": true, "function": "PaperPC_API::lead_api", "parameters": "=lead", "response": "lead lead_html ?color_classes", "response_deprecated": "value result" }, { - "name": "mailtext", "get": true, "post": true, + "name": "mailtext", "get": true, "function": "Mail_API::mailtext", "parameters": "?template ?p ?r ?email ?given_name ?family_name ?affiliation ?reason ?width ?text ?subject ?body", "response": "?templates ?subject ?body" }, { - "name": "manager", "paper": true, "get": true, + "name": "manager", "get": true, "paper": true, "function": "PaperPC_API::manager_api", "response": "manager manager_html ?color_classes", "response_deprecated": "value result" }, { - "name": "manager", "paper": true, "post": true, + "name": "manager", "post": true, "paper": true, "function": "PaperPC_API::manager_api", "parameters": "=manager", "response": "manager manager_html ?color_classes" }, { - "name": "administrator", "alias": "manager" + "name": "administrator", "alias": "manager", "get": true, "post": true }, { "name": "namedformula", "get": true, @@ -169,7 +169,7 @@ "response": "searches" }, { - "name": "mentioncompletion", "get": true, "post": true, + "name": "mentioncompletion", "get": true, "function": "Completion_API::mentioncompletion_api", "parameters": "?p", "response": "mentioncompletion" @@ -187,21 +187,21 @@ "function": "Paper_API::run" }, { - "name": "pc", "get": true, "post": true, + "name": "pc", "get": true, "function": "PaperPC_API::pc_api", "response": "pc" }, { - "name": "requestreview", "paper": true, "post": true, + "name": "requestreview", "post": true, "paper": true, "function": "RequestReview_API::requestreview", "parameters": "?round =email ?=given_name ?=family_name ?=name ?=affiliation ?=override ?=reason" }, { - "name": "review", "paper": true, "get": true, "post": true, + "name": "review", "get": true, "post": true, "paper": true, "function": "Review_API::review" }, { - "name": "reviewhistory", "paper": true, "get": true, "post": true, + "name": "reviewhistory", "get": true, "paper": true, "function": "Review_API::reviewhistory" }, { @@ -210,19 +210,19 @@ "response": "samples types" }, { - "name": "reviewrating", "paper": true, "get": true, + "name": "reviewrating", "get": true, "paper": true, "function": "Review_API::reviewrating", "parameters": "r", "response": "?ratings ?user_rating" }, { - "name": "reviewrating", "paper": true, "post": true, + "name": "reviewrating", "post": true, "paper": true, "function": "Review_API::reviewrating", "parameters": "r =user_rating", "response": "?ratings ?user_rating" }, { - "name": "reviewround", "paper": true, "post": true, "xxx": true, + "name": "reviewround", "post": true, "xxx": true, "paper": true, "function": "Review_API::reviewround" }, { @@ -242,10 +242,10 @@ "response": "value pref ?prefexp ?topic_score" }, { - "name": "reviewpref", "alias": "revpref" + "name": "reviewpref", "alias": "revpref", "get": true, "post": true }, { - "name": "pref", "alias": "revpref" + "name": "pref", "alias": "revpref", "get": true, "post": true }, { "name": "search", "get": true, @@ -269,19 +269,16 @@ "parameters": "v", "response": "postvalue sessioninfo" }, - { - "name": "settags", "alias": "tags" - }, { "name": "tags", "get": true, "paper": true, "function": "Tags_API::run", - "response": "pid tags tags_edit_text tags_view_html tag_decoration_html color_classes" + "response": "pid tags tags_edit_text tags_view_html tag_decoration_html color_classes ?tags_conflicted ?color_classes_conflicted" }, { "name": "tags", "post": true, "function": "Tags_API::run", "parameters": "?p ?=tags ?=addtags ?=deltags ?=tagassignment ?=search", - "response": "?pid ?tags ?tags_edit_text ?tags_view_html ?tag_decoration_html ?color_classes ?p ?ids ?groups ?hotlist ?search_params" + "response": "?pid ?tags ?tags_edit_text ?tags_view_html ?tag_decoration_html ?color_classes ?tags_conflicted ?color_classes_conflicted ?p ?ids ?groups ?hotlist ?search_params" }, { "name": "settingdescriptions", "get": true, @@ -295,12 +292,12 @@ "response": "?dry_run ?changes" }, { - "name": "shepherd", "paper": true, "get": true, + "name": "shepherd", "get": true, "paper": true, "function": "PaperPC_API::shepherd_api", "response": "shepherd shepherd_html ?color_classes" }, { - "name": "shepherd", "paper": true, "post": true, + "name": "shepherd", "post": true, "paper": true, "function": "PaperPC_API::shepherd_api", "parameters": "=shepherd", "response": "shepherd shepherd_html ?color_classes", @@ -322,13 +319,10 @@ "parameters": "tag" }, { - "name": "tagmessages", "paper": true, "get": true, + "name": "tagmessages", "get": true, "paper": true, "function": "Tags_API::tagmessages_api", "response": "pid" }, - { - "name": "tagreport", "alias": "tagmessages" - }, { "name": "track", "post": true, "parameters": "track ?tracker_start_at ?p ?=hotlist-info" @@ -340,14 +334,14 @@ "response": "?tracker ?tracker_recent ?tracker_status ?now tracker_status_at tracker_eventid ?new_trackerid ?tracker_site" }, { - "name": "trackerstatus", "get": true, "post": true, + "name": "trackerstatus", "get": true, "function": "MeetingTracker::trackerstatus_api" }, { "name": "upload", "post": true, "function": "Upload_API::run", "parameters": "?p ?dtype ?start ?offset ?finish ?token ?cancel ?=size ?=mimetype ?=filename ?@blob", - "response": "token dtype filename mimetype size ranges hash server_progress_loaded server_progress_max" + "response": "token ?dtype ?filename ?mimetype ?size ?ranges ?hash ?server_progress_loaded ?server_progress_max" }, { @@ -369,10 +363,7 @@ "response": "report display_current display_default display_difference display_default_message_list" }, { - "name": "listreport", "alias": "viewoptions" - }, - { - "name": "votereport", "paper": true, "get": true, "post": true, + "name": "votereport", "get": true, "paper": true, "function": "Tags_API::votereport_api", "parameters": "tag", "response": "tag report" diff --git a/etc/listactions.json b/etc/listactions.json index 5c3dd8e306..5a2cb42d55 100644 --- a/etc/listactions.json +++ b/etc/listactions.json @@ -39,7 +39,7 @@ "function": "+GetPcassignments_ListAction" }, { - "name": "get/revform", "get": true, "paper_optional": true, + "name": "get/revform", "get": true, "title": "Review assignments/Your review forms", "order": 20000, "allow_if": "reviewer", @@ -47,7 +47,7 @@ "zip": false, "all": false }, { - "name": "get/revformz", "get": true, "paper_optional": true, + "name": "get/revformz", "get": true, "title": "Review assignments/Your review forms (zip)", "order": 20001, "allow_if": "reviewer", @@ -55,7 +55,7 @@ "zip": true, "all": false }, { - "name": "get/allrevform", "get": true, "paper_optional": true, + "name": "get/allrevform", "get": true, "title": "Review assignments/All review forms", "order": 20002, "allow_if": "manager", @@ -63,7 +63,7 @@ "zip": false, "all": true }, { - "name": "get/allrevformz", "get": true, "paper_optional": true, + "name": "get/allrevformz", "get": true, "title": "Review assignments/All review forms (zip)", "order": 20003, "allow_if": "manager", diff --git a/src/conference.php b/src/conference.php index dc04496e32..7e93bf243f 100644 --- a/src/conference.php +++ b/src/conference.php @@ -5472,8 +5472,12 @@ function has_api($fn, ?Contact $user = null, $method = null) { return !!$this->api($fn, $user, $method); } function api($fn, ?Contact $user = null, $method = null) { - $xtp = (new XtParams($this, $user))->add_allow_checker_method($method); + $xtp = (new XtParams($this, $user))->set_require_key_for_method($method); $uf = $xtp->search_name($this->api_map(), $fn); + if (!$uf && $method === "POST") { + $xtp->set_require_key_for_method("GET"); // XXX should not do this + $uf = $xtp->search_name($this->api_map(), $fn); + } return self::xt_enabled($uf) ? $uf : null; } /** @return JsonResult */ diff --git a/src/listaction.php b/src/listaction.php index 8c8f64b432..ca42f9fb06 100644 --- a/src/listaction.php +++ b/src/listaction.php @@ -21,25 +21,25 @@ static function eperm() { } /** @return ComponentSet */ - static function grouped_extensions(Contact $user) { - $gex = new ComponentSet($user, ["etc/listactions.json"], $user->conf->opt("listActions")); - foreach ($gex->members("__expand") as $gj) { - if (!isset($gj->allow_if) || $gex->allowed($gj->allow_if, $gj)) { + static function components(Contact $user) { + $cs = new ComponentSet($user, ["etc/listactions.json"], $user->conf->opt("listActions")); + foreach ($cs->members("__expand") as $gj) { + if (!isset($gj->allow_if) || $cs->allowed($gj->allow_if, $gj)) { Conf::xt_resolve_require($gj); - call_user_func($gj->expand_function, $gex, $gj); + call_user_func($gj->expand_function, $cs, $gj); } } - return $gex; + return $cs; } - /** @param ComponentSet $gex + /** @param ComponentSet $cs * @param string $group * @return list */ - static function members_selector_options($gex, $group) { + static function members_selector_options($cs, $group) { $last_group = null; $sel_opt = []; $p = strlen($group) + 1; - foreach ($gex->members($group) as $rf) { + foreach ($cs->members($group) as $rf) { if (!str_starts_with($rf->name, "__")) { $as = strpos($rf->title, "/"); if ($as === false) { @@ -78,16 +78,24 @@ static private function do_call($name, Contact $user, Qrequest $qreq, $selection return JsonResult::make_error(403, "<0>Missing credentials"); } $conf = $user->conf; - $gex = self::grouped_extensions($user); - $gex->xtp->add_allow_checker_method($qreq->method()); - $uf = $gex->get($name); - if (!$uf && ($slash = strpos($name, "/"))) { - $uf = $gex->get(substr($name, 0, $slash)); + $cs = self::components($user); + $slash = strpos($name, "/"); + $cs->xtp->set_require_key_for_method($qreq->method()); + $uf = $cs->get($name); + if (!$uf && $slash > 0) { + $uf = $cs->get(substr($name, 0, $slash)); + } + // XXX allow `POST` requests to `get` + if (!$uf && $qreq->is_post()) { + $cs->xtp->set_require_key_for_method("GET"); + $uf = $cs->get($name); + if (!$uf && $slash > 0) { + $uf = $cs->get(substr($name, 0, $slash)); + } } if (!$uf) { - $gex->reset_context(); - $gex->xtp->allow_checkers = []; - $uf1 = $gex->get($name); + $cs->reset_context()->set_require_key_for_method(null); + $uf1 = $cs->get($name); if ($uf1) { return JsonResult::make_error(405, "<0>Method not supported"); } diff --git a/src/paperlist.php b/src/paperlist.php index 231a34c17a..6e8282e35f 100644 --- a/src/paperlist.php +++ b/src/paperlist.php @@ -1986,19 +1986,19 @@ private function _footer($ncol) { $atab = $qreq->fn ?? $qreq->atab; } - $gex = ListAction::grouped_extensions($this->user); - $gex->add_xt_checker([$this, "list_checker"]); - $gex->apply_key_filter("display_if"); + $cs = ListAction::components($this->user); + $cs->add_xt_checker([$this, "list_checker"]); + $cs->apply_key_filter("display_if"); if ($this->_footer_filter) { - $gex->apply_filter($this->_footer_filter); + $cs->apply_filter($this->_footer_filter); } $lllgroups = []; $whichlll = -1; - foreach ($gex->members("") as $rf) { + foreach ($cs->members("") as $rf) { if (isset($rf->render_function) && !str_starts_with($rf->name, "__") && Conf::xt_resolve_require($rf) - && ($lllg = call_user_func($rf->render_function, $this, $qreq, $gex, $rf))) { + && ($lllg = call_user_func($rf->render_function, $this, $qreq, $cs, $rf))) { if (is_string($lllg)) { $lllg = [$lllg]; } diff --git a/src/xtparams.php b/src/xtparams.php index 91fc8cb498..3c1489bdeb 100644 --- a/src/xtparams.php +++ b/src/xtparams.php @@ -11,8 +11,8 @@ class XtParams { public $alias = true; /** @var string */ public $reflags = ""; - /** @var ?list */ - public $allow_checkers; + /** @var ?string */ + public $require_key; /** @var ?list */ public $primitive_checkers; /** @var ?object */ @@ -31,37 +31,15 @@ function __construct($conf, $user) { $this->user = $user; } - /** @param object $xt - * @param XtParams $xtp - * @return ?bool */ - static function allow_checker_GET($xt, $xtp) { - return isset($xt->alias) ? null : $xt->get ?? false; - } - - /** @param object $xt - * @param XtParams $xtp - * @return ?bool */ - static function allow_checker_HEAD($xt, $xtp) { - return isset($xt->alias) ? null : $xt->head ?? $xt->get ?? false; - } - - /** @param object $xt - * @param XtParams $xtp - * @return ?bool */ - static function allow_checker_POST($xt, $xtp) { - return isset($xt->alias) ? null : $xt->post ?? $xt->get ?? false; - } - /** @param ?string $method * @return $this */ - function add_allow_checker_method($method) { - if ($method === "GET" || $method === "HEAD" || $method === "POST") { - $this->allow_checkers[] = "XtParams::allow_checker_{$method}"; - } else if ($method !== null && $method !== "") { - $method = strtolower($method); - $this->allow_checkers[] = function ($xt, $xtp) use ($method) { - return isset($xt->alias) ? null : $xt->$method ?? false; - }; + function set_require_key_for_method($method) { + if ($method === null || $method === "") { + $this->require_key = null; + } else if ($method === "GET" || $method === "HEAD") { + $this->require_key = "get"; + } else { + $this->require_key = strtolower($method); } return $this; } @@ -69,16 +47,7 @@ function add_allow_checker_method($method) { /** @param object $xt * @return bool */ function checkf($xt) { - if (isset($xt->allow_if) && !$this->check($xt->allow_if, $xt)) { - return false; - } - if ($this->allow_checkers !== null) { - foreach ($this->allow_checkers as $checker) { - if (($x = $checker($xt, $this)) !== null) - return $x; - } - } - return true; + return !isset($xt->allow_if) || $this->check($xt->allow_if, $xt); } /** @param string $s @@ -344,25 +313,30 @@ function search_list($list) { if ($nlist > 1) { usort($list, "Conf::xt_priority_compare"); } + $reqkey = $this->require_key; for ($i = 0; $i < $nlist; ++$i) { $xt = $list[$i]; + if ($reqkey !== null && !($xt->{$reqkey} ?? null)) { + continue; + } while ($i + 1 < $nlist && ($xt->merge ?? false)) { ++$i; - $overlay = $xt; - $xt = clone $list[$i]; - foreach (get_object_vars($overlay) as $k => $v) { + if ($reqkey !== null && !($list[$i]->{$reqkey} ?? null)) { + continue; + } + $nxt = clone $list[$i]; + foreach (get_object_vars($xt) as $k => $v) { if ($k === "merge" || $k === "__source_order") { // skip - } else if ($v === null) { - unset($xt->{$k}); - } else if (!property_exists($xt, $k) + } else if (!property_exists($nxt, $k) || !is_object($v) - || !is_object($xt->{$k})) { - $xt->{$k} = $v; + || !is_object($nxt->{$k})) { + $nxt->{$k} = $v; } else { - object_replace_recursive($xt->{$k}, $v); + object_replace_recursive($nxt->{$k}, $v); } } + $xt = $nxt; } if (isset($xt->deprecated) && $xt->deprecated) { $name = $xt->name ?? "";