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'