ADB 'connect' command may trigger a device to be owned by the user!

What is the issue or idea you have? If the remote debug URL of a device is known while this device is free that is owned by no one, a STF user, that is an authenticated one, may run an adb connect command on this URL which triggers the acquisition of control of this device by this user.

This behaviour is abnormal and is compounded by the latest version of ADB (1.0.40 Version 4986621) which comes with this new feature:

Add support for reconnection of TCP connections. Upon disconnection, adb will attempt to reconnect for up to 60 seconds before abandoning a connection

resulting to an automatic owning of the device every time the user try to release the device either through the Stop Using button or the API or automatic timeout (i.e. which all disconnect the TCP connection), until the user kills the adb server or runs an adb disconnect command on this URL.

The worst part is that in case of a STF machine restarting, the TCP port of the Remote debug URL may change for the considered device, and even can be assigned to another device of the same stf-provider, leading the automatic connection to be made to the latter, I have seen this behaviour by making tests.

Does it only happen on a specific device? Please run adb devices -l and paste the corresponding row. no device specific.

Please provide the steps to reproduce the issue.

1. get the remote debug URL url-1 of the device device-1 using UI or API, and then keep free device-1
2. run the command : adb connect url-1
3. tale a look at UI and observe that device-1 is now owned by the user
4. run the command : adb devices to check that device-1 is well connected
5. try to release device-1 using UI or API or automatic timeout, and in case of latest ADB binary is used, observe that device-1 is owned and connected again just after being released

What is the expected behavior? At step 2 above, ADB should return an error like :

unable to connect to device-1: Connection refused

What is your proposal? In fact, I felt some problems in the code:

1. the API remoteDisconnectUserDeviceBySerial does not work properly because it does not avoid the user to ADB connect to the target device
2. the API deleteUserDeviceBySerial as well as kicking actions triggered at UI side and automatic timeout should include a processing to stop unexpected ADB connections.

So, for test purpose only, I have made some simple changes in a test branch (i.e. see below), and I have tested it with success, I hope that will help you to bring the complete and final changes into the master branch.

diff --git a/lib/units/device/plugins/connect.js b/lib/units/device/plugins/connect.js
index 90bf3f7..2bc728c 100644
--- a/lib/units/device/plugins/connect.js
+++ b/lib/units/device/plugins/connect.js
@@ -117,6 +117,11 @@ module.exports = syrup.serial()
       if (plugin.isRunning()) {
         activeServer.close()
         activeServer.end()
+
+/******************* added ******************/
+        activeServer = null
+
+/*****************************************************/
       }
     })
 
@@ -131,8 +136,12 @@ module.exports = syrup.serial()
     }
 
     lifecycle.observe(plugin.stop)
-    group.on('leave', plugin.end)
 
+/******************* replaced ******************/
+    //group.on('leave', plugin.end)
+    group.on('leave', plugin.stop)
+
+/*****************************************************/
     router
       .on(wire.ConnectStartMessage, function(channel) {
         var reply = wireutil.reply(options.serial)
@@ -163,7 +172,12 @@ module.exports = syrup.serial()
       })
       .on(wire.ConnectStopMessage, function(channel) {
         var reply = wireutil.reply(options.serial)
-        plugin.end()
+
+/******************* replaced ******************/
+        // plugin.end()
+        plugin.stop()
+
+/********************************************************/
           .then(function() {
             push.send([
               channel

Nevertheless, using the latest version of ADB, a little problem remains as described hereafter:

1. user-1 takes the control of device-1
2. user-1 ADB connects to device-1
3. user-1 releases device-1 at time T0
4. (ADB tries to connect to device-1 without success because the server for device-1 is stopped, so it's OK)
5. Before time T0+60s, user-2 takes the control of device-1
6. ADB always tries to connect to device-1 without success because it is unauthorized (i.e. bad ADB key), but since the server for device-1 is now started, a TCP connection is still set up about every 300 ms, and the server logs them at the same rate (i.e New remote ADB connection from ::ffff:192.168.1.127), until user-2 decides to release device-1 , or user-1 decides to ADB disconnect from device-1.

So, perhaps only authorized ADB connections should be logged ?

Do you see errors or warnings in the stf local output? If so, please paste them or the full log here. N/A

Please run stf doctor and paste the output here. debian@debian:~/stf-master$ stf doctor

2018-10-26T15:43:07.148Z INF/cli:doctor 30483 [*] OS Arch: x64
2018-10-26T15:43:07.149Z INF/cli:doctor 30483 [*] OS Platform: linux
2018-10-26T15:43:07.150Z INF/cli:doctor 30483 [*] OS Platform: 3.16.0-4-amd64
2018-10-26T15:43:07.150Z INF/cli:doctor 30483 [*] Using Node 8.9.3
2018-10-26T15:43:07.168Z INF/cli:doctor 30483 [*] Using ZeroMQ 4.0.5
2018-10-26T15:43:07.185Z INF/cli:doctor 30483 [*] Using ADB 1.0.36
2018-10-26T15:43:07.186Z ERR/cli:doctor 30483 [*] ProtoBuf is not installed (`protoc` is missing)
2018-10-26T15:43:07.211Z INF/cli:doctor 30483 [*] Using GraphicsMagick 1.3.20
2018-10-26T15:43:07.232Z INF/cli:doctor 30483 [*] Using RethinkDB 2.3.6~0jessie