From 8f115ca3300e2fa213e3f5803af9050a53d08658 Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Tue, 8 Sep 2026 15:22:10 -0400 Subject: [PATCH 1/7] Split shipped monitor bundle into per-monitor policies The product shipped one Configuration Policy carrying every shopfloor app monitor, wrapped in a shipped Site Policy that applied it to every endpoint. That bundled two decisions a site cannot separate: which monitors exist, and which endpoints get them. A site that wanted eDNC on one PC type and MTConnect on another had to delete the shipped policy and rebuild both halves. Ship one Configuration Policy per monitor with no PC type attribution, and stop shipping a Site Policy at all. Composition is a site decision; the product now supplies parts, not an assembled opinion. Shipped Configuration Policies become editable and deletable. Provenance stays a source tag rather than a lock, which is safe because seeding is insert-if- absent and no reconciling seeder exists, so a site edit is never stomped. The stale comments claiming shipped rows were read-only and reconciled described behavior the code never had. --- plugins/computers/api/policy_routes.py | 18 +- plugins/computers/models/policy.py | 19 +- .../versions/0006_endpoint_monitors.py | 5 +- .../versions/0007_shipped_monitor_policy.py | 22 +- .../0019_split_shipped_monitor_policies.py | 318 ++++++++++++++++++ tests/test_plugin_migrations.py | 4 +- .../test_endpoints_shipped_monitor_policy.py | 141 -------- .../test_endpoints_split_monitor_policies.py | 224 ++++++++++++ 8 files changed, 591 insertions(+), 160 deletions(-) create mode 100644 plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py delete mode 100644 tests/test_plugins/test_endpoints_shipped_monitor_policy.py create mode 100644 tests/test_plugins/test_endpoints_split_monitor_policies.py diff --git a/plugins/computers/api/policy_routes.py b/plugins/computers/api/policy_routes.py index 72f350d5..dde1a342 100644 --- a/plugins/computers/api/policy_routes.py +++ b/plugins/computers/api/policy_routes.py @@ -19,7 +19,7 @@ from shopdb.api import (db, AuditLog, success_response, error_response, from .routes import computers_bp from ..models import (ConfigurationPolicy, ConfigurationPolicyAction, BusinessPolicy, BusinessPolicyItem, POLICY_SOURCE_LOCAL, - POLICY_SOURCE_AUTOAPP, + POLICY_SOURCE_AUTOAPP, POLICY_SOURCE_SHIPPED, ACTION_MODE_DESIRED, ACTION_MODE_GATE) from ..policyflag import policy_flag_enabled from ..services.apppolicy import disown_if_generated @@ -262,8 +262,12 @@ def update_configuration_policy(policy_id: int): 'Configuration policy not found', http_code=404) # An autoapp row is generated from an Application's "Required on all PCs" # checkbox and IS editable: editing it is precisely how an admin takes - # ownership. Genuinely external sources (shipped/managed) stay read-only. - if policy.source not in (POLICY_SOURCE_LOCAL, POLICY_SOURCE_AUTOAPP): + # ownership. A shipped row is editable for the same reason - the product + # ships a starting point, not an opinion about one site's floor - and keeps + # source='shipped' so provenance survives the edit. Genuinely external + # sources (a future managed owner) stay read-only. + if policy.source not in (POLICY_SOURCE_LOCAL, POLICY_SOURCE_AUTOAPP, + POLICY_SOURCE_SHIPPED): return error_response(ErrorCodes.CONFLICT, 'Policy is managed externally and is read-only', http_code=409) @@ -326,9 +330,11 @@ def delete_configuration_policy(policy_id: int): if not policy: return error_response(ErrorCodes.NOT_FOUND, 'Configuration policy not found', http_code=404) - # As with update: a generated row is the site's to remove, an externally - # managed one is not. - if policy.source not in (POLICY_SOURCE_LOCAL, POLICY_SOURCE_AUTOAPP): + # As with update: a generated or shipped row is the site's to remove, an + # externally managed one is not. A site that does not run MTConnect should + # be able to delete its monitor policy outright rather than carry it. + if policy.source not in (POLICY_SOURCE_LOCAL, POLICY_SOURCE_AUTOAPP, + POLICY_SOURCE_SHIPPED): return error_response(ErrorCodes.CONFLICT, 'Policy is managed externally and is read-only', http_code=409) diff --git a/plugins/computers/models/policy.py b/plugins/computers/models/policy.py index 82e16c00..994bc4d8 100644 --- a/plugins/computers/models/policy.py +++ b/plugins/computers/models/policy.py @@ -27,10 +27,21 @@ from .jsontext import JSONText # owner. Kept a plain string, not an enum column, so adding a source is data. POLICY_SOURCE_LOCAL = 'local' -# Product-owned rows we ship (the shopfloor app monitor bundle). Same read-only -# treatment as any non-local source: the CRUD API refuses to edit or delete -# them, the seeder reconciles them. Distinct from 'local' so a site's own -# policies are never confused with what the product shipped. +# Rows the product seeds (the shopfloor monitor policies, one per monitor). +# Distinct from 'local' so a site's own policies are never confused with what +# the product shipped, and so the UI can badge the origin. +# +# A shipped row is EDITABLE and DELETABLE, unlike a genuinely external source. +# The product ships a starting point, not an opinion about one site's floor: a +# plant that runs no MTConnect must be able to drop that policy. source stays +# 'shipped' through an edit, so provenance is a fact about where the row came +# from rather than a claim about who controls it now. +# +# Nothing reconciles these rows. The seeding migrations are insert-if-absent +# (they SELECT by name and return early if it exists), so an upgrade cannot +# stomp a site's edits. That property is load-bearing now that the rows are +# editable - do not replace it with a reconciling seeder without adding a +# dirty flag first. POLICY_SOURCE_SHIPPED = 'shipped' # Rows generated FOR US by another surface rather than authored by hand: today diff --git a/plugins/endpoints/migrations/versions/0006_endpoint_monitors.py b/plugins/endpoints/migrations/versions/0006_endpoint_monitors.py index db0bc87c..aa016f4b 100644 --- a/plugins/endpoints/migrations/versions/0006_endpoint_monitors.py +++ b/plugins/endpoints/migrations/versions/0006_endpoint_monitors.py @@ -13,8 +13,9 @@ so the catalog matches what a deployed agent already watches: process agent.exe -> MTConnect service MTConnect UDC Agent -> MTConnect -Shipped rows are product-owned (source='shipped'): the CRUD API refuses to edit -or delete them. Launch paths mirror the agent's launchpaths block; collector and +Shipped rows (source='shipped') keep their kind and displayname - that pair is +the identity the monitor policies match on - but a site may edit their paths and +active flag. Launch paths mirror the agent's launchpaths block; collector and log paths are left null until site paths are supplied. Idempotent: creation checks the live schema first so a db.create_all() site diff --git a/plugins/endpoints/migrations/versions/0007_shipped_monitor_policy.py b/plugins/endpoints/migrations/versions/0007_shipped_monitor_policy.py index e7672199..49e04b0f 100644 --- a/plugins/endpoints/migrations/versions/0007_shipped_monitor_policy.py +++ b/plugins/endpoints/migrations/versions/0007_shipped_monitor_policy.py @@ -1,12 +1,22 @@ -"""Ship the shopfloor app monitors as a read-only Site Policy. +"""Ship the shopfloor app monitors as a Site Policy bundle. + +SUPERSEDED BY 0019. This migration is kept because installs have stamped it, but +what it seeds no longer survives an upgrade: 0019 splits this single bundle into +one Configuration Policy per monitor and REMOVES the shipped Site Policy, so +sites compose their own. Read this for history; do not extend it. + +Two claims below were true when written and are now false, corrected here rather +than left to mislead: the seeded rows are NOT read-only (0019 admits 'shipped' +at the policy CRUD guards), and no seeder reconciles them - the idempotency +check at the top of upgrade() returns early if the policy exists, which is what +keeps a site's edits safe across upgrades. Wraps the four shipped endpointmonitors (eDNC, Universal Data Collector, -MTConnect process, MTConnect UDC Agent service) in a product-owned policy bundle -so the agent stops carrying them as hardcoded appsettings.json defaults and reads -its watch-list from the server instead. +MTConnect process, MTConnect UDC Agent service) in a policy bundle so the agent +stops carrying them as hardcoded appsettings.json defaults and reads its +watch-list from the server instead. -Seeds two computers-plugin rows, both source='shipped' (read-only in the UI, the -CRUD API refuses to edit or delete them): +Seeds two computers-plugin rows, both source='shipped': ConfigurationPolicy "Shopfloor App Monitors" scope {"type":"true"} (every endpoint), one action per shipped monitor, diff --git a/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py b/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py new file mode 100644 index 00000000..69d1028b --- /dev/null +++ b/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py @@ -0,0 +1,318 @@ +"""Split the shipped monitor bundle into one policy per monitor. + +0007 seeded a single Configuration Policy "Shopfloor App Monitors" carrying all +four shipped monitor actions, wrapped in a shipped Site (business) Policy of the +same name. That bundle was all-or-nothing: a site that runs eDNC but no +MTConnect could not detach the half it does not have, because the whole row was +product-owned and read-only. + +This migration replaces it with one Configuration Policy per shipped monitor: + + Monitor: eDNC (process) + Monitor: Universal Data Collector (process) + Monitor: MTConnect (process) + Monitor: MTConnect (service) + +Each carries exactly one 'monitor:' action, mode='gate', +scope {"type":"true"} - no PC type attribution. The name carries the kind +because displayname alone is not unique: MTConnect ships as both a process and +a service, and configurationpolicies.name is UNIQUE. + +source stays 'shipped'. Provenance is still true - the product did ship these - +and the UI keeps showing where they came from. What changes is that the CRUD +guards now admit 'shipped' for edit/delete, so a site can retune or drop one +without losing the record of its origin. + +The shipped SITE policy is REMOVED and not replaced. Composition is a site +decision: which monitors apply to which endpoints, and at what priority, is +exactly the judgement a plant makes about its own floor. Shipping one meant +every install inherited "apply all four to every endpoint" as a starting point +that could not be edited. Sites now build their own from the per-monitor pieces. + +Removing it is the behaviour change to be aware of on upgrade: monitors that +were attached through that bundle stop being attached until a site policy +composes them. It is deliberate and it is the point of the split - see the +"off by default" direction in the 2026-09-08 plan - but it is not silent, since +the per-monitor policies are listed and ready to compose. + +Idempotent on the policy names, so a re-run neither duplicates the split rows +nor resurrects the bundle. Guards on the policy tables being present and no-ops +if they are not, matching 0007. + +Revision ID: endpoints0019splitmonitorpolicies +Revises: endpoints0018endpointlocks +""" +from alembic import op + +import sqlalchemy as sa + +from datetime import datetime + + +revision = 'endpoints0019splitmonitorpolicies' +down_revision = 'endpoints0018endpointlocks' +branch_labels = None +depends_on = None + + +BUNDLE_NAME = 'Shopfloor App Monitors' + +# The shipped monitors, by identity (kind, displayname) - never by hardcoded id, +# so this survives id drift across installs exactly as 0007 does. +_SHIPPED_MONITORS = ( + ('process', 'eDNC'), + ('process', 'Universal Data Collector'), + ('process', 'MTConnect'), + ('service', 'MTConnect'), +) + +_REQUIRED = {'endpointmonitors', 'configurationpolicies', + 'configurationpolicyactions', 'businesspolicies', + 'businesspolicyitems'} + + +def _tables(bind): + from sqlalchemy import inspect + + return set(inspect(bind).get_table_names()) + + +def _policy_name(kind, displayname): + return f'Monitor: {displayname} ({kind})' + + +def _policy_description(kind, displayname): + noun = 'process' if kind == 'process' else 'service' + return (f'Product-shipped watch-list entry for the {displayname} {noun}. ' + f'Attach it to a Site Policy to apply it to endpoints.') + + +def _config_policy_id(bind, name): + return bind.execute( + sa.text('SELECT configurationpolicyid FROM configurationpolicies ' + 'WHERE name = :name'), + {'name': name}).scalar() + + +def _drop_config_policy(bind, name): + """Delete a configuration policy, its actions, and any items citing it.""" + policyid = _config_policy_id(bind, name) + if policyid is None: + return + # Clear children explicitly rather than lean on ON DELETE CASCADE, which + # sqlite does not enforce by default. + bind.execute( + sa.text('DELETE FROM businesspolicyitems ' + 'WHERE configurationpolicyid = :id'), {'id': policyid}) + bind.execute( + sa.text('DELETE FROM configurationpolicyactions ' + 'WHERE configurationpolicyid = :id'), {'id': policyid}) + bind.execute( + sa.text('DELETE FROM configurationpolicies ' + 'WHERE configurationpolicyid = :id'), {'id': policyid}) + + +def _drop_business_policy(bind, name): + policyid = bind.execute( + sa.text('SELECT businesspolicyid FROM businesspolicies ' + 'WHERE name = :name AND source = :source'), + {'name': name, 'source': 'shipped'}).scalar() + if policyid is None: + return + bind.execute( + sa.text('DELETE FROM businesspolicyitems WHERE businesspolicyid = :id'), + {'id': policyid}) + bind.execute( + sa.text('DELETE FROM businesspolicies WHERE businesspolicyid = :id'), + {'id': policyid}) + + +def _monitor_id(bind, kind, displayname): + return bind.execute( + sa.text('SELECT endpointmonitorid FROM endpointmonitors ' + 'WHERE kind = :kind AND displayname = :displayname'), + {'kind': kind, 'displayname': displayname}).scalar() + + +def upgrade(): + bind = op.get_bind() + if not _REQUIRED.issubset(_tables(bind)): + return + + now = datetime.utcnow() + + config_policies = sa.table( + 'configurationpolicies', + sa.column('name', sa.String), + sa.column('description', sa.Text), + sa.column('scopejson', sa.Text), + sa.column('source', sa.String), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + actions = sa.table( + 'configurationpolicyactions', + sa.column('configurationpolicyid', sa.Integer), + sa.column('action', sa.String), + sa.column('mode', sa.String), + sa.column('desiredstate', sa.Text), + sa.column('params', sa.Text), + sa.column('intervalminutes', sa.Integer), + sa.column('sortorder', sa.Integer), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + + for kind, displayname in _SHIPPED_MONITORS: + name = _policy_name(kind, displayname) + if _config_policy_id(bind, name) is not None: + continue + monitorid = _monitor_id(bind, kind, displayname) + if monitorid is None: + # A site that never had this monitor gets no policy for it. + continue + + op.bulk_insert(config_policies, [{ + 'name': name, + 'description': _policy_description(kind, displayname), + 'scopejson': '{"type": "true"}', + 'source': 'shipped', + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + }]) + op.bulk_insert(actions, [{ + 'configurationpolicyid': _config_policy_id(bind, name), + 'action': f'monitor:{monitorid}', + 'mode': 'gate', + 'desiredstate': None, + 'params': None, + 'intervalminutes': None, + 'sortorder': 0, + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + }]) + + # Retire the monolith. The business policy goes first so its item rows are + # gone before the configuration policy it cited is removed. + _drop_business_policy(bind, BUNDLE_NAME) + _drop_config_policy(bind, BUNDLE_NAME) + + +def downgrade(): + """Reinstate the single shipped bundle and drop the per-monitor policies. + + Rebuilt from the same monitor identities rather than from anything the + upgrade stashed, so it lands in the state 0007 produces regardless of what + a site did to the split rows in between. + """ + bind = op.get_bind() + if not _REQUIRED.issubset(_tables(bind)): + return + + now = datetime.utcnow() + + monitor_ids = [] + for kind, displayname in _SHIPPED_MONITORS: + monitorid = _monitor_id(bind, kind, displayname) + if monitorid is not None: + monitor_ids.append(monitorid) + + for kind, displayname in _SHIPPED_MONITORS: + _drop_config_policy(bind, _policy_name(kind, displayname)) + + if not monitor_ids or _config_policy_id(bind, BUNDLE_NAME) is not None: + return + + config_policies = sa.table( + 'configurationpolicies', + sa.column('name', sa.String), + sa.column('description', sa.Text), + sa.column('scopejson', sa.Text), + sa.column('source', sa.String), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + op.bulk_insert(config_policies, [{ + 'name': BUNDLE_NAME, + 'description': 'Product-shipped watch-list for the shopfloor apps ' + '(eDNC, Universal Data Collector, MTConnect). Read-only.', + 'scopejson': '{"type": "true"}', + 'source': 'shipped', + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + }]) + config_policy_id = _config_policy_id(bind, BUNDLE_NAME) + + actions = sa.table( + 'configurationpolicyactions', + sa.column('configurationpolicyid', sa.Integer), + sa.column('action', sa.String), + sa.column('mode', sa.String), + sa.column('desiredstate', sa.Text), + sa.column('params', sa.Text), + sa.column('intervalminutes', sa.Integer), + sa.column('sortorder', sa.Integer), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + op.bulk_insert(actions, [{ + 'configurationpolicyid': config_policy_id, + 'action': f'monitor:{monitorid}', + 'mode': 'gate', + 'desiredstate': None, + 'params': None, + 'intervalminutes': None, + 'sortorder': index, + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + } for index, monitorid in enumerate(monitor_ids)]) + + business_policies = sa.table( + 'businesspolicies', + sa.column('name', sa.String), + sa.column('description', sa.Text), + sa.column('priority', sa.Integer), + sa.column('source', sa.String), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + op.bulk_insert(business_policies, [{ + 'name': BUNDLE_NAME, + 'description': 'Product-shipped Site Policy that applies the shopfloor ' + 'app monitors to every endpoint. Read-only.', + 'priority': 100, + 'source': 'shipped', + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + }]) + business_policy_id = bind.execute( + sa.text('SELECT businesspolicyid FROM businesspolicies ' + 'WHERE name = :name'), {'name': BUNDLE_NAME}).scalar() + + items = sa.table( + 'businesspolicyitems', + sa.column('businesspolicyid', sa.Integer), + sa.column('configurationpolicyid', sa.Integer), + sa.column('sortorder', sa.Integer), + sa.column('createddate', sa.DateTime), + sa.column('modifieddate', sa.DateTime), + sa.column('isactive', sa.Boolean), + ) + op.bulk_insert(items, [{ + 'businesspolicyid': business_policy_id, + 'configurationpolicyid': config_policy_id, + 'sortorder': 0, + 'createddate': now, + 'modifieddate': now, + 'isactive': True, + }]) diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index ed669573..146c748a 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -113,7 +113,9 @@ EXPECTED_HEAD_REVISION['warranty'] = 'warranty0002proof' # and endpointmonitorimpacts. # 0018 adds endpointlocks: the admin quarantine refusing actions from non-admins # on one endpoint, a caller-authority axis separate from the policy hold. -EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0018endpointlocks' +# 0019 splits the 0007 bundle into one shipped Configuration Policy per monitor +# and drops the shipped Business Policy, so sites compose their own. +EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0019splitmonitorpolicies' # Plugins built after the cutover: their 0001 baseline really creates tables the # core chain never owned. diff --git a/tests/test_plugins/test_endpoints_shipped_monitor_policy.py b/tests/test_plugins/test_endpoints_shipped_monitor_policy.py deleted file mode 100644 index 901d586d..00000000 --- a/tests/test_plugins/test_endpoints_shipped_monitor_policy.py +++ /dev/null @@ -1,141 +0,0 @@ -"""The shipped shopfloor app monitors ride a read-only Site Policy. - -Migration endpoints0007shippedpolicy offloads the eDNC/UDC/MTConnect monitors -from the agent's hardcoded appsettings.json into a product-owned policy bundle: -a shipped ConfigurationPolicy "Shopfloor App Monitors" (global scope, one -desired-state monitor action per shipped monitor) wrapped in a shipped -BusinessPolicy "Shopfloor App Monitors". - -Run the endpoints chain against a fresh engine, alongside the computers policy -tables the bundle writes into, and assert the bundle exists, is source='shipped' -(read-only), and attaches every shipped monitor as a desired/running:true action. -""" - -import importlib.util -import json -from pathlib import Path - -from sqlalchemy import create_engine, inspect, text -from alembic.migration import MigrationContext -from alembic.operations import Operations - - -_ROOT = Path(__file__).resolve().parent.parent.parent -_ENDPOINTS_VERSIONS = _ROOT / 'plugins' / 'endpoints' / 'migrations' / 'versions' -_COMPUTERS_VERSIONS = _ROOT / 'plugins' / 'computers' / 'migrations' / 'versions' - -CONFIG_POLICY_NAME = 'Shopfloor App Monitors' -BUSINESS_POLICY_NAME = 'Shopfloor App Monitors' - - -def _load(path, prefix): - spec = importlib.util.spec_from_file_location(prefix + path.stem, str(path)) - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - return module - - -def _run_bundle_migrations(connection): - """Build the policy tables (computers 0004), the monitor catalog (endpoints - 0006) and the shipped bundle (endpoints 0007) on one engine, in order. - - The core baseline creates every plugin's tables before per-plugin chains run, - so on a real deploy the computers policy tables already exist when endpoints - 0007 runs. Here we stand up exactly the three migrations the bundle depends - on - the earlier endpoints revisions carry FKs to core tables that a bare - engine does not have, and are irrelevant to what 0007 seeds. - """ - operations = Operations(MigrationContext.configure(connection)) - operations._install_proxy() - try: - _load(_COMPUTERS_VERSIONS / '0004_policies.py', - 'computers_mig_').upgrade() - _load(_ENDPOINTS_VERSIONS / '0006_endpoint_monitors.py', - 'endpoints_mig_0006_').upgrade() - _load(_ENDPOINTS_VERSIONS / '0007_shipped_monitor_policy.py', - 'endpoints_mig_0007_').upgrade() - finally: - operations._remove_proxy() - connection.commit() - - -def test_shipped_bundle_seeds_read_only_site_policy(): - engine = create_engine('sqlite://') - with engine.connect() as connection: - _run_bundle_migrations(connection) - - config = connection.execute(text( - 'SELECT configurationpolicyid, source, scopejson ' - 'FROM configurationpolicies WHERE name = :name'), - {'name': CONFIG_POLICY_NAME}).first() - assert config is not None, 'shipped Configuration Policy was not seeded' - config_id, config_source, scopejson = config - assert config_source == 'shipped' - assert json.loads(scopejson) == {'type': 'true'} - - business = connection.execute(text( - 'SELECT businesspolicyid, source FROM businesspolicies ' - 'WHERE name = :name'), {'name': BUSINESS_POLICY_NAME}).first() - assert business is not None, 'shipped Business Policy was not seeded' - business_id, business_source = business - assert business_source == 'shipped' - - item = connection.execute(text( - 'SELECT configurationpolicyid FROM businesspolicyitems ' - 'WHERE businesspolicyid = :id'), {'id': business_id}).first() - assert item is not None and item[0] == config_id - - -def test_shipped_bundle_attaches_every_shipped_monitor(): - engine = create_engine('sqlite://') - with engine.connect() as connection: - _run_bundle_migrations(connection) - - monitor_ids = {row[0] for row in connection.execute(text( - 'SELECT endpointmonitorid FROM endpointmonitors ' - "WHERE source = 'shipped'"))} - assert monitor_ids, 'no shipped monitors to attach' - - config_id = connection.execute(text( - 'SELECT configurationpolicyid FROM configurationpolicies ' - 'WHERE name = :name'), {'name': CONFIG_POLICY_NAME}).scalar() - - actions = connection.execute(text( - 'SELECT action, mode, desiredstate FROM configurationpolicyactions ' - 'WHERE configurationpolicyid = :id'), {'id': config_id}).all() - - attached_ids = set() - for action, mode, desiredstate in actions: - assert action.startswith('monitor:') - # Gate, not desired: the shipped bundle reports state and exposes - # Start/Stop in the UI, but never auto-starts a process. A desired - # running state is a deliberate per-monitor opt-in, not shipped. - assert mode == 'gate' - assert desiredstate is None - attached_ids.add(int(action[len('monitor:'):])) - - assert attached_ids == monitor_ids - - -def test_shipped_bundle_is_idempotent(): - engine = create_engine('sqlite://') - with engine.connect() as connection: - _run_bundle_migrations(connection) - # Re-run only the shipped-bundle migration; it must not duplicate rows. - operations = Operations(MigrationContext.configure(connection)) - operations._install_proxy() - try: - _load(_ENDPOINTS_VERSIONS / '0007_shipped_monitor_policy.py', - 'endpoints_mig_rerun_').upgrade() - finally: - operations._remove_proxy() - connection.commit() - - config_count = connection.execute(text( - 'SELECT COUNT(*) FROM configurationpolicies WHERE name = :name'), - {'name': CONFIG_POLICY_NAME}).scalar() - business_count = connection.execute(text( - 'SELECT COUNT(*) FROM businesspolicies WHERE name = :name'), - {'name': BUSINESS_POLICY_NAME}).scalar() - assert config_count == 1 - assert business_count == 1 diff --git a/tests/test_plugins/test_endpoints_split_monitor_policies.py b/tests/test_plugins/test_endpoints_split_monitor_policies.py new file mode 100644 index 00000000..41dd0bf1 --- /dev/null +++ b/tests/test_plugins/test_endpoints_split_monitor_policies.py @@ -0,0 +1,224 @@ +"""The shipped shopfloor monitors ride one Configuration Policy per monitor. + +0007 seeded a single "Shopfloor App Monitors" bundle wrapped in a shipped Site +Policy. 0019 replaced that with one policy per monitor and removed the shipped +Site Policy, so sites compose their own. + +Run the endpoints chain against a fresh engine, alongside the computers policy +tables the migrations write into, and assert the post-0019 shape: a policy per +shipped monitor, each carrying exactly one gate-mode monitor action with no PC +type attribution, and no shipped Site Policy left behind. + +The idempotency test is the load-bearing one. Nothing reconciles shipped rows, +so insert-if-absent is the only thing protecting a site's edits from an upgrade +that re-runs a seeding migration - see POLICY_SOURCE_SHIPPED in +plugins/computers/models/policy.py. +""" + +import importlib.util +import json +from pathlib import Path + +from sqlalchemy import create_engine, text +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +_ROOT = Path(__file__).resolve().parent.parent.parent +_ENDPOINTS_VERSIONS = _ROOT / 'plugins' / 'endpoints' / 'migrations' / 'versions' +_COMPUTERS_VERSIONS = _ROOT / 'plugins' / 'computers' / 'migrations' / 'versions' + +BUNDLE_NAME = 'Shopfloor App Monitors' +SPLIT_MIGRATION = '0019_split_shipped_monitor_policies.py' + +_SHIPPED_MONITORS = ( + ('process', 'eDNC'), + ('process', 'Universal Data Collector'), + ('process', 'MTConnect'), + ('service', 'MTConnect'), +) + + +def _policy_name(kind, displayname): + return f'Monitor: {displayname} ({kind})' + + +def _load(path, prefix): + spec = importlib.util.spec_from_file_location(prefix + path.stem, str(path)) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _run(connection, *loaded): + operations = Operations(MigrationContext.configure(connection)) + operations._install_proxy() + try: + for module in loaded: + module.upgrade() + finally: + operations._remove_proxy() + connection.commit() + + +def _run_bundle_migrations(connection): + """Build the policy tables, monitor catalog, bundle and split, in order. + + The core baseline creates every plugin's tables before per-plugin chains + run, so on a real deploy the computers policy tables already exist. Here we + stand up exactly the migrations the bundle depends on - the earlier + endpoints revisions carry FKs to core tables a bare engine does not have. + """ + _run( + connection, + _load(_COMPUTERS_VERSIONS / '0004_policies.py', 'computers_mig_'), + _load(_ENDPOINTS_VERSIONS / '0006_endpoint_monitors.py', + 'endpoints_mig_0006_'), + _load(_ENDPOINTS_VERSIONS / '0007_shipped_monitor_policy.py', + 'endpoints_mig_0007_'), + _load(_ENDPOINTS_VERSIONS / SPLIT_MIGRATION, 'endpoints_mig_0019_'), + ) + + +def _monitor_id(connection, kind, displayname): + return connection.execute(text( + 'SELECT endpointmonitorid FROM endpointmonitors ' + 'WHERE kind = :kind AND displayname = :displayname'), + {'kind': kind, 'displayname': displayname}).scalar() + + +def test_split_seeds_one_policy_per_shipped_monitor(): + engine = create_engine('sqlite://') + with engine.connect() as connection: + _run_bundle_migrations(connection) + + for kind, displayname in _SHIPPED_MONITORS: + name = _policy_name(kind, displayname) + row = connection.execute(text( + 'SELECT configurationpolicyid, source, scopejson ' + 'FROM configurationpolicies WHERE name = :name'), + {'name': name}).first() + assert row is not None, f'{name} was not seeded' + policyid, source, scopejson = row + assert source == 'shipped' + # No PC type attribution: the policy applies wherever a site + # composes it, and carries no opinion about which types match. + assert json.loads(scopejson) == {'type': 'true'} + + actions = connection.execute(text( + 'SELECT action, mode, desiredstate ' + 'FROM configurationpolicyactions ' + 'WHERE configurationpolicyid = :id'), {'id': policyid}).all() + assert len(actions) == 1, f'{name} should carry exactly one action' + action, mode, desiredstate = actions[0] + # Gate, not desired: a monitor reports state and exposes Start/Stop + # but never auto-starts a process. A desired running state is a + # deliberate per-monitor opt-in, not something the product ships on. + assert mode == 'gate' + assert desiredstate is None + expected = _monitor_id(connection, kind, displayname) + assert action == f'monitor:{expected}' + + +def test_split_removes_the_shipped_site_policy(): + engine = create_engine('sqlite://') + with engine.connect() as connection: + _run_bundle_migrations(connection) + + business = connection.execute(text( + 'SELECT COUNT(*) FROM businesspolicies WHERE name = :name'), + {'name': BUNDLE_NAME}).scalar() + assert business == 0, 'the shipped Site Policy must not survive 0019' + + config = connection.execute(text( + 'SELECT COUNT(*) FROM configurationpolicies WHERE name = :name'), + {'name': BUNDLE_NAME}).scalar() + assert config == 0, 'the bundled Configuration Policy must be retired' + + items = connection.execute(text( + 'SELECT COUNT(*) FROM businesspolicyitems')).scalar() + assert items == 0 + + +def test_no_shipped_business_policy_remains(): + """Sites build their own composition; the product ships none.""" + engine = create_engine('sqlite://') + with engine.connect() as connection: + _run_bundle_migrations(connection) + + shipped = connection.execute(text( + "SELECT COUNT(*) FROM businesspolicies WHERE source = 'shipped'" + )).scalar() + assert shipped == 0 + + +def test_split_is_idempotent_and_preserves_site_edits(): + """A re-run must not duplicate rows nor stomp an edit. + + Nothing reconciles shipped rows, so this is the whole protection: the + migration returns early when the policy name is already present. If that + ever becomes a reconciling seeder, this test is what fails. + """ + engine = create_engine('sqlite://') + with engine.connect() as connection: + _run_bundle_migrations(connection) + + edited = _policy_name('process', 'eDNC') + connection.execute(text( + 'UPDATE configurationpolicies SET description = :description ' + 'WHERE name = :name'), + {'description': 'site edited', 'name': edited}) + connection.commit() + + _run(connection, _load(_ENDPOINTS_VERSIONS / SPLIT_MIGRATION, + 'endpoints_mig_rerun_')) + + for kind, displayname in _SHIPPED_MONITORS: + count = connection.execute(text( + 'SELECT COUNT(*) FROM configurationpolicies ' + 'WHERE name = :name'), + {'name': _policy_name(kind, displayname)}).scalar() + assert count == 1 + + description = connection.execute(text( + 'SELECT description FROM configurationpolicies WHERE name = :name'), + {'name': edited}).scalar() + assert description == 'site edited', 'a re-run stomped a site edit' + + +def test_downgrade_restores_the_bundle(): + engine = create_engine('sqlite://') + with engine.connect() as connection: + _run_bundle_migrations(connection) + + split = _load(_ENDPOINTS_VERSIONS / SPLIT_MIGRATION, + 'endpoints_mig_down_') + operations = Operations(MigrationContext.configure(connection)) + operations._install_proxy() + try: + split.downgrade() + finally: + operations._remove_proxy() + connection.commit() + + config_id = connection.execute(text( + 'SELECT configurationpolicyid FROM configurationpolicies ' + 'WHERE name = :name'), {'name': BUNDLE_NAME}).scalar() + assert config_id is not None, 'downgrade did not restore the bundle' + + business_id = connection.execute(text( + 'SELECT businesspolicyid FROM businesspolicies WHERE name = :name'), + {'name': BUNDLE_NAME}).scalar() + assert business_id is not None + + item = connection.execute(text( + 'SELECT configurationpolicyid FROM businesspolicyitems ' + 'WHERE businesspolicyid = :id'), {'id': business_id}).first() + assert item is not None and item[0] == config_id + + for kind, displayname in _SHIPPED_MONITORS: + count = connection.execute(text( + 'SELECT COUNT(*) FROM configurationpolicies ' + 'WHERE name = :name'), + {'name': _policy_name(kind, displayname)}).scalar() + assert count == 0, 'downgrade left the split policies behind' From 0b5546bcef1e7afc5269d5dfab6c09c127af0e0a Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Tue, 8 Sep 2026 16:47:56 -0400 Subject: [PATCH 2/7] Cap plugin revision ids at the 32 chars MySQL stores Rolling the containers failed on strict MySQL: the endpoints chain could not stamp its new head with "Data too long for column version_num". Plugin version tables declare version_num VARCHAR(32) and the new revision id was 33 chars. Nothing in the suite caught it. The tests run on SQLite, which does not enforce declared column widths, so an over-long id passes locally and fails only on a real deploy - after the migration body has run and only the stamp fails. The rollback was clean here, but the failure lands at the worst possible point. Shorten the id and assert the limit in the test that already parses every plugin revision, so the next over-long id fails on a laptop instead of during a roll. Core is deliberately not covered: its alembic_version is VARCHAR(128), and it already carries ids longer than 32 that are correct. --- .../versions/0019_split_shipped_monitor_policies.py | 6 ++++-- tests/test_plugin_migrations.py | 13 ++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py b/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py index 69d1028b..92ea28dd 100644 --- a/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py +++ b/plugins/endpoints/migrations/versions/0019_split_shipped_monitor_policies.py @@ -39,7 +39,7 @@ Idempotent on the policy names, so a re-run neither duplicates the split rows nor resurrects the bundle. Guards on the policy tables being present and no-ops if they are not, matching 0007. -Revision ID: endpoints0019splitmonitorpolicies +Revision ID: endpoints0019splitpolicies Revises: endpoints0018endpointlocks """ from alembic import op @@ -49,7 +49,9 @@ import sqlalchemy as sa from datetime import datetime -revision = 'endpoints0019splitmonitorpolicies' +# Alembic stores this in alembic_version.version_num, a VARCHAR(32). A longer +# id passes on SQLite and fails on strict MySQL with "Data too long". +revision = 'endpoints0019splitpolicies' down_revision = 'endpoints0018endpointlocks' branch_labels = None depends_on = None diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index 146c748a..36930f35 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -115,7 +115,7 @@ EXPECTED_HEAD_REVISION['warranty'] = 'warranty0002proof' # on one endpoint, a caller-authority axis separate from the policy hold. # 0019 splits the 0007 bundle into one shipped Configuration Policy per monitor # and drops the shipped Business Policy, so sites compose their own. -EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0019splitmonitorpolicies' +EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0019splitpolicies' # Plugins built after the cutover: their 0001 baseline really creates tables the # core chain never owned. @@ -208,6 +208,17 @@ def test_plugin_has_migration_chain(plugin): if isinstance(node.value, ast.Constant): down = node.value.value assert rev, f"{plugin}: {script.name} has no revision id" + # alembic_version_.version_num is VARCHAR(32). A longer id + # passes every SQLite test and then fails the real deploy on strict + # MySQL with "Data too long for column 'version_num'", after the + # migration body has already run. Caught + # endpoints0019splitmonitorpolicies (33 chars) during a container roll. + # The CORE chain's alembic_version is VARCHAR(128), so core ids are not + # bound by this and are not checked here. + assert len(rev) <= 32, ( + f"{plugin}: {script.name} revision id {rev!r} is {len(rev)} chars; " + 'alembic_version.version_num holds 32' + ) assert found_down, f"{plugin}: {script.name} has no down_revision" revisions.add(rev) if down is None: From 67e701b538fb51c93f45437e83e2d3801af63841 Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Tue, 8 Sep 2026 17:16:34 -0400 Subject: [PATCH 3/7] Let the policy list edit the rows the API accepts The shipped Configuration Policies rendered with working-looking Edit and Delete buttons that did nothing. The API had already been widened to accept shipped rows, but the buttons still carried :disabled on source !== local, so the write was never sent and the change looked like it had never rolled. Gate both buttons on the set the API actually accepts. The shipped badge stays: it is provenance, not a lock, and it has to survive an edit so a site can still tell a product-supplied row from one of its own. Business Policies keep the local-only rule. Their API guard is unchanged and no shipped Site Policy exists now, so the UI already matches. Cover it with a spec, because the failure mode is a disabled attribute silently disagreeing with a server guard - nothing errors, the button just does nothing. --- .../views/ConfigurationPoliciesList.vue | 16 +++++++++-- .../views/configurationPolicyEditable.spec.js | 27 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 plugins/computers/frontend/views/configurationPolicyEditable.spec.js diff --git a/plugins/computers/frontend/views/ConfigurationPoliciesList.vue b/plugins/computers/frontend/views/ConfigurationPoliciesList.vue index 45eb98ae..b5badc89 100644 --- a/plugins/computers/frontend/views/ConfigurationPoliciesList.vue +++ b/plugins/computers/frontend/views/ConfigurationPoliciesList.vue @@ -42,8 +42,8 @@ - - + + @@ -287,6 +287,18 @@ async function save() { } } +// Mirrors the API guard in plugins/computers/api/policy_routes.py: local, +// autoapp and shipped rows are all editable. A shipped row is a starting point +// the product supplies, not a fixed opinion, and it keeps source='shipped' so +// provenance survives the edit. Only a genuinely external managed owner is +// read-only. Disabling on source==='local' alone stranded shipped rows behind +// dead buttons while the API was accepting the write. +const EDITABLE_SOURCES = ['local', 'autoapp', 'shipped'] + +function iseditable(p) { + return EDITABLE_SOURCES.includes(p.source) +} + async function remove(p) { if (!confirm(`Delete configuration policy "${p.name}"?`)) return try { diff --git a/plugins/computers/frontend/views/configurationPolicyEditable.spec.js b/plugins/computers/frontend/views/configurationPolicyEditable.spec.js new file mode 100644 index 00000000..9dec5bdc --- /dev/null +++ b/plugins/computers/frontend/views/configurationPolicyEditable.spec.js @@ -0,0 +1,27 @@ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { fileURLToPath } from 'url' +import { dirname, join } from 'path' + +const here = dirname(fileURLToPath(import.meta.url)) +const view = readFileSync(join(here, 'ConfigurationPoliciesList.vue'), 'utf8') + +// The API decides what may be edited; the buttons only reflect that decision. +// These drifted once: policy_routes.py was widened to accept shipped rows while +// the buttons still disabled on source !== 'local', so every shipped policy sat +// behind a dead Edit button and the change looked like it had never rolled. +// If the API guard changes, EDITABLE_SOURCES here changes with it. +describe('the configuration policy buttons follow the API guard', () => { + it('treats shipped as editable, not read-only', () => { + expect(view).toContain("EDITABLE_SOURCES = ['local', 'autoapp', 'shipped']") + }) + + it('does not gate the buttons on local alone', () => { + expect(view).not.toContain(":disabled=\"p.source !== 'local'\"") + }) + + it('gates edit and delete on the same rule', () => { + const disabled = view.match(/:disabled="!iseditable\(p\)"/g) || [] + expect(disabled).toHaveLength(2) + }) +}) From fd180991991382ef366522330b5ec7cc4f5055f8 Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Wed, 9 Sep 2026 06:37:02 -0400 Subject: [PATCH 4/7] Gate the script library on the grant that pushes scripts The script library was gated on applications.*, so anyone holding the software-delivery grant could rewrite the bytes of a script that a separately trusted operator later pushed to a bay. The push was gated on endpoints.runscript; the CONTENT of what got pushed was not. An installer is a vendor artefact, but a script here is arbitrary PowerShell this site wrote that the agent runs as SYSTEM, so the write and the push must require the same trust. All six script routes now require endpoints.runscript, including the body download - reading the bytes is not a lesser act than listing them. Core names the permission as a string rather than importing it from the endpoints plugin so the module stays importable with the plugin absent. This tightens access and there is deliberately no backfill: the operator scope reaches applications.create/edit, so operator-scope holders lose the library. Granting them endpoints.runscript on upgrade would undo the change and hand every operator remote SYSTEM code execution, since the same grant gates the push. A test pins that no prebuilt scope confers it. Adds the System Scripts settings page and card. The route requires the permission rather than admin, because the grant is meant for non-admin operators and the sibling admin gate would lock them out of the library the grant exists for. The settings icon map gains terminal and key; the agent-key card had been asking for key since it shipped and silently drawing a puzzle piece. The library had no test coverage before this. --- frontend/src/api/index.js | 7 +- frontend/src/composables/settingsCatalog.js | 6 +- plugins/endpoints/frontend/routes.js | 14 + .../frontend/views/SystemScriptsSettings.vue | 252 ++++++++++++++++++ .../views/systemScriptsSettings.spec.js | 50 ++++ plugins/endpoints/plugin.py | 15 ++ shopdb/core/api/scripts.py | 44 ++- tests/test_core/test_script_library_gate.py | 160 +++++++++++ 8 files changed, 533 insertions(+), 15 deletions(-) create mode 100644 plugins/endpoints/frontend/views/SystemScriptsSettings.vue create mode 100644 plugins/endpoints/frontend/views/systemScriptsSettings.spec.js create mode 100644 tests/test_core/test_script_library_gate.py diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index 29c39600..30fe4c8a 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -827,8 +827,11 @@ export const applicationsApi = { } // ShopDB-hosted script library. Backs run_script delivery: the agent fetches a -// script body by digest with its collector credential and runs it in the -// script's configured run context (system or user). +// script body by digest with its collector credential and runs it as SYSTEM. +// The UI does not offer a run context - scripts exist to trigger an already +// registered scheduled task, and the agent service has no user session to +// borrow. Anything needing a user's own token must be that scheduled task's +// problem, not this library's. export const scriptsApi = { list(params = {}) { return api.get('/scripts', { params }) diff --git a/frontend/src/composables/settingsCatalog.js b/frontend/src/composables/settingsCatalog.js index 0634c7a4..eae99a18 100644 --- a/frontend/src/composables/settingsCatalog.js +++ b/frontend/src/composables/settingsCatalog.js @@ -4,7 +4,7 @@ // (landing) read the merged catalog so plugins add settings cards with no core // edit. Fetched once into a module-level ref and shared across callers. import { ref, computed } from 'vue' -import { Ruler, Wrench, Cog, Package, Settings, Puzzle, SlidersHorizontal, Palette, Bell, Network, Printer, Droplets } from 'lucide-vue-next' +import { Ruler, Wrench, Cog, Package, Settings, Puzzle, SlidersHorizontal, Palette, Bell, Network, Printer, Droplets, Terminal, KeyRound } from 'lucide-vue-next' import api from '../api' import router from '../router' import { settingsGroups } from '../views/settings/settingsNav' @@ -24,6 +24,10 @@ const iconMap = { network: Network, printer: Printer, droplets: Droplets, + terminal: Terminal, + // The endpoints agent-key card has asked for 'key' since it shipped and has + // been silently drawing a puzzle piece the whole time. + key: KeyRound, } function resolveIcon(key) { diff --git a/plugins/endpoints/frontend/routes.js b/plugins/endpoints/frontend/routes.js index 93119886..f573103e 100644 --- a/plugins/endpoints/frontend/routes.js +++ b/plugins/endpoints/frontend/routes.js @@ -23,6 +23,20 @@ export default [ name: 'endpoints-monitors-settings', component: () => import('./views/MonitorsSettings.vue'), meta: { requiresAuth: true, requiresAdmin: true, plugin: 'endpoints' } + }, + // requiresPermission, not requiresAdmin: endpoints.runscript is grantable to + // non-admin operators, and the sibling pages' admin gate would lock the + // holders of the grant out of the library they are trusted to manage. The + // API enforces the same permission, so this only keeps the menu honest. + { + path: 'settings/endpoints/scripts', + name: 'endpoints-scripts-settings', + component: () => import('./views/SystemScriptsSettings.vue'), + meta: { + requiresAuth: true, + requiresPermission: 'endpoints.runscript', + plugin: 'endpoints' + } } ] diff --git a/plugins/endpoints/frontend/views/SystemScriptsSettings.vue b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue new file mode 100644 index 00000000..33ef0cba --- /dev/null +++ b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue @@ -0,0 +1,252 @@ + + + + + diff --git a/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js b/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js new file mode 100644 index 00000000..38ce5877 --- /dev/null +++ b/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js @@ -0,0 +1,50 @@ +/** + * System Scripts settings page. + * + * Two things are worth pinning here and nothing else is: + * + * 1. The page must NOT offer a run context. Scripts run as SYSTEM because the + * agent is a service with no user session; exposing a picker would let + * someone author a script that reads as "run as the logged-on user" and + * silently does not. + * 2. The SYSTEM warning must be present on both the list and the add/edit + * form. Someone pasting a script into the modal may never have read the + * page copy above it. + * + * Asserted against the file text, matching configurationPolicyEditable.spec.js. + */ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { join, dirname } from 'path' +import { fileURLToPath } from 'url' + +const here = dirname(fileURLToPath(import.meta.url)) +const source = readFileSync(join(here, 'SystemScriptsSettings.vue'), 'utf8') +const routes = readFileSync(join(here, '..', 'routes.js'), 'utf8') + +describe('SystemScriptsSettings', () => { + it('warns that scripts run as SYSTEM on the list page and in the form', () => { + const warnings = source.match(/runs? as SYSTEM/g) || [] + expect(warnings.length).toBeGreaterThanOrEqual(2) + }) + + it('does not offer a run context picker', () => { + expect(source).not.toMatch(/runcontext/i) + }) + + it('does not send a runcontext in its save payload', () => { + const payload = source.slice(source.indexOf('const payload = {')) + expect(payload.slice(0, 200)).not.toMatch(/runcontext/i) + }) + + it('flags a script with no body as unrunnable', () => { + expect(source).toMatch(/hasbody/) + expect(source).toMatch(/cannot run/i) + }) + + it('is gated on the standalone script permission, not admin', () => { + const route = routes.slice(routes.indexOf("'settings/endpoints/scripts'")) + expect(route).toMatch(/requiresPermission: 'endpoints\.runscript'/) + expect(route.slice(0, 300)).not.toMatch(/requiresAdmin/) + }) +}) diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 5c69e98c..aa05540f 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -320,6 +320,21 @@ class EndpointsPlugin(BasePlugin): 'agent, and configure what it reports.', 'position': 27, }, + # Its own card rather than a section of the agent key page: the + # audience differs. Key management is admin-only, while the script + # library is reachable by any endpoints.runscript holder, and + # burying it under an admin page would hide it from exactly the + # operators the grant exists for. + { + 'group': 'Endpoints', + 'to': '/settings/endpoints/scripts', + 'icon': 'terminal', + 'title': 'System Scripts', + 'description': 'PowerShell the agent can run on an endpoint. ' + 'Runs as SYSTEM; used to trigger scheduled ' + 'tasks already registered on the endpoint.', + 'position': 28, + }, ] def get_navigation_items(self) -> List[Dict]: diff --git a/shopdb/core/api/scripts.py b/shopdb/core/api/scripts.py index 8b606d41..2c7fc903 100644 --- a/shopdb/core/api/scripts.py +++ b/shopdb/core/api/scripts.py @@ -6,8 +6,23 @@ its body by digest on push (the same model as application installer delivery). The body never crosses the command wire and is never pre-staged on the endpoint; the agent downloads it with its collector credential and verifies sha256. -Gated on the applications.* permissions - scripts are software delivery like -apps, so they ride the same operator grant rather than a separate permission. +Gated on endpoints.runscript, the same standalone grant that authorizes pushing +a script. These are NOT applications: an application installer is a vendor +artefact, while a script here is arbitrary PowerShell this site wrote that the +agent runs as SYSTEM. Gating library management on applications.* meant anyone +with the software-delivery grant could rewrite the bytes of a script a trusted +operator later pushes, which is a privilege escalation wearing a content- +management hat. The write and the push now require the same trust. + +This TIGHTENS access, and there is deliberately NO backfill migration. The +operator scope grants applications.create/edit (they are edit verbs), so +operator-scope holders could reach this library before and cannot now. Do not +"fix" that by granting endpoints.runscript to applications.edit holders on +upgrade: it is in SCOPE_NEVER_GRANT precisely because it is a per-user checkbox +decision, and the same grant also authorizes the remote PUSH, so a backfill +would hand every operator SYSTEM code execution on the fleet. Admins are +unaffected via the role bypass; anyone else who genuinely needs the library +gets the checkbox in the user editor. """ import os @@ -32,6 +47,11 @@ SCRIPT_EXTENSIONS = {'.ps1'} SCRIPT_URL_PREFIX = '/api/scripts/body/' # A delivery script is small; cap well under the installer limit. MAX_SCRIPT_BYTES = 1 * 1024 * 1024 +# Owned by the endpoints plugin, which is where the risk is described in full +# ("ARBITRARY CODE, executes as SYSTEM"). Core names it as a string rather than +# importing it so this module stays importable with the plugin absent - the +# grant simply cannot be held by anyone if nothing seeds it. +SCRIPT_PERMISSION = 'endpoints.runscript' def _script_dir(): @@ -42,7 +62,7 @@ scripts_bp = Blueprint('scripts', __name__) @scripts_bp.route('', methods=['GET']) -@require_permission('applications.view') +@require_permission(SCRIPT_PERMISSION) def list_scripts(): """List scripts (active by default).""" query = Script.query @@ -55,7 +75,7 @@ def list_scripts(): @scripts_bp.route('/', methods=['GET']) -@require_permission('applications.view') +@require_permission(SCRIPT_PERMISSION) def get_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -74,7 +94,7 @@ def _validate_runcontext(value): @scripts_bp.route('', methods=['POST']) -@require_permission('applications.create') +@require_permission(SCRIPT_PERMISSION) def create_script(): """Create a script catalog entry (body uploaded separately).""" data = request.get_json(silent=True) or {} @@ -103,7 +123,7 @@ def create_script(): @scripts_bp.route('/', methods=['PUT']) -@require_permission('applications.edit') +@require_permission(SCRIPT_PERMISSION) def update_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -135,7 +155,7 @@ def update_script(script_id: int): @scripts_bp.route('/', methods=['DELETE']) -@require_permission('applications.delete') +@require_permission(SCRIPT_PERMISSION) def delete_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -149,7 +169,7 @@ def delete_script(script_id: int): @scripts_bp.route('//body', methods=['POST']) -@require_permission('applications.edit') +@require_permission(SCRIPT_PERMISSION) def upload_script_body(script_id: int): """Upload (or replace) the .ps1 body for a script. @@ -196,15 +216,15 @@ def upload_script_body(script_id: int): @scripts_bp.route('/body/', methods=['GET']) -@require_permission('applications.download') +@require_permission(SCRIPT_PERMISSION) def download_script_body(filename: str): """Operator-facing script body download (JWT/RBAC). The endpoint agent uses the collector-authenticated plugin route instead; this one is for humans reviewing what a script does. - Gated on applications.download, not applications.view: this serves the - SYSTEM-context PowerShell a site pushes to its bays, and a guest may be - granted the metadata permission. + Same grant as the rest of the library: this serves the SYSTEM-context + PowerShell a site pushes to its bays, so reading it is not a lesser act + than listing it. """ return send_upload(_script_dir(), filename, as_attachment=True) diff --git a/tests/test_core/test_script_library_gate.py b/tests/test_core/test_script_library_gate.py new file mode 100644 index 00000000..e65e4e3e --- /dev/null +++ b/tests/test_core/test_script_library_gate.py @@ -0,0 +1,160 @@ +"""The script library is gated on its own grant, not on applications.*. + +A script here is not an application. An installer is a vendor artefact; a script +is arbitrary PowerShell this site wrote that the agent runs as SYSTEM. While the +library was gated on applications.*, anyone holding the software-delivery grant +could rewrite the bytes of a script a trusted operator later pushed - the push +was gated on endpoints.runscript, but the CONTENT of what got pushed was not. +That is privilege escalation wearing a content-management hat. + +These tests pin the gate itself. The library had NO coverage before this. +""" +import io + +import pytest +from werkzeug.security import generate_password_hash + +from shopdb.core.models import Permission + + +APPLICATIONS_GRANTS = [ + ('applications.view', 'View applications', 'applications'), + ('applications.create', 'Create applications', 'applications'), + ('applications.edit', 'Edit applications', 'applications'), + ('applications.delete', 'Delete applications', 'applications'), + ('applications.download', 'Download installers', 'applications'), +] +SCRIPT_GRANT = ('endpoints.runscript', 'Run staged scripts', 'endpoints') + + +@pytest.fixture +def seeded(db): + Permission.seed_entries(APPLICATIONS_GRANTS + [SCRIPT_GRANT]) + db.session.commit() + + +def _user_with(client, db, username, grants): + """Create a user holding exactly the named permissions, return its headers. + + Direct permissions, no role: a role named 'admin' would short-circuit the + gate and prove nothing about the permission under test. + """ + from shopdb.core.models import Permission, User + + user = User( + username=username, + email=f'{username}@test.local', + passwordhash=generate_password_hash('testpass'), + ) + for name in grants: + permission = Permission.query.filter_by(name=name).one() + user.directpermissions.append(permission) + db.session.add(user) + db.session.commit() + + login = client.post('/api/auth/login', + json={'username': username, 'password': 'testpass'}) + assert login.status_code == 200, login.get_json() + token = login.get_json()['data']['access_token'] + return {'Authorization': f'Bearer {token}'} + + +def test_the_applications_grant_does_not_open_the_script_library( + client, db, seeded): + """The whole point: every applications.* grant, and still no access.""" + headers = _user_with(client, db, 'appsonly', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + + assert client.get('/api/scripts', headers=headers).status_code == 403 + assert client.post('/api/scripts', headers=headers, + json={'scriptname': 'x'}).status_code == 403 + + +def test_the_runscript_grant_opens_the_script_library(client, db, seeded): + headers = _user_with(client, db, 'pusher', ['endpoints.runscript']) + + assert client.get('/api/scripts', headers=headers).status_code == 200 + created = client.post('/api/scripts', headers=headers, + json={'scriptname': 'map-drives'}) + assert created.status_code == 201, created.get_json() + + +def test_writing_a_script_body_needs_the_same_grant(client, db, seeded): + """Uploading the bytes is the dangerous half; it must not be cheaper to + reach than the push that runs them.""" + author = _user_with(client, db, 'author', ['endpoints.runscript']) + created = client.post('/api/scripts', headers=author, + json={'scriptname': 'payload'}) + scriptid = created.get_json()['data']['scriptid'] + + appsuser = _user_with(client, db, 'appsuser2', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + body = (io.BytesIO(b"Write-Output 'hi'\r\n"), 'payload.ps1') + denied = client.post(f'/api/scripts/{scriptid}/body', headers=appsuser, + data={'file': body}, + content_type='multipart/form-data') + assert denied.status_code == 403 + + +def test_reading_a_script_body_needs_the_same_grant(client, db, seeded): + """Reading is not a lesser act: the body is the SYSTEM-context PowerShell + itself, so it must not be readable by a weaker grant than writing it.""" + appsuser = _user_with(client, db, 'appsuser3', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + response = client.get('/api/scripts/body/script-1.ps1', headers=appsuser) + assert response.status_code == 403 + + +def test_upload_pins_the_digest_of_the_stored_bytes(client, db, seeded): + """The agent refuses a body whose sha256 does not match, so a wrong digest + here is a script that can never run. Untested until now.""" + import hashlib + + headers = _user_with(client, db, 'digest', ['endpoints.runscript']) + created = client.post('/api/scripts', headers=headers, + json={'scriptname': 'digest-check'}) + scriptid = created.get_json()['data']['scriptid'] + + payload = b"Write-Output 'pinned'\r\n" + uploaded = client.post( + f'/api/scripts/{scriptid}/body', headers=headers, + data={'file': (io.BytesIO(payload), 'digest-check.ps1')}, + content_type='multipart/form-data') + assert uploaded.status_code == 200, uploaded.get_json() + + data = uploaded.get_json()['data'] + assert data['sha256'] == hashlib.sha256(payload).hexdigest() + assert data['hasbody'] is True + + +def test_a_duplicate_script_name_is_refused(client, db, seeded): + """The accidental-overwrite guard the library was specified to have.""" + headers = _user_with(client, db, 'dupes', ['endpoints.runscript']) + first = client.post('/api/scripts', headers=headers, + json={'scriptname': 'same-name'}) + assert first.status_code == 201 + second = client.post('/api/scripts', headers=headers, + json={'scriptname': 'same-name'}) + assert second.status_code == 400 + assert 'already exists' in second.get_json()['data']['error']['message'] + + +def test_no_prebuilt_scope_confers_the_script_permission(): + """Pins the decision NOT to backfill the tightened gate. + + Moving the library onto endpoints.runscript takes it away from operator + scope holders, who reach applications.create/edit. The tempting upgrade fix + is to grant endpoints.runscript to whoever holds applications.edit - that + would undo the whole change AND hand every operator remote SYSTEM code + execution, because the same grant gates the push. If this test fails, + someone has widened a scope; do not relax it. + """ + from shopdb.core.models.user import ( + SCOPE_NEVER_GRANT, + scope_permission_names, + ) + from shopdb.core.api.scripts import SCRIPT_PERMISSION + + assert SCRIPT_PERMISSION in SCOPE_NEVER_GRANT + for scope in ('reporter', 'technician', 'operator'): + assert SCRIPT_PERMISSION not in scope_permission_names(scope) From f02e34410115d432edda01263dde8023761b9f3e Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Wed, 9 Sep 2026 06:37:26 -0400 Subject: [PATCH 5/7] Correct two gate commands that pass without running npx vitest run bypasses the pretest hook that stages plugin frontends, so plugin specs were never collected - it reported 28 spec files where npm test reports 30. A plugin frontend spec could pass CI unread. Bare bash resolves to WSL on Windows dev boxes and fails to find /bin/bash, which redirected to a log reads as a clean style run with no findings. Both are the same failure shape: a command that reports success without having done the work. Also records that a multi-line here-string passed to git commit -m silently no-ops, reporting success without creating the commit. --- .github/copilot-instructions.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index c3f98f13..a738ef29 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,4 +1,4 @@ -# Copilot instructions for shopdb-flask +, # Copilot instructions for shopdb-flask Follow these when suggesting code. They are enforced by a naming/style hook and by CI (`.github/workflows/ci.yml`) - suggestions that break them fail the build. @@ -7,11 +7,12 @@ by CI (`.github/workflows/ci.yml`) - suggestions that break them fail the build. ## General Guidelines - Finish the in-flight task before starting new workstreams. - When asked to "take notes" on a future idea, capture it in the notes file only - do not implement it, and do not expand on it at length in replies. -- Use a temp file for multi-line git commit messages instead of heredoc syntax: `git commit -F `. When committing in PowerShell, never pass multi-line commit messages inline (no here-strings like @'...'@ or multiple -m flags). PowerShell mangles them and git errors with "did not match any file(s) known to git". Instead, write the commit message to a temp file in the WORKSPACE ROOT (e.g. `commitmsg.tmp`), run `git commit -F commitmsg.tmp`, then delete the file. Do NOT place the temp file inside .git/ - git prunes stray files there and the file disappears before commit runs. +- Use a temp file for multi-line git commit messages instead of heredoc syntax: `git commit -F `. When committing in PowerShell, always write the message to a temp file in the WORKSPACE ROOT (e.g. `commitmsg.tmp`), run `git commit -F commitmsg.tmp`, then delete the file. Passing a multi-line here-string to `git commit -m` silently no-ops: it reports success but does not create the commit. Always verify with `git log` after committing. Do NOT place the temp file inside .git/ - git prunes stray files there and the file disappears before commit runs. - Avoid using `powershell.exe -File` for launching scripts, as it fails if the script file is missing or quarantined. - Long-running commands (pytest suites, npm builds, container rolls) must be redirected to a log file under `devnotes/` (e.g. `devnotes/_pytest.log`) and launched non-blocking; never run as a blocking foreground command. Do not poll or tail the output; the user watches the file and will say when it is done. - Before taking any action that modifies code or files, warn the user first if a test run or build may be in progress, so discussion about the project can continue without altering files underneath a running test. - Always invoke shell scripts with the explicit Git Bash path: `& "C:\Program Files\Git\bin\bash.exe" scripts/