[NC-2247] Fixing node whitelist isPermitted check (#766)

* [NC-2247] Fixing node whitelist isPermitted check

* Typos

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

@@ -211,6 +211,7 @@ public class PeerDiscoveryController {
     }
 
     if (!whitelistIfPresentIsNodePermitted(sender)) {
+      LOG.trace("Dropping packet from peer not in the whitelist ({})", sender.getEnodeURI());
       return;
     }
 
@@ -222,6 +223,7 @@ public class PeerDiscoveryController {
 
     switch (packet.getType()) {
       case PING:
+        LOG.trace("Received PING packet from {}", sender.getEnodeURI());
         if (!peerBlacklisted && addToPeerTable(peer)) {
           final PingPacketData ping = packet.getPacketData(PingPacketData.class).get();
           respondToPing(ping, packet.getHash(), peer);
@@ -230,6 +232,7 @@ public class PeerDiscoveryController {
         break;
       case PONG:
         {
+          LOG.trace("Received PONG packet from {}", sender.getEnodeURI());
           matchInteraction(packet)
               .ifPresent(
                   interaction -> {
@@ -246,6 +249,7 @@ public class PeerDiscoveryController {
           break;
         }
       case NEIGHBORS:
+        LOG.trace("Received NEIGHBORS packet from {}", sender.getEnodeURI());
         matchInteraction(packet)
             .ifPresent(
                 interaction -> {
@@ -271,6 +275,7 @@ public class PeerDiscoveryController {
         break;
 
       case FIND_NEIGHBORS:
+        LOG.trace("Received FIND_NEIGHBORS packet from {}", sender.getEnodeURI());
         if (!peerKnown || peerBlacklisted) {
           break;
         }

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java

@@ -99,7 +99,24 @@ public class NodeWhitelistController {
   }
 
   public boolean isPermitted(final Peer node) {
-    return (nodesWhitelist.contains(node));
+    return nodesWhitelist
+        .stream()
+        .anyMatch(
+            p -> {
+              boolean idsMatch = node.getId().equals(p.getId());
+              boolean hostsMatch = node.getEndpoint().getHost().equals(p.getEndpoint().getHost());
+              boolean udpPortsMatch =
+                  node.getEndpoint().getUdpPort() == p.getEndpoint().getUdpPort();
+              boolean tcpPortsMatchIfPresent = true;
+              if (node.getEndpoint().getTcpPort().isPresent()
+                  && p.getEndpoint().getTcpPort().isPresent()) {
+                tcpPortsMatchIfPresent =
+                    node.getEndpoint().getTcpPort().getAsInt()
+                        == p.getEndpoint().getTcpPort().getAsInt();
+              }
+
+              return idsMatch && hostsMatch && udpPortsMatch && tcpPortsMatchIfPresent;
+            });
   }
 
   public List<Peer> getNodesWhitelist() {

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java

@@ -16,8 +16,10 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResult;
 
 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 tech.pegasys.pantheon.util.bytes.BytesValue;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -125,4 +127,107 @@ public class NodeWhitelistControllerTest {
 
     assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result");
   }
+
+  @Test
+  public void whenNodeIdsAreDifferentItShouldNotBePermitted() {
+    Peer peer1 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303);
+    Peer peer2 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xbbba80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303);
+
+    controller.addNode(peer1);
+
+    assertThat(controller.isPermitted(peer2)).isFalse();
+  }
+
+  @Test
+  public void whenNodesHostsAreDifferentItShouldNotBePermitted() {
+    Peer peer1 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303);
+    Peer peer2 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.2",
+            30303);
+
+    controller.addNode(peer1);
+
+    assertThat(controller.isPermitted(peer2)).isFalse();
+  }
+
+  @Test
+  public void whenNodesUdpPortsAreDifferentItShouldNotBePermitted() {
+    Peer peer1 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30301);
+    Peer peer2 =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30302);
+
+    controller.addNode(peer1);
+
+    assertThat(controller.isPermitted(peer2)).isFalse();
+  }
+
+  @Test
+  public void whenCheckingIfNodeIsPermittedTcpPortShouldNotBeConsideredIfAbsent() {
+    Peer peerWithTcpPortSet =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303,
+            10001);
+    Peer peerWithoutTcpPortSet =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303);
+
+    controller.addNode(peerWithoutTcpPortSet);
+
+    assertThat(controller.isPermitted(peerWithTcpPortSet)).isTrue();
+  }
+
+  @Test
+  public void whenCheckingIfNodeIsPermittedTcpPortShouldBeConsideredIfPresent() {
+    Peer peerWithTcpPortSet =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303,
+            10001);
+    Peer peerWithDifferentTcpPortSet =
+        new DefaultPeer(
+            BytesValue.fromHexString(
+                "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"),
+            "127.0.0.1",
+            30303,
+            10002);
+
+    controller.addNode(peerWithDifferentTcpPortSet);
+
+    assertThat(controller.isPermitted(peerWithTcpPortSet)).isFalse();
+  }
 }