feat(settings): close PMS-113 - unify module-config, add category endpoints + value validation, gate optional modules at runtime #87
No reviewers
Labels
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
psa-systems/mokosh-server!87
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/pms-113-closeout"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PMS-113 ('Story: Settings and module configuration') had three marked-resolved subtasks but the audit revealed real gaps in 5 of 6 ACs. This PR closes all 5.
AC1 (category + per-key tenant_settings + value validation):
Adds GET /api/v1/settings/:category and GET/PUT/DELETE /api/v1/settings/:category/:key. The PUT body is the minimal shape {value: }; the category and key come from the URL so the SPA never repeats them in the payload. A hand-rolled validate_setting_value match in src/modules/settings/models.rs rejects malformed known (category, key) shapes with 422 (e.g. branding.primary_color must be #abcdef, billing_prefs.currency must be 3 uppercase letters, ticketing.auto_close_resolved_after_days must be 1..=90). Unknown (category, key) pairs are accepted with a tracing::warn! so the SPA can ship new knobs without a server-side change.
AC2 (single canonical module-config route):
The settings module is now the single canonical writer for module_config. SettingsService::{get,upsert}_module_config are the source of truth. TenantService::{get,update}_module_config (PMS-21's duplicate SQL) are deleted; the /api/v1/tenants/:id/modules/:module handlers in src/modules/tenants/routes.rs now delegate to SettingsService through an Arc threaded into TenantRouterState. The authz check (super_admin cross-tenant; tenant admin own-tenant) stays on the tenants surface. The SettingsService Arc is constructed once in create_api_router and shared between settings_routes, tenant_routes, and the Extension layer that RequireModuleEnabled reads.
AC3 (runtime module gating, the keystone):
New extractor RequireModuleEnabled<G: ModuleGate> in src/modules/auth/middleware.rs. A blanket FromRequestParts impl authenticates the caller and then queries SettingsService::is_module_enabled(tenant_id, G::NAME); when the module is disabled it returns 404 NotFound so a probing client cannot distinguish a disabled module from an unmounted route. The SettingsService instance is fetched from the request extensions (added at the api_v1 router level via .layer(axum::Extension(settings_service))).
A gated_module! macro declares one unit struct + one ModuleGate impl + one RequireFoo type alias per gateable module. Nine per-module gates ship: RequireBilling (billing), RequireProjects (projects), RequireCalendar (calendar), RequireContracts (contracts), RequireAssets (assets), RequireKnowledgeBase (knowledge_base), RequireRmm (rmm_integration), RequireReports (reports), RequireTimeTracking (time_tracking).
A.4 sweep: every RequireAuth(...): RequireAuth handler argument across all 9 optional-module routes is replaced with the module-specific gate destructuring (e.g. RequireBilling { user, .. }: RequireBilling). 142 sites across 9 modules. Core modules (ticketing, contacts, notifications) stay unconditional. Portal authentication runs on its own auth path and is not affected.
AC4 (defaults + soft fallback):
SettingsService::get_module_config now returns ModuleConfigResponse::default_for(module) when no row exists, matching the soft-default behaviour the tenants-side helper already had pre-removal. ModuleConfigResponse.id is now Option so the soft default carries None; the persisted form still gets Some(id). Migration 023's per-default-tenant seed is unchanged (all 13 modules seeded with is_enabled=TRUE for the default tenant).
AC5 (authz):
Settings module endpoints scope to user.tenant_id implicitly (no :tenant_id in the path), so cross-tenant access is structurally impossible. RequireAdmin gates writes. The tenants-side /tenants/:id/modules/:module surface keeps its existing super_admin-cross-tenant + tenant-admin-own-tenant authz from PMS-21.
AC6 (tests):
New tests/settings.rs has 10 integration tests covering:
The PMS-21 module-config tests in tests/tenants.rs continue to pass against the new delegation path; the old hard-NotFound assertions were never written there because those tests already used the soft-default behaviour.
Verified locally:
cargo test --test settings against a live postgres is deferred to CI; the dev-01 dev stack is currently down per the user's earlier instruction.
Plan + closeout notes: docs/mokosh-upgrade/auto-stories/PMS-113/plan.md.
Closes PMS-113.
PMS-113 ('Story: Settings and module configuration') had three marked-resolved subtasks but the audit revealed real gaps in 5 of 6 ACs. This PR closes all 5. AC1 (category + per-key tenant_settings + value validation): Adds GET /api/v1/settings/:category and GET/PUT/DELETE /api/v1/settings/:category/:key. The PUT body is the minimal shape {value: <any>}; the category and key come from the URL so the SPA never repeats them in the payload. A hand-rolled validate_setting_value match in src/modules/settings/models.rs rejects malformed known (category, key) shapes with 422 (e.g. branding.primary_color must be #abcdef, billing_prefs.currency must be 3 uppercase letters, ticketing.auto_close_resolved_after_days must be 1..=90). Unknown (category, key) pairs are accepted with a tracing::warn! so the SPA can ship new knobs without a server-side change. AC2 (single canonical module-config route): The settings module is now the single canonical writer for module_config. SettingsService::{get,upsert}_module_config are the source of truth. TenantService::{get,update}_module_config (PMS-21's duplicate SQL) are deleted; the /api/v1/tenants/:id/modules/:module handlers in src/modules/tenants/routes.rs now delegate to SettingsService through an Arc threaded into TenantRouterState. The authz check (super_admin cross-tenant; tenant admin own-tenant) stays on the tenants surface. The SettingsService Arc is constructed once in create_api_router and shared between settings_routes, tenant_routes, and the Extension layer that RequireModuleEnabled reads. AC3 (runtime module gating, the keystone): New extractor RequireModuleEnabled<G: ModuleGate> in src/modules/auth/middleware.rs. A blanket FromRequestParts impl authenticates the caller and then queries SettingsService::is_module_enabled(tenant_id, G::NAME); when the module is disabled it returns 404 NotFound so a probing client cannot distinguish a disabled module from an unmounted route. The SettingsService instance is fetched from the request extensions (added at the api_v1 router level via .layer(axum::Extension(settings_service))). A gated_module! macro declares one unit struct + one ModuleGate impl + one RequireFoo type alias per gateable module. Nine per-module gates ship: RequireBilling (billing), RequireProjects (projects), RequireCalendar (calendar), RequireContracts (contracts), RequireAssets (assets), RequireKnowledgeBase (knowledge_base), RequireRmm (rmm_integration), RequireReports (reports), RequireTimeTracking (time_tracking). A.4 sweep: every RequireAuth(...): RequireAuth handler argument across all 9 optional-module routes is replaced with the module-specific gate destructuring (e.g. RequireBilling { user, .. }: RequireBilling). 142 sites across 9 modules. Core modules (ticketing, contacts, notifications) stay unconditional. Portal authentication runs on its own auth path and is not affected. AC4 (defaults + soft fallback): SettingsService::get_module_config now returns ModuleConfigResponse::default_for(module) when no row exists, matching the soft-default behaviour the tenants-side helper already had pre-removal. ModuleConfigResponse.id is now Option<Uuid> so the soft default carries None; the persisted form still gets Some(id). Migration 023's per-default-tenant seed is unchanged (all 13 modules seeded with is_enabled=TRUE for the default tenant). AC5 (authz): Settings module endpoints scope to user.tenant_id implicitly (no :tenant_id in the path), so cross-tenant access is structurally impossible. RequireAdmin gates writes. The tenants-side /tenants/:id/modules/:module surface keeps its existing super_admin-cross-tenant + tenant-admin-own-tenant authz from PMS-21. AC6 (tests): New tests/settings.rs has 10 integration tests covering: - tenant_setting_round_trip_persists - tenant_setting_value_validation_rejects_bad_shape - tenant_setting_unknown_category_accepted_with_warn - get_settings_by_category_lists_only_that_category - tenant_setting_delete_then_get_returns_404 - module_config_settings_surface_round_trip - module_config_tenants_surface_delegates_to_settings (PUT on /tenants/.../modules then GET on /settings/modules sees the same row) - module_config_get_missing_returns_soft_default (AC4 keystone) - disabled_module_returns_404_on_route_access (AC3 keystone) - enabled_module_response_unchanged (sanity) The PMS-21 module-config tests in tests/tenants.rs continue to pass against the new delegation path; the old hard-NotFound assertions were never written there because those tests already used the soft-default behaviour. Verified locally: - cargo check --tests - cargo check --no-default-features --features server,multi-tenant - cargo check --no-default-features --features server,single-tenant - cargo clippy --tests -- -Dwarnings - cargo fmt --all --check cargo test --test settings against a live postgres is deferred to CI; the dev-01 dev stack is currently down per the user's earlier instruction. Plan + closeout notes: docs/mokosh-upgrade/auto-stories/PMS-113/plan.md. Closes PMS-113.