diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d46109c..af07bb30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,104 @@ ADR-007 and ADR-002. ### Added +- **Audit log retention.** The audit log is append-only and nothing ever removed + a row, so it grew for as long as a site ran - here to the point where the + audit page was tens of thousands of pages deep and therefore useless, because + nobody pages through that to find anything. Site & Facility gains + `audit_retentiondays` plus a card showing the log's size and exactly how many + entries a purge would remove, and there is `flask audit stats` / + `flask audit purge --apply` for the server. **Opt-in: the default is 0, which + keeps everything.** An upgrade that silently started deleting a site's audit + history would be indefensible. Deletion runs in committed batches, because one + `DELETE` over a million rows holds table locks long enough to block the + collectors posting inventory; a run that stops early reports how much is left + rather than claiming it finished. Purging needs its own `audit.purge` + permission - reading the log and destroying it are different acts. + +- **Guides: images, and a reader who cannot see drafts.** Guide images are + fetched through the authenticated API client instead of being pointed at by + ``. An image tag sends no Authorization header and this app keeps its + JWT in one, so every guide image was a broken icon for every user; the same + change makes stored image links survive a subpath mount. A failed image now + says so in words rather than showing a broken icon. Revision and image routes + are gated on their parent guide - revision ids are sequential, so reading one + guide used to be enough to walk the ids and read every unpublished draft on + the site. Search, the guide list and the tag chips now match the PUBLISHED + text, not the working copy, so a reader can no longer get a hit on wording + that is not in the page when they open it. + +### Fixed + +- **A guide's URL no longer moves when its title is edited.** The slug was + regenerated on every save that carried a title, and the editor autosaves every + three seconds - so fixing a typo in a heading silently moved the guide and + broke exactly the pasted links the stored slug exists to protect. The slug is + now set once, at create. + +- **Publishing a guide after a failed save reported success.** `publish()` ran + regardless of whether the pre-publish save reached the server, so the author + saw "Published version N" while readers got the previous revision. A failed + save now stops the publish and says why. + +- **The ADR-015 site-literal check was enforcing nothing.** Its patterns are + assembled from fragments so the published script does not contain the strings + it greps for, but they used the apostrophe-escape form (`'a'\''b'`) instead of + concatenation (`'a''b'`), embedding a literal apostrophe in every pattern. + Nothing could ever match and the check passed everything from the day it was + written. Fixed, and it immediately found 25 real violations - among them a QA + ServiceNow tenant URL sitting in the shipped defaults in seven places. + +- **ServiceNow ships off and blank.** The defaults pointed at one organisation's + QA ServiceNow tenant with the integration enabled, so a site that had never + configured ServiceNow still rendered ticket links - to a tenant its + technicians cannot reach, and for an adopting site, into somebody else's + instance. There is no cross-site default for "which ServiceNow is yours". + +- **Credentials are no longer stored in backup revisions.** The NTLARS/DNC + registry carries an FTP password in cleartext, and it was stored verbatim - + rendered by the revision panel and handed back by the download route as a + ready-to-import `.reg`. Credential values are now redacted at parse, so they + never reach the database, and the rendered file NAMES each withheld value + instead of carrying a placebo one: importing a fake password would produce a + machine that looks restored and cannot connect. `scripts/redact_backup_secrets.py` + scrubs revisions recorded before this (dry run by default). Rotate any + password that was stored. + +- **A search domain that fails is no longer indistinguishable from one that + found nothing.** Each domain catches broadly and returns no rows so one broken + table cannot take the whole search box down - but the technician was then told + they might have misspelled a query that was right, and concluded the record + did not exist. Failed domains are named in the response, shown as a banner, + and suppress the did-you-mean. + +- **An empty `sourcehostname` defeated backup dedup permanently.** The dedup + query maps `''` to `IS NULL` while the insert stored the raw `''`, so those + rows could never match their own chain: every post inserted, the table grew + without bound and the history filled with identical entries. Normalized once + at ingest, whitespace included. + +- **`printedparts` migration 0003 was unguarded.** Unlike its twenty siblings it + ran a bare `ALTER`, so `flask plugin upgrade-all` failed on any database built + by `create_all` and took 0004 down with it. The migration test that should + have caught this only covered single-migration chains; it now covers every + post-cutover chain. + +- **Guide links with several parameters opened the wrong page.** Text is escaped + before formatting, so a captured URL arrives carrying entities and was then + escaped a second time - `&amp;` in every multi-parameter ServiceNow and + SharePoint link. Emphasis could also reach inside an `href`, and + `//host` passed as an in-app link when the browser reads it as off-site. + +- **Retire an asset from the interface.** Machines, PCs, printers and measuring + tools had no way to take a record out of ShopDB without calling the API, so a + scrapped tool stayed in every list and report. The endpoints already existed + and were already soft deletes: the record leaves the lists while its + calibration history, backups and relationships stay put, and it can be + restored by setting `isactive` back. Called Retire rather than Delete for + that reason, and the confirm says what is kept. USB is not included - it + retires through a status column in the external CMMC database and needs its + own decision. + - **`flask catalog dedupe`, `merge` and `rename`: catalog hygiene.** The legacy data carried space-stripped twins - `DellInc.` beside `Dell Inc.`, `Windows10` beside `Windows 10` - which rode into the shipped seed catalog @@ -90,6 +188,24 @@ ADR-007 and ADR-002. ### Fixed +- **Every detail page had the same browser tab title.** A machine, a PC and a + printer all read "West Jefferson ShopDB - Machine" or the equivalent, so two + tabs on two different records were indistinguishable, a bookmark said nothing + about what it pointed at, and a link shared into Teams previewed as the + section rather than the thing. Detail pages now name the record: machines, + PCs, printers, network devices, subnets, measuring tools, USB devices, + printed parts, applications, employees, knowledge base articles and guides. + A record with no usable name, or one that failed to load, keeps the section + name, and a long name is cut at a word boundary. Report pages gained proper + titles too, which also fixes "Pc Relationships". + +- **Guides would have been dropped at provisioning.** The plugin was missing + from `deploy/site-profile-universal.json`, the profile the released Windows + installer is built from, so the wizard would not have offered it and + `flask plugin prune-schema` drops the tables of any plugin absent from the + profile (ADR-014). Its model also imported core directly rather than through + `shopdb.api`, which the contract test catches. + - **The shipped catalog offered two ways to spell the same vendor, and one row that should never have left a test.** `HP Inc.` and `Zebra Technologies` were empty rows duplicating `HP` and `Zebra`, which carry every model between diff --git a/deploy/site-profile-universal.json b/deploy/site-profile-universal.json index 64b7539e..7308f77d 100644 --- a/deploy/site-profile-universal.json +++ b/deploy/site-profile-universal.json @@ -5,6 +5,7 @@ "computers", "employees", "geenforce", + "guides", "knowledgebase", "machines", "measuringtools", diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index f323ff10..e13ef02e 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -359,6 +359,11 @@ export const printersApi = { update(id, data) { return api.put(`/printers/${id}`, data) }, + // Soft delete: the server deactivates the asset, so the printer leaves every + // list while its history and relationships stay intact. + remove(id) { + return api.delete(`/printers/${id}`) + }, updateExtension(id, data) { return api.put(`/printers/${id}/printerdata`, data) }, diff --git a/frontend/src/composables/pageTitle.js b/frontend/src/composables/pageTitle.js new file mode 100644 index 00000000..0b5777b0 --- /dev/null +++ b/frontend/src/composables/pageTitle.js @@ -0,0 +1,91 @@ +// The browser tab title, including WHICH record is open. +// +// The router can only name the KIND of page, because it runs before the view +// has fetched anything: every application read " ShopDB - +// Application", so two tabs on two different machines were indistinguishable, +// and a shared link previewed as the section rather than the thing. +// +// So a detail view names itself once its record arrives: +// +// setRecordTitle(machine.assetnumber) +// +// THE ORDERING IS THE WHOLE PROBLEM. The router's afterEach awaits the site +// settings before it can build " ShopDB - ", so on a cold load +// it finishes AFTER a view that fetched quickly has already set a good title, +// and would overwrite it with the generic one. Both writers therefore go +// through here, and the generic write is dropped when a record title already +// belongs to the route being landed on. The key is the route's fullPath, so +// navigating away invalidates the previous record's claim. + +import { getFacilityName } from '@/utils/siteSettings' + +let activeKey = null // fullPath of the route the tab is currently showing +let recordTitle = null // record name set by a view for activeKey, if any +let sitePrefix = 'ShopDB' // ' ShopDB', resolved once and cached + +function compose(page) { + return page && page !== 'Home' ? `${sitePrefix} - ${page}` : sitePrefix +} + +async function resolveSitePrefix() { + try { + const facility = await getFacilityName() + if (facility && facility !== 'ShopDB') sitePrefix = `${facility} ShopDB` + } catch (settingsError) { + // settings unavailable: plain ShopDB is a fine tab title + } + return sitePrefix +} + +// Called by the router on every navigation, BEFORE it awaits anything, so a +// view that resolves first is not treated as belonging to the previous route. +export function beginNavigation(key) { + activeKey = key + recordTitle = null +} + +// Called by the router once it knows the site prefix. Skipped when a view has +// already named the record for this route. +export async function applyGenericTitle(key, page) { + await resolveSitePrefix() + if (key !== activeKey) return // navigated on while we awaited + // A view that named the record before the facility name arrived composed its + // title against the placeholder prefix, so rewrite it rather than leaving + // 'ShopDB - CSF16' without the facility name in front of it. + document.title = compose(recordTitle !== null ? recordTitle : page) +} + +// A tab shows very little, and the browser truncates mid-word wherever it runs +// out. Cut at a word boundary instead, so what survives is readable. Knowledge +// base articles are the case that forces this: their name is a whole sentence +// of description, not a short label. +const MAXLENGTH = 60 + +function shorten(text) { + if (text.length <= MAXLENGTH) return text + const cut = text.slice(0, MAXLENGTH) + const space = cut.lastIndexOf(' ') + return `${(space > MAXLENGTH / 2 ? cut.slice(0, space) : cut).trimEnd()}...` +} + +// Called by a detail view once its record has loaded. An empty or missing name +// leaves the generic title in place rather than showing a bare id: "Machine" +// beats "Machine 4192" for somebody scanning a row of tabs, and a record that +// failed to load has no name to show anyway. +export function setRecordTitle(name) { + const text = (name === null || name === undefined) ? '' : String(name).trim() + if (!text) return + recordTitle = shorten(text) + document.title = compose(recordTitle) +} + +// Exposed for tests. +export function _state() { + return { activeKey, recordTitle, sitePrefix } +} + +export function _reset() { + activeKey = null + recordTitle = null + sitePrefix = 'ShopDB' +} diff --git a/frontend/src/composables/pageTitle.spec.js b/frontend/src/composables/pageTitle.spec.js new file mode 100644 index 00000000..ae7aab07 --- /dev/null +++ b/frontend/src/composables/pageTitle.spec.js @@ -0,0 +1,177 @@ +// The tab title names the record, and the two writers do not fight. +// +// The router can only name the KIND of page - it runs before the view has +// fetched anything - so a detail view names the record itself once it loads. +// That leaves two writers for one string, and the router's is asynchronous +// (it awaits the site settings), so on a cold load it can finish LAST and +// overwrite a good title with the generic one. These pin that ordering. + +import { describe, it, expect, beforeEach, vi } from 'vitest' + +// The real getFacilityName caches, so only the first call is slow and every +// later one is instant. Mirror that: pending calls are held until the test +// releases them, and the value is served synchronously from then on. A mock +// that hands out a fresh unresolved promise per call hangs the moment a test +// navigates twice. +let pending = [] +let settled = null + +function resolveFacility(value) { + settled = value + const waiting = pending + pending = [] + waiting.forEach((resolve) => resolve(value)) +} + +vi.mock('@/utils/siteSettings', () => ({ + getFacilityName: () => (settled !== null + ? Promise.resolve(settled) + : new Promise((resolve) => { pending.push(resolve) })), +})) + +import { + beginNavigation, applyGenericTitle, setRecordTitle, _reset, +} from './pageTitle' + +beforeEach(() => { + _reset() + document.title = '' + pending = [] + settled = null +}) + +describe('the generic title', () => { + it('names the section when no record has been set', async () => { + beginNavigation('/machines') + const done = applyGenericTitle('/machines', 'Machines') + resolveFacility('West Jefferson') + await done + + expect(document.title).toBe('West Jefferson ShopDB - Machines') + }) + + it('falls back to plain ShopDB when the facility is unset', async () => { + beginNavigation('/machines') + const done = applyGenericTitle('/machines', 'Machines') + resolveFacility('ShopDB') + await done + + expect(document.title).toBe('ShopDB - Machines') + }) + + it('drops the site name entirely on the dashboard', async () => { + beginNavigation('/') + const done = applyGenericTitle('/', 'Home') + resolveFacility('West Jefferson') + await done + + expect(document.title).toBe('West Jefferson ShopDB') + }) +}) + +describe('a record title', () => { + it('names the record once the view has loaded it', async () => { + beginNavigation('/machines/12') + const done = applyGenericTitle('/machines/12', 'Machines') + resolveFacility('West Jefferson') + await done + + setRecordTitle('CSF16') + + expect(document.title).toBe('West Jefferson ShopDB - CSF16') + }) + + it('SURVIVES a slow router that resolves afterwards', async () => { + // The regression this file exists for. The view wins the race, the + // settings fetch lands second, and the generic write must stand down. + beginNavigation('/machines/12') + const done = applyGenericTitle('/machines/12', 'Machines') + + setRecordTitle('CSF16') // view got there first + resolveFacility('West Jefferson') + await done // router finishes last + + expect(document.title).toBe('West Jefferson ShopDB - CSF16') + }) + + it('is ignored when the record has no usable name', async () => { + beginNavigation('/machines/12') + const done = applyGenericTitle('/machines/12', 'Machines') + resolveFacility('West Jefferson') + await done + + setRecordTitle('') + setRecordTitle(null) + setRecordTitle(undefined) + setRecordTitle(' ') + + // the section name is a better tab than a blank or a bare id + expect(document.title).toBe('West Jefferson ShopDB - Machines') + }) + + it('shortens a name too long for a tab, at a word boundary', async () => { + // Knowledge base articles are named with a sentence of description, and a + // browser truncates those mid-word wherever it happens to run out. + beginNavigation('/knowledgebase/2') + const done = applyGenericTitle('/knowledgebase/2', 'Knowledge Base') + resolveFacility('West Jefferson') + await done + + const full = 'Documentation on how to image a Standard or Business PC ' + + 'in GCC High using MediaCreator Lite' + setRecordTitle(full) + + expect(document.title.length).toBeLessThan(90) + expect(document.title.startsWith('West Jefferson ShopDB - ')).toBe(true) + expect(document.title.endsWith('...')).toBe(true) + + // what survives is a whole-word prefix of the original: the character + // following the cut in the source is a space, so no word was split + const kept = document.title + .replace('West Jefferson ShopDB - ', '') + .replace(/\.\.\.$/, '') + expect(full.startsWith(kept)).toBe(true) + expect(full[kept.length]).toBe(' ') + }) + + it('leaves a name that already fits alone', async () => { + beginNavigation('/machines/14') + const done = applyGenericTitle('/machines/14', 'Machines') + resolveFacility('West Jefferson') + await done + + setRecordTitle('0600') + + expect(document.title).toBe('West Jefferson ShopDB - 0600') + }) + + it('does not leak onto the next page', async () => { + beginNavigation('/machines/12') + const first = applyGenericTitle('/machines/12', 'Machines') + resolveFacility('West Jefferson') + await first + setRecordTitle('CSF16') + + beginNavigation('/printers') + const second = applyGenericTitle('/printers', 'Printers') + await second + + expect(document.title).toBe('West Jefferson ShopDB - Printers') + }) + + it('is discarded when the user navigates away mid-flight', async () => { + // Two navigations overlap; the first one's late generic write must not + // land on the second page. + beginNavigation('/machines/12') + const stale = applyGenericTitle('/machines/12', 'Machines') + + beginNavigation('/printers/3') + const fresh = applyGenericTitle('/printers/3', 'Printers') + setRecordTitle('WJPRT04') + + resolveFacility('West Jefferson') + await Promise.all([stale, fresh]) + + expect(document.title).toBe('West Jefferson ShopDB - WJPRT04') + }) +}) diff --git a/frontend/src/composables/systemSettings.js b/frontend/src/composables/systemSettings.js index 4f9fa842..69b090a9 100644 --- a/frontend/src/composables/systemSettings.js +++ b/frontend/src/composables/systemSettings.js @@ -45,6 +45,7 @@ export const assetTypeCols = [ export const searchDomains = [ { key: 'application', label: 'Applications' }, { key: 'knowledgebase', label: 'Knowledge Base' }, + { key: 'guide', label: 'Guides' }, { key: 'employee', label: 'Employees' }, { key: 'machine', label: 'Machines' }, { key: 'computer', label: 'PCs' }, diff --git a/frontend/src/router/index.js b/frontend/src/router/index.js index f01a802c..37562394 100644 --- a/frontend/src/router/index.js +++ b/frontend/src/router/index.js @@ -6,7 +6,7 @@ import { setupComplete, setupSkipped, isSetupLoaded, refreshSetupState, needsAdmin, isNeedsAdminLoaded, refreshNeedsAdmin } from '../composables/setupState' import { loadEnabledPlugins, isPluginEnabled } from '../composables/enabledPlugins' import { useToast } from '../composables/toast' -import { getFacilityName } from '../utils/siteSettings' +import { beginNavigation, applyGenericTitle } from '../composables/pageTitle' import { stagedChildren, stagedToplevel } from './routes.gen' // Auto-discover all route modules from routes/ directory. A module's default @@ -147,14 +147,12 @@ function pageTitleFor(route) { word.charAt(0).toUpperCase() + word.slice(1)).join(' ') } -router.afterEach(async (to) => { - let siteTitle = 'ShopDB' - try { - const facility = await getFacilityName() - if (facility && facility !== 'ShopDB') siteTitle = `${facility} ShopDB` - } catch (titleError) { /* settings unavailable - plain ShopDB */ } - const page = pageTitleFor(to) - document.title = page && page !== 'Home' ? `${siteTitle} - ${page}` : siteTitle +router.afterEach((to) => { + // Claim the route BEFORE awaiting anything: a detail view that resolves + // quickly sets the record title, and the generic write below must not + // overwrite it. See composables/pageTitle.js. + beginNavigation(to.fullPath) + applyGenericTitle(to.fullPath, pageTitleFor(to)) }) // A deploy replaces the hashed chunk files, so a tab that was open across it diff --git a/frontend/src/utils/siteSettings.js b/frontend/src/utils/siteSettings.js index c7ec67ef..a333a950 100644 --- a/frontend/src/utils/siteSettings.js +++ b/frontend/src/utils/siteSettings.js @@ -98,22 +98,20 @@ function darkenHexColor(hex, fraction) { // ServiceNow ticket-link config. Returns the enabled flag, incident/change URL // templates, and the global-search URL template (all with a {ticket} -// placeholder). Defaults mirror the previously-hardcoded GE ServiceNow URLs. +// placeholder). +// +// The defaults are BLANK and the integration ships OFF, matching the server. +// They used to be one organisation's QA ServiceNow tenant, so any site that had +// not configured ServiceNow still rendered ticket links - pointing its +// technicians at a tenant they cannot reach and, for an adopting site, at +// somebody else's. A blank URL renders tickets as plain text, which is the +// correct behaviour for "not configured". export async function getServicenowUrls() { - const enabledRaw = await getSetting('servicenow_enabled', 'true') + const enabledRaw = await getSetting('servicenow_enabled', 'false') const enabled = enabledRaw !== 'false' && enabledRaw !== '0' && enabledRaw !== false - const incidentUrl = await getSetting( - 'servicenow_incident_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) - const changeUrl = await getSetting( - 'servicenow_change_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) - const searchUrl = await getSetting( - 'servicenow_search_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) + const incidentUrl = await getSetting('servicenow_incident_url', '') + const changeUrl = await getSetting('servicenow_change_url', '') + const searchUrl = await getSetting('servicenow_search_url', '') return { enabled, incidentUrl, changeUrl, searchUrl } } diff --git a/frontend/src/views/SearchResults.vue b/frontend/src/views/SearchResults.vue index 80d262f8..3e3d4b41 100644 --- a/frontend/src/views/SearchResults.vue +++ b/frontend/src/views/SearchResults.vue @@ -35,6 +35,18 @@
Searching...
diff --git a/plugins/applications/frontend/views/ApplicationDetail.vue b/plugins/applications/frontend/views/ApplicationDetail.vue index 88fb5734..2f31b286 100644 --- a/plugins/applications/frontend/views/ApplicationDetail.vue +++ b/plugins/applications/frontend/views/ApplicationDetail.vue @@ -168,6 +168,7 @@ import { applicationsApi } from '@/api' import { getContactEmailDomain } from '@/utils/siteSettings' import { sanitizeNotesHtml } from '@/utils/sanitizeHtml' import { fileHref } from '@/utils/basePath' +import { setRecordTitle } from '@/composables/pageTitle' const route = useRoute() @@ -202,6 +203,7 @@ onMounted(async () => { // Load application details const response = await applicationsApi.get(route.params.id) app.value = response.data.data + setRecordTitle(app.value?.appname) // Load versions try { diff --git a/plugins/backups/migrations/versions/0002_backups_lastseenat.py b/plugins/backups/migrations/versions/0002_backups_lastseenat.py index b4f5c70a..e71501a8 100644 --- a/plugins/backups/migrations/versions/0002_backups_lastseenat.py +++ b/plugins/backups/migrations/versions/0002_backups_lastseenat.py @@ -27,8 +27,10 @@ depends_on = None def upgrade(): - columns = {c['name'] for c in - sa.inspect(op.get_bind()).get_columns('backuprevisions')} + inspector = sa.inspect(op.get_bind()) + if 'backuprevisions' not in inspector.get_table_names(): + return + columns = {c['name'] for c in inspector.get_columns('backuprevisions')} if 'lastseenat' not in columns: op.add_column('backuprevisions', sa.Column('lastseenat', sa.DateTime(), nullable=True)) @@ -37,7 +39,9 @@ def upgrade(): def downgrade(): - columns = {c['name'] for c in - sa.inspect(op.get_bind()).get_columns('backuprevisions')} + inspector = sa.inspect(op.get_bind()) + if 'backuprevisions' not in inspector.get_table_names(): + return + columns = {c['name'] for c in inspector.get_columns('backuprevisions')} if 'lastseenat' in columns: op.drop_column('backuprevisions', 'lastseenat') diff --git a/plugins/backups/plugin.py b/plugins/backups/plugin.py index b2e59cc4..cb6f6575 100644 --- a/plugins/backups/plugin.py +++ b/plugins/backups/plugin.py @@ -410,7 +410,13 @@ class BackupsPlugin(BasePlugin): # with_for_update serializes concurrent posts for the same chain: the # fleet collects on one GE-Enforce cycle, so a retry could otherwise # pass the hash check twice and insert the same revision. - sourcehostname = payload.get('sourcehostname') + # Normalize ONCE, and use the same value to query and to store. + # samesource() maps '' to IS NULL, but the insert below used to store + # the raw '' - so a client posting an empty hostname wrote rows that its + # own next post could never match. Every post then deduped against + # nothing, inserting forever: unbounded growth, and a history no + # technician can read. Whitespace does the same thing more quietly. + sourcehostname = (payload.get('sourcehostname') or '').strip() or None latest = (db.session.query(BackupRevision) .filter(BackupRevision.assetid == assetid, BackupRevision.backupkind == kindkey, @@ -476,7 +482,7 @@ class BackupsPlugin(BasePlugin): sharepath=sharepath, sourcefilename=sourcefilename, bytesize=bytesize, - sourcehostname=payload.get('sourcehostname'), + sourcehostname=sourcehostname, collectedat=collectedat or datetime.utcnow(), lastseenat=datetime.utcnow(), ) diff --git a/plugins/backups/services/ntlars.py b/plugins/backups/services/ntlars.py index 6e65ff9d..d0d992c4 100644 --- a/plugins/backups/services/ntlars.py +++ b/plugins/backups/services/ntlars.py @@ -32,6 +32,31 @@ import re SCHEMA = 'ntlars/1' +# SECRETS ARE NOT BACKED UP. +# +# The DNC registry carries an FTP password in cleartext. Stored verbatim it +# reached three places it must not: the backuprevisions table, the revision +# panel (which renders values), and the download route (which hands back a +# ready-to-import .reg). Anyone with backups.view could read the fleet's FTP +# password off a web page. +# +# Redaction happens at PARSE, so the secret never enters the database at all. +# Redacting only on display would leave it at rest and one query away. +# +# What is deliberately NOT done: dropping the value silently. This module's +# parser refuses to discard an unparseable value precisely because a backup that +# looks complete and restores a machine with a setting missing is the worst +# outcome. A redacted value keeps its NAME and records itself in +# `redacted`, so both the panel and a restore can say plainly that this one +# field must be set by hand. +REDACTEDMARKER = '***REDACTED***' +SECRETNAMEPATTERN = re.compile(r'passw|pwd|secret|apikey|token', re.IGNORECASE) + + +def issecretname(name): + """Whether a registry value name holds a credential.""" + return bool(SECRETNAMEPATTERN.search(name or '')) + REGROOT = 'HKEY_LOCAL_MACHINE' DNCPATH = r'SOFTWARE\GE Aircraft Engines\DNC' DNCPATHWOW = r'SOFTWARE\WOW6432Node\GE Aircraft Engines\DNC' @@ -245,6 +270,20 @@ def parse(raw): dialect = 'wow6432node' if sawwow else ('ntlars' if sawplain else 'unknown') + # Strip credentials before the projection is returned - i.e. before anything + # persists it. Recorded by path\name so the omission is visible rather than + # discovered during a restore. + redacted = [] + for path in keys: + for name, value in keys[path].items(): + if not issecretname(name): + continue + if value.get('data') in ('', None): + continue # nothing to hide + keys[path][name] = {'type': value.get('type', 'REG_SZ'), + 'data': REDACTEDMARKER} + redacted.append('{}\\{}'.format(path, name) if path else name) + return { 'schema': SCHEMA, 'sourcedialect': dialect, @@ -252,6 +291,7 @@ def parse(raw): {'path': path, 'values': dict(sorted(keys[path].items()))} for path in sorted(keys) ], + 'redacted': sorted(redacted), } @@ -291,6 +331,12 @@ def render(projection, dialect='ntlars', comments=None): dialect='ntlars' omits WOW6432Node - use with the NTLARS Load... button dialect='wow6432node' includes it - use with `reg import` on 64-bit + + Redacted credentials are rendered as a comment naming the value, NOT as the + marker string. Importing '***REDACTED***' as the FTP password would leave a + machine that looks restored and cannot connect, and the technician would be + debugging the network. A commented-out value fails loudly at the one moment + somebody is looking at the file. """ if dialect not in ('ntlars', 'wow6432node'): raise ValueError('unknown dialect: {!r}'.format(dialect)) @@ -302,10 +348,28 @@ def render(projection, dialect='ntlars', comments=None): if comments: out.append('') + redacted = projection.get('redacted') or [] + if redacted: + out.append('; ---------------------------------------------------------------') + out.append('; {} credential value(s) were NOT backed up and are NOT in this' + .format(len(redacted))) + out.append('; file. Set them by hand after importing:') + for name in redacted: + out.append('; {}'.format(name)) + out.append('; ---------------------------------------------------------------') + out.append('') + for key in projection.get('keys', []): path = key.get('path', '') out.append('[{}]'.format(root + ('\\' + path if path else ''))) for name, entry in (key.get('values') or {}).items(): + # A redacted credential is commented out, never emitted as the + # marker. Importing the marker as a password produces a machine that + # looks restored and cannot log in. + if (entry or {}).get('data') == REDACTEDMARKER: + out.append('; "{}" was not backed up - set it by hand'.format( + _escape(name))) + continue out.append(_rendervalue(name, entry)) out.append('') diff --git a/plugins/computers/frontend/routes.js b/plugins/computers/frontend/routes.js index cd724a09..828d9122 100644 --- a/plugins/computers/frontend/routes.js +++ b/plugins/computers/frontend/routes.js @@ -6,7 +6,7 @@ export default [ path: 'reports/pc-relationships', name: 'report-pc-relationships', component: () => import('./views/PCRelationshipsReport.vue'), - meta: { plugin: 'computers' } + meta: { plugin: 'computers', title: 'PC Relationships Report' } }, { path: 'settings/pctypemapping', diff --git a/plugins/computers/frontend/views/PCDetail.vue b/plugins/computers/frontend/views/PCDetail.vue index e1ca33da..0ceca6e7 100644 --- a/plugins/computers/frontend/views/PCDetail.vue +++ b/plugins/computers/frontend/views/PCDetail.vue @@ -5,6 +5,7 @@
Print Label Edit + Back to List
@@ -276,7 +277,7 @@ diff --git a/plugins/guides/frontend/routes.js b/plugins/guides/frontend/routes.js new file mode 100644 index 00000000..5958a0e3 --- /dev/null +++ b/plugins/guides/frontend/routes.js @@ -0,0 +1,42 @@ +/** + * Guides plugin routes. + * + * Three views, and the split between the last two is deliberate: reading a + * guide and editing one are different jobs with different rights, and the + * checkboxes are live in exactly one of them. + * + * meta.plugin = 'guides' so the ADR-009 router guard redirects when the backend + * plugin is disabled. Every route requires auth because every guides API route + * is behind JWT plus a guides.* permission. + */ +export default [ + { + path: 'guides', + name: 'guides', + component: () => import('./views/GuidesList.vue'), + meta: { requiresAuth: true, plugin: 'guides' } + }, + { + path: 'guides/new', + name: 'guide-new', + component: () => import('./views/GuideEditor.vue'), + meta: { requiresAuth: true, plugin: 'guides' } + }, + { + path: 'guides/:slug', + name: 'guide-view', + component: () => import('./views/GuideView.vue'), + meta: { requiresAuth: true, plugin: 'guides' } + }, + { + path: 'guides/:guideid/edit', + name: 'guide-edit', + component: () => import('./views/GuideEditor.vue'), + meta: { requiresAuth: true, plugin: 'guides' } + } +] + +// No full-screen routes: every view renders inside AppLayout. Declared so +// routes.gen.js can read `.toplevel` off the module without Rollup warning +// about a missing export. +export const toplevel = [] diff --git a/plugins/guides/frontend/utils/images.js b/plugins/guides/frontend/utils/images.js new file mode 100644 index 00000000..2241f18e --- /dev/null +++ b/plugins/guides/frontend/utils/images.js @@ -0,0 +1,77 @@ +/** + * Resolving guide images. + * + * A guide image lives behind an authenticated route, and an is a + * plain browser request that carries no Authorization header - this app keeps + * its JWT in one. Pointing src at the image route therefore yields 401 and a + * broken-image icon for every reader, which is how guides first shipped. + * + * So the bytes are fetched through the api client, which attaches the token and + * resolves against its own baseURL - the latter also fixing a stored + * '/api/guides/images/N' string under a subpath mount, where the API is not at + * /api at all. + * + * Used by the renderer AND the editor: an author needs to see the screenshot + * they just pasted as much as a reader does. + */ +import { reactive, watch, onBeforeUnmount } from 'vue' +import api from '@/api' + +/** The image id a block refers to, whichever way it was written. + * + * Blocks store imageid (written since this was fixed) or a url string (written + * before it). The url is never used as a src, only read for its id. + */ +export function imageIdOf (block) { + if (!block) return '' + if (block.imageid) return String(block.imageid) + const match = /\/guides\/images\/(\d+)/.exec(block.url || '') + return match ? match[1] : '' +} + +/** + * Keeps object URLs for the image blocks in `blocksRef`, fetching each id once + * however many blocks use it, and revoking them as blocks go away. + * + * Returns `state(block)` -> { src, failed }. + */ +export function useGuideImages (blocksRef) { + const images = reactive({}) + + async function load (id) { + if (images[id]) return + images[id] = { src: '', failed: false } + try { + const response = await api.get(`/guides/images/${id}`, { responseType: 'blob' }) + images[id].src = URL.createObjectURL(response.data) + } catch (err) { + images[id].failed = true + } + } + + function release (id) { + if (images[id] && images[id].src) URL.revokeObjectURL(images[id].src) + delete images[id] + } + + function state (block) { + const id = imageIdOf(block) + if (!id) return { src: '', failed: true } + return images[id] || { src: '', failed: false } + } + + // Editing replaces the block list on every keystroke, so resolve only ids + // that are new and keep the ones already held. Without that, every keystroke + // would refetch every image in the guide. + watch(blocksRef, blocks => { + const wanted = new Set( + (blocks || []).filter(b => b && b.type === 'image') + .map(imageIdOf).filter(Boolean)) + wanted.forEach(load) + Object.keys(images).forEach(id => { if (!wanted.has(id)) release(id) }) + }, { immediate: true, deep: true }) + + onBeforeUnmount(() => { Object.keys(images).forEach(release) }) + + return { state } +} diff --git a/plugins/guides/frontend/utils/inline.js b/plugins/guides/frontend/utils/inline.js new file mode 100644 index 00000000..f789ca6d --- /dev/null +++ b/plugins/guides/frontend/utils/inline.js @@ -0,0 +1,106 @@ +/** + * Inline text rendering for guide blocks. + * + * A guide stores its text as a RESTRICTED Markdown subset - bold, italic, + * inline code, links, line breaks - and nothing else. This turns that into + * HTML. + * + * ESCAPE FIRST, THEN FORMAT. The order is the whole security model: every + * angle bracket and ampersand becomes an entity before any pattern is applied, + * so no markup an author pasted can survive into the DOM. There is no path here + * that emits a tag the code below did not write itself. + * + * The restriction is not only about safety. Pasted rich text carries fonts and + * hardcoded colours, which then fight the theme - black text on a dark + * background, and nobody can say why. Emphasis is semantic; the stylesheet + * decides how it looks. That is what makes dark mode work without an author + * thinking about it, and why a guide written next year still matches one + * written today. + */ + +function escapeHtml (text) { + return String(text == null ? '' : text) + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') +} + +// The text was HTML-escaped before any pattern ran, so a captured URL arrives +// carrying entities: a real query string like ?a=1&b=2 reaches here as +// ?a=1&b=2. Escaping that again for the attribute produced &amp; and a +// link that opened the wrong record - the ServiceNow and SharePoint links that +// carry several parameters were all broken this way. Decode first, then let the +// single escape on output put back exactly one level. +function decodeEntities (text) { + return String(text) + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/&/g, '&') // last, so &lt; does not become < +} + +// Only http(s) and mailto. A javascript: or data: URL in a link is the one way +// an author could otherwise smuggle script past the escaping above. +function safeHref (url) { + const trimmed = String(url || '').trim() + if (/^(https?:|mailto:)/i.test(trimmed)) return trimmed + // In-app link. NOT '//host', which looks like a path and is read by the + // browser as protocol-relative - an off-site link wearing a local costume. + if (trimmed.startsWith('/') && !trimmed.startsWith('//')) return trimmed + return null +} + +export function renderInline (text) { + let html = escapeHtml(text) + + // Inline code first, so formatting characters inside `code` stay literal - + // a path like C:\**temp** should not turn half of itself bold. + const codeSpans = [] + html = html.replace(/`([^`]+)`/g, (match, code) => { + codeSpans.push(code) + return '\u0000CODE' + (codeSpans.length - 1) + '\u0000' + }) + + // [label](url). The URL pattern allows ONE level of balanced parentheses so + // real links survive - Wikipedia-style /wiki/Foo_(bar) is the common case, + // and a naive [^)]+ silently truncates it into a broken link. + // Finished links are stashed like code spans, so the emphasis passes below + // cannot reach inside an href. Without that, a URL containing *characters* + // has injected into the middle of the attribute and the link breaks. + const links = [] + html = html.replace(/\[([^\]]+)\]\(((?:[^()\s]|\([^()\s]*\))+)\)/g, (match, label, url) => { + const href = safeHref(decodeEntities(url)) + if (!href) return label + const external = /^https?:/i.test(href) + links.push('' + label + '') + return '\u0000LINK' + (links.length - 1) + '\u0000' + }) + + // **bold** then *italic*, longest marker first so **x** is not read as + // *(*x*)*. + html = html.replace(/\*\*([^*]+)\*\*/g, '$1') + html = html.replace(/(^|[^*])\*([^*]+)\*(?!\*)/g, '$1$2') + + html = html.replace(/\n/g, '
') + + html = html.replace(/\u0000LINK(\d+)\u0000/g, (match, index) => links[Number(index)]) + + html = html.replace(/\u0000CODE(\d+)\u0000/g, (match, index) => + '' + codeSpans[Number(index)] + '') + + return html +} + +/** Plain text, for titles and anywhere markup would be noise. */ +export function stripInline (text) { + return String(text == null ? '' : text) + .replace(/`([^`]+)`/g, '$1') + .replace(/\[([^\]]+)\]\([^)\s]+\)/g, '$1') + .replace(/\*\*([^*]+)\*\*/g, '$1') + .replace(/\*([^*]+)\*/g, '$1') +} diff --git a/plugins/guides/frontend/utils/inline.spec.js b/plugins/guides/frontend/utils/inline.spec.js new file mode 100644 index 00000000..012eb916 --- /dev/null +++ b/plugins/guides/frontend/utils/inline.spec.js @@ -0,0 +1,50 @@ +/** + * Inline rendering: the security model and the link bugs it hid. + * + * Escape-then-format is the whole defence, and it has one awkward consequence + * that produced real broken links: by the time a URL is captured it is already + * escaped, so it has to be decoded before it can be escaped again exactly once. + */ +import { describe, it, expect } from 'vitest' +import { renderInline } from './inline.js' + +describe('renderInline', () => { + it('escapes markup an author pasted', () => { + expect(renderInline('')) + .toBe('<img src=x onerror=alert(1)>') + }) + + it('keeps a multi-parameter URL intact', () => { + // Double-escaped, this became &amp; and the link opened the wrong + // record. Every ServiceNow and SharePoint link carries several parameters. + const html = renderInline('[ticket](https://x.example/nav?a=1&b=2&c=3)') + expect(html).toContain('href="https://x.example/nav?a=1&b=2&c=3"') + expect(html).not.toContain('&amp;') + }) + + it('does not let emphasis reach inside an href', () => { + const html = renderInline('[t](https://x.example/a*b*c)') + expect(html).toContain('href="https://x.example/a*b*c"') + expect(html).not.toContain('') + }) + + it('refuses a protocol-relative URL dressed as an in-app path', () => { + // '//evil.example' starts with '/' but the browser reads it as off-site. + expect(renderInline('[evil](//evil.example/x)')).toBe('evil') + }) + + it('refuses javascript: and keeps the label', () => { + expect(renderInline('[bad](javascript:alert(1))')).toBe('bad') + }) + + it('allows an in-app link', () => { + expect(renderInline('[map](/map)')).toBe('map') + }) + + it('still renders bold, italic and code', () => { + expect(renderInline('**b**')).toBe('b') + expect(renderInline('*i*')).toBe('i') + expect(renderInline('`C:\\**temp**`')) + .toBe('C:\\**temp**') + }) +}) diff --git a/plugins/guides/frontend/utils/proof.js b/plugins/guides/frontend/utils/proof.js new file mode 100644 index 00000000..5212ea3f --- /dev/null +++ b/plugins/guides/frontend/utils/proof.js @@ -0,0 +1,145 @@ +/** + * Proofing: the mistakes that matter in a PROCEDURE, not a document. + * + * A spellchecker catches "teh". None of these are spelling, and every one has + * been seen for real: + * + * - a smart quote in a command, so the command does not run + * - a checklist item with no text, so a step is silently missing + * - a heading with nothing under it, left when a draft was reorganised + * - an image with no alt text, invisible to anyone who cannot see it + * - a TODO that was never finished, published anyway + * + * Warnings, not blocks. A person publishing a guide at 14:40 with one empty + * bullet should be told, not stopped - the tool does not know that the bullet is + * deliberate, and a procedure that exists imperfectly beats one nobody wrote. + */ + +// Characters an editor or a word processor substitutes silently. In prose they +// are harmless; in a command they are the difference between working and a +// syntax error nobody can explain. +const SMARTCHARS = [ + // Written as escapes, not literals: the repo's style gate forbids non-ASCII in + // source, and a file whose job is detecting smart quotes cannot contain any. + // The escapes are also unambiguous in a diff, where a curly quote and a + // straight one look identical. + ['\u2018', "'"], ['\u2019', "'"], // single quotes + ['\u201C', '"'], ['\u201D', '"'], // double quotes + ['\u2013', '-'], ['\u2014', '-'], // en dash, em dash + ['\u00A0', ' '], // non-breaking space +] + +const UNFINISHED = /\b(TODO|TBD|FIXME|XXX|\?\?\?)\b/i + +function issue (level, blockid, message, fix) { + return { level, blockid, message, fix: fix || null } +} + +export function proof (guide, blocks) { + const issues = [] + + if (!guide.title || !guide.title.trim()) { + issues.push(issue('error', null, 'This guide has no title')) + } + if (!guide.summary || !guide.summary.trim()) { + issues.push(issue('warn', null, + 'No summary. It is what people read in the list and in search results.')) + } + if (!guide.tags || !guide.tags.trim()) { + issues.push(issue('warn', null, 'No tags, so this will be hard to find')) + } + if (!blocks.length) { + issues.push(issue('error', null, 'This guide is empty')) + return issues + } + + blocks.forEach((block, index) => { + const where = 'block ' + (index + 1) + ' (' + block.type + ')' + + if (block.type === 'code') { + // The important one. An autocorrected quote in a command looks right and + // does not run. + // + // Reported ONCE per block, however many offending characters it has: a + // command with an opening and a closing smart quote is one problem with + // one fix, and listing it twice makes the panel look like it is nagging. + const found = SMARTCHARS.filter(([bad]) => (block.text || '').includes(bad)) + if (found.length) { + issues.push(issue('error', block.id, + where + ': contains ' + found.map(([, good]) => 'a curly or long ' + + (good === "'" ? 'quote' : good === '"' ? 'double quote' : + good === '-' ? 'dash' : 'space')).filter( + (v, i, a) => a.indexOf(v) === i).join(' and ') + + ' where a command needs the plain character. Autocorrect does this, ' + + 'and the command will not run.', + { type: 'replaceAll', pairs: found })) + } + if (!(block.text || '').trim()) { + issues.push(issue('error', block.id, where + ' is empty')) + } + } else if (['heading', 'paragraph', 'callout'].includes(block.type)) { + if (!(block.text || '').trim() && !(block.title || '').trim()) { + issues.push(issue('warn', block.id, where + ' is empty')) + } + } + + if (block.type === 'step' && !(block.title || '').trim() + && !(block.text || '').trim()) { + issues.push(issue('warn', block.id, where + ' has no title and no text')) + } + + if (['checklist', 'bullets', 'numbers'].includes(block.type)) { + const empties = (block.items || []).filter( + item => !String(typeof item === 'string' ? item : item.text || '').trim()) + if (empties.length) { + issues.push(issue('warn', block.id, + where + ': ' + empties.length + ' empty item' + + (empties.length > 1 ? 's' : '') + + '. An empty step reads as a step somebody forgot to write.')) + } + } + + if (block.type === 'image' && !(block.alt || '').trim()) { + issues.push(issue('warn', block.id, + where + ': no alt text. Anyone who cannot see the image gets nothing.')) + } + + if (block.type === 'table' && !(block.rows || []).length) { + issues.push(issue('warn', block.id, where + ' has a header but no rows')) + } + + const text = [block.text, block.title].filter(Boolean).join(' ') + if (UNFINISHED.test(text)) { + issues.push(issue('warn', block.id, + where + ': still contains ' + text.match(UNFINISHED)[0])) + } + }) + + // A heading immediately followed by another heading usually means the section + // under the first one was moved or never written. + blocks.forEach((block, index) => { + if (block.type !== 'heading') return + const next = blocks[index + 1] + if (!next || next.type === 'heading') { + issues.push(issue('warn', block.id, + 'Heading "' + (block.text || '').slice(0, 40) + '" has nothing under it')) + } + }) + + return issues +} + +/** Apply a suggested fix in place, for the ones where the answer is obvious. */ +export function applyFix (block, fix) { + if (!fix) return false + const before = block.text || '' + let after = before + if (fix.type === 'replaceAll') { + for (const [bad, good] of fix.pairs) after = after.split(bad).join(good) + } else if (fix.type === 'replace') { + after = after.split(fix.from).join(fix.to) + } + if (after === before) return false + block.text = after + return true +} diff --git a/plugins/guides/frontend/utils/templates.js b/plugins/guides/frontend/utils/templates.js new file mode 100644 index 00000000..1a166f81 --- /dev/null +++ b/plugins/guides/frontend/utils/templates.js @@ -0,0 +1,74 @@ +/** + * Starting points for a new guide. + * + * The blank page is the real barrier to somebody writing a procedure down. A + * skeleton with the right SHAPE - what you need first, the steps, how you know + * it worked, what to do when it does not - gets a half-decent guide out of + * somebody who would otherwise have written nothing. + * + * The shape is lifted from the KB articles the team already writes, so guides + * written here read like the ones written before them. + */ + +function id (prefix, n) { + return prefix + n +} + +export const templates = [ + { + key: 'blank', + name: 'Blank', + description: 'Start with nothing', + blocks: () => [], + }, + { + key: 'procedure', + name: 'Procedure', + description: 'Before you start, numbered steps, how to check it worked', + blocks: () => [ + { id: id('t', 1), type: 'paragraph', + text: 'What this covers, and when somebody would need it.' }, + { id: id('t', 2), type: 'callout', variant: 'warning', title: 'Before you start', + text: 'Anything that has to be true first, or that will bite halfway through.' }, + { id: id('t', 3), type: 'heading', text: 'Steps' }, + { id: id('t', 4), type: 'step', title: 'First step', text: '' }, + { id: id('t', 5), type: 'step', title: 'Second step', text: '' }, + { id: id('t', 6), type: 'heading', text: 'Check it worked' }, + { id: id('t', 7), type: 'checklist', items: [ + { id: id('ti', 1), text: '' }, + { id: id('ti', 2), text: '' }, + ] }, + ], + }, + { + key: 'troubleshooting', + name: 'Troubleshooting', + description: 'Symptom, likely causes, what to check first', + blocks: () => [ + { id: id('t', 1), type: 'paragraph', text: 'The symptom, as somebody would describe it.' }, + { id: id('t', 2), type: 'heading', text: 'Check these first' }, + { id: id('t', 3), type: 'checklist', items: [ + { id: id('ti', 1), text: '' }, + { id: id('ti', 2), text: '' }, + ] }, + { id: id('t', 4), type: 'heading', text: 'Likely causes' }, + { id: id('t', 5), type: 'table', + header: ['What you see', 'What it usually means'], rows: [['', '']] }, + { id: id('t', 6), type: 'callout', variant: 'info', + text: 'The thing that looks like the cause and is not.' }, + ], + }, + { + key: 'checklist', + name: 'Checklist', + description: 'A list to work through, nothing else', + blocks: () => [ + { id: id('t', 1), type: 'paragraph', text: 'When to use this list.' }, + { id: id('t', 2), type: 'checklist', items: [ + { id: id('ti', 1), text: '' }, + { id: id('ti', 2), text: '' }, + { id: id('ti', 3), text: '' }, + ] }, + ], + }, +] diff --git a/plugins/guides/frontend/views/GuideEditor.vue b/plugins/guides/frontend/views/GuideEditor.vue new file mode 100644 index 00000000..5d5dd462 --- /dev/null +++ b/plugins/guides/frontend/views/GuideEditor.vue @@ -0,0 +1,717 @@ + + + + + diff --git a/plugins/guides/frontend/views/GuideView.vue b/plugins/guides/frontend/views/GuideView.vue new file mode 100644 index 00000000..8b32133a --- /dev/null +++ b/plugins/guides/frontend/views/GuideView.vue @@ -0,0 +1,273 @@ + + + + + + + diff --git a/plugins/guides/frontend/views/GuidesList.vue b/plugins/guides/frontend/views/GuidesList.vue new file mode 100644 index 00000000..052d9eea --- /dev/null +++ b/plugins/guides/frontend/views/GuidesList.vue @@ -0,0 +1,163 @@ + + + + + diff --git a/plugins/guides/migrations/env.py b/plugins/guides/migrations/env.py new file mode 100644 index 00000000..f3de904d --- /dev/null +++ b/plugins/guides/migrations/env.py @@ -0,0 +1,16 @@ +"""Alembic environment for the guides plugin migration chain. + +Delegates to the shared runner in shopdb.plugins.alembic_template, which filters +the metadata to this plugin's tables and drives Alembic against the per-plugin +version table alembic_version_guides. See ADR-008 for the ownership model. + +Like the backups plugin and unlike the cutover ones, guides is NEW: its 0001 +baseline really CREATES its tables, because the core chain never built them. +""" +import os + +os.environ['PLUGIN_NAME'] = 'guides' + +from shopdb.plugins.alembic_template import run_migrations # noqa: E402 + +run_migrations() diff --git a/plugins/guides/migrations/script.py.mako b/plugins/guides/migrations/script.py.mako new file mode 100644 index 00000000..2c015630 --- /dev/null +++ b/plugins/guides/migrations/script.py.mako @@ -0,0 +1,24 @@ +"""${message} + +Revision ID: ${up_revision} +Revises: ${down_revision | comma,n} +Create Date: ${create_date} + +""" +from alembic import op +import sqlalchemy as sa +${imports if imports else ""} + +# revision identifiers, used by Alembic. +revision = ${repr(up_revision)} +down_revision = ${repr(down_revision)} +branch_labels = ${repr(branch_labels)} +depends_on = ${repr(depends_on)} + + +def upgrade(): + ${upgrades if upgrades else "pass"} + + +def downgrade(): + ${downgrades if downgrades else "pass"} diff --git a/plugins/guides/migrations/versions/0001_guides_baseline.py b/plugins/guides/migrations/versions/0001_guides_baseline.py new file mode 100644 index 00000000..8964c3a7 --- /dev/null +++ b/plugins/guides/migrations/versions/0001_guides_baseline.py @@ -0,0 +1,114 @@ +"""guides plugin baseline: guides, guiderevisions, guideimages. + +A new plugin, so this builds its tables rather than stamping an anchor over +core-owned ones (contrast the cutover plugins, whose 0001 is a no-op). + +Idempotent: each table is created only if it is not already there, so a database +where an earlier run half-succeeded can be upgraded again without hand-editing +alembic_version_guides. +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy import inspect + + +revision = 'guides0001baseline' +down_revision = None +branch_labels = None +depends_on = None + + +def _tables(): + return set(inspect(op.get_bind()).get_table_names()) + + +def upgrade(): + existing = _tables() + + if 'guides' not in existing: + op.create_table( + 'guides', + sa.Column('guideid', sa.Integer(), nullable=False), + sa.Column('title', sa.String(length=300), nullable=False), + # 191 because this is UNIQUE-indexed and MySQL caps an index key at + # 767 bytes; utf8mb4 is 4 bytes per character. + sa.Column('slug', sa.String(length=191), nullable=False), + sa.Column('summary', sa.String(length=1000)), + sa.Column('tags', sa.String(length=500)), + sa.Column('status', sa.String(length=20), nullable=False, + server_default='draft'), + sa.Column('blocksjson', sa.dialects.mysql.MEDIUMTEXT() + if op.get_bind().dialect.name == 'mysql' else sa.Text(), + nullable=False), + sa.Column('publishedrevisionid', sa.Integer()), + sa.Column('createdby', sa.Integer()), + sa.Column('updatedby', sa.Integer()), + sa.Column('publishedat', sa.DateTime()), + # BaseModel contributes these to every model in the app; a plugin + # migration has to spell them out or the table is missing columns + # the ORM selects on every query. + sa.Column('createddate', sa.DateTime(), nullable=False), + sa.Column('modifieddate', sa.DateTime(), nullable=False), + sa.Column('isactive', sa.Boolean(), nullable=False), + sa.Column('revision', sa.Integer(), nullable=False, server_default='1'), + sa.PrimaryKeyConstraint('guideid'), + sa.UniqueConstraint('slug', name='uq_guides_slug'), + ) + op.create_index('idx_guides_status', 'guides', ['status']) + + if 'guiderevisions' not in existing: + op.create_table( + 'guiderevisions', + sa.Column('guiderevisionid', sa.Integer(), nullable=False), + sa.Column('guideid', sa.Integer(), nullable=False), + sa.Column('revision', sa.Integer(), nullable=False), + sa.Column('title', sa.String(length=300), nullable=False), + sa.Column('summary', sa.String(length=1000)), + sa.Column('tags', sa.String(length=500)), + sa.Column('blocksjson', sa.dialects.mysql.MEDIUMTEXT() + if op.get_bind().dialect.name == 'mysql' else sa.Text(), + nullable=False), + sa.Column('contenthash', sa.String(length=64), nullable=False), + sa.Column('note', sa.String(length=500)), + sa.Column('createdby', sa.Integer()), + # BaseModel contributes these to every model in the app; a plugin + # migration has to spell them out or the table is missing columns + # the ORM selects on every query. + sa.Column('createddate', sa.DateTime(), nullable=False), + sa.Column('modifieddate', sa.DateTime(), nullable=False), + sa.Column('isactive', sa.Boolean(), nullable=False), + sa.PrimaryKeyConstraint('guiderevisionid'), + sa.ForeignKeyConstraint(['guideid'], ['guides.guideid'], + ondelete='CASCADE'), + ) + op.create_index('idx_guiderevisions_guide', 'guiderevisions', ['guideid']) + op.create_index('idx_guiderevisions_hash', 'guiderevisions', ['contenthash']) + + if 'guideimages' not in existing: + op.create_table( + 'guideimages', + sa.Column('guideimageid', sa.Integer(), nullable=False), + sa.Column('guideid', sa.Integer()), + sa.Column('filename', sa.String(length=255)), + sa.Column('mimetype', sa.String(length=100), nullable=False), + sa.Column('bytesize', sa.Integer(), nullable=False), + sa.Column('data', sa.dialects.mysql.MEDIUMBLOB() + if op.get_bind().dialect.name == 'mysql' else sa.LargeBinary(), + nullable=False), + sa.Column('uploadedby', sa.Integer()), + # BaseModel contributes these to every model in the app; a plugin + # migration has to spell them out or the table is missing columns + # the ORM selects on every query. + sa.Column('createddate', sa.DateTime(), nullable=False), + sa.Column('modifieddate', sa.DateTime(), nullable=False), + sa.Column('isactive', sa.Boolean(), nullable=False), + sa.PrimaryKeyConstraint('guideimageid'), + sa.ForeignKeyConstraint(['guideid'], ['guides.guideid'], + ondelete='CASCADE'), + ) + op.create_index('idx_guideimages_guide', 'guideimages', ['guideid']) + + +def downgrade(): + for table in ('guideimages', 'guiderevisions', 'guides'): + op.drop_table(table) diff --git a/plugins/guides/models/__init__.py b/plugins/guides/models/__init__.py index c32b970e..02a44397 100644 --- a/plugins/guides/models/__init__.py +++ b/plugins/guides/models/__init__.py @@ -1,3 +1,3 @@ -from .guide import Guide, GuideImage, BLOCKTYPES +from .guide import Guide, GuideRevision, GuideImage, BLOCKTYPES -__all__ = ['Guide', 'GuideImage', 'BLOCKTYPES'] +__all__ = ['Guide', 'GuideRevision', 'GuideImage', 'BLOCKTYPES'] diff --git a/plugins/guides/models/guide.py b/plugins/guides/models/guide.py index 5df9ef4b..809445b8 100644 --- a/plugins/guides/models/guide.py +++ b/plugins/guides/models/guide.py @@ -33,8 +33,7 @@ without an author thinking about it. import json from datetime import datetime -from shopdb.api import db -from shopdb.core.models.base import BaseModel +from shopdb.api import db, BaseModel def _utcnow(): @@ -74,7 +73,12 @@ class Guide(BaseModel): # derived on read so an edited title cannot silently break a link somebody # pasted into a work order. title = db.Column(db.String(300), nullable=False) - slug = db.Column(db.String(300), nullable=False, unique=True, index=True) + # 191, not 300: this column is UNIQUE-indexed, and MySQL caps an index key + # at 767 bytes. utf8mb4 stores up to 4 bytes per character, so 191 * 4 = + # 764 is the longest indexable string - 300 produces + # "Specified key was too long" the moment the migration runs on a real + # database, having passed every SQLite test. + slug = db.Column(db.String(191), nullable=False, unique=True, index=True) # One line, shown in the list and in search results. summary = db.Column(db.String(1000)) @@ -93,15 +97,39 @@ class Guide(BaseModel): # the same reason the backups plugin stores its projection as text. blocksjson = db.Column(db.Text(length=16777215), nullable=False, default='[]') + # What READERS see. Points at an immutable snapshot in guiderevisions, not + # at the working copy above. + # + # This is the whole point of separating the two: without it, editing a live + # procedure changes it under a technician who is halfway through following + # it. With it, they keep reading the published version until somebody + # deliberately publishes a new one. NULL means never published - a draft + # nobody outside the authors can see. + # + # use_alter, because this and guiderevisions.guideid form a cycle: a guide + # points at its published revision, and every revision points back at its + # guide. Without it SQLAlchemy cannot order CREATE/DROP and warns that it + # cannot sort the tables. The database-level constraint is deliberately not + # created by the migration either - the cycle buys nothing there, and the + # pointer is maintained in one place (services/authoring.py). + publishedrevisionid = db.Column( + db.Integer, db.ForeignKey('guiderevisions.guiderevisionid', + ondelete='SET NULL', + use_alter=True, + name='fk_guides_publishedrevision')) + + # createddate / modifieddate / isactive come from BaseModel - no second set + # of timestamps on top of them, or the next person has to work out which + # pair is authoritative. publishedat is ours because "when did readers start + # seeing this" is a different fact from "when was the row last touched". createdby = db.Column(db.Integer, db.ForeignKey('users.userid')) updatedby = db.Column(db.Integer, db.ForeignKey('users.userid')) - createdat = db.Column(db.DateTime, nullable=False, default=_utcnow) - updatedat = db.Column(db.DateTime, nullable=False, default=_utcnow, - onupdate=_utcnow) publishedat = db.Column(db.DateTime) - # Bumped on every save. Cheap, and it gives a printed copy something - # honest to put in its footer: "version 7, printed 26 Aug 2026". + # The working copy's version number. Bumped only when a save actually + # changes something, so it counts real edits rather than keystrokes, and a + # printed copy has something honest for its footer: "version 7 (published), + # printed 26 Aug 2026". revision = db.Column(db.Integer, nullable=False, default=1) @property @@ -138,8 +166,10 @@ class Guide(BaseModel): 'tags': self.taglist(), 'status': self.status, 'revision': self.revision, - 'createdat': self.createdat.isoformat() + 'Z' if self.createdat else None, - 'updatedat': self.updatedat.isoformat() + 'Z' if self.updatedat else None, + # API names stay createdat/updatedat: the column names are a + # storage detail, and renaming a payload field breaks callers. + 'createdat': self.createddate.isoformat() + 'Z' if self.createddate else None, + 'updatedat': self.modifieddate.isoformat() + 'Z' if self.modifieddate else None, 'publishedat': (self.publishedat.isoformat() + 'Z' if self.publishedat else None), } @@ -148,6 +178,76 @@ class Guide(BaseModel): return data +class GuideRevision(BaseModel): + """An immutable snapshot of a guide, written when a save changes something. + + Full snapshots rather than diffs. A guide is small, and a snapshot answers + the question people actually ask - "what did this say in March, when the + job last ran" - without replaying a chain of deltas to find out. The same + reasoning the part-marker backups follow, for the same reason: the version + you need is the one nobody thought to keep. + + DEDUP: a save whose content hashes identically to the latest revision writes + nothing. Without that, autosave produces a hundred identical versions a day + and the history becomes noise that hides the three edits that mattered. + + Never updated after insert. A revision that can be edited is not a record of + anything. + """ + + __tablename__ = 'guiderevisions' + + guiderevisionid = db.Column(db.Integer, primary_key=True) + guideid = db.Column(db.Integer, + db.ForeignKey('guides.guideid', ondelete='CASCADE'), + nullable=False, index=True) + + # Monotonic per guide, and shown to people: "version 7". + revision = db.Column(db.Integer, nullable=False) + + # The whole guide as it stood, not just the blocks: a title or tag change is + # a change worth being able to see and roll back. + title = db.Column(db.String(300), nullable=False) + summary = db.Column(db.String(1000)) + tags = db.Column(db.String(500)) + blocksjson = db.Column(db.Text(length=16777215), nullable=False) + + # sha256 of the canonical content, and the dedup key. + contenthash = db.Column(db.String(64), nullable=False, index=True) + + # Optional "what changed and why", the way a commit message earns its keep. + note = db.Column(db.String(500)) + + createdby = db.Column(db.Integer, db.ForeignKey('users.userid')) + + @property + def blocks(self): + if not self.blocksjson: + return [] + try: + data = json.loads(self.blocksjson) + except (ValueError, TypeError): + return [] + return data if isinstance(data, list) else [] + + def to_dict(self, includeblocks=False): + data = { + 'guiderevisionid': self.guiderevisionid, + 'guideid': self.guideid, + 'revision': self.revision, + 'title': self.title, + 'summary': self.summary, + 'tags': [t.strip() for t in (self.tags or '').split(',') if t.strip()], + 'note': self.note, + 'createdby': self.createdby, + 'createdat': self.createddate.isoformat() + 'Z' if self.createddate else None, + 'shorthash': (self.contenthash or '')[:12], + } + if includeblocks: + data['blocks'] = self.blocks + return data + + class GuideImage(BaseModel): """An image pasted or dropped into a guide. @@ -174,7 +274,6 @@ class GuideImage(BaseModel): data = db.Column(db.LargeBinary(length=(2 ** 24) - 1), nullable=False) uploadedby = db.Column(db.Integer, db.ForeignKey('users.userid')) - createdat = db.Column(db.DateTime, nullable=False, default=_utcnow) def to_dict(self): return { @@ -184,5 +283,5 @@ class GuideImage(BaseModel): 'mimetype': self.mimetype, 'bytesize': self.bytesize, 'url': '/api/guides/images/{}'.format(self.guideimageid), - 'createdat': self.createdat.isoformat() + 'Z' if self.createdat else None, + 'createdat': self.createddate.isoformat() + 'Z' if self.createddate else None, } diff --git a/plugins/guides/plugin.py b/plugins/guides/plugin.py new file mode 100644 index 00000000..c8b0e387 --- /dev/null +++ b/plugins/guides/plugin.py @@ -0,0 +1,92 @@ +"""Guides plugin. + +Step-by-step procedures written and read inside ShopDB: images, working +checklists, dark mode and printable output, with drafts and revision history. + +Distinct from the knowledgebase plugin, which is a LINK REGISTRY - a +description, a URL and keywords pointing at a document held somewhere else. +This holds the document. They share a tag vocabulary on purpose, so one search +can cover both. +""" + +import json +import logging +from pathlib import Path +from typing import List, Dict, Optional, Type + +from flask import Flask, Blueprint + +from shopdb.plugins.base import BasePlugin, PluginMeta + +from .models import Guide, GuideRevision, GuideImage +from .api import guides_bp + +logger = logging.getLogger(__name__) + + +class GuidesPlugin(BasePlugin): + + def __init__(self): + self._manifest = self._load_manifest() + + def _load_manifest(self) -> Dict: + manifest_path = Path(__file__).parent / 'manifest.json' + if manifest_path.exists(): + with open(manifest_path, 'r') as fh: + return json.load(fh) + return {} + + @property + def meta(self) -> PluginMeta: + return PluginMeta( + name=self._manifest.get('name', 'guides'), + version=self._manifest.get('version', '1.0.0'), + description=self._manifest.get('description', 'Step-by-step guides'), + author=self._manifest.get('author', 'ShopDB Team'), + dependencies=self._manifest.get('dependencies', []), + core_version=self._manifest.get('core_version', '>=0.1.0,<1.0.0'), + api_prefix=self._manifest.get('api_prefix', '/api/guides'), + ) + + def get_blueprint(self) -> Optional[Blueprint]: + return guides_bp + + def get_models(self) -> List[Type]: + return [Guide, GuideRevision, GuideImage] + + def get_permissions(self) -> List: + """Five permissions, and the split between the last two is the point. + + publish is separate from edit so a technician can write up a procedure + while somebody else decides it is fit for the floor. Without that split, + anyone who can fix a typo can also put an unreviewed procedure in front + of an operator. + + create without edit is a working combination on purpose: an author may + edit their OWN guide while it is unpublished, and needs guides.edit once + other people are following it (see _mayedit in the API). + """ + return [ + ('guides.view', 'View guides', 'guides'), + ('guides.create', 'Create guides and edit your own drafts', 'guides'), + ('guides.edit', 'Edit any guide, including published ones', 'guides'), + ('guides.publish', 'Publish or withdraw a guide', 'guides'), + ('guides.delete', 'Delete guides', 'guides'), + ] + + def get_navigation_items(self) -> List[Dict]: + # Position 8, beside Slides (7), rather than down in the Information + # section with the reference lists. A guide is something you go and DO + # something with, like a slide deck you put on a screen - not a + # catalogue you look things up in. + return [ + { + 'name': 'Guides', + 'icon': 'book-open', + 'route': '/guides', + 'position': 8, + }, + ] + + def init_app(self, app: Flask, db_instance) -> None: + logger.info('Guides plugin initialized (v%s)', self.meta.version) diff --git a/plugins/guides/services/authoring.py b/plugins/guides/services/authoring.py new file mode 100644 index 00000000..1afd4389 --- /dev/null +++ b/plugins/guides/services/authoring.py @@ -0,0 +1,133 @@ +"""Saving, revisioning and publishing a guide. + +Kept out of the API layer so the rules are testable without a request, and so +there is exactly ONE path that writes a guide. A second write path is how a +system ends up with revisions that skipped the dedup, or a published pointer +that points at nothing. +""" + +from datetime import datetime + +from shopdb.api import db + +from ..models import Guide, GuideRevision +from .blocks import validate, contenthash + + +def _utcnow(): + return datetime.utcnow() + + +def save(guide, title=None, summary=None, tags=None, blocks=None, note=None, + userid=None): + """Apply an edit to the working copy, writing a revision if it changed. + + Returns (guide, revision_or_None). None means the save was a no-op: the + content hashed identically to the last revision, so nothing was recorded. + That is the normal outcome of an autosave where the author changed nothing, + and it is what keeps the history readable. + """ + if title is not None: + guide.title = title.strip() + if summary is not None: + guide.summary = summary.strip() or None + if tags is not None: + # Normalised once, here: trimmed, empties dropped, order preserved. + # Tags are compared case-insensitively when hashing, but stored as the + # author typed them - "PXE" should not become "pxe" on screen. + guide.tags = ', '.join(t.strip() for t in tags.split(',') if t.strip()) or None + if blocks is not None: + guide.blocks = validate(blocks) + + incoming = contenthash(guide.title, guide.summary, guide.tags, guide.blocks) + + latest = (GuideRevision.query + .filter_by(guideid=guide.guideid) + .order_by(GuideRevision.revision.desc()) + .first()) + if latest is not None and latest.contenthash == incoming: + # Nothing a reader would notice has changed. Touch nothing else: an + # updatedat that moves on every autosave makes "last edited" a lie. + return guide, None + + guide.revision = (latest.revision + 1) if latest else 1 + guide.updatedby = userid + + revision = GuideRevision( + guideid=guide.guideid, + revision=guide.revision, + title=guide.title, + summary=guide.summary, + tags=guide.tags, + blocksjson=guide.blocksjson, + contenthash=incoming, + note=(note or '').strip() or None, + createdby=userid, + ) + db.session.add(revision) + db.session.flush() + return guide, revision + + +def publish(guide, userid=None): + """Point readers at the current working copy. + + Publishing does NOT snapshot: it promotes the latest existing revision. If + it snapshotted, a publish with unsaved edits would create a version nobody + reviewed - which is exactly the accident this workflow exists to prevent. + """ + latest = (GuideRevision.query + .filter_by(guideid=guide.guideid) + .order_by(GuideRevision.revision.desc()) + .first()) + if latest is None: + raise ValueError('nothing to publish: this guide has no saved content') + + guide.publishedrevisionid = latest.guiderevisionid + guide.status = 'published' + guide.publishedat = _utcnow() + guide.updatedby = userid + db.session.flush() + return latest + + +def unpublish(guide, userid=None): + """Withdraw a guide from readers without deleting anything. + + For a procedure that turns out to be wrong. The revisions stay, so it can be + corrected and published again, and so there is still a record of what people + were being told in the meantime. + """ + guide.publishedrevisionid = None + guide.status = 'draft' + guide.updatedby = userid + db.session.flush() + return guide + + +def readable(guide): + """The revision a READER should be served, or None. + + Never the working copy: a published guide shows what was published, even + while somebody is editing the next version. Editors get the working copy + through the editor, which is a different route on purpose. + """ + if not guide.publishedrevisionid: + return None + return db.session.get(GuideRevision, guide.publishedrevisionid) + + +def restore(guide, revision, userid=None): + """Copy an old revision back onto the working copy. + + Deliberately NOT a rollback of history: restoring writes a NEW revision with + the old content. The record of what was published between then and now stays + intact, which is the difference between a history and a rewrite. + """ + return save(guide, + title=revision.title, + summary=revision.summary or '', + tags=revision.tags or '', + blocks=revision.blocks, + note='Restored from version {}'.format(revision.revision), + userid=userid) diff --git a/plugins/guides/services/blocks.py b/plugins/guides/services/blocks.py index ef07608b..08f06c0c 100644 --- a/plugins/guides/services/blocks.py +++ b/plugins/guides/services/blocks.py @@ -17,6 +17,7 @@ Storing text verbatim keeps what the author typed recoverable even if the rendering rules change later. """ +import hashlib import json from ..models import BLOCKTYPES @@ -129,3 +130,23 @@ def checklistitemcount(blocks): if isinstance(block, dict) and block.get('type') == 'checklist': total += len(block.get('items') or []) return total + + +def contenthash(title, summary, tags, blocks): + """sha256 over everything a reader would notice changing. + + Title, summary and tags are included, not just the blocks: renaming a guide + or retagging it is a change worth a revision, and hashing only the body + would silently drop it from the history. + + Canonical by construction - the blocks are re-serialized with sorted keys + and no whitespace, so the same content saved twice hashes the same however + the editor happened to order its JSON that day. + """ + canonical = json.dumps({ + 'title': (title or '').strip(), + 'summary': (summary or '').strip(), + 'tags': sorted(t.strip().lower() for t in (tags or '').split(',') if t.strip()), + 'blocks': blocks or [], + }, sort_keys=True, separators=(',', ':')) + return hashlib.sha256(canonical.encode('utf-8')).hexdigest() diff --git a/plugins/knowledgebase/frontend/views/KnowledgeBaseDetail.vue b/plugins/knowledgebase/frontend/views/KnowledgeBaseDetail.vue index 2d2d8b85..25364a19 100644 --- a/plugins/knowledgebase/frontend/views/KnowledgeBaseDetail.vue +++ b/plugins/knowledgebase/frontend/views/KnowledgeBaseDetail.vue @@ -1,212 +1,214 @@ - - - - - + + + + + diff --git a/plugins/machines/frontend/views/MachineDetail.vue b/plugins/machines/frontend/views/MachineDetail.vue index 4e7810c0..fb48ed49 100644 --- a/plugins/machines/frontend/views/MachineDetail.vue +++ b/plugins/machines/frontend/views/MachineDetail.vue @@ -12,6 +12,7 @@ Edit + Back to List @@ -248,7 +249,7 @@