diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f8ca72f..8b2050e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,45 @@ ADR-007 and ADR-002. ### Added +- **`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 + and out to every site that ran `seed catalog`. `dedupe` merges only + whitespace-and-punctuation twins, because those are one name typed two ways; + `Dell` and `Dell Inc.` normalise apart and are deliberately left alone, since + that is a naming choice only a person can make. The surviving row is the most + referenced, tie-broken toward the spaced spelling. `merge` makes that + judgment call explicitly and refuses to invent a target that does not exist. + `rename` fixes a stripped name with no twin, which neither of the others can + reach. All three report and change nothing unless `--apply` is passed, and + `dedupe` is idempotent. References are discovered from the live schema, not + listed in code: there are 15 foreign keys into these three tables, and + `machines.controllervendorid` is not one anybody would enumerate by hand. + +- **Secure Send: hand somebody a password without leaving it in a chat + window.** A link in Tech Tools that carries one credential, dies when it is + opened, and expires on its own if nobody opens it. The secret is encrypted in + the sender's browser and the key rides in the URL fragment, which browsers + never transmit, so the server stores ciphertext it cannot read and a + compromised database yields nothing. Nobody can recover a lost secret either, + including an administrator, which is the same property seen from the other + side. Redeeming is a POST because chat and mail clients prefetch links to + build previews, and a one-view secret consumed by a preview window is a secret + the recipient never sees; a GET reports availability only. Expiry defaults to + 24 hours (maximum a week) and views to one, whichever comes first, and a spent + row is deleted. Creating needs a login and `tools.secrets`; reading needs + neither, because the recipient usually has no account and the 128-bit token is + the authorisation. The 4096-character ciphertext limit is deliberately the + whole server-side enforcement: a zero-knowledge server cannot tell a password + from a document, so size is the only control it has, and raising it turns this + into the file transfer it was scoped not to be. Documented in + `docs/SECURE-SEND.md`. +- **A site can let people without an account create a Secure Send link.** + New `securesend_allow_guests` setting (Settings > Tools > Secure Send), off by + default because a guest secret has no creator and the audit trail can then say + when and from where but never who. Guest creation is rate limited per address + and the setting fails closed if it cannot be read, so an error can never be + what opens an unauthenticated write endpoint. - **The code generator can print a back side.** A badge card is two-sided and the generator only ever emitted one page per card. The Back side section turns on a second page per card carrying either the same code again, a fixed @@ -51,6 +90,48 @@ ADR-007 and ADR-002. ### Fixed +- **The shipped seed catalog contained duplicate vendors, models and operating + systems.** Four space-stripped twins, each with a correctly spaced original + already present, so a new site inherited the same company listed twice. + `OptiPlex5000` had no twin and was renamed to `OptiPlex 5000` rather than + dropped. Vendors 53 to 51, models 128 to 127, operating systems 14 to 12. + Not a code fault: the collector strips only leading and trailing whitespace + and treats the OS list as lookup-only, so it neither created these nor is + creating more. They came in with the classic-ASP import. + +- **A maintenance script could not find the database the site actually uses.** + `python scripts/export_catalog.py` on a deployed site died with 1045 "Access + denied for user 'root'@'localhost'", naming a user that site does not have. + Nothing loads `.env` for a plain script run: `wsgi.py` does it for the served + app and the `flask` CLI does it for itself, so both reach the real database, + while a bare `python scripts/...` gets neither and falls back to a hardcoded + `root@localhost` URL. The read-only failure is the harmless version - six of + the seven scripts write, and on a box where those fallback credentials happen + to resolve they would have silently rewritten the wrong database. All seven + now load `.env` from the repo root, resolved from the script's own location + rather than the working directory. + +- **A search result opened the list it sits in, not the record it found.** A + NOTICE linked to `/notifications`, so clicking the row search had already + identified sent the reader to the list page to find it again by hand. The id + was in the payload the whole time and only the URL ignored it. The subnet + searcher had the same defect: an IP search resolves which subnet an address + falls inside, then linked to the `/network` hub rather than that subnet. + Both now link to the record. Every other result type already carried its id. + +- **`flask plugin new` scaffolded a frontend that no lean build could prune.** + It wrote views to `frontend/src/views//` and a route file to + `frontend/src/router/routes/.js`, while all 16 bundled plugins keep + their frontend in `plugins//frontend/`, staged by + `stage-frontend.mjs`. Nothing looked broken, which is why it lasted: the + router still globs `./routes/*.js` during the Phase 4 dual-location + transition, so the pages loaded. What a new plugin author silently lost was + ADR-013 lean builds, since pruning works off the staged tree, plus stranded + views on deleting the plugin. The templates for the right layout already + existed; only the output paths were wrong. Their core imports moved to the + `@` alias, which resolves the same before and after staging, where the + previous `../../api` was correct only in the old location. + - **A zero bar height printed a barcode 176 wide and 0 high.** Nothing rejected it: the value went straight into a CSS length, `height: 0in` is perfectly valid, and an image at `height: 100%` of a zero-height box is zero pixels @@ -81,9 +162,62 @@ ADR-007 and ADR-002. the wrong code. The page index now travels with the page. - The back label's dotted preview outline outspecified the print reset and drew a dashed rule down the edge of every second card. +- **The warranty list could not be searched by machine number.** The number was + in the payload and rendered in the table as a chip, but the search box looked + only at vendor, service level, service tag and asset number. That made the + identifier the feature was built around - a shopfloor PC is warranted as a PC + but found by the machine it drives - the one thing the box could not find. + Machine number and machine name are now searched too, and the placeholder + names them. +- **`flask plugin upgrade-all` failed a plugin whose tables already existed** + ("table securesendsecrets already exists" on the tools plugin). Every + post-cutover baseline emitted a plain `op.create_table`, which is correct only + while a table's existence and its plugin's Alembic stamp cannot disagree. + They can: anything that builds tables from the models stamps nothing, and a + run that creates a table then fails before its stamp commits leaves the same + split, where re-running is the obvious response. Creates and indexes across + all nine table-creating plugin migrations now skip what is already there. + Indexes are matched on their COLUMNS rather than their name, because + `db.create_all()` names them by SQLAlchemy's convention and a migration by + this repo's, so a name-keyed check would silently add a duplicate index + instead of skipping. Note the remaining limit: this makes a baseline + idempotent, not a whole chain. A longer chain can still meet an `ALTER` whose + column is already present, which no guard on `create_table` addresses. +- **Saving a machine that has a controlling PC answered "this relationship + already exists" and refused the save.** Reported against a map position + change, but the map was incidental: the form re-POSTed the controlling-PC + link on every save, and the row was already there. It used "do I know the + existing link's id" as a stand-in for "is there an existing link", and read + that id from a key the API does not emit, so the two disagreed on any machine + that had a PC. The same conflation meant CLEARING a machine's controlling PC + silently did nothing, which was the more dangerous half and had not been + reported. The loaded PC is now what decides, and re-selecting the same PC is + a no-op rather than a delete-and-recreate. ### Changed +- **The computers collector no longer accepts `defaultprinter` or `printers`, + and can no longer write a printer assignment.** Those keys used to become + `defaultprinter`/`connectedto` relationship rows straight from a report, which + contradicts the rule the whole printer feature rests on: a collector report is + OBSERVED state and never ASSIGNED state, or a bay that installed the wrong + printer makes itself right simply by reporting it. It was not a dormant code + path - the collector schema advertised both keys and the example client in + `docs/COLLECTOR-INTEGRATION.md` set `defaultprinter`, while another page of + the same document promised nothing on that path writes an assignment row. + **A client still sending them keeps working**: the keys are ignored, not + rejected, and the response carries a warning naming them and pointing at + `POST /api/collector/printers` for reporting queues and + `seed-from-observed` for adopting them. Silently dropping them would have left + an integrator believing assignments were still being written. +- **Tech Tools no longer asks people to sign in for tools that need no + account.** `/tools`, `/tools/codes` and `/tools/secure-send` were all behind + `requiresAuth`. The label generator runs entirely in the browser - no API, no + data, nothing to protect - so gating it only ever cost a guest the use of it, + and gating the Secure Send page contradicted the guest setting that lets + somebody with no account create a link. The server still decides who may + create a secret: making a page reachable is not the same as allowing the + action, and the two were conflated. - **Settings on the left, labels on the right.** The controls were a narrow column with the preview trailing underneath, so the labels being adjusted were off the bottom of the screen. The preview is now a column of its own that diff --git a/plugins/backups/migrations/versions/0001_backups_baseline.py b/plugins/backups/migrations/versions/0001_backups_baseline.py index 885a3ca9..5af4fb47 100644 --- a/plugins/backups/migrations/versions/0001_backups_baseline.py +++ b/plugins/backups/migrations/versions/0001_backups_baseline.py @@ -12,6 +12,11 @@ time (NoReferencedTableError). Same shape autogenerate produces. Tables inherit the connection's default charset, matching how the core chain creates its own. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'backuprevisions', sa.Column('backuprevisionid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -42,15 +47,15 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('backuprevisionid'), ) - op.create_index('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) - op.create_index('ixbackuprevisionsbackupkind', 'backuprevisions', - ['backupkind']) - op.create_index('ixbackuprevisionscontenthash', 'backuprevisions', - ['contenthash']) + create_index_if_missing('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) + create_index_if_missing('ixbackuprevisionsbackupkind', 'backuprevisions', + ['backupkind']) + create_index_if_missing('ixbackuprevisionscontenthash', 'backuprevisions', + ['contenthash']) # The dedup read path is "latest revision for this asset+kind", so the # composite index is the one that actually gets used on every collector post. - op.create_index('ixbackuprevisionsassetkind', 'backuprevisions', - ['assetid', 'backupkind']) + create_index_if_missing('ixbackuprevisionsassetkind', 'backuprevisions', + ['assetid', 'backupkind']) def downgrade(): diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index 086836aa..b78866a9 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -16,16 +16,14 @@ from .api import computers_bp logger = logging.getLogger(__name__) -# Marker stamped on PC->printer links this collector creates. The stale-link -# archive only touches rows carrying this label, so manually-created printer -# relationships are never removed by a collector push. Stored in the -# assetrelationships.label column (no origin column exists; see BUILD notes). -PRINTER_LINK_ORIGIN = 'collector:printers' - # Marker stamped on PC->measuringtool "controls" links this collector creates -# for metrology PCs (CMM, Keyence, Genspect, wax-and-trace). Same stale-link -# discipline as PRINTER_LINK_ORIGIN: only rows carrying this label are archived -# by a collector push, so hand-made tool links survive. +# for metrology PCs (CMM, Keyence, Genspect, wax-and-trace). Only rows carrying +# this label are archived by a collector push, so hand-made tool links survive. +# Stored in assetrelationships.label (no origin column exists; see BUILD notes). +# +# There is deliberately no printer equivalent any more. PRINTER_LINK_ORIGIN went +# with _sync_printer_links: printer ASSIGNMENT is never collected, so there are +# no collector-owned printer rows left to label or archive. MEASURINGTOOL_LINK_ORIGIN = 'collector:measuringtool' # Marker stamped on the PC->machine "controls" link built from the reported @@ -363,26 +361,10 @@ class ComputersPlugin(BasePlugin): 'type': 'array', 'items': {'name': 'string', 'version': 'string'}, }, - # Printer identifiers reported by the GE-Enforce side, which - # runs Get-CimInstance Win32_Printer and marks the default with - # the Default flag. An identifier is the printer's windows name, - # share name, hostname, or port/IP; the collector resolves it - # flexibly to a printer asset. Both optional. Presence of either - # key drives the PC->printer relationship sync (and stale-link - # archive); absence leaves existing printer links untouched. - 'defaultprinter': { - 'type': 'string', - 'description': ('Default printer identifier (Win32_Printer ' - 'with Default=true): windows name, share ' - 'name, hostname, or port IP.'), - }, - 'printers': { - 'type': 'array', - 'items': {'type': 'string'}, - 'description': ('All installed network printer identifiers ' - '(Win32_Printer): windows name / share / ' - 'hostname / IP. Unresolved -> warning.'), - }, + # RETIRED: 'defaultprinter' and 'printers' used to live here and + # wrote PC->printer assignment rows from a report. That is the + # one thing this path must never do - see _RETIRED_PRINTER_KEYS. + # A client still sending them is warned, not rejected. 'accessprotocols': { 'type': 'array', 'items': {'type': 'string'}, @@ -647,8 +629,9 @@ class ComputersPlugin(BasePlugin): else: machinelinks = [] - # Printer relationship sync (only when the payload carried printer data). - printerlinks = self._sync_printer_links(comp.asset, payload, warnings) + # Printer assignment is NOT collected. See _warn_retired_printer_keys. + self._warn_retired_printer_keys(payload, warnings) + printerlinks = [] # Measuring-tool sync: metrology PCs (CMM/Keyence/Genspect/wax-trace) # get an attached MeasuringTool asset auto-created and linked. @@ -742,134 +725,38 @@ class ComputersPlugin(BasePlugin): AccessProtocol.protocolid.in_(wanted)).all()] if wanted else [] return sorted(names) - def _sync_printer_links(self, pcasset, payload, warnings): - """Idempotently sync PC->printer relationships from collector printer data. + def _warn_retired_printer_keys(self, payload, warnings): + """Refuse to turn a report into an assignment, out loud. - Default printer -> 'defaultprinter' (directional). Other reported - printers -> 'connectedto' (symmetric). Resolves each identifier to a - printer asset by windows name / share / hostname / asset number / name - or a communications IP. Unresolved identifiers add a warning and never - fail the push. + RETIRED 2026-08-24. This used to be _sync_printer_links, which wrote + 'defaultprinter' and 'connectedto' rows straight from a collector + payload. That contradicted the rule the whole printer feature rests on: + ASSIGNED and OBSERVED never mix. A bay that installed the wrong printer + would have made itself right simply by reporting it, and enforcement + would stop meaning anything. - Stale-link archive: on each push, collector-tagged links (label == - PRINTER_LINK_ORIGIN) whose (target, type) pair is not in the reported - desired set are set inactive. Only tagged rows are touched, so manual - links survive. Runs only when the payload carried a printer key - ('defaultprinter' or 'printers'); a PC that reports without printer data - keeps its existing links. Returns the desired-link list (created + kept). + Worse than dormant, it was ADVERTISED: the collector schema listed both + keys and COLLECTOR-INTEGRATION.md's example client set defaultprinter, + while the same document promised on another page that nothing on this + path writes an assignment row. The document contradicted itself and the + code sided with the wrong half. + + Observed queues have their own path (POST /api/collector/printers) and + become assignments only when a person asks, through + seed-from-observed after reviewing the comparison. + + WARNS RATHER THAN REJECTS. Unknown keys are not validated here, so a + client written against the old docs keeps pushing successfully; only its + printer keys stop doing anything. Silently ignoring them would leave an + integrator believing assignments were being written. """ - from shopdb.api import ( - AssetRelationship, RelationshipType, Asset, Communication) - - has_default = 'defaultprinter' in payload - has_list = 'printers' in payload - if not has_default and not has_list: - return [] - - try: - from plugins.printers.models import Printer - except ImportError: - warnings.append('printers plugin unavailable; printer links skipped') - return [] - - dp_type = RelationshipType.query.filter_by( - relationshiptype='defaultprinter').first() - ct_type = RelationshipType.query.filter_by( - relationshiptype='connectedto').first() - if not dp_type or not ct_type: - warnings.append('printer relationship types missing; ' - 'run flask seed reference-data') - return [] - - def resolve(identifier): - # first match wins: printer text identity, then a printer IP. - ident = (identifier or '').strip() - if not ident: - return None - printer = ( - Printer.query.join(Asset, Asset.assetid == Printer.assetid) - .filter(Asset.isactive == True) - .filter(db.or_( - Printer.windowsname.ilike(ident), - Printer.hostname.ilike(ident), - Printer.sharename.ilike(ident), - Asset.assetnumber.ilike(ident), - Asset.name.ilike(ident), - )).first()) - if printer: - return printer.asset - comm = ( - db.session.query(Communication) - .join(Printer, Printer.assetid == Communication.assetid) - .filter(Communication.ipaddress == ident) - .first()) - if comm: - return db.session.get(Asset, comm.assetid) - return None - - pcid = pcasset.assetid - desired = set() # (targetassetid, relationshiptypeid) to keep - printerlinks = [] - - default_id = None - default_ident = (payload.get('defaultprinter') or '').strip() - if default_ident: - target = resolve(default_ident) - if target: - default_id = target.assetid - desired.add((default_id, dp_type.relationshiptypeid)) - self._sync_one(pcid, default_id, dp_type) - printerlinks.append({'assetid': default_id, - 'relationshiptype': 'defaultprinter'}) - else: - warnings.append(f'unresolved default printer: {default_ident}') - - for ident in payload.get('printers') or []: - target = resolve(ident) - if not target: - warnings.append(f'unresolved printer: {ident}') - continue - if target.assetid == default_id: - continue # already the default link - desired.add((target.assetid, ct_type.relationshiptypeid)) - self._sync_one(pcid, target.assetid, ct_type) - printerlinks.append({'assetid': target.assetid, - 'relationshiptype': 'connectedto'}) - - # Archive collector-tagged links no longer reported (manual links, with - # a NULL/other label, are never matched here). - collector_rows = AssetRelationship.query.filter( - AssetRelationship.sourceassetid == pcid, - AssetRelationship.relationshiptypeid.in_( - [dp_type.relationshiptypeid, ct_type.relationshiptypeid]), - AssetRelationship.isactive == True, - AssetRelationship.label == PRINTER_LINK_ORIGIN, - ).all() - for rel in collector_rows: - if (rel.targetassetid, rel.relationshiptypeid) not in desired: - rel.isactive = False - - return printerlinks - - def _sync_one(self, pcid, printerid, reltype): - """Reactivate or create one collector PC->printer link (idempotent).""" - from shopdb.api import AssetRelationship - existing = AssetRelationship.query.filter_by( - sourceassetid=pcid, targetassetid=printerid, - relationshiptypeid=reltype.relationshiptypeid).first() - if existing: - # do not re-stamp label: a pre-existing manual row stays manual. - if not existing.isactive: - existing.isactive = True - return existing - rel = AssetRelationship( - sourceassetid=pcid, targetassetid=printerid, - relationshiptypeid=reltype.relationshiptypeid, - label=PRINTER_LINK_ORIGIN) - db.session.add(rel) - return rel - - # -- measuring-tool sync ----------------------------------------------- + sent = [key for key in ('defaultprinter', 'printers') if key in payload] + if sent: + warnings.append( + '%s ignored: a collector report never sets printer assignment. ' + 'Report queues to POST /api/collector/printers, then adopt them ' + 'with POST /api/printers/assignments/seed-from-observed.' + % ' and '.join(sent)) def _sync_machine_link(self, comp, machinenumber, warnings, origin_label=MACHINE_LINK_ORIGIN, preview=False): diff --git a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py index 2c9831f5..e232aacb 100644 --- a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py +++ b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py @@ -8,6 +8,11 @@ tables are created parent-first. computertypeid / measuringtooltypeid / publishedby are SOFT references (plain columns, no FK) to optional plugins/core. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'manifestscopes', sa.Column('scopeid', sa.Integer(), nullable=False), sa.Column('scopename', sa.String(length=64), nullable=False), @@ -37,7 +42,7 @@ def upgrade(): sa.PrimaryKeyConstraint('scopeid'), sa.UniqueConstraint('scopename', 'phase', name='uq_scope_name_phase'), ) - op.create_table( + create_table_if_missing( 'manifestentries', sa.Column('entryid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -80,10 +85,10 @@ def upgrade(): sa.PrimaryKeyConstraint('entryid'), sa.UniqueConstraint('scopeid', 'name', name='uq_entry_scope_name'), ) - op.create_index('idx_entry_scope', 'manifestentries', ['scopeid']) - op.create_index('idx_entry_scope_order', 'manifestentries', - ['scopeid', 'sortorder']) - op.create_table( + create_index_if_missing('idx_entry_scope', 'manifestentries', ['scopeid']) + create_index_if_missing('idx_entry_scope_order', 'manifestentries', + ['scopeid', 'sortorder']) + create_table_if_missing( 'manifestentrypctypes', sa.Column('entrypctypeid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -93,8 +98,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrypctypeid'), ) - op.create_index('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) - op.create_table( + create_index_if_missing('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) + create_table_if_missing( 'manifestentryhostnames', sa.Column('entryhostnameid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -104,8 +109,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entryhostnameid'), ) - op.create_index('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) - op.create_table( + create_index_if_missing('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) + create_table_if_missing( 'manifestentrymachinenumbers', sa.Column('entrymachinenumberid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -115,9 +120,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrymachinenumberid'), ) - op.create_index('idx_machinenumber_entry', 'manifestentrymachinenumbers', - ['entryid']) - op.create_table( + create_index_if_missing('idx_machinenumber_entry', 'manifestentrymachinenumbers', + ['entryid']) + create_table_if_missing( 'manifestinusechecks', sa.Column('inusecheckid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -127,7 +132,7 @@ def upgrade(): sa.PrimaryKeyConstraint('inusecheckid'), sa.UniqueConstraint('entryid'), ) - op.create_table( + create_table_if_missing( 'manifestinusecheckprocesses', sa.Column('inusecheckprocessid', sa.Integer(), nullable=False), sa.Column('inusecheckid', sa.Integer(), nullable=False), @@ -140,9 +145,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('inusecheckprocessid'), ) - op.create_index('idx_process_check', 'manifestinusecheckprocesses', - ['inusecheckid']) - op.create_table( + create_index_if_missing('idx_process_check', 'manifestinusecheckprocesses', + ['inusecheckid']) + create_table_if_missing( 'manifestpublishedversions', sa.Column('publishedversionid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -158,9 +163,9 @@ def upgrade(): sa.UniqueConstraint('scopeid', 'versionnumber', name='uq_published_scope_version'), ) - op.create_index('idx_published_scope', 'manifestpublishedversions', - ['scopeid']) - op.create_table( + create_index_if_missing('idx_published_scope', 'manifestpublishedversions', + ['scopeid']) + create_table_if_missing( 'manifestpayloads', sa.Column('payloadid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -173,8 +178,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('payloadid'), ) - op.create_index('idx_payload_entry', 'manifestpayloads', ['entryid']) - op.create_table( + create_index_if_missing('idx_payload_entry', 'manifestpayloads', ['entryid']) + create_table_if_missing( 'manifestenforcementreports', sa.Column('reportid', sa.Integer(), nullable=False), sa.Column('hostname', sa.String(length=100), nullable=False), @@ -192,13 +197,13 @@ def upgrade(): sa.Column('iscurrent', sa.Boolean(), nullable=False), sa.PrimaryKeyConstraint('reportid'), ) - op.create_index('idx_report_host', 'manifestenforcementreports', - ['hostname']) - op.create_index('idx_report_current', 'manifestenforcementreports', - ['iscurrent']) - op.create_index('idx_report_host_scope', 'manifestenforcementreports', - ['hostname', 'scopename', 'phase']) - op.create_table( + create_index_if_missing('idx_report_host', 'manifestenforcementreports', + ['hostname']) + create_index_if_missing('idx_report_current', 'manifestenforcementreports', + ['iscurrent']) + create_index_if_missing('idx_report_host_scope', 'manifestenforcementreports', + ['hostname', 'scopename', 'phase']) + create_table_if_missing( 'manifestenforcementresults', sa.Column('resultid', sa.Integer(), nullable=False), sa.Column('reportid', sa.Integer(), nullable=False), @@ -212,9 +217,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('resultid'), ) - op.create_index('idx_result_report', 'manifestenforcementresults', - ['reportid']) - op.create_table( + create_index_if_missing('idx_result_report', 'manifestenforcementresults', + ['reportid']) + create_table_if_missing( 'pctypealiases', sa.Column('aliasid', sa.Integer(), nullable=False), sa.Column('aliasgroup', sa.Integer(), nullable=False), @@ -223,7 +228,7 @@ def upgrade(): sa.UniqueConstraint('aliasgroup', 'aliasname', name='uq_alias_group_name'), ) - op.create_index('idx_alias_group', 'pctypealiases', ['aliasgroup']) + create_index_if_missing('idx_alias_group', 'pctypealiases', ['aliasgroup']) def downgrade(): diff --git a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py index d196a2ce..31081846 100644 --- a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py +++ b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py @@ -10,6 +10,11 @@ Create Date: 2026-07-21 """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -24,7 +29,7 @@ def upgrade(): insp = sa.inspect(bind) if 'manifestblobs' in insp.get_table_names(): return - op.create_table( + create_table_if_missing( 'manifestblobs', sa.Column('sha256', sa.String(length=64), nullable=False), sa.Column('filename', sa.String(length=255), nullable=False), diff --git a/plugins/machines/frontend/views/MachineForm.vue b/plugins/machines/frontend/views/MachineForm.vue index efe0846c..cf8c28f2 100644 --- a/plugins/machines/frontend/views/MachineForm.vue +++ b/plugins/machines/frontend/views/MachineForm.vue @@ -379,6 +379,7 @@ import PrinterAssignmentPicker from '@/components/PrinterAssignmentPicker.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' import { apiError } from '@/utils/apiError' +import { planControllingPcChange } from './controllingPc' const { isEnabled } = useIdentifierFlags() @@ -440,6 +441,9 @@ const relationshipTypes = ref([]) const controllingPcId = ref(null) const relationshipTypeId = ref(null) const existingRelationshipId = ref(null) +// Which PC the link named when the form loaded, so an unchanged one can be left +// alone. Editing a machine is mostly editing something else about it. +const loadedControllingPcId = ref(null) const currentMachine = ref(null) // Filter models by selected machine vendor @@ -575,7 +579,16 @@ onMounted(async () => { if (rel.sourceasset?.assettypename === 'computer' && (rel.relationshiptypename || '').toLowerCase() === 'controls') { controllingPcId.value = rel.sourceasset.assetid relationshipTypeId.value = rel.relationshiptypeid - existingRelationshipId.value = rel.assetrelationshipid + // relationshipid. `assetrelationshipid` is not a key the API has + // ever emitted - to_dict serialises the column names and the + // column is relationshipid - so this read undefined every time, + // and saveRelationship then took the "there is nothing to + // replace" path on a machine that had a controlling PC. Saving + // ANY change, a map position included, re-POSTed the link that + // was already there and the server correctly answered 409 "this + // relationship already exists". + existingRelationshipId.value = rel.relationshipid + loadedControllingPcId.value = rel.sourceasset.assetid break } } @@ -693,31 +706,40 @@ async function saveMachine() { } async function saveRelationship(assetId) { - // If no PC selected and no existing relationship, nothing to do - if (!controllingPcId.value && !existingRelationshipId.value) { + // The decision lives in controllingPc.js, where it is covered by tests. + // See that file for why "is there a link" and "do I know its id" are two + // different questions and what happened when they were treated as one. + const plan = planControllingPcChange({ + selected: controllingPcId.value, + loaded: loadedControllingPcId.value, + existingRelationshipId: existingRelationshipId.value, + }) + + if (plan.action === 'none') { return } - // If clearing the relationship - if (!controllingPcId.value && existingRelationshipId.value) { - await assetsApi.deleteRelationship(existingRelationshipId.value) + if (plan.deleteId !== null) { + await assetsApi.deleteRelationship(plan.deleteId) + existingRelationshipId.value = null + loadedControllingPcId.value = null + } + + if (plan.action === 'unlink') { return } - // If PC is selected - if (controllingPcId.value) { - // If there's an existing relationship, delete it first - if (existingRelationshipId.value) { - await assetsApi.deleteRelationship(existingRelationshipId.value) - } + // PC controls Machine, so the PC is the source and the machine the target. + const created = await assetsApi.createRelationship({ + sourceassetid: controllingPcId.value, + targetassetid: assetId, + relationshiptypeid: relationshipTypeId.value + }) - // Create new relationship (PC controls Machine, so PC is source, Machine is target) - await assetsApi.createRelationship({ - sourceassetid: controllingPcId.value, - targetassetid: assetId, - relationshiptypeid: relationshipTypeId.value - }) - } + // Keep the form honest about what is now stored, so a second save without a + // reload does not repeat the create it just did. + existingRelationshipId.value = created?.data?.data?.relationshipid ?? null + loadedControllingPcId.value = controllingPcId.value } diff --git a/plugins/machines/frontend/views/controllingPc.js b/plugins/machines/frontend/views/controllingPc.js new file mode 100644 index 00000000..1352a4e8 --- /dev/null +++ b/plugins/machines/frontend/views/controllingPc.js @@ -0,0 +1,68 @@ +// What saving the machine form should do about its controlling-PC link. +// +// Pulled out of MachineForm.vue because the component decided this inline and +// got it wrong in a way nothing could catch: it treated "I know the id of the +// existing link" as "there IS an existing link". The id was read from +// `rel.assetrelationshipid`, a key the API has never emitted - the column is +// `relationshipid` and to_dict serialises column names - so it was undefined on +// every load, and the two questions gave opposite answers on a machine that +// plainly had a controlling PC. +// +// Both failures followed from that single conflation: +// - Saving ANY change, a map position included, re-POSTed the link that was +// already there. The server answered 409 "this relationship already +// exists", correctly, and the form surfaced it as a failed save. +// - Clearing the controlling PC did nothing at all, because "no selection and +// no known id" reads as "nothing to do". +// +// So THE LOADED PC IS THE AUTHORITY on whether a link exists, and the +// relationship id is only ever used to delete a row. If the id goes missing +// again the worst case is a link that cannot be removed, never a duplicate +// create. + +/** + * Decide what to do with the controlling-PC link on save. + * + * @param {object} state + * @param {number|string|null} state.selected - PC id chosen in the form now. + * @param {number|string|null} state.loaded - PC id the link named on load. + * @param {number|null} state.existingRelationshipId - row id to delete, if known. + * @returns {{action: 'none'|'unlink'|'link'|'relink', deleteId: number|null}} + */ +export function planControllingPcChange({ selected, loaded, existingRelationshipId } = {}) { + const now = normalize(selected) + const before = normalize(loaded) + const deleteId = existingRelationshipId ?? null + + // Unchanged is not a change. Somebody moving a marker on the map is not + // asking to be re-linked to the same PC, and delete-then-create on an + // untouched link opens a window where the machine has no controlling PC - + // which resolve_asset_position walks across, so a failure between the two + // calls would lose the machine's inherited position along with the link. + if (now === before) { + return { action: 'none', deleteId: null } + } + + if (now === null) { + // Cleared. Nothing to delete means nothing to do, rather than an error: + // the link is already absent, which is the state being asked for. + return deleteId === null + ? { action: 'none', deleteId: null } + : { action: 'unlink', deleteId } + } + + if (before === null) { + return { action: 'link', deleteId: null } + } + + // Swapped to a different PC. The old row goes first: 'controls' is singular + // per source, not per target, so leaving it would leave the machine with two. + return { action: 'relink', deleteId } +} + +// Ids arrive as numbers from the API and strings from a hands back '42' while the relationship carried 42. Comparing + // those raw makes an untouched link look like a swap, which is the same + // duplicate POST by another route. + expect(planControllingPcChange({ + selected: '42', loaded: 42, existingRelationshipId: 7, + })).toEqual({ action: 'none', deleteId: null }) + }) + + it('treats an empty selection as cleared, not as a PC named ""', () => { + expect(planControllingPcChange({ + selected: '', loaded: 42, existingRelationshipId: 7, + })).toEqual({ action: 'unlink', deleteId: 7 }) + }) + + it('does nothing when there was no PC and none was chosen', () => { + expect(planControllingPcChange({ + selected: null, loaded: null, existingRelationshipId: null, + })).toEqual({ action: 'none', deleteId: null }) + }) + + it('does not attempt a delete it cannot perform', () => { + // Cleared, but the row id was never learned. Asking the API to delete + // `undefined` produced a request against /relationships/undefined. + expect(planControllingPcChange({ + selected: null, loaded: 42, existingRelationshipId: undefined, + })).toEqual({ action: 'none', deleteId: null }) + }) +}) diff --git a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py index 8b102464..15722d64 100644 --- a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py +++ b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py @@ -17,6 +17,11 @@ connection's default charset, so on a utf8mb4 database they are utf8mb4, matchin how the core chain creates its tables. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -28,7 +33,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'measuringtooltypes', sa.Column('measuringtooltypeid', sa.Integer(), nullable=False), sa.Column('name', sa.String(length=100), nullable=False), @@ -40,7 +45,7 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtooltypeid'), sa.UniqueConstraint('name'), ) - op.create_table( + create_table_if_missing( 'measuringtools', sa.Column('measuringtoolid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -59,8 +64,8 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtoolid'), sa.UniqueConstraint('assetid'), ) - op.create_index('idx_measuringtool_type', 'measuringtools', - ['measuringtooltypeid']) + create_index_if_missing('idx_measuringtool_type', 'measuringtools', + ['measuringtooltypeid']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py index f0bb3484..3d16b508 100644 --- a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py +++ b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py @@ -11,6 +11,11 @@ ops are written out explicitly anyway to match the measuringtools exemplar and keep the baseline reviewable. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -22,7 +27,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditems', sa.Column('printeditemid', sa.Integer(), nullable=False), sa.Column('itemcode', sa.String(length=20), nullable=True), @@ -39,9 +44,9 @@ def upgrade(): sa.PrimaryKeyConstraint('printeditemid'), sa.UniqueConstraint('itemcode'), ) - op.create_index('ix_printeditems_itemcode', 'printeditems', ['itemcode']) + create_index_if_missing('ix_printeditems_itemcode', 'printeditems', ['itemcode']) - op.create_table( + create_table_if_missing( 'printeditemtransactions', sa.Column('transactionid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -58,12 +63,12 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('transactionid'), ) - op.create_index('ix_printeditemtransactions_printeditemid', - 'printeditemtransactions', ['printeditemid']) - op.create_index('ix_printeditemtransactions_employeesso', - 'printeditemtransactions', ['employeesso']) - op.create_index('ix_printeditemtransactions_transactiondate', - 'printeditemtransactions', ['transactiondate']) + create_index_if_missing('ix_printeditemtransactions_printeditemid', + 'printeditemtransactions', ['printeditemid']) + create_index_if_missing('ix_printeditemtransactions_employeesso', + 'printeditemtransactions', ['employeesso']) + create_index_if_missing('ix_printeditemtransactions_transactiondate', + 'printeditemtransactions', ['transactiondate']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py index d0b2c9a9..0cfd8553 100644 --- a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py +++ b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py @@ -5,6 +5,11 @@ the ADR-008 payoff: the plugin evolves its own schema without touching the core chain. Applied by `flask plugin upgrade-all`. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -16,7 +21,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditemfiles', sa.Column('fileid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.PrimaryKeyConstraint('fileid'), sa.UniqueConstraint('storedfilename'), ) - op.create_index('ix_printeditemfiles_printeditemid', - 'printeditemfiles', ['printeditemid']) + create_index_if_missing('ix_printeditemfiles_printeditemid', + 'printeditemfiles', ['printeditemid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0002_printersupplyalerts.py b/plugins/printers/migrations/versions/0002_printersupplyalerts.py index e563e2ca..fe909c18 100644 --- a/plugins/printers/migrations/versions/0002_printersupplyalerts.py +++ b/plugins/printers/migrations/versions/0002_printersupplyalerts.py @@ -4,6 +4,11 @@ Stores the last tier alerted per (printer, supply) so the poller fires once on a downward crossing and re-arms after a refill. Idempotent create. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printersupplyalerts' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printersupplyalerts', sa.Column('printersupplyalertid', sa.Integer(), primary_key=True), sa.Column('printerid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.UniqueConstraint('printerid', 'supplykey', name='uq_printersupplyalert_printer_key'), ) - op.create_index('ix_printersupplyalerts_printerid', - 'printersupplyalerts', ['printerid']) + create_index_if_missing('ix_printersupplyalerts_printerid', + 'printersupplyalerts', ['printerid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0005_observedqueues.py b/plugins/printers/migrations/versions/0005_observedqueues.py index 8eeea6cd..0a92c9a9 100644 --- a/plugins/printers/migrations/versions/0005_observedqueues.py +++ b/plugins/printers/migrations/versions/0005_observedqueues.py @@ -21,6 +21,11 @@ Revision ID: printers0005observedqueues Revises: printers0004drivervendor """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -36,7 +41,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printerobservedqueues' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printerobservedqueues', sa.Column('printerobservedqueueid', sa.Integer(), nullable=False), # Nullable and no index of its own beyond the explicit one below: an @@ -67,10 +72,10 @@ def upgrade(): ) # Port address is the primary match key from an observed queue back to a # printer asset, so every comparison read hits it. - op.create_index('idx_printerobservedqueues_portaddress', - 'printerobservedqueues', ['portaddress']) - op.create_index('idx_printerobservedqueues_assetid', - 'printerobservedqueues', ['assetid']) + create_index_if_missing('idx_printerobservedqueues_portaddress', + 'printerobservedqueues', ['portaddress']) + create_index_if_missing('idx_printerobservedqueues_assetid', + 'printerobservedqueues', ['assetid']) def downgrade(): diff --git a/plugins/tools/api/routes.py b/plugins/tools/api/routes.py index f55dd620..9dd4231d 100644 --- a/plugins/tools/api/routes.py +++ b/plugins/tools/api/routes.py @@ -6,9 +6,15 @@ on first read needs somewhere to keep the ciphertext until then. TWO DIFFERENT DOORS, on purpose. -CREATE needs a login and a permission. Anonymous creation would be an -untraceable way to move data out of a system that otherwise records who did -what, and the creator is the only person these rows can ever name. +CREATE needs a LOGIN and nothing more. Sharing a password is something anybody +at the site may legitimately need to do, and a permission somebody has to be +granted first turns a two-minute job into a ticket - which is how people end up +pasting credentials into chat instead. + +The login stays, though, and is not a formality: anonymous creation would make +this an unattributable write endpoint that stores arbitrary ciphertext, and the +creator is the only person these rows can ever name. Everybody browsing this app +is already signed in, so requiring it costs nothing. READ needs neither, because the recipient usually has no account here and the link is normally handed over in a call or a chat. The unguessable token IS the @@ -27,12 +33,12 @@ import secrets as pysecrets from datetime import datetime, timedelta, timezone from flask import Blueprint, request -from flask_jwt_extended import jwt_required, get_jwt_identity +from flask import current_app +from flask_jwt_extended import jwt_required, get_jwt_identity, current_user from shopdb.api import ( - db, + db, cache, Setting, success_response, error_response, ErrorCodes, - require_permission, ) from ..models import SecureSecret @@ -50,18 +56,113 @@ MAX_CIPHERTEXT = 4096 DEFAULT_EXPIRY_HOURS = 24 MAX_EXPIRY_HOURS = 168 -MAX_VIEWS = 5 +# Generous, because "how many people need this password" is the site's business +# and not a number worth arguing with. The floor matters more than the ceiling: +# a secret readable an unlimited number of times is a password on a web page. +MAX_VIEWS = 100 + +# Guest creation, when a site enables it, is an unauthenticated write. Bounded +# per address so it cannot be used to fill a table or park data: a person +# sharing passwords does it a handful of times an hour, not hundreds. +GUEST_CREATE_MAX = 10 +GUEST_CREATE_WINDOW_SECONDS = 600 def _utcnow(): return datetime.now(timezone.utc).replace(tzinfo=None) +def _sweep_expired(): + """Delete secrets whose window has passed, whoever asked. + + Redeeming removes a row, and so does touching an expired one - but a secret + nobody ever opens was removed by NOTHING. The recipient goes on holiday, the + link is never clicked, and the ciphertext sits there for good: not a + disclosure risk, since it cannot be read, but it makes "nothing lingers" + untrue and grows the table in exactly the case where the feature failed. + + Swept opportunistically rather than on a schedule: creating a secret is + authenticated and infrequent, and an air-gapped site is not going to have a + cron job anybody remembers to add. + """ + stale = SecureSecret.query.filter(SecureSecret.expiresat < _utcnow()).all() + for secret in stale: + db.session.delete(secret) + return len(stale) + + +def _guests_allowed(): + """May somebody with no account create a secret? Default NO. + + Read straight from the row rather than the settings cache: this decides + whether an unauthenticated caller may write, and being wrong on the + permissive side for five minutes is the wrong way round to be wrong. + """ + try: + row = Setting.query.filter_by(key='securesend_allow_guests').first() + except Exception: + # FAIL CLOSED. If the setting cannot be read - the row is missing, the + # table is not there yet on a half-migrated site, the database is + # unhappy - the answer is no. An error must never be the thing that + # opens an unauthenticated write endpoint. + current_app.logger.exception('could not read securesend_allow_guests') + return False + return bool(row and str(row.value).strip().lower() in ('true', '1', 'yes')) + + +def _guest_ratelimited(): + """Bounded guest creation, per address, in a fixed window. + + Same shape as the login limiter. Not a security boundary - a determined + person has more addresses - but it stops an open endpoint being an easy + place to park data or fill a disk. + """ + address = (request.headers.get('X-Forwarded-For', '').split(',')[0].strip() + or request.remote_addr or 'unknown') + bucket = int(_utcnow().timestamp()) // GUEST_CREATE_WINDOW_SECONDS + key = f'securesendguest:{address}:{bucket}' + used = cache.get(key) or 0 + if used >= GUEST_CREATE_MAX: + return True + cache.set(key, used + 1, timeout=GUEST_CREATE_WINDOW_SECONDS) + return False + + @tools_bp.route('/secrets', methods=['POST']) -@jwt_required() -@require_permission('tools.secrets') +@jwt_required(optional=True) def create_secret(): - """Store an already-encrypted secret and return its token.""" + """Store an already-encrypted secret and return its token. + + OPTIONAL jwt so the SETTING decides rather than the decorator: a guest has + to reach the handler to be told whether this site lets them in. + """ + # WHO MAY CREATE. Signed in: the tools.secrets permission, with the usual + # admin bypass. Not signed in: only when the site has deliberately opened + # it, and then within a rate limit. + # get_jwt_identity(), NOT `current_user is None`: current_user is a + # LocalProxy that is never None itself, so that test silently took the + # signed-in branch for a guest and then blew up dereferencing it. + identity = get_jwt_identity() + if identity is None: + if not _guests_allowed(): + return error_response( + ErrorCodes.UNAUTHORIZED, + 'Sign in to create a secret link.', + http_code=401) + if _guest_ratelimited(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Too many links from this address. Try again shortly.', + http_code=429) + elif not current_user.haspermission('tools.secrets'): + return error_response( + ErrorCodes.FORBIDDEN, + 'You do not have permission to create secret links.', + http_code=403) + # Tidy up on the way past. Cheap, and it keeps the table bounded without a + # scheduler existing anywhere. + _sweep_expired() + data = request.get_json(silent=True) or {} ciphertext = (data.get('ciphertext') or '').strip() iv = (data.get('iv') or '').strip() @@ -173,4 +274,9 @@ def peek_secret(token): """ secret = SecureSecret.query.filter_by(token=(token or '').strip()).first() live = bool(secret and not secret.is_expired() and not secret.is_spent()) + if secret is not None and not live: + # A late click tidies its own row, so a dead secret does not need + # somebody to create a new one before it is cleaned up. + db.session.delete(secret) + db.session.commit() return success_response({'available': live}) diff --git a/plugins/tools/frontend/api.js b/plugins/tools/frontend/api.js new file mode 100644 index 00000000..5d5ed13c --- /dev/null +++ b/plugins/tools/frontend/api.js @@ -0,0 +1,23 @@ +// Tech Tools API client. +// +// Only secure send needs one - every other tool runs entirely in the browser +// and talks to nothing. +import api from '@/api' + +export const toolsApi = { + createSecret(payload) { + return api.post('/tools/secrets', payload) + }, + + // Is the link still live? Spends nothing, so it is safe for the read page to + // ask before anyone clicks. + peekSecret(token) { + return api.get(`/tools/secrets/${token}`) + }, + + // POST, not GET, and that is deliberate: chat and mail clients prefetch GET + // urls to build previews, and a preview must not consume a one-view secret. + redeemSecret(token) { + return api.post(`/tools/secrets/${token}`) + }, +} diff --git a/plugins/tools/frontend/routes.js b/plugins/tools/frontend/routes.js index e17ac47e..7e15889b 100644 --- a/plugins/tools/frontend/routes.js +++ b/plugins/tools/frontend/routes.js @@ -15,12 +15,48 @@ export default [ path: 'tools', name: 'tools', component: () => import('./views/ToolsIndex.vue'), - meta: { requiresAuth: true, plugin: 'tools' } + // NO requiresAuth. Every tool here is usable without an account: the label + // generator runs entirely in the browser, and secure send decides for + // itself (a site can let guests create, and receiving never needs a login). + // Gating the index turned a public tool into one nobody could reach. + meta: { plugin: 'tools' } }, { path: 'tools/codes', name: 'tools-codes', component: () => import('./views/CodeGenerator.vue'), - meta: { requiresAuth: true, plugin: 'tools' } + // Browser-only: no API, no data, nothing to protect. + meta: { plugin: 'tools' } + }, + { + path: 'settings/secure-send', + name: 'secure-send-settings', + component: () => import('./views/SecureSendSettings.vue'), + // Admin, like every other settings page: this decides whether an + // unauthenticated caller may write to the site. + meta: { requiresAuth: true, requiresAdmin: true, plugin: 'tools' } + }, + { + path: 'tools/secure-send', + name: 'tools-secure-send', + component: () => import('./views/SecureSendCreate.vue'), + // Reachable by anyone; the SERVER decides who may actually create. A signed + // in user needs tools.secrets, and a guest needs the site to have turned on + // securesend_allow_guests. The page reads that setting so a guest who + // cannot create is told so, instead of being handed a form that 401s. + meta: { plugin: 'tools' } + }, +] + +// PUBLIC and OUTSIDE AppLayout. The recipient of a secret usually has no account +// here, so this page carries no navigation and asks for no login - the +// unguessable token in the path, and the key in the fragment, are what let them +// read it. Short path because the whole link gets pasted into a chat message. +export const toplevel = [ + { + path: '/s/:token', + name: 'secure-send-read', + component: () => import('./views/SecureSendRead.vue'), + meta: { plugin: 'tools' } }, ] diff --git a/plugins/tools/frontend/securesend.js b/plugins/tools/frontend/securesend.js new file mode 100644 index 00000000..e2ced454 --- /dev/null +++ b/plugins/tools/frontend/securesend.js @@ -0,0 +1,136 @@ +// Client-side crypto for secure send. +// +// THE KEY NEVER LEAVES THIS FILE'S PROCESS. It is generated here, put in the +// URL fragment, and read back out of the fragment on the other side. Browsers +// do not transmit fragments, so it reaches the recipient's browser and nowhere +// else - not the server, not its logs, not a proxy in between. That is what +// makes the stored ciphertext useless to anyone who reads the database. +// +// AES-GCM because it authenticates as well as encrypts: a tampered ciphertext +// fails to decrypt rather than producing plausible rubbish. +// +// Web Crypto only, no dependency. It is available on every browser the fleet +// runs and requires a secure context, which is the right constraint anyway - +// the page and its scripts still cross the network even though the key does not. + +const ALGORITHM = 'AES-GCM' +const KEY_BITS = 256 +const IV_BYTES = 12 + +function toBase64(bytes) { + let binary = '' + for (const byte of new Uint8Array(bytes)) binary += String.fromCharCode(byte) + return btoa(binary) +} + +function fromBase64(text) { + const binary = atob(text) + const bytes = new Uint8Array(binary.length) + for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i) + return bytes +} + +// base64url for the fragment: '+' and '/' are legal in a fragment but survive +// copy-paste and link rewriting far less reliably than the url-safe alphabet. +function toBase64Url(bytes) { + return toBase64(bytes).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '') +} + +function fromBase64Url(text) { + const padded = text.replace(/-/g, '+').replace(/_/g, '/') + return fromBase64(padded + '='.repeat((4 - (padded.length % 4)) % 4)) +} + +export function cryptoAvailable() { + return typeof crypto !== 'undefined' && !!crypto.subtle +} + +/** Encrypt text. Returns the payload for the server AND the key for the link. */ +export async function sealSecret(plaintext) { + const key = await crypto.subtle.generateKey( + { name: ALGORITHM, length: KEY_BITS }, true, ['encrypt', 'decrypt']) + const iv = crypto.getRandomValues(new Uint8Array(IV_BYTES)) + const ciphertext = await crypto.subtle.encrypt( + { name: ALGORITHM, iv }, key, new TextEncoder().encode(plaintext)) + const raw = await crypto.subtle.exportKey('raw', key) + return { + ciphertext: toBase64(ciphertext), + iv: toBase64(iv), + // Goes in the fragment. Never sent anywhere. + fragmentKey: toBase64Url(raw), + } +} + +/** Decrypt what the server handed back, using the key from the fragment. */ +export async function openSecret({ ciphertext, iv, fragmentKey }) { + const key = await crypto.subtle.importKey( + 'raw', fromBase64Url(fragmentKey), + { name: ALGORITHM, length: KEY_BITS }, false, ['decrypt']) + const plaintext = await crypto.subtle.decrypt( + { name: ALGORITHM, iv: fromBase64(iv) }, key, fromBase64(ciphertext)) + return new TextDecoder().decode(plaintext) +} + +// -- Keeping this from becoming a document transfer --------------------------- +// +// The server can only bound SIZE, because it cannot read what it holds. These +// run here, where the plaintext is still visible, and are advisory: they guide +// somebody who is about to do the wrong thing without realising. A determined +// person can POST directly, which is why the size limit on the server is the +// control that actually enforces. + +// Matches MAX_CIPHERTEXT server-side, minus room for base64 expansion and the +// GCM tag, so the browser refuses before a round trip rather than after one. +export const MAX_PLAINTEXT = 2800 + +const MAX_LINES = 20 + +// A pasted file, recognised by how it starts. Cheap, high signal. +const FILE_SIGNATURES = [ + ['%PDF', 'a PDF'], + ['PK\x03\x04', 'a zip or Office file'], + ['{\\rtf', 'an RTF document'], + ['\x89PNG', 'a PNG image'], + // JPEG magic bytes as escapes rather than literals: the naming gate bans + // non-ASCII source, and these are bytes rather than text anyway. + ['\xff\xd8\xff', 'a JPEG image'], + ['data:', 'a data URI'], +] + +/** Why this text should not be sent, or null when it is fine. */ +export function refuseReason(text) { + const value = text || '' + if (!value.trim()) return 'Nothing to send yet.' + + if (value.length > MAX_PLAINTEXT) { + return `That is ${value.length} characters. This is for passwords and short ` + + `text, not documents - the limit is ${MAX_PLAINTEXT}.` + } + + for (const [signature, description] of FILE_SIGNATURES) { + if (value.startsWith(signature)) { + return `That looks like ${description} pasted as text. Secure send is for ` + + 'credentials, not file transfer.' + } + } + + const lines = value.split('\n').length + if (lines > MAX_LINES) { + return `That is ${lines} lines. A password is one - this looks like a ` + + 'document, and secure send is not a file transfer.' + } + + return null +} + +/** + * Reject a paste that carries FILES rather than text. + * + * Removes the easiest wrong path - dragging a PDF in, or Ctrl+V on a copied + * file - without anyone having to read a rule about it. + */ +export function pasteCarriesFiles(event) { + const items = event?.clipboardData?.items + if (!items) return false + return Array.from(items).some(item => item.kind === 'file') +} diff --git a/plugins/tools/frontend/tools.js b/plugins/tools/frontend/tools.js index aa93420b..95127ee0 100644 --- a/plugins/tools/frontend/tools.js +++ b/plugins/tools/frontend/tools.js @@ -24,11 +24,21 @@ export const TOOLS = [ standalone: true, keywords: ['qr', 'barcode', 'code128', 'label', 'zebra', 'sticker', 'csv', 'print'], }, + { + id: 'secure-send', + name: 'Secure Send', + description: 'Hand somebody a password by a link that works once and then destroys it.', + category: 'security', + route: '/tools/secure-send', + keywords: ['password', 'secret', 'credential', 'share', 'link', 'expire', + 'one time', 'burn', 'pusher'], + }, ] export const CATEGORY_LABELS = { labels: 'Labels & Printing', convert: 'Conversion', + security: 'Secrets & Access', network: 'Network', } diff --git a/plugins/tools/frontend/views/SecureSendCreate.vue b/plugins/tools/frontend/views/SecureSendCreate.vue new file mode 100644 index 00000000..a679db72 --- /dev/null +++ b/plugins/tools/frontend/views/SecureSendCreate.vue @@ -0,0 +1,251 @@ + + + + + diff --git a/plugins/tools/frontend/views/SecureSendRead.vue b/plugins/tools/frontend/views/SecureSendRead.vue new file mode 100644 index 00000000..e562f3c0 --- /dev/null +++ b/plugins/tools/frontend/views/SecureSendRead.vue @@ -0,0 +1,154 @@ + + + + + diff --git a/plugins/tools/frontend/views/SecureSendSettings.vue b/plugins/tools/frontend/views/SecureSendSettings.vue new file mode 100644 index 00000000..3c1a345d --- /dev/null +++ b/plugins/tools/frontend/views/SecureSendSettings.vue @@ -0,0 +1,77 @@ + + + + + diff --git a/plugins/tools/migrations/versions/0001_securesend_baseline.py b/plugins/tools/migrations/versions/0001_securesend_baseline.py index ebdc7dae..2ead58df 100644 --- a/plugins/tools/migrations/versions/0001_securesend_baseline.py +++ b/plugins/tools/migrations/versions/0001_securesend_baseline.py @@ -12,6 +12,11 @@ plugin's tables, so the foreign key to the core users table cannot resolve at CreateTable-compile time. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'securesendsecrets', sa.Column('secretid', sa.Integer(), nullable=False), # 128 bits of randomness as hex. With no login on the read side this is @@ -47,13 +52,13 @@ def upgrade(): ) # Unique, because the token is looked up on every read and two rows sharing # one would make a link ambiguous. - op.create_index('ixsecuresendsecretstoken', 'securesendsecrets', - ['token'], unique=True) + create_index_if_missing('ixsecuresendsecretstoken', 'securesendsecrets', + ['token'], unique=True) # Expiry sweeps scan on this. - op.create_index('ixsecuresendsecretsexpiresat', 'securesendsecrets', - ['expiresat']) - op.create_index('ixsecuresendsecretscreatedby', 'securesendsecrets', - ['createdbyuserid']) + create_index_if_missing('ixsecuresendsecretsexpiresat', 'securesendsecrets', + ['expiresat']) + create_index_if_missing('ixsecuresendsecretscreatedby', 'securesendsecrets', + ['createdbyuserid']) def downgrade(): diff --git a/plugins/tools/plugin.py b/plugins/tools/plugin.py index a1cbc7fb..b86a564b 100644 --- a/plugins/tools/plugin.py +++ b/plugins/tools/plugin.py @@ -54,21 +54,66 @@ class ToolsPlugin(BasePlugin): return [SecureSecret] def get_permissions(self) -> List: - """One permission, on CREATING a secret only. + """Who may CREATE a secret, when guests are not allowed. - There is deliberately no read permission: the recipient usually has no - account here, and the unguessable token is what authorises the read. - Gating that side would make the tool useless for the case it exists for - - handing somebody a credential while you are on a call with them. + The default access model: signed in, and holding this. Reading needs no + permission at all - the recipient usually has no account here and the + unguessable token is the authorisation. - No delete permission either. A secret is spent by being read or by - expiring, and both remove the row; a hand-delete route would only add a - way to destroy the record of a secret that had not been read yet. + A site that turns on securesend_allow_guests bypasses this for people + with no account; it still applies to anyone who IS signed in, so a + member without it cannot create one while a guest can. That is + deliberate: opening the door to guests is a decision about the door, not + about revoking what a role does or does not hold. """ return [ ('tools.secrets', 'Create secure send links', 'tools'), ] + def get_settings_defaults(self) -> List[dict]: + """The one switch this plugin owns. + + Public so the create page can read it BEFORE login and show a guest the + form rather than a sign-in wall. It says nothing sensitive: whether the + door is open is visible to anyone who tries the door. + """ + return [ + { + 'key': 'securesend_allow_guests', + 'value': 'false', + 'valuetype': 'boolean', + 'category': 'tools', + 'public': True, + 'description': ('Let people with no account create secure send ' + 'links. Off by default: a guest secret has no ' + 'creator, so the audit trail can say when and ' + 'from where but never who. Receiving a secret ' + 'never needs an account either way.'), + }, + ] + + def get_settings_cards(self) -> List[dict]: + """A page for the one switch this plugin owns (ADR-010). + + get_settings_defaults SEEDS the row; this is what gives somebody a place + to change it. Declaring only the first left the setting in the database + with no way to reach it, which is indistinguishable from the feature not + existing. + """ + return [ + { + 'group': 'Tools', + 'to': '/settings/secure-send', + # From the settings iconMap; an unknown key falls back to a + # puzzle piece, which reads as "nobody chose this". + 'icon': 'wrench', + 'title': 'Secure Send', + 'description': 'Whether people without an account can create ' + 'one-view password links', + 'position': 60, + }, + ] + def get_navigation_items(self) -> List[dict]: return [ { diff --git a/plugins/warranty/frontend/views/WarrantiesList.vue b/plugins/warranty/frontend/views/WarrantiesList.vue index e208cc04..1913dd92 100644 --- a/plugins/warranty/frontend/views/WarrantiesList.vue +++ b/plugins/warranty/frontend/views/WarrantiesList.vue @@ -24,7 +24,7 @@ - + {{ filteredItems.length }} of {{ items.length }} @@ -186,6 +186,7 @@ import api, { warrantyApi, assetsApi, vendorsApi } from '@/api' import { useToast } from '@/composables/toast' import { apiError } from '@/utils/apiError' import MachineMapChip from '../components/MachineMapChip.vue' +import { matchesWarrantySearch } from './warrantySearch' const toast = useToast() const route = useRoute() @@ -197,16 +198,11 @@ const search = ref('') const page = ref(1) const perPage = 25 -// Client-side search across vendor / service level / tag / linked asset numbers. -const filteredItems = computed(() => { - const term = search.value.trim().toLowerCase() - if (!term) return items.value - return items.value.filter(w => - (w.vendor || '').toLowerCase().includes(term) || - (w.servicelevel || '').toLowerCase().includes(term) || - (w.servicetag || '').toLowerCase().includes(term) || - (w.assets || []).some(a => (a.assetnumber || '').toLowerCase().includes(term))) -}) +// Client-side search. The field list lives in warrantySearch.js, where it is +// covered by tests - the machine number was rendered in this table for a long +// time before anything let you search for it. +const filteredItems = computed(() => + items.value.filter(w => matchesWarrantySearch(w, search.value))) const totalPages = computed(() => Math.max(1, Math.ceil(filteredItems.value.length / perPage))) const paginatedItems = computed(() => filteredItems.value.slice((page.value - 1) * perPage, page.value * perPage)) diff --git a/plugins/warranty/frontend/views/warrantySearch.js b/plugins/warranty/frontend/views/warrantySearch.js new file mode 100644 index 00000000..8579e299 --- /dev/null +++ b/plugins/warranty/frontend/views/warrantySearch.js @@ -0,0 +1,44 @@ +// What the warranty list's search box matches against. +// +// Pulled out of WarrantiesList.vue so the field list is somewhere a test can +// see it. It went wrong the way a field list usually does: silently, by a +// column being added to the table and not to the filter. +// +// THE MACHINE NUMBER IS THE POINT. _related_machine in the warranty API exists +// precisely because "a shopfloor PC is bought, warranted and replaced as a PC, +// but it is FOUND by the machine it drives - nobody walks the floor looking for +// an asset number". The payload carries it, the table renders it in a chip, and +// the filter did not look at it - so the one identifier the feature was built +// around was the one thing the search box could not find. + +/** + * Does this warranty match the search term? + * + * @param {object} warranty - a row from GET /api/warranties + * @param {string} term - the raw search box contents + * @returns {boolean} true when the term is empty or any searched field contains it + */ +export function matchesWarrantySearch(warranty, term) { + const needle = (term || '').trim().toLowerCase() + if (!needle) return true + if (!warranty) return false + + const has = (value) => String(value ?? '').toLowerCase().includes(needle) + + if (has(warranty.vendor) || has(warranty.servicelevel) || has(warranty.servicetag)) { + return true + } + + return (warranty.assets || []).some((asset) => { + if (has(asset?.assetnumber)) return true + // The related machine is optional: a warranty on something that drives no + // machine has none, and a machine that was never linked leaves it null. + const machine = asset?.machine + return !!machine && (has(machine.machinenumber) || has(machine.name)) + }) +} + +/** The fields above, in the order the placeholder should name them. */ +export const WARRANTY_SEARCH_FIELDS = [ + 'vendor', 'service level', 'tag', 'asset number', 'machine', +] diff --git a/plugins/warranty/frontend/views/warrantySearch.spec.js b/plugins/warranty/frontend/views/warrantySearch.spec.js new file mode 100644 index 00000000..fc2e17cd --- /dev/null +++ b/plugins/warranty/frontend/views/warrantySearch.spec.js @@ -0,0 +1,79 @@ +import { describe, it, expect } from 'vitest' + +import { matchesWarrantySearch } from './warrantySearch' + +// Reported: the warranty page could not be searched by machine number. +// +// The number was in the payload (_related_machine puts it there) and rendered +// in the table as a chip, but the filter looked only at vendor, service level, +// service tag and asset number. So the identifier the feature was built around +// - a PC is warranted as a PC but FOUND by the machine it drives - was the one +// thing the box could not find. + +function warranty(overrides = {}) { + return { + vendor: 'Dell', + servicelevel: 'ProSupport', + servicetag: 'ABC1234', + assets: [{ + assetnumber: 'PC-00912', + name: 'Bay 3105 workstation', + machine: { machinenumber: '3016', name: 'Makino a61nx' }, + }], + ...overrides, + } +} + +describe('matchesWarrantySearch', () => { + it('matches on the machine number', () => { + // The reported case. + expect(matchesWarrantySearch(warranty(), '3016')).toBe(true) + }) + + it('matches on the machine name', () => { + expect(matchesWarrantySearch(warranty(), 'makino')).toBe(true) + }) + + it('still matches the fields it always did', () => { + expect(matchesWarrantySearch(warranty(), 'dell')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'prosupport')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'abc1234')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'pc-00912')).toBe(true) + }) + + it('is case insensitive and ignores surrounding space', () => { + expect(matchesWarrantySearch(warranty(), ' MAKINO ')).toBe(true) + }) + + it('does not match an unrelated term', () => { + expect(matchesWarrantySearch(warranty(), '9999')).toBe(false) + }) + + it('keeps every row when the term is empty', () => { + expect(matchesWarrantySearch(warranty(), '')).toBe(true) + expect(matchesWarrantySearch(warranty(), ' ')).toBe(true) + expect(matchesWarrantySearch(warranty(), undefined)).toBe(true) + }) + + it('handles a warranty whose asset drives no machine', () => { + // machine is null whenever the covered asset has no controls link - a + // printer, or a PC nobody linked yet. It must not throw. + const row = warranty({ assets: [{ assetnumber: 'PR-42', machine: null }] }) + expect(matchesWarrantySearch(row, 'PR-42')).toBe(true) + expect(matchesWarrantySearch(row, '3016')).toBe(false) + }) + + it('handles a warranty with no linked assets at all', () => { + const row = warranty({ assets: [] }) + expect(matchesWarrantySearch(row, 'dell')).toBe(true) + expect(matchesWarrantySearch(row, '3016')).toBe(false) + }) + + it('tolerates missing fields rather than matching everything', () => { + // String(undefined) is 'undefined', which would make a search for 'und' + // match every row with an empty field. + const row = { vendor: null, servicelevel: undefined, assets: null } + expect(matchesWarrantySearch(row, 'und')).toBe(false) + expect(matchesWarrantySearch(row, 'null')).toBe(false) + }) +}) diff --git a/scripts/backfill_vendor_from_model.py b/scripts/backfill_vendor_from_model.py index d80f923f..7b51d4aa 100644 --- a/scripts/backfill_vendor_from_model.py +++ b/scripts/backfill_vendor_from_model.py @@ -33,9 +33,22 @@ Dry run by default; nothing is written without --commit. import argparse import os import sys +from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + # Each asset table that carries both a model link and its own vendor, with the # column that identifies a row in reports and logs. TARGETS = [ diff --git a/scripts/collapse_duplicate_backup_revisions.py b/scripts/collapse_duplicate_backup_revisions.py index d76fc82f..fe792980 100644 --- a/scripts/collapse_duplicate_backup_revisions.py +++ b/scripts/collapse_duplicate_backup_revisions.py @@ -38,11 +38,24 @@ database backup before --commit anyway; this deletes rows. import argparse import os import sys +from pathlib import Path from collections import defaultdict # Run from anywhere: put the repo root (parent of scripts/) on the path. sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + from shopdb import create_app from shopdb.api import db from shopdb.core.models import Asset diff --git a/scripts/export_catalog.py b/scripts/export_catalog.py index cf0d8b77..f3ba662d 100644 --- a/scripts/export_catalog.py +++ b/scripts/export_catalog.py @@ -25,10 +25,23 @@ import argparse import json import os import sys +from pathlib import Path from collections import OrderedDict sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + DEFAULT_OUT = os.path.join('shopdb', 'data', 'catalog.json') diff --git a/scripts/import_ntlars_backups.py b/scripts/import_ntlars_backups.py index 95b69aad..212e9c9c 100644 --- a/scripts/import_ntlars_backups.py +++ b/scripts/import_ntlars_backups.py @@ -21,10 +21,23 @@ Dry run by default. Nothing is written without --commit. import argparse import os import sys +from pathlib import Path from datetime import datetime sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + from shopdb import create_app # noqa: E402 from shopdb.api import db # noqa: E402 diff --git a/scripts/reclassify_servers_to_network.py b/scripts/reclassify_servers_to_network.py index ba296799..2bf50e5b 100644 --- a/scripts/reclassify_servers_to_network.py +++ b/scripts/reclassify_servers_to_network.py @@ -19,10 +19,23 @@ Usage (on the target instance, in the app dir): import argparse import os import sys +from pathlib import Path # Run from anywhere: put the repo root (parent of scripts/) on the path. sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + from shopdb import create_app from shopdb.api import db from shopdb.core.models import Asset, AssetType diff --git a/scripts/refile_partmarker_backups.py b/scripts/refile_partmarker_backups.py index a5f7f9f2..b29a4474 100644 --- a/scripts/refile_partmarker_backups.py +++ b/scripts/refile_partmarker_backups.py @@ -35,11 +35,24 @@ Dry run by default. Take a database backup before --commit. import argparse import os import sys +from pathlib import Path from collections import defaultdict # Run from anywhere: put the repo root (parent of scripts/) on the path. sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + from shopdb import create_app from shopdb.api import db, Asset, AssetRelationship, RelationshipType from plugins.backups.models import BackupRevision diff --git a/scripts/retype_models.py b/scripts/retype_models.py index 1b94843d..01f5d1af 100644 --- a/scripts/retype_models.py +++ b/scripts/retype_models.py @@ -33,9 +33,22 @@ import csv import os import re import sys +from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from dotenv import load_dotenv + +# Load .env before anything reads config. wsgi.py does this for the served app +# and the `flask` CLI does it for itself, but a plain `python scripts/...` run +# gets neither: DATABASE_URL stays unset, FLASK_ENV defaults to development, +# and config falls back to a hardcoded root@localhost URL. That fails on a real +# deployment with a bare 1045 Access denied for 'root', naming a user the site +# does not use - and on a box where those root credentials happen to work, it +# is worse, because the script silently reads or WRITES the wrong database. +load_dotenv(Path(__file__).resolve().parent.parent / '.env') + + # Each asset class, its own type vocabulary, and the modeltypes.category that a # newly created model type should carry. CLASSES = { diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 8cc89ae8..75d2b8df 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -251,13 +251,15 @@ def register_blueprints(app: Flask): def register_cli_commands(app: Flask): """Register Flask CLI commands.""" from .plugins.cli import plugin_cli - from .cli import db_cli, seed_cli, relationships_cli, csv_cli + from .cli import (db_cli, seed_cli, relationships_cli, csv_cli, + catalog_cli) app.cli.add_command(plugin_cli) app.cli.add_command(db_cli) app.cli.add_command(seed_cli) app.cli.add_command(relationships_cli) app.cli.add_command(csv_cli) + app.cli.add_command(catalog_cli) def register_error_handlers(app: Flask): diff --git a/shopdb/cli/__init__.py b/shopdb/cli/__init__.py index 05f40cb3..460c5561 100644 --- a/shopdb/cli/__init__.py +++ b/shopdb/cli/__init__.py @@ -1535,3 +1535,303 @@ def audit_relationships(): 'knowing which side is authoritative.') else: click.echo(click.style('No relationship faults found.', fg='green')) + + +# --- catalog hygiene -------------------------------------------------------- + +@click.group('catalog') +def catalog_cli(): + """Clean up the shared vendor/model/OS catalog.""" + pass + + +# Catalog tables that carry a human-typed name, with the column that names them. +# Each is free vocabulary somebody can add to, which is how twins appear. +_CATALOG_TABLES = ( + ('vendors', 'vendor'), + ('models', 'modelnumber'), + ('operatingsystems', 'osname'), +) + + +def _catalog_norm(value): + """Collapse a name to what it would be with no spacing or punctuation. + + Only whitespace and punctuation are removed, and case is folded. That is + deliberately narrow: 'Dell Inc.' and 'DellInc.' are the SAME name typed two + ways, while 'Dell' and 'Dell Inc.' are a naming CHOICE and must never be + merged automatically. Use `catalog merge` for that. + """ + import re + return re.sub(r'[\s\-_.,/]+', '', str(value or '').strip().lower()) + + +def _inbound_foreign_keys(inspector, table): + """Every (table, column) in the schema that points at `table`'s key. + + Discovered from the live schema rather than listed here, because a plugin + can add one: `machines.controllervendorid` and `printerdrivers. + modelnumberid` both reference the catalog and neither is obvious. + """ + refs = [] + for candidate in inspector.get_table_names(): + for fk in inspector.get_foreign_keys(candidate): + if fk['referred_table'] == table: + refs.append((candidate, + fk['constrained_columns'][0], + fk['referred_columns'][0])) + return refs + + +def _reference_counts(connection, refs, keyvalue): + """How many rows across the schema point at one catalog row.""" + import sqlalchemy as sa + total = 0 + detail = [] + for tablename, column, _ in refs: + count = connection.execute( + sa.text(f'SELECT COUNT(*) FROM {tablename} WHERE {column} = :k'), + {'k': keyvalue}).scalar() + if count: + detail.append(f'{tablename}.{column}={count}') + total += count + return total, detail + + +def _merge_catalog_rows(connection, table, keycolumn, refs, loser, winner): + """Repoint every reference off `loser` onto `winner`, then delete it.""" + import sqlalchemy as sa + moved = 0 + for tablename, column, _ in refs: + result = connection.execute( + sa.text(f'UPDATE {tablename} SET {column} = :w WHERE {column} = :l'), + {'w': winner, 'l': loser}) + moved += result.rowcount or 0 + connection.execute( + sa.text(f'DELETE FROM {table} WHERE {keycolumn} = :l'), {'l': loser}) + return moved + + +@catalog_cli.command('dedupe') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the changes. Without this the command only reports.') +@with_appcontext +def catalog_dedupe(apply_changes): + """Merge catalog rows that are the same name typed two ways. + + Legacy data arrived carrying space-stripped twins - 'DellInc.' beside + 'Dell Inc.', 'Windows10' beside 'Windows 10' - which then rode into the + shipped seed catalog and out to every site that ran `seed catalog`. They + are invisible until somebody opens a vendor picker and finds the same + company twice. + + ONLY whitespace-and-punctuation twins are touched. 'Dell' and 'Dell Inc.' + are a naming choice, not a typo, and are left alone; use `catalog merge` + to make that call explicitly. + + The surviving row is the one with the most references, tie-broken toward + the spaced spelling, because that is the one a person typed deliberately. + Every foreign key pointing at the loser is repointed first, so nothing is + orphaned; references are counted across the LIVE schema, which picks up + plugin tables such as machines.controllervendorid. + + Reports only unless --apply is passed. Idempotent. + """ + import sqlalchemy as sa + from collections import defaultdict + from shopdb.extensions import db + + engine = db.session.get_bind() + inspector = sa.inspect(engine) + existing = set(inspector.get_table_names()) + + planned = [] + with engine.begin() as connection: + for table, namecolumn in _CATALOG_TABLES: + if table not in existing: + continue + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + refs = [r for r in _inbound_foreign_keys(inspector, table) + if r[0] in existing] + + rows = connection.execute(sa.text( + f'SELECT {keycolumn}, {namecolumn} FROM {table}')).fetchall() + groups = defaultdict(list) + for key, name in rows: + if name: + groups[_catalog_norm(name)].append((key, name)) + + for _, members in sorted(groups.items()): + if len(members) < 2: + continue + scored = [] + for key, name in members: + count, detail = _reference_counts(connection, refs, key) + # most-referenced wins; then the spelling with whitespace, + # which is the one somebody typed on purpose; then longest. + scored.append((count, ' ' in str(name), len(str(name)), + key, name, detail)) + scored.sort(reverse=True) + winner = scored[0] + for loser in scored[1:]: + planned.append({ + 'table': table, 'keycolumn': keycolumn, 'refs': refs, + 'winner_key': winner[3], 'winner_name': winner[4], + 'loser_key': loser[3], 'loser_name': loser[4], + 'loser_refs': loser[0], 'loser_detail': loser[5], + }) + + if not planned: + click.echo(click.style('No catalog twins found.', fg='green')) + return + + click.echo(f'{len(planned)} duplicate row(s):') + click.echo('') + for item in planned: + where = (', '.join(item['loser_detail']) if item['loser_detail'] + else 'nothing references it') + click.echo(f" {item['table']}: {item['loser_name']!r} " + f"-> {item['winner_name']!r}") + click.echo(f" {item['loser_refs']} reference(s): {where}") + + if not apply_changes: + click.echo('') + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + moved = 0 + for item in planned: + moved += _merge_catalog_rows( + connection, item['table'], item['keycolumn'], item['refs'], + item['loser_key'], item['winner_key']) + + click.echo('') + click.echo(click.style( + f'Merged {len(planned)} row(s), repointed {moved} reference(s).', + fg='green')) + + +@catalog_cli.command('merge') +@click.argument('table', type=click.Choice([t for t, _ in _CATALOG_TABLES])) +@click.argument('source') +@click.argument('target') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the changes. Without this the command only reports.') +@with_appcontext +def catalog_merge(table, source, target, apply_changes): + """Merge one catalog row into another by name: SOURCE is absorbed by TARGET. + + For the judgment calls `catalog dedupe` deliberately refuses to make - + 'Dell' and 'Dell Inc.' are different names, and only a person knows the + site means one company. + + flask catalog merge vendors "Dell" "Dell Inc." --apply + + Every reference to SOURCE is repointed to TARGET, then SOURCE is deleted. + Reports only unless --apply is passed. + """ + import sqlalchemy as sa + from shopdb.extensions import db + + namecolumn = dict(_CATALOG_TABLES)[table] + engine = db.session.get_bind() + inspector = sa.inspect(engine) + existing = set(inspector.get_table_names()) + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + refs = [r for r in _inbound_foreign_keys(inspector, table) if r[0] in existing] + + with engine.begin() as connection: + def lookup(value): + return connection.execute( + sa.text(f'SELECT {keycolumn} FROM {table} ' + f'WHERE {namecolumn} = :v'), {'v': value}).fetchone() + + sourcerow, targetrow = lookup(source), lookup(target) + if not sourcerow: + click.echo(click.style(f'No {table} row named {source!r}.', fg='yellow')) + return + if not targetrow: + click.echo(click.style(f'No {table} row named {target!r}. ' + f'Refusing to invent it.', fg='red')) + raise SystemExit(1) + if sourcerow[0] == targetrow[0]: + click.echo(click.style('Source and target are the same row.', fg='yellow')) + return + + count, detail = _reference_counts(connection, refs, sourcerow[0]) + where = ', '.join(detail) if detail else 'nothing references it' + click.echo(f'{table}: {source!r} -> {target!r}') + click.echo(f' {count} reference(s): {where}') + + if not apply_changes: + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + moved = _merge_catalog_rows(connection, table, keycolumn, refs, + sourcerow[0], targetrow[0]) + + click.echo(click.style( + f'Merged {source!r} into {target!r}, repointed {moved} reference(s).', + fg='green')) + + +@catalog_cli.command('rename') +@click.argument('table', type=click.Choice([t for t, _ in _CATALOG_TABLES])) +@click.argument('source') +@click.argument('target') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the change. Without this the command only reports.') +@with_appcontext +def catalog_rename(table, source, target, apply_changes): + """Rename one catalog row in place, keeping every reference to it. + + A space-stripped name with NO twin cannot be deduped - there is nothing to + merge it into - so 'OptiPlex5000' would survive a `catalog dedupe` that + correctly removed 'OptiPlex7080'. This fixes that case. + + flask catalog rename models "OptiPlex5000" "OptiPlex 5000" --apply + + Refuses when the target name already exists, because that is a merge and + has to move references rather than rename a row: use `catalog merge`. + """ + import sqlalchemy as sa + from shopdb.extensions import db + + namecolumn = dict(_CATALOG_TABLES)[table] + engine = db.session.get_bind() + inspector = sa.inspect(engine) + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + + with engine.begin() as connection: + def lookup(value): + return connection.execute( + sa.text(f'SELECT {keycolumn} FROM {table} ' + f'WHERE {namecolumn} = :v'), {'v': value}).fetchone() + + sourcerow = lookup(source) + if not sourcerow: + click.echo(click.style(f'No {table} row named {source!r}.', fg='yellow')) + return + if lookup(target): + click.echo(click.style( + f'{table} already has a row named {target!r}. That is a merge, ' + f'not a rename - use `catalog merge`.', fg='red')) + raise SystemExit(1) + + click.echo(f'{table}: rename {source!r} -> {target!r} ' + f'(id {sourcerow[0]}, references unaffected)') + if not apply_changes: + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + connection.execute( + sa.text(f'UPDATE {table} SET {namecolumn} = :t WHERE {keycolumn} = :k'), + {'t': target, 'k': sourcerow[0]}) + + click.echo(click.style(f'Renamed {source!r} to {target!r}.', fg='green')) diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index a308bc5b..d023304c 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -658,7 +658,10 @@ def _search_subnets(query): 'id': subnet.subnetid, 'title': f'{subnet.name} ({subnet.cidr})', 'subtitle': subnet.description or subnet.subnettype, - 'url': f'/network', + # the matched subnet, not the hub: searching an IP + # already identified WHICH subnet, so landing on the + # hub makes the reader find it again by hand + 'url': f'/networks/{subnet.subnetid}', 'relevance': 70 }) except ValueError: @@ -779,7 +782,9 @@ def _search_notifications(query, search_term): 'id': notif.notificationid, 'title': notif.title, 'subtitle': notif.notificationtype.typename if notif.notificationtype else None, - 'url': f'/notifications', + # the notification itself, not the list it sits in: the + # `notification-detail` route renders one by id + 'url': f'/notifications/{notif.notificationid}', 'relevance': min(int(base_relevance), 100), 'ticketnumber': notif.ticketnumber, 'iscurrent': notif.is_current diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index d2f2830c..ff681833 100644 --- a/shopdb/core/api/settings.py +++ b/shopdb/core/api/settings.py @@ -777,6 +777,13 @@ def build_default_settings(): # QR codes and any absolute link the app emits - and facility name shown on # the shopfloor dashboard. Blank site_base_url falls back to the browsing # origin so nothing breaks before a site configures it. + # + # THE SUBPATH BELONGS IN THE VALUE. Consumers append the in-app route and + # add nothing themselves (resolveCodeText in print/assetLabel.js, buildQrUrl + # in utils/qrTarget.js), so a site served at /shopdb must carry that here or + # every generated link 404s. This said "scheme + host" for a while, which is + # true only of a site mounted at the root - and the email copy of the same + # setting has always said https://host/ops. sitedefaults = [ { 'key': 'setup_complete', @@ -811,7 +818,7 @@ def build_default_settings(): 'value': '', 'valuetype': 'string', 'category': 'site', - 'description': 'Public base URL of this site (scheme + host), e.g. https://shopdb.example.net. Used for QR codes and absolute links. Blank = use the browsing origin.' + 'description': 'Public base URL of this site INCLUDING any subpath it is served under, e.g. https://shopdb.example.net or https://host/shopdb. Consumers append the in-app route to this and add nothing themselves, so a subpath left off here produces links that 404. Blank = use the browsing origin.' }, { 'key': 'facility_name', diff --git a/shopdb/data/catalog.json b/shopdb/data/catalog.json index c2a7b917..34aaf6cc 100644 --- a/shopdb/data/catalog.json +++ b/shopdb/data/catalog.json @@ -383,9 +383,6 @@ { "osname": "Microsoft Windows 11 Enterprise" }, - { - "osname": "MicrosoftWindows10Enterprise" - }, { "osname": "TBD" }, @@ -404,9 +401,6 @@ { "osname": "Windows Server 2022", "osversion": "2022" - }, - { - "osname": "Windows10" } ], "vendors": [ @@ -432,15 +426,9 @@ { "vendor": "Cisco" }, - { - "vendor": "Dell" - }, { "vendor": "Dell Inc." }, - { - "vendor": "DellInc." - }, { "vendor": "DMG Mori" }, @@ -620,8 +608,8 @@ "modeltype": "Switch" }, { - "modelnumber": "OptiPlex5000", - "vendor": "Dell" + "modelnumber": "OptiPlex 5000", + "vendor": "Dell Inc." }, { "modelnumber": "Dell Pro 13 Plus PB13250", @@ -775,10 +763,6 @@ "vendor": "Dell Inc.", "modeltype": "LocationOnly" }, - { - "modelnumber": "OptiPlex7080", - "vendor": "DellInc." - }, { "modelnumber": "NT-4300", "vendor": "DMG Mori", diff --git a/shopdb/plugins/alembic_template.py b/shopdb/plugins/alembic_template.py index fa91fa24..70e272fe 100644 --- a/shopdb/plugins/alembic_template.py +++ b/shopdb/plugins/alembic_template.py @@ -104,7 +104,12 @@ def create_plugin_tables(plugin_name: str): this is safe on an existing database that has the table from the pre-cutover core baseline as well as on a fresh install (ADR-014 Phase 2). - Called from each plugin's 0001 baseline.py upgrade(). + NOT actually called from anywhere: every post-cutover baseline emits + explicit Alembic ops instead, because this helper's per-plugin MetaData + cannot resolve a foreign key to a CORE table at CreateTable-compile time. + Kept because it is still the reference for what idempotent means here, and + because create_table_if_missing below is the same rule in a form the + baselines can use. See that function for what went wrong without it. """ from alembic import op from sqlalchemy import inspect @@ -120,6 +125,67 @@ def create_plugin_tables(plugin_name: str): op.execute(str(CreateTable(table).compile(dialect=bind.dialect))) +def create_table_if_missing(table_name: str, *columns, **kwargs) -> bool: + """op.create_table, skipped when the table is already there. + + WHY EVERY POST-CUTOVER BASELINE NEEDS THIS. A baseline is the sole creator + of its plugin's tables, so it was written as a plain op.create_table - which + is correct only if the table's existence and the plugin's Alembic stamp can + never disagree. They can, and routinely do: + + * `flask db-utils create-all` runs db.create_all(), which builds EVERY + table registered on the metadata - plugin tables included - and stamps + NOTHING. `flask plugin upgrade-all` then runs the baseline against + tables that already exist. + * A run that creates the table and then fails before its stamp commits + leaves the same split, and re-running is the obvious thing to try. + + Reported as `flask plugin upgrade-all` failing the tools plugin with + "table securesendsecrets already exists". Nothing caught it because the + migration test drops the post-cutover tables db.create_all() made before + running upgrade-all, which arranges the one state where an unguarded create + works and never the state a real database is in. + + Returns True when it created the table, so a caller can skip the indexes + that belong to it. + """ + from alembic import op + from sqlalchemy import inspect + + if inspect(op.get_bind()).has_table(table_name): + logger.info('%s already exists, skipping create', table_name) + return False + op.create_table(table_name, *columns, **kwargs) + return True + + +def create_index_if_missing(index_name: str, table_name: str, columns, **kwargs): + """op.create_index, skipped when an equivalent index already exists. + + Matched on the COLUMN LIST rather than the name, because the two creators + do not agree on names. db.create_all() takes them from SQLAlchemy's default + convention (`ix_securesendsecrets_token`) while a baseline spells them the + way this repo's naming rules require (`ixsecuresendsecretstoken`). Matching + on the name would call the index missing and add a second one over the same + column - MySQL permits that, with only a warning, so the duplicate would + accumulate silently on every affected site. + """ + from alembic import op + from sqlalchemy import inspect + + wanted = [str(column) for column in columns] + inspector = inspect(op.get_bind()) + if not inspector.has_table(table_name): + op.create_index(index_name, table_name, columns, **kwargs) + return + for existing in inspector.get_indexes(table_name): + if list(existing.get('column_names') or []) == wanted: + logger.info('index over %s(%s) already exists as %s, skipping', + table_name, ', '.join(wanted), existing.get('name')) + return + op.create_index(index_name, table_name, columns, **kwargs) + + def drop_plugin_tables(plugin_name: str): """Mirror of create_plugin_tables for downgrade(). Drops in reverse FK order.""" diff --git a/shopdb/plugins/scaffolder.py b/shopdb/plugins/scaffolder.py index ea6eebe7..acb70434 100644 --- a/shopdb/plugins/scaffolder.py +++ b/shopdb/plugins/scaffolder.py @@ -81,18 +81,28 @@ def _scaffold_frontend( name: str, substitutions: dict, plugin_target: Path, - frontend_dir: Path, template_root: Path, overwrite: bool, ) -> None: """Render the frontend starting points for a scaffolded plugin. - Writes the paste-in api-client snippet into the plugin directory, then the - Vue views and the router route file into the real frontend tree. The - snippet is emitted regardless of whether the frontend tree exists, because - it is a plugin-directory artifact useful even for external-repo plugins. - Views and the route file are skipped when frontend_dir is missing, which is - the normal case for a plugin developed in its own repository. + Everything lands INSIDE the plugin, under `plugins//frontend/`: + `routes.js` plus `views/{List,Detail,Form}.vue`, and the paste-in + api-client snippet at the plugin root. + + The plugin owns its frontend because that is what makes it prunable. + scripts/stage-frontend.mjs copies each plugin's `frontend/` into + `frontend/src/.plugins-staged//` and aggregates the route modules + into `routes.gen.js`; a plugin left out of SITE_PLUGINS is simply not + copied, so its frontend leaves the build with it (ADR-013). A route file + written into the CORE tree instead is still picked up - the router globs + `./routes/*.js` during the Phase 4 dual-location transition - but it is + invisible to that pruning, and deleting the plugin strands its views. + + This is also why nothing here depends on the core frontend tree existing. + A plugin developed in its own repo gets exactly the same layout, and its + frontend starts working the moment the plugin directory is dropped into + `plugins/`. Ordering matters: every view is written before the route file. A route file that lazy-imports a view that is not on disk crashes the Vite dev server, @@ -104,7 +114,7 @@ def _scaffold_frontend( plugin_name = substitutions['Name'] - # snippet lands in the plugin dir; author pastes it into api/index.js + # snippet lands at the plugin root; author pastes it into api/index.js snippet_template = fe_templates / 'frontend-api-snippet.js.tmpl' if snippet_template.exists(): _render_template( @@ -114,11 +124,8 @@ def _scaffold_frontend( overwrite, ) - # views and route need the real frontend tree; external repos skip these - if not frontend_dir.exists(): - return - - views_dir = frontend_dir / 'views' / name + frontend_dir = plugin_target / 'frontend' + views_dir = frontend_dir / 'views' # views first: route file lazy-imports them, missing views 500 vite view_templates = { @@ -141,7 +148,7 @@ def _scaffold_frontend( if route_template.exists(): _render_template( route_template, - frontend_dir / 'router' / 'routes' / f'{name}.js', + frontend_dir / 'routes.js', substitutions, overwrite, ) @@ -154,7 +161,6 @@ def scaffold_plugin( template_root: Optional[Path] = None, overwrite: bool = False, frontend: bool = True, - frontend_dir: Optional[Path] = None, ) -> Path: """Generate a new plugin from templates. @@ -166,11 +172,10 @@ def scaffold_plugin( overwrite: If True, overwrite an existing plugin directory and any existing generated frontend files frontend: If True, also render the Vue frontend starting points (list, - detail, form views, a route file, and a paste-in api-client snippet) - frontend_dir: Frontend src directory to render views/routes into - (default: /../frontend/src). Views and the route file - are skipped when this directory does not exist, which is the normal - case for a plugin developed in its own repository. + detail, form views, a route file, and a paste-in api-client + snippet) into `plugins//frontend/`. There is no separate + output directory: the plugin owns its frontend so that + stage-frontend.mjs can stage it and a lean build can prune it. Returns: Path to the generated plugin directory. @@ -205,7 +210,9 @@ def scaffold_plugin( for template_path in template_root.rglob('*.tmpl'): rel = template_path.relative_to(template_root) - # frontend templates render into the frontend tree, not the plugin dir + # frontend templates are handled by _scaffold_frontend: they are + # renamed on the way out (List.vue.tmpl -> List.vue) and are + # skipped entirely when frontend=False if rel.parts and rel.parts[0] == 'frontend': continue @@ -222,13 +229,10 @@ def scaffold_plugin( out_path.write_text(rendered) if frontend: - if frontend_dir is None: - frontend_dir = plugins_dir.parent / 'frontend' / 'src' _scaffold_frontend( name=name, substitutions=substitutions, plugin_target=target, - frontend_dir=Path(frontend_dir), template_root=template_root, overwrite=overwrite, ) diff --git a/shopdb/plugins/templates/frontend/routes.js.tmpl b/shopdb/plugins/templates/frontend/routes.js.tmpl index 758b6f70..1616ee33 100644 --- a/shopdb/plugins/templates/frontend/routes.js.tmpl +++ b/shopdb/plugins/templates/frontend/routes.js.tmpl @@ -1,35 +1,49 @@ /** * ${Name} plugin routes. * - * Auto-discovered by the router via import.meta.glob, so no registration - * edit is needed. Every route carries meta.plugin '${name}' so the ADR-009 - * guard redirects to the dashboard when the ${name} backend plugin is - * disabled. Form routes add requiresAuth so anonymous users cannot reach - * create or edit. + * This file lives in the plugin, not in the core frontend tree. + * scripts/stage-frontend.mjs copies plugins/${name}/frontend/ into + * frontend/src/.plugins-staged/${name}/ and aggregates this default export + * into routes.gen.js, which the router spreads in - so no registration edit + * is needed, and pruning the plugin for a lean build removes its frontend + * with it (ADR-013). + * + * Every route carries meta.plugin '${name}' so the ADR-009 guard redirects to + * the dashboard when the ${name} backend plugin is disabled. Form routes add + * requiresAuth so anonymous users cannot reach create or edit. + * + * View imports are relative (./views/...) because they travel with this file. + * Anything from the CORE tree uses the @ alias (@/api, @/components/...), + * which resolves the same before and after staging. */ export default [ { path: '${name}', name: '${name}', - component: () => import('../../views/${name}/${Name}List.vue'), + component: () => import('./views/${Name}List.vue'), meta: { plugin: '${name}' } }, { path: '${name}/new', name: '${name}-new', - component: () => import('../../views/${name}/${Name}Form.vue'), + component: () => import('./views/${Name}Form.vue'), meta: { requiresAuth: true, plugin: '${name}' } }, { path: '${name}/:id', name: '${name}-detail', - component: () => import('../../views/${name}/${Name}Detail.vue'), + component: () => import('./views/${Name}Detail.vue'), meta: { plugin: '${name}' } }, { path: '${name}/:id/edit', name: '${name}-edit', - component: () => import('../../views/${name}/${Name}Form.vue'), + component: () => import('./views/${Name}Form.vue'), meta: { requiresAuth: true, plugin: '${name}' } } ] + +// Full-screen routes that render OUTSIDE AppLayout (kiosk, print pages). +// Declared even when empty so routes.gen.js can read `.toplevel` off the +// module without Rollup warning about a missing export. +export const toplevel = [] diff --git a/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl b/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl index 9777a799..b1dc5ec1 100644 --- a/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl +++ b/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl @@ -78,12 +78,12 @@