From bb38f83b886d0bf8fc63ffc6d42eae91c2a36daf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 12 Sep 2023 10:22:44 +0930 Subject: [PATCH] checkrune: make nodeid and method optional. nodeid is only useful when we know the peer we're talking to (e.g. commando). Signed-off-by: Rusty Russell No-schema-diff-check: We're simply making optional, not deprecating! --- doc/lightning-checkrune.7.md | 2 +- doc/schemas/checkrune.request.json | 8 +++--- lightningd/runes.c | 20 +++++++++----- tests/test_runes.py | 43 ++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 13 deletions(-) diff --git a/doc/lightning-checkrune.7.md b/doc/lightning-checkrune.7.md index 9d895b2a8..163d672c0 100644 --- a/doc/lightning-checkrune.7.md +++ b/doc/lightning-checkrune.7.md @@ -4,7 +4,7 @@ lightning-checkrune -- Command to Validate Rune SYNOPSIS -------- -**checkrune** *rune* *nodeid* *method* [*params*] +**checkrune** *rune* [*nodeid*] [*method*] [*params*] DESCRIPTION ----------- diff --git a/doc/schemas/checkrune.request.json b/doc/schemas/checkrune.request.json index e0029afbe..83d0fa476 100644 --- a/doc/schemas/checkrune.request.json +++ b/doc/schemas/checkrune.request.json @@ -3,9 +3,7 @@ "type": "object", "additionalProperties": false, "required": [ - "nodeid", - "rune", - "method" + "rune" ], "added": "v23.08", "properties": { @@ -15,11 +13,11 @@ }, "nodeid": { "type": "string", - "description": "node id of your node" + "description": "node id of requesting node *(required until v23.11)*" }, "method": { "type": "string", - "description": "method for which rune needs to be validated" + "description": "method for which rune needs to be validated *(required until v23.11)*" }, "params": { "oneOf": [ diff --git a/lightningd/runes.c b/lightningd/runes.c index a93d342ce..99b940c87 100644 --- a/lightningd/runes.c +++ b/lightningd/runes.c @@ -674,11 +674,17 @@ static const char *check_condition(const tal_t *ctx, if (streq(alt->fieldname, "time")) { return rune_alt_single_int(ctx, alt, time_now().ts.tv_sec); } else if (streq(alt->fieldname, "id")) { - const char *id = node_id_to_hexstr(tmpctx, cinfo->peer); - return rune_alt_single_str(ctx, alt, id, strlen(id)); + if (cinfo->peer) { + const char *id = node_id_to_hexstr(tmpctx, cinfo->peer); + return rune_alt_single_str(ctx, alt, id, strlen(id)); + } + return rune_alt_single_missing(ctx, alt); } else if (streq(alt->fieldname, "method")) { - return rune_alt_single_str(ctx, alt, - cinfo->method, strlen(cinfo->method)); + if (cinfo->method) { + return rune_alt_single_str(ctx, alt, + cinfo->method, strlen(cinfo->method)); + } + return rune_alt_single_missing(ctx, alt); } else if (streq(alt->fieldname, "pnum")) { return rune_alt_single_int(ctx, alt, (cinfo && cinfo->params) ? cinfo->params->size : 0); } else if (streq(alt->fieldname, "rate")) { @@ -748,8 +754,8 @@ static struct command_result *json_checkrune(struct command *cmd, if (!param(cmd, buffer, params, p_req("rune", param_rune, &ras), - p_req("nodeid", param_node_id, &nodeid), - p_req("method", param_string, &method), + p_opt("nodeid", param_node_id, &nodeid), + p_opt("method", param_string, &method), p_opt("params", param_params, &methodparams), NULL)) return command_param_failed(); @@ -793,6 +799,6 @@ static const struct json_command checkrune_command = { "checkrune", "utility", json_checkrune, - "Checks rune for validity with required {nodeid}, {rune}, {method} and optional {params} and returns {valid: true} or error message" + "Checks rune for validity with required {rune} and optional {nodeid}, {method}, {params} and returns {valid: true} or error message" }; AUTODATA(json_command, &checkrune_command); diff --git a/tests/test_runes.py b/tests/test_runes.py index 802395020..45d70b356 100644 --- a/tests/test_runes.py +++ b/tests/test_runes.py @@ -471,6 +471,49 @@ def test_commando_blacklist_migration(node_factory): {'start': 9, 'end': 9}]} +def test_missing_method_or_nodeid(node_factory): + """For v23.11 we realized that nodeid should not be required for checkrune, which means method (which followed it) could not be require either""" + l1 = node_factory.get_node() + rune1 = l1.rpc.createrune(restrictions=[["method=getinfo"]])['rune'] + rune2 = l1.rpc.createrune(restrictions=[["id={}".format(l1.info['id'])]])['rune'] + rune3 = l1.rpc.createrune(restrictions=[["method!"]])['rune'] + rune4 = l1.rpc.createrune(restrictions=[["id!"]])['rune'] + + # Simple cases with nodeid and method + assert l1.rpc.checkrune(rune=rune1, + nodeid=l1.info['id'], + method='getinfo')['valid'] is True + assert l1.rpc.checkrune(rune=rune2, + nodeid=l1.info['id'], + method='getinfo')['valid'] is True + # No nodeid works for rune1 + assert l1.rpc.checkrune(rune=rune1, + method='getinfo')['valid'] is True + # No method works for rune2 + assert l1.rpc.checkrune(rune=rune2, + nodeid=l1.info['id'])['valid'] is True + + # No method works for rune3 + assert l1.rpc.checkrune(rune=rune3, + nodeid=l1.info['id'])['valid'] is True + # No id works for rune4 + assert l1.rpc.checkrune(rune=rune4, + method='getinfo')['valid'] is True + + # This fails, as method is missing + with pytest.raises(RpcError, match='Not permitted: method not present'): + l1.rpc.checkrune(rune=rune1) + # This fails, as id is missing + with pytest.raises(RpcError, match='Not permitted: id not present'): + l1.rpc.checkrune(rune=rune2) + # This fails, as method is present + with pytest.raises(RpcError, match='Not permitted: method is present'): + l1.rpc.checkrune(rune=rune3, method='getinfo') + # This fails, as id is present + with pytest.raises(RpcError, match='Not permitted: id is present'): + l1.rpc.checkrune(rune=rune4, nodeid=l1.info['id']) + + def test_showrune_id(node_factory): l1 = node_factory.get_node()