Re-running configure.py keeps every admin host (#891)

configure.py

@@ -178,19 +178,41 @@ def collect_answers():
     print("your nick, which anyone can take while you are offline. Log into")
     print("services and set +x, then /whois yourself - you want the host it")
     print("shows, ending in something like .users.undernet.org.")
-    current_hostmasks = _current("ADMIN_HOSTMASKS")
-    default_host = (current_hostmasks[0].rsplit("@", 1)[-1]
-                    if current_hostmasks and current_hostmasks[0] else "")
-    suffix = f" [{default_host}]" if default_host else ""
-    admin_host = input(f"Your services host (blank to skip){suffix}: ").strip() or default_host
-    while admin_host:
+    # A RE-RUN KEEPS WHAT IS THERE (#891). This used to take the first
+    # configured entry as the prompt's default and, on a blank answer,
+    # write [that one] back - so an operator with two hosts (home and
+    # phone) who pressed Enter here lost the second, silently: _current()'s
+    # own promise is that re-running never does that. It also indexed the
+    # value directly, so a comma-separated string setting offered its first
+    # CHARACTER, and a wildcard first entry was run through the validator
+    # below as if the operator had just typed it. The hosts are read the
+    # way the console itself reads them (either form, deduplicated, host
+    # part only), a blank answer writes nothing, and only a typed host is
+    # written.
+    current_hosts = adminchat.admin_host_patterns()
+    if current_hosts:
+        print(f"  Configured now: {', '.join(current_hosts)}")
+        prompt = "Your services host (blank keeps what is configured): "
+    else:
+        prompt = "Your services host (blank to skip): "
+    while True:
+        admin_host = input(prompt).strip()
+        if not admin_host:
+            break
         problem = settings_file.admin_host_problem(admin_host)
         if not problem:
             break
         print(f"  That will not do: {problem}.")
-        admin_host = input("Your services host (blank to skip): ").strip()
-    if admin_host:
-        changes["ADMIN_HOSTMASKS"] = [f"*!*@{admin_host}"]
+    if admin_host and admin_host.lower() not in current_hosts:
+        if len(current_hosts) > 1:
+            # Several were set up by hand; one question cannot know which of
+            # them a new host replaces, so it is added and the others stay.
+            changes["ADMIN_HOSTMASKS"] = ([f"*!*@{host}" for host in current_hosts]
+                                          + [f"*!*@{admin_host}"])
+            print(f"  Added beside the {len(current_hosts)} already configured. To remove "
+                  f"one, edit ADMIN_HOSTMASKS in settings.conf.")
+        else:
+            changes["ADMIN_HOSTMASKS"] = [f"*!*@{admin_host}"]
 
     print()
     print("Admin console password (for the DCC CHAT console - see")

docs/INSTALL.md

@@ -106,7 +106,7 @@ On Windows that command is **`py configure.py`**. A python.org install gives you
 2. **IRC server** (`irc.undernet.org` unless you say otherwise).
 3. **Channel(s)**, comma-separated.
 4. **Admin nick** - who may run `!ban`, `!rehash`, `!update`, `!clearqueue`.
-5. **Your services host**, optional - blank skips it. Locks the admin console (and the in-channel admin commands, once this is set) to your account rather than just your nick, which anyone can take while you are offline; see [ADMIN-CONSOLE.md](ADMIN-CONSOLE.md#how-the-host-proves-your-login) for how to read it off `/whois`.
+5. **Your services host**, optional - blank skips it. On a re-run it shows the hosts already configured and blank keeps every one of them; a host you type replaces a single one, or is added beside them when there are several. Locks the admin console (and the in-channel admin commands, once this is set) to your account rather than just your nick, which anyone can take while you are offline; see [ADMIN-CONSOLE.md](ADMIN-CONSOLE.md#how-the-host-proves-your-login) for how to read it off `/whois`.
 6. **Admin console password**, typed twice and never shown; only its hash is written.
 7. **Music directory** - optional here (see below); if the folder does not exist it offers to create it.
 8. **Web dashboard, yes or no** (off unless you say yes). A yes asks two more: whether it should be reachable from other devices on your LAN, and - if Flask is not installed - whether to install it now.

docs/UPDATES-PUBLIC.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- **Fixed: re-running `configure.py` could remove all but one of your admin hosts.** If you had set up more than one services host (home and phone, say), pressing Enter at the services-host question kept only the first, and the others silently lost admin access. Enter now keeps every host that is configured - they are shown to you - and a new host is added beside several rather than replacing them.
 - **Added (mIRC console): the @DCCore window's button now lights up like a channel's.** It turns the message colour (red, by default) when there is new activity - a request, a send, a search - and the highlight colour on a failed transfer, so failures stand out. The five-minute status line and joins, parts and bans do not light it, as they would not in a channel. Needs mIRC 7 or later; update `dccore.mrc` and reload it.
 - **Fixed: pasting a batch of requests could mute you and then ban you for an hour, and the mute notice wrongly said your queue had been cleared.** Asking for a file counted toward the flood protection exactly like a search, so pasting about a dozen rows from the list - one album, the ordinary way these lists are used - could get the eleventh line muted and the twelfth banned for an hour, on any client or connection that sends a paste quickly. Asking for files is no longer rate-limited at all: the limit is your queue, as it always should have been (100 files each by default), and past it you are told once rather than once per line. Searches and the other commands are unchanged. The mute notice was also wrong in a way that caused the ban - what it drops is the bot's pending replies to you, never your queued files - so it now says that other commands are ignored for the next 30 seconds and that your queued files are safe.
 - **Fixed: pasting several lines from the list at once could be answered "Busy looking up other files - try again in a moment".** Requesting a file meant searching the library for it every single time - even for a file just sent, and even for the file next to it in the same album - and only two of those searches could run at once, so most of a pasted batch was refused outright (and "try again in a moment" is the one thing that risks tripping the flood protection). The bot now remembers where it found a file and which folders it has been finding them in, so a batch from one album costs one search instead of nine, and a request that arrives while the library is busy waits its turn instead of being turned away. What it remembers is dropped on `!rehash`, so changing your library folders takes effect for requests straight away rather than a few minutes later.

docs/UPDATES.md

@@ -4,6 +4,24 @@ All version changes, optimizations, and bug fixes made over time in the DCCore p
 
 ## 🟨 Unreleased
 
+### 🔑 Re-running configure.py keeps every admin host (#891)
+
+Found reviewing #811. The services-host question took the **first** configured `ADMIN_HOSTMASKS` entry as its
+default and, on a blank answer, wrote `[that one]` back - so an operator with two hosts (home and phone) who
+re-ran `configure.py` and pressed Enter lost the second, silently, and the phone could no longer run admin
+commands. `configure._current()`'s own promise is that re-running never does that. It also indexed the value
+directly, so a comma-separated string setting offered its first *character*, and a wildcard first entry was put
+through the validator as if just typed (*"That will not do: '\*.home.net' has a '\*' in it"* about a value
+nobody entered).
+
+The hosts are now read the way the console reads them - `adminchat.admin_host_patterns()`, either form,
+deduplicated, host part only - and shown (*Configured now: ...*). A blank answer writes nothing; a host already
+there changes nothing; a new host replaces a single configured one, as before, or is **added** beside several,
+since one question cannot know which of several a new host replaces - and says how to remove one.
+`tests/test_a_rerun_of_configure_keeps_every_admin_host.py` (8), through `collect_answers()` with canned input;
+six fail on the old code, the other two are the unchanged cases. INSTALL.md's question list says what a re-run
+does.
+
 ### 🔴 The @DCCore window lights up like a channel: red on activity, highlight on a failure (#892)
 
 Asked by the operator: every other mIRC window's button turns red when something new is said in it, and `@DCCore`

tests/test_a_rerun_of_configure_keeps_every_admin_host.py

@@ -0,0 +1,129 @@
+"""Re-running configure.py keeps every admin host that is configured (#891).
+
+#811 added a services-host question. On a re-run it took the FIRST
+configured entry as its default and, on a blank answer, wrote [that one]
+back - so an operator with two hosts (home and phone) who pressed Enter
+lost the second without a word, and the phone could no longer run admin
+commands. configure._current()'s own promise is that re-running never
+silently changes what is configured. It also indexed the value directly,
+so a comma-separated string setting offered its first character, and a
+wildcard first entry was put through the validator as if just typed.
+
+Driven through collect_answers() with canned input, as #811's own tests are.
+"""
+
+import contextlib
+import io
+import os
+import sys
+import unittest
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)
+
+import adminchat  # noqa: E402
+import configure  # noqa: E402
+
+from tests.support import DCCoreTestCase  # noqa: E402
+
+HOME = "me.users.undernet.org"
+PHONE = "my-phone.example.net"
+
+
+class ARerun(DCCoreTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.tree = self.make_tree()
+        real = adminchat._read_password
+        self.addCleanup(setattr, adminchat, "_read_password", real)
+
+    def run_with_host_answers(self, *host_answers):
+        """The whole question flow; `host_answers` are what is typed at the
+        services-host prompt (more than one when the first is refused)."""
+        answers = iter(["MyBot", "", "#my-channel", "MyAdmin", *host_answers,
+                        self.tree.music, "n"])
+        passwords = iter(["secret123", "secret123"])
+        adminchat._read_password = lambda prompt: next(passwords)
+        import builtins
+        real_input = builtins.input
+        builtins.input = lambda prompt="": next(answers)
+        said = io.StringIO()
+        try:
+            with contextlib.redirect_stdout(said):
+                changes, _hash = configure.collect_answers()
+        finally:
+            builtins.input = real_input
+        return changes, said.getvalue()
+
+    def test_two_hosts_and_a_blank_answer_writes_nothing(self):
+        """The reported case."""
+        self.set_config(ADMIN_HOSTMASKS=["*!*@" + HOME, "*!*@" + PHONE])
+
+        changes, said = self.run_with_host_answers("")
+
+        self.assertNotIn("ADMIN_HOSTMASKS", changes,
+                         "a blank answer rewrote the hosts - the second one would be lost")
+        self.assertIn("Configured now: %s, %s" % (HOME, PHONE), said)
+
+    def test_one_host_and_a_blank_answer_writes_nothing_either(self):
+        self.set_config(ADMIN_HOSTMASKS=["*!*@" + HOME])
+
+        changes, _said = self.run_with_host_answers("")
+
+        self.assertNotIn("ADMIN_HOSTMASKS", changes)
+
+    def test_typing_a_host_that_is_already_there_changes_nothing(self):
+        self.set_config(ADMIN_HOSTMASKS=["*!*@" + HOME, "*!*@" + PHONE])
+
+        changes, _said = self.run_with_host_answers(PHONE.upper())
+
+        self.assertNotIn("ADMIN_HOSTMASKS", changes)
+
+    def test_a_new_host_beside_several_is_added_and_they_are_kept(self):
+        self.set_config(ADMIN_HOSTMASKS=["*!*@" + HOME, "*!*@" + PHONE])
+
+        changes, said = self.run_with_host_answers("laptop.users.undernet.org")
+
+        self.assertEqual(changes["ADMIN_HOSTMASKS"],
+                         ["*!*@" + HOME, "*!*@" + PHONE, "*!*@laptop.users.undernet.org"])
+        self.assertIn("Added beside the 2 already configured", said)
+
+    def test_a_new_host_replacing_the_only_one_is_written_as_before(self):
+        self.set_config(ADMIN_HOSTMASKS=["*!*@" + HOME])
+
+        changes, _said = self.run_with_host_answers("new.users.undernet.org")
+
+        self.assertEqual(changes["ADMIN_HOSTMASKS"], ["*!*@new.users.undernet.org"])
+
+    def test_a_wildcard_entry_is_not_put_through_the_validator_on_a_blank(self):
+        """It used to be the prompt's default, so Enter printed "That will
+        not do: '*.home.net' has a '*' in it" about a value nobody typed."""
+        self.set_config(ADMIN_HOSTMASKS=["*!*@*.home.net"])
+
+        changes, said = self.run_with_host_answers("")
+
+        self.assertNotIn("That will not do", said)
+        self.assertNotIn("ADMIN_HOSTMASKS", changes)
+
+    def test_a_comma_separated_setting_is_read_as_hosts_not_characters(self):
+        self.set_config(ADMIN_HOSTMASKS="*!*@%s,*!*@%s" % (HOME, PHONE))
+
+        changes, said = self.run_with_host_answers("")
+
+        self.assertIn("Configured now: %s, %s" % (HOME, PHONE), said)
+        self.assertNotIn("ADMIN_HOSTMASKS", changes)
+
+    def test_nothing_configured_still_behaves_as_811_wrote_it(self):
+        self.set_config(ADMIN_HOSTMASKS=[])
+
+        blank, _said = self.run_with_host_answers("")
+        typed, _said = self.run_with_host_answers("myaccount.users.undernet.org")
+
+        self.assertNotIn("ADMIN_HOSTMASKS", blank)
+        self.assertEqual(typed["ADMIN_HOSTMASKS"], ["*!*@myaccount.users.undernet.org"])
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_the_console_guide_says_what_configure_does.py

@@ -45,8 +45,12 @@ def test_a_prompt_asks_for_the_services_host_right_after_admin_nick(self):
         self.assertIn('changes["ADMIN_HOSTMASKS"]', between)
 
     def test_a_blank_answer_writes_nothing(self):
+        """The write is guarded on a typed answer. Since #891 it is also
+        skipped for a host already configured, and a blank answer on a
+        re-run keeps every configured host - that is executed, not read, in
+        test_a_rerun_of_configure_keeps_every_admin_host.py."""
         code = read("configure.py")
-        self.assertIn('if admin_host:\n        changes["ADMIN_HOSTMASKS"]', code)
+        self.assertIn('if admin_host and admin_host.lower() not in current_hosts:', code)
 
 
 class TheGuideSaysSo(unittest.TestCase):

tests/test_the_guide_lists_what_configure_asks.py

@@ -35,7 +35,7 @@ def read(relative):
     ('_ask("IRC server"', "IRC server"),
     ('_ask("Channel(s), comma-separated"', "Channel(s)"),
     ('_ask("Admin nick', "Admin nick"),
-    ('input(f"Your services host (blank to skip){suffix}: ")', "services host"),
+    ('prompt = "Your services host (blank keeps what is configured): "', "services host"),  # #891
     ('_read_password("Password: ")', "password"),
     ('input(f"Music directory (full path)', "Music directory"),
     ('input("Enable it? [y/N]: ")', "Web dashboard"),