From 68ddf87d3bc10ff02a00728a7bde5585c940682f Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Tue, 8 Sep 2026 07:36:19 -0400 Subject: [PATCH] Gate operator reports behind their own grant Operator report screenshots are an uncontrolled capture of whatever was on the operator's screen when they filed the report, so they can carry part numbers, customer names, or other shopfloor content. The rest of endpoints.view is uniform machine-collected telemetry. Reading a PC's CPU load does not imply a right to read someone's screen, so the reports and their images move to endpoints.operatorreports. The image route is the real control point, not the panel: the panel only decides whether a tab is drawn, while the bytes stay reachable by URL to anyone who can guess an id. Both GET routes now require the new grant. Migration 7d47 backfills the grant to everyone holding endpoints.view so nobody loses a panel they were already using; tightening from there is a per-account decision in the user editor. The grant is in the scope denylist, so it is a per-user direct grant and Reporter cannot hold it. Also adds test_user_editor_checkboxes_match_the_server_action_list. The SCOPE_ENDPOINT_ACTIONS comment claimed a sync guard kept the Vue checkbox list aligned with the Python tuple, but no such test existed. The lists had already drifted: endpoints.operatorreports reached a running container with no way to grant it through the UI. The guard catches drift in both directions - a name only in Python ships the feature dark, and a name only in Vue is worse, since the admin ticks a box and believes access was granted while the server refuses to store it. --- frontend/src/views/settings/UsersList.vue | 9 +- .../7d47_endpoints_operatorreports.py | 97 ++++++++++++++++ plugins/computers/frontend/views/PCDetail.vue | 14 ++- plugins/endpoints/api/routes.py | 13 ++- plugins/endpoints/plugin.py | 15 +++ shopdb/core/models/user.py | 5 + tests/test_core/test_user_scopes.py | 31 +++++ .../test_endpoints_operator_reports.py | 106 +++++++++++++++++- 8 files changed, 280 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/7d47_endpoints_operatorreports.py diff --git a/frontend/src/views/settings/UsersList.vue b/frontend/src/views/settings/UsersList.vue index 0f81f6d9..3b02ad76 100644 --- a/frontend/src/views/settings/UsersList.vue +++ b/frontend/src/views/settings/UsersList.vue @@ -366,15 +366,18 @@ const userForm = reactive({ unlock: false }) -// The four endpoint actions the shop gates per person rather than per policy. +// The endpoint actions the shop gates per person rather than per policy. // Named here rather than derived from the permission catalog so the user editor // stays a short, meaningful list instead of the full permission grid, which -// belongs to the role editor. +// belongs to the role editor. Mirrors SCOPE_ENDPOINT_ACTIONS in +// shopdb/core/models/user.py; adding one here without adding it there leaves a +// checkbox the server will refuse to store. const endpointActionPermissions = [ { name: 'endpoints.restart', label: 'Remote Restart' }, { name: 'endpoints.runscript', label: 'Script Push' }, { name: 'endpoints.updateapp', label: 'Application Update' }, - { name: 'endpoints.corptools', label: 'Corporate Tool Use' } + { name: 'endpoints.corptools', label: 'Corporate Tool Use' }, + { name: 'endpoints.operatorreports', label: 'Operator Reports' } ] // What each scope means, in the words shown at the point of use. diff --git a/migrations/versions/7d47_endpoints_operatorreports.py b/migrations/versions/7d47_endpoints_operatorreports.py new file mode 100644 index 00000000..556756f3 --- /dev/null +++ b/migrations/versions/7d47_endpoints_operatorreports.py @@ -0,0 +1,97 @@ +"""Split operator report viewing out of endpoints.view. + +Operator reports carry a screenshot of whatever was on the operator's screen +when they filed the report. That is an uncontrolled human capture - it can +include part numbers, customer names, email, or anything else that happened to +be visible - whereas the rest of endpoints.view is uniform machine-collected +telemetry. Reading a PC's CPU load does not imply a right to read someone's +screen, so the reports and their images move to their own grant. + +Backfill preserves access rather than revoking it: every user who can see +endpoints today gets the new grant, so nobody loses a panel they were already +using. Tightening from there is an administrative decision made per account in +the user editor, not something this migration should force. The grant is a +per-user direct permission (endpoints.operatorreports is in the scope +denylist), so the backfill writes userpermissions rows, not role rows. + +Revision ID: 7d47_endpoints_operatorreports +Revises: 7d46_nest_app_packages +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d47_endpoints_operatorreports' +down_revision = '7d46_nest_app_packages' +branch_labels = None +depends_on = None + + +NEW_PERMISSION = ( + 'endpoints.operatorreports', + 'View operator-submitted reports and their screenshots', + 'endpoints', +) + +SOURCE_PERMISSION = 'endpoints.view' + + +def _tables_present(bind): + inspector = sa.inspect(bind) + tables = set(inspector.get_table_names()) + return {'users', 'roles', 'permissions', 'rolepermissions', + 'userpermissions', 'userroles'} <= tables + + +def upgrade(): + bind = op.get_bind() + if not _tables_present(bind): + return + + name, description, category = NEW_PERMISSION + # Guarded in Python so the statement stays portable: MySQL's FROM DUAL + # idiom does not run on SQLite, which the test suite uses. + exists = bind.execute(sa.text( + 'SELECT 1 FROM permissions WHERE name = :name'), + {'name': name}).first() + if not exists: + bind.execute(sa.text( + 'INSERT INTO permissions (name, description, category) ' + 'VALUES (:name, :description, :category)'), + {'name': name, 'description': description, 'category': category}) + + # Preserve access for anyone who reaches operator reports today, which is + # anyone holding endpoints.view through a role. NOT EXISTS keeps the + # migration re-runnable and avoids duplicate rows for users in two + # qualifying roles. + bind.execute(sa.text( + 'INSERT INTO userpermissions (userid, permissionid) ' + 'SELECT DISTINCT ur.userid, new.permissionid ' + 'FROM userroles ur ' + 'JOIN rolepermissions rp ON rp.roleid = ur.roleid ' + 'JOIN permissions old ON old.permissionid = rp.permissionid ' + 'JOIN permissions new ON new.name = :newname ' + 'WHERE old.name = :oldname ' + 'AND NOT EXISTS (SELECT 1 FROM userpermissions existing ' + ' WHERE existing.userid = ur.userid ' + ' AND existing.permissionid = new.permissionid)'), + {'newname': name, 'oldname': SOURCE_PERMISSION}) + + +def downgrade(): + bind = op.get_bind() + if not _tables_present(bind): + return + + name = NEW_PERMISSION[0] + bind.execute(sa.text( + 'DELETE FROM userpermissions WHERE permissionid IN ' + '(SELECT permissionid FROM permissions WHERE name = :name)'), + {'name': name}) + bind.execute(sa.text( + 'DELETE FROM rolepermissions WHERE permissionid IN ' + '(SELECT permissionid FROM permissions WHERE name = :name)'), + {'name': name}) + bind.execute(sa.text( + 'DELETE FROM permissions WHERE name = :name'), {'name': name}) diff --git a/plugins/computers/frontend/views/PCDetail.vue b/plugins/computers/frontend/views/PCDetail.vue index 15a3f642..6a8deb72 100644 --- a/plugins/computers/frontend/views/PCDetail.vue +++ b/plugins/computers/frontend/views/PCDetail.vue @@ -829,6 +829,13 @@ const hasCloudIdentity = computed(() => portalTemplatesLoaded.value && Boolean( // during setup, which reads canDeploy before this line would otherwise run. const canDeploy = computed(() => authStore.hasPermission('endpoints.corptools')) +// Operator report screenshots are an uncontrolled capture of whatever was on +// the operator's screen, so they carry their own grant rather than riding +// endpoints.view. Declared alongside canDeploy for the same reason: detailtabs +// reads it during setup. +const canViewOperatorReports = computed( + () => authStore.hasPermission('endpoints.operatorreports')) + const detailtabs = computed(() => { const filled = assetPanels.value?.populatedGroups ?? new Set() const has = (key) => filled.has ? filled.has(key) : false @@ -844,7 +851,12 @@ const detailtabs = computed(() => { // Operator Reports earns a tab rather than a card in System Monitors: it is // what a person reported, not what the agent measured, and the thumbnail // grid is much bulkier than the fact cards around it. - if (has('reports')) tabs.push({ key: 'reports', label: 'Operator Reports' }) + // Permission-gated as well as data-gated: the screenshots can carry shopfloor + // content that endpoints.view does not imply a right to see. The panel and + // image routes both re-check server-side; this only hides the tab. + if (has('reports') && canViewOperatorReports.value) { + tabs.push({ key: 'reports', label: 'Operator Reports' }) + } if (has('software') || installedApps.value.length > 0) { tabs.push({ key: 'software', label: 'Software', count: installedApps.value.length || null }) } diff --git a/plugins/endpoints/api/routes.py b/plugins/endpoints/api/routes.py index cc1524fa..85c7107b 100644 --- a/plugins/endpoints/api/routes.py +++ b/plugins/endpoints/api/routes.py @@ -3036,7 +3036,7 @@ def upload_operator_report(): @endpoints_bp.route('/panel//operatorreports', methods=['GET']) -@require_permission('endpoints.view') +@require_permission('endpoints.operatorreports') def panel_operator_reports(asset_id: int): """Operator Reports panel: this PC's recent operator issue reports. @@ -3058,9 +3058,16 @@ def panel_operator_reports(asset_id: int): @endpoints_bp.route('/operatorreports//image', methods=['GET']) -@require_permission('endpoints.view') +@require_permission('endpoints.operatorreports') def serve_operator_report_image(operatorreportid: int): - """Serve one operator report's screenshot (user JWT + endpoints.view).""" + """Serve one operator report's screenshot. + + Requires a user JWT plus endpoints.operatorreports. + This route, not the panel, is what actually protects the image: the panel + only decides whether a tab is drawn, while the bytes are reachable by URL to + anyone who can guess an id. Screenshots are an uncontrolled capture of the + operator's screen, so this must never widen back to endpoints.view. + """ from shopdb.api import send_upload report = db.session.get(OperatorReport, operatorreportid) diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index a4ac7a8f..5c69e98c 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -177,6 +177,16 @@ class EndpointsPlugin(BasePlugin): # shop-floor PC is a heavier grant than reading its telemetry. ('endpoints.command', 'Send allowlisted commands to endpoint agents', 'endpoints'), + # Operator-submitted issue reports and their screenshots. Separate + # from endpoints.view because the screenshot is whatever was on the + # operator's screen when they hit report: it can carry part numbers, + # customer names or other shopfloor content that reading telemetry + # does not imply a right to see. Telemetry is machine-collected and + # uniform; this is an uncontrolled human capture, so it is its own + # grant rather than a facet of endpoints.view. + ('endpoints.operatorreports', + 'View operator-submitted reports and their screenshots', + 'endpoints'), # Elevated grant for destructive endpoint actions (system_restart). # Required IN ADDITION to endpoints.command by the enqueue route, so a # remote reboot is a strictly heavier grant than a routine toggle. @@ -702,6 +712,11 @@ class EndpointsPlugin(BasePlugin): 'empty': 'No operator reports for this endpoint.', 'position': 18, 'group': 'reports', + # Not endpoints.view: the screenshots can carry shopfloor + # content that reading telemetry does not imply a right to see. + # The panel route and the image route re-check this server-side; + # withholding the declaration only keeps the tab from appearing. + 'permission': 'endpoints.operatorreports', }, ] diff --git a/shopdb/core/models/user.py b/shopdb/core/models/user.py index 01dafe2f..58ade2f2 100644 --- a/shopdb/core/models/user.py +++ b/shopdb/core/models/user.py @@ -263,6 +263,10 @@ SCOPE_NEVER_GRANT = { 'endpoints.runscript', 'endpoints.updateapp', 'endpoints.corptools', + # Operator report screenshots are an uncontrolled capture of the operator's + # screen and may carry shopfloor content, so who sees them is a per-account + # decision rather than something a scope confers. + 'endpoints.operatorreports', # Fleet-wide and agent-distribution capabilities, admin-only by nature. 'endpoints.fleet', 'endpoints.agentpackage', @@ -278,6 +282,7 @@ SCOPE_ENDPOINT_ACTIONS = ( 'endpoints.runscript', 'endpoints.updateapp', 'endpoints.corptools', + 'endpoints.operatorreports', ) diff --git a/tests/test_core/test_user_scopes.py b/tests/test_core/test_user_scopes.py index 9faf1885..e9f41fe7 100644 --- a/tests/test_core/test_user_scopes.py +++ b/tests/test_core/test_user_scopes.py @@ -18,6 +18,37 @@ def _core_catalog(): return list(Permission.CORE_PERMISSIONS) +def test_user_editor_checkboxes_match_the_server_action_list(): + """The Vue checkbox list must match SCOPE_ENDPOINT_ACTIONS exactly. + + These two lists are maintained by hand in different languages, so they drift + silently and in both directions. A name in Python but not in Vue is a grant + no admin can actually give - the feature ships dark. A name in Vue but not in + Python is worse: the checkbox appears, the admin ticks it believing access + was granted, and the server refuses to store it. + + This is the guard the SCOPE_ENDPOINT_ACTIONS comment claims exists. It did + not, which is exactly how endpoints.operatorreports reached a running + container with no way to grant it. + """ + import re + from pathlib import Path + + source = Path(__file__).resolve().parents[2].joinpath( + 'frontend', 'src', 'views', 'settings', 'UsersList.vue').read_text( + encoding='utf-8') + + block = re.search( + r'const endpointActionPermissions = \[(.*?)\]', source, re.S) + assert block, 'endpointActionPermissions not found in UsersList.vue' + infrontend = set(re.findall(r"name:\s*'([^']+)'", block.group(1))) + + assert infrontend == set(SCOPE_ENDPOINT_ACTIONS), ( + 'User editor checkboxes and SCOPE_ENDPOINT_ACTIONS disagree. ' + f'Only in Vue: {sorted(infrontend - set(SCOPE_ENDPOINT_ACTIONS))}. ' + f'Only in Python: {sorted(set(SCOPE_ENDPOINT_ACTIONS) - infrontend)}.') + + # --------------------------------------------------------------------------- # Derived grant lists # --------------------------------------------------------------------------- diff --git a/tests/test_plugins/test_endpoints_operator_reports.py b/tests/test_plugins/test_endpoints_operator_reports.py index e5219a94..02752a78 100644 --- a/tests/test_plugins/test_endpoints_operator_reports.py +++ b/tests/test_plugins/test_endpoints_operator_reports.py @@ -3,9 +3,11 @@ The tray captures a screenshot and a free-text note; the agent posts them to POST /api/endpoints/operatorreports on the collector key. The bytes land on disk under the operator-reports uploads dir and the row holds metadata. The -human-facing panel and image-serve routes are JWT + endpoints.view gated, and -the serve route carries the inert-upload headers. A retention sweep purges rows -and files older than the configured window. +human-facing panel and image-serve routes are JWT + endpoints.operatorreports +gated - deliberately NOT endpoints.view, because the screenshot is an +uncontrolled capture of the operator's screen and reading telemetry does not +imply a right to see it - and the serve route carries the inert-upload headers. +A retention sweep purges rows and files older than the configured window. """ import io @@ -283,3 +285,101 @@ def test_upload_refuses_an_image_over_the_cap(client, db, app): assert 'limit' in response.get_json()['data']['error']['message'] assert OperatorReport.query.filter_by( reportedhostname='OPREPORT-HUGE').first() is None + + +def _viewer_headers(client, db, permissions, username): + """Log in a user holding exactly `permissions` as direct grants. + + Deliberately direct grants rather than a role: endpoints.operatorreports is + in the scope denylist, so a role can never confer it and a role-based + fixture could not express the case these tests need. + """ + from shopdb.core.models import Permission, User + from werkzeug.security import generate_password_hash + + user = User(username=username, email=f'{username}@test.local', + passwordhash=generate_password_hash('testpass')) + for name in permissions: + perm = Permission.query.filter_by(name=name).first() + if perm is None: + perm = Permission(name=name, description=name, + category='endpoints') + db.session.add(perm) + db.session.flush() + user.directpermissions.append(perm) + db.session.add(user) + db.session.commit() + + response = client.post('/api/auth/login', + json={'username': username, 'password': 'testpass'}) + assert response.status_code == 200, response.get_json() + token = response.get_json()['data']['access_token'] + return {'Authorization': f'Bearer {token}'} + + +def _report_for(client, db, app, hostname): + """Upload one report on the collector key and return its row.""" + from plugins.endpoints.models import OperatorReport + + asset = _computer(db, hostname) + client.post(UPLOAD_URL, + data={'hostname': hostname, 'details': 'x', + 'screenshot': (io.BytesIO(_png_bytes()), 'shot.png')}, + content_type='multipart/form-data', + headers=_collector_headers(app)) + return OperatorReport.query.filter_by(assetid=asset.assetid).first() + + +def test_endpoints_view_alone_cannot_fetch_a_report_screenshot( + client, db, app): + """The whole point of the split: telemetry access is not screenshot access. + + The screenshot is an uncontrolled capture of the operator's screen, so a + user who can read a PC's telemetry must NOT get the image for free. If this + regresses, sensitive shopfloor content is exposed to every endpoint viewer. + """ + report = _report_for(client, db, app, 'OPREPORT-VIEWONLY') + headers = _viewer_headers(client, db, ['endpoints.view'], 'telemetryonly') + + response = client.get(IMAGE_URL.format(report.operatorreportid), + headers=headers) + assert response.status_code == 403 + + +def test_endpoints_view_alone_cannot_list_operator_reports(client, db, app): + """The panel route leaks the report text and filenames, so it is gated too. + + Hiding the tab in the UI is not a control; the route must refuse directly. + """ + report = _report_for(client, db, app, 'OPREPORT-PANELDENY') + headers = _viewer_headers(client, db, ['endpoints.view'], 'panelviewonly') + + response = client.get(PANEL_URL.format(report.assetid), headers=headers) + assert response.status_code == 403 + + +def test_the_operatorreports_grant_opens_both_routes(client, db, app): + """The positive case: the dedicated grant is sufficient on its own.""" + report = _report_for(client, db, app, 'OPREPORT-GRANTED') + headers = _viewer_headers( + client, db, ['endpoints.view', 'endpoints.operatorreports'], + 'reportviewer') + + image = client.get(IMAGE_URL.format(report.operatorreportid), + headers=headers) + assert image.status_code == 200 + + panel = client.get(PANEL_URL.format(report.assetid), headers=headers) + assert panel.status_code == 200 + reports = panel.get_json()['data']['reports'] + assert len(reports) == 1 + + +def test_operator_reports_panel_declares_its_permission(): + """The panel carries the grant so the tab is not advertised without it.""" + from plugins.endpoints.plugin import EndpointsPlugin + + panels = EndpointsPlugin().get_asset_panels() + panel = next(p for p in panels + if p.get('render') == 'operatorreports') + assert panel['permission'] == 'endpoints.operatorreports'