Repository: opentrons/opentrons
Issue: Bug: SSID with space character

Unable to connect to wifi networks with a space in the network name (ssid).

## Expected
- User can connect a robot to a wifi network with a space in its SSID (e.g. 'wifi network')
- If user is unable to connect, they see a 'failure' message

## Observed
- Robot returns a success message but fails to connect

## Steps to recreate
- Connect to any robot over usb (tested with 'young-moon' , commit 40d19e8)
- Select a wifi network from the 'connectivity' dropdown that has a space in its name (e.g. 'big duck')
- Observe that the app:
-- returns a 'success' message
-- shows that the robot is not connected to the new network

From Redux console:
```
type(pin): "api:WIFI_SUCCESS"
▶payload(pin)
▶robot(pin)
name(pin): "Opentrons USB"
ip(pin): "[fd00:0:cafe:fefe::1]"
port(pin): 31950
wired(pin): true
isConnected(pin): false
path(pin): "configure"
▶response(pin)
ssid(pin): "Big Duck"
message(pin): "Unknown parameter: Duck Error: No network with SSID 'Big' found."
```

Changed files:
api/opentrons/server/endpoints/wifi.py, api/tests/opentrons/server/test_wifi_endpoints.py

diff --git a/api/opentrons/server/endpoints/wifi.py b/api/opentrons/server/endpoints/wifi.py
index 55138c4310..4050502b10 100644
--- a/api/opentrons/server/endpoints/wifi.py
+++ b/api/opentrons/server/endpoints/wifi.py
@@ -1,105 +1,82 @@
-import os
 import json
 import logging
 import subprocess
 from aiohttp import web
 
 log = logging.getLogger(__name__)
-ENABLE_NMCLI = os.environ.get('ENABLE_NETWORKING_ENDPOINTS', '')
 
 
 async def list_networks(request):
     """
     Get request will return a list of discovered ssids.
-
-    If the environment variable ENABLE_NETWORKING_ENDPOINTS is set, this list
-    is discovered using `nmcli`. If it is not set, then a dummy list of strings
-    is returned. The environment variable will be set on the robot running the
-    server, but will not be set during development on a laptop, or while
-    running the server during test on CI.
     """
     res = {"list": []}
-    if ENABLE_NMCLI:
-        try:
-            proc = subprocess.run(["nmcli", "--terse",
-                                   "--fields", "ssid,signal,active",
-                                   "device", "wifi", "list"],
-                                  stdout=subprocess.PIPE,
-                                  stderr=subprocess.PIPE)
-        except subprocess.CalledProcessError as e:
-            res = "CalledProcessError: {}".format(e.stdout)
-        except FileNotFoundError as e:
-            res = "FileNotFoundError: {}".format(e)
-        else:
-            out = proc.stdout.decode().strip()
-            err = proc.stderr.decode().strip()
-
-            log.debug("'nmcli device wifi list' stdout: {}".format(out))
-            if len(err) > 0:
-                log.error("'nmcli device wifi list' stderr: {}".format(err))
-
-            lines = out.split("\\n")
-            networks = [x.split(":") for x in lines]
-            res["list"] = [
-                {
-                    "ssid": n[0],
-                    "signal": int(n[1]) if n[1].isdigit() else None,
-                    "active": n[2].lower() == "yes"
-                }
-                for n in networks if len(n) >= 3
-            ]
+
+    try:
+        cmd = [
+            "nmcli",
+            "--terse",
+            "--fields",
+            "ssid,signal,active",
+            "device",
+            "wifi",
+            "list"]
+        out, err = _subprocess(' '.join(cmd))
+    except subprocess.CalledProcessError as e:
+        res = "CalledProcessError: {}".format(e.stdout)
+        status = 500
+    except FileNotFoundError as e:
+        res = "FileNotFoundError: {}".format(e)
+        status = 500
     else:
+        log.debug("'nmcli device wifi list' stdout: {}".format(out))
+        if len(err) > 0:
+            log.error("'nmcli device wifi list' stderr: {}".format(err))
+
+        lines = out.split("\\n")
+        networks = [x.split(":") for x in lines]
         res["list"] = [
-            {"ssid": "a", "signal": 42, "active": True},
-            {"ssid": "b", "signal": 43, "active": False},
-            {"ssid": "c", "signal": None, "active": False}
+            {
+                "ssid": n[0],
+                "signal": int(n[1]) if n[1].isdigit() else None,
+                "active": n[2].lower() == "yes"
+            }
+            for n in networks if len(n) >= 3
         ]
+        status = 200
 
-    # TODO(mc, 2019-02-20): return error status code if error
-    return web.json_response(res)
+    return web.json_response(res, status=status)
 
 
 async def configure(request):
     """
-    Post request should include a json body with fields "ssid" and "psk" for
-    network name and password respectively. Robot will attempt to connect to
-    this network and respond with Ok if successful or an error code if not.
-
-    If the environment variable ENABLE_NETWORKING_ENDPOINTS is set, this is
-    perfomed using `nmcli`. If it is not set, then a dummy status is returned.
-    The environment variable will be set on the robot running the server, but
-    will not be set during development on a laptop, or while running the server
-    during test on CI.
+    Post request should include a json body with fields "ssid" (required) and
+    "psk" (optional) for network name and password respectively. Robot will
+    attempt to connect to this network and respond with Ok if successful or an
+    error code if not.
     """
-    status = None
     result = {}
-    message = ''
 
     try:
-        body = await request.text()
-        jbody = json.loads(body)
-        ssid = jbody.get('ssid')
-        psk = jbody.get('psk')
+        body = await request.json()
+        ssid = body.get('ssid')
+        psk = body.get('psk')
 
-        if ssid is None or psk is None:
+        if ssid is None:
             status = 400
-            message = 'Error: "ssid": string, "psk": string are required'
-        elif ENABLE_NMCLI:
-            cmd = 'nmcli device wifi connect {} password "{}"'.format(
-                ssid, psk)
+            message = 'Error: "ssid" string is required'
+        else:
+            cmd = 'nmcli device wifi connect "{}"'.format(
+                ssid)
+            password = ' password "{}"'.format(psk) if psk else ''
             # some nmcli errors go to stdout (e.g. bad psk), while others go to
-            # stderr (e.g. bad ssid), so we pipe both outputs together
-            proc = subprocess.run(cmd, shell=True, stdout=subprocess.PIPE,
-                                  stderr=subprocess.STDOUT)
-            message = proc.stdout.decode().strip().split("\\r")[-1]
+            # stderr (e.g. bad ssid), so we put both outputs together
+            res, err = _subprocess(cmd + password)
+            message = (res + err).split("\\r")[-1]
             log.debug("nmcli device wifi connect -> {}".format(message))
 
-            status = 201 if not message.startswith('Error:') else 401
+            status = 401 if 'Error:' in message else 201
             result['ssid'] = ssid
-        else:
-            status = 200
-            message = 'Configuration unchanged'
-            result['ssid'] = 'a'
 
     except json.JSONDecodeError as e:
         log.debug("Error: JSONDecodeError in /wifi/configure: {}".format(e))
@@ -128,26 +105,32 @@ async def status(request):
       "full" - connection to router and internet
       "unknown" - an exception occured while trying to determine status
 
-    If the environment variable ENABLE_NETWORKING_ENDPOINTS is set, this list
-    is discovered using `nmcli`. If it is not set, then "testing" is returned.
-    The environment variable will be set on the robot running the server, but
-    will not be set during development on a laptop, or while running the server
-    during test on CI.
     """
     connectivity = {"status": "unknown"}
-    if ENABLE_NMCLI:
-        try:
-            proc = subprocess.run(["nmcli", "networking", "connectivity"],
-                                  stdout=subprocess.PIPE)
-            res = proc.stdout.decode().strip()
-            log.debug("Connectivity: {}".format(res))
-            connectivity["status"] = res
-        except subprocess.CalledProcessError as e:
-            log.error("CalledProcessError: {}".format(e.stdout))
-        except FileNotFoundError as e:
-            log.error("FileNotFoundError: {}".format(e))
-    else:
-        connectivity['status'] = "testing"
+    try:
+        cmd = ["nmcli", "networking", "connectivity"]
+        res, _ = _subprocess(' '.join(cmd))
+        log.debug("Connectivity: {}".format(res))
+        connectivity["status"] = res
+        status = 200
+    except subprocess.CalledProcessError as e:
+        log.error("CalledProcessError: {}".format(e.stdout))
+        status = 500
+    except FileNotFoundError as e:
+        log.error("FileNotFoundError: {}".format(e))
+        status = 500
 
-    # TODO(mc, 2019-02-20): return error status code if error
-    return web.json_response(connectivity)
+    return web.json_response(connectivity, status=status)
+
+
+def _subprocess(cmd: str) -> (str, str):
+    """
+    Runs the command in a subprocess shell (use the string form of command,
+    not the list of strings) and returns the captured stdout output.
+    :param cmd: a command string to execute in a subprocess shell
+    :return: (stdout, stderr)
+    """
+    proc = subprocess.run(cmd, shell=True, stdout=subprocess.PIPE)
+    out = proc.stdout.decode().strip()
+    err = proc.stderr.decode().strip()
+    return out, err
diff --git a/api/tests/opentrons/server/test_wifi_endpoints.py b/api/tests/opentrons/server/test_wifi_endpoints.py
index 9b30e1308c..889b4e7316 100755
--- a/api/tests/opentrons/server/test_wifi_endpoints.py
+++ b/api/tests/opentrons/server/test_wifi_endpoints.py
@@ -1,45 +1,78 @@
 import json
 from opentrons.server.main import init
+from opentrons.server.endpoints import wifi
 
+"""
+All mocks in this test suite represent actual output from nmcli commands
+"""
 
-async def test_wifi_status(virtual_smoothie_env, loop, test_client):
+
+async def test_wifi_status(
+        virtual_smoothie_env, loop, test_client, monkeypatch):
     app = init(loop)
     cli = await loop.create_task(test_client(app))
 
-    expected = json.dumps({'status': 'testing'})
+    def mock_subprocess(cmd):
+        # Command: `nmcli networking connectivity`
+        res = "full"
+        return res, ''
+
+    monkeypatch.setattr(wifi, '_subprocess', mock_subprocess)
+
+    expected = json.dumps({'status': 'full'})
     resp = await cli.get('/wifi/status')
     text = await resp.text()
     assert resp.status == 200
     assert text == expected
 
 
-async def test_wifi_list(virtual_smoothie_env, loop, test_client):
+async def test_wifi_list(virtual_smoothie_env, loop, test_client, monkeypatch):
     app = init(loop)
     cli = await loop.create_task(test_client(app))
 
+    def mock_subprocess(cmd):
+        # Command: `nmcli --terse --fields ssid,signal,active device wifi list`
+        res = """Opentrons:81:yes
+Big Duck:47:no
+HP-Print-1-LaserJet Pro:35:no
+Guest Wireless:24:no
+"""
+        return res, ''
+
+    monkeypatch.setattr(wifi, '_subprocess', mock_subprocess)
+
     expected = json.dumps({
         'list': [
-            {'ssid': 'a', 'signal': 42, 'active': True},
-            {'ssid': 'b', 'signal': 43, 'active': False},
-            {'ssid': 'c', 'signal': None, 'active': False}
+            {'ssid': 'Opentrons', 'signal': 81, 'active': True},
+            {'ssid': 'Big Duck', 'signal': 47, 'active': False},
+            {'ssid': 'HP-Print-1-LaserJet Pro', 'signal': 35, 'active': False},
+            {'ssid': 'Guest Wireless', 'signal': 24, 'active': False}
         ]
     })
+
     resp = await cli.get('/wifi/list')
     text = await resp.text()
     assert resp.status == 200
     assert text == expected
 
 
-async def test_wifi_configure(virtual_smoothie_env, loop, test_client):
+async def test_wifi_configure(
+        virtual_smoothie_env, loop, test_client, monkeypatch):
     app = init(loop)
     cli = await loop.create_task(test_client(app))
 
-    expected = json.dumps({
-        'ssid': 'a',
-        'message': 'Configuration unchanged'
-    })
-    resp = await cli.post('/wifi/configure',
-                          json={'ssid': 'this', 'psk': 'that'})
-    text = await resp.text()
-    assert resp.status == 200
-    assert text == expected
+    msg = "Device 'wlan0' successfully activated with '076aa998-0275-4aa0-bf85-e9629021e267'."  # noqa
+
+    def mock_subprocess(cmd):
+        # Command: nmcli device wifi connect "{ssid}" password "{psk}"
+        return msg, ''
+
+    monkeypatch.setattr(wifi, '_subprocess', mock_subprocess)
+
+    expected = {'ssid': 'Opentrons', 'message': msg}
+
+    resp = await cli.post(
+        '/wifi/configure', json={'ssid': 'Opentrons', 'psk': 'scrt sqrl'})
+    body = await resp.json()
+    assert resp.status == 201
+    assert body == expected