Pass identity information into RpcMethod from Http Service #189

rain-on · 2019-11-15T06:20:38Z

To support Privacy in a multi-tenancy operating mode, privacy-oriented requests are required to be submitted with a JWT containing the Enclave public key to be used for the operation.

This enclave key is not part of the Json RPC Body, but rather part of the header - thus the JsonRpcRequest class has been renamed JsonRpcRequestBody (and represents only the body of the request), and a new JsonRpcRequest class has been created (comprising a User, and also the a JsonRpcRequestBody).

All tests, and methods have been updated to handle the additional layer of indirection.

Signed-off-by: Trent Mohay trent.mohay@consensys.net

jframe · 2019-11-15T06:27:17Z

...eum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/JsonRpcRequest.java

-  @JsonGetter("params")
-  public Object[] getParams() {
-    return params;
+  public JsonRpcRequest(final JsonRpcRequestBody requestBody, final Optional<User> user) {


The user doesn't need to be optional as there another constructor to allow for the none case

jframe · 2019-11-15T06:28:24Z

...eum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/JsonRpcRequest.java

-  @JsonIgnore
-  public int getParamLength() {
-    return hasParams() ? params.length : 0;
+  public Optional<User> getIdentity() {


What's the distinction between user and identity? Perhaps this should be just getUser?

done - was written before finding the User class 👍

jframe · 2019-11-17T23:03:01Z

...c/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/websocket/WebSocketRequestHandler.java

@@ -76,7 +77,8 @@ public void handle(
            LOG.debug("WS-RPC request -> {}", request.getMethod());
            request.setConnectionId(id);
            if (AuthenticationUtils.isPermitted(authenticationService, user, method)) {
-              future.complete(method.response(request));
+              final JsonRpcRequest fullRequest = new JsonRpcRequest(request, user);


What's the distinction between request and fullRequest?

FullRequest is now "requestContext" - and contains the request, and the user

jframe · 2019-11-17T23:05:48Z

...dger/besu/ethereum/api/jsonrpc/websocket/subscription/request/SubscriptionRequestMapper.java

@@ -85,8 +85,8 @@ public UnsubscribeRequest mapUnsubscribeRequest(final JsonRpcRequest jsonRpcRequ
  }

  private WebSocketRpcRequest validateRequest(final JsonRpcRequest jsonRpcRequest) {
-    if (jsonRpcRequest instanceof WebSocketRpcRequest) {
-      return (WebSocketRpcRequest) jsonRpcRequest;
+    if (jsonRpcRequest.getRequestBody() instanceof WebSocketRpcRequest) {


This reads a bit odd. Thinking should rename WebSocketRpcRequest to WebSocketRpcRequestBody as well

jframe · 2019-11-17T23:12:42Z

...org/hyperledger/besu/ethereum/api/jsonrpc/websocket/methods/EthSubscribeIntegrationTest.java

@@ -62,7 +62,7 @@ public void before() {
  public void shouldAddConnectionToMap(final TestContext context) {
    final Async async = context.async();

-    final JsonRpcRequest subscribeRequest = createEthSubscribeRequest(CONNECTION_ID_1);
+    final JsonRpcRequestBody subscribeRequest = createEthSubscribeRequest(CONNECTION_ID_1);


rename createEthSubscribeRequest to createEthSubscribeRequestBody

jframe · 2019-11-17T23:13:39Z

...g/hyperledger/besu/ethereum/api/jsonrpc/websocket/methods/EthUnsubscribeIntegrationTest.java

@@ -63,7 +63,7 @@ public void shouldRemoveConnectionWithSingleSubscriptionFromMap(final TestContex
    final Long subscriptionId = subscriptionManager.subscribe(subscribeRequest);
    assertThat(subscriptionManager.getSubscriptionById(subscriptionId)).isNotNull();

-    final JsonRpcRequest unsubscribeRequest =
+    final JsonRpcRequestBody unsubscribeRequest =


rename to unsubscribeRequestBody

jframe · 2019-11-17T23:13:51Z

...g/hyperledger/besu/ethereum/api/jsonrpc/websocket/methods/EthUnsubscribeIntegrationTest.java

@@ -95,7 +95,7 @@ public void shouldRemoveSubscriptionAndKeepConnection(final TestContext context)
    assertThat(subscriptionManager.getSubscriptionById(subscriptionId1)).isNotNull();
    assertThat(subscriptionManager.getSubscriptionById(subscriptionId2)).isNotNull();

-    final JsonRpcRequest unsubscribeRequest =
+    final JsonRpcRequestBody unsubscribeRequest =


same as above. rename to unsubscribeRequestBody

jframe · 2019-11-17T23:14:56Z

.../besu/ethereum/api/jsonrpc/websocket/subscription/request/SubscriptionRequestMapperTest.java

@@ -54,83 +55,88 @@ public void before() {

  @Test
  public void mapRequestToUnsubscribeRequest() {
-    final JsonRpcRequest jsonRpcRequest =
+    final JsonRpcRequestBody jsonRpcRequest =


nit: these request variable names should be jsonRpcRequestBody instead

jframe · 2019-11-17T23:16:25Z

...um/stratum/src/main/java/org/hyperledger/besu/ethereum/stratum/Stratum1EthProxyProtocol.java

-      if ("eth_getWork".equals(req.getMethod())) {
-        sendNewWork(conn, req.getId());
-      } else if ("eth_submitWork".equals(req.getMethod())) {
+      if ("eth_getWork".equals(req.getRequestBody().getMethod())) {


nit: extract req.getRequestBody().getMethod() to local var

think there's something wrong here - have changed this to use a JsonRequestBody ... the Json Deserialise was never going to work ... :/

jframe · 2019-11-17T23:16:44Z

ethereum/stratum/src/main/java/org/hyperledger/besu/ethereum/stratum/Stratum1Protocol.java

@@ -83,9 +83,9 @@ public Stratum1Protocol(final String extranonce) {

  @Override
  public boolean canHandle(final String initialMessage, final StratumConnection conn) {
-    JsonRpcRequest req;
+    JsonRpcRequestBody req;


rename req to reqBody

CjHare · 2019-11-18T00:37:31Z

...eum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/JsonRpcRequest.java

-  @JsonIgnore
-  public boolean isNotification() {
-    return isNotification;
+  public JsonRpcRequestBody getRequestBody() {


I'd suggest keeping the requestBody encapsulated and proving the request through to the requestBody (i.e mediator pattern)

public Object getId(){ return requestBody.getId(); }

move JsonRpcRequest to JsonRpcRequestContext, making this irrelevant (I think?) - ok?

CjHare

I'm going to suggest encapsulating the JsonRpcRequestBody in the JsonRpcRequest and add an identity of the JSON-RPC in the constructor (e.g. WebSockets)

CjHare

I'll recommend renaming JsonRpcRequestBody to JsonRpcRequest as it is still the JSON-RPC request as defined by the specification (https://www.jsonrpc.org/specification).

Otherwise it is looking good 👍

The context includes the user data of the logged in user making the json rpc request. Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

CjHare

Switching the request to a context is a worthy change, as it'll provide the necessary structure for future extensions to request authorisation.

CjHare

You'll want to take a look at the PrivGetTransactionReceiptTest (CI complains of a casting problem on compile)

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

…er#189) The context includes the user data of the logged in user making the json rpc request. Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

…er#189) The context includes the user data of the logged in user making the json rpc request. Signed-off-by: Trent Mohay <trent.mohay@consensys.net> Signed-off-by: edwardmack <ed@edwardmack.com>

rain-on requested review from jframe and CjHare November 15, 2019 06:20

jframe reviewed Nov 15, 2019

View reviewed changes

jframe reviewed Nov 17, 2019

View reviewed changes

CjHare reviewed Nov 18, 2019

View reviewed changes

CjHare reviewed Nov 19, 2019

View reviewed changes

rain-on force-pushed the identity_rpc branch from cae7e5f to 93cb6d2 Compare November 19, 2019 22:06

shemnon and others added 2 commits November 20, 2019 09:54

Replacing JsonRpcRequest with JsonRpcRequestContext

73833b0

The context includes the user data of the logged in user making the json rpc request. Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

Merge remote-tracking branch 'upstream/master' into identity_rpc

551712b

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

rain-on force-pushed the identity_rpc branch from 93cb6d2 to 551712b Compare November 19, 2019 22:55

CjHare approved these changes Nov 19, 2019

View reviewed changes

CjHare reviewed Nov 20, 2019

View reviewed changes

Fix broken test

f1aa54c

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

jframe approved these changes Nov 20, 2019

View reviewed changes

Merge branch 'master' into identity_rpc

83906e8

rain-on merged commit 732cfcc into hyperledger:master Nov 20, 2019

rain-on deleted the identity_rpc branch November 20, 2019 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass identity information into RpcMethod from Http Service #189

Pass identity information into RpcMethod from Http Service #189

rain-on commented Nov 15, 2019

jframe Nov 15, 2019

rain-on Nov 18, 2019

jframe Nov 15, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

jframe Nov 17, 2019

rain-on Nov 18, 2019

CjHare Nov 18, 2019

rain-on Nov 18, 2019

CjHare Nov 19, 2019

CjHare left a comment

CjHare left a comment •

edited

Loading

CjHare left a comment •

edited

Loading

CjHare left a comment

Pass identity information into RpcMethod from Http Service #189

Pass identity information into RpcMethod from Http Service #189

Conversation

rain-on commented Nov 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment

CjHare left a comment • edited Loading

Choose a reason for hiding this comment

CjHare left a comment • edited Loading

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment

CjHare left a comment •

edited

Loading

CjHare left a comment •

edited

Loading