From 1d2bbb3630631fbdc18debf10dddf075ba4e6003 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Thu, 3 Sep 2026 14:53:57 -0400 Subject: [PATCH] One key for audit retention, and stop the SAML page claiming a feature exists Wave 6: two settings pages that told an administrator the site was configured when it was not. That is worse than a missing feature, because nobody goes looking for it. Audit retention had TWO keys. shopdb/core/services/auditretention.py reads `audit_retentiondays` (category site, default 0). A second `audit_retention_days` was seeded under category audit with a default of 90, and the Audit & Logging page wrote to that one - so an administrator set a period, reloaded, saw it persisted, and nothing ever consulted it. Retention stayed off. The page now writes the key that is read, the duplicate is out of the seed list, and migration 7d39 carries a value an operator actually set on the dead key over to the live one before deleting it: their intent was "keep 30 days", and the fix should not silently discard it. The page's copy also claimed logs "will be automatically purged", which nothing does - purging is manual, from the site settings page or `flask audit purge --apply`. The Authentication page offered a full SAML configuration - metadata URL, entity id, ACS URL, auto-create, admin group - and reported "Configured - SAML login available" once two fields held text. There is no SAML anywhere in the codebase: no login route, no ACS, no assertion handling. SAML IS planned, so the settings stay and the page keeps them; what changes is that it now says plainly that nothing consumes them yet, and the status line never claims availability. The dangerous control there was "Allow Local Login". Turning it off did nothing at all, so an administrator could believe they had closed local password login while it stayed wide open. It is locked on until there is another way in, and says why. CONFIG.md marks the whole block reserved rather than describing it as working configuration. Scope: product --- docs/CONFIG.md | 32 ++++--- frontend/src/assets/style.css | 12 +++ frontend/src/views/settings/AuditSettings.vue | 14 +-- .../views/settings/AuthenticationSettings.vue | 43 ++++++--- .../7d39_audit_retention_key_merge.py | 95 +++++++++++++++++++ shopdb/core/api/settings.py | 15 ++- tests/test_core/test_audit_retention_key.py | 56 +++++++++++ 7 files changed, 230 insertions(+), 37 deletions(-) create mode 100644 migrations/versions/7d39_audit_retention_key_merge.py create mode 100644 tests/test_core/test_audit_retention_key.py diff --git a/docs/CONFIG.md b/docs/CONFIG.md index e5d6ad6a..db5daaed 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -138,6 +138,7 @@ read back through the API. | `setup_complete` | `false` | Set true once the first-run wizard finishes; gates the `/setup` route. | | `employee_directory_mode` | `selfhosted` | `selfhosted` (tables in this app) or `external` (a separate HR database, see `EMPLOYEE_DB_*`). | | `usb_directory_mode` | `selfhosted` | `selfhosted` or `external` (a separate `cmmc_usb` database, see `CMMC_USB_DB_*`). | +| `audit_retentiondays` | `0` | Delete audit entries older than this many days. 0 keeps everything forever. Saving a period deletes nothing by itself: purge from this page or run `flask audit purge --apply`. | | `site_base_url` | (empty) | Public base URL for QR codes and absolute links. Scheme + host, optionally including the subpath a site is served under (`https://host/shopdb`). Blank = use the browsing origin. | | `facility_name` | (empty) | Facility name in the dashboard header. Blank = frontend falls back to `ShopDB`. | | `pc_access_domain` | `device.geaerospace.net` | Domain appended to a PC hostname for remote-access links. Blank = hostname as-is. | @@ -287,23 +288,32 @@ Three flows use it: ### audit -| Key | Default | Notes | -|-----|---------|-------| -| `audit_retention_days` | `90` | Days to retain audit logs (0 = keep forever). | +Retention lives in the `site` section as `audit_retentiondays`. It was +documented here as `audit_retention_days` as well, which was a second key that +nothing read - the settings page wrote to it and retention stayed off. One key +now, and the page writes that one. ### auth +**SAML is not implemented yet.** These keys exist so a site can be configured +ahead of the feature, and the settings page says so on screen. Nothing reads +them today: there is no SAML login route and no assertion is accepted, so +sign-in is local accounts only whatever is set here. In particular +`saml_allow_local_login` does NOT disable password login - the toggle is locked +on until there is another way in, because a control that appears to close the +local door while it stays open is worse than no control. + | Key | Default | Notes | |-----|---------|-------| -| `saml_enabled` | `false` | Enable SAML SSO. | -| `saml_idp_metadata_url` | (empty) | SAML IdP metadata URL. | -| `saml_entity_id` | (empty) | SAML SP entity id. | -| `saml_acs_url` | (empty) | SAML Assertion Consumer Service URL. | -| `saml_allow_local_login` | `true` | Allow local username/password login when SAML is on. | -| `saml_auto_create_users` | `true` | Auto-create users on first SAML login. | -| `saml_admin_group` | (empty) | SAML group name that grants the admin role. | +| `saml_enabled` | `false` | Reserved. Enable SAML SSO once it ships. | +| `saml_idp_metadata_url` | (empty) | Reserved. SAML IdP metadata URL. | +| `saml_entity_id` | (empty) | Reserved. SAML SP entity id. | +| `saml_acs_url` | (empty) | Reserved. SAML Assertion Consumer Service URL. | +| `saml_allow_local_login` | `true` | Reserved, and locked on. Local login cannot be turned off while it is the only way in. | +| `saml_auto_create_users` | `true` | Reserved. Auto-create users on first SAML login. | +| `saml_admin_group` | (empty) | Reserved. SAML group name that grants the admin role. | -**Personal API tokens.** Besides login JWTs and SAML, a user may create +**Personal API tokens.** Besides login JWTs, a user may create personal API tokens (PATs) for scripts and integrations, from Settings > API Tokens (or `POST /api/apitokens`). A PAT is sent like a JWT (`Authorization: Bearer shopdb_pat_...`), authenticates as its owning user diff --git a/frontend/src/assets/style.css b/frontend/src/assets/style.css index 30b8d035..e3f6afff 100644 --- a/frontend/src/assets/style.css +++ b/frontend/src/assets/style.css @@ -1153,6 +1153,18 @@ td.actions .btn + .btn { margin: 0 0 1rem 0; line-height: 1.5; } +/* A settings block whose feature is not built yet. Reads as a warning rather + than body text, because the failure it prevents is an administrator + believing a control does something. */ +.setting-notice { + border-left: 3px solid var(--warning); + background: var(--bg); + color: var(--text); + font-size: 0.9rem; + line-height: 1.5; + margin: 0 0 1rem 0; + padding: 0.6rem 0.8rem; +} .settings-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(180px, 1fr)); diff --git a/frontend/src/views/settings/AuditSettings.vue b/frontend/src/views/settings/AuditSettings.vue index 7831da07..b4c1f132 100644 --- a/frontend/src/views/settings/AuditSettings.vue +++ b/frontend/src/views/settings/AuditSettings.vue @@ -8,8 +8,10 @@

Audit Log Retention

- Configure how long audit logs are retained. Older logs will be automatically purged. - Set to 0 to keep logs indefinitely. + How long audit entries are kept. 0 keeps everything forever, which is + the default. Saving a period does NOT delete anything by itself: + purge from Settings > Site & Facility, or run + flask audit purge --apply on the server.

@@ -17,13 +19,13 @@ Retention Period (days) - 0 = keep forever, recommended: 90 days + 0 = keep forever
diff --git a/frontend/src/views/settings/AuthenticationSettings.vue b/frontend/src/views/settings/AuthenticationSettings.vue index 4bc673ab..4dfa58ee 100644 --- a/frontend/src/views/settings/AuthenticationSettings.vue +++ b/frontend/src/views/settings/AuthenticationSettings.vue @@ -7,9 +7,16 @@

SAML Single Sign-On

+

+ Not implemented yet. These settings are stored so a + site can be configured ahead of the feature landing, but nothing + consumes them today: there is no SAML login route, and no assertion + is accepted. Sign-in is local accounts only, whatever is set here. +

- Enable SAML SSO for enterprise authentication. Users can sign in using your organization's - identity provider (Azure AD, Okta, etc.). Local login can remain enabled as a fallback. + When it ships, users will sign in through your organization's identity + provider (Azure AD, Okta, etc.), with local login available as a + fallback.

@@ -82,19 +89,26 @@
+
- Keep enabled for admin fallback access + + Always on for now. Local accounts are the only way in until SAML + sign-in ships, so this cannot be turned off yet. +
@@ -148,15 +162,20 @@ const { // Per-page connection status (stays with the page, not the composable). const samlStatus = computed(() => { if (!settings.saml_enabled) return 'inactive' - if (!settings.saml_idp_metadata_url || !settings.saml_entity_id) return 'warning' + // 'pending' rather than a healthy state even when every field is filled in: + // nothing is connected until the feature exists. return 'pending' }) +// Never says "available". It used to report "Configured - SAML login +// available" as soon as two fields held text, which is a claim about a feature +// that does not exist: an administrator could reasonably read that as SSO +// being live and stop issuing local accounts. const samlMessage = computed(() => { - if (!settings.saml_enabled) return 'Disabled - using local authentication only' - if (!settings.saml_idp_metadata_url) return 'IdP metadata URL not configured' - if (!settings.saml_entity_id) return 'Entity ID not configured' - return 'Configured - SAML login available' + if (!settings.saml_enabled) return 'Off - local accounts only' + if (!settings.saml_idp_metadata_url) return 'IdP metadata URL not set' + if (!settings.saml_entity_id) return 'Entity ID not set' + return 'Settings saved - waiting on SAML support, local accounts still in use' }) onMounted(loadSettings) diff --git a/migrations/versions/7d39_audit_retention_key_merge.py b/migrations/versions/7d39_audit_retention_key_merge.py new file mode 100644 index 00000000..3ac412dd --- /dev/null +++ b/migrations/versions/7d39_audit_retention_key_merge.py @@ -0,0 +1,95 @@ +"""Merge the second audit-retention setting into the one that is actually read. + +Two keys existed for one setting. shopdb/core/services/auditretention.py reads +`audit_retentiondays` (seeded under category 'site', default 0). A second key +`audit_retention_days` was seeded under category 'audit' with a default of 90, +and the Audit & Logging settings page wrote to that one - so an administrator +set a retention period, reloaded, saw it persisted, and nothing ever consulted +it. Retention stayed off while the page said otherwise. + +The page now writes the key that is read, and the duplicate is out of the seed +list. This handles the sites that already have it: if an operator set a real +value on the dead key and never set the live one, that intent is carried over +rather than dropped. Their intent was "keep 30 days", and the fix should not +silently discard it. + +Not carried over when the live key already holds a non-default value - that one +was set through the page that worked, and it wins. + +The dead key is then deleted. Leaving it would preserve the ambiguity that +caused this, and nothing reads it. + +Revision ID: 7d39_audit_retention_key_merge +Revises: 7d38_widen_importbatch_status +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d39_audit_retention_key_merge' +down_revision = '7d38_widen_importbatch_status' +branch_labels = None +depends_on = None + +DEAD = 'audit_retention_days' +LIVE = 'audit_retentiondays' + + +def _value(bind, key): + return bind.execute( + sa.text('SELECT value FROM settings WHERE `key` = :key' + if bind.dialect.name == 'mysql' + else 'SELECT value FROM settings WHERE "key" = :key'), + {'key': key}).scalar() + + +def upgrade(): + bind = op.get_bind() + if not sa.inspect(bind).has_table('settings'): + return + + dead = _value(bind, DEAD) + if dead is None: + return + + live = _value(bind, LIVE) + # '90' was the dead key's seeded default, so it is not evidence of intent. + operator_set_dead = dead not in (None, '', '90') + live_is_default = live in (None, '', '0') + + if operator_set_dead and live_is_default: + if live is None: + bind.execute(sa.text( + 'INSERT INTO settings (`key`, value, valuetype, category, ' + 'description, createddate, modifieddate, isactive) VALUES ' + '(:key, :value, :valuetype, :category, :description, ' + 'NOW(), NOW(), 1)' + if bind.dialect.name == 'mysql' else + 'INSERT INTO settings ("key", value, valuetype, category, ' + 'description, createddate, modifieddate, isactive) VALUES ' + '(:key, :value, :valuetype, :category, :description, ' + "datetime('now'), datetime('now'), 1)"), + {'key': LIVE, 'value': dead, 'valuetype': 'integer', + 'category': 'site', + 'description': 'Delete audit log entries older than this many ' + 'days. 0 keeps everything forever.'}) + else: + bind.execute(sa.text( + 'UPDATE settings SET value = :value WHERE `key` = :key' + if bind.dialect.name == 'mysql' else + 'UPDATE settings SET value = :value WHERE "key" = :key'), + {'value': dead, 'key': LIVE}) + + bind.execute(sa.text( + 'DELETE FROM settings WHERE `key` = :key' + if bind.dialect.name == 'mysql' else + 'DELETE FROM settings WHERE "key" = :key'), {'key': DEAD}) + + +def downgrade(): + """Deliberately a no-op. + + Re-creating a key that nothing reads, to sit beside the one that works, + would restore the defect. The live setting keeps whatever value it holds. + """ diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index c314688a..063e50ed 100644 --- a/shopdb/core/api/settings.py +++ b/shopdb/core/api/settings.py @@ -1460,14 +1460,13 @@ def build_default_settings(): 'Incoming Webhook), adaptivecard (Teams Workflow), ' 'or json (generic {title,text})' }, - # Audit log settings - { - 'key': 'audit_retention_days', - 'value': '90', - 'valuetype': 'integer', - 'category': 'audit', - 'description': 'Number of days to retain audit logs (0 = keep forever)' - }, + # Audit log retention lives at 'audit_retentiondays' (category site), + # defined above and read by shopdb/core/services/auditretention.py. A + # second key 'audit_retention_days' was seeded here with a different + # default and a different category, and the Audit & Logging page wrote + # to THAT one - so the page saved a value, read it back, and nothing + # ever consulted it. Removed rather than aliased: two keys for one + # setting is how this happened. # Authentication settings { 'key': 'saml_enabled', diff --git a/tests/test_core/test_audit_retention_key.py b/tests/test_core/test_audit_retention_key.py new file mode 100644 index 00000000..fe8d09e2 --- /dev/null +++ b/tests/test_core/test_audit_retention_key.py @@ -0,0 +1,56 @@ +"""One setting, one key. + +Audit retention had two: shopdb/core/services/auditretention.py reads +`audit_retentiondays`, and the Audit & Logging settings page wrote +`audit_retention_days`. The page saved, reloaded, showed the value back, and +nothing ever consulted it - retention stayed off while the screen said it was +on. A settings page that saves a key nothing reads is worse than a missing +feature, because it tells an administrator the site is configured when it is +not. +""" +import json +import re +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] + + +def _seeded_keys(): + """Every setting key in the DEFAULTS list, read statically.""" + source = (REPO / 'shopdb' / 'core' / 'api' / 'settings.py').read_text() + return re.findall(r"^\s*'key': '([a-z0-9_]+)'", source, re.M) + + +def test_only_one_audit_retention_key_is_seeded(): + keys = _seeded_keys() + retention = sorted({key for key in keys if 'retention' in key}) + assert retention == ['audit_retentiondays'], retention + + +def test_the_service_and_the_settings_pages_use_the_same_key(): + """The three places that mention retention must agree: the service that + reads it, and both settings pages that write it.""" + service = (REPO / 'shopdb' / 'core' / 'services' / 'auditretention.py').read_text() + assert "audit_retentiondays" in service + assert "audit_retention_days" not in service + + for page in ('AuditSettings.vue', 'SiteSettings.vue'): + text = (REPO / 'frontend' / 'src' / 'views' / 'settings' / page).read_text() + if 'retention' not in text.lower(): + continue + assert 'audit_retention_days' not in text, page + assert 'audit_retentiondays' in text, page + + +def test_the_retention_setting_actually_drives_the_report(client, db, + auth_headers): + """End to end: set it through the API the page uses, and read it back from + the endpoint that decides what a purge would remove.""" + saved = client.put('/api/settings/audit_retentiondays', + json={'value': 30}, headers=auth_headers) + assert saved.status_code == 200, saved.get_json() + + report = client.get('/api/auditlogs/retention', headers=auth_headers) + assert report.status_code == 200, report.get_json() + assert report.get_json()['data']['retentiondays'] == 30, \ + 'the setting the page writes must be the one the service reads'