Gate the USB kiosk the same way, and make an unreachable setting a test failure
The USB kiosk had a feature flag and nothing else. A flag is a decision, not a credential: with the kiosk on, anyone who could reach the app could move a drive between checked-out and checked-in and sign it with a badge they guessed. Same reasoning that closed the parts kiosk, smaller blast radius - which is why Darkwing did not raise it and why it was worth asking about anyway. Both kiosks now share one mechanism. The device token moved out of the printedparts api wrapper into a request interceptor that attaches X-Kiosk-Token by URL prefix, with one localStorage key per kiosk - they are separate screens in separate rooms, and a shared token would mean provisioning one grants the other. claimKioskToken() is shared too, so both views take the token from a ?token= URL once and strip it from the address bar identically. Then the part that matters more than either fix. I told you to set the new settings in the UI, and that was impossible: every plugin settings page carries a hardcoded key list, and nothing had added them. printedparts_kiosk_enabled, printedparts_kiosk_token and usb_kiosk_token were declared, seeded and read correctly, and rendered by no page - changeable only by writing SQL. tests/test_plugin_settings_reachable.py now fails when any declared setting appears nowhere in its plugin's frontend. It found seven MORE that were already in that state, all pre-existing: collector_corporate_ranges computers_configpolicies_enabled computers_machinelink_alerts computers_quietreporthours geenforce_reportstaleminutes printers_dashboardpercent warranty_expiringdays All seven now have fields. I first parked them in a KNOWN_GAPS ratchet, then fixed them and deleted the ratchet - a backlog list in a new check is a way of agreeing the check does not apply yet. There is no backlog; the check is absolute. Two of my own mistakes on the way, both caught by tests rather than by reading: the blanket rename of the USB guard also rewrote the call inside _kiosk_open, so it called itself until the stack ran out; and the first version of the reachable check skipped all 17 plugins because it looked for get_settings() when the method is get_settings_defaults() - it passed by testing nothing, which is the exact shape it exists to catch. Suite 2336, frontend 316, build clean. Scope: product
This commit is contained in:
cproudlock
88
tests/test_plugin_settings_reachable.py
Normal file
88
tests/test_plugin_settings_reachable.py
Normal file
@@ -0,0 +1,88 @@
|
||||
"""Every setting a plugin declares must be reachable in its settings page.
|
||||
|
||||
A setting exists so a site can change it. One that is seeded into the database
|
||||
but rendered by no page is not configurable - it is a constant with extra steps,
|
||||
and the only way to change it is direct SQL.
|
||||
|
||||
This was written after shipping exactly that twice in one afternoon:
|
||||
printedparts_kiosk_enabled / _kiosk_token and then usb_kiosk_token were declared
|
||||
in get_settings(), seeded correctly, and read correctly by the API - and none of
|
||||
them appeared on any page, because each settings view carries a hardcoded KEYS
|
||||
list that nobody remembered to extend. The instructions for turning the kiosk
|
||||
back on were impossible to follow.
|
||||
|
||||
The check is deliberately crude: it greps the plugin's frontend for the key as a
|
||||
literal. That is enough to catch the failure it exists for, and it cannot be
|
||||
satisfied by accident.
|
||||
"""
|
||||
|
||||
import pathlib
|
||||
import re
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
REPO = pathlib.Path(__file__).resolve().parents[1]
|
||||
PLUGINS = REPO / 'plugins'
|
||||
|
||||
# Settings a site is not meant to edit through a page. Each needs a reason.
|
||||
EXEMPT = {
|
||||
# Written by the app itself when the agent key is minted, not typed.
|
||||
'plugin.endpoints.agentkeyprovisioned',
|
||||
'plugin.endpoints.envkeyfallback',
|
||||
}
|
||||
|
||||
|
||||
|
||||
def _declared_settings(plugin_dir):
|
||||
"""Keys the plugin declares in get_settings_defaults(), read from the source.
|
||||
|
||||
Importing the plugin would need an app context and a database; the
|
||||
declaration is a literal list, so reading it is enough and much cheaper.
|
||||
"""
|
||||
source = (plugin_dir / 'plugin.py')
|
||||
if not source.is_file():
|
||||
return []
|
||||
text = source.read_text(encoding='utf-8')
|
||||
match = re.search(r'def get_settings_defaults\(self\).*?(?=\n def |\Z)', text, re.S)
|
||||
if not match:
|
||||
return []
|
||||
return re.findall(r"'key':\s*'([^']+)'", match.group(0))
|
||||
|
||||
|
||||
def _frontend_text(plugin_dir):
|
||||
frontend = plugin_dir / 'frontend'
|
||||
if not frontend.is_dir():
|
||||
return ''
|
||||
parts = []
|
||||
for path in frontend.rglob('*'):
|
||||
if path.suffix in ('.vue', '.js') and path.is_file():
|
||||
parts.append(path.read_text(encoding='utf-8'))
|
||||
return '\n'.join(parts)
|
||||
|
||||
|
||||
PLUGIN_DIRS = sorted(
|
||||
d for d in PLUGINS.iterdir()
|
||||
if d.is_dir() and (d / 'manifest.json').is_file()
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('plugin_dir', PLUGIN_DIRS, ids=lambda d: d.name)
|
||||
def test_every_declared_setting_is_reachable_in_the_ui(plugin_dir):
|
||||
declared = [k for k in _declared_settings(plugin_dir) if k not in EXEMPT]
|
||||
if not declared:
|
||||
pytest.skip('declares no settings')
|
||||
|
||||
frontend = _frontend_text(plugin_dir)
|
||||
if not frontend:
|
||||
pytest.skip('ships no frontend')
|
||||
|
||||
missing = [key for key in declared if key not in frontend]
|
||||
assert not missing, (
|
||||
'%s declares %d setting(s) that appear nowhere in its frontend, so a '
|
||||
'site cannot change them without writing SQL:\n %s\n'
|
||||
'Add them to the plugin settings view (its KEYS list AND a field), or '
|
||||
'to EXEMPT in this file with a reason. There is no backlog list: the '
|
||||
'seven this check found when it was written were all fixed, and it '
|
||||
'stays that way.'
|
||||
% (plugin_dir.name, len(missing), '\n '.join(missing)))
|
||||
@@ -16,6 +16,12 @@ import pytest
|
||||
from shopdb.core.models import Setting
|
||||
|
||||
|
||||
KIOSK_TOKEN = 'usb-kiosk-device-token-for-tests'
|
||||
# What a provisioned screen sends. Turning the kiosk on is a decision; this is
|
||||
# the credential.
|
||||
KIOSK_HEADERS = {'X-Kiosk-Token': KIOSK_TOKEN}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def kiosk(db):
|
||||
"""Self-hosted USB, kiosk ON, one employee and one available drive."""
|
||||
@@ -26,6 +32,8 @@ def kiosk(db):
|
||||
valuetype='string', category='site'))
|
||||
db.session.add(Setting(key='usb_kiosk_enabled', value='true',
|
||||
valuetype='boolean', category='usb'))
|
||||
db.session.add(Setting(key='usb_kiosk_token', value=KIOSK_TOKEN,
|
||||
valuetype='string', category='usb'))
|
||||
db.session.add(DirectoryEmployee(sso=270015376, firstname='Dana',
|
||||
lastname='Reyes'))
|
||||
db.session.add(DirectoryEmployee(sso=270099999, firstname='Sam',
|
||||
@@ -48,12 +56,12 @@ def _setting(db, key, value):
|
||||
|
||||
def _out(client, badge='270015376', **extra):
|
||||
return client.post('/api/usb/kiosk/checkout',
|
||||
json=dict({'device_id': 'USB-1', 'badge': badge}, **extra))
|
||||
json=dict({'device_id': 'USB-1', 'badge': badge}, **extra), headers=KIOSK_HEADERS)
|
||||
|
||||
|
||||
def _in(client, badge='270015376', **extra):
|
||||
return client.post('/api/usb/kiosk/checkin',
|
||||
json=dict({'device_id': 'USB-1', 'badge': badge}, **extra))
|
||||
json=dict({'device_id': 'USB-1', 'badge': badge}, **extra), headers=KIOSK_HEADERS)
|
||||
|
||||
|
||||
# --- the door itself --------------------------------------------------------
|
||||
@@ -67,7 +75,7 @@ def test_the_kiosk_does_not_exist_until_a_site_turns_it_on(client, kiosk):
|
||||
"""
|
||||
_setting(kiosk, 'usb_kiosk_enabled', 'false')
|
||||
|
||||
assert client.get('/api/usb/kiosk/device/USB-1').status_code == 404
|
||||
assert client.get('/api/usb/kiosk/device/USB-1', headers=KIOSK_HEADERS).status_code == 404
|
||||
assert _out(client).status_code == 404
|
||||
assert _in(client).status_code == 404
|
||||
|
||||
@@ -81,14 +89,14 @@ def test_it_ships_off(app):
|
||||
|
||||
def test_no_login_is_required(client, kiosk):
|
||||
"""The whole point: no Authorization header anywhere in this file."""
|
||||
response = client.get('/api/usb/kiosk/device/USB-1')
|
||||
response = client.get('/api/usb/kiosk/device/USB-1', headers=KIOSK_HEADERS)
|
||||
assert response.status_code == 200
|
||||
assert response.get_json()['data']['device_id'] == 'USB-1'
|
||||
|
||||
|
||||
def test_the_kiosk_cannot_create_edit_or_delete(client, kiosk):
|
||||
"""It moves a device between two states. It is not an admin surface."""
|
||||
assert client.post('/api/usb/kiosk/device/USB-1').status_code in (404, 405)
|
||||
assert client.post('/api/usb/kiosk/device/USB-1', headers=KIOSK_HEADERS).status_code in (404, 405)
|
||||
assert client.delete('/api/usb/kiosk/device/USB-1').status_code in (404, 405)
|
||||
# And the authenticated write routes are still gated.
|
||||
assert client.post('/api/usb/USB-1/checkout',
|
||||
@@ -113,7 +121,7 @@ def test_a_badge_it_cannot_name_is_refused(client, kiosk):
|
||||
|
||||
def test_a_missing_badge_is_refused(client, kiosk):
|
||||
assert client.post('/api/usb/kiosk/checkout',
|
||||
json={'device_id': 'USB-1'}).status_code == 400
|
||||
json={'device_id': 'USB-1'}, headers=KIOSK_HEADERS).status_code == 400
|
||||
|
||||
|
||||
# --- taking one -------------------------------------------------------------
|
||||
@@ -139,7 +147,7 @@ def test_a_drive_already_out_cannot_be_taken_again(client, kiosk):
|
||||
|
||||
def test_an_unknown_code_is_a_404(client, kiosk):
|
||||
response = client.post('/api/usb/kiosk/checkout',
|
||||
json={'device_id': 'NOPE', 'badge': '270015376'})
|
||||
json={'device_id': 'NOPE', 'badge': '270015376'}, headers=KIOSK_HEADERS)
|
||||
assert response.status_code == 404
|
||||
|
||||
|
||||
@@ -198,7 +206,7 @@ def test_a_drive_that_is_not_out_cannot_be_returned(client, kiosk):
|
||||
def test_the_lookup_names_the_holder_but_not_their_badge(client, kiosk):
|
||||
"""The confirm step has to show whose it is; it does not need their SSO."""
|
||||
_out(client)
|
||||
data = client.get('/api/usb/kiosk/device/USB-1').get_json()['data']
|
||||
data = client.get('/api/usb/kiosk/device/USB-1', headers=KIOSK_HEADERS).get_json()['data']
|
||||
|
||||
assert data['holdername'] == 'Dana Reyes'
|
||||
assert '270015376' not in str(data)
|
||||
@@ -211,11 +219,11 @@ def test_the_lookup_names_the_holder_but_not_their_badge(client, kiosk):
|
||||
# real person, the same bar taking a drive already had to clear.
|
||||
|
||||
def _session(client, badge='270015376'):
|
||||
return client.post('/api/usb/kiosk/session', json={'badge': badge})
|
||||
return client.post('/api/usb/kiosk/session', json={'badge': badge}, headers=KIOSK_HEADERS)
|
||||
|
||||
|
||||
def test_the_device_list_costs_a_valid_badge(client, kiosk):
|
||||
assert client.post('/api/usb/kiosk/session', json={}).status_code == 400
|
||||
assert client.post('/api/usb/kiosk/session', json={}, headers=KIOSK_HEADERS).status_code == 400
|
||||
assert _session(client, badge='999999999').status_code == 422
|
||||
|
||||
|
||||
@@ -223,7 +231,7 @@ def test_the_list_is_not_reachable_when_the_kiosk_is_off(client, kiosk):
|
||||
_setting(kiosk, 'usb_kiosk_enabled', 'false')
|
||||
assert _session(client).status_code == 404
|
||||
assert client.post('/api/usb/kiosk/others',
|
||||
json={'badge': '270015376'}).status_code == 404
|
||||
json={'badge': '270015376'}, headers=KIOSK_HEADERS).status_code == 404
|
||||
|
||||
|
||||
def test_the_session_separates_yours_from_available(client, kiosk):
|
||||
@@ -254,7 +262,7 @@ def test_names_appear_only_on_the_deliberate_second_step(client, kiosk):
|
||||
_out(client)
|
||||
|
||||
rows = client.post('/api/usb/kiosk/others',
|
||||
json={'badge': '270099999'}).get_json()['data']
|
||||
json={'badge': '270099999'}, headers=KIOSK_HEADERS).get_json()['data']
|
||||
assert [r['device_id'] for r in rows] == ['USB-1']
|
||||
assert rows[0]['holdername'] == 'Dana Reyes'
|
||||
|
||||
@@ -265,7 +273,7 @@ def test_the_others_list_is_gone_when_returns_for_others_are_refused(client, kio
|
||||
_out(client)
|
||||
|
||||
assert client.post('/api/usb/kiosk/others',
|
||||
json={'badge': '270099999'}).status_code == 404
|
||||
json={'badge': '270099999'}, headers=KIOSK_HEADERS).status_code == 404
|
||||
|
||||
|
||||
def test_a_retired_drive_is_not_offered(client, kiosk):
|
||||
@@ -276,3 +284,33 @@ def test_a_retired_drive_is_not_offered(client, kiosk):
|
||||
|
||||
data = _session(client).get_json()['data']
|
||||
assert data['available'] == []
|
||||
|
||||
|
||||
# --- the flag is a decision, the token is the credential ---------------------
|
||||
#
|
||||
# usb_kiosk_enabled alone used to be the whole gate. With the kiosk on, anyone
|
||||
# who could reach the app could move a drive between checked-out and checked-in
|
||||
# and have it recorded against a badge they guessed.
|
||||
|
||||
def test_kiosk_is_shut_without_the_device_token(client, kiosk):
|
||||
assert client.get('/api/usb/kiosk/device/USB-1').status_code == 404
|
||||
assert client.post('/api/usb/kiosk/checkout',
|
||||
json={'device_id': 'USB-1',
|
||||
'badge': '270015376'}).status_code == 404
|
||||
|
||||
|
||||
def test_kiosk_is_shut_with_the_wrong_device_token(client, kiosk):
|
||||
wrong = {'X-Kiosk-Token': KIOSK_TOKEN + 'x'}
|
||||
assert client.get('/api/usb/kiosk/device/USB-1',
|
||||
headers=wrong).status_code == 404
|
||||
assert client.post('/api/usb/kiosk/checkout', headers=wrong,
|
||||
json={'device_id': 'USB-1',
|
||||
'badge': '270015376'}).status_code == 404
|
||||
|
||||
|
||||
def test_the_token_alone_is_not_enough_when_the_kiosk_is_off(client, kiosk):
|
||||
"""Both halves are required: a provisioned screen at a site that has since
|
||||
switched the kiosk off gets nothing."""
|
||||
_setting(kiosk, 'usb_kiosk_enabled', 'false')
|
||||
assert client.get('/api/usb/kiosk/device/USB-1',
|
||||
headers=KIOSK_HEADERS).status_code == 404
|
||||
|
||||
Reference in New Issue
Block a user