ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Apply skill-driven review fixes: security, hook isolation, tests, docs

Browse Source 
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_<PLUGIN> 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 <noreply@anthropic.com>
This commit is contained in:
cproudlock
2026-06-26 19:25:52 -04:00
parent f663cc5bbe
commit 5fa5160420
16 changed files with 273 additions and 198 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
shopdb/__init__.py
View File

@@ -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_<PLUGINNAME>) 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)

160
shopdb/cli/__init__.py
View File

@@ -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:

6
shopdb/config.py
View File

@@ -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'

8
shopdb/core/api/collector.py
View File

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

10
shopdb/core/api/dashboard.py
View File

@@ -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([

19
shopdb/core/api/settings.py
View File

@@ -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_<name>_<assettype>_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)

57
shopdb/plugins/__init__.py
View File

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