Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[WIP] 3116 valid number rpc params #1901

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/
package tech.pegasys.pantheon.ethereum.jsonrpc.internal;

import tech.pegasys.pantheon.ethereum.jsonrpc.internal.exception.InvalidJsonRpcParameters;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.exception.InvalidJsonRpcRequestException;

import java.util.Arrays;
Expand Down Expand Up @@ -69,6 +70,15 @@ public Object[] getParams() {
return params;
}

@JsonIgnore
public void assertMaxLength(final int expectedLength) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest reworking the structure of this code a bit by:

  • Adding a method int getMaxAllowedParams() to the JsonRpcMethod interface
  • Adding a default method assertValidRequest(JsonRpcRequest request) to JsonRpcMethod that throws an InvalidJsonRcpParameters exception as you have here if there are too many params
  • Validating the request by calling jsonRpcMethod.assertValidRequest(request) before executing jsonRpcMethod.response(request) in JsonRpcHttpService

This will make it straightforward to consistently enforce this validation without expecting every method to run these checks manually. And it allows methods to override the default validation if necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! Definitely a better code structure, I will do my best to implement it.
Quick question : should I still manually add a tooManyParams test for every method, or should I just add one test for one method and "assume" that it will be correct for other methods ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems kind of painful to handcraft tests for every method. But if there was some way to "automatically" test each method, that would be cool. One idea:

  • Write a parameterized test (example) that runs for every method and constructs a request with 1 too many parameters, then verifies that the expected exception is thrown

Otherwise, I think we can probably get away with just adding a single test to JsonRpcHttpServiceTest.

final int length = getParamLength();
if (length > expectedLength) {
throw new InvalidJsonRpcParameters(
"Too many parameters, expected " + expectedLength + ", got " + length + " .");
}
}

@JsonIgnore
public boolean isNotification() {
return isNotification;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public String getName() {

@Override
protected BlockParameter blockParameter(final JsonRpcRequest request) {
request.assertMaxLength(1);
return getParameters().required(request.getParams(), 0, BlockParameter.class);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
if (request.getParamLength() != 3) {
return new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);
}
request.assertMaxLength(3);

final Address address = parameters.required(request.getParams(), 0, Address.class);
final String privateFrom = parameters.required(request.getParams(), 1, String.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
request.assertMaxLength(1);
LOG.trace("Executing {}", RpcMethod.EEA_GET_TRANSACTION_RECEIPT.getMethodName());
final Hash transactionHash = parameters.required(request.getParams(), 0, Hash.class);
final Optional<TransactionLocation> maybeLocation =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
if (request.getParamLength() != 1) {
return new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);
}
request.assertMaxLength(1);
final String rawPrivateTransaction = parameters.required(request.getParams(), 0, String.class);

final PrivateTransaction privateTransaction;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
request.assertMaxLength(3);
LOG.trace("Executing {}", RpcMethod.PRIV_CREATE_PRIVACY_GROUP.getMethodName());

final CreatePrivacyGroupParameter parameter =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {

request.assertMaxLength(1);
if (privacyEnabled) {
return new JsonRpcSuccessResponse(
request.getId(), Address.privacyPrecompiled(privacyAddress).toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
request.assertMaxLength(1);
LOG.trace("Executing {}", RpcMethod.PRIV_GET_PRIVATE_TRANSACTION.getMethodName());

final Hash hash = parameters.required(request.getParams(), 0, Hash.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.JsonRpcMethod;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcError;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.results.Quantity;
Expand All @@ -43,9 +41,7 @@ public String getName() {

@Override
public JsonRpcResponse response(final JsonRpcRequest request) {
if (request.getParamLength() != 2) {
return new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);
}
request.assertMaxLength(2);

final Address address = parameters.required(request.getParams(), 0, Address.class);
final String privacyGroupId = parameters.required(request.getParams(), 1, String.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.privacy.eea;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -91,4 +92,19 @@ public void nonceProviderThrowsAnExceptionProducesErrorResponse() {
assertThat(errorResponse.getError())
.isEqualTo(JsonRpcError.GET_PRIVATE_TRANSACTION_NONCE_ERROR);
}

@Test
public void tooManyParamsThrowsAnException() {
final long reportedNonce = 8L;
final EeaGetTransactionCount method =
new EeaGetTransactionCount(new JsonRpcParameter(), nonceProvider);

when(nonceProvider.determineNonce(privateFrom, privateFor, address)).thenReturn(reportedNonce);

final Object[] jsonBody = new Object[] {address.toString(), privateFrom, privateFor, "tooMany"};
final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("2.0", "eea_getTransactionCount", jsonBody);
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> method.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.privacy.methods.eea;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.nullable;
Expand Down Expand Up @@ -184,4 +185,61 @@ public void createsPrivateTransactionReceipt() throws Exception {

assertEquals("0x0bac79b78b9866ef11c989ad21a7fcf15f7a18d7", result.getContractAddress());
}

@Test
public void tooManyParamsThrowsException() {
final BytesValue mockBytesValue = mock(BytesValue.class);
final Block chainBlock = mock(Block.class);
final long mockLong = 10;
final BlockBody blockBody = mock(BlockBody.class);
final Transaction mockTx = mock(Transaction.class);
final BlockHeader mockBlockHeader = mock(BlockHeader.class);

final Log mockLog = mock(Log.class);
final Address mockAddress = mock(Address.class);
when(mockLog.getLogger()).thenReturn(mockAddress);
final LogTopic mockLogTopic = mock(LogTopic.class);
final List<LogTopic> listLogTopic = Arrays.asList(mockLogTopic);
when(mockLog.getTopics()).thenReturn(listLogTopic);
when(mockLog.getData()).thenReturn(mockBytesValue);
final List<Log> mockLogList = Arrays.asList(mockLog);
final PrivateTransactionStorage privateTransactionStorage =
mock(PrivateTransactionStorage.class);
final List<Transaction> mockListTx = Arrays.asList(mockTx, transaction);
final TransactionLocation transactionLocation = new TransactionLocation(mockBlockHash, 1);

doReturn(privateTransactionStorage).when(privacyParameters).getPrivateTransactionStorage();
when(privateTransactionStorage.getEvents(any(Bytes32.class)))
.thenReturn(Optional.of(mockLogList));
when(privateTransactionStorage.getOutput(any(Bytes32.class)))
.thenReturn(Optional.of(mockBytesValue));

final EeaGetTransactionReceipt eeaGetTransactionReceipt =
new EeaGetTransactionReceipt(blockchainQueries, enclave, parameters, privacyParameters);
final Object[] params = new Object[] {transaction.hash(), "tooManyParams"};
final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("1", "eea_getTransactionReceipt", params);

when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
when(blockchain.getTransactionByHash(transaction.hash())).thenReturn(Optional.of(transaction));
when(blockchain.getTransactionLocation(nullable(Hash.class)))
.thenReturn(Optional.of(transactionLocation));
final BytesValueRLPOutput bvrlp = new BytesValueRLPOutput();
privateTransaction.writeTo(bvrlp);
when(enclave.receive(any(ReceiveRequest.class)))
.thenReturn(
new ReceiveResponse(
Base64.getEncoder().encodeToString(bvrlp.encoded().extractArray()).getBytes(UTF_8),
""));

when(blockchain.getChainHeadBlock()).thenReturn(chainBlock);
when(chainBlock.getHash()).thenReturn(mockTransactionHash);
when(blockchainQueries.getBlockchain().getChainHeadBlockNumber()).thenReturn(mockLong);
when(blockBody.getTransactions()).thenReturn(mockListTx);
when(blockchain.getBlockHeader(mockBlockHash)).thenReturn(Optional.of(mockBlockHeader));
when(mockBlockHeader.getHash()).thenReturn(mockTransactionHash);
when(blockchain.getBlockBody(mockBlockHash)).thenReturn(Optional.of(blockBody));
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> eeaGetTransactionReceipt.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.privacy.methods.eea;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -125,37 +126,22 @@ public void requestIsMissingParameter() {
final JsonRpcRequest request =
new JsonRpcRequest("2.0", "eea_sendRawTransaction", new String[] {});

final JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);

final JsonRpcResponse actualResponse = method.response(request);

assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> method.response(request));
}

@Test
public void requestHasNullObjectParameter() {
final JsonRpcRequest request = new JsonRpcRequest("2.0", "eea_sendRawTransaction", null);

final JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);

final JsonRpcResponse actualResponse = method.response(request);

assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> method.response(request));
}

@Test
public void requestHasNullArrayParameter() {
final JsonRpcRequest request =
new JsonRpcRequest("2.0", "eea_sendRawTransaction", new String[] {null});

final JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS);

final JsonRpcResponse actualResponse = method.response(request);

assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> method.response(request));
}

@Test
Expand Down Expand Up @@ -386,4 +372,16 @@ private void verifyErrorForInvalidTransaction(
public void getMethodReturnsExpectedName() {
assertThat(method.getName()).matches("eea_sendRawTransaction");
}

@Test
public void tooManyParamsThrowsException() {

final Object[] params = new Object[] {VALID_PRIVATE_TRANSACTION_RLP, "tooManyParams"};

final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("2.0", "eea_sendRawTransaction", params);

assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> method.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.privacy.methods.priv;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.catchThrowableOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -246,4 +247,20 @@ public void returnsCorrectErrorEnclaveError() {

assertThat(result).isEqualTo(JsonRpcError.CREATE_PRIVACY_GROUP_ERROR);
}

@Test
public void tooManyParamsThrowsException() {

final PrivCreatePrivacyGroup privCreatePrivacyGroup =
new PrivCreatePrivacyGroup(enclave, privacyParameters, parameters);

final CreatePrivacyGroupParameter param =
new CreatePrivacyGroupParameter(ADDRESSES, NAME, DESCRIPTION);
final Object[] params = new Object[] {param, "tooManyParams"};

final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("1", "priv_createPrivacyGroup", params);
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> privCreatePrivacyGroup.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.privacy.methods.priv;

import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -66,4 +67,21 @@ public void verifyErrorPrivacyDisabled() {
assertEquals(JsonRpcResponseType.ERROR, response.getType());
assertEquals(JsonRpcError.PRIVACY_NOT_ENABLED, ((JsonRpcErrorResponse) response).getError());
}

@Test
public void tooManyParamsThrowsException() {
when(privacyParameters.getPrivacyAddress()).thenReturn(rawPrivacyAddress);
when(privacyParameters.isEnabled()).thenReturn(true);

final PrivGetPrivacyPrecompileAddress privGetPrivacyPrecompileAddress =
new PrivGetPrivacyPrecompileAddress(privacyParameters);

final Object[] params = new Object[] {new Object[0], "tooManyParams"};

final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("1", "priv_getPrivacyPrecompileAddress", params);

assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> privGetPrivacyPrecompileAddress.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -168,4 +169,21 @@ public void returnsPrivateTransactionGroup() throws Exception {

assertThat(result).isEqualToComparingFieldByField(privateTransactionGroupResult);
}

@Test
public void tooManyParamsThrowsException() {
when(blockchain.transactionByHash(any(Hash.class)))
.thenReturn(Optional.of(returnedTransaction));
when(returnedTransaction.getTransaction()).thenReturn(justTransaction);
when(justTransaction.getPayload()).thenReturn(BytesValues.fromBase64(""));

final PrivGetPrivateTransaction privGetPrivateTransaction =
new PrivGetPrivateTransaction(blockchain, enclave, parameters, privacyParameters);

final Object[] params = new Object[] {enclaveKey, "tooManyParams"};
final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("1", "priv_getPrivateTransaction", params);
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> privGetPrivateTransaction.response(tooManyParamsReq));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package tech.pegasys.pantheon.ethereum.jsonrpc.internal.privacy.methods.priv;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -54,4 +55,21 @@ public void verifyTransactionCount() {

assertEquals(String.format("0x%X", NONCE), response.getResult());
}

@Test
public void tooManyParamsThrowsException() {
final PrivateTransactionHandler privateTransactionHandler =
mock(PrivateTransactionHandler.class);
when(privateTransactionHandler.getSenderNonce(senderAddress, privacyGroupId)).thenReturn(NONCE);

final PrivGetTransactionCount privGetTransactionCount =
new PrivGetTransactionCount(parameters, privateTransactionHandler);

final Object[] params = new Object[] {senderAddress, privacyGroupId, "tooManyParams"};
final JsonRpcRequest tooManyParamsReq =
new JsonRpcRequest("1", "priv_getTransactionCount", params);

assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> privGetTransactionCount.response(tooManyParamsReq));
}
}
Loading