From fd513c08eb88fc34272ac5ebb5af8b18028f8ca8 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Thu, 3 Sep 2026 17:15:03 -0400 Subject: [PATCH] Remediate the 2026-09-03 feature-test findings --- frontend/src/api/index.js | 15 + frontend/src/assets/style.css | 12 + frontend/src/components/EmployeeSearch.vue | 523 +++++++++--------- frontend/src/components/PaginationBar.vue | 179 +++--- frontend/src/composables/assetTypeIds.js | 56 ++ frontend/src/composables/assetTypeIds.spec.js | 94 ++++ frontend/src/composables/employeeResolve.js | 55 ++ .../src/composables/employeeResolve.spec.js | 81 +++ frontend/src/views/SearchResults.vue | 6 + frontend/src/views/settings/AuditSettings.vue | 14 +- .../views/settings/AuthenticationSettings.vue | 43 +- .../7d39_audit_retention_key_merge.py | 95 ++++ ...d40_machine_maintenance_dates_are_dates.py | 73 +++ .../versions/7d41_drop_guest_tools_secrets.py | 56 ++ plugins/backups/api/routes.py | 37 +- plugins/computers/api/routes.py | 83 ++- .../frontend/views/BusinessPoliciesList.vue | 21 +- .../views/ConfigurationPoliciesList.vue | 28 +- plugins/computers/frontend/views/PCDetail.vue | 31 +- plugins/computers/frontend/views/PCForm.vue | 89 ++- .../computers/frontend/views/PCTypesList.vue | 2 +- plugins/computers/frontend/views/PCsList.vue | 25 +- plugins/employees/api/routes.py | 68 ++- plugins/endpoints/api/routes.py | 7 +- plugins/endpoints/plugin.py | 25 +- plugins/geenforce/api/routes.py | 9 + plugins/geenforce/service.py | 39 ++ plugins/guides/api/routes.py | 11 + plugins/knowledgebase/api/routes.py | 7 +- plugins/machines/api/routes.py | 65 ++- .../machines/frontend/views/MachineDetail.vue | 38 +- .../machines/frontend/views/MachineForm.vue | 21 +- .../frontend/views/MachineTypesList.vue | 2 +- .../machines/frontend/views/MachinesList.vue | 12 + plugins/machines/models/machine.py | 8 +- plugins/measuringtools/api/routes.py | 64 ++- .../frontend/views/MeasuringToolDetail.vue | 33 +- .../frontend/views/MeasuringToolForm.vue | 4 +- .../frontend/views/MeasuringToolTypesList.vue | 2 +- .../frontend/views/MeasuringToolsList.vue | 14 +- plugins/network/api/routes.py | 82 ++- .../frontend/views/NetworkDeviceForm.vue | 9 +- .../frontend/views/NetworkDevicesList.vue | 12 +- .../frontend/views/NetworkTypesList.vue | 2 +- plugins/notifications/api/routes.py | 6 +- .../frontend/views/NotificationForm.vue | 25 + plugins/printedparts/api/routes.py | 88 ++- .../frontend/views/PartsKiosk.vue | 54 +- plugins/printedparts/services/badges.py | 28 +- plugins/printers/api/asset_routes.py | 64 ++- .../printers/frontend/views/PrinterDetail.vue | 33 +- .../frontend/views/PrinterDriversList.vue | 43 +- .../printers/frontend/views/PrinterForm.vue | 17 +- .../frontend/views/PrinterTypesList.vue | 2 +- .../printers/frontend/views/PrintersList.vue | 12 +- .../printers/frontend/views/TonerReport.vue | 28 +- .../slides/frontend/views/SlideManager.vue | 39 ++ plugins/tools/api/routes.py | 5 +- plugins/tools/plugin.py | 20 +- plugins/usb/api/routes.py | 22 +- plugins/usb/api/selfhosted.py | 143 ++++- .../versions/0004_checkin_identity.py | 59 ++ plugins/usb/models/usb_device.py | 7 + plugins/warranty/api/routes.py | 49 +- scripts/check-naming-and-style.sh | 10 +- shopdb/__init__.py | 98 ++++ shopdb/api/__init__.py | 22 + shopdb/config.py | 21 +- shopdb/core/api/apitokens.py | 13 +- shopdb/core/api/applications.py | 39 +- shopdb/core/api/assets.py | 68 ++- shopdb/core/api/auditlogs.py | 18 +- shopdb/core/api/maplevels.py | 35 +- shopdb/core/api/models.py | 1 - shopdb/core/api/reports.py | 35 +- shopdb/core/api/search.py | 25 +- shopdb/core/api/settings.py | 43 +- shopdb/core/api/users.py | 21 +- shopdb/core/models/asset.py | 18 + shopdb/core/models/auditlog.py | 351 ++++++------ shopdb/core/models/base.py | 9 +- shopdb/utils/authz.py | 15 +- shopdb/utils/badges.py | 70 +++ shopdb/utils/datevalues.py | 82 +++ shopdb/utils/queryargs.py | 64 +++ tests/test_core/test_apitokens.py | 42 ++ tests/test_core/test_appversion_delivery.py | 50 ++ tests/test_core/test_audit_retention_key.py | 99 ++++ tests/test_core/test_bad_query_args.py | 120 ++++ tests/test_core/test_credential_paths.py | 80 +++ tests/test_core/test_employees.py | 81 +++ tests/test_core/test_error_envelope.py | 66 +++ tests/test_core/test_guest_role.py | 32 ++ tests/test_core/test_identifiers.py | 70 +++ tests/test_core/test_maplevels_public.py | 56 ++ tests/test_core/test_perpage_cap.py | 79 +++ tests/test_core/test_public_read_surface.py | 13 + tests/test_core/test_search_retired.py | 69 +++ tests/test_core/test_strict_write_errors.py | 140 +++++ tests/test_plugin_migrations.py | 5 +- tests/test_plugins/test_backups.py | 139 +++++ tests/test_plugins/test_computers_audit.py | 104 ++++ tests/test_plugins/test_dualpath_single.py | 117 +++- tests/test_plugins/test_endpoints_plugin.py | 63 +++ tests/test_plugins/test_geenforce_export.py | 58 ++ tests/test_plugins/test_guides.py | 21 + tests/test_plugins/test_measuringtools.py | 156 ++++++ tests/test_plugins/test_network_validation.py | 80 +++ .../test_plugins/test_printedparts_ledger.py | 120 ++++ .../test_printer_driver_resolution.py | 57 ++ tests/test_plugins/test_usb_kiosk.py | 123 ++++ tests/test_plugins/test_warranty_proof.py | 45 ++ 112 files changed, 5182 insertions(+), 857 deletions(-) create mode 100644 frontend/src/composables/assetTypeIds.js create mode 100644 frontend/src/composables/assetTypeIds.spec.js create mode 100644 frontend/src/composables/employeeResolve.js create mode 100644 frontend/src/composables/employeeResolve.spec.js create mode 100644 migrations/versions/7d39_audit_retention_key_merge.py create mode 100644 migrations/versions/7d40_machine_maintenance_dates_are_dates.py create mode 100644 migrations/versions/7d41_drop_guest_tools_secrets.py create mode 100644 plugins/usb/migrations/versions/0004_checkin_identity.py create mode 100644 shopdb/utils/badges.py create mode 100644 shopdb/utils/datevalues.py create mode 100644 shopdb/utils/queryargs.py create mode 100644 tests/test_core/test_audit_retention_key.py create mode 100644 tests/test_core/test_bad_query_args.py create mode 100644 tests/test_core/test_credential_paths.py create mode 100644 tests/test_core/test_error_envelope.py create mode 100644 tests/test_core/test_maplevels_public.py create mode 100644 tests/test_core/test_perpage_cap.py create mode 100644 tests/test_core/test_search_retired.py create mode 100644 tests/test_core/test_strict_write_errors.py create mode 100644 tests/test_plugins/test_computers_audit.py create mode 100644 tests/test_plugins/test_network_validation.py diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index 858814e5..6c7a0da0 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -405,6 +405,12 @@ export const vendorsApi = { list(params = {}) { return api.get('/vendors', { params }) }, + // Every vendor, paged. perpage is capped at 100 server-side, so a picker + // asking for 500 silently showed the first 100 and dropped the rest - with a + // 200, so nothing looked wrong. + listAll(params = {}) { + return fetchAllPages('/vendors', params) + }, get(id) { return api.get(`/vendors/${id}`) }, @@ -667,6 +673,10 @@ export const businessunitsApi = { list(params = {}) { return api.get('/businessunits', { params }) }, + // See vendorsApi.listAll: the cap is 100 whatever the caller asks for. + listAll(params = {}) { + return fetchAllPages('/businessunits', params) + }, get(id) { return api.get(`/businessunits/${id}`) }, @@ -1306,6 +1316,11 @@ export const slidesApi = { }, remove(surface, files) { return api.post(`/slides/${surface}/delete`, { files }) + }, + // Per-slide hold time. The route has always existed; nothing called it, so + // the documented per-slide seconds could only be set with curl. + update(surface, slideid, data) { + return api.patch(`/slides/${surface}/${slideid}`, data) } } diff --git a/frontend/src/assets/style.css b/frontend/src/assets/style.css index 30b8d035..e3f6afff 100644 --- a/frontend/src/assets/style.css +++ b/frontend/src/assets/style.css @@ -1153,6 +1153,18 @@ td.actions .btn + .btn { margin: 0 0 1rem 0; line-height: 1.5; } +/* A settings block whose feature is not built yet. Reads as a warning rather + than body text, because the failure it prevents is an administrator + believing a control does something. */ +.setting-notice { + border-left: 3px solid var(--warning); + background: var(--bg); + color: var(--text); + font-size: 0.9rem; + line-height: 1.5; + margin: 0 0 1rem 0; + padding: 0.6rem 0.8rem; +} .settings-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(180px, 1fr)); diff --git a/frontend/src/components/EmployeeSearch.vue b/frontend/src/components/EmployeeSearch.vue index 9be6b154..b34a5f2d 100644 --- a/frontend/src/components/EmployeeSearch.vue +++ b/frontend/src/components/EmployeeSearch.vue @@ -1,251 +1,272 @@ - - - - - + + + + + diff --git a/frontend/src/components/PaginationBar.vue b/frontend/src/components/PaginationBar.vue index 6bced812..0f892c64 100644 --- a/frontend/src/components/PaginationBar.vue +++ b/frontend/src/components/PaginationBar.vue @@ -1,83 +1,96 @@ - - - + + + diff --git a/frontend/src/composables/assetTypeIds.js b/frontend/src/composables/assetTypeIds.js new file mode 100644 index 00000000..eb013aef --- /dev/null +++ b/frontend/src/composables/assetTypeIds.js @@ -0,0 +1,56 @@ +// Asset-type ids resolved by NAME, once per page load. +// +// Four asset forms carried a hardcoded id - `const COMPUTER_ASSETTYPEID = 2`, +// and the same for machines, printers and network devices - to tell +// CustomFieldsInputs which type's fields to render. The ids come from seed +// order, not from anything stable: on the instance those constants were written +// against, computer was 2; on a freshly seeded one it is 1 and 2 is +// network_device. So the PC form asked for another type's custom fields, +// rendered them, and dropped whatever the operator typed - with a success +// toast, because the values were keyed to a type the record does not have. +// +// A name is the stable thing. `assettype` is a fixed vocabulary (ADR-001) and +// the API already publishes the mapping; nothing else should hardcode a number. +import { reactive } from 'vue' +import { assetsApi } from '../api' + +const state = reactive({ + byname: {}, + loaded: false +}) + +let inflight = null + +export async function loadAssetTypeIds() { + if (state.loaded) return state + if (inflight) return inflight + inflight = assetsApi.types.list() + .then((response) => { + const rows = response?.data?.data || response?.data || [] + const byname = {} + rows.forEach((row) => { + if (row && row.assettype) byname[row.assettype] = row.assettypeid + }) + state.byname = byname + state.loaded = true + return state + }) + .catch(() => { + // Leave it unloaded rather than caching a failure: a form that could not + // resolve its type renders no custom fields, which is honest. Guessing an + // id is what caused the defect this replaces. + state.loaded = false + return state + }) + .finally(() => { inflight = null }) + return inflight +} + +// Undefined until loaded, and undefined for a name the server does not know. +// CustomFieldsInputs treats an undefined assettypeid as "nothing to render", +// which is the safe reading: better a missing panel than the wrong type's. +export function assetTypeId(name) { + return state.byname[name] +} + +export { state } diff --git a/frontend/src/composables/assetTypeIds.spec.js b/frontend/src/composables/assetTypeIds.spec.js new file mode 100644 index 00000000..995d01cd --- /dev/null +++ b/frontend/src/composables/assetTypeIds.spec.js @@ -0,0 +1,94 @@ +// Asset-type ids must come from the server by name. +// +// Four forms hardcoded the id. Those constants were written against one +// instance's seed order and were wrong on another: PCForm said 2, and on a +// freshly seeded database 2 is network_device. The form asked +// CustomFieldsInputs for that type's fields, rendered them, and dropped what +// the operator typed - with a success toast, because the values were keyed to +// a type the record does not have. +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../api', () => ({ + assetsApi: { + types: { + list: vi.fn() + } + } +})) + +import { assetsApi } from '../api' +import { loadAssetTypeIds, assetTypeId, state } from './assetTypeIds' + +function reset() { + state.byname = {} + state.loaded = false + assetsApi.types.list.mockReset() +} + +describe('assetTypeIds', () => { + beforeEach(reset) + + it('resolves an id by name from what the server actually has', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [ + { assettypeid: 1, assettype: 'computer' }, + { assettypeid: 2, assettype: 'network_device' }, + { assettypeid: 4, assettype: 'machine' } + ] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('computer')).toBe(1) + expect(assetTypeId('network_device')).toBe(2) + expect(assetTypeId('machine')).toBe(4) + }) + + it('survives a differently seeded site, which is the whole point', async () => { + // The same names, different ids. A hardcoded 2 would be network_device + // here and computer in the other fixture. + assetsApi.types.list.mockResolvedValue({ + data: { data: [ + { assettypeid: 2, assettype: 'computer' }, + { assettypeid: 3, assettype: 'network_device' } + ] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('computer')).toBe(2) + expect(assetTypeId('network_device')).toBe(3) + }) + + it('fetches once even when several forms ask at the same time', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [{ assettypeid: 1, assettype: 'computer' }] } + }) + + await Promise.all([loadAssetTypeIds(), loadAssetTypeIds(), loadAssetTypeIds()]) + await loadAssetTypeIds() + + expect(assetsApi.types.list).toHaveBeenCalledTimes(1) + }) + + it('returns undefined rather than a guess when the load fails', async () => { + assetsApi.types.list.mockRejectedValue(new Error('network down')) + + await loadAssetTypeIds() + + // Undefined means CustomFieldsInputs renders nothing. A missing panel is + // honest; the wrong type's panel is what this replaces. + expect(assetTypeId('computer')).toBeUndefined() + expect(state.loaded).toBe(false) + }) + + it('returns undefined for a type the server does not know', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [{ assettypeid: 1, assettype: 'computer' }] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('forklift')).toBeUndefined() + }) +}) diff --git a/frontend/src/composables/employeeResolve.js b/frontend/src/composables/employeeResolve.js new file mode 100644 index 00000000..acdd141c --- /dev/null +++ b/frontend/src/composables/employeeResolve.js @@ -0,0 +1,55 @@ +// Turning what somebody typed into an employee, before giving up on it. +// +// Both employee pickers had the same shape: click a suggestion and you get the +// person, with their SSO; type the whole name and press Enter and you get a +// free-text label (`NAME:Jane Smith`) that is linked to nothing. It looks +// identical on screen - the chip says "Jane Smith" either way - and the +// consequences only show up later: no photo, no directory link, and a +// recognition that cannot be found by the person it is about. +// +// Nobody types a name in full expecting a different outcome from picking it +// off the list. So a typed name is looked up first, and only becomes a custom +// label when the directory genuinely does not have it. +// +// An ambiguous name is NOT guessed at. Two people called J Smith is exactly +// when picking the wrong one matters most, so the caller is told to choose. +import { employeesApi } from '../api' + +function fullname(employee) { + return `${employee.First_Name || ''} ${employee.Last_Name || ''}`.trim() +} + +// { match } when one employee is clearly meant, { matches } when several are +// possible, {} when the directory has nobody by that name. +export async function resolveEmployeeName(name) { + const query = (name || '').trim() + if (!query) return {} + + let rows = [] + try { + const response = await employeesApi.search(query) + rows = response?.data?.data || [] + } catch (err) { + // A directory that cannot be reached must not silently turn a real + // employee into a custom label - the caller decides what to do about it. + // The MESSAGE, not the error object: a rejected mock handed to + // console.error is re-surfaced by the test runner as an unhandled error, + // which fails the very test that proves this branch works. + console.warn('Employee lookup failed:', err && err.message) + return { failed: true } + } + + if (!rows.length) return {} + + // An exact full-name match wins over a partial one: searching "Dana Reyes" + // can also return "Dana Reyes-Smith", and the person typing the shorter name + // meant the shorter name. + const exact = rows.filter( + (row) => fullname(row).toLowerCase() === query.toLowerCase()) + if (exact.length === 1) return { match: exact[0] } + if (exact.length > 1) return { matches: exact } + if (rows.length === 1) return { match: rows[0] } + return { matches: rows } +} + +export { fullname } diff --git a/frontend/src/composables/employeeResolve.spec.js b/frontend/src/composables/employeeResolve.spec.js new file mode 100644 index 00000000..860102f3 --- /dev/null +++ b/frontend/src/composables/employeeResolve.spec.js @@ -0,0 +1,81 @@ +// Typing a name in full must reach the same employee as picking it off the list. +// +// Both pickers turned a typed name straight into `NAME:` - a free-text +// label linked to nothing. On screen the chip is identical to a real +// selection, so the difference only shows up later: no photo, no directory +// record, and a recognition that cannot be found by the person it is about. +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../api', () => ({ + employeesApi: { search: vi.fn() } +})) + +import { employeesApi } from '../api' +import { resolveEmployeeName } from './employeeResolve' + +const DANA = { SSO: 270015376, First_Name: 'Dana', Last_Name: 'Reyes' } +const DANA_HYPHEN = { SSO: 270099999, First_Name: 'Dana', Last_Name: 'Reyes-Smith' } +const SAM = { SSO: 270000001, First_Name: 'Sam', Last_Name: 'Reyes' } + +function found(rows) { + employeesApi.search.mockResolvedValue({ data: { data: rows } }) +} + +describe('resolveEmployeeName', () => { + beforeEach(() => employeesApi.search.mockReset()) + + it('resolves a full name to the employee', async () => { + found([DANA]) + const { match } = await resolveEmployeeName('Dana Reyes') + expect(match.SSO).toBe(270015376) + }) + + it('prefers an exact full-name match over a longer partial one', async () => { + // Searching "Dana Reyes" also returns "Dana Reyes-Smith"; the person who + // typed the shorter name meant the shorter name. + found([DANA, DANA_HYPHEN]) + const { match, matches } = await resolveEmployeeName('Dana Reyes') + expect(matches).toBeUndefined() + expect(match.SSO).toBe(270015376) + }) + + it('refuses to guess between genuinely ambiguous matches', async () => { + found([DANA, SAM]) + const { match, matches } = await resolveEmployeeName('Reyes') + expect(match).toBeUndefined() + expect(matches).toHaveLength(2) + }) + + it('reports nothing found, so the caller may fall back to a label', async () => { + found([]) + const result = await resolveEmployeeName('Somebody External') + expect(result.match).toBeUndefined() + expect(result.matches).toBeUndefined() + expect(result.failed).toBeUndefined() + }) + + it('says the lookup FAILED rather than reporting nobody', async () => { + // The difference matters: "not in the directory" may become a custom + // label, "could not ask" must not. + // A THENABLE that calls its reject callback, rather than a mock returning + // a rejected promise. The await sees the same failure, but no rejected + // promise object exists for the runner to collect as an unhandled error - + // which it does even when the code under test catches it, failing the very + // test that proves the catch works. + employeesApi.search.mockImplementationOnce(() => { + const failure = new Error('network down') + return { then: (_ok, fail) => fail(failure) } + }) + // resolves.toEqual, so the runner sees the rejection consumed by an + // assertion rather than as an unhandled error of its own. + await expect(resolveEmployeeName('Dana Reyes')).resolves.toEqual({ + failed: true + }) + }) + + it('does not call the directory for an empty box', async () => { + const result = await resolveEmployeeName(' ') + expect(employeesApi.search).not.toHaveBeenCalled() + expect(result).toEqual({}) + }) +}) diff --git a/frontend/src/views/SearchResults.vue b/frontend/src/views/SearchResults.vue index 3e3d4b41..41f9985b 100644 --- a/frontend/src/views/SearchResults.vue +++ b/frontend/src/views/SearchResults.vue @@ -83,6 +83,12 @@ {{ result.location }} {{ result.ticketnumber }} Active + + retired - Keep enabled for admin fallback access + + Always on for now. Local accounts are the only way in until SAML + sign-in ships, so this cannot be turned off yet. +
@@ -148,15 +162,20 @@ const { // Per-page connection status (stays with the page, not the composable). const samlStatus = computed(() => { if (!settings.saml_enabled) return 'inactive' - if (!settings.saml_idp_metadata_url || !settings.saml_entity_id) return 'warning' + // 'pending' rather than a healthy state even when every field is filled in: + // nothing is connected until the feature exists. return 'pending' }) +// Never says "available". It used to report "Configured - SAML login +// available" as soon as two fields held text, which is a claim about a feature +// that does not exist: an administrator could reasonably read that as SSO +// being live and stop issuing local accounts. const samlMessage = computed(() => { - if (!settings.saml_enabled) return 'Disabled - using local authentication only' - if (!settings.saml_idp_metadata_url) return 'IdP metadata URL not configured' - if (!settings.saml_entity_id) return 'Entity ID not configured' - return 'Configured - SAML login available' + if (!settings.saml_enabled) return 'Off - local accounts only' + if (!settings.saml_idp_metadata_url) return 'IdP metadata URL not set' + if (!settings.saml_entity_id) return 'Entity ID not set' + return 'Settings saved - waiting on SAML support, local accounts still in use' }) onMounted(loadSettings) diff --git a/migrations/versions/7d39_audit_retention_key_merge.py b/migrations/versions/7d39_audit_retention_key_merge.py new file mode 100644 index 00000000..3ac412dd --- /dev/null +++ b/migrations/versions/7d39_audit_retention_key_merge.py @@ -0,0 +1,95 @@ +"""Merge the second audit-retention setting into the one that is actually read. + +Two keys existed for one setting. shopdb/core/services/auditretention.py reads +`audit_retentiondays` (seeded under category 'site', default 0). A second key +`audit_retention_days` was seeded under category 'audit' with a default of 90, +and the Audit & Logging settings page wrote to that one - so an administrator +set a retention period, reloaded, saw it persisted, and nothing ever consulted +it. Retention stayed off while the page said otherwise. + +The page now writes the key that is read, and the duplicate is out of the seed +list. This handles the sites that already have it: if an operator set a real +value on the dead key and never set the live one, that intent is carried over +rather than dropped. Their intent was "keep 30 days", and the fix should not +silently discard it. + +Not carried over when the live key already holds a non-default value - that one +was set through the page that worked, and it wins. + +The dead key is then deleted. Leaving it would preserve the ambiguity that +caused this, and nothing reads it. + +Revision ID: 7d39_audit_retention_key_merge +Revises: 7d38_widen_importbatch_status +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d39_audit_retention_key_merge' +down_revision = '7d38_widen_importbatch_status' +branch_labels = None +depends_on = None + +DEAD = 'audit_retention_days' +LIVE = 'audit_retentiondays' + + +def _value(bind, key): + return bind.execute( + sa.text('SELECT value FROM settings WHERE `key` = :key' + if bind.dialect.name == 'mysql' + else 'SELECT value FROM settings WHERE "key" = :key'), + {'key': key}).scalar() + + +def upgrade(): + bind = op.get_bind() + if not sa.inspect(bind).has_table('settings'): + return + + dead = _value(bind, DEAD) + if dead is None: + return + + live = _value(bind, LIVE) + # '90' was the dead key's seeded default, so it is not evidence of intent. + operator_set_dead = dead not in (None, '', '90') + live_is_default = live in (None, '', '0') + + if operator_set_dead and live_is_default: + if live is None: + bind.execute(sa.text( + 'INSERT INTO settings (`key`, value, valuetype, category, ' + 'description, createddate, modifieddate, isactive) VALUES ' + '(:key, :value, :valuetype, :category, :description, ' + 'NOW(), NOW(), 1)' + if bind.dialect.name == 'mysql' else + 'INSERT INTO settings ("key", value, valuetype, category, ' + 'description, createddate, modifieddate, isactive) VALUES ' + '(:key, :value, :valuetype, :category, :description, ' + "datetime('now'), datetime('now'), 1)"), + {'key': LIVE, 'value': dead, 'valuetype': 'integer', + 'category': 'site', + 'description': 'Delete audit log entries older than this many ' + 'days. 0 keeps everything forever.'}) + else: + bind.execute(sa.text( + 'UPDATE settings SET value = :value WHERE `key` = :key' + if bind.dialect.name == 'mysql' else + 'UPDATE settings SET value = :value WHERE "key" = :key'), + {'value': dead, 'key': LIVE}) + + bind.execute(sa.text( + 'DELETE FROM settings WHERE `key` = :key' + if bind.dialect.name == 'mysql' else + 'DELETE FROM settings WHERE "key" = :key'), {'key': DEAD}) + + +def downgrade(): + """Deliberately a no-op. + + Re-creating a key that nothing reads, to sit beside the one that works, + would restore the defect. The live setting keeps whatever value it holds. + """ diff --git a/migrations/versions/7d40_machine_maintenance_dates_are_dates.py b/migrations/versions/7d40_machine_maintenance_dates_are_dates.py new file mode 100644 index 00000000..74b48014 --- /dev/null +++ b/migrations/versions/7d40_machine_maintenance_dates_are_dates.py @@ -0,0 +1,73 @@ +"""Store a maintenance date as a DATE, because that is what it is. + +lastmaintenancedate and nextmaintenancedate were DATETIME columns holding +midnight. BaseModel.to_dict serialises a datetime as isoformat + 'Z', so +2026-12-15 00:00:00 went out as "2026-12-15T00:00:00Z" and a browser west of +UTC rendered it as 14 December. The maintenance panel showed every date one +calendar day early on this site, and would show it a day LATE east of UTC. + +There is no time in a maintenance date. Nobody records that a machine was +serviced at midnight; the form offers a date picker, the CSV importer takes +'2026-06-01', and the value is compared against other dates. Every sibling +already models it correctly - measuringtools.lastcalibrationdate, +warranties.startdate and enddate are all db.Date - so this is machines being +the odd one out rather than a new decision. + +As a Date, to_dict emits "2026-12-15" with no timezone marker at all, and +nothing can shift it. + +MySQL truncates the (midnight) time component on the ALTER, so no data moves. +SQLite stores what it is given and does not enforce column types, so the +conversion is a no-op there and is skipped. + +Revision ID: 7d40_machine_maintenance_dates +Revises: 7d39_audit_retention_key_merge +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d40_machine_maintenance_dates' +down_revision = '7d39_audit_retention_key_merge' +branch_labels = None +depends_on = None + +COLUMNS = ('lastmaintenancedate', 'nextmaintenancedate') + + +def _machines_table(bind): + inspector = sa.inspect(bind) + if 'machines' not in inspector.get_table_names(): + return None + return {column['name'] for column in inspector.get_columns('machines')} + + +def upgrade(): + bind = op.get_bind() + if bind.dialect.name == 'sqlite': + return + present = _machines_table(bind) + if not present: + return + for column in COLUMNS: + if column in present: + op.alter_column('machines', column, + existing_type=sa.DateTime(), + type_=sa.Date(), + existing_nullable=True) + + +def downgrade(): + bind = op.get_bind() + if bind.dialect.name == 'sqlite': + return + present = _machines_table(bind) + if not present: + return + for column in COLUMNS: + if column in present: + op.alter_column('machines', column, + existing_type=sa.Date(), + type_=sa.DateTime(), + existing_nullable=True) diff --git a/migrations/versions/7d41_drop_guest_tools_secrets.py b/migrations/versions/7d41_drop_guest_tools_secrets.py new file mode 100644 index 00000000..50ac629d --- /dev/null +++ b/migrations/versions/7d41_drop_guest_tools_secrets.py @@ -0,0 +1,56 @@ +"""Revoke a guest grant that never did anything, before it locks the role. + +tools.secrets was declared guest-grantable, so the role grid offered it and +stored it - and the secure-send route never consulted the guest ROLE at all. It +decides on the securesend_allow_guests setting, which fails closed and carries +the rate limit. A site that ticked the box got no change in behaviour, in either +direction. + +The plugin no longer declares it grantable. That alone would repeat the deadlock +fixed in 723a4ff: update_role refuses the WHOLE request when a submitted +permission is not grantable, and the role grid resubmits what the role already +holds - so a site whose guest role holds tools.secrets could no longer save the +guest role at all, including to change something else entirely. + +So the grant is removed here, from the one role that could hold it. Nothing is +lost: it granted nothing. Guest secret-link creation stays exactly where it was, +under securesend_allow_guests. + +Only the GUEST role is touched. A signed-in role holding tools.secrets is +unaffected - that permission works normally for an account. + +Revision ID: 7d41_drop_guest_tools_secrets +Revises: 7d40_machine_maintenance_dates +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d41_drop_guest_tools_secrets' +down_revision = '7d40_machine_maintenance_dates' +branch_labels = None +depends_on = None + + +def upgrade(): + bind = op.get_bind() + inspector = sa.inspect(bind) + tables = set(inspector.get_table_names()) + if not {'roles', 'permissions', 'rolepermissions'} <= tables: + return + + bind.execute(sa.text( + 'DELETE FROM rolepermissions WHERE roleid IN ' + '(SELECT roleid FROM roles WHERE rolename = :guest) ' + 'AND permissionid IN ' + '(SELECT permissionid FROM permissions WHERE name = :permission)'), + {'guest': 'guest', 'permission': 'tools.secrets'}) + + +def downgrade(): + """Deliberately a no-op. + + Re-granting a permission that nothing reads would restore the confusion + rather than any capability. + """ diff --git a/plugins/backups/api/routes.py b/plugins/backups/api/routes.py index 26953834..06cec5fb 100644 --- a/plugins/backups/api/routes.py +++ b/plugins/backups/api/routes.py @@ -116,13 +116,25 @@ def asset_revisions(assetid): revisions = query.order_by(BackupRevision.backuprevisionid.desc()).all() - # "Latest" is per kind, not per asset, so an unfiltered listing still marks - # the current revision of each kind correctly. + # "Latest" is per kind AND per source chain. Per kind alone was wrong on + # the case this exists for: NTLARS settings describe a machine but are read + # off the controlling PC, so a machine with two reporting PCs has two + # independent chains of the same kind. Marking only the highest revision id + # meant the second PC's current revision was badged as history and lost its + # "last verified" stamp - the machine looked like it had one source that + # kept re-reporting, rather than two. + # + # sourcehostname is the chain: it records which PC produced the revision. + # A revision with none (an older row, or a share-drop with no host) forms + # its own chain under the empty key, which keeps the per-kind behaviour for + # everything that has only ever had one source. seen = set() out = [] for revision in revisions: - islatest = revision.backupkind not in seen - seen.add(revision.backupkind) + chain = (revision.backupkind, + (revision.sourcehostname or '').strip().lower()) + islatest = chain not in seen + seen.add(chain) out.append(_summary(revision, islatest=islatest)) return success_response(out) @@ -297,6 +309,23 @@ def diff_revision(backuprevisionid): againstid = request.args.get('against', type=int) if againstid: other = db.session.get(BackupRevision, againstid) + # It has to belong to the SAME asset. Any revision id was accepted, so + # ?against= rendered that asset's + # configuration values as the "before" side of this asset's history - + # a diff that reads as a change on this machine and describes a + # different one. The default branch below has always been asset-scoped; + # only the explicit one was not. + if other is not None and other.assetid != revision.assetid: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Revision {} belongs to a different asset and cannot be ' + 'compared against this one'.format(againstid)) + if other is not None and other.backupkind != revision.backupkind: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Revision {} is a {} backup and this one is {} - there is ' + 'nothing to compare'.format(againstid, other.backupkind, + revision.backupkind)) else: # The previous revision FROM THE SAME PC. Several PCs can share a # machine number, and diffing across them reported one device's COM port diff --git a/plugins/computers/api/routes.py b/plugins/computers/api/routes.py index 9adb608a..c455f32d 100644 --- a/plugins/computers/api/routes.py +++ b/plugins/computers/api/routes.py @@ -7,7 +7,7 @@ from shopdb.api import db, Asset, AssetType, OperatingSystem, Application, Audit from ..models import Computer, ComputerType, ComputerInstalledApp, AccessProtocol, ComputerAccess -from shopdb.api import require_permission, require_role, apply_import_timestamps +from shopdb.api import require_permission, require_role, apply_import_timestamps, intarg computers_bp = Blueprint('computers', __name__) @@ -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 @@ -380,20 +416,20 @@ def list_computers(): ) # Computer type filter - if type_id := request.args.get('typeid', request.args.get('type_id')): - query = query.filter(Computer.computertypeid == int(type_id)) + if (type_id := intarg('typeid', intarg('type_id'))) is not None: + query = query.filter(Computer.computertypeid == type_id) # OS filter - if os_id := request.args.get('osid', request.args.get('os_id')): - query = query.filter(Computer.osid == int(os_id)) + if (os_id := intarg('osid', intarg('os_id'))) is not None: + query = query.filter(Computer.osid == os_id) # Location filter - if location_id := request.args.get('locationid', request.args.get('location_id')): - query = query.filter(Asset.locationid == int(location_id)) + if (location_id := intarg('locationid', intarg('location_id'))) is not None: + query = query.filter(Asset.locationid == location_id) # Business unit filter - if bu_id := request.args.get('businessunitid', request.args.get('businessunit_id')): - query = query.filter(Asset.businessunitid == int(bu_id)) + if (bu_id := intarg('businessunitid', intarg('businessunit_id'))) is not None: + query = query.filter(Asset.businessunitid == bu_id) # Shopfloor filter (by the Shopfloor computer type) if shopfloor := request.args.get('shopfloor'): @@ -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, - ipaddress=ip, isprimary=True)) + db.session.add(Communication( + 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: diff --git a/plugins/computers/frontend/views/BusinessPoliciesList.vue b/plugins/computers/frontend/views/BusinessPoliciesList.vue index e2f3b971..db4d7a4e 100644 --- a/plugins/computers/frontend/views/BusinessPoliciesList.vue +++ b/plugins/computers/frontend/views/BusinessPoliciesList.vue @@ -2,7 +2,14 @@

Site Policies

- + +
+ +
+ Site policies are not enabled on this site. +

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.

@@ -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) {