This PR implements a new RPC, getrpcwhitelist, that returns whitelisted methods for the calling user.
Example:
./src/bitcoin-cli -regtest getrpcwhitelist
{
"methods": [
"getbalance",
"getrpcwhitelist",
"getwalletinfo"
]
}
A new functional test, rpc_getrpcwhitelist, and a release-notes entry are included.
Agree with making this list a separate RPC call (it could potentially be long), but what about putting user on the normal getrpcinfo? It seems general enough.
Agree with making this list a separate RPC call (it could potentially be long), but what about putting
useron the normalgetrpcinfo? It seems general enough.
Ok. Should I open a new PR or change it en passant?
getrpcinfo leaks other information, so it might not be whitelisted, see #12248 (comment) . I agree with the suggestion, but it should be done separate form introducing this new call.
getrpcinfoleaks other information, so it might not be whitelisted, see #12248 (comment) . I agree with the suggestion, but it should be done separate form introducing this new call.
Ok, will remove user from this rpc and add it to getrpcinfo in a separate PR.
So help and getrpcwhitelist are always available, or are there more "public" calls?
So
helpandgetrpcwhitelistare always available, or are there more "public" calls?
I did not make getrpcwhitelist always available. But I can make it, if this should be the case.
--EDIT: Sorry, I was a bit misleading: getrpcwhitelist is available by default, but when user is setting a whitelist in the config, then getrpcwhitelist must be added there as well.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
getrpcinfo should not be made public by default. Concept ACK on the new RPC though.
~The new RPC should return the permissions for the current user only; returning the permissions for other users should not be allowed.~ nvm
321 | @@ -322,3 +322,8 @@ void StopHTTPRPC() 322 | httpRPCTimerInterface.reset(); 323 | } 324 | } 325 | + 326 | +const std::set<std::string>& GetWhitelistedRpcs(const std::string& userName) 327 | +{ 328 | + return g_rpc_whitelist[userName];
return g_rpc_whitelist.at(userName);
Otherwise there is no guarantee that the whitelist will not get corrupted
Thanks!
17 | @@ -18,6 +18,7 @@ 18 | #include <memory> // for unique_ptr 19 | #include <unordered_map> 20 | 21 | +const std::set<std::string>& GetWhitelistedRpcs(const std::string&);
Didn't include src/httprpc.h because would add a circular dependency?
Yes, because travis failed on two builds with such an error.
I think it's probably better to add the #include and update test/lint/lint-circular-dependencies.sh.
Was this resolved by mistake?
I don't think I should simply change the linter-checks because of a single function. Maybe someone more qualified should be asked for advice.
Even nicer would be to solve the circular dependency in the first place. Using a forward declare here instead of an include isn't a solution - it's just hiding it from the linter.
If it's too much work to resolve that in this PR, my suggestion is being explicit about it and adding an exception to the linter.
I'll try to extract the circular structures into another include and then recombine everything. Maybe better in a separate PR?
So, I have extracted the following classes and functions into a new include rpc/interfaces.h
class RPCTimerBase
class RPCTimerInterface
class CRPCCommand
class CRPCTable
std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq);
void RPCSetTimerInterface(RPCTimerInterface *iface);
void RPCSetTimerInterfaceIfUnset(RPCTimerInterface *iface);
void RPCUnsetTimerInterface(RPCTimerInterface *iface);
A test run with circular-dependencies.pyscript shows no httprpc->server->httprpc anymore (but there are other unrelated circular dependencies still)
I have thought about extracting their implementations in rpc/server.cpp as well, but as this would look even more invasive, I'll remain with this single include file for now.
I think, I should open a separate PR and let others discuss about it. And if it gets merged, I'll rebase this PR afterwards.
I have outsourced this task into this PR (draft): https://github.com/bitcoin/bitcoin/pull/18871
Will resolve this conversation if you don't mind. In case my other PR gets merged we can continue from there (with rebase etc.).
321 | @@ -322,3 +322,8 @@ void StopHTTPRPC() 322 | httpRPCTimerInterface.reset(); 323 | } 324 | } 325 | + 326 | +const std::set<std::string>& GetWhitelistedRpcs(const std::string& userName)
Nit: user_name
Probably can use unordered_set, but maybe that's a refactor for g_rpc_whitelist...
@luke-jr I think unordered_set will be slower in most cases & use more memory, hence choosing set.
Thanks. Adapted.
242 | + "\nReturns whitelisted RPCs for the current user.\n", 243 | + {}, 244 | + RPCResult{ 245 | + RPCResult::Type::OBJ, "", "", 246 | + { 247 | + {RPCResult::Type::ARR, "methods", "List of RPCs that the user is allowed to call",
Is there a case for making it an Object with possible details of more fine-grained calls? Or is it safe to assume we won't go beyond the method level for permissions?
I think that's a decent idea for future-proofing the return type, but there is some disagreement of the level of permission control we want to allow in core v.s. a layer on top.
utACK, some nits/questions
I liked this and I am looking forward to seeing this reopened.
@brakmic any context on why you closed it out? Do you want someone to pick it up or are you planning on restarting it?
Happy to help you move it along (review process in Bitcoin Core can be slow!)