Auto-register staged agent builds as a pushable application

Gate collect_log on a minimum agent version and bridge the agent version store to the applications table so a staged build can actually be pushed. update_app resolves an install source from the applications table alone; it cannot see agentstore. A build could therefore be staged and current while no application row existed at all, which is what the fleet was in: the agent was unpushable. Staging is now the single source of truth - it packages the current build and upserts the row in the same request, so the row cannot describe a build that is no longer staged. PARTIAL BUILD ADVISORY This is steps 1-4 of a longer rollout and is deliberately incomplete: - There is no push trigger yet. The registered package is resolvable and verifiable, but nothing enqueues an upgrade; the "behind current build" indicator is still display-only. - The upgrade template script, its script row, and the enqueue route (endpoints.agentupdate) are not written. - Blocking constraint for the next slice: run_script executes powershell.exe -File <script> with NO argument channel, so a delivered upgrade script cannot be passed BaseUrl/CollectorKey and must either source them itself or run_script must be extended first. Nothing here touches a live endpoint. Staging only changes what a future push would contain. Details: - Package members carry a fixed timestamp. zipfile stamps the current mtime by default, so identical input produced a different archive digest on every call. update_app pins a download by SHA-256, so a drifting hash would rewrite installersha256 out from under an endpoint mid-download and fail its verification. - All three routes that can change 'current' re-register (upload, adopt-legacy, rollback). Covering only upload would leave the row pinned to the build an operator just rolled away from, and the push would ship it anyway - silent drift. - Registration cannot fail an upload. The binary is staged and audited by the time it runs, so a packaging problem is reported in the response payload rather than raised; returning an error would tell the operator a successful upload was rejected. - The package is written to a temp name and os.replace'd, so an interrupted write cannot leave a truncated archive at the path the row advertises. - The row is isinstallable but hidden: machine-managed, and it must not appear in the software catalogue beside real licensed applications. - Added application_package_location() to the shopdb.api contract surface (lazy, matching collector_authorized) returning the package directory and URL prefix together. agentregistry reached into shopdb.core internals, violating the plugin import contract; sourcing both from one place also stops the write location and installpath from drifting apart. collect_log is refused at enqueue below its minimum agent version, with numeric comparison - string ordering misplaces 1.9.0.0 against 1.16.0. The refusal is mirrored in the UI so a command slot is not spent on a call that is guaranteed to fail. Test isolation: create_app('testing') resolves instance_path to the real repo instance/ directory, so agent upload tests write into the developer's actual store. The new tests redirect both the store and package output into tmp_path and include a guard test that fails if that isolation regresses. NOTE: the pre-existing test_endpoints_agentupload.py still has this leak and deposits 1.5.0.0 and 1.6.3 into instance/siteagent/ on a full run - not fixed here. Suite: 2834 passed, 11 skipped. The two failures (test_docs_publishable, and a symlink scan needing Windows privileges) were confirmed pre-existing by stashing this work and reproducing them.
2026-09-09 21:40:51 -04:00
parent eee06743a6
commit d32293ccb2
11 changed files with 983 additions and 6 deletions
--- a/tests/test_plugins/test_endpoints_agent_registration.py
+++ b/tests/test_plugins/test_endpoints_agent_registration.py
@@ -0,0 +1,250 @@
+"""Auto-registration of staged agent builds as a pushable Application.
+
+The endpoint update_app push resolves an install source from the applications
+table alone; it cannot see the agent version store. Before this, a build could
+be staged and current while no application row existed at all, which is exactly
+what was found in the fleet - the agent was unpushable.
+
+These tests pin the properties that make the bridge trustworthy:
+  - staging creates the row, and re-staging updates it in place (never a row
+    per build, because appname is unique and drift is the failure mode);
+  - the packaged archive is byte-stable for unchanged input, because update_app
+    pins a download to a digest and a drifting hash breaks verification;
+  - every path that changes 'current' re-registers, including rollback;
+  - registration failure never fails an upload that already landed on disk.
+
+Uploads use PE-magic bytes with a monkeypatched version parser, matching the
+approach in test_endpoints_agentupload.py - building a real PE with an
+RT_VERSION resource inline is impractical.
+
+Every test runs against an ISOLATED store. create_app('testing') resolves
+instance_path to the real repo instance/ directory, so without redirection
+these tests stage fake 130-byte binaries into the developer's actual agent
+store and leave invented versions behind in its manifest. The isolate fixture
+below is autouse for that reason: it is a correctness requirement, not a
+convenience.
+"""
+
+import hashlib
+import io
+import os
+import zipfile
+
+import pytest
+
+from plugins.endpoints.api import agentstore, agentupload, agentregistry
+from shopdb.core.models.application import Application
+
+UPLOAD_URL = '/api/endpoints/siteagent/binaries/{}'
+CURRENT_URL = '/api/endpoints/siteagent/binaries/{}/current'
+
+_PE_BYTES = b'MZ' + b'\x00' * 128
+
+
+@pytest.fixture(autouse=True)
+def isolate_store(tmp_path, monkeypatch):
+    """Point the agent store and package output at a per-test directory.
+
+    Both must be redirected. Redirecting only the store would still write the
+    ~zip into the real applicationpackages tree under a live application id.
+    """
+    siteagent = tmp_path / 'siteagent'
+    packages = tmp_path / 'applicationpackages'
+    siteagent.mkdir()
+    packages.mkdir()
+    monkeypatch.setattr(agentstore, '_siteagent_dir', lambda: str(siteagent))
+    monkeypatch.setattr(agentregistry, 'application_package_location',
+                        lambda: (str(packages), '/api/applications/package/'))
+    return tmp_path
+
+
+def _upload(client, headers, monkeypatch, version, binkey='agent',
+            payload=None, makecurrent=None):
+    """Stage a build through the real HTTP route."""
+    monkeypatch.setattr(agentstore, 'read_pe_file_version', lambda data: version)
+    form = {'file': (io.BytesIO(payload or _PE_BYTES), 'shopdbagent.exe'),
+            'version': version}
+    if makecurrent is not None:
+        form['makecurrent'] = makecurrent
+    return client.post(UPLOAD_URL.format(binkey), data=form,
+                       content_type='multipart/form-data', headers=headers)
+
+
+def _agent_row():
+    return Application.query.filter_by(
+        appname=agentregistry.AGENT_APPNAME).first()
+
+
+def test_staging_a_build_creates_a_pushable_application_row(
+        client, db, app, auth_headers, monkeypatch):
+    """Without this row, update_app has nothing to resolve and cannot push."""
+    assert _agent_row() is None
+
+    response = _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+    assert response.status_code == 200
+
+    registration = response.get_json()['data']['registration']
+    assert registration['ok'] is True
+
+    row = _agent_row()
+    assert row is not None
+    # The row must be resolvable and pinned: a path update_app can fetch, and a
+    # digest it can verify the bytes against.
+    assert row.installpath == '/api/applications/package/{}/{}'.format(
+        row.appid, agentregistry.AGENT_PACKAGE_FILENAME)
+    assert row.installersha256 == registration['sha256']
+    assert row.isinstallable is True
+    # Hidden: this is machine-managed, and must not appear in the software
+    # catalogue beside real licensed applications.
+    assert row.ishidden is True
+
+
+def test_restaging_updates_the_same_row_rather_than_adding_one(
+        client, db, app, auth_headers, monkeypatch):
+    """appname is unique; a row per build would break the push target."""
+    _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+    first = _agent_row()
+    firstid = first.appid
+
+    response = _upload(client, auth_headers, monkeypatch, '1.17.0.0')
+    assert response.status_code == 200
+
+    assert Application.query.filter_by(
+        appname=agentregistry.AGENT_APPNAME).count() == 1
+    row = _agent_row()
+    assert row.appid == firstid
+    # The row must follow the newly current build, not keep pointing at 1.16.
+    assert row.installersha256 == \
+        response.get_json()['data']['registration']['sha256']
+
+
+def test_packaging_the_same_build_twice_yields_the_same_digest(
+        client, db, app, auth_headers, monkeypatch):
+    """update_app pins bytes by digest.
+
+    zipfile stamps members with the current mtime by default, which made the
+    archive hash change on every call for identical input. That would rewrite
+    installersha256 out from under an endpoint mid-download and fail its
+    SHA-256 verification.
+    """
+    _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+
+    firstpayload, firstmanifest = agentstore.build_current_package()
+    secondpayload, secondmanifest = agentstore.build_current_package()
+
+    assert firstmanifest['sha256'] == secondmanifest['sha256']
+    assert firstpayload == secondpayload
+
+
+def test_the_package_contains_the_staged_binary_under_a_fixed_name(
+        client, db, app, auth_headers, monkeypatch):
+    """The install script addresses members by name, not by discovery."""
+    _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+
+    payload, manifest = agentstore.build_current_package()
+    with zipfile.ZipFile(io.BytesIO(payload)) as archive:
+        assert archive.testzip() is None
+        names = archive.namelist()
+    assert agentstore.PACKAGE_MEMBER_NAMES['agent'] in names
+    assert manifest['agentversion'] == '1.16.0.0'
+    assert manifest['contents']['agent']['version'] == '1.16.0.0'
+
+
+def test_the_written_package_file_matches_the_recorded_digest(
+        client, db, app, auth_headers, monkeypatch):
+    """A row whose digest disagrees with the file on disk is unpushable."""
+    _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+    row = _agent_row()
+
+    path = agentregistry._package_path(row.appid)
+    assert os.path.isfile(path)
+    with open(path, 'rb') as handle:
+        ondisk = hashlib.sha256(handle.read()).hexdigest()
+    assert ondisk == row.installersha256
+    # The atomic-write temp file must not survive a successful write.
+    assert not os.path.isfile(path + '.tmp')
+
+
+def test_rolling_back_repoints_the_row_at_the_restored_build(
+        client, db, app, auth_headers, monkeypatch):
+    """A rollback changes the bytes the fleet should get.
+
+    If only upload re-registered, the row would still pin the build the
+    operator just rolled away from, and the push would ship it anyway.
+    """
+    _upload(client, auth_headers, monkeypatch, '1.16.0.0',
+            payload=b'MZ' + b'\x01' * 256)
+    oldsha = _agent_row().installersha256
+
+    _upload(client, auth_headers, monkeypatch, '1.17.0.0',
+            payload=b'MZ' + b'\x02' * 256)
+    newsha = _agent_row().installersha256
+    assert newsha != oldsha
+
+    response = client.put(CURRENT_URL.format('agent'),
+                          json={'version': '1.16.0.0'}, headers=auth_headers)
+    assert response.status_code == 200
+    assert response.get_json()['data']['registration']['ok'] is True
+
+    assert agentstore.current_version('agent') == '1.16.0.0'
+    # Back to the rolled-to build's package, not the newer one.
+    assert _agent_row().installersha256 == oldsha
+
+
+def test_a_registration_failure_does_not_fail_the_upload(
+        client, db, app, auth_headers, monkeypatch):
+    """The binary is already staged and audited by the time this runs.
+
+    Turning a packaging problem into an error response would tell the operator
+    the upload was rejected when it actually succeeded, inviting a re-upload of
+    a build that is already there.
+    """
+    def _explode():
+        raise OSError('simulated packaging failure')
+
+    monkeypatch.setattr(agentregistry.agentstore, 'build_current_package',
+                        _explode)
+
+    response = _upload(client, auth_headers, monkeypatch, '1.16.0.0')
+
+    assert response.status_code == 200
+    body = response.get_json()['data']
+    # The staging itself still stands...
+    assert body['current'] == '1.16.0.0'
+    # ...and the failure is surfaced rather than swallowed.
+    assert body['registration']['ok'] is False
+    assert 'simulated packaging failure' in body['registration']['error']
+
+
+def test_staging_only_a_tray_reports_that_there_is_nothing_to_package(
+        client, db, app, auth_headers, monkeypatch):
+    """The bundle is defined by the agent; a tray alone is not pushable."""
+    response = _upload(client, auth_headers, monkeypatch, '1.6.5.0',
+                       binkey='tray')
+
+    assert response.status_code == 200
+    registration = response.get_json()['data']['registration']
+    assert registration['ok'] is False
+    assert 'no current agent build' in registration['error']
+    assert _agent_row() is None
+
+
+def test_these_tests_do_not_touch_the_real_agent_store(
+        client, db, app, auth_headers, monkeypatch, isolate_store):
+    """Guard the isolation itself.
+
+    An earlier draft of this file wrote fake 130-byte binaries into the repo's
+    real instance/siteagent/ tree and invented a version in its manifest,
+    because create_app('testing') shares the developer's instance_path. If that
+    redirection ever regresses, fail here rather than silently corrupting a
+    staged build again.
+    """
+    _upload(client, auth_headers, monkeypatch, '9.9.9.9')
+
+    # The staged build landed inside the per-test directory...
+    staged = isolate_store / 'siteagent' / 'agent' / '9.9.9.9'
+    assert staged.is_dir()
+
+    # ...and the real store never saw it.
+    realstore = os.path.join(app.instance_path, 'siteagent', 'agent', '9.9.9.9')
+    assert not os.path.exists(realstore)
--- a/tests/test_plugins/test_endpoints_agent_version_guard.py
+++ b/tests/test_plugins/test_endpoints_agent_version_guard.py
@@ -0,0 +1,158 @@
+"""The agent version guard on command enqueue.
+
+An agent older than an action has no case for it and falls through to its
+unknown-action default, answering "Unknown or disallowed action". That reads as
+an authorization refusal, so the operator goes looking at permissions they
+already hold - which is exactly what happened on a live endpoint running 1.15.0
+after collect_log shipped in 1.16.0.
+
+These tests pin the refusal at the SERVER, before the command is ever queued,
+and pin that the message names both versions so the answer is actionable.
+"""
+
+from werkzeug.security import generate_password_hash
+
+
+def _endpoint(db, hostname, agentversion):
+    """An Asset + Computer pair whose reported state carries an agent version."""
+    from shopdb.api import Asset, AssetType
+    from plugins.computers.models import Computer
+    from plugins.endpoints.models import EndpointState
+
+    assettype = AssetType.query.filter_by(assettype='computer').first()
+    if assettype is None:
+        assettype = AssetType(assettype='computer', pluginname='computers',
+                              tablename='computers')
+        db.session.add(assettype)
+        db.session.flush()
+
+    asset = Asset(assetnumber=hostname, name=hostname,
+                  assettypeid=assettype.assettypeid, isactive=True)
+    db.session.add(asset)
+    db.session.flush()
+    db.session.add(Computer(assetid=asset.assetid, hostname=hostname))
+    db.session.add(EndpointState(assetid=asset.assetid, hostname=hostname,
+                                 agentversion=agentversion))
+    db.session.commit()
+    return asset.assetid
+
+
+def _admin(db, client, username='guardadmin'):
+    """An admin, so permission and policy gates cannot be what refuses."""
+    from shopdb.core.models import User, Role, Permission
+
+    Permission.seed()
+    db.session.flush()
+    role = Role.query.filter_by(rolename='admin').first()
+    if role is None:
+        role = Role(rolename='admin')
+        db.session.add(role)
+        db.session.commit()
+
+    user = User(username=username, email=username + '@test.local',
+                passwordhash=generate_password_hash('testpass'))
+    user.roles.append(role)
+    db.session.add(user)
+    db.session.commit()
+
+    response = client.post('/api/auth/login',
+                           json={'username': username, 'password': 'testpass'})
+    token = response.get_json()['data']['access_token']
+    return {'Authorization': f'Bearer {token}'}
+
+
+def _collect(client, headers, assetid):
+    return client.post(f'/api/endpoints/{assetid}/commands',
+                       json={'action': 'collect_log',
+                             'target': r'C:\logs\app.log'},
+                       headers=headers)
+
+
+def test_an_agent_below_the_minimum_is_refused_at_enqueue(app, client, db):
+    """The command must not be queued at all.
+
+    Queueing it would spend the endpoint's one in-flight command slot and leave
+    a failed row that reads as endpoint misbehaviour, when the truth is the
+    agent simply predates the feature.
+    """
+    from plugins.endpoints.models import EndpointCommand
+
+    assetid = _endpoint(db, 'oldagentbox', '1.15.0.0')
+    headers = _admin(db, client)
+
+    response = _collect(client, headers, assetid)
+
+    assert response.status_code == 400
+    message = response.get_json()['data']['error']['message']
+    # Both versions, or the operator cannot tell what to do about it.
+    assert '1.16.0' in message
+    assert '1.15.0.0' in message
+    assert EndpointCommand.query.filter_by(assetid=assetid).count() == 0
+
+
+def test_an_agent_at_the_minimum_is_allowed(app, client, db):
+    """The guard must not refuse a capable endpoint.
+
+    1.16.0.0 against a 1.16.0 floor is the four-vs-three-part spelling of the
+    same build; a lexical compare would get this wrong.
+    """
+    from plugins.endpoints.models import EndpointCommand
+
+    assetid = _endpoint(db, 'newagentbox', '1.16.0.0')
+    headers = _admin(db, client)
+
+    response = _collect(client, headers, assetid)
+
+    assert response.status_code in (200, 201)
+    assert EndpointCommand.query.filter_by(assetid=assetid,
+                                           action='collect_log').count() == 1
+
+
+def test_a_lexically_larger_but_older_version_is_still_refused(app, client, db):
+    """'1.9.0.0' must not pass a '1.16.0' floor.
+
+    String comparison says '9' > '1' and lets it through. This is the specific
+    bug the numeric parse exists to prevent, so it gets its own test rather
+    than living only in the helper's unit coverage.
+    """
+    assetid = _endpoint(db, 'lexicaltrapbox', '1.9.0.0')
+    headers = _admin(db, client)
+
+    response = _collect(client, headers, assetid)
+
+    assert response.status_code == 400
+    assert '1.16.0' in response.get_json()['data']['error']['message']
+
+
+def test_an_unknown_agent_version_is_refused_rather_than_guessed(app, client, db):
+    """A placeholder version fails closed.
+
+    Seeded rows carry '0.0.0-sample'. There is no honest ordering between a
+    placeholder and a real build, and letting it through only moves the failure
+    to the agent where the error message is the misleading one.
+    """
+    assetid = _endpoint(db, 'samplebox', '0.0.0-sample')
+    headers = _admin(db, client)
+
+    response = _collect(client, headers, assetid)
+
+    assert response.status_code == 400
+
+
+def test_an_action_without_a_floor_is_unaffected(app, client, db):
+    """The guard is per-action, not global.
+
+    collect_now declares no minimum, so an old agent must still be able to run
+    it - otherwise adding a floor to one action quietly narrows the others.
+    """
+    from plugins.endpoints.models import EndpointCommand
+
+    assetid = _endpoint(db, 'refreshbox', '1.11.0.0')
+    headers = _admin(db, client)
+
+    response = client.post(f'/api/endpoints/{assetid}/commands',
+                           json={'action': 'collect_now'}, headers=headers)
+
+    assert response.status_code in (200, 201)
+    assert EndpointCommand.query.filter_by(assetid=assetid,
+                                           action='collect_now').count() == 1
--- a/tests/test_plugins/test_endpoints_plugin.py
+++ b/tests/test_plugins/test_endpoints_plugin.py
@@ -11,7 +11,8 @@ from plugins.endpoints.plugin import EndpointsPlugin, CURATED_EVENT_IDS
 from plugins.endpoints.eventcatalog import EVENT_CATALOG, CRITICAL_EVENT_IDS
 from plugins.endpoints.commands import (
    COMMAND_ALLOWLIST, is_allowed_action, elevated_permission_for,
-    requires_target,
+    requires_target, minimum_agent_version_for, agent_meets_minimum,
+    parse_agent_version,
 )


@@ -216,6 +217,61 @@ def test_collect_log_survives_a_policy_that_never_mentions_it():
    assert elevated_permission_for('collect_log') == 'endpoints.updateapp'


+def test_collect_log_declares_the_agent_version_that_implements_it():
+    """collect_log names its minimum agent version.
+
+    An agent older than the action has no case for it and falls through to its
+    unknown-action default, answering "Unknown or disallowed action" - which
+    reads as an authorization refusal and sends the operator to check a grant
+    they already hold. The floor is what lets the server refuse first with the
+    actionable answer instead.
+    """
+    assert minimum_agent_version_for('collect_log') == '1.16.0'
+    # Actions that every shipped agent understands must NOT declare a floor,
+    # or the guard starts refusing endpoints that are perfectly capable.
+    assert minimum_agent_version_for('collect_now') is None
+    assert minimum_agent_version_for('system_restart') is None
+
+
+def test_agent_version_comparison_survives_real_fleet_version_strings():
+    """Version comparison is numeric per part, not lexical.
+
+    Two traps, both real in this fleet. Agents report four parts ('1.15.0.0')
+    while the csproj and binary store carry three ('1.16.0'), so the same build
+    must compare equal across both spellings. And a lexical compare ranks
+    '1.9.0.0' ABOVE '1.16.0' because '9' > '1' - which would silently let an
+    old agent past the guard, the exact failure the guard exists to prevent.
+    """
+    assert parse_agent_version('1.16.0') == parse_agent_version('1.16.0.0')
+    assert agent_meets_minimum('1.16.0', '1.16.0')
+    assert agent_meets_minimum('1.16.0.0', '1.16.0')
+    assert agent_meets_minimum('1.17.0.0', '1.16.0')
+    assert agent_meets_minimum('2.0.0.0', '1.16.0')
+    assert not agent_meets_minimum('1.15.0.0', '1.16.0')
+    assert not agent_meets_minimum('1.11.0.0', '1.16.0')
+    # The lexical trap.
+    assert not agent_meets_minimum('1.9.0.0', '1.16.0')
+
+
+def test_an_unreadable_agent_version_fails_the_guard_closed():
+    """An unparseable or absent version does not satisfy a minimum.
+
+    Eight endpoints in this fleet report '0.0.0-sample', and a never-checked-in
+    endpoint reports nothing at all. There is no honest ordering between a
+    placeholder and a real build, so the guard refuses rather than guessing.
+    Letting it through would only move the failure to the agent, where the
+    error message is the misleading one.
+    """
+    assert parse_agent_version('0.0.0-sample') is None
+    assert not agent_meets_minimum('0.0.0-sample', '1.16.0')
+    assert not agent_meets_minimum(None, '1.16.0')
+    assert not agent_meets_minimum('', '1.16.0')
+    # No declared floor means no opinion: an action every agent implements must
+    # stay available even to an endpoint whose version we cannot read.
+    assert agent_meets_minimum('0.0.0-sample', None)
+    assert agent_meets_minimum(None, None)
+
+
 def test_run_script_and_update_app_are_allowlisted_delivery_actions():
    """The two software-delivery actions are allowlisted with a targetkind so
    the enqueue route accepts them and carries their subject on the wire: