From 4a32c7226881ae2d2b2e9bae9d96e2731f3d2bd8 Mon Sep 17 00:00:00 2001 From: Lucas Saldanha Date: Fri, 1 Feb 2019 16:29:17 +1300 Subject: [PATCH] Error response handling for permissions APIs (#736) * 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 Signed-off-by: Adrian Sutton --- .../AddAccountsToWhitelistSuccessfully.java | 4 +- .../dsl/condition/perm/AddNodeSuccess.java | 4 +- ...moveAccountsFromWhitelistSuccessfully.java | 4 +- .../dsl/condition/perm/RemoveNodeSuccess.java | 4 +- .../dsl/transaction/ResponseTypes.java | 8 +-- ...PermAddAccountsToWhitelistTransaction.java | 6 +- .../perm/PermAddNodeTransaction.java | 4 +- ...emoveAccountsFromWhitelistTransaction.java | 6 +- .../perm/PermRemoveNodeTransaction.java | 4 +- .../PermAddAccountsToWhitelist.java | 9 ++- .../PermAddNodesToWhitelist.java | 4 +- .../PermRemoveAccountsFromWhitelist.java | 9 ++- .../PermRemoveNodesFromWhitelist.java | 4 +- .../internal/response/JsonRpcError.java | 21 +++--- .../response/JsonRpcSuccessResponse.java | 5 ++ .../PermAddAccountsToWhitelistTest.java | 30 +++++++-- .../PermAddNodesToWhitelistTest.java | 30 +++++++-- .../PermRemoveAccountsFromWhitelistTest.java | 31 +++++++-- .../PermRemoveNodesFromWhitelistTest.java | 30 +++++++-- .../NodeWhitelistController.java | 53 ++++++++------- .../NodeWhitelistControllerTest.java | 47 ++++++++++++-- .../AccountWhitelistController.java | 65 +++++++++---------- .../WhitelistOperationResult.java | 22 +++++++ .../AccountWhitelistControllerTest.java | 49 ++++++++------ 24 files changed, 305 insertions(+), 148 deletions(-) create mode 100644 ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/WhitelistOperationResult.java diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddAccountsToWhitelistSuccessfully.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddAccountsToWhitelistSuccessfully.java index 6e7252e194..793b60fddd 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddAccountsToWhitelistSuccessfully.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddAccountsToWhitelistSuccessfully.java @@ -29,7 +29,7 @@ public class AddAccountsToWhitelistSuccessfully implements Condition { @Override public void verify(final Node node) { - Boolean result = node.execute(transaction); - assertThat(result).isTrue(); + String result = node.execute(transaction); + assertThat(result).isEqualTo("Success"); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddNodeSuccess.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddNodeSuccess.java index f3a92c6266..b270c9cc9f 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddNodeSuccess.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/AddNodeSuccess.java @@ -28,7 +28,7 @@ public class AddNodeSuccess implements Condition { @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"); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveAccountsFromWhitelistSuccessfully.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveAccountsFromWhitelistSuccessfully.java index 6001f2ac56..145edb804d 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveAccountsFromWhitelistSuccessfully.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveAccountsFromWhitelistSuccessfully.java @@ -29,7 +29,7 @@ public class RemoveAccountsFromWhitelistSuccessfully implements Condition { @Override public void verify(final Node node) { - Boolean result = node.execute(transaction); - assertThat(result).isTrue(); + String result = node.execute(transaction); + assertThat(result).isEqualTo("Success"); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveNodeSuccess.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveNodeSuccess.java index 0b8856888c..3038611066 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveNodeSuccess.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/perm/RemoveNodeSuccess.java @@ -28,7 +28,7 @@ public class RemoveNodeSuccess implements Condition { @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"); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/ResponseTypes.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/ResponseTypes.java index 6df0e623e9..b810e266ff 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/ResponseTypes.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/ResponseTypes.java @@ -28,15 +28,15 @@ public class ResponseTypes { public static class ProposalsResponse extends Response> {} - public static class AddAccountsToWhitelistResponse extends Response {} + public static class AddAccountsToWhitelistResponse extends Response {} - public static class RemoveAccountsFromWhitelistResponse extends Response {} + public static class RemoveAccountsFromWhitelistResponse extends Response {} public static class GetAccountsWhitelistResponse extends Response> {} - public static class AddNodeResponse extends Response {} + public static class AddNodeResponse extends Response {} - public static class RemoveNodeResponse extends Response {} + public static class RemoveNodeResponse extends Response {} public static class GetNodesWhitelistResponse extends Response> {} } diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermAddAccountsToWhitelistTransaction.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermAddAccountsToWhitelistTransaction.java index 313d9cd981..85ff519458 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermAddAccountsToWhitelistTransaction.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermAddAccountsToWhitelistTransaction.java @@ -21,7 +21,7 @@ import tech.pegasys.pantheon.tests.acceptance.dsl.transaction.Transaction; import java.io.IOException; import java.util.List; -public class PermAddAccountsToWhitelistTransaction implements Transaction { +public class PermAddAccountsToWhitelistTransaction implements Transaction { private final List accounts; @@ -30,10 +30,10 @@ public class PermAddAccountsToWhitelistTransaction implements Transaction { +public class PermAddNodeTransaction implements Transaction { private final List enodeList; public PermAddNodeTransaction(final List enodeList) { @@ -29,7 +29,7 @@ public class PermAddNodeTransaction implements Transaction { } @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(); diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermRemoveAccountsFromWhitelistTransaction.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermRemoveAccountsFromWhitelistTransaction.java index e62a58a636..2dda2933cf 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermRemoveAccountsFromWhitelistTransaction.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/perm/PermRemoveAccountsFromWhitelistTransaction.java @@ -21,7 +21,7 @@ import tech.pegasys.pantheon.tests.acceptance.dsl.transaction.Transaction; import java.io.IOException; import java.util.List; -public class PermRemoveAccountsFromWhitelistTransaction implements Transaction { +public class PermRemoveAccountsFromWhitelistTransaction implements Transaction { private final List accounts; @@ -30,11 +30,11 @@ public class PermRemoveAccountsFromWhitelistTransaction implements Transaction { +public class PermRemoveNodeTransaction implements Transaction { private final List enodeList; public PermRemoveNodeTransaction(final List enodeList) { @@ -29,7 +29,7 @@ public class PermRemoveNodeTransaction implements Transaction { } @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(); diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelist.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelist.java index 496a8d148e..0b6f844c9b 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelist.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelist.java @@ -20,7 +20,7 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResp 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; @@ -44,9 +44,12 @@ public class PermAddAccountsToWhitelist implements JsonRpcMethod { @SuppressWarnings("unchecked") public JsonRpcResponse response(final JsonRpcRequest request) { final List 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); @@ -57,7 +60,7 @@ public class PermAddAccountsToWhitelist implements JsonRpcMethod { 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"); } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelist.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelist.java index f178546728..c7f14a5262 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelist.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelist.java @@ -61,7 +61,9 @@ public class PermAddNodesToWhitelist implements JsonRpcMethod { 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: diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelist.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelist.java index 04e923a985..9292466265 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelist.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelist.java @@ -20,7 +20,7 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResp 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; @@ -44,9 +44,12 @@ public class PermRemoveAccountsFromWhitelist implements JsonRpcMethod { @SuppressWarnings("unchecked") public JsonRpcResponse response(final JsonRpcRequest request) { final List 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); @@ -57,7 +60,7 @@ public class PermRemoveAccountsFromWhitelist implements JsonRpcMethod { 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"); } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelist.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelist.java index 0bfc5a4412..fc0cd5a032 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelist.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelist.java @@ -62,7 +62,9 @@ public class PermRemoveNodesFromWhitelist implements JsonRpcMethod { 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: diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java index c70b4d9117..3b8ea99adc 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java @@ -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; diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcSuccessResponse.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcSuccessResponse.java index b5b075a1e4..35e9f29b09 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcSuccessResponse.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcSuccessResponse.java @@ -28,6 +28,11 @@ public class JsonRpcSuccessResponse implements JsonRpcResponse { this.result = result; } + public JsonRpcSuccessResponse(final Object id) { + this.id = id; + this.result = "Success"; + } + @JsonGetter("id") public Object getId() { return id; diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelistTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelistTest.java index 40e5104f85..0beeb30d97 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelistTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddAccountsToWhitelistTest.java @@ -26,7 +26,7 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResp 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; @@ -55,10 +55,10 @@ public class PermAddAccountsToWhitelistTest { } @Test - public void whenAccountsAreAddedToWhitelistShouldReturnTrue() { + public void whenAccountsAreAddedToWhitelistShouldReturnSuccess() { List 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)); @@ -69,7 +69,8 @@ public class PermAddAccountsToWhitelistTest { 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<>())); @@ -80,7 +81,8 @@ public class PermAddAccountsToWhitelistTest { 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<>())); @@ -91,7 +93,21 @@ public class PermAddAccountsToWhitelistTest { 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<>())); diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelistTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelistTest.java index 7c5df8c1b5..b0280c4aec 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelistTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermAddNodesToWhitelistTest.java @@ -14,12 +14,12 @@ package tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.permissioning; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResult; -import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResultType; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter; @@ -30,7 +30,9 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessRe import tech.pegasys.pantheon.ethereum.p2p.P2pDisabledException; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController; +import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult; +import java.util.ArrayList; import java.util.List; import org.assertj.core.util.Lists; @@ -43,7 +45,6 @@ import org.mockito.junit.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class PermAddNodesToWhitelistTest { - private static final boolean JSON_SUCCESS = true; private PermAddNodesToWhitelist method; private static final String METHOD_NAME = "perm_addNodesToWhitelist"; @@ -108,7 +109,22 @@ public class PermAddNodesToWhitelistTest { when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.addNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY)); + + final JsonRpcResponse actual = method.response(request); + + assertThat(actual).isEqualToComparingFieldByFieldRecursively(expected); + } + + @Test + public void whenRequestContainsEmptyListOfNodesShouldReturnEmptyEntryError() { + final JsonRpcRequest request = buildRequest(new ArrayList<>()); + final JsonRpcResponse expected = + new JsonRpcErrorResponse(request.getId(), JsonRpcError.NODE_WHITELIST_EMPTY_ENTRY); + + when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); + when(nodeWhitelistController.addNodes(eq(new ArrayList<>()))) + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY)); final JsonRpcResponse actual = method.response(request); @@ -118,11 +134,11 @@ public class PermAddNodesToWhitelistTest { @Test public void shouldAddSingleValidNode() { final JsonRpcRequest request = buildRequest(Lists.newArrayList(enode1)); - final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId(), JSON_SUCCESS); + final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId()); when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.addNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.SUCCESS)); final JsonRpcResponse actual = method.response(request); @@ -135,11 +151,11 @@ public class PermAddNodesToWhitelistTest { @Test public void shouldAddMultipleValidNodes() { final JsonRpcRequest request = buildRequest(Lists.newArrayList(enode1, enode2, enode3)); - final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId(), JSON_SUCCESS); + final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId()); when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.addNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.SUCCESS)); final JsonRpcResponse actual = method.response(request); diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelistTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelistTest.java index c6f21fbd0d..a0f3bb401a 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelistTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveAccountsFromWhitelistTest.java @@ -26,7 +26,7 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResp 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.ArrayList; import java.util.Arrays; @@ -55,10 +55,11 @@ public class PermRemoveAccountsFromWhitelistTest { } @Test - public void whenAccountsAreRemovedFromWhitelistShouldReturnTrue() { + public void whenAccountsAreRemovedFromWhitelistShouldReturnSuccess() { List accounts = Arrays.asList("0x0", "0x1"); - JsonRpcResponse expectedResponse = new JsonRpcSuccessResponse(null, true); - when(accountWhitelist.removeAccounts(eq(accounts))).thenReturn(RemoveResult.SUCCESS); + JsonRpcResponse expectedResponse = new JsonRpcSuccessResponse(null); + when(accountWhitelist.removeAccounts(eq(accounts))) + .thenReturn(WhitelistOperationResult.SUCCESS); JsonRpcResponse actualResponse = method.response(request(accounts)); @@ -69,7 +70,8 @@ public class PermRemoveAccountsFromWhitelistTest { public void whenAccountIsInvalidShouldReturnInvalidAccountErrorResponse() { JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_INVALID_ENTRY); - when(accountWhitelist.removeAccounts(any())).thenReturn(RemoveResult.ERROR_INVALID_ENTRY); + when(accountWhitelist.removeAccounts(any())) + .thenReturn(WhitelistOperationResult.ERROR_INVALID_ENTRY); JsonRpcResponse actualResponse = method.response(request(new ArrayList<>())); @@ -80,7 +82,8 @@ public class PermRemoveAccountsFromWhitelistTest { public void whenAccountIsAbsentShouldReturnAbsentAccountErrorResponse() { JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_ABSENT_ENTRY); - when(accountWhitelist.removeAccounts(any())).thenReturn(RemoveResult.ERROR_ABSENT_ENTRY); + when(accountWhitelist.removeAccounts(any())) + .thenReturn(WhitelistOperationResult.ERROR_ABSENT_ENTRY); JsonRpcResponse actualResponse = method.response(request(new ArrayList<>())); @@ -91,7 +94,21 @@ public class PermRemoveAccountsFromWhitelistTest { public void whenInputHasDuplicatedAccountsShouldReturnDuplicatedEntryErrorResponse() { JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, JsonRpcError.ACCOUNT_WHITELIST_DUPLICATED_ENTRY); - when(accountWhitelist.removeAccounts(any())).thenReturn(RemoveResult.ERROR_DUPLICATED_ENTRY); + when(accountWhitelist.removeAccounts(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.removeAccounts(eq(new ArrayList<>()))) + .thenReturn(WhitelistOperationResult.ERROR_EMPTY_ENTRY); JsonRpcResponse actualResponse = method.response(request(new ArrayList<>())); diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelistTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelistTest.java index 8c04ae9386..b36089cdeb 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelistTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/permissioning/PermRemoveNodesFromWhitelistTest.java @@ -14,12 +14,12 @@ package tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.permissioning; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResult; -import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResultType; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter; @@ -30,7 +30,9 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessRe import tech.pegasys.pantheon.ethereum.p2p.P2pDisabledException; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController; +import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult; +import java.util.ArrayList; import java.util.List; import org.assertj.core.util.Lists; @@ -43,7 +45,6 @@ import org.mockito.junit.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class PermRemoveNodesFromWhitelistTest { - private static final boolean SUCCESS_RESULT = true; private PermRemoveNodesFromWhitelist method; private static final String METHOD_NAME = "perm_removeNodesFromWhitelist"; @@ -93,11 +94,11 @@ public class PermRemoveNodesFromWhitelistTest { @Test public void shouldRemoveSingleValidNode() { final JsonRpcRequest request = buildRequest(Lists.newArrayList(enode1)); - final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId(), SUCCESS_RESULT); + final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId()); when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.removeNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.SUCCESS)); final JsonRpcResponse actual = method.response(request); @@ -110,11 +111,11 @@ public class PermRemoveNodesFromWhitelistTest { @Test public void shouldRemoveMultipleValidNodes() { final JsonRpcRequest request = buildRequest(Lists.newArrayList(enode1, enode2, enode3)); - final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId(), SUCCESS_RESULT); + final JsonRpcResponse expected = new JsonRpcSuccessResponse(request.getId()); when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.removeNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.SUCCESS)); final JsonRpcResponse actual = method.response(request); @@ -144,7 +145,22 @@ public class PermRemoveNodesFromWhitelistTest { when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); when(nodeWhitelistController.removeNodes(any())) - .thenReturn(new NodesWhitelistResult(NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY)); + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY)); + + final JsonRpcResponse actual = method.response(request); + + assertThat(actual).isEqualToComparingFieldByFieldRecursively(expected); + } + + @Test + public void whenRequestContainsEmptyListOfNodesShouldReturnEmptyEntryError() { + final JsonRpcRequest request = buildRequest(new ArrayList<>()); + final JsonRpcResponse expected = + new JsonRpcErrorResponse(request.getId(), JsonRpcError.NODE_WHITELIST_EMPTY_ENTRY); + + when(p2pNetwork.getNodeWhitelistController()).thenReturn(nodeWhitelistController); + when(nodeWhitelistController.removeNodes(eq(new ArrayList<>()))) + .thenReturn(new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY)); final JsonRpcResponse actual = method.response(request); diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java index 133c8cdaf9..d88fe5f6af 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java @@ -15,6 +15,7 @@ package tech.pegasys.pantheon.ethereum.p2p.permissioning; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.permissioning.PermissioningConfiguration; +import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult; import java.net.URI; import java.util.ArrayList; @@ -48,21 +49,20 @@ public class NodeWhitelistController { } public NodesWhitelistResult addNodes(final List peers) { - if (peerListHasDuplicates(peers)) { - return new NodesWhitelistResult( - NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY, - String.format("Specified peer list contains duplicates")); + final NodesWhitelistResult inputValidationResult = validInput(peers); + if (inputValidationResult.result() != WhitelistOperationResult.SUCCESS) { + return inputValidationResult; } for (DefaultPeer peer : peers) { if (nodesWhitelist.contains(peer)) { return new NodesWhitelistResult( - NodesWhitelistResultType.ERROR_EXISTING_ENTRY, + WhitelistOperationResult.ERROR_EXISTING_ENTRY, String.format("Specified peer: %s already exists in whitelist.", peer.getId())); } } peers.forEach(this::addNode); - return new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS); + return new NodesWhitelistResult(WhitelistOperationResult.SUCCESS); } private boolean peerListHasDuplicates(final List peers) { @@ -70,21 +70,35 @@ public class NodeWhitelistController { } public NodesWhitelistResult removeNodes(final List peers) { - if (peerListHasDuplicates(peers)) { - return new NodesWhitelistResult( - NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY, - String.format("Specified peer list contains duplicates")); + final NodesWhitelistResult inputValidationResult = validInput(peers); + if (inputValidationResult.result() != WhitelistOperationResult.SUCCESS) { + return inputValidationResult; } for (DefaultPeer peer : peers) { if (!(nodesWhitelist.contains(peer))) { return new NodesWhitelistResult( - NodesWhitelistResultType.ERROR_ABSENT_ENTRY, + WhitelistOperationResult.ERROR_ABSENT_ENTRY, String.format("Specified peer: %s does not exist in whitelist.", peer.getId())); } } peers.forEach(this::removeNode); - return new NodesWhitelistResult(NodesWhitelistResultType.SUCCESS); + return new NodesWhitelistResult(WhitelistOperationResult.SUCCESS); + } + + private NodesWhitelistResult validInput(final List peers) { + if (peers == null || peers.isEmpty()) { + return new NodesWhitelistResult( + WhitelistOperationResult.ERROR_EMPTY_ENTRY, String.format("Null/empty peers list")); + } + + if (peerListHasDuplicates(peers)) { + return new NodesWhitelistResult( + WhitelistOperationResult.ERROR_DUPLICATED_ENTRY, + String.format("Specified peer list contains duplicates")); + } + + return new NodesWhitelistResult(WhitelistOperationResult.SUCCESS); } public boolean isPermitted(final Peer node) { @@ -100,21 +114,21 @@ public class NodeWhitelistController { } public static class NodesWhitelistResult { - private final NodesWhitelistResultType result; + private final WhitelistOperationResult result; private final Optional message; - NodesWhitelistResult(final NodesWhitelistResultType fail, final String message) { + NodesWhitelistResult(final WhitelistOperationResult fail, final String message) { this.result = fail; this.message = Optional.of(message); } @VisibleForTesting - public NodesWhitelistResult(final NodesWhitelistResultType success) { + public NodesWhitelistResult(final WhitelistOperationResult success) { this.result = success; this.message = Optional.empty(); } - public NodesWhitelistResultType result() { + public WhitelistOperationResult result() { return result; } @@ -126,11 +140,4 @@ public class NodeWhitelistController { public boolean contains(final Peer node) { return (!nodeWhitelistSet || (nodesWhitelist.contains(node))); } - - public enum NodesWhitelistResultType { - SUCCESS, - ERROR_DUPLICATED_ENTRY, - ERROR_EXISTING_ENTRY, - ERROR_ABSENT_ENTRY - } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java index 1db270a142..fe9311842c 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java @@ -14,11 +14,12 @@ package tech.pegasys.pantheon.ethereum.p2p.permissioning; import static org.assertj.core.api.Assertions.assertThat; import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResult; -import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResultType; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.permissioning.PermissioningConfiguration; +import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult; +import java.util.ArrayList; import java.util.Arrays; import com.google.common.collect.Lists; @@ -47,7 +48,7 @@ public class NodeWhitelistControllerTest { controller.addNode(DefaultPeer.fromURI(enode1)); NodesWhitelistResult expected = - new NodesWhitelistResult(NodesWhitelistResultType.ERROR_EXISTING_ENTRY); + new NodesWhitelistResult(WhitelistOperationResult.ERROR_EXISTING_ENTRY); NodesWhitelistResult actualResult = controller.addNodes( Lists.newArrayList(DefaultPeer.fromURI(enode1), DefaultPeer.fromURI(enode2))); @@ -58,7 +59,7 @@ public class NodeWhitelistControllerTest { @Test public void whenAddNodesInputHasDuplicatedNodesShouldReturnDuplicatedEntryError() { NodesWhitelistResult expected = - new NodesWhitelistResult(NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY); + new NodesWhitelistResult(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY); NodesWhitelistResult actualResult = controller.addNodes( @@ -67,10 +68,28 @@ public class NodeWhitelistControllerTest { assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); } + @Test + public void whenAddNodesInputHasEmptyListOfNodesShouldReturnErrorEmptyEntry() { + NodesWhitelistResult expected = + new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY); + NodesWhitelistResult actualResult = controller.removeNodes(new ArrayList<>()); + + assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); + } + + @Test + public void whenAddNodesInputHasNullListOfNodesShouldReturnErrorEmptyEntry() { + NodesWhitelistResult expected = + new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY); + NodesWhitelistResult actualResult = controller.removeNodes(null); + + assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); + } + @Test public void whenRemoveNodesInputHasAbsentNodeShouldReturnRemoveErrorAbsentEntry() { NodesWhitelistResult expected = - new NodesWhitelistResult(NodesWhitelistResultType.ERROR_ABSENT_ENTRY); + new NodesWhitelistResult(WhitelistOperationResult.ERROR_ABSENT_ENTRY); NodesWhitelistResult actualResult = controller.removeNodes( Lists.newArrayList(DefaultPeer.fromURI(enode1), DefaultPeer.fromURI(enode2))); @@ -81,11 +100,29 @@ public class NodeWhitelistControllerTest { @Test public void whenRemoveNodesInputHasDuplicateNodesShouldReturnErrorDuplicatedEntry() { NodesWhitelistResult expected = - new NodesWhitelistResult(NodesWhitelistResultType.ERROR_DUPLICATED_ENTRY); + new NodesWhitelistResult(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY); NodesWhitelistResult actualResult = controller.removeNodes( Lists.newArrayList(DefaultPeer.fromURI(enode1), DefaultPeer.fromURI(enode1))); assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); } + + @Test + public void whenRemoveNodesInputHasEmptyListOfNodesShouldReturnErrorEmptyEntry() { + NodesWhitelistResult expected = + new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY); + NodesWhitelistResult actualResult = controller.removeNodes(new ArrayList<>()); + + assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); + } + + @Test + public void whenRemoveNodesInputHasNullListOfNodesShouldReturnErrorEmptyEntry() { + NodesWhitelistResult expected = + new NodesWhitelistResult(WhitelistOperationResult.ERROR_EMPTY_ENTRY); + NodesWhitelistResult actualResult = controller.removeNodes(null); + + assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); + } } diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistController.java index 4067c0720d..7a919f0da7 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistController.java @@ -26,44 +26,57 @@ public class AccountWhitelistController { public AccountWhitelistController(final PermissioningConfiguration configuration) { if (configuration != null && configuration.isAccountWhitelistSet()) { - addAccounts(configuration.getAccountWhitelist()); + this.isAccountWhitelistSet = configuration.isAccountWhitelistSet(); + if (!configuration.getAccountWhitelist().isEmpty()) { + addAccounts(configuration.getAccountWhitelist()); + } } } - public AddResult addAccounts(final List accounts) { - if (containsInvalidAccount(accounts)) { - return AddResult.ERROR_INVALID_ENTRY; - } - - if (inputHasDuplicates(accounts)) { - return AddResult.ERROR_DUPLICATED_ENTRY; + public WhitelistOperationResult addAccounts(final List accounts) { + final WhitelistOperationResult inputValidationResult = inputValidation(accounts); + if (inputValidationResult != WhitelistOperationResult.SUCCESS) { + return inputValidationResult; } boolean inputHasExistingAccount = accounts.stream().anyMatch(accountWhitelist::contains); if (inputHasExistingAccount) { - return AddResult.ERROR_EXISTING_ENTRY; + return WhitelistOperationResult.ERROR_EXISTING_ENTRY; } this.isAccountWhitelistSet = true; this.accountWhitelist.addAll(accounts); - return AddResult.SUCCESS; + return WhitelistOperationResult.SUCCESS; } - public RemoveResult removeAccounts(final List accounts) { - if (containsInvalidAccount(accounts)) { - return RemoveResult.ERROR_INVALID_ENTRY; - } - - if (inputHasDuplicates(accounts)) { - return RemoveResult.ERROR_DUPLICATED_ENTRY; + public WhitelistOperationResult removeAccounts(final List accounts) { + final WhitelistOperationResult inputValidationResult = inputValidation(accounts); + if (inputValidationResult != WhitelistOperationResult.SUCCESS) { + return inputValidationResult; } if (!accountWhitelist.containsAll(accounts)) { - return RemoveResult.ERROR_ABSENT_ENTRY; + return WhitelistOperationResult.ERROR_ABSENT_ENTRY; } this.accountWhitelist.removeAll(accounts); - return RemoveResult.SUCCESS; + return WhitelistOperationResult.SUCCESS; + } + + private WhitelistOperationResult inputValidation(final List accounts) { + if (accounts == null || accounts.isEmpty()) { + return WhitelistOperationResult.ERROR_EMPTY_ENTRY; + } + + if (containsInvalidAccount(accounts)) { + return WhitelistOperationResult.ERROR_INVALID_ENTRY; + } + + if (inputHasDuplicates(accounts)) { + return WhitelistOperationResult.ERROR_DUPLICATED_ENTRY; + } + + return WhitelistOperationResult.SUCCESS; } private boolean inputHasDuplicates(final List accounts) { @@ -94,18 +107,4 @@ public class AccountWhitelistController { return false; } } - - public enum AddResult { - SUCCESS, - ERROR_DUPLICATED_ENTRY, - ERROR_EXISTING_ENTRY, - ERROR_INVALID_ENTRY - } - - public enum RemoveResult { - SUCCESS, - ERROR_ABSENT_ENTRY, - ERROR_INVALID_ENTRY, - ERROR_DUPLICATED_ENTRY - } } diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/WhitelistOperationResult.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/WhitelistOperationResult.java new file mode 100644 index 0000000000..6818120ba4 --- /dev/null +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/WhitelistOperationResult.java @@ -0,0 +1,22 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.permissioning; + +public enum WhitelistOperationResult { + SUCCESS, + ERROR_DUPLICATED_ENTRY, + ERROR_EMPTY_ENTRY, + ERROR_EXISTING_ENTRY, + ERROR_INVALID_ENTRY, + ERROR_ABSENT_ENTRY +} diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistControllerTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistControllerTest.java index b4ea8e4e46..ec70c76778 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistControllerTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/AccountWhitelistControllerTest.java @@ -15,9 +15,6 @@ package tech.pegasys.pantheon.ethereum.permissioning; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.when; -import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController.AddResult; -import tech.pegasys.pantheon.ethereum.permissioning.AccountWhitelistController.RemoveResult; - import java.util.ArrayList; import java.util.Arrays; @@ -85,29 +82,29 @@ public class AccountWhitelistControllerTest { @Test public void addAccountsWithInvalidAccountShouldReturnInvalidEntryResult() { - AddResult addResult = controller.addAccounts(Arrays.asList("0x0")); + WhitelistOperationResult addResult = controller.addAccounts(Arrays.asList("0x0")); - assertThat(addResult).isEqualTo(AddResult.ERROR_INVALID_ENTRY); + assertThat(addResult).isEqualTo(WhitelistOperationResult.ERROR_INVALID_ENTRY); assertThat(controller.getAccountWhitelist()).isEmpty(); } @Test public void addExistingAccountShouldReturnExistingEntryResult() { controller.addAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - AddResult addResult = + WhitelistOperationResult addResult = controller.addAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(addResult).isEqualTo(AddResult.ERROR_EXISTING_ENTRY); + assertThat(addResult).isEqualTo(WhitelistOperationResult.ERROR_EXISTING_ENTRY); assertThat(controller.getAccountWhitelist()) .containsExactly("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73"); } @Test public void addValidAccountsShouldReturnSuccessResult() { - AddResult addResult = + WhitelistOperationResult addResult = controller.addAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(addResult).isEqualTo(AddResult.SUCCESS); + assertThat(addResult).isEqualTo(WhitelistOperationResult.SUCCESS); assertThat(controller.getAccountWhitelist()) .containsExactly("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73"); } @@ -116,49 +113,63 @@ public class AccountWhitelistControllerTest { public void removeExistingAccountShouldReturnSuccessResult() { controller.addAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - RemoveResult removeResult = + WhitelistOperationResult removeResult = controller.removeAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(removeResult).isEqualTo(RemoveResult.SUCCESS); + assertThat(removeResult).isEqualTo(WhitelistOperationResult.SUCCESS); assertThat(controller.getAccountWhitelist()).isEmpty(); } @Test public void removeAbsentAccountShouldReturnAbsentEntryResult() { - RemoveResult removeResult = + WhitelistOperationResult removeResult = controller.removeAccounts(Arrays.asList("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(removeResult).isEqualTo(RemoveResult.ERROR_ABSENT_ENTRY); + assertThat(removeResult).isEqualTo(WhitelistOperationResult.ERROR_ABSENT_ENTRY); assertThat(controller.getAccountWhitelist()).isEmpty(); } @Test public void removeInvalidAccountShouldReturnInvalidEntryResult() { - RemoveResult removeResult = controller.removeAccounts(Arrays.asList("0x0")); + WhitelistOperationResult removeResult = controller.removeAccounts(Arrays.asList("0x0")); - assertThat(removeResult).isEqualTo(RemoveResult.ERROR_INVALID_ENTRY); + assertThat(removeResult).isEqualTo(WhitelistOperationResult.ERROR_INVALID_ENTRY); assertThat(controller.getAccountWhitelist()).isEmpty(); } @Test public void addDuplicatedAccountShouldReturnDuplicatedEntryResult() { - AddResult addResult = + WhitelistOperationResult addResult = controller.addAccounts( Arrays.asList( "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73", "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(addResult).isEqualTo(AddResult.ERROR_DUPLICATED_ENTRY); + assertThat(addResult).isEqualTo(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY); } @Test public void removeDuplicatedAccountShouldReturnDuplicatedEntryResult() { - RemoveResult removeResult = + WhitelistOperationResult removeResult = controller.removeAccounts( Arrays.asList( "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73", "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")); - assertThat(removeResult).isEqualTo(RemoveResult.ERROR_DUPLICATED_ENTRY); + assertThat(removeResult).isEqualTo(WhitelistOperationResult.ERROR_DUPLICATED_ENTRY); + } + + @Test + public void removeNullListShouldReturnEmptyEntryResult() { + WhitelistOperationResult removeResult = controller.removeAccounts(null); + + assertThat(removeResult).isEqualTo(WhitelistOperationResult.ERROR_EMPTY_ENTRY); + } + + @Test + public void removeEmptyListShouldReturnEmptyEntryResult() { + WhitelistOperationResult removeResult = controller.removeAccounts(new ArrayList<>()); + + assertThat(removeResult).isEqualTo(WhitelistOperationResult.ERROR_EMPTY_ENTRY); } }