From 5fa5160420142c91b91c3ad54564264a0539a6da Mon Sep 17 00:00:00 2001 From: cproudlock Date: Fri, 26 Jun 2026 19:25:52 -0400 Subject: [PATCH] Apply skill-driven review fixes: security, hook isolation, tests, docs Addresses findings from a 6-lens review against the project skills (defining-asset-contract, enforcing-plugin-contract, hardening-flask-config, integrating-plugin-hooks, pinning-flask-behavior, simplifying-python). Security (hardening-flask-config): - Load per-plugin COLLECTOR_API_KEY_ from env in create_app. from_object only copies class attributes, so per-plugin keys (ADR-006) were dead in real deploys and silently fell back to the shared key. - EMPLOYEE_DB_USER/PASSWORD no longer default to root/rootpassword (no safe default for a secret; unset fails loud). Documented in .env.example + DEPLOY.md. - COLLECTOR_API_KEY + per-plugin + EMPLOYEE_DB_* added to .env.example/DEPLOY.md. Hook isolation (integrating-plugin-hooks): - collector _collector_plugins and dashboard get_navigation now re-raise in dev/test and log+isolate in prod, instead of silently swallowing a broken plugin hook. Plugin loader (enforcing-plugin-contract): - enable_plugin/install_plugin read dependencies+version from the manifest instead of instantiating the plugin class. - _register_plugin_components rejects a second plugin claiming an already-used api_prefix (reset per app in init_app). Tests (pinning-flask-behavior): - test_identifiers.py: gauge/maintenance round-trip on computer/printer/network create+update; per-type seed yields the 12 identifier keys. - contract tests for apply_collector_payload presence + schema-declarers-implement. - security tests for per-plugin key env loading + no employee-db password default. Docs/contract sync (defining-asset-contract): - PLUGIN-HOOKS.md documents apply_collector_payload; stale 0.2.0 -> 0.3.0. - ADR-006 documents apply_collector_payload + single-dispatch rationale. - ADR-001 enumerates the expanded shopdb.api import surface. Simplify (simplifying-python): - De-duplicate the 21-entry settings defaults: shared build_default_settings() used by both the /settings/seed route and the CLI (were drifting copies). - Remove dead AssetStatus import + redundant AssetType local import in computers plugin; comment the statusid=1 collector default. 153 tests pass (was 145), naming/style green. Co-Authored-By: Claude Opus 4.8 --- .env.example | 9 + docs/DEPLOY.md | 3 + docs/PLUGIN-HOOKS.md | 27 ++- .../adr/ADR-001-asset-as-platform-contract.md | 21 ++- docs/adr/ADR-006-collector-contract.md | 23 ++- plugins/computers/plugin.py | 6 +- shopdb/__init__.py | 8 + shopdb/cli/__init__.py | 160 +----------------- shopdb/config.py | 6 +- shopdb/core/api/collector.py | 8 +- shopdb/core/api/dashboard.py | 10 +- shopdb/core/api/settings.py | 19 ++- shopdb/plugins/__init__.py | 57 ++++--- tests/test_core/test_identifiers.py | 77 +++++++++ tests/test_plugin_contract.py | 17 ++ tests/test_security_config.py | 20 +++ 16 files changed, 273 insertions(+), 198 deletions(-) create mode 100644 tests/test_core/test_identifiers.py diff --git a/.env.example b/.env.example index ce2de8f..0fa2371 100644 --- a/.env.example +++ b/.env.example @@ -58,3 +58,12 @@ ZABBIX_TOKEN= # COLLECTOR_API_KEY_ first, then COLLECTOR_API_KEY as fallback. # COLLECTOR_API_KEY= # COLLECTOR_API_KEY_COMPUTERS= + +# ---- Employee directory database (optional, read-only) ---- +# Separate HR/employee lookup DB consumed by the notifications plugin and the +# public shopfloor kiosks. Leave unset if the feature is not used; there is no +# safe default for the password, so an unset password fails loud. +# EMPLOYEE_DB_HOST= +# EMPLOYEE_DB_USER= +# EMPLOYEE_DB_PASSWORD= +# EMPLOYEE_DB_NAME=wjf_employees diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 7e9e74e..e1f5190 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -32,6 +32,9 @@ Edit `.env`: | `API_PORT` | No | Default 5001 | | `LOG_LEVEL` | No | Default INFO | | `ZABBIX_URL`, `ZABBIX_TOKEN` | No | Only if printers plugin uses Zabbix | +| `COLLECTOR_API_KEY` | No | Shared key for `/api/collector/*` ingest. Required only if unattended collectors push data. Endpoint fails closed (denies) when unset. | +| `COLLECTOR_API_KEY_` | No | Per-plugin override (e.g. `COLLECTOR_API_KEY_COMPUTERS`), checked before the shared key (ADR-006) | +| `EMPLOYEE_DB_HOST/USER/PASSWORD/NAME` | No | Read-only HR directory for notifications + kiosks. No safe default for the password. | ## Step 2: Bring up the stack diff --git a/docs/PLUGIN-HOOKS.md b/docs/PLUGIN-HOOKS.md index e38490a..422789a 100644 --- a/docs/PLUGIN-HOOKS.md +++ b/docs/PLUGIN-HOOKS.md @@ -9,7 +9,7 @@ The contract is locked in [ADR-001](../docs/adr/ADR-001-asset-as-platform-contra The framework declares its contract version in `shopdb/__init__.py`: ```python -__contract_version__ = '0.2.0' +__contract_version__ = '0.3.0' ``` Each plugin's `manifest.json` declares the range of contract versions it supports: @@ -222,6 +222,31 @@ class ComputersPlugin(BasePlugin): If the hook returns `None` (the default), no collector endpoint is registered. +### `apply_collector_payload(payload: Dict) -> Dict` + +Companion to `get_collector_schema` (ADR-006). The generic +`/api/collector/` endpoint calls this after the payload passes +identity validation, to idempotently upsert an asset. Return a dict with at +least `action` (`created` | `updated` | `noop`), `assetid`, and `warnings` +(list). + +This is a CONDITIONAL hook: it is only required when `get_collector_schema` +returns non-None. The BasePlugin default raises `NotImplementedError` (the +dispatcher turns that into a 500), so a plugin that declares a schema but +forgets the upsert fails loud. Plugins with no collector schema never need it. +The `test_schema_declaring_plugins_implement_apply` contract test enforces the +pairing. + +```python +def apply_collector_payload(self, payload): + host = payload['hostname'] + comp = Computer.query.filter(Computer.hostname.ilike(host)).first() + action = 'updated' if comp else 'created' + # ... create-or-update Asset + extension ... + db.session.commit() + return {'action': action, 'assetid': comp.assetid, 'warnings': []} +``` + ## Lifecycle hooks These run when the plugin's installation state changes. All optional. diff --git a/docs/adr/ADR-001-asset-as-platform-contract.md b/docs/adr/ADR-001-asset-as-platform-contract.md index ff703f8..ffa5d27 100644 --- a/docs/adr/ADR-001-asset-as-platform-contract.md +++ b/docs/adr/ADR-001-asset-as-platform-contract.md @@ -36,10 +36,29 @@ The following are the public, versioned surface. Plugin authors may depend on th - `AuditLog` API: `audit_log(action, entitytype, entityid, ...)` for plugins to record audit entries with consistent schema - `Setting` API: `plugin.get_setting(key)` and `plugin.set_setting(key, value)` for plugin-scoped config persisted via the core `Setting` model +- `resolve_asset_position(asset)` for the documented position-resolution algorithm + +#### Import surface (`shopdb.api`) - expanded in __contract_version__ 0.3.0 + +`shopdb.api` is the ONLY core module plugins may import (plus `shopdb.plugins.base` +for the ABC). Deep imports of `shopdb.core.*`, `shopdb.extensions`, or +`shopdb.utils.*` are contract violations enforced by the +`test_plugins_only_import_contract_surface` test. The surface re-exports: + +- Infrastructure: `db`, `cache` +- Model bases: `BaseModel`, `AuditMixin` +- Core models: `Asset`, `AssetType`, `AssetStatus`, `Vendor`, `Model`, + `Communication`, `CommunicationType`, `Location`, `Setting`, `AuditLog`, + `Application`, `AppVersion`, `OperatingSystem` +- Responses: `success_response`, `error_response`, `paginated_response`, `ErrorCodes` +- Pagination: `get_pagination_params`, `paginate_query` +- Helpers: `audit_log`, `resolve_asset_position`; legacy `employee_connection` + +Adding a name here is a minor (additive) contract change; removing one is major. #### Plugin contract -- `BasePlugin` ABC and its hooks (search, navigation, dashboard, relationships, collector schema) +- `BasePlugin` ABC and its hooks (search, navigation, dashboard, relationships, collector schema + `apply_collector_payload`) #### Excluded from the contract for v1 diff --git a/docs/adr/ADR-006-collector-contract.md b/docs/adr/ADR-006-collector-contract.md index c18b477..e277f5a 100644 --- a/docs/adr/ADR-006-collector-contract.md +++ b/docs/adr/ADR-006-collector-contract.md @@ -15,7 +15,7 @@ This calls for a generalizable contract: any plugin that wants to accept externa ## Decision -`BasePlugin` gets one new optional hook: +`BasePlugin` gets two new hooks (added in __contract_version__ 0.2.x -> the surface is carried at 0.3.0): ```python def get_collector_schema(self) -> Optional[dict]: @@ -29,9 +29,26 @@ def get_collector_schema(self) -> Optional[dict]: - 'fields': JSON Schema definitions for the rest of the payload. """ return None + +def apply_collector_payload(self, payload: dict) -> dict: + """Idempotently upsert an asset from a validated collector payload. + + Called by /api/collector/ after identity validation. + CONDITIONAL hook: required only when get_collector_schema returns + non-None. Default raises NotImplementedError (the dispatcher returns + 500) so a schema-without-upsert fails loud. Returns a dict with at + least 'action' ('created'|'updated'|'noop'), 'assetid', 'warnings'. + """ + raise NotImplementedError ``` -Plugin loader auto-registers an endpoint at `/api/collector/` for each plugin returning a schema. Auth is API-key, separate from JWT. Per-plugin keys via env vars: +The pairing (schema present => apply implemented) is enforced by the +`test_schema_declaring_plugins_implement_apply` contract test. + +A single dynamic dispatch route `/api/collector/` serves every +plugin that returns a schema (rather than registering a blueprint per plugin), +because Flask forbids `register_blueprint` after the first request and plugins +can be enabled at runtime. Auth is API-key, separate from JWT. Per-plugin keys via env vars: - `COLLECTOR_API_KEY_` (preferred, plugin-specific) - `COLLECTOR_API_KEY` (fallback, shared) @@ -125,7 +142,7 @@ Migration path: ## References - `shopdb/core/api/collector.py` (legacy endpoint to be removed) -- `shopdb/plugins/base.py` (`get_collector_schema` hook to be added) +- `shopdb/plugins/base.py` (`get_collector_schema` + `apply_collector_payload` hooks) - ADR-001 (asset model the collectors target) - ADR-002 (collector schema is part of plugin contract; changes to the hook signature are major bumps) - The PXE project (`/home/camp/projects/pxe/`) which feeds the computers collector diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index 6f4e5c0..8e0508c 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -9,7 +9,7 @@ from flask import Flask, Blueprint import click from shopdb.plugins.base import BasePlugin, PluginMeta -from shopdb.api import db, AssetType, AssetStatus +from shopdb.api import db, AssetType from .models import Computer, ComputerType, ComputerInstalledApp from .api import computers_bp @@ -86,7 +86,7 @@ class ComputersPlugin(BasePlugin): def apply_collector_payload(self, payload: Dict) -> Dict: """Idempotent upsert of a PC from a collector payload (by hostname).""" from datetime import datetime - from shopdb.api import Asset, AssetType, Application, Communication, CommunicationType + from shopdb.api import Asset, Application, Communication, CommunicationType warnings = [] hostname = (payload.get('hostname') or '').strip() @@ -101,6 +101,8 @@ class ComputersPlugin(BasePlugin): action = 'updated' if not comp: atype = AssetType.query.filter_by(assettype='computer').first() + # statusid=1 is the first seeded asset status ("In Use"); a + # collector-discovered PC is by definition in use. asset = Asset(assetnumber=hostname, assettypeid=atype.assettypeid, statusid=1) db.session.add(asset) diff --git a/shopdb/__init__.py b/shopdb/__init__.py index cfd7ee8..7dab0d3 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -44,6 +44,14 @@ def create_app(config_name: str = None) -> Flask: # Load instance config if exists app.config.from_pyfile('config.py', silent=True) + # Per-plugin collector keys (ADR-006) are dynamic env-vars + # (COLLECTOR_API_KEY_) that from_object cannot pick up because + # they are not class attributes. Copy them in explicitly so per-plugin + # credential isolation works in real deploys, not just tests. + for envname, envvalue in os.environ.items(): + if envname.startswith('COLLECTOR_API_KEY_') and envvalue: + app.config[envname] = envvalue + # Ensure instance folder exists os.makedirs(app.instance_path, exist_ok=True) diff --git a/shopdb/cli/__init__.py b/shopdb/cli/__init__.py index d67fa6a..1ee37de 100644 --- a/shopdb/cli/__init__.py +++ b/shopdb/cli/__init__.py @@ -213,165 +213,9 @@ def seed_settings(): """Seed default system settings.""" from shopdb.extensions import db from shopdb.core.models import Setting + from shopdb.core.api.settings import build_default_settings - defaults = [ - # Zabbix integration - { - 'key': 'zabbix_enabled', - 'value': 'false', - 'valuetype': 'boolean', - 'category': 'integrations', - 'description': 'Enable Zabbix integration for printer supply monitoring' - }, - { - 'key': 'zabbix_url', - 'value': '', - 'valuetype': 'string', - 'category': 'integrations', - 'description': 'Zabbix API URL (e.g., http://zabbix.example.com:8080)' - }, - { - 'key': 'zabbix_token', - 'value': '', - 'valuetype': 'string', - 'category': 'integrations', - 'description': 'Zabbix API authentication token' - }, - # Email/SMTP settings - { - 'key': 'smtp_enabled', - 'value': 'false', - 'valuetype': 'boolean', - 'category': 'email', - 'description': 'Enable email notifications and alerts' - }, - { - 'key': 'smtp_host', - 'value': '', - 'valuetype': 'string', - 'category': 'email', - 'description': 'SMTP server hostname' - }, - { - 'key': 'smtp_port', - 'value': '587', - 'valuetype': 'integer', - 'category': 'email', - 'description': 'SMTP server port (usually 587 for TLS, 465 for SSL, 25 for unencrypted)' - }, - { - 'key': 'smtp_username', - 'value': '', - 'valuetype': 'string', - 'category': 'email', - 'description': 'SMTP authentication username' - }, - { - 'key': 'smtp_password', - 'value': '', - 'valuetype': 'string', - 'category': 'email', - 'description': 'SMTP authentication password' - }, - { - 'key': 'smtp_use_tls', - 'value': 'true', - 'valuetype': 'boolean', - 'category': 'email', - 'description': 'Use TLS encryption for SMTP connection' - }, - { - 'key': 'smtp_from_address', - 'value': '', - 'valuetype': 'string', - 'category': 'email', - 'description': 'From address for outgoing emails' - }, - { - 'key': 'smtp_from_name', - 'value': 'ShopDB', - 'valuetype': 'string', - 'category': 'email', - 'description': 'From name for outgoing emails' - }, - { - 'key': 'alert_recipients', - 'value': '', - 'valuetype': 'string', - 'category': 'email', - 'description': 'Default email recipients for alerts (comma-separated)' - }, - # Audit log settings - { - 'key': 'audit_retention_days', - 'value': '90', - 'valuetype': 'integer', - 'category': 'audit', - 'description': 'Number of days to retain audit logs (0 = keep forever)' - }, - # Authentication settings - { - 'key': 'saml_enabled', - 'value': 'false', - 'valuetype': 'boolean', - 'category': 'auth', - 'description': 'Enable SAML SSO authentication' - }, - { - 'key': 'saml_idp_metadata_url', - 'value': '', - 'valuetype': 'string', - 'category': 'auth', - 'description': 'SAML Identity Provider metadata URL' - }, - { - 'key': 'saml_entity_id', - 'value': '', - 'valuetype': 'string', - 'category': 'auth', - 'description': 'SAML Service Provider entity ID (e.g., https://shopdb.example.com)' - }, - { - 'key': 'saml_acs_url', - 'value': '', - 'valuetype': 'string', - 'category': 'auth', - 'description': 'SAML Assertion Consumer Service URL' - }, - { - 'key': 'saml_allow_local_login', - 'value': 'true', - 'valuetype': 'boolean', - 'category': 'auth', - 'description': 'Allow local username/password login when SAML is enabled' - }, - { - 'key': 'saml_auto_create_users', - 'value': 'true', - 'valuetype': 'boolean', - 'category': 'auth', - 'description': 'Automatically create users on first SAML login' - }, - { - 'key': 'saml_admin_group', - 'value': '', - 'valuetype': 'string', - 'category': 'auth', - 'description': 'SAML group name that grants admin role' - }, - ] - - # Asset identifier toggles, per identifier AND per asset type (ADR-001). - from shopdb.core.api.settings import IDENTIFIER_LABELS, IDENTIFIER_ASSETTYPES - for name, label in IDENTIFIER_LABELS.items(): - for assettype in IDENTIFIER_ASSETTYPES: - defaults.append({ - 'key': f'identifier_{name}_{assettype}_enabled', - 'value': 'true', - 'valuetype': 'boolean', - 'category': 'identifiers', - 'description': f'Show the {label} identifier on {assettype} assets', - }) + defaults = build_default_settings() created = 0 for d in defaults: diff --git a/shopdb/config.py b/shopdb/config.py index deecffa..708afbc 100644 --- a/shopdb/config.py +++ b/shopdb/config.py @@ -62,9 +62,11 @@ class Config: # Read-only HR/employee directory database (separate from the app DB). # Credentials come from the environment; never hardcode them in source. + # No safe default for the password: unset means empty, the connection + # fails loud rather than silently trying a guessed credential. EMPLOYEE_DB_HOST = os.environ.get('EMPLOYEE_DB_HOST', 'localhost') - EMPLOYEE_DB_USER = os.environ.get('EMPLOYEE_DB_USER', 'root') - EMPLOYEE_DB_PASSWORD = os.environ.get('EMPLOYEE_DB_PASSWORD', 'rootpassword') + EMPLOYEE_DB_USER = os.environ.get('EMPLOYEE_DB_USER', '') + EMPLOYEE_DB_PASSWORD = os.environ.get('EMPLOYEE_DB_PASSWORD', '') EMPLOYEE_DB_NAME = os.environ.get('EMPLOYEE_DB_NAME', 'wjf_employees') CACHE_TYPE = 'SimpleCache' diff --git a/shopdb/core/api/collector.py b/shopdb/core/api/collector.py index 2bfa4c9..d98f87f 100644 --- a/shopdb/core/api/collector.py +++ b/shopdb/core/api/collector.py @@ -81,7 +81,13 @@ def _collector_plugins(): try: schema = plugin.get_collector_schema() except Exception: - schema = None + # Fail loud in dev/test so a broken hook is visible; isolate the + # misbehaving plugin in prod and keep serving the healthy ones. + if current_app.config.get('DEBUG') or current_app.config.get('TESTING'): + raise + current_app.logger.exception( + 'Plugin %s get_collector_schema failed', name) + continue if schema: result[name] = (plugin, schema) return result diff --git a/shopdb/core/api/dashboard.py b/shopdb/core/api/dashboard.py index 7ed5610..a4371ea 100644 --- a/shopdb/core/api/dashboard.py +++ b/shopdb/core/api/dashboard.py @@ -125,15 +125,19 @@ def get_navigation(): # away (its routes stay registered until the next restart - Flask cannot # unregister a blueprint at runtime). for name, plugin in pm.get_all_plugins().items(): + if not pm.registry.is_enabled(name): + continue try: - if not pm.registry.is_enabled(name): - continue items = plugin.get_navigation_items() for item in items: item['plugin'] = name all_items.extend(items) except Exception: - pass + # Fail loud in dev/test; isolate a broken plugin in prod. + if current_app.config.get('DEBUG') or current_app.config.get('TESTING'): + raise + current_app.logger.exception( + 'Plugin %s get_navigation_items failed', name) # Add core information section items all_items.extend([ diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index 3625e0d..5533f66 100644 --- a/shopdb/core/api/settings.py +++ b/shopdb/core/api/settings.py @@ -161,10 +161,12 @@ def create_setting(): return success_response(setting.to_dict(), message='Setting created', http_code=201) -@settings_bp.route('/seed', methods=['POST']) -@jwt_required() -def seed_default_settings(): - """Seed default settings if they don't exist.""" +def build_default_settings(): + """Return the full default-settings list (identifier toggles + static). + + Shared by the /settings/seed route and the `flask seed settings` CLI so + the two definitions never drift. + """ # Asset identifier feature toggles, per identifier AND per asset type. # Key format: identifier___enabled (boolean). Admins pick # which optional identifiers show on which asset types. See ADR-001. @@ -327,8 +329,15 @@ def seed_default_settings(): }, ] + return defaults + + +@settings_bp.route('/seed', methods=['POST']) +@jwt_required() +def seed_default_settings(): + """Seed default settings if they don't exist.""" created = 0 - for d in defaults: + for d in build_default_settings(): if not Setting.query.filter_by(key=d['key']).first(): setting = Setting(**d) db.session.add(setting) diff --git a/shopdb/plugins/__init__.py b/shopdb/plugins/__init__.py index 9b1d74e..e9dee4c 100644 --- a/shopdb/plugins/__init__.py +++ b/shopdb/plugins/__init__.py @@ -40,11 +40,18 @@ class PluginManager: self.migration_manager: Optional[PluginMigrationManager] = None self._app: Optional[Flask] = None self._db = None + # API prefixes already claimed by a registered plugin blueprint, to + # detect two plugins overlapping on the same /api/... namespace. + self._registered_prefixes: set = set() def init_app(self, app: Flask, db) -> None: """Initialize plugin manager with Flask app.""" self._app = app self._db = db + # Reset per-app so the prefix-uniqueness guard tracks only this app's + # registrations (the manager is a process-wide singleton; tests build + # multiple apps from it). + self._registered_prefixes = set() # Setup paths instance_path = Path(app.instance_path) @@ -104,11 +111,18 @@ class PluginManager: # Register blueprint blueprint = plugin.get_blueprint() if blueprint: - self._app.register_blueprint( - blueprint, - url_prefix=plugin.meta.api_prefix - ) - logger.debug(f"Registered blueprint: {plugin.meta.api_prefix}") + prefix = plugin.meta.api_prefix + # Guard against two plugins claiming the same API prefix; Flask only + # rejects duplicate blueprint names, not overlapping url_prefixes, so + # an overlap would silently shadow routes. + if prefix in self._registered_prefixes: + raise ValueError( + f"Plugin {plugin.meta.name} api_prefix '{prefix}' is already " + f"claimed by another blueprint" + ) + self._app.register_blueprint(blueprint, url_prefix=prefix) + self._registered_prefixes.add(prefix) + logger.debug(f"Registered blueprint: {prefix}") # Register CLI commands for cmd in plugin.get_cli_commands(): @@ -160,17 +174,16 @@ class PluginManager: logger.warning(f"Plugin {name} is already installed") return False - # Load plugin class - plugin_class = self.loader.load_plugin_class(name) - if not plugin_class: + # Read metadata from the manifest (single source of truth) instead of + # instantiating the plugin class just to inspect deps/version. + manifest = self.loader.load_manifest(name) + if not manifest: logger.error(f"Plugin {name} not found") return False - - temp_plugin = plugin_class() - meta = temp_plugin.meta + manifest_version = manifest.get('version') # Check dependencies - for dep in meta.dependencies: + for dep in manifest.get('dependencies', []): if not self.registry.is_installed(dep): logger.error( f"Plugin {name} requires {dep} to be installed first" @@ -185,7 +198,7 @@ class PluginManager: return False # Register plugin - self.registry.register(name, meta.version) + self.registry.register(name, manifest_version) # Load the plugin plugin = self.loader.load_plugin(name, self._app, self._db) @@ -193,7 +206,7 @@ class PluginManager: self._register_plugin_components(plugin) plugin.on_install(self._app) - logger.info(f"Installed plugin: {name} v{meta.version}") + logger.info(f"Installed plugin: {name} v{manifest_version}") return True def uninstall_plugin(self, name: str, remove_data: bool = False) -> bool: @@ -246,14 +259,14 @@ class PluginManager: logger.info(f"Plugin {name} is already enabled") return True - # Check dependencies are enabled - plugin_class = self.loader.load_plugin_class(name) - if plugin_class: - temp = plugin_class() - for dep in temp.meta.dependencies: - if not self.registry.is_enabled(dep): - logger.error(f"Cannot enable {name}: {dep} is not enabled") - return False + # Check dependencies are enabled. Read deps from the manifest, not by + # instantiating the plugin class (manifest is the single source of + # truth; instantiating fires __init__ side effects unnecessarily). + manifest = self.loader.load_manifest(name) + for dep in manifest.get('dependencies', []): + if not self.registry.is_enabled(dep): + logger.error(f"Cannot enable {name}: {dep} is not enabled") + return False self.registry.enable(name) diff --git a/tests/test_core/test_identifiers.py b/tests/test_core/test_identifiers.py new file mode 100644 index 0000000..48b06e4 --- /dev/null +++ b/tests/test_core/test_identifiers.py @@ -0,0 +1,77 @@ +"""Tests for per-asset-type optional identifiers (gauge/maintenance refs). + +Pins two behaviors that shipped without coverage: +1. gaugelabreference + maintenancereference round-trip through each plugin's + asset create AND update endpoints (computer, printer, network). +2. The per-type identifier seed produces one setting per + (identifier x asset type) pair. +""" + +import pytest + +from shopdb.core.api.settings import IDENTIFIER_LABELS, IDENTIFIER_ASSETTYPES + + +@pytest.fixture +def asset_types(db): + """Seed the asset types the create endpoints look up by name.""" + from shopdb.core.models import AssetType + + for name in ('computer', 'printer', 'network_device'): + db.session.add(AssetType(assettype=name, pluginname=name, + tablename=name, description=name)) + db.session.commit() + + +# (endpoint prefix, extension key in the response, extra create fields) +PLUGIN_CASES = [ + ('/api/computers', 'computer', {}), + ('/api/printers', 'printer', {}), + ('/api/network', 'networkdevice', {}), +] + + +@pytest.mark.parametrize('prefix,extkey,extra', PLUGIN_CASES) +def test_gauge_maintenance_roundtrip(client, db, auth_headers, asset_types, + prefix, extkey, extra): + """Gauge/maintenance refs persist on create and update for each type.""" + payload = { + 'assetnumber': f'AST-{extkey}', + 'gaugelabreference': 'GL-1', + 'maintenancereference': 'MNT-1', + **extra, + } + created = client.post(prefix, json=payload, headers=auth_headers) + assert created.status_code == 201, created.get_json() + body = created.get_json()['data'] + assert body['gaugelabreference'] == 'GL-1' + assert body['maintenancereference'] == 'MNT-1' + + extid = body[extkey][f'{extkey}id'] if extkey != 'networkdevice' \ + else body[extkey]['networkdeviceid'] + + updated = client.put(f'{prefix}/{extid}', + json={'gaugelabreference': 'GL-2', + 'maintenancereference': 'MNT-2'}, + headers=auth_headers) + assert updated.status_code == 200, updated.get_json() + + fetched = client.get(f'{prefix}/{extid}', headers=auth_headers).get_json()['data'] + assert fetched['gaugelabreference'] == 'GL-2' + assert fetched['maintenancereference'] == 'MNT-2' + + +def test_per_type_identifier_seed_count(client, db, auth_headers): + """Seeding produces one boolean setting per identifier x asset type.""" + response = client.post('/api/settings/seed', headers=auth_headers) + assert response.status_code == 200 + + from shopdb.core.models import Setting + keys = {s.key for s in Setting.query.filter_by(category='identifiers').all()} + expected = { + f'identifier_{name}_{assettype}_enabled' + for name in IDENTIFIER_LABELS + for assettype in IDENTIFIER_ASSETTYPES + } + assert expected <= keys + assert len(expected) == len(IDENTIFIER_LABELS) * len(IDENTIFIER_ASSETTYPES) diff --git a/tests/test_plugin_contract.py b/tests/test_plugin_contract.py index 9d9a031..05e5ba8 100644 --- a/tests/test_plugin_contract.py +++ b/tests/test_plugin_contract.py @@ -146,6 +146,23 @@ def test_baseplugin_has_collector_schema_hook(): assert hasattr(BasePlugin, 'get_collector_schema') +def test_baseplugin_has_apply_collector_payload_hook(): + """The collector upsert hook is on the contract surface (ADR-006).""" + assert hasattr(BasePlugin, 'apply_collector_payload') + + +def test_schema_declaring_plugins_implement_apply(plugin_instances): + """Any plugin returning a collector schema must implement the upsert hook.""" + for name, plugin in plugin_instances.items(): + if plugin.get_collector_schema() is not None: + overridden = type(plugin).apply_collector_payload \ + is not BasePlugin.apply_collector_payload + assert overridden, ( + f'Plugin {name} declares a collector schema but does not ' + f'override apply_collector_payload' + ) + + # Imports a plugin may make from the core. shopdb.api is the contract surface; # shopdb.plugins.base is the plugin ABC. Anything else (shopdb.core.*, # shopdb.extensions, shopdb.utils.*) is a contract violation per ADR-001. diff --git a/tests/test_security_config.py b/tests/test_security_config.py index 2f35aff..0cd1e17 100644 --- a/tests/test_security_config.py +++ b/tests/test_security_config.py @@ -68,3 +68,23 @@ def test_production_validate_passes_with_complete_config(clean_env): clean_env.setenv('DATABASE_URL', 'mysql+pymysql://u:p@db/shopdb') clean_env.setenv('CORS_ORIGINS', 'https://shopdb.example.com') ProductionConfig.validate() + + +def test_per_plugin_collector_key_loaded_from_env(monkeypatch): + """COLLECTOR_API_KEY_ is a dynamic env var; create_app must load it. + + from_object only copies class attributes, so per-plugin keys (ADR-006) + would be invisible without the explicit env scan in create_app. + """ + from shopdb import create_app + + monkeypatch.setenv('COLLECTOR_API_KEY_COMPUTERS', 'computers-secret') + app = create_app('testing') + assert app.config.get('COLLECTOR_API_KEY_COMPUTERS') == 'computers-secret' + + +def test_employee_db_password_has_no_default(): + """No safe default for the employee-DB password: unset means empty.""" + if 'EMPLOYEE_DB_PASSWORD' not in os.environ: + from shopdb.config import Config + assert Config.EMPLOYEE_DB_PASSWORD == ''