diff --git a/controller/api/captive_portal.py b/controller/api/captive_portal.py index fd4cf8e2..cccc1398 100644 --- a/controller/api/captive_portal.py +++ b/controller/api/captive_portal.py @@ -14,6 +14,7 @@ from fastapi.templating import Jinja2Templates from controller.lib.admin import get_hotspot_device, get_hotspot_ssid from controller.lib.captive_portal import ( + PORTAL_CONTINUE_PATH, PORTAL_PATH, PROBE_PATHS, PROBE_SUCCESS_RESPONSES, @@ -34,21 +35,35 @@ templates = Jinja2Templates(directory=str(Path(__file__).parent.parent / "templa NO_STORE = {"Cache-Control": "no-store"} -@router.get(PORTAL_PATH, include_in_schema=False) -async def portal(request: Request): - """The landing page: this WROLPi's address and links to WROLPi and the Controller.""" +def _render_portal(request: Request, connected: bool): if is_docker_mode(): raise HTTPException(status_code=404) ip = get_hotspot_ip(get_hotspot_device()) if not ip: # Hotspot is down; a page with the wrong address is worse than none. raise HTTPException(status_code=404) - # The phone has seen the address; let its next probe succeed so it offers "Done". - acknowledge_client(request.client.host if request.client else None) - context = {"ip": ip, "ssid": get_hotspot_ssid()} + context = {"ip": ip, "ssid": get_hotspot_ssid(), "connected": connected, + "continue_path": PORTAL_CONTINUE_PATH} return templates.TemplateResponse(request, "portal.html", context, headers=NO_STORE) +@router.get(PORTAL_PATH, include_in_schema=False) +async def portal(request: Request): + """The welcome page: this WROLPi's address and a Continue button.""" + return _render_portal(request, connected=False) + + +@router.get(PORTAL_CONTINUE_PATH, include_in_schema=False) +async def portal_continue(request: Request): + """ + The page after Continue. The phone re-probes after this navigation; the client is now + acknowledged so that probe succeeds and the sheet offers "Done". + """ + response = _render_portal(request, connected=True) # 404s first if the hotspot is down + acknowledge_client(request.client.host if request.client else None) + return response + + async def probe(request: Request): """ A connectivity probe. Anything but the expected reply makes the phone show the portal, so diff --git a/controller/lib/captive_portal.py b/controller/lib/captive_portal.py index 6f3fe1d7..cf615135 100644 --- a/controller/lib/captive_portal.py +++ b/controller/lib/captive_portal.py @@ -19,6 +19,7 @@ PROBE_HOSTS = ( 'captive.g.aaplimg.com', # the CDN name captive.apple.com aliases to 'connectivitycheck.gstatic.com', # Android 'connectivitycheck.android.com', # Android (older) + 'clients3.google.com', # Android (older) 'www.msftconnecttest.com', # Windows 10+ 'www.msftncsi.com', # Windows 7/8 'detectportal.firefox.com', # Firefox @@ -55,6 +56,9 @@ INCLUDE_FILE = DNSMASQ_SHARED_DIR / '50-wrolpi-captive-portal.conf' DEFAULT_HOTSPOT_IP = '10.42.0.1' PORTAL_PATH = '/portal' +# A full-page navigation the user makes from the portal. Phones only re-probe (and offer +# "Done") after a navigation, so the client is acknowledged here, never on viewing the page. +PORTAL_CONTINUE_PATH = '/portal/continue' def is_captive_portal_enabled() -> bool: @@ -135,13 +139,13 @@ def dnsmasq_config_ip() -> Optional[str]: return None -# Clients (by hotspot IP) which have loaded the portal page. Their later probes get the success -# reply. Reset whenever the hotspot starts so a new session shows the page again. +# Clients (by hotspot IP) which tapped Continue on the portal page. Their later probes get the +# success reply. Reset whenever the hotspot starts so a new session shows the page again. _acknowledged_clients: set[str] = set() def acknowledge_client(ip: str): - """The client at `ip` has seen the portal page; its probes now say "online".""" + """The client at `ip` tapped Continue on the portal page; its probes now say "online".""" if ip: _acknowledged_clients.add(ip) diff --git a/controller/templates/portal.html b/controller/templates/portal.html index 141d45bd..abe11d61 100644 --- a/controller/templates/portal.html +++ b/controller/templates/portal.html @@ -49,18 +49,28 @@ + {% if connected %} +

You are connected

+

Tap Done in the corner of this window to keep using the + {{ ssid }} hotspot.

+ {% else %}

This is your WROLPi

You are connected to the {{ ssid }} hotspot.

+ {% endif %}

The address of this WROLPi is

{{ ip }}
+ {% if connected %} Open WROLPi Open the Controller + {% else %} + {# A real navigation: phones only re-check the network (and offer "Done") after one. #} + Continue + {% endif %}
-

Tap Done to stay connected, then open your browser to - https://{{ ip }}.

+

Open your browser to https://{{ ip }} to use WROLPi.

Your browser will warn about the certificate the first time. That is expected for an offline WROLPi; continue to the site.

This hotspot does not provide Internet access.

diff --git a/controller/test/test_captive_portal.py b/controller/test/test_captive_portal.py index a001bfea..58a64443 100644 --- a/controller/test/test_captive_portal.py +++ b/controller/test/test_captive_portal.py @@ -12,6 +12,7 @@ from controller.lib.captive_portal import ( DEFAULT_HOTSPOT_IP, FRIENDLY_NAME, PROBE_HOSTS, + PORTAL_CONTINUE_PATH, PROBE_PATHS, PROBE_SUCCESS_RESPONSES, acknowledge_client, @@ -169,12 +170,37 @@ class TestProbeRedirects: class TestAcknowledgedClients: - """Once a client has seen the portal page, its probes say "online" so the OS offers Done.""" + """Once a client taps Continue, its probes say "online" so the OS offers Done.""" - @pytest.mark.parametrize("path", PROBE_PATHS) - def test_probe_succeeds_after_portal_seen(self, test_client, path): + def test_viewing_the_portal_does_not_acknowledge(self, test_client): + """iOS fetches the page in the background before showing it; only a tap counts.""" with mock.patch("controller.api.captive_portal.get_hotspot_ip", return_value='10.42.0.1'): assert test_client.get('/portal').status_code == 200 + assert not is_client_acknowledged('testclient') + assert test_client.get('/hotspot-detect.html', follow_redirects=False).status_code == 302 + + def test_continue_page(self, test_client): + with mock.patch("controller.api.captive_portal.get_hotspot_ip", return_value='10.42.0.1'): + response = test_client.get(PORTAL_CONTINUE_PATH) + assert response.status_code == 200 + assert response.headers['cache-control'] == 'no-store' + assert 'Done' in response.text + assert '10.42.0.1' in response.text + assert 'https://10.42.0.1/' in response.text + assert is_client_acknowledged('testclient') + + def test_continue_404_when_hotspot_down(self, test_client): + with mock.patch("controller.api.captive_portal.get_hotspot_ip", return_value=None): + assert test_client.get(PORTAL_CONTINUE_PATH).status_code == 404 + assert not is_client_acknowledged('testclient') + + def test_continue_404_in_docker(self, test_client_docker_mode): + assert test_client_docker_mode.get(PORTAL_CONTINUE_PATH).status_code == 404 + + @pytest.mark.parametrize("path", PROBE_PATHS) + def test_probe_succeeds_after_continue(self, test_client, path): + with mock.patch("controller.api.captive_portal.get_hotspot_ip", return_value='10.42.0.1'): + assert test_client.get(PORTAL_CONTINUE_PATH).status_code == 200 response = test_client.get(path, follow_redirects=False) status, media_type, body = PROBE_SUCCESS_RESPONSES[path] assert response.status_code == status @@ -190,12 +216,6 @@ class TestAcknowledgedClients: assert response.status_code == 200 assert 'Success' in response.text - def test_portal_404_does_not_acknowledge(self, test_client): - """A hotspot-down 404 is not the client seeing the address.""" - with mock.patch("controller.api.captive_portal.get_hotspot_ip", return_value=None): - assert test_client.get('/portal').status_code == 404 - assert test_client.get('/generate_204', follow_redirects=False).status_code == 302 - def test_other_clients_still_redirected(self, test_client): acknowledge_client('10.42.0.99') assert not is_client_acknowledged('testclient') @@ -220,8 +240,7 @@ class TestPortalPage: assert response.headers['cache-control'] == 'no-store' body = response.text assert '10.42.0.1' in body - assert 'https://10.42.0.1/' in body - assert 'https://10.42.0.1/controller/' in body + assert f'href="{PORTAL_CONTINUE_PATH}"' in body assert 'WROLPi' in body # the SSID def test_404_when_hotspot_down(self, test_client):