Record how a PC is reached, and stop pages implying they can do things

Three findings, all about a screen or a log saying something that is not so.

Editing a PC wrote an audit entry for its named fields and nothing for the two
changes that matter most when somebody investigates later: the primary IP
address, which is how the machine is found, and the remote-access protocols and
ports, which is how somebody connects to it. Both are on the same form, in the
same request, and both were invisible afterwards - a PC that moved address had
no history of moving, and a PC that gained a VNC listener had no record of
gaining one. Both are recorded now, and re-sending the same value still records
nothing, because an audit log full of non-events is one nobody reads.

While proving that, a silent discard turned up in the same lines: both the
create and the update path did `if ip_comtype:` around the IP write and did
nothing at all when the 'IP' communication type row was missing. Somebody typed
an address, saved, got a 200, and the address was gone. A missing reference row
is a seeding gap, not a reason to drop what was typed, so it is created on
demand.

The PCs list rendered a 403 as "No computers found", which tells a person
without the permission that the fleet is empty and sends them looking for the
data rather than for the permission. It now says which permission is missing.

The Site Policies and Configuration Policies pages rendered their empty table
and an Add button while the policy feature flag was off - every route answers
404 "Policy feature is not enabled", so the button could never work. Both pages
now say the feature is off and that the empty list is a consequence of that,
not of nothing being configured.

Nine tests. The audit entries verified by planting: remove either and two fail.

Scope: product

plugins/computers/api/routes.py

@@ -258,13 +258,42 @@ def _computer_access_links(comp, domain=None):
     return links
 
 
+def _ip_comtype():
+    """The 'IP' communication type, created if this site has none.
+
+    Both the create and the update path did `if ip_comtype:` and silently did
+    nothing when the row was missing - so a person typed an address, saved, got
+    a 200, and the address was gone. A missing reference row is a seeding gap,
+    not a reason to discard what somebody typed.
+    """
+    comtype = CommunicationType.query.filter_by(comtype='IP').first()
+    if comtype is None:
+        comtype = CommunicationType(comtype='IP', description='IP address')
+        db.session.add(comtype)
+        db.session.flush()
+    return comtype
+
+
 def _sync_access_methods(comp, data):
     """Replace a computer's enabled protocols from data['accessmethods'] (a list
     of {protocolid, portoverride?}). No-op if the key is absent, so callers that
-    don't touch access aren't affected."""
+    don't touch access aren't affected.
+
+    Returns a {'old': [...], 'new': [...]} change entry when the set actually
+    moved, or None. The caller records it: which remote-access protocols reach
+    a shop-floor PC, and on which ports, is exactly the kind of change an audit
+    log exists to remember, and this wrote nothing at all.
+    """
     if 'accessmethods' not in data:
-        return
+        return None
     desired = data.get('accessmethods') or []
+
+    def describe(rows):
+        return sorted('%s:%s' % (row.protocolid, row.portoverride or 'default')
+                      for row in rows)
+
+    before = describe(ComputerAccess.query.filter_by(
+        computerid=comp.computerid).all())
     ComputerAccess.query.filter_by(computerid=comp.computerid).delete()
     seen = set()
     for m in desired:
@@ -287,6 +316,13 @@ def _sync_access_methods(comp, data):
             isactive=True,
         ))
 
+    db.session.flush()
+    after = describe(ComputerAccess.query.filter_by(
+        computerid=comp.computerid).all())
+    if before != after:
+        return {'old': before, 'new': after}
+    return None
+
 
 # =============================================================================
 # Computers CRUD
@@ -587,7 +623,7 @@ def create_computer():
 
     # Optional primary IP communication
     if data.get('ipaddress'):
-        ip_comtype = CommunicationType.query.filter_by(comtype='IP').first()
+        ip_comtype = _ip_comtype()
         if ip_comtype:
             db.session.add(Communication(
                 assetid=asset.assetid,
@@ -596,7 +632,9 @@ def create_computer():
                 isprimary=True,
             ))
 
-    # Remote-access protocols
+    # Remote-access protocols. The return value is not recorded here: a create
+    # is already described in full by its 'created' entry, and there is no
+    # previous state to compare against.
     _sync_access_methods(comp, data)
 
     # Preserve legacy timestamps in import mode (no-op otherwise)
@@ -697,20 +735,27 @@ def update_computer(computer_id: int):
         ip = (data.get('ipaddress') or '').strip()
         primary = Communication.query.filter_by(
             assetid=asset.assetid, isprimary=True).first()
+        # Recorded, like every other field on this form. The address a PC
+        # answers on is how somebody finds it later; changing it wrote no audit
+        # entry at all, so a machine that moved had no history of moving.
+        oldip = primary.ipaddress if primary else None
+        if (oldip or None) != (ip or None):
+            changes['ipaddress'] = {'old': oldip, 'new': ip or None}
         if ip:
             if primary:
                 primary.ipaddress = ip
             else:
-                ip_comtype = CommunicationType.query.filter_by(comtype='IP').first()
-                if ip_comtype:
                 db.session.add(Communication(
-                        assetid=asset.assetid, comtypeid=ip_comtype.comtypeid,
+                    assetid=asset.assetid,
+                    comtypeid=_ip_comtype().comtypeid,
                     ipaddress=ip, isprimary=True))
         elif primary:
             primary.ipaddress = None
 
     # Remote-access protocols
-    _sync_access_methods(comp, data)
+    accesschange = _sync_access_methods(comp, data)
+    if accesschange:
+        changes['accessmethods'] = accesschange
 
     # Audit log if there were changes
     if changes:

plugins/computers/frontend/views/BusinessPoliciesList.vue

@@ -2,7 +2,14 @@
   <div>
     <div class="page-header">
       <h2>Site Policies</h2>
-      <button class="btn btn-primary" @click="openModal()">+ Add Site Policy</button>
+      <button v-if="!featureOff" class="btn btn-primary" @click="openModal()">+ Add Site Policy</button>
+    </div>
+
+    <div v-if="featureOff" class="card feature-off-notice">
+      <strong>Site policies are not enabled on this site.</strong>
+      <p>Nothing can be created here until the feature is turned on.
+        The list below is empty because the feature is off, not
+        because nothing has been configured.</p>
     </div>
 
     <div class="card">
@@ -110,6 +117,7 @@ const toast = useToast()
 const policies = ref([])
 const configPolicies = ref([])
 const loading = ref(true)
+const featureOff = ref(false)
 
 const showModal = ref(false)
 const editing = ref(null)
@@ -135,6 +143,10 @@ async function loadData() {
     configPolicies.value = config.data.data || []
   } catch (err) {
     console.error('Error loading business policies:', err)
+    // The API answers 404 'Policy feature is not enabled' when the
+    // flag is off. Without this the page rendered its empty table
+    // and an Add button for a feature that cannot accept one.
+    featureOff.value = err?.response?.status === 404
   } finally {
     loading.value = false
   }
@@ -214,6 +226,13 @@ async function remove(p) {
 </script>
 
 <style scoped>
+.feature-off-notice {
+  border-left: 3px solid var(--warning);
+  margin-bottom: 1rem;
+}
+.feature-off-notice p {
+  margin: 0.4rem 0 0 0;
+}
 .item-row { display: flex; gap: 8px; align-items: center; margin-bottom: 6px; }
 .order-badge {
   display: inline-flex; align-items: center; justify-content: center;

plugins/computers/frontend/views/ConfigurationPoliciesList.vue

@@ -2,7 +2,14 @@
   <div>
     <div class="page-header">
       <h2>Configuration Policies</h2>
-      <button class="btn btn-primary" @click="openModal()">+ Add Policy</button>
+      <button v-if="!featureOff" class="btn btn-primary" @click="openModal()">+ Add Policy</button>
+    </div>
+
+    <div v-if="featureOff" class="card feature-off-notice">
+      <strong>Configuration policies are not enabled on this site.</strong>
+      <p>Nothing can be created here until the feature is turned on. The
+        list is empty because the feature is off, not because nothing
+        has been configured.</p>
     </div>
 
     <div class="card">
@@ -123,6 +130,7 @@ const apps = ref([])
 const actionCatalog = ref([])
 const monitors = ref([])
 const loading = ref(true)
+const featureOff = ref(false)
 
 const showModal = ref(false)
 const editing = ref(null)
@@ -155,8 +163,13 @@ async function loadData() {
 
     if (pol.status === 'fulfilled') {
       policies.value = pol.value.data.data || []
+      featureOff.value = false
     } else {
-      failures.push('policies')
+      // The API answers 404 'Policy feature is not enabled' when the flag is
+      // off. Without telling them apart the page rendered an empty table and
+      // an Add button for a feature that cannot accept one.
+      featureOff.value = pol.reason?.response?.status === 404
+      if (!featureOff.value) failures.push('policies')
       console.error('Error loading policies:', pol.reason)
     }
 
@@ -284,6 +297,13 @@ async function remove(p) {
 </script>
 
 <style scoped>
+.feature-off-notice {
+  border-left: 3px solid var(--warning);
+  margin-bottom: 1rem;
+}
+.feature-off-notice p {
+  margin: 0.4rem 0 0 0;
+}
 .modal-lg {
   max-width: 760px;
 }

plugins/computers/frontend/views/PCsList.vue

@@ -72,7 +72,15 @@
               </tr>
               <tr v-if="computers.length === 0">
                 <td colspan="8" style="text-align: center; color: var(--text-light);">
-                  No computers found
+                  <!-- A 403 is not an empty result. Rendering "No computers
+                       found" to somebody who simply may not see them tells
+                       them the fleet is empty, and they go looking for the
+                       data rather than for the permission. -->
+                  <template v-if="denied">
+                    You do not have permission to view computers. Ask an
+                    administrator for the computers.view permission.
+                  </template>
+                  <template v-else>No computers found</template>
                   <SearchSuggestion :suggestion="suggestion" @pick="applySuggestion" />
                 </td>
               </tr>
@@ -114,6 +122,8 @@ function applySuggestion(word) {
   loadComputers()
 }
 const showRetired = ref(false)
+// Told apart from an empty list: see the table's empty row.
+const denied = ref(false)
 const totalPages = ref(1)
 const perPage = ref(20)
 
@@ -140,6 +150,7 @@ async function loadComputers() {
     totalPages.value = response.data.meta?.pagination?.totalpages || response.data.meta?.pagination?.total_pages || 1
   } catch (error) {
     console.error('Error loading computers:', error)
+    denied.value = error?.response?.status === 403
   } finally {
     loading.value = false
   }

tests/test_plugins/test_computers_audit.py

@@ -0,0 +1,104 @@
+"""Changing how a PC is reached has to leave a record.
+
+Editing a computer wrote an audit entry for its named fields and nothing at all
+for two changes that matter most to anybody investigating later:
+
+  * the primary IP address - how the PC is found on the network
+  * the remote-access protocols and their ports - how somebody connects to it
+
+Both are set on the same form, in the same request, and both were invisible
+afterwards. A PC that moved address had no history of moving, and a PC that
+gained a VNC listener had no record of gaining one.
+"""
+import pytest
+
+from shopdb.api import db
+
+
+@pytest.fixture
+def pc(client, db, auth_headers):
+    from shopdb.core.models import AssetType, AssetStatus
+
+    if not AssetType.query.filter_by(assettype='computer').first():
+        db.session.add(AssetType(assettype='computer'))
+    if not AssetStatus.query.first():
+        db.session.add(AssetStatus(status='In Use'))
+    db.session.commit()
+
+    created = client.post('/api/computers', headers=auth_headers, json={
+        'assetnumber': 'AUDIT-PC-1',
+        'hostname': 'auditpc1',
+        'ipaddress': '10.20.30.40',
+    })
+    assert created.status_code == 201, created.get_json()
+    data = created.get_json()['data']
+    return data.get('computerid') or data['computer']['computerid']
+
+
+def _entries(entityid):
+    from shopdb.core.models import AuditLog
+
+    return (AuditLog.query
+            .filter_by(entitytype='Computer', entityid=entityid)
+            .order_by(AuditLog.auditlogid.desc()).all())
+
+
+def test_moving_a_pc_to_another_address_is_recorded(client, auth_headers, pc):
+    response = client.put('/api/computers/%d' % pc, headers=auth_headers,
+                          json={'ipaddress': '10.20.30.99'})
+    assert response.status_code == 200, response.get_json()
+
+    latest = _entries(pc)[0]
+    assert 'ipaddress' in (latest.changes or {}), latest.changes
+    assert latest.changes['ipaddress']['old'] == '10.20.30.40'
+    assert latest.changes['ipaddress']['new'] == '10.20.30.99'
+
+
+def test_an_unchanged_address_records_nothing(client, auth_headers, pc):
+    """A PUT that re-sends the same value is not a change, and an audit log
+    full of non-events is one nobody reads."""
+    before = len(_entries(pc))
+    response = client.put('/api/computers/%d' % pc, headers=auth_headers,
+                          json={'ipaddress': '10.20.30.40'})
+    assert response.status_code == 200
+    assert len(_entries(pc)) == before
+
+
+def test_changing_the_remote_access_protocols_is_recorded(client, db,
+                                                          auth_headers, pc):
+    from plugins.computers.models import AccessProtocol
+
+    rdp = AccessProtocol(name='RDP', scheme='rdp', linktemplate='rdp://{host}')
+    vnc = AccessProtocol(name='VNC', scheme='vnc', linktemplate='vnc://{host}')
+    db.session.add_all([rdp, vnc])
+    db.session.commit()
+
+    client.put('/api/computers/%d' % pc, headers=auth_headers,
+               json={'accessmethods': [{'protocolid': rdp.protocolid}]})
+    response = client.put('/api/computers/%d' % pc, headers=auth_headers,
+                          json={'accessmethods': [
+                              {'protocolid': vnc.protocolid,
+                               'portoverride': 5999}]})
+    assert response.status_code == 200, response.get_json()
+
+    latest = _entries(pc)[0]
+    assert 'accessmethods' in (latest.changes or {}), latest.changes
+    change = latest.changes['accessmethods']
+    assert change['old'] == ['%d:default' % rdp.protocolid]
+    assert change['new'] == ['%d:5999' % vnc.protocolid]
+
+
+def test_resending_the_same_protocols_records_nothing(client, db,
+                                                      auth_headers, pc):
+    from plugins.computers.models import AccessProtocol
+
+    ssh = AccessProtocol(name='SSH', scheme='ssh', linktemplate='ssh://{host}')
+    db.session.add(ssh)
+    db.session.commit()
+
+    client.put('/api/computers/%d' % pc, headers=auth_headers,
+               json={'accessmethods': [{'protocolid': ssh.protocolid}]})
+    before = len(_entries(pc))
+    client.put('/api/computers/%d' % pc, headers=auth_headers,
+               json={'accessmethods': [{'protocolid': ssh.protocolid}]})
+    assert len(_entries(pc)) == before