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'