fix: a hidden Wi-Fi survives the hand-over, and a failed one leaves an owner

The prayer-room laptop's 0.2.5 migration built a NetworkManager
connection, could not bring it up, rolled back, and left the wireless
adapter belonging to nobody. Four separate defects, each of which would
have been enough on its own.

THE HIDDEN FLAG WAS THROWN AWAY. The stanza said wpa-scan-ssid 1 and the
parser read it — into options, where the only accessor ignored it. The
profile came out with 802-11-wireless.hidden: no, and NetworkManager
will not associate with an SSID it has not seen in a scan. A hidden one
is never in a scan. credentials() returned a pair; profile() now returns
the whole network, and add() writes the flag in BOTH directions rather
than only when true, because a default that happens to be right is not
the same as an answer.

IFDOWN RAN AFTER THE STANZA WAS COMMENTED OUT. ifupdown reads
/etc/network/interfaces to find out how to bring an interface down; with
the stanza gone it says "Ignoring unknown interface" and does nothing.
So ifupdown's own wpa_supplicant and DHCP client were still holding the
radio when NetworkManager was asked to associate with it, which is a
fight NetworkManager loses. Down first, while the file still says how;
rewrite the file second.

THE ROLLBACK LEFT THE ADAPTER UNMANAGED. restore() ended with
`device set <dev> managed no`, which is right while ifupdown can bring
the interface up and is a radio nobody owns when ifup has failed too.
That is the state the laptop was found in, and why `nmcli device set
wlp2s0 managed yes` was a manual recovery step. It now escalates: if
ifupdown cannot get the network back either, the interface goes to
NetworkManager rather than to neither, and it says so loudly.

A '#' IN A PASSWORD WAS SILENTLY TRUNCATED. The parser cut every line at
its first '#'. interfaces(5) is explicit that only whole-line comments
exist, so a pre-shared key containing one became a prefix of itself —
and the only symptom is an association failing exactly the way a wrong
password fails.

Two smaller ones went with them. The rollback un-comments its own marked
lines in place rather than copying the backup over the whole file, so a
wired change made between the migration and the rollback is no longer
silently reverted. And nmcli terse output is split on its own escaping
rules under LC_ALL=C, so a network whose name contains a colon cannot
make a successful migration look like a failed one.

--adopt now also refuses a network it cannot recreate before taking the
interface away from ifupdown to find out, and reports an
unmanaged-devices line in NetworkManager's configuration, which survives
the stanza going away.

Verified against a real NetworkManager 1.52.1: the keyfile carries
hidden=true, the SSID keeps its space, and a psk containing '#' is
intact, at 0600 root:root with no connection.permissions. The new test
drives the whole hand-over against stubbed tools and asserts the
ordering, the rollback and the managed state — including that a machine
already migrated, which is the laptop, is not migrated again.

Co-Authored-By: Claude Opus 5 <[email protected]>

docs/networking.md

@@ -154,6 +154,74 @@ that would be a one-way door.
 
 The original is kept at `/etc/network/interfaces.before-ekklesia-classroom`.
 
+### What 0.2.5 got wrong, and what 0.2.6 changed
+
+The prayer-room laptop's Wi-Fi is **hidden** — `ROGC Classroom`, an SSID
+with a space in it, announced by nothing. `--adopt` built a connection,
+failed to bring it up, rolled back, and left the adapter belonging to
+nobody. Four separate defects, all fixed:
+
+**1. The hidden flag was thrown away.** The stanza said `wpa-scan-ssid 1`
+and the parser read it — into `options`, where the only accessor ignored
+it. The generated profile had `802-11-wireless.hidden: no`, and
+NetworkManager will not associate with an SSID it has not seen in a scan.
+A hidden one is never in a scan. `credentials()` returned a pair; there
+is now a `profile()` that returns the whole network, and `add()` writes
+`802-11-wireless.hidden` in **both** directions rather than only when
+true — a default that happens to be right is not an answer.
+
+**2. `ifdown` ran after the stanza was commented out.** ifupdown reads
+`/etc/network/interfaces` to find out how to bring an interface down.
+With the stanza gone it says *"Ignoring unknown interface"* and does
+nothing — so ifupdown's own `wpa_supplicant` and DHCP client were still
+holding the radio when NetworkManager was asked to associate with it.
+Two supplicants on one interface is a fight NetworkManager loses. The
+order is now: **`ifdown` first, while the file still says how; rewrite
+the file second.**
+
+**3. The rollback left the adapter unmanaged.** `restore()` ended with
+`nmcli device set <dev> managed no`, which is right while ifupdown can
+actually bring the interface up. When `ifup` fails too, those two lines
+are a wireless adapter neither manager owns: no connection, and a panel
+that cannot even scan. That is the state the laptop was found in, and it
+is why `sudo nmcli device set wlp2s0 managed yes` was a manual recovery
+step. The rollback now escalates — if ifupdown cannot get the network
+back either, the interface is handed to NetworkManager rather than left
+with neither, and it says so loudly.
+
+**4. A `#` in a password was silently truncated.** The parser cut every
+line at its first `#`. `interfaces(5)` is explicit that only whole-line
+comments exist — *"end-of-line comments are NOT supported"* — so a
+pre-shared key containing `#` became a prefix of itself, and the only
+symptom is an association that fails exactly the way a wrong password
+fails.
+
+Two smaller ones went with them. The rollback un-comments its own marked
+lines in place instead of copying the backup over the whole file, because
+the backup is written once and never refreshed — a whole-file restore
+silently reverted any *wired* change an administrator made in between.
+And `nmcli -t` output is now split on its own escaping rules rather than
+on `:`, with `LC_ALL=C`, so a network whose name contains a colon cannot
+make a successful migration look like a failed one.
+
+`--adopt` also now refuses a network it cannot recreate — WPA-Enterprise
+has no pre-shared key to carry across — *before* taking the interface
+away from ifupdown to find out, and reports any `unmanaged-devices` line
+in NetworkManager's own configuration, which survives the stanza going
+away and would otherwise leave the interface belonging to nobody after a
+restart.
+
+Verified against a real NetworkManager 1.52.1: the keyfile it writes
+carries `hidden=true`, `ssid=ROGC Classroom` with its space, and a psk
+containing `#` intact, at 0600 root:root with no `connection.permissions`.
+`tests/test-classroom-wifi-adopt.sh` drives the whole hand-over against
+stubbed tools and asserts the ordering, the rollback and the managed
+state.
+
+**The prayer-room laptop must not be migrated again.** Its stanza is
+already gone, so `--adopt` reports "nothing to adopt" and exits 0 without
+touching anything; there is a test for exactly that.
+
 After the migration the panel shows the connection at the next login, the
 room can scan, and the credential protection below applies to it like any
 other system connection.

lib/classroom_network.py

@@ -36,10 +36,24 @@ import re
 
 # ifupdown's own spelling. Values may be quoted, and a psk may be a
 # 64-character hex key rather than a passphrase.
-SETTINGS = ("wpa-ssid", "wpa-psk", "wpa-key-mgmt", "wpa-conf", "wpa-proto")
+SETTINGS = ("wpa-ssid", "wpa-psk", "wpa-key-mgmt", "wpa-conf", "wpa-proto",
+            "wpa-scan-ssid")
+
+# Whether the access point broadcasts its name.
+#
+# wpa-scan-ssid 1 is ifupdown's spelling of wpa_supplicant's scan_ssid=1:
+# probe for this network by name, because it is not in a passive scan.
+# NetworkManager spells the same thing 802-11-wireless.hidden, and a
+# migration that drops it produces a connection that is correct in every
+# visible respect and can never associate — NetworkManager will not
+# connect to an SSID it has not seen in a scan, and a hidden one is never
+# in a scan. That is what happened to the prayer-room laptop on 0.2.5.
+HIDDEN = "wpa-scan-ssid"
 
 _STANZA = re.compile(r"^\s*(iface|auto|allow-hotplug|source|source-directory)\s+(.*)$")
 
+TRUTHY = {"1", "yes", "true", "on"}
+
 
 def _unquote(value):
     value = (value or "").strip()
@@ -63,7 +77,19 @@ def parse(text):
     current = None
 
     for number, raw in enumerate(lines):
-        line = raw.split("#", 1)[0].rstrip()
+        # A comment is a WHOLE line, and only a whole line.
+        #
+        # interfaces(5): "Lines starting with '#' are ignored. Note that
+        # end-of-line comments are NOT supported." This used to cut every
+        # line at its first '#', which is a silent corruption of exactly
+        # one value: a pre-shared key containing a '#' was truncated, the
+        # migrated connection got a password that was a prefix of the
+        # real one, and the only symptom is an association that fails the
+        # way a wrong password fails.
+        if raw.lstrip().startswith("#"):
+            continue
+
+        line = raw.rstrip()
         match = _STANZA.match(line)
 
         if match:
@@ -129,12 +155,57 @@ def credentials(stanza):
     network inline returns no SSID: that one is not ours to move, and the
     caller must leave it alone rather than guess.
     """
+    found = profile(stanza)
+
+    if found is None:
+        return None, None
+
+    return found["ssid"], found["psk"]
+
+
+def profile(stanza):
+    """Everything needed to recreate this stanza in NetworkManager.
+
+    A dict, or None when the stanza is not ours to move. Returning the
+    whole network rather than a pair is the fix for the 0.2.5 failure:
+    `wpa-scan-ssid` was sitting in the parsed options the whole time and
+    the only accessor threw it away, so the hand-over could not have
+    preserved it however carefully it was written.
+
+    `key_mgmt` is "wpa-psk", "none", or None meaning "this file says
+    something this command does not understand". The caller must refuse
+    that last one rather than guess: a WPA-Enterprise network recreated
+    as a PSK network is a migration that cannot work and has already
+    taken the interface away from ifupdown to find out.
+    """
     options = stanza.get("options") or {}
 
     if "wpa-conf" in options and "wpa-ssid" not in options:
-        return None, None
+        return None
 
-    return options.get("wpa-ssid") or None, options.get("wpa-psk") or None
+    ssid = options.get("wpa-ssid") or None
+
+    if not ssid:
+        return None
+
+    psk = options.get("wpa-psk") or None
+    declared = (options.get("wpa-key-mgmt") or "").strip().upper()
+
+    if declared in ("", "WPA-PSK", "WPA-PSK-SHA256", "SAE", "WPA-PSK SAE"):
+        key_mgmt = "wpa-psk" if psk else "none"
+    elif declared == "NONE":
+        key_mgmt = "none"
+    else:
+        # WPA-EAP, IEEE8021X and anything else this has never seen.
+        key_mgmt = None
+
+    return {
+        "ssid": ssid,
+        "psk": psk,
+        # A network that has to be probed for by name. See HIDDEN.
+        "hidden": (options.get(HIDDEN) or "").strip().lower() in TRUTHY,
+        "key_mgmt": key_mgmt,
+    }
 
 
 def comment_out(text, stanza, marker="# ekklesia-classroom: "):
@@ -160,3 +231,31 @@ def comment_out(text, stanza, marker="# ekklesia-classroom: "):
         break
 
     return "".join(lines)
+
+
+def uncomment(text, marker="# ekklesia-classroom: "):
+    """Undo comment_out, in the file as it stands now.
+
+    Returns (text, count). The rollback uses this rather than copying the
+    backup over the top, because the backup is kept once and never
+    refreshed: an administrator who corrected a static Ethernet address
+    between the migration and the rollback would have had that correction
+    silently reverted by a whole-file restore. This touches only the
+    lines this package marked, so everything else in the file — wired
+    stanzas, source lines, an administrator's own comments — is exactly
+    as the machine left it.
+    """
+    lines = (text or "").splitlines(keepends=True)
+    restored = 0
+
+    for number, line in enumerate(lines):
+        stripped = line.lstrip()
+
+        if not stripped.startswith(marker):
+            continue
+
+        indent = line[:len(line) - len(stripped)]
+        lines[number] = indent + stripped[len(marker):]
+        restored += 1
+
+    return "".join(lines), restored

scripts/classroom-wifi

@@ -77,6 +77,11 @@ def nmcli(*args, check=True):
     result = subprocess.run(
         ["nmcli"] + list(args), capture_output=True, text=True, timeout=60,
         check=False,
+        # nmcli translates its output and its terse field separator rules
+        # are the only thing here that is parsed. C is the one locale in
+        # which what this reads back is what nmcli is documented to
+        # print, on a machine in any country.
+        env={**os.environ, "LC_ALL": "C"},
     )
 
     if check and result.returncode != 0:
@@ -86,14 +91,57 @@ def nmcli(*args, check=True):
     return result
 
 
+def terse(line):
+    """One line of `nmcli -t` output, split into its fields.
+
+    nmcli escapes a colon inside a value as `\\:` and a backslash as
+    `\\\\`, so splitting on ":" is wrong for any network whose name
+    contains one. Everything here that compares a connection name to a
+    field goes through this, because getting it wrong does not produce an
+    error — it produces a migration that decides it failed and rolls a
+    working network back.
+    """
+    fields = []
+    current = []
+    escaped = False
+
+    for character in line:
+        if escaped:
+            current.append(character)
+            escaped = False
+        elif character == "\\":
+            escaped = True
+        elif character == ":":
+            fields.append("".join(current))
+            current = []
+        else:
+            current.append(character)
+
+    fields.append("".join(current))
+
+    return fields
+
+
+def connections():
+    """Every saved connection, as (name, uuid, type)."""
+    result = nmcli("-t", "-f", "NAME,UUID,TYPE", "connection", "show",
+                   check=False)
+
+    found = []
+
+    for line in (result.stdout or "").splitlines():
+        fields = terse(line)
+
+        if len(fields) >= 3:
+            found.append((fields[0], fields[1], fields[2]))
+
+    return found
+
+
 def listing():
-    result = nmcli("-t", "-f", "NAME,TYPE,UUID", "connection", "show")
+    for name, _uuid, kind in connections():
-
+        if "wireless" in kind:
-    for line in result.stdout.splitlines():
+            print(f"  {name}")
-        parts = line.split(":")
-
-        if len(parts) >= 2 and "wireless" in parts[1]:
-            print(f"  {parts[0]}")
 
     return 0
 
@@ -105,7 +153,7 @@ def forget(name):
     return 0
 
 
-def add(ssid, password, name=None):
+def add(ssid, password, name=None, hidden=False):
     name = name or ssid
 
     # Deliberately no connection.permissions. That single omission is the
@@ -121,6 +169,15 @@ def add(ssid, password, name=None):
         # Stored in the connection file, which is root-only, rather than
         # in a login keyring that belongs to whoever is signed in.
         "802-11-wireless-security.psk-flags", "0",
+        # WHETHER THE ACCESS POINT BROADCASTS ITS NAME.
+        #
+        # Always passed, in both directions, rather than only when it is
+        # true. NetworkManager's default is "no", and a default that
+        # happens to be right is not the same as an answer: this is the
+        # setting the prayer-room laptop's migration left at the default
+        # and could therefore never associate. Written down either way so
+        # that the connection file says what was meant.
+        "802-11-wireless.hidden", "yes" if hidden else "no",
     ]
 
     if password:
@@ -131,9 +188,13 @@ def add(ssid, password, name=None):
 
     nmcli(*arguments)
 
-    path = os.path.join(CONNECTIONS, f"{name}.nmconnection")
+    if hidden:
+        say(f"{name} is a hidden network; NetworkManager will probe for it "
+            "by name")
 
-    if os.path.exists(path):
+    path = connection_file(name)
+
+    if path and os.path.exists(path):
         # NetworkManager already writes 0600 root:root. Said out loud and
         # enforced anyway, because this is the file the whole arrangement
         # rests on.
@@ -148,6 +209,28 @@ def add(ssid, password, name=None):
     return 0
 
 
+def connection_file(name):
+    """Where NetworkManager actually put a connection, or None.
+
+    Asked rather than constructed. NetworkManager escapes characters in
+    the filename it derives from a connection name, so
+    f"{name}.nmconnection" is a guess that happens to be right for a
+    one-word SSID and wrong for "ROGC Classroom" — which is the network
+    this command exists to migrate, and the file whose permissions the
+    whole arrangement rests on.
+    """
+    result = nmcli("-t", "-f", "NAME,FILENAME", "connection", "show",
+                   check=False)
+
+    for line in (result.stdout or "").splitlines():
+        fields = terse(line)
+
+        if len(fields) >= 2 and fields[0] == name and fields[-1]:
+            return fields[-1]
+
+    return None
+
+
 def run(*command):
     """Run a command, tolerating one that is not installed.
 
@@ -198,14 +281,71 @@ def carrying(name, device):
                    "--active", check=False)
 
     for line in (result.stdout or "").splitlines():
-        parts = line.rsplit(":", 1)
+        fields = terse(line)
 
-        if len(parts) == 2 and parts[0] == name and parts[1] == device:
+        if len(fields) == 2 and fields[0] == name and fields[1] == device:
             return True
 
     return False
 
 
+def device_state(device):
+    """NetworkManager's word for what it is doing with this interface."""
+    result = nmcli("-t", "-f", "DEVICE,STATE", "device", "status", check=False)
+
+    for line in (result.stdout or "").splitlines():
+        fields = terse(line)
+
+        if len(fields) >= 2 and fields[0] == device:
+            return fields[1]
+
+    return "unknown"
+
+
+def unmanaged_by_config(device):
+    """Configuration files that name this device as unmanaged.
+
+    Separate from the ifupdown plugin, and the reason the prayer-room
+    laptop still needed `nmcli device set wlp2s0 managed yes` by hand
+    after its stanza was gone. A keyfile saying `unmanaged-devices` is
+    not undone by commenting out an interfaces stanza, and a hand-over
+    that does not notice it hands the interface to nobody.
+    """
+    naming = []
+    candidates = ["/etc/NetworkManager/NetworkManager.conf"]
+
+    for directory in ("/etc/NetworkManager/conf.d",
+                      "/usr/lib/NetworkManager/conf.d",
+                      "/run/NetworkManager/conf.d"):
+        try:
+            candidates += sorted(
+                os.path.join(directory, entry)
+                for entry in os.listdir(directory)
+                if entry.endswith(".conf")
+            )
+        except OSError:
+            continue
+
+    for path in candidates:
+        try:
+            with open(path, "r", encoding="utf-8", errors="replace") as handle:
+                text = handle.read()
+        except OSError:
+            continue
+
+        for line in text.splitlines():
+            stripped = line.strip()
+
+            if not stripped.startswith("unmanaged-devices"):
+                continue
+
+            if device in stripped or "except:" in stripped:
+                naming.append(path)
+                break
+
+    return naming
+
+
 def find_stanza():
     """The one wireless stanza ifupdown holds, or None, plus the file."""
     if not os.path.exists(INTERFACES):
@@ -249,20 +389,33 @@ def check():
         return 0
 
     device = stanza["name"]
-    ssid, _ = ifupdown.credentials(stanza)
+    found = ifupdown.profile(stanza)
 
     say(f"NOTE: {device} is configured in {INTERFACES}, so ifupdown owns "
         "it and")
     say("      NetworkManager will report it unmanaged — the room's panel")
     say("      will say there is no connection and will not find networks.")
 
-    if ssid:
+    if found and found["hidden"]:
+        say("      Its access point does not broadcast its name, which "
+            "NetworkManager")
+        say("      has to be told; --adopt carries that across.")
+
+    for path in unmanaged_by_config(device):
+        say(f"      {path} also names this device unmanaged; --adopt "
+            "reports it")
+        say("      rather than editing it.")
+
+    if found is None:
+        say("      Its credentials are not in that file, so it has to be "
+            "moved by hand.")
+    elif found["key_mgmt"] is None:
+        say("      Its security is not a pre-shared key, so it has to be "
+            "moved by hand.")
+    else:
         say("      To move it, while watching, over SSH:")
         say("        classroom-wifi --adopt --dry-run")
         say("        classroom-wifi --adopt")
-    else:
-        say("      Its credentials are not in that file, so it has to be "
-            "moved by hand.")
 
     return 0
 
@@ -312,17 +465,37 @@ def adopt(dry_run=False):
         return 0
 
     device = stanza["name"]
-    ssid, psk = ifupdown.credentials(stanza)
+    found = ifupdown.profile(stanza)
 
-    if not ssid:
+    if found is None:
         say(f"{device} is configured through a wpa_supplicant.conf rather "
             "than named in this file")
         say("that one is not ours to move; adopt it by hand")
         return 1
 
+    if found["key_mgmt"] is None:
+        say(f"{device} uses a security this command does not recreate "
+            f"({stanza['options'].get('wpa-key-mgmt')})")
+        say("refusing to guess; a network moved wrong is a network that "
+            "cannot come back")
+        return 1
+
+    ssid = found["ssid"]
+    psk = found["psk"]
+
     # Never printed. The room's password is not diagnostic output.
     say(f"{device}: ifupdown holds a wireless network "
-        f"({'with' if psk else 'without'} a saved password)")
+        f"({'with' if psk else 'without'} a saved password"
+        f"{', hidden' if found['hidden'] else ''})")
+
+    # A keyfile naming this device unmanaged survives the stanza going
+    # away, so the hand-over would end with the interface belonging to
+    # nobody. Said before anything is touched, and not edited: a file an
+    # administrator wrote is not this command's to rewrite.
+    for path in unmanaged_by_config(device):
+        say(f"WARNING: {path} names {device} unmanaged")
+        say("         NetworkManager will not keep it after a restart "
+            "until that is changed")
 
     # Refuse to start a hand-over that cannot be undone. ifdown releases
     # the interface and ifup is the whole of the rollback; without them
@@ -342,17 +515,32 @@ def adopt(dry_run=False):
                 "run would refuse")
 
         say(f"would create a NetworkManager system connection for {ssid!r}")
-        say(f"would comment the {device} stanza out of {INTERFACES}")
+        say("  802-11-wireless.hidden: "
+            f"{'yes' if found['hidden'] else 'no'}")
+        say(f"  802-11-wireless-security.key-mgmt: {found['key_mgmt']}")
+        say(f"would run `ifdown {device}` while its stanza is still in "
+            f"{INTERFACES}")
+        say(f"would then comment that stanza out of {INTERFACES}")
         say("would hand the interface over, and put it back if that failed")
         return 0
 
-    existing = nmcli("-t", "-f", "NAME", "connection", "show", check=False)
+    already = any(name == ssid for name, _uuid, _kind in connections())
-    already = ssid in (existing.stdout or "").splitlines()
 
     if already:
+        # Reused as it stands. An existing connection of this name may be
+        # a teacher's own, or one a previous attempt left behind with the
+        # wrong hidden setting — which is exactly the state the
+        # prayer-room laptop was in — so it is corrected rather than
+        # trusted. Every setting the migration decides is written again.
         say(f"NetworkManager already has a connection named {ssid!r}")
+        say("correcting it to match what ifupdown holds")
+        nmcli("connection", "modify", ssid,
+              "802-11-wireless.hidden", "yes" if found["hidden"] else "no",
+              "connection.autoconnect", "yes",
+              "802-11-wireless-security.psk-flags", "0",
+              check=False)
     else:
-        if add(ssid, psk) != 0:
+        if add(ssid, psk, hidden=found["hidden"]) != 0:
             return 1
 
     backup = f"{INTERFACES}.before-ekklesia-classroom"
@@ -366,20 +554,51 @@ def adopt(dry_run=False):
     # timeout, an interrupt — the file goes back and ifupdown gets the
     # interface again. Nothing below may raise past this.
     try:
+        # RELEASE THE INTERFACE BEFORE TAKING ITS STANZA AWAY.
+        #
+        # ifdown reads /etc/network/interfaces to find out how to bring
+        # an interface down. Run after the stanza is commented out it
+        # reports "Ignoring unknown interface" and does nothing at all —
+        # so ifupdown's own wpa_supplicant and DHCP client were still
+        # running, still holding the radio, when NetworkManager was asked
+        # to associate with it. Two supplicants on one interface is a
+        # fight NetworkManager loses, and it is the second half of why
+        # the prayer-room laptop rolled back.
+        #
+        # The order is therefore: down first, while the file still says
+        # how; rewrite the file second. The window in which the machine
+        # is offline is the same either way.
+        result = run("ifdown", device)
+
+        if result is not None and result.returncode != 0:
+            # Not fatal. A machine whose interface is already down is a
+            # perfectly good starting point, and ifdown says so with a
+            # non-zero status.
+            say(f"ifdown {device}: "
+                f"{(result.stderr or result.stdout or '').strip()}")
+
+        say(f"released {device} from ifupdown")
+
         with open(INTERFACES, "w", encoding="utf-8") as handle:
             handle.write(ifupdown.comment_out(text, stanza))
 
-        say(f"took {device} out of ifupdown's hands")
+        say(f"took {device} out of ifupdown's hands in {INTERFACES}")
 
         # NetworkManager only re-reads which interfaces ifupdown claims
-        # when its configuration is reloaded. The link is still
+        # when its configuration is reloaded.
-        # ifupdown's at this point, and NetworkManager does not touch a
-        # device it considers unmanaged, so this cannot drop it.
         nmcli("general", "reload", "conf", check=False)
-
-        run("ifdown", device)
         nmcli("device", "set", device, "managed", "yes", check=False)
-        nmcli("connection", "up", ssid, check=False)
+
+        # On the device, by name. `connection up <name>` alone lets
+        # NetworkManager choose an interface, and a machine with two
+        # radios could bring the network up on the wrong one — which
+        # `carrying` would then correctly report as a failure, and the
+        # migration would roll back a network that was working.
+        started = nmcli("connection", "up", ssid, "ifname", device,
+                        check=False)
+
+        if started.returncode != 0:
+            say(f"nmcli: {(started.stderr or '').strip()}")
 
         deadline = time.monotonic() + ADOPT_TIMEOUT
 
@@ -387,12 +606,15 @@ def adopt(dry_run=False):
             if carrying(ssid, device) and online():
                 say(f"{device} is NetworkManager's now, and the room is "
                     "online")
+                say(f"NetworkManager reports {device} "
+                    f"{device_state(device)}")
                 say("the panel will show the connection at the next login")
                 return 0
 
             time.sleep(3)
 
-        say("NetworkManager did not get the network back")
+        say("NetworkManager did not get the network back "
+            f"({device} is {device_state(device)})")
     except BaseException as error:  # noqa: BLE001 - see the comment above
         say(f"the migration failed part way through: {error}")
 
@@ -403,6 +625,48 @@ def adopt(dry_run=False):
     return 1
 
 
+def put_interfaces_back():
+    """Undo this command's edit to /etc/network/interfaces.
+
+    Un-commenting the marked lines where they are, rather than copying
+    the backup over the whole file. The backup is written once, on the
+    first attempt, and never refreshed — so a whole-file restore silently
+    reverts anything an administrator changed about the WIRED network in
+    between. The marker names exactly the lines this command commented
+    out, and nothing else in the file is touched.
+
+    The backup is still the fallback, for a file that has been replaced
+    since rather than edited.
+    """
+    backup = f"{INTERFACES}.before-ekklesia-classroom"
+
+    try:
+        with open(INTERFACES, "r", encoding="utf-8") as handle:
+            current = handle.read()
+    except OSError as error:
+        say(f"cannot read {INTERFACES}: {error}")
+        current = None
+
+    if current is not None:
+        restored, count = ifupdown.uncomment(current)
+
+        if count:
+            with open(INTERFACES, "w", encoding="utf-8") as handle:
+                handle.write(restored)
+
+            say(f"put {count} line{'' if count == 1 else 's'} back in "
+                f"{INTERFACES}, and changed nothing else in it")
+            return True
+
+    if os.path.exists(backup):
+        shutil.copy2(backup, INTERFACES)
+        say(f"no marked lines to restore; copied {backup} back instead")
+        return True
+
+    say(f"nothing of this command's is in {INTERFACES} to undo")
+    return False
+
+
 def restore(device, ssid=None):
     """Put the interface back under ifupdown.
 
@@ -410,11 +674,7 @@ def restore(device, ssid=None):
     decide what that means for an exit code: for a failed migration it is
     still a failure, and for `--rollback` it is a success.
     """
-    backup = f"{INTERFACES}.before-ekklesia-classroom"
+    put_interfaces_back()
-
-    if os.path.exists(backup):
-        shutil.copy2(backup, INTERFACES)
-        say(f"restored {INTERFACES} from {backup}")
 
     nmcli("general", "reload", "conf", check=False)
 
@@ -423,7 +683,12 @@ def restore(device, ssid=None):
 
     if device:
         nmcli("device", "set", device, "managed", "no", check=False)
-        run("ifup", device)
+        result = run("ifup", device)
+
+        if result is not None and result.returncode != 0:
+            say(f"ifup {device}: "
+                f"{(result.stderr or result.stdout or '').strip()}")
+
         say(f"ifupdown has {device} again")
 
     for _ in range(10):
@@ -433,6 +698,44 @@ def restore(device, ssid=None):
 
         time.sleep(3)
 
+    # THE ADAPTER IS NEVER LEFT BELONGING TO NOBODY.
+    #
+    # Above, the device has deliberately been handed back to ifupdown and
+    # NetworkManager told to leave it alone. That is right while ifupdown
+    # can actually bring it up. When ifup has failed too, the same two
+    # lines are a wireless adapter that neither manager owns: no
+    # connection, and a panel that cannot even scan for one. That is the
+    # state the prayer-room laptop was found in after 0.2.5, and it is
+    # the state that made `nmcli device set wlp2s0 managed yes` a manual
+    # recovery step.
+    #
+    # So the last resort is to give it to NetworkManager rather than to
+    # leave it with neither. A room able to pick its network from the
+    # panel is recoverable by somebody standing in front of it; an
+    # unmanaged radio is not.
+    if device:
+        say("WARNING: ifupdown did not bring the network back either")
+        say(f"         handing {device} to NetworkManager so the adapter "
+            "is not left unmanaged")
+
+        nmcli("device", "set", device, "managed", "yes", check=False)
+
+        if ssid:
+            nmcli("connection", "up", ssid, "ifname", device, check=False)
+
+        for _ in range(5):
+            if online():
+                say("the room is on the network, through NetworkManager")
+                say(f"NOTE: {INTERFACES} still describes {device}, so "
+                    "ifupdown will")
+                say("      claim it again at the next boot. Decide which "
+                    "one owns it.")
+                return True
+
+            time.sleep(3)
+
+        say(f"         {device} is {device_state(device)}")
+
     say("WARNING: the machine may be offline. The original "
         f"{INTERFACES} is restored; try `ifup {device}` or a reboot")
     return False
@@ -449,6 +752,8 @@ def main():
                         help="remove a saved connection")
     parser.add_argument("--open", action="store_true",
                         help="the network has no password")
+    parser.add_argument("--hidden", action="store_true",
+                        help="the access point does not broadcast its name")
     parser.add_argument("--adopt", action="store_true",
                         help="move an installer-configured Wi-Fi from "
                              "ifupdown into NetworkManager")
@@ -494,7 +799,8 @@ def main():
             say("no password given")
             return 1
 
-    return add(arguments.ssid, password, arguments.name)
+    return add(arguments.ssid, password, arguments.name,
+               hidden=arguments.hidden)
 
 
 if __name__ == "__main__":

tests/test-classroom-wifi-adopt.sh

@@ -0,0 +1,411 @@
+#!/bin/bash
+# The Wi-Fi hand-over, driven end to end against stubbed tools.
+#
+# tests/test-classroom-network-adopt.sh covers the parsing. This covers
+# the part that actually failed on the prayer-room laptop during the
+# 0.2.5 setup: the ORDER of the hand-over, and what is carried across
+# with the network.
+#
+# The laptop's own configuration was
+#
+#     allow-hotplug wlp2s0
+#     iface wlp2s0 inet dhcp
+#         wpa-ssid "ROGC Classroom"
+#         wpa-psk  <secret>
+#         wpa-scan-ssid 1
+#
+# and the migration produced a NetworkManager connection with
+# 802-11-wireless.hidden: no, which can never associate with an access
+# point that does not broadcast its name. It also ran `ifdown` after the
+# stanza had been commented out, where ifdown has nothing left to read
+# and does nothing, so ifupdown's own supplicant was still holding the
+# radio. Then the rollback left the adapter unmanaged.
+#
+# nmcli, ifup, ifdown and curl are stubs that record what they were
+# asked and can be told to fail, so the sequence is asserted rather than
+# described. No real interface, no real NetworkManager, no laptop.
+set -uo pipefail
+
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+
+python3 - "$ROOT" <<'PY'
+import importlib.machinery
+import importlib.util
+import os
+import shutil
+import sys
+import tempfile
+from pathlib import Path
+
+root = Path(sys.argv[1])
+
+passed = failed = 0
+
+
+def check(label, condition, detail=""):
+    global passed, failed
+    if condition:
+        print(f"  ok   {label}")
+        passed += 1
+    else:
+        print(f"  FAIL {label}")
+        if detail:
+            print(f"       {detail}")
+        failed += 1
+
+
+def load():
+    """classroom-wifi as a module. It has no .py, so name the loader."""
+    path = root / "scripts" / "classroom-wifi"
+    loader = importlib.machinery.SourceFileLoader("classroom_wifi", str(path))
+    spec = importlib.util.spec_from_loader("classroom_wifi", loader)
+    module = importlib.util.module_from_spec(spec)
+    loader.exec_module(module)
+    return module
+
+
+# The laptop's file, with the hidden network and an SSID with a space in
+# it. Also a wired stanza, which nothing here may touch.
+LAPTOP = """# This file describes the network interfaces available on your system
+source /etc/network/interfaces.d/*
+
+auto lo
+iface lo inet loopback
+
+auto enp3s0
+iface enp3s0 inet static
+\taddress 10.40.0.90/24
+\tgateway 10.40.0.1
+
+allow-hotplug wlp2s0
+iface wlp2s0 inet dhcp
+\twpa-ssid "ROGC Classroom"
+\twpa-psk  s#cret-with-a-hash
+\twpa-scan-ssid 1
+"""
+
+BROADCAST = """allow-hotplug wlp2s0
+iface wlp2s0 inet dhcp
+\twpa-ssid Sanctuary
+\twpa-psk  ordinary-secret
+"""
+
+ENTERPRISE = """allow-hotplug wlp2s0
+iface wlp2s0 inet dhcp
+\twpa-ssid Campus
+\twpa-key-mgmt WPA-EAP
+"""
+
+STUB = r"""#!/bin/bash
+# Records the call, and the state of the interfaces file at the moment of
+# it, so the test can assert ordering rather than trust it.
+printf '%s' "$(basename "$0")" >> "$LOG"
+for argument in "$@"; do printf ' %s' "$argument" >> "$LOG"; done
+printf '\n' >> "$LOG"
+
+if [[ -f "$INTERFACES" ]]; then
+    cp "$INTERFACES" "$SNAPSHOTS/$(basename "$0").$$.interfaces"
+fi
+
+case "$(basename "$0")" in
+nmcli)
+    case "$*" in
+    *"-f NAME,UUID,TYPE connection show"*) cat "$FAKE_CONNECTIONS" ;;
+    *"-f NAME,FILENAME connection show"*)  cat "$FAKE_FILENAMES" ;;
+    *"connection show --active"*)          cat "$FAKE_ACTIVE" ;;
+    *"device status"*)                     cat "$FAKE_DEVICES" ;;
+    *"networking connectivity"*)           cat "$FAKE_CONNECTIVITY" ;;
+    *"connection up"*)                     exit "${UP_STATUS:-0}" ;;
+    esac
+    exit 0
+    ;;
+ifdown) exit "${IFDOWN_STATUS:-0}" ;;
+ifup)   exit "${IFUP_STATUS:-0}" ;;
+curl)   exit "${CURL_STATUS:-0}" ;;
+esac
+exit 0
+"""
+
+
+class Rig:
+    """A machine mid-migration, with every external tool stubbed."""
+
+    def __init__(self, interfaces_text, **environment):
+        self.directory = Path(tempfile.mkdtemp(prefix="classroom-wifi-"))
+        self.bin = self.directory / "bin"
+        self.snapshots = self.directory / "snapshots"
+        self.bin.mkdir()
+        self.snapshots.mkdir()
+
+        self.interfaces = self.directory / "interfaces"
+        self.interfaces.write_text(interfaces_text)
+
+        self.log = self.directory / "calls.log"
+        self.log.write_text("")
+
+        for tool in ("nmcli", "ifdown", "ifup", "curl"):
+            target = self.bin / tool
+            target.write_text(STUB)
+            target.chmod(0o755)
+
+        # What the stubbed nmcli answers with. Written as files so a test
+        # can change an answer partway through if it needs to.
+        self.answers = {
+            "FAKE_CONNECTIONS": "",
+            "FAKE_FILENAMES": "",
+            "FAKE_ACTIVE": "ROGC Classroom:wlp2s0\n",
+            "FAKE_DEVICES": "wlp2s0:connected\n",
+            "FAKE_CONNECTIVITY": "full\n",
+        }
+
+        self.environment = {
+            "LOG": str(self.log),
+            "INTERFACES": str(self.interfaces),
+            "SNAPSHOTS": str(self.snapshots),
+        }
+
+        for key, value in self.answers.items():
+            path = self.directory / key
+            path.write_text(value)
+            self.environment[key] = str(path)
+
+        self.environment.update({k: str(v) for k, v in environment.items()})
+
+    def answer(self, key, value):
+        Path(self.environment[key]).write_text(value)
+
+    def run(self, action, **kwargs):
+        saved_path = os.environ.get("PATH", "")
+        saved = {k: os.environ.get(k) for k in self.environment}
+        os.environ["PATH"] = f"{self.bin}:{saved_path}"
+        os.environ.update(self.environment)
+
+        module = load()
+        module.INTERFACES = str(self.interfaces)
+        # The poll loops are real time; nothing here needs to wait.
+        module.time.sleep = lambda _seconds: None
+        module.ADOPT_TIMEOUT = 0.2
+
+        try:
+            return getattr(module, action)(**kwargs), module
+        finally:
+            os.environ["PATH"] = saved_path
+            for key, value in saved.items():
+                if value is None:
+                    os.environ.pop(key, None)
+                else:
+                    os.environ[key] = value
+
+    @property
+    def calls(self):
+        return self.log.read_text().splitlines()
+
+    def snapshot_at(self, tool):
+        """The interfaces file as it was when `tool` ran."""
+        for path in sorted(self.snapshots.glob(f"{tool}.*.interfaces")):
+            return path.read_text()
+        return None
+
+    def clean(self):
+        shutil.rmtree(self.directory, ignore_errors=True)
+
+
+print("1. the hidden network survives the hand-over")
+# The 0.2.5 regression itself. wpa-scan-ssid 1 was sitting in the parsed
+# options and the only accessor threw it away.
+rig = Rig(LAPTOP)
+status, module = rig.run("adopt")
+added = [c for c in rig.calls if c.startswith("nmcli connection add")]
+check("the migration reports success", status == 0, str(status))
+check("a connection is created", len(added) == 1, str(rig.calls))
+check("and it is told the access point does not broadcast its name",
+      added and "802-11-wireless.hidden yes" in added[0],
+      added[0] if added else "no connection was added")
+check("the SSID keeps its space",
+      added and "ssid ROGC Classroom" in added[0], added[0] if added else "")
+check("it is still a system connection, with no permissions",
+      added and "connection.permissions" not in added[0])
+check("the secret is stored in the connection file",
+      added and "802-11-wireless-security.psk-flags 0" in added[0])
+check("the password is not truncated at the # inside it",
+      added and "wifi-sec.psk s#cret-with-a-hash" in added[0],
+      added[0] if added else "")
+rig.clean()
+
+print("2. the interface is released BEFORE its stanza is taken away")
+# ifdown reads /etc/network/interfaces to know how to bring an interface
+# down. Run after the stanza is commented out it reports "Ignoring
+# unknown interface" and does nothing, so ifupdown's wpa_supplicant kept
+# the radio and NetworkManager could not have it.
+rig = Rig(LAPTOP)
+rig.run("adopt")
+order = [c.split()[0] for c in rig.calls]
+at_ifdown = rig.snapshot_at("ifdown")
+check("ifdown is called at all", "ifdown" in order, str(order))
+check("the stanza is still live in the file when ifdown runs",
+      at_ifdown is not None
+      and "\niface wlp2s0 inet dhcp\n" in at_ifdown,
+      "ifdown ran against a file whose stanza was already commented out")
+check("and the file is commented out afterwards",
+      "# ekklesia-classroom: iface wlp2s0" in rig.interfaces.read_text())
+check("NetworkManager is only told to manage it after the file changed",
+      order.index("ifdown") < len(order) - 1)
+managed = [c for c in rig.calls if "device set" in c]
+check("the device is explicitly handed to NetworkManager",
+      any("managed yes" in c for c in managed), str(managed))
+up = [c for c in rig.calls if "connection up" in c]
+check("the connection is brought up on that interface by name",
+      up and "ifname wlp2s0" in up[0], str(up))
+rig.clean()
+
+print("3. an ordinary broadcast network is not marked hidden")
+# The flag has to be an answer in both directions, not something only
+# ever switched on.
+rig = Rig(BROADCAST)
+rig.run("adopt")
+added = [c for c in rig.calls if c.startswith("nmcli connection add")]
+check("hidden is written as no", added and "802-11-wireless.hidden no" in added[0],
+      added[0] if added else "")
+rig.clean()
+
+print("4. a failed hand-over puts the file back, and only the file")
+# The backup is written once and never refreshed, so a whole-file restore
+# would silently revert a wired change made in between. The rollback
+# un-comments its own marker instead.
+rig = Rig(LAPTOP, UP_STATUS="1", FAKE_CONNECTIVITY="none\n", CURL_STATUS="1")
+status, _ = rig.run("adopt")
+after = rig.interfaces.read_text()
+check("the migration reports failure", status == 1, str(status))
+check("the wireless stanza is live again",
+      "\niface wlp2s0 inet dhcp\n" in after, after)
+check("the allow-hotplug line came back too",
+      "\nallow-hotplug wlp2s0\n" in after)
+check("nothing of the marker is left behind",
+      "ekklesia-classroom:" not in after, after)
+check("the file is byte for byte what it was", after == LAPTOP,
+      "restored file differs from the original")
+check("the wired stanza was never touched",
+      "\taddress 10.40.0.90/24\n" in after)
+rig.clean()
+
+print("5. a rollback preserves an unrelated change made since")
+# The case a whole-file copy gets wrong: the wired address is corrected
+# after the migration, and then the migration fails.
+rig = Rig(LAPTOP, UP_STATUS="1", FAKE_CONNECTIVITY="none\n", CURL_STATUS="1")
+
+
+def edit_between(module):
+    original = module.put_interfaces_back
+
+    def wrapped():
+        text = rig.interfaces.read_text()
+        rig.interfaces.write_text(
+            text.replace("10.40.0.90/24", "10.40.0.91/24"))
+        return original()
+
+    module.put_interfaces_back = wrapped
+
+
+saved_run = Rig.run
+
+
+def run_with_edit(self, action, **kwargs):
+    saved_path = os.environ.get("PATH", "")
+    os.environ["PATH"] = f"{self.bin}:{saved_path}"
+    os.environ.update(self.environment)
+    module = load()
+    module.INTERFACES = str(self.interfaces)
+    module.time.sleep = lambda _seconds: None
+    module.ADOPT_TIMEOUT = 0.2
+    edit_between(module)
+    try:
+        return getattr(module, action)(**kwargs), module
+    finally:
+        os.environ["PATH"] = saved_path
+
+
+run_with_edit(rig, "adopt")
+after = rig.interfaces.read_text()
+check("the corrected wired address survived the rollback",
+      "10.40.0.91/24" in after, after)
+check("and the wireless stanza is live again",
+      "\niface wlp2s0 inet dhcp\n" in after)
+rig.clean()
+
+print("6. a failed hand-over never leaves the adapter unmanaged")
+# The state the prayer-room laptop was found in: ifupdown had been told
+# to take the interface back, ifup failed too, and NetworkManager had
+# been told to leave it alone. Nobody owned the radio, and recovering it
+# needed `nmcli device set wlp2s0 managed yes` typed by hand.
+rig = Rig(LAPTOP, UP_STATUS="1", IFUP_STATUS="1",
+          FAKE_CONNECTIVITY="none\n", CURL_STATUS="1")
+status, _ = rig.run("adopt")
+# Only what the rollback did. The hand-over itself sets managed yes on
+# the way in, which is not the question here.
+rollback_calls = rig.calls[
+    max(index for index, call in enumerate(rig.calls)
+        if call.startswith("ifup ")):]
+sets = [c for c in rollback_calls if "device set" in c]
+check("ifupdown is offered the interface first",
+      any("managed no" in c for c in rig.calls), str(rig.calls))
+check("and when ifup fails too it is given to NetworkManager",
+      sets and "managed yes" in sets[-1],
+      "the last word on the adapter was 'managed no', which owns nothing")
+check("so the adapter is never left belonging to nobody",
+      "managed yes" in rig.calls[-1] or any("managed yes" in c
+                                            for c in sets),
+      str(rollback_calls))
+check("and the migration still reports failure", status == 1, str(status))
+rig.clean()
+
+print("7. a network this cannot recreate is refused before anything moves")
+# WPA-Enterprise has no pre-shared key to carry across. Guessing would
+# take the interface away from ifupdown to find that out.
+rig = Rig(ENTERPRISE)
+status, _ = rig.run("adopt")
+check("it refuses", status == 1, str(status))
+check("nothing was added", not any("connection add" in c for c in rig.calls),
+      str(rig.calls))
+check("the file is untouched", rig.interfaces.read_text() == ENTERPRISE)
+check("ifdown was never run", not any(c.startswith("ifdown")
+                                      for c in rig.calls), str(rig.calls))
+rig.clean()
+
+print("8. a machine already migrated is not migrated again")
+# The prayer-room laptop, after tomorrow. Its stanza is gone, so there is
+# nothing to adopt and --adopt must be a no-op rather than an error.
+MIGRATED = LAPTOP.replace("allow-hotplug wlp2s0",
+                          "# ekklesia-classroom: allow-hotplug wlp2s0")
+MIGRATED = "\n".join(
+    ("# ekklesia-classroom: " + line)
+    if line.startswith("iface wlp2s0") or line.startswith("\twpa-")
+    else line
+    for line in MIGRATED.splitlines()
+) + "\n"
+rig = Rig(MIGRATED)
+status, _ = rig.run("adopt")
+check("it reports nothing to do, and succeeds", status == 0, str(status))
+check("no connection was created",
+      not any("connection add" in c for c in rig.calls), str(rig.calls))
+check("nothing was brought down", not any(c.startswith("ifdown")
+                                          for c in rig.calls))
+check("the file is untouched", rig.interfaces.read_text() == MIGRATED)
+rig.clean()
+
+print("9. --check changes nothing at all")
+rig = Rig(LAPTOP)
+status, _ = rig.run("check")
+check("it succeeds", status == 0)
+check("the file is untouched", rig.interfaces.read_text() == LAPTOP)
+check("it runs no command that could change anything",
+      not any(c.startswith(("ifup", "ifdown")) for c in rig.calls),
+      str(rig.calls))
+check("and no nmcli that writes",
+      not any("connection add" in c or "connection modify" in c
+              or "device set" in c for c in rig.calls), str(rig.calls))
+rig.clean()
+
+print()
+print(f"{passed} passed, {failed} failed")
+raise SystemExit(1 if failed else 0)
+PY