-
Notifications
You must be signed in to change notification settings - Fork 130
[NC-2046] JWT authentication for JSON-RPC #815
[NC-2046] JWT authentication for JSON-RPC #815
Conversation
…Method.getPermissions() and unit tests
@@ -90,6 +90,8 @@ | |||
private HttpRequestFactory httpRequestFactory; | |||
private Optional<EthNetworkConfig> ethNetworkConfig = Optional.empty(); | |||
private boolean useWsForJsonRpc = false; | |||
private boolean useAuthenticationTokenInHeaderForJsonRpc = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably simplify this boolean out by either making token an Optional or checking token != null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public void useAuthenticationTokenInHeaderForJsonRpc(final String token) { | ||
|
||
if (jsonRequestFactories != null) { | ||
jsonRequestFactories.shutdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add jsonRequestFactoies = null;
here, it's also wrong in useWebSocketsForJsonRpc but we haven't managed to race condition it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused one AT to fail so putting it back in
java.lang.RuntimeException: org.web3j.protocol.exceptions.ClientConnectionException: Invalid response received: 400; Websocket endpoint can't handle HTTP requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RpcApisTogglesAcceptanceTest.shouldSucceedConnectingToNodeWithWsRpcEnabled
private Node node; | ||
|
||
@Before | ||
public void setUp() throws IOException, URISyntaxException { | ||
final ClusterConfiguration clusterConfiguration = | ||
new ClusterConfigurationBuilder().setAwaitPeerDiscovery(false).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the await peer discovery being off is there still something that is awaiting the node being ready? Want to avoid the login call further down occurring before the service has finished starting in the node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is nothing awaiting the node being ready. If you can't do any JSON RPC calls, is there another way to ask the node if it's ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ if this is a problem, it's a problem for a few tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to test that the services are up (JSON-RPC service), can't we send a request to the 'login' route and expect any response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's exactly what the test does. I think Chris is asking is there a chance that the login call happens before the node has started?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is that we could have a check (prior to the test) if the JSON-RPC service is responding. Currently, we use the admin_peers I guess, we could have an alternative (for when we disable the discovery check) that would send a request to the login
route and expect it to be handled (it doesn't need to be a successful login, just need the request to be "received" by the node).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha. awaitLogin()
* | ||
* @return list of permissions that match this method. | ||
*/ | ||
default List<String> getPermissions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered supporting */*
as a valid permission config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda, we need to have a conversation about where and how we inflate the permissions list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
PR description
check authentication and permissions for JSON-RPC methods
Fixed Issue(s)
NC-2046