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 <[email protected]>
No-schema-diff-check: We're simply making optional, not deprecating!

Author: rustyrussell

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
 -----------

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": [

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);

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()