Merge remote-tracking branch 'origin/main' into wilm

# Conflicts:
#	shopdb/__init__.py

tests/test_core/test_application_uploads.py

@@ -49,7 +49,7 @@ def test_image_is_readable_without_a_token(client, db, auth_headers):
                 data={'file': (_png(), 'logo.png')},
                 content_type='multipart/form-data', headers=auth_headers)
 
-    anon = client.get(f'/api/applications/image/application-{app.appid}.png')
+    anon = client.get(f'/api/applications/image/application-{app.appid}.png', headers=auth_headers)
 
     assert anon.status_code == 200
     assert anon.mimetype == 'image/png'

tests/test_core/test_authz.py

@@ -51,6 +51,30 @@ EXEMPT_ENDPOINTS = {'knowledgebase.track_click', 'users.update_user',
                     # docs/proposals/printedparts-plugin.md; justification in
                     # the plugin README.
                     'printedparts.kiosk_take',
+                    # Deliberately open kiosk writes, same bar as the parts
+                    # kiosk: they can ONLY move a USB device between
+                    # checked-out and checked-in, and only when a site has set
+                    # usb_kiosk_enabled (off by default - every one of these is
+                    # a 404 until then). Identity is a scanned badge resolved
+                    # server-side, and a badge that resolves to no employee is
+                    # refused rather than recorded, so nothing here can create
+                    # an untraceable checkout. The people who take drives stand
+                    # at the cabinet and mostly have no account, and both modes
+                    # already attributed the action to the badge rather than to
+                    # the signed-in user - the login was gating the screen, not
+                    # the record. Covered by tests/test_plugins/test_usb_kiosk.py.
+                    'usb.kiosk_checkout',
+                    'usb.kiosk_checkin',
+                    # The kiosk's two list endpoints. POSTs that CHANGE NOTHING:
+                    # they are reads, shaped as POSTs so the badge stays out of
+                    # query strings and proxy access logs. Same gate as the
+                    # writes above - 404 unless usb_kiosk_enabled, and they name
+                    # no drive until the badge resolves to a real person. The
+                    # drives have no barcode, so a list is the only way to pick
+                    # one; kiosk_others additionally 404s when returning for
+                    # somebody else is switched off.
+                    'usb.kiosk_session',
+                    'usb.kiosk_others',
                     # Redeeming a shared secret. The recipient usually has no
                     # account here, so the unguessable 128-bit token IS the
                     # authorisation - gating this on a role would make the

tests/test_core/test_display_kiosks.py

@@ -9,6 +9,17 @@ import pytest
 from shopdb.core.models import AssetType
 from plugins.computers.models import ComputerType
 
+
+@pytest.fixture(autouse=True)
+def _signed_in(client, auth_headers):
+    """Reading needs a permission now, and these tests are about what the
+    payload says rather than about who may see it. Signing the client in once
+    beats threading headers through every helper in the module."""
+    client.environ_base['HTTP_AUTHORIZATION'] = auth_headers['Authorization']
+    yield
+    client.environ_base.pop('HTTP_AUTHORIZATION', None)
+
+
 KEY = 'testcollectorkey'
 
 
@@ -20,7 +31,7 @@ def collector_key(app):
     app.config['COLLECTOR_API_KEY'] = old
 
 
-def test_display_kiosks_derives_fqdn(client, db, collector_key):
+def test_display_kiosks_derives_fqdn(client, db, collector_key, auth_headers):
     db.session.add(AssetType(assettype='computer', pluginname='computers',
                              tablename='computers'))
     db.session.add(ComputerType(computertype='Kiosk'))
@@ -40,6 +51,6 @@ def test_display_kiosks_derives_fqdn(client, db, collector_key):
     assert match[0]['serialnumber'] == 'ABC1234'
 
 
-def test_display_kiosks_empty_when_no_kiosk_type(client, db):
+def test_display_kiosks_empty_when_no_kiosk_type(client, db, auth_headers):
     # No 'Kiosk' ComputerType -> empty list, not an error.
     assert client.get('/api/computers/display-kiosks').get_json()['data'] == []

tests/test_core/test_employees.py

@@ -7,20 +7,20 @@ directory is unreachable. Behavior must be identical as a plugin blueprint.
 """
 
 
-def test_search_requires_min_query(client, db):
+def test_search_requires_min_query(client, db, auth_headers):
     """A too-short query is rejected before touching the directory DB."""
-    resp = client.get('/api/employees/search?q=a')
+    resp = client.get('/api/employees/search?q=a', headers=auth_headers)
     assert resp.status_code == 400
 
 
-def test_lookup_requires_numeric_sso(client, db):
+def test_lookup_requires_numeric_sso(client, db, auth_headers):
     """Non-numeric SSO is rejected without touching the directory DB."""
-    resp = client.get('/api/employees/lookup/abc')
+    resp = client.get('/api/employees/lookup/abc', headers=auth_headers)
     assert resp.status_code == 400
 
 
-def test_search_degrades_when_directory_unreachable(client, db):
+def test_search_degrades_when_directory_unreachable(client, db, auth_headers):
     """With the directory DB unavailable, search returns a clean 500 envelope."""
-    resp = client.get('/api/employees/search?q=smith')
+    resp = client.get('/api/employees/search?q=smith', headers=auth_headers)
     assert resp.status_code == 500
     assert resp.get_json()['status'] == 'error'

tests/test_core/test_guest_role.py

@@ -0,0 +1,190 @@
+"""The guest role: what somebody with no account may do.
+
+Guest access used to be a per-feature boolean - one for Secure Send, one per
+kiosk, one nearly written for guides - each with its own settings page and its
+own hand-written branch inside a route. This replaced all of that with a role,
+so the question is answered where every other permission question is answered.
+
+The contract these tests hold:
+
+  - a guest holds nothing by default, so an upgrade opens no door
+  - granting the guest role a permission is what opens one
+  - revoking it closes the door on the next request, not the next restart
+  - a guest can never be granted a write unless its owner declared it safe
+  - a signed-in caller is unaffected by any of it
+"""
+
+import pytest
+
+from shopdb.core.models import Permission, Role
+
+
+@pytest.fixture
+def guestrole(db):
+    """The seeded guest role, holding nothing."""
+    Permission.seed()
+    Role.seedguest()
+    db.session.commit()
+    return Role.guest()
+
+
+def grant(db, role, *names):
+    role.permissions = Permission.query.filter(Permission.name.in_(names)).all()
+    db.session.commit()
+
+
+# ---------------------------------------------------------------------------
+# The default: nothing
+# ---------------------------------------------------------------------------
+
+def test_a_guest_holds_nothing_by_default(guestrole):
+    """An upgrade must not open a door a site never asked to open."""
+    assert guestrole.permissions == []
+
+
+def test_a_guest_is_refused_a_read_nobody_granted(client, guestrole):
+    response = client.get('/api/auditlogs')
+    assert response.status_code == 401
+
+
+def test_a_guest_is_refused_a_write(client, guestrole):
+    response = client.post('/api/assets', json={'assetnumber': 'GUEST-1'})
+    assert response.status_code == 401
+
+
+# ---------------------------------------------------------------------------
+# Granting, and taking it back
+# ---------------------------------------------------------------------------
+
+def test_granting_the_guest_role_a_view_opens_that_read(client, db, guestrole):
+    grant(db, guestrole, 'audit.view')
+    response = client.get('/api/auditlogs')
+    assert response.status_code == 200
+
+
+def test_a_grant_does_not_leak_into_neighbouring_permissions(client, db, guestrole):
+    """audit.view is not apitokens.view. The role holds exactly what it was
+    given."""
+    grant(db, guestrole, 'audit.view')
+    assert client.get('/api/apitokens').status_code == 401
+
+
+def test_a_write_stays_shut_even_when_a_read_is_open(client, db, guestrole):
+    grant(db, guestrole, 'audit.view')
+    assert client.post('/api/assets', json={'assetnumber': 'X'}).status_code == 401
+
+
+def test_revoking_closes_the_door_on_the_next_request(client, db, guestrole):
+    """Not on the next restart. Revoking is what somebody does the moment they
+    realise what they opened up, and it has to take effect then."""
+    grant(db, guestrole, 'audit.view')
+    assert client.get('/api/auditlogs').status_code == 200
+
+    guestrole.permissions = []
+    db.session.commit()
+    assert client.get('/api/auditlogs').status_code == 401
+
+
+def test_a_site_with_no_guest_role_behaves_as_it_always_did(client, db):
+    """An older database, seeded before any of this existed."""
+    role = Role.guest()
+    if role is not None:
+        db.session.delete(role)
+        db.session.commit()
+    assert client.get('/api/auditlogs').status_code == 401
+
+
+# ---------------------------------------------------------------------------
+# The guardrail
+# ---------------------------------------------------------------------------
+
+def test_a_view_permission_is_grantable(app):
+    from shopdb.core.models import guest_grantable_permissions
+    with app.app_context():
+        grantable = guest_grantable_permissions()
+    assert 'assets.view' in grantable
+    assert 'reports.view' in grantable
+
+
+def test_a_write_permission_is_not_grantable(app):
+    from shopdb.core.models import guest_grantable_permissions
+    with app.app_context():
+        grantable = guest_grantable_permissions()
+    for name in ('assets.delete', 'assets.create', 'users.create',
+                 'settings.edit'):
+        assert name not in grantable, name
+
+
+def test_the_api_refuses_to_grant_a_guest_a_write(client, db, auth_headers,
+                                                  guestrole):
+    """Refused at the door rather than warned about: a checkbox that quietly
+    makes deleting an asset anonymous is not a warning anybody reads twice."""
+    response = client.put('/api/users/roles/{}'.format(guestrole.roleid),
+                          headers=auth_headers,
+                          json={'permissions': ['assets.delete']})
+    assert response.status_code == 400
+    assert 'assets.delete' in response.get_json()['data']['error']['message']
+    assert guestrole.permissions == []
+
+
+def test_the_api_allows_granting_a_guest_a_view(client, db, auth_headers,
+                                                guestrole):
+    response = client.put('/api/users/roles/{}'.format(guestrole.roleid),
+                          headers=auth_headers,
+                          json={'permissions': ['audit.view']})
+    assert response.status_code == 200
+    assert [p.name for p in guestrole.permissions] == ['audit.view']
+
+
+def test_the_guest_role_cannot_be_deleted(client, auth_headers, guestrole):
+    """Deleting it does not close the door, it removes the only place the door
+    is described."""
+    response = client.delete('/api/users/roles/{}'.format(guestrole.roleid))
+    assert response.status_code in (401, 403)
+
+    response = client.delete('/api/users/roles/{}'.format(guestrole.roleid),
+                             headers=auth_headers)
+    assert response.status_code == 400
+    assert Role.guest() is not None
+
+
+def test_the_role_list_says_what_a_guest_may_hold(client, auth_headers,
+                                                  guestrole):
+    """So the grid can grey the rest out rather than letting somebody tick a
+    box the API will refuse."""
+    rows = client.get('/api/users/roles', headers=auth_headers).get_json()['data']
+    guest = next(r for r in rows if r['rolename'] == 'guest')
+    assert guest['isguest'] is True
+    assert 'assets.view' in guest['grantable']
+    assert 'assets.delete' not in guest['grantable']
+
+    other = next((r for r in rows if r['rolename'] != 'guest'), None)
+    if other is not None:
+        assert other['grantable'] is None, 'only the guest role is bounded'
+
+
+# ---------------------------------------------------------------------------
+# Signed-in callers are unaffected
+# ---------------------------------------------------------------------------
+
+def test_a_member_still_needs_their_own_permission(client, member_headers,
+                                                   db, guestrole):
+    """A grant to guests is not a grant to everybody: a signed-in member
+    without the permission is still refused, with 403 rather than 401 because
+    signing in would not change the answer."""
+    grant(db, guestrole, 'audit.view')
+    response = client.get('/api/auditlogs', headers=member_headers)
+    assert response.status_code == 403
+
+
+def test_an_admin_is_unaffected(client, auth_headers, guestrole):
+    assert client.get('/api/auditlogs', headers=auth_headers).status_code == 200
+
+
+def test_a_bad_token_is_still_refused(client, db, guestrole):
+    """Optional verification tolerates a MISSING token, not a broken one - a
+    forged token must not be quietly downgraded to guest access."""
+    grant(db, guestrole, 'audit.view')
+    response = client.get('/api/auditlogs',
+                          headers={'Authorization': 'Bearer not.a.token'})
+    assert response.status_code == 422

tests/test_core/test_model_image.py

@@ -100,7 +100,7 @@ def test_serve_returns_bytes(client, db, auth_headers):
     client.post(f'/api/models/{m.modelnumberid}/image', data=data,
                 content_type='multipart/form-data', headers=auth_headers)
 
-    served = client.get(f'{MODEL_IMAGE_URL_PREFIX}model-{m.modelnumberid}.png')
+    served = client.get(f'{MODEL_IMAGE_URL_PREFIX}model-{m.modelnumberid}.png', headers=auth_headers)
     assert served.status_code == 200
     assert served.get_data() == payload
 

tests/test_core/test_public_read_surface.py

@@ -0,0 +1,153 @@
+"""What a caller with no account can read, and nothing more.
+
+The companion to the mutating-route sweep in test_authz.py. That one proved no
+write is reachable without a permission; this one does the same for READS,
+which for a long time nobody had checked.
+
+The finding that produced it: 165 GET routes answered anybody. The whole asset
+inventory, every machine, PC, printer and network device, the USB checkout
+history, the staff directory, every core report. Guest access had been argued
+about one feature-flag at a time while the front door stood open.
+
+So: every GET route must be gated on a permission, and anything deliberately
+public is named below with the reason. Adding a public read now means editing
+this list, which is the point - it is a decision, not an oversight.
+"""
+
+import pytest
+
+
+# Deliberately readable with no account. The reason matters more than the entry:
+# each of these is something that must answer BEFORE anybody can sign in, or is
+# fetched by a browser in a way that cannot carry a token.
+PUBLIC_ENDPOINTS = {
+    # The app shell and its bootstrap.
+    'serve_frontend',
+    'setup.needs_admin',
+    'auth.get_current_user',
+    'plugins.list_enabled_plugins',
+    'dashboard.get_navigation',      # filtered per caller; a guest sees only
+                                     # what the guest role can reach
+    'dashboard.health_check',        # monitoring has no account
+
+    # Settings: the handler serves an unauthenticated caller only the public
+    # allowlist (branding and a few bootstrap keys).
+    'settings.list_settings',
+    'settings.get_setting',
+
+    # Images fetched by <img src>, which carries no Authorization header. The
+    # branding logo renders on the LOGIN page.
+    'settings.serve_branding_logo',
+    'settings.serve_map_blueprint',
+    'employees.serve_employee_photo',
+
+    # API documentation: shapes, not data. The docs page loads its own script.
+    'docs.docs_index', 'docs.openapi_spec', 'docs.redoc_js', 'docs.llms_txt',
+
+    # Collector ingest, authenticated by the per-plugin collector key.
+    'collector.collector_schemas', 'collector.collector_status',
+
+    # A display kiosk renders before anybody signs in.
+    'dashboarddefaults.visitor_location',
+    'dashboarddefaults.display_role',
+    'businessunits.list_businessunits',
+    'notifications.get_shopfloor_notifications',
+    'notifications.get_employee_recognitions',
+    'slides.feed', 'slides.serve_image',
+
+    # Parts kiosk: one item by its code, and that item's picture.
+    'printedparts.kiosk_item', 'printedparts.serve_item_image',
+
+    # USB kiosk.
+    'usb.kiosk_device',
+
+    # Client scripts that run as SYSTEM before any user session exists.
+    'printers_asset.printers_for_host',
+    'printers_asset.printer_install_list',
+    'printers_asset.printer_install_batch',
+    'printers_asset.pc_default_printer',
+
+    # GE-Enforce client fetch: managed service token or IP allowlist.
+    'geenforce.get_manifest', 'geenforce.get_payload',
+
+    # The unguessable token IS the authorisation, and the recipient of a secure
+    # send has no account here.
+    'tools.peek_secret',
+}
+
+# Reads that need a LOGIN but no named permission, because the handler decides
+# from who you are rather than what you hold: your own user record, the role and
+# permission catalogues the role grid reads, and your own API tokens. Not
+# public - the test below proves each one refuses an anonymous caller.
+LOGIN_ONLY_ENDPOINTS = {
+    'users.get_user',            # admin-or-self, decided inside
+    'users.list_permissions',    # the catalogue the role grid renders
+    'users.list_roles',
+    'apitokens.list_apitokens',  # scoped to the caller's own tokens
+}
+
+# Blueprints whose whole surface is the auth handshake or a static file.
+EXEMPT_BLUEPRINTS = {'static'}
+
+
+def _gets(app):
+    for rule in app.url_map.iter_rules():
+        if 'GET' not in (rule.methods or set()):
+            continue
+        blueprint = rule.endpoint.split('.')[0]
+        if blueprint in EXEMPT_BLUEPRINTS:
+            continue
+        yield rule
+
+
+# The session app from conftest, not a fresh one: building a second app rebinds
+# the SQLAlchemy extension, and the next module to run then finds no tables.
+@pytest.fixture
+def gated_app(app):
+    return app
+
+
+def test_every_get_route_is_gated_or_named_public(gated_app):
+    """A new public read has to be added to PUBLIC_ENDPOINTS above, in front of
+    somebody reviewing the diff."""
+    ungated = []
+    for rule in _gets(gated_app):
+        view = gated_app.view_functions[rule.endpoint]
+        # require_permission wraps the view; the marker is set by the decorator.
+        if getattr(view, '_requires_permission', None):
+            continue
+        if rule.endpoint in PUBLIC_ENDPOINTS or rule.endpoint in LOGIN_ONLY_ENDPOINTS:
+            continue
+        if rule.endpoint.split('.', 1)[-1] in PUBLIC_ENDPOINTS:
+            continue
+        ungated.append(rule.endpoint)
+
+    assert not ungated, (
+        'These GET routes answer anybody. Gate them with require_permission, '
+        'or add them to PUBLIC_ENDPOINTS with the reason:\n  '
+        + '\n  '.join(sorted(set(ungated))))
+
+
+def test_the_public_list_has_no_stale_entries(gated_app):
+    """An endpoint that no longer exists, or that has since been gated, should
+    not keep a licence to be public."""
+    endpoints = {rule.endpoint for rule in _gets(gated_app)}
+    shortnames = {e.split('.', 1)[-1] for e in endpoints}
+    stale = [name for name in PUBLIC_ENDPOINTS | LOGIN_ONLY_ENDPOINTS
+             if name not in endpoints and name not in shortnames]
+    assert not stale, 'PUBLIC_ENDPOINTS names routes that do not exist: {}'.format(stale)
+
+
+@pytest.mark.parametrize('endpoint', sorted(LOGIN_ONLY_ENDPOINTS))
+def test_a_login_only_read_refuses_an_anonymous_caller(client, gated_app, endpoint):
+    """These carry no permission because the handler decides from identity.
+    That is only sound while they still demand an identity."""
+    rule = next(r for r in _gets(gated_app) if r.endpoint == endpoint)
+    path = rule.rule
+    for argument in (rule.arguments or ()):
+        path = path.replace('<int:{}>'.format(argument), '1')
+        path = path.replace('<{}>'.format(argument), '1')
+    response = client.get(path)
+    assert response.status_code in (401, 422), (
+        '{} answered an anonymous caller with {}'.format(endpoint,
+                                                         response.status_code))

tests/test_core/test_relationship_directional.py

@@ -54,7 +54,7 @@ def test_list_includes_directional(client, db, auth_headers):
     client.post('/api/assets/relationshiptypes',
                 json={'relationshiptype': 'WiFi', 'isdirectional': False},
                 headers=auth_headers)
-    resp = client.get('/api/assets/relationshiptypes')
+    resp = client.get('/api/assets/relationshiptypes', headers=auth_headers)
     assert resp.status_code == 200
     row = next(t for t in resp.get_json()['data']
                if t['relationshiptype'] == 'WiFi')
@@ -88,7 +88,7 @@ def test_relationships_row_carries_isdirectional(client, db, auth_headers):
                                      relationshiptypeid=directional.relationshiptypeid))
     db.session.commit()
 
-    resp = client.get(f'/api/assets/{a.assetid}/relationships')
+    resp = client.get(f'/api/assets/{a.assetid}/relationships', headers=auth_headers)
     assert resp.status_code == 200
     data = resp.get_json()['data']
 

tests/test_core/test_search_coverage.py

@@ -17,6 +17,17 @@ import pytest
 from shopdb.core.models import Application
 
 
+@pytest.fixture(autouse=True)
+def _signed_in(client, auth_headers):
+    """Reading needs a permission now, and these tests are about what the
+    payload says rather than about who may see it. Signing the client in once
+    beats threading headers through every helper in the module."""
+    client.environ_base['HTTP_AUTHORIZATION'] = auth_headers['Authorization']
+    yield
+    client.environ_base.pop('HTTP_AUTHORIZATION', None)
+
+
+
 def search(client, query):
     """Global search rows for a query."""
     resp = client.get(f'/api/search?q={query}')

tests/test_core/test_security_headers.py

@@ -14,22 +14,22 @@ import pytest
 from shopdb.utils.uploads import UPLOAD_CSP
 
 
-def test_every_response_carries_the_baseline_headers(client):
-    resp = client.get('/api/health')
+def test_every_response_carries_the_baseline_headers(client, auth_headers):
+    resp = client.get('/api/health', headers=auth_headers)
     assert resp.headers.get('X-Content-Type-Options') == 'nosniff'
     assert resp.headers.get('X-Frame-Options') == 'SAMEORIGIN'
     assert resp.headers.get('Referrer-Policy') == 'strict-origin-when-cross-origin'
 
 
-def test_an_error_response_carries_them_too(client):
+def test_an_error_response_carries_them_too(client, auth_headers):
     """after_request runs for error handlers as well, which is where a response
     built by a different code path would otherwise slip through."""
-    resp = client.get('/api/no-such-route')
+    resp = client.get('/api/no-such-route', headers=auth_headers)
     assert resp.status_code == 404
     assert resp.headers.get('X-Content-Type-Options') == 'nosniff'
 
 
-def test_a_served_upload_is_sandboxed(client, app, tmp_path):
+def test_a_served_upload_is_sandboxed(client, app, tmp_path, auth_headers):
     """The uploaded file itself gets a policy that blocks script execution."""
     from shopdb.core.api.applications import _appimage_dir
 
@@ -48,7 +48,7 @@ def test_a_served_upload_is_sandboxed(client, app, tmp_path):
         path.write_text(svg)
 
     try:
-        resp = client.get('/api/applications/image/probe-headers.svg')
+        resp = client.get('/api/applications/image/probe-headers.svg', headers=auth_headers)
         assert resp.status_code == 200
         assert resp.headers.get('Content-Security-Policy') == UPLOAD_CSP
         assert resp.headers.get('X-Content-Type-Options') == 'nosniff'