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

Commit

Permalink
Error response handling for permissions APIs (#736)
Browse files Browse the repository at this point in the history 
* Generifying account/node result object

* Added null/empty check on whitelist ops

* Updating successful whitelist operation to return Success instead of true

* Updating error messages

* Moving Success string into JsonRpcSuccessful response

* Updating unit tests
  • Loading branch information
@lucassaldanha
lucassaldanha authored Feb 1, 2019
1 parent bf0ff40 commit fd65738
Show file tree
Hide file tree
Showing 24 changed files with 305 additions and 148 deletions.
4 changes: 2 additions & 2 deletions ...asys/pantheon/tests/acceptance/dsl/condition/perm/AddAccountsToWhitelistSuccessfully.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public AddAccountsToWhitelistSuccessfully(

@Override
public void verify(final Node node) {
Boolean result = node.execute(transaction);
assertThat(result).isTrue();
String result = node.execute(transaction);
assertThat(result).isEqualTo("Success");
}
}
4 changes: 2 additions & 2 deletions ...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddNodeSuccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public AddNodeSuccess(final PermAddNodeTransaction transactions) {

@Override
public void verify(final Node node) {
final Boolean response = node.execute(transaction);
assertThat(response).isTrue();
final String response = node.execute(transaction);
assertThat(response).isEqualTo("Success");
}
}
4 changes: 2 additions & 2 deletions ...pantheon/tests/acceptance/dsl/condition/perm/RemoveAccountsFromWhitelistSuccessfully.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public RemoveAccountsFromWhitelistSuccessfully(

@Override
public void verify(final Node node) {
Boolean result = node.execute(transaction);
assertThat(result).isTrue();
String result = node.execute(transaction);
assertThat(result).isEqualTo("Success");
}
}
4 changes: 2 additions & 2 deletions ...est/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveNodeSuccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public RemoveNodeSuccess(final PermRemoveNodeTransaction transaction) {

@Override
public void verify(final Node node) {
final Boolean response = node.execute(transaction);
assertThat(response).isTrue();
final String response = node.execute(transaction);
assertThat(response).isEqualTo("Success");
}
}
8 changes: 4 additions & 4 deletions ...s/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/ResponseTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ public static class SignersBlockResponse extends Response<List<Address>> {}

public static class ProposalsResponse extends Response<Map<Address, Boolean>> {}

public static class AddAccountsToWhitelistResponse extends Response<Boolean> {}
public static class AddAccountsToWhitelistResponse extends Response<String> {}

public static class RemoveAccountsFromWhitelistResponse extends Response<Boolean> {}
public static class RemoveAccountsFromWhitelistResponse extends Response<String> {}

public static class GetAccountsWhitelistResponse extends Response<List<String>> {}

public static class AddNodeResponse extends Response<Boolean> {}
public static class AddNodeResponse extends Response<String> {}

public static class RemoveNodeResponse extends Response<Boolean> {}
public static class RemoveNodeResponse extends Response<String> {}

public static class GetNodesWhitelistResponse extends Response<List<String>> {}
}
6 changes: 3 additions & 3 deletions ...pantheon/tests/acceptance/dsl/transaction/perm/PermAddAccountsToWhitelistTransaction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.io.IOException;
import java.util.List;

public class PermAddAccountsToWhitelistTransaction implements Transaction<Boolean> {
public class PermAddAccountsToWhitelistTransaction implements Transaction<String> {

private final List<String> accounts;

Expand All @@ -30,10 +30,10 @@ public PermAddAccountsToWhitelistTransaction(final List<String> accounts) {
}

@Override
public Boolean execute(final JsonRequestFactories node) {
public String execute(final JsonRequestFactories node) {
try {
AddAccountsToWhitelistResponse response = node.perm().addAccountsToWhitelist(accounts).send();
assertThat(response.getResult()).isTrue();
assertThat(response.getResult()).isEqualTo("Success");
return response.getResult();
} catch (IOException e) {
throw new RuntimeException(e);
Expand Down
4 changes: 2 additions & 2 deletions ...a/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermAddNodeTransaction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@
import java.io.IOException;
import java.util.List;

public class PermAddNodeTransaction implements Transaction<Boolean> {
public class PermAddNodeTransaction implements Transaction<String> {
private final List<String> enodeList;

public PermAddNodeTransaction(final List<String> enodeList) {
this.enodeList = enodeList;
}

@Override
public Boolean execute(final JsonRequestFactories node) {
public String execute(final JsonRequestFactories node) {
try {
final AddNodeResponse result = node.perm().addNodesToWhitelist(enodeList).send();
assertThat(result).isNotNull();
Expand Down
6 changes: 3 additions & 3 deletions ...eon/tests/acceptance/dsl/transaction/perm/PermRemoveAccountsFromWhitelistTransaction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.io.IOException;
import java.util.List;

public class PermRemoveAccountsFromWhitelistTransaction implements Transaction<Boolean> {
public class PermRemoveAccountsFromWhitelistTransaction implements Transaction<String> {

private final List<String> accounts;

Expand All @@ -30,11 +30,11 @@ public PermRemoveAccountsFromWhitelistTransaction(final List<String> accounts) {
}

@Override
public Boolean execute(final JsonRequestFactories node) {
public String execute(final JsonRequestFactories node) {
try {
RemoveAccountsFromWhitelistResponse response =
node.perm().removeAccountsFromWhitelist(accounts).send();
assertThat(response.getResult()).isTrue();
assertThat(response.getResult()).isEqualTo("Success");
return response.getResult();
} catch (IOException e) {
throw new RuntimeException(e);
Expand Down
4 changes: 2 additions & 2 deletions ...ech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermRemoveNodeTransaction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@
import java.io.IOException;
import java.util.List;

public class PermRemoveNodeTransaction implements Transaction<Boolean> {
public class PermRemoveNodeTransaction implements Transaction<String> {
private final List<String> enodeList;

public PermRemoveNodeTransaction(final List<String> enodeList) {
this.enodeList = enodeList;
}

@Override
public Boolean execute(final JsonRequestFactories node) {
public String execute(final JsonRequestFactories node) {
try {
final RemoveNodeResponse result = node.perm().removeNodesFromWhitelist(enodeList).send();
assertThat(result).isNotNull();
Expand Down
9 changes: 6 additions & 3 deletions .../pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelist.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController.AddResult;
import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult;

import java.util.List;

Expand All @@ -44,9 +44,12 @@ public String getName() {
@SuppressWarnings("unchecked")
public JsonRpcResponse response(final JsonRpcRequest request) {
final List<String> accountsList = parameters.required(request.getParams(), 0, List.class);
final AddResult addResult = whitelistController.addAccounts(accountsList);
final WhitelistOperationResult addResult = whitelistController.addAccounts(accountsList);

switch (addResult) {
case ERROR_EMPTY_ENTRY:
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_EMPTY_ENTRY);
case ERROR_INVALID_ENTRY:
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_INVALID_ENTRY);
Expand All @@ -57,7 +60,7 @@ public JsonRpcResponse response(final JsonRpcRequest request) {
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_DUPLICATED_ENTRY);
case SUCCESS:
return new JsonRpcSuccessResponse(request.getId(), true);
return new JsonRpcSuccessResponse(request.getId());
default:
throw new IllegalStateException("Unmapped result from AccountWhitelistController");
}
Expand Down
4 changes: 3 additions & 1 deletion ...sys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelist.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ public JsonRpcResponse response(final JsonRpcRequest req) {

switch (nodesWhitelistResult.result()) {
case SUCCESS:
return new JsonRpcSuccessResponse(req.getId(), true);
return new JsonRpcSuccessResponse(req.getId());
case ERROR_EMPTY_ENTRY:
return new JsonRpcErrorResponse(req.getId(), JsonRpcError.NODE_WHITELIST_EMPTY_ENTRY);
case ERROR_EXISTING_ENTRY:
return new JsonRpcErrorResponse(req.getId(), JsonRpcError.NODE_WHITELIST_EXISTING_ENTRY);
case ERROR_DUPLICATED_ENTRY:
Expand Down
9 changes: 6 additions & 3 deletions ...heon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelist.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController.RemoveResult;
import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult;

import java.util.List;

Expand All @@ -44,9 +44,12 @@ public String getName() {
@SuppressWarnings("unchecked")
public JsonRpcResponse response(final JsonRpcRequest request) {
final List<String> accountsList = parameters.required(request.getParams(), 0, List.class);
final RemoveResult removeResult = whitelistController.removeAccounts(accountsList);
final WhitelistOperationResult removeResult = whitelistController.removeAccounts(accountsList);

switch (removeResult) {
case ERROR_EMPTY_ENTRY:
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_EMPTY_ENTRY);
case ERROR_INVALID_ENTRY:
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_INVALID_ENTRY);
Expand All @@ -57,7 +60,7 @@ public JsonRpcResponse response(final JsonRpcRequest request) {
return new JsonRpcErrorResponse(
request.getId(), JsonRpcError.ACCOUNT_WHITELIST_DUPLICATED_ENTRY);
case SUCCESS:
return new JsonRpcSuccessResponse(request.getId(), true);
return new JsonRpcSuccessResponse(request.getId());
default:
throw new IllegalStateException("Unmapped result from AccountWhitelistController");
}
Expand Down
4 changes: 3 additions & 1 deletion ...antheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelist.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ public JsonRpcResponse response(final JsonRpcRequest req) {

switch (nodesWhitelistResult.result()) {
case SUCCESS:
return new JsonRpcSuccessResponse(req.getId(), true);
return new JsonRpcSuccessResponse(req.getId());
case ERROR_EMPTY_ENTRY:
return new JsonRpcErrorResponse(req.getId(), JsonRpcError.NODE_WHITELIST_EMPTY_ENTRY);
case ERROR_ABSENT_ENTRY:
return new JsonRpcErrorResponse(req.getId(), JsonRpcError.NODE_WHITELIST_MISSING_ENTRY);
case ERROR_DUPLICATED_ENTRY:
Expand Down
21 changes: 11 additions & 10 deletions .../src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,21 @@ public enum JsonRpcError {
COINBASE_NOT_SPECIFIED(-32000, "Coinbase must be explicitly specified"),

// Permissioning errors
ACCOUNT_WHITELIST_NOT_SET(-32000, "Account whitelist hasn't been set"),
ACCOUNT_WHITELIST_DUPLICATED_ENTRY(-32000, "Request can't contain duplicated entries"),
ACCOUNT_WHITELIST_EXISTING_ENTRY(-32000, "Can't add existing account to whitelist"),
ACCOUNT_WHITELIST_ABSENT_ENTRY(-32000, "Can't remove absent account from whitelist"),
ACCOUNT_WHITELIST_INVALID_ENTRY(-32000, "Can't add invalid account address to the whitelist"),
ACCOUNT_WHITELIST_NOT_SET(-32000, "Account whitelist has not been set"),
ACCOUNT_WHITELIST_EMPTY_ENTRY(-32000, "Request contains an empty list of accounts"),
ACCOUNT_WHITELIST_INVALID_ENTRY(-32000, "Request contains an invalid account"),
ACCOUNT_WHITELIST_DUPLICATED_ENTRY(-32000, "Request contains duplicate accounts"),
ACCOUNT_WHITELIST_EXISTING_ENTRY(-32000, "Cannot add an existing account to whitelist"),
ACCOUNT_WHITELIST_ABSENT_ENTRY(-32000, "Cannot remove an absent account from whitelist"),

// Node whitelist errors
NODE_WHITELIST_NOT_SET(-32000, "Node whitelist has not been set"),
NODE_WHITELIST_DUPLICATED_ENTRY(-32000, "Request can't contain duplicated node entries"),
NODE_WHITELIST_EXISTING_ENTRY(-32000, "Node whitelist can't contain duplicated node entries"),
NODE_WHITELIST_MISSING_ENTRY(-32000, "Node whitelist does not contain a specified node"),
NODE_WHITELIST_INVALID_ENTRY(-32000, "Unable to add invalid node to the node whitelist"),
NODE_WHITELIST_EMPTY_ENTRY(-32000, "Request contains an empty list of nodes"),
NODE_WHITELIST_INVALID_ENTRY(-32000, "Request contains an invalid node"),
NODE_WHITELIST_DUPLICATED_ENTRY(-32000, "Request contains duplicate nodes"),
NODE_WHITELIST_EXISTING_ENTRY(-32000, "Cannot add an existing node to whitelist"),
NODE_WHITELIST_MISSING_ENTRY(-32000, "Cannot remove an absent node from whitelist"),

// Private transaction errors
ENCLAVE_IS_DOWN(-32000, "Enclave is down");

private final int code;
Expand Down
5 changes: 5 additions & 0 deletions ...java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcSuccessResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public JsonRpcSuccessResponse(final Object id, final Object result) {
this.result = result;
}

public JsonRpcSuccessResponse(final Object id) {
this.id = id;
this.result = "Success";
}

@JsonGetter("id")
public Object getId() {
return id;
Expand Down
30 changes: 23 additions & 7 deletions ...theon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelistTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController;
import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController.AddResult;
import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -55,10 +55,10 @@ public void getNameShouldReturnExpectedName() {
}

@Test
public void whenAccountsAreAddedToWhitelistShouldReturnTrue() {
public void whenAccountsAreAddedToWhitelistShouldReturnSuccess() {
List<String> accounts = Arrays.asList("0x0", "0x1");
JsonRpcResponse expectedResponse = new JsonRpcSuccessResponse(null, true);
when(accountWhitelist.addAccounts(eq(accounts))).thenReturn(AddResult.SUCCESS);
JsonRpcResponse expectedResponse = new JsonRpcSuccessResponse(null);
when(accountWhitelist.addAccounts(eq(accounts))).thenReturn(WhitelistOperationResult.SUCCESS);

JsonRpcResponse actualResponse = method.response(request(accounts));

Expand All @@ -69,7 +69,8 @@ public void whenAccountsAreAddedToWhitelistShouldReturnTrue() {
public void whenAccountIsInvalidShouldReturnInvalidAccountErrorResponse() {
JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_INVALID_ENTRY);
when(accountWhitelist.addAccounts(any())).thenReturn(AddResult.ERROR_INVALID_ENTRY);
when(accountWhitelist.addAccounts(any()))
.thenReturn(WhitelistOperationResult.ERROR_INVALID_ENTRY);

JsonRpcResponse actualResponse = method.response(request(new ArrayList<>()));

Expand All @@ -80,7 +81,8 @@ public void whenAccountIsInvalidShouldReturnInvalidAccountErrorResponse() {
public void whenAccountExistsShouldReturnExistingEntryErrorResponse() {
JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_EXISTING_ENTRY);
when(accountWhitelist.addAccounts(any())).thenReturn(AddResult.ERROR_EXISTING_ENTRY);
when(accountWhitelist.addAccounts(any()))
.thenReturn(WhitelistOperationResult.ERROR_EXISTING_ENTRY);

JsonRpcResponse actualResponse = method.response(request(new ArrayList<>()));

Expand All @@ -91,7 +93,21 @@ public void whenAccountExistsShouldReturnExistingEntryErrorResponse() {
public void whenInputHasDuplicatedAccountsShouldReturnDuplicatedEntryErrorResponse() {
JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_DUPLICATED_ENTRY);
when(accountWhitelist.addAccounts(any())).thenReturn(AddResult.ERROR_DUPLICATED_ENTRY);
when(accountWhitelist.addAccounts(any()))
.thenReturn(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY);

JsonRpcResponse actualResponse = method.response(request(new ArrayList<>()));

assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
}

@Test
public void whenEmptyListOnRequestShouldReturnEmptyEntryErrorResponse() {
JsonRpcResponse expectedResponse =
new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_EMPTY_ENTRY);

when(accountWhitelist.addAccounts(eq(new ArrayList<>())))
.thenReturn(WhitelistOperationResult.ERROR_EMPTY_ENTRY);

JsonRpcResponse actualResponse = method.response(request(new ArrayList<>()));

Expand Down
Loading

0 comments on commit fd65738

Please sign in to comment.