wireguard: Improve reliability functional tests by waiting after ops

Tests:

- Functional tests run much more reliably. Although there were several failures
still observed due to following reasons:

  - Due to FreedomBox service receiving SIGSEGV (presumably during NM DBus
  operations).

  - Error something like Invalid UTF-8: setting not known during removal of
  a client.

  - Unknown failures caused by newly added clients not showing up on the page.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

freedombox/modules/wireguard/tests/test_functional.py

@@ -73,23 +73,35 @@ class TestWireguardApp(functional.BaseAppTests):
             with functional.wait_for_page_update(browser):
                 start_server_button.first.click()
 
-        browser.find_by_css('.btn-add-client').first.click()
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-add-client').first.click()
+
         browser.find_by_id('id_public_key').fill(key)
         functional.submit(browser, form_class='form-add-client')
 
     def _edit_client(self, browser, key1, key2):
         """Edit a client"""
         functional.nav_to_module(browser, 'wireguard')
-        browser.links.find_by_href(self._get_client_href(key1)).first.click()
+        with functional.wait_for_page_update(browser):
-        browser.find_by_css('.btn-edit-client').first.click()
+            browser.links.find_by_href(
+                self._get_client_href(key1)).first.click()
+
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-edit-client').first.click()
+
         browser.find_by_id('id_public_key').fill(key2)
         functional.submit(browser, form_class='form-edit-client')
 
     def _delete_client(self, browser, key):
         """Delete a client"""
         functional.nav_to_module(browser, 'wireguard')
-        browser.links.find_by_href(self._get_client_href(key)).first.click()
+        with functional.wait_for_page_update(browser):
-        browser.find_by_css('.btn-delete-client').first.click()
+            browser.links.find_by_href(
+                self._get_client_href(key)).first.click()
+
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-delete-client').first.click()
+
         functional.submit(browser, form_class='form-delete-client')
 
     def test_add_edit_delete_client(self, session_browser):
@@ -124,37 +136,38 @@ class TestWireguardApp(functional.BaseAppTests):
             with functional.wait_for_page_update(session_browser):
                 start_server_button.first.click()
 
-        session_browser.find_by_css('.btn-auto-add-client').first.click()
+        with functional.wait_for_page_update(session_browser):
+            session_browser.find_by_css('.btn-auto-add-client').first.click()
 
         client_pubkey = session_browser.find_by_css(
-                '.pubkey-val').first.text.strip()
+            '.pubkey-val').first.text.strip()
 
         # Verify private key reveal
-        privkey_reveal = session_browser.find_by_css(
+        privkey_reveal = session_browser.find_by_css('.privkey-val')
-                '.privkey-val')
         assert privkey_reveal, "Private key reveal should be present"
         privkey_reveal.click()
         client_privkey = session_browser.find_by_css(
-                '.privkey-val').text.splitlines()[1]
+            '.privkey-val').text.splitlines()[1]
         assert len(client_privkey) == 44, (("Private key should be base64 "
                                             "(44 chars)"))
 
         # Verify config download and QR links
         download_link = session_browser.links.find_by_href(
-                '/freedombox/apps/wireguard/client/auto-add/action/download/')
+            '/freedombox/apps/wireguard/client/auto-add/action/download/')
         qr_link = session_browser.links.find_by_href(
-                '/freedombox/apps/wireguard/client/auto-add/action/qr/')
+            '/freedombox/apps/wireguard/client/auto-add/action/qr/')
         assert download_link, "Download config link should exist"
         assert qr_link, "QR code link should exist"
 
         # Submit to add the client
         with functional.wait_for_page_update(session_browser):
             session_browser.find_by_css(
-                    '.btn-auto-add-connection').first.click()
+                '.btn-auto-add-connection').first.click()
 
         # Verify client was added successfully
-        assert self._client_exists(session_browser, client_pubkey), ((
+        assert self._client_exists(
-            "Auto-generated client should exist"))
+            session_browser,
+            client_pubkey), (("Auto-generated client should exist"))
 
         # Clean up
         self._delete_client(session_browser, client_pubkey)
@@ -179,8 +192,10 @@ class TestWireguardApp(functional.BaseAppTests):
             return browser.find_by_css(f'tr.{key} > td').first.text
 
         functional.nav_to_module(browser, 'wireguard')
-        href = self._get_server_href(browser, config['peer_public_key'])
+        with functional.wait_for_page_update(browser):
-        href.first.click()
+            href = self._get_server_href(browser, config['peer_public_key'])
+            href.first.click()
+
         assert get_value('peer-endpoint') == config['peer_endpoint']
         assert get_value('peer-public-key') == config['peer_public_key']
         assert get_value('server-ip-address-and-network'
@@ -192,7 +207,9 @@ class TestWireguardApp(functional.BaseAppTests):
     def _add_server(browser, config):
         """Add a server."""
         functional.nav_to_module(browser, 'wireguard')
-        browser.find_by_css('.btn-add-server').first.click()
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-add-server').first.click()
+
         browser.find_by_id('id_peer_endpoint').fill(config['peer_endpoint'])
         browser.find_by_id('id_peer_public_key').fill(
             config['peer_public_key'])
@@ -205,9 +222,13 @@ class TestWireguardApp(functional.BaseAppTests):
     def _edit_server(self, browser, config1, config2):
         """Edit a server."""
         functional.nav_to_module(browser, 'wireguard')
-        self._get_server_href(browser,
+        with functional.wait_for_page_update(browser):
-                              config1['peer_public_key']).first.click()
+            href = self._get_server_href(browser, config1['peer_public_key'])
-        browser.find_by_css('.btn-edit-server').first.click()
+            href.first.click()
+
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-edit-server').first.click()
+
         browser.find_by_id('id_peer_endpoint').fill(config2['peer_endpoint'])
         browser.find_by_id('id_peer_public_key').fill(
             config2['peer_public_key'])
@@ -220,8 +241,13 @@ class TestWireguardApp(functional.BaseAppTests):
     def _delete_server(self, browser, config):
         """Delete a server"""
         functional.nav_to_module(browser, 'wireguard')
-        self._get_server_href(browser, config['peer_public_key']).first.click()
+        with functional.wait_for_page_update(browser):
-        browser.find_by_css('.btn-delete-server').first.click()
+            href = self._get_server_href(browser, config['peer_public_key'])
+            href.first.click()
+
+        with functional.wait_for_page_update(browser):
+            browser.find_by_css('.btn-delete-server').first.click()
+
         functional.submit(browser, form_class='form-delete-server')
 
     def test_add_edit_delete_server(self, session_browser):

freedombox/modules/wireguard/utils.py

@@ -79,8 +79,7 @@ def get_nm_info():
         settings_ipv6 = connection.get_setting_ip6_config()
 
         ip_address, ip_address_and_network = _get_nm_address_info(
-                settings_ipv4, settings_ipv6
+            settings_ipv4, settings_ipv6)
-        )
 
         info['ip_address'] = ip_address
         info['ip_address_and_network'] = ip_address_and_network
@@ -162,7 +161,11 @@ def delete_connections():
         connection.delete()
 
 
-def _get_public_key_from_private_key(private_key):
+def _get_public_key_from_private_key(private_key: str | None) -> str | None:
+    """Return public key from private key running wg command."""
+    if not private_key:
+        return None
+
     process = subprocess.run(['wg', 'pubkey'], check=True, capture_output=True,
                              input=private_key.encode())
     return process.stdout.decode().strip()
@@ -207,6 +210,28 @@ def add_server(settings):
 
     network.add_connection(settings)
 
+    # Wait upto 10 seconds for server to appear
+    public_key = settings['wireguard']['peer_public_key']
+    for _ in range(10):
+        info = get_info()
+        if info['my_client'] and info['my_client']['servers']:
+            found = False
+            for _, server in info['my_client']['servers'].items():
+                for _, peer in server['peers'].items():
+                    if peer['public_key'] == public_key:
+                        found = True
+                        break
+
+                if found:
+                    break
+
+            if found:
+                break
+
+        time.sleep(1)
+    else:
+        logging.warning('Could not add server')
+
 
 def edit_server(interface, settings):
     """Edit information for connecting to a server."""
@@ -220,6 +245,22 @@ def edit_server(interface, settings):
     network.reactivate_connection(connection.get_uuid())
 
 
+def delete_server(interface):
+    """Delete information for connecting to a server."""
+    connection = network.get_connection_by_interface_name(interface)
+    network.delete_connection(connection.get_uuid())
+
+    # Wait upto 10 seconds for server to disappear
+    for _ in range(10):
+        info = get_nm_info()
+        if interface not in info:
+            break
+
+        time.sleep(1)
+    else:
+        logging.warning('Could not delete server')
+
+
 def setup_server():
     """Setup a server connection that clients can connect to."""
     app = app_module.App.get('wireguard')
@@ -249,6 +290,16 @@ def setup_server():
     network.add_connection(settings)
     logger.info('Created new WireGuard server connection')
 
+    # Wait upto 10 seconds for server to appear
+    for _ in range(10):
+        info = get_info()
+        if info['my_server'] and info['my_server']['public_key']:
+            break
+
+        time.sleep(1)
+    else:
+        logging.warning('Could not add server')
+
 
 def _get_next_available_ip_address(settings):
     """Get the next available IP address to allocate to a client."""
@@ -311,6 +362,23 @@ def add_client(public_key):
     connection.commit_changes(True)
     network.reactivate_connection(connection.get_uuid())
 
+    # Wait upto 10 seconds for client to appear
+    for _ in range(10):
+        info = get_info()
+        if info['my_server'] and info['my_server']['peers']:
+            found = False
+            for _, peer in info['my_server']['peers'].items():
+                if peer['public_key'] == public_key:
+                    found = True
+                    break
+
+            if found:
+                break
+
+        time.sleep(1)
+    else:
+        logging.warning('Could not add client %s', public_key)
+
 
 def remove_client(public_key):
     """Remove permission for a client to connect our server."""
@@ -325,6 +393,23 @@ def remove_client(public_key):
     connection.commit_changes(True)
     network.reactivate_connection(connection.get_uuid())
 
+    # Wait upto 10 seconds for client to disappear
+    for _ in range(10):
+        info = get_info()
+        if info['my_server']:
+            found = False
+            for _, peer in info['my_server']['peers'].items():
+                if peer['public_key'] == public_key:
+                    found = True
+                    break
+
+            if not found:
+                break
+
+        time.sleep(1)
+    else:
+        logging.warning('Could not remove client %s', public_key)
+
 
 def build_client_config(client_ip: str, client_privkey: str,
                         client_pubkey: str, endpoint: str) -> str:

freedombox/modules/wireguard/views.py

@@ -3,8 +3,8 @@
 Views for WireGuard application.
 """
 
-from io import BytesIO
 import urllib.parse
+from io import BytesIO
 
 from django.contrib import messages
 from django.contrib.messages.views import SuccessMessageMixin
@@ -14,7 +14,6 @@ from django.urls import reverse_lazy
 from django.utils.translation import gettext as _
 from django.views.generic import FormView, TemplateView, View
 
-from freedombox import network
 from freedombox.modules.names.components import DomainName
 from freedombox.views import AppView
 
@@ -381,8 +380,7 @@ class DeleteServerView(SuccessMessageMixin, TemplateView):
 
     def post(self, request, interface):
         """Delete the server."""
-        connection = network.get_connection_by_interface_name(interface)
+        utils.delete_server(interface)
-        network.delete_connection(connection.get_uuid())
         messages.success(request, _('Server deleted.'))
         return redirect('wireguard:index')