From 54c5f5a026de33fec62f9b3df5f100281e7f6368 Mon Sep 17 00:00:00 2001 From: Reinhard Budde Date: Tue, 6 Aug 2024 13:41:52 +0200 Subject: [PATCH] Issue #1656: improve the registration of txt4 controller - if the SAME robot issue a registration request - with the same token - that has been approved in the past - then accept this token as approved --- .../robotCommunication/RobotCommunicator.java | 60 ++++++++++++------- .../iais/roberta/RobotCommunicatorTest.java | 40 ------------- .../RobotCommunicatorTest.java | 42 +++++++++++++ .../restServices/robot/RobotCommand.java | 5 +- 4 files changed, 83 insertions(+), 64 deletions(-) delete mode 100644 OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/RobotCommunicatorTest.java create mode 100644 OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicatorTest.java diff --git a/OpenRobertaRobot/src/main/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicator.java b/OpenRobertaRobot/src/main/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicator.java index 6454f86acc..ad368c1774 100644 --- a/OpenRobertaRobot/src/main/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicator.java +++ b/OpenRobertaRobot/src/main/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicator.java @@ -14,6 +14,7 @@ import de.fhg.iais.roberta.robotCommunication.RobotCommunicationData.State; import de.fhg.iais.roberta.util.Key; import de.fhg.iais.roberta.util.dbc.Assert; +import de.fhg.iais.roberta.util.dbc.DbcException; /** * class, that synchronizes the communication between the bricks and the web-app. Thread-safe. See class {@link RobotCommunicationData} for further @@ -37,11 +38,11 @@ public RobotCommunicator() { * * @return true if the ticket has been accepted */ - public boolean addNewRegistration(RobotCommunicationData newRobotCommunicationData) { - String token = newRobotCommunicationData.getToken(); + RegistrationRequest addNewRegistration(RobotCommunicationData newRobotCommunicationData) { + String newToken = newRobotCommunicationData.getToken(); String newIdentificator = newRobotCommunicationData.getRobotIdentificator(); - Assert.isTrue(token != null && newIdentificator != null); - RobotCommunicationData existingRobotCommunicationData = this.allStates.get(token); + Assert.isTrue(newToken != null && newIdentificator != null); + RobotCommunicationData existingRobotCommunicationData = this.allStates.get(newToken); if ( existingRobotCommunicationData != null ) { String existingIdentificator = existingRobotCommunicationData.getRobotIdentificator(); if ( existingIdentificator == null @@ -50,34 +51,45 @@ public boolean addNewRegistration(RobotCommunicationData newRobotCommunicationDa || existingIdentificator.equals("unknown") ) { LOG.info("ROBOT_RC: token already used. New token required"); - return false; + return RegistrationRequest.TOKEN_INVALID; } } for ( String storedToken : this.allStates.keySet() ) { RobotCommunicationData storedState = this.allStates.get(storedToken); if ( newIdentificator.equals(storedState.getRobotIdentificator()) && !newIdentificator.equals("usb") - && !newIdentificator.equals("unknown") ) { - LOG.info("ROBOT_RC: token approval request for robot [" + newIdentificator + "], but an old request is pending. Start abort old request"); - this.allStates.remove(storedToken); - storedState.abort(); // notifyAll() executed - LOG.info("ROBOT_RC: token approval request for robot [" + newIdentificator + "], but an old request is pending. End abort old request."); + && !newIdentificator.equals("unknown") ) // + { + if ( storedState.getToken().equals(newToken) ) { + // TODO: keep? Experimental for Fischertechnik. A register for a token that was the same as a past registration. Keep running! + LOG.info("ROBOT_RC: token approved for robot [" + newIdentificator + "]. Token " + newToken + " unchanged"); + return RegistrationRequest.REPEATED_REGISTRATION_REQUEST; + } else { + this.allStates.remove(storedToken); + storedState.abort(); // notifyAll() executed + LOG.info("ROBOT_RC: token approval request for robot [" + newIdentificator + "]. An old request is pending and aborted."); + } } } - this.allStates.put(token, newRobotCommunicationData); - return true; + this.allStates.put(newToken, newRobotCommunicationData); + return RegistrationRequest.NEW_REGISTRATION_REQUEST; } public boolean robotWantsTokenToBeApproved(RobotCommunicationData newRobotCommunicationData) { - if ( addNewRegistration(newRobotCommunicationData) ) { - return newRobotCommunicationData.robotTokenAgreementRequest(); // this will freeze the request until another thread issues a notifyAll() - } else { - try { - Thread.sleep(1000); // to avoid a DOS attack (either by a bad implementation of the robot-server protocol or by an attacker), we sleep - } catch ( InterruptedException e ) { - // ignore the interrupt - } - return false; + switch ( addNewRegistration(newRobotCommunicationData) ) { + case TOKEN_INVALID: + try { + Thread.sleep(1000); // to avoid server congestion if a robot uses a bad implementation of the robot-server protocol + } catch ( InterruptedException e ) { + // ignore the interrupt + } + return false; + case NEW_REGISTRATION_REQUEST: + return newRobotCommunicationData.robotTokenAgreementRequest(); // this will freeze the request until another thread issues a notifyAll() + case REPEATED_REGISTRATION_REQUEST: + return true; // was already approved. Second approvement would irritate the user + default: + throw new DbcException("invalid registration request"); } } @@ -286,4 +298,10 @@ public boolean stop(String token, String robotName) { } return false; } + + public enum RegistrationRequest { + TOKEN_INVALID, + NEW_REGISTRATION_REQUEST, + REPEATED_REGISTRATION_REQUEST; + } } diff --git a/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/RobotCommunicatorTest.java b/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/RobotCommunicatorTest.java deleted file mode 100644 index d5e2d4255b..0000000000 --- a/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/RobotCommunicatorTest.java +++ /dev/null @@ -1,40 +0,0 @@ -package de.fhg.iais.roberta; - -import org.junit.Assert; -import org.junit.Test; - -import de.fhg.iais.roberta.robotCommunication.RobotCommunicationData; -import de.fhg.iais.roberta.robotCommunication.RobotCommunicator; -import de.fhg.iais.roberta.util.dbc.DbcException; - -public class RobotCommunicatorTest { - static RobotCommunicationData badRegistration = new RobotCommunicationData(null, null, null, null, null, null, null, null, null); - static RobotCommunicationData goodRegistration1 = new RobotCommunicationData("12345678", null, "00:11:22:33:44:55", null, null, null, null, null, null); - static RobotCommunicationData goodRegistration2 = new RobotCommunicationData("12345678", null, "13:05:98:29:12:99", null, null, null, null, null, null); - - @Test - public void testFirstCanRegister() throws Exception { - RobotCommunicator robotCommunicator = new RobotCommunicator(); - Assert.assertTrue(robotCommunicator.addNewRegistration(goodRegistration1)); - } - - @Test(expected = DbcException.class) - public void testBadRegistrationIsRejected() throws Exception { - RobotCommunicator robotCommunicator = new RobotCommunicator(); - robotCommunicator.addNewRegistration(badRegistration); - } - - @Test - public void testRegisterTwiceWithSameIp() throws Exception { - RobotCommunicator robotCommunicator = new RobotCommunicator(); - robotCommunicator.addNewRegistration(goodRegistration1); - Assert.assertTrue(robotCommunicator.addNewRegistration(goodRegistration1)); - } - - @Test - public void testCantRegisterTwice() throws Exception { - RobotCommunicator robotCommunicator = new RobotCommunicator(); - robotCommunicator.addNewRegistration(goodRegistration1); - Assert.assertFalse(robotCommunicator.addNewRegistration(goodRegistration2)); - } -} \ No newline at end of file diff --git a/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicatorTest.java b/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicatorTest.java new file mode 100644 index 0000000000..854df19bf2 --- /dev/null +++ b/OpenRobertaRobot/src/test/java/de/fhg/iais/roberta/robotCommunication/RobotCommunicatorTest.java @@ -0,0 +1,42 @@ +package de.fhg.iais.roberta.robotCommunication; + +import org.junit.Assert; +import org.junit.Test; + +import de.fhg.iais.roberta.util.dbc.DbcException; + +public class RobotCommunicatorTest { + static RobotCommunicationData badRegistration = new RobotCommunicationData(null, null, null, null, null, null, null, null, null); + static RobotCommunicationData goodRegistration1 = new RobotCommunicationData("12345678", null, "00:11:22:33:44:55", null, null, null, null, null, null); + static RobotCommunicationData goodRegistration2 = new RobotCommunicationData("12345678", null, "13:05:98:29:12:99", null, null, null, null, null, null); + + @Test + public void testFirstCanRegister() throws Exception { + RobotCommunicator rc = new RobotCommunicator(); + expect(rc.addNewRegistration(goodRegistration1), RobotCommunicator.RegistrationRequest.NEW_REGISTRATION_REQUEST); + } + + @Test(expected = DbcException.class) + public void testBadRegistrationIsRejected() throws Exception { + RobotCommunicator rc = new RobotCommunicator(); + rc.addNewRegistration(badRegistration); + } + + @Test + public void testRegisterTwiceWithSameIp() { + RobotCommunicator rc = new RobotCommunicator(); + rc.addNewRegistration(goodRegistration1); + expect(rc.addNewRegistration(goodRegistration1), RobotCommunicator.RegistrationRequest.REPEATED_REGISTRATION_REQUEST); + } + + @Test + public void testCantRegisterTwice() { + RobotCommunicator rc = new RobotCommunicator(); + rc.addNewRegistration(goodRegistration1); + expect(rc.addNewRegistration(goodRegistration2), RobotCommunicator.RegistrationRequest.TOKEN_INVALID); + } + + private void expect(RobotCommunicator.RegistrationRequest result, RobotCommunicator.RegistrationRequest expected) { + Assert.assertEquals(expected, result); + } +} \ No newline at end of file diff --git a/OpenRobertaServer/src/main/java/de/fhg/iais/roberta/javaServer/restServices/robot/RobotCommand.java b/OpenRobertaServer/src/main/java/de/fhg/iais/roberta/javaServer/restServices/robot/RobotCommand.java index 0ae512d659..4aa6813531 100644 --- a/OpenRobertaServer/src/main/java/de/fhg/iais/roberta/javaServer/restServices/robot/RobotCommand.java +++ b/OpenRobertaServer/src/main/java/de/fhg/iais/roberta/javaServer/restServices/robot/RobotCommand.java @@ -61,7 +61,6 @@ public Response handle(JSONObject requestEntity) throws JSONException, Interrupt } String robot = r.getWithDefault("?", "robot"); - String macaddr = r.getWithDefault("?", "macaddr"); String robotName = r.getWithDefault("?", "robotname", "brickname"); String batteryvoltage = r.getWithDefault("?", "battery"); String menuversion = r.getWithDefault("?", "menuversion"); @@ -72,6 +71,7 @@ public Response handle(JSONObject requestEntity) throws JSONException, Interrupt JSONObject response; switch (cmd) { case CMD_REGISTER: + String macaddr = r.get("macaddr"); LOG.info("ROBOT_PROTOCOL: robot [" + macaddr + "] send token " + token + " for user approval"); RobotCommunicationData state = new RobotCommunicationData(token, robot, macaddr, robotName, batteryvoltage, menuversion, runtimeVersion, pluginName, firmwareversion); @@ -91,8 +91,7 @@ public Response handle(JSONObject requestEntity) throws JSONException, Interrupt return Response.serverError().build(); } else { if (!command.equals(CMD_REPEAT) || logPush) { - LOG.info("ROBOT_PROTOCOL: the command " + command + " is pushed to robot [" + macaddr - + "] for token " + token + " [count: " + counter + "]"); + LOG.info("ROBOT_PROTOCOL: the command " + command + " is pushed to robot with token " + token + " [count: " + counter + "]"); } response = new JSONObject().put(CMD, command); response.put(SUBTYPE, this.brickCommunicator.getSubtype());