fix(flags): close CRM/accounting feature-gating gaps from the audit

A sweep of every CRM/accounting toggle found surfaces still reachable with their flag OFF. Adds a shared requireFeatureFlag middleware (the two existing per-file copies predate it) and closes the gaps: - Hours logging: only createEntry checked the flag — edit/delete/bill and the list/summary routes were permission-only. Gate all six /hour-entries routes on the hoursLogging master so a disabled feature can't be read, mutated, or invoiced via a direct API hit. - Installment plans: PUT /deals/:uuid/installment-plan mutates invoices but wasn't bills-gated; add requireFeatureFlag('bills'). - Customer invoice PDF: /invoices/:id/pdf lacked the feature_bills check the list + quotes routes have. Also fixes the quotes-PDF gate, which read req.customer.feature_quotes (never populated → silent no-op). - Customer contracts: /contracts + /contracts/:id/pdf were gated by neither the master nor a per-customer column. Per-customer contracts override (the missing counterpart): - Migration 131 adds customer_accounts.feature_contracts, default TRUE so existing customers keep their Contracts tab (preserve-visuals). - Effective resolver now contractsMaster AND feature_contracts; admin detail page gains the toggle; service/validator/serializer wired. Cleanups: - Drop stale `taxReport` from the sidebar's Clients-reveal list (Tax moved to Accounting); add the missing `projects` so it mirrors the context derivation. - SettingsPage tab-snap effect now depends on flags.accounting. - Fix stale taxReport "forced off when bills off" comment (it's accounting).
2026-06-16 13:12:26 +02:00
parent 873be910a5
commit 03fa3d8296
11 changed files with 141 additions and 17 deletions
@@ -0,0 +1,32 @@
+/**
+ * requireFeatureFlag(key, code?) — 403 when the named `feature_flags` row is off.
+ *
+ * Belt-and-braces gate for admin routes whose feature can be toggled in
+ * Settings → Features. The frontend hides disabled surfaces, but a direct API
+ * hit must still be refused so a disabled feature is never actable. Mirrors the
+ * truthy logic feature_flags uses everywhere (true | 1 | '1').
+ *
+ * Several route files (adminLedger, adminExpenses) predate this and define an
+ * identical local `requireFlag`; new gates should import this instead.
+ */
+const { db } = require('../database/db');
+
+function requireFeatureFlag(key, code) {
+  return async (req, res, next) => {
+    try {
+      const row = await db('feature_flags').where({ key }).first();
+      const enabled = row && (row.value === true || row.value === 1 || row.value === '1');
+      if (!enabled) {
+        return res.status(403).json({
+          error: `${key} feature is disabled`,
+          code: code || `${key.replace(/([a-z])([A-Z])/g, '$1_$2').toUpperCase()}_DISABLED`,
+        });
+      }
+      return next();
+    } catch (err) {
+      return next(err);
+    }
+  };
+}
+
+module.exports = { requireFeatureFlag };
@@ -10,6 +10,13 @@ const express = require('express');
 const { body, param, query } = require('express-validator');
 const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
+const { requireFeatureFlag } = require('../middleware/requireFeatureFlag');
+
+// Hour-entry routes are gated by the hoursLogging master so a direct API hit
+// can't read/edit/delete/bill logged hours while the feature is off (the
+// frontend already hides the surface). Per-customer enforcement stays in
+// customerHoursService.createEntry.
+const requireHoursLogging = requireFeatureFlag('hoursLogging', 'HOURS_LOGGING_DISABLED');
 const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers');
 const customerAccountsService = require('../services/customerAccountsService');
 const customerHoursService = require('../services/customerHoursService');
@@ -66,6 +73,9 @@ function transformCustomer(c) {
    // set one; the editor surfaces it as an empty input and forces a
    // per-entry override on every logged block.
    featureHoursLogging: c.feature_hours_logging === true || c.feature_hours_logging === 1,
+    // Contracts override (migration 131). Opt-out: absent column (older row /
+    // un-selected) reads as ON so existing customers keep the Contracts tab.
+    featureContracts: c.feature_contracts === undefined ? true : (c.feature_contracts === true || c.feature_contracts === 1),
    hourlyRateMinor: c.hourly_rate_minor != null ? Number(c.hourly_rate_minor) : null,
    // Per-customer Skonto opt-out (migration 112). When true, none of
    // this customer's invoices qualify for an early-payment discount,
@@ -387,6 +397,7 @@ router.put('/:id', [
  body('feature_calendar').optional().isBoolean(),
  body('feature_quotes').optional().isBoolean(),
  body('feature_bills').optional().isBoolean(),
+  body('feature_contracts').optional().isBoolean(),
  // Hours logging (migration 129).
  body('feature_hours_logging').optional().isBoolean(),
  body('hourly_rate_minor').optional({ nullable: true }).isInt({ min: 0 }),
@@ -541,6 +552,7 @@ router.put('/:id/events', [
 // can't collide with the int-validated :id pattern.
 router.get('/hour-entries/unbilled-summary', [
  adminAuth,
+  requireHoursLogging,
  requirePermission('customers.view'),
 ], handleAsync(async (req, res) => {
  const summary = await customerHoursService.getUnbilledSummaryByCustomer();
@@ -549,6 +561,7 @@ router.get('/hour-entries/unbilled-summary', [

 router.get('/:id/hour-entries', [
  adminAuth,
+  requireHoursLogging,
  requirePermission('customers.view'),
  param('id').isInt({ min: 1 }),
  query('status').optional().isIn(['unbilled', 'billed', 'cancelled']),
@@ -563,6 +576,7 @@ router.get('/:id/hour-entries', [

 router.post('/:id/hour-entries', [
  adminAuth,
+  requireHoursLogging,
  // Migration 134 — hour entries are customer-scoped writes; same scope
  // as customer record edits, narrower than invite/create.
  requirePermission('customers.edit'),
@@ -585,6 +599,7 @@ router.post('/:id/hour-entries', [

 router.put('/:id/hour-entries/:entryId', [
  adminAuth,
+  requireHoursLogging,
  requirePermission('customers.edit'),
  param('id').isInt({ min: 1 }),
  param('entryId').isInt({ min: 1 }),
@@ -605,6 +620,7 @@ router.put('/:id/hour-entries/:entryId', [

 router.delete('/:id/hour-entries/:entryId', [
  adminAuth,
+  requireHoursLogging,
  requirePermission('customers.edit'),
  param('id').isInt({ min: 1 }),
  param('entryId').isInt({ min: 1 }),
@@ -619,6 +635,7 @@ router.delete('/:id/hour-entries/:entryId', [

 router.post('/:id/hour-entries/bill', [
  adminAuth,
+  requireHoursLogging,
  requirePermission('customers.edit'),
  param('id').isInt({ min: 1 }),
 ], handleAsync(async (req, res) => {
@@ -21,6 +21,7 @@ const express = require('express');
 const { param, body } = require('express-validator');
 const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
+const { requireFeatureFlag } = require('../middleware/requireFeatureFlag');
 const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers');
 const dealsService = require('../services/dealsService');
 const invoiceService = require('../services/invoiceService');
@@ -56,6 +57,9 @@ router.get(
 */
 router.put(
  '/:uuid/installment-plan',
+  // Mutates invoices — gate on the bills flag like every other invoice
+  // write path, so installment plans can't be reshaped with Bills off.
+  requireFeatureFlag('bills', 'BILLS_DISABLED'),
  requirePermission('bills.manage'),
  [
    param('uuid').isString().isLength({ min: 32, max: 36 }),
@@ -48,10 +48,9 @@ const KNOWN_FLAGS = [
  // payment-check email flow without waiting 30 days, etc.).
  // Strictly opt-in.
  'crmDevelopment',
-  // Tax / Steuer report sub-tab under Clients. Independent toggle so
-  // admins who use Bills but don't need the tax export (or aren't
-  // ready to enable it yet) can leave it off. Forced off when `bills`
-  // is off (no invoices → nothing to report).
+  // Tax / Steuer report — an Accounting sub-feature (moved out of CRM).
+  // Independent of `bills`; forced off when the `accounting` master is off
+  // (see applyDependencyRules below).
  'taxReport',
  // Hours logging (migration 129). Master switch for the per-customer
  // Hours card + the auto-append into monthly draft / "Bill these
@@ -550,11 +550,15 @@ router.get('/invoices', customerAuth, async (req, res) => {
 */
 router.get('/quotes/:id/pdf', customerAuth, async (req, res) => {
  try {
-    // Feature-gate identically to /quotes (list endpoint).
-    if (req.customer.feature_quotes === false || req.customer.feature_quotes === 0 || req.customer.feature_quotes === '0') {
-      return res.status(403).json({ error: 'Quotes are disabled for this account' });
-    }
    const { db: dbi } = require('../database/db');
+    // Feature-gate identically to /quotes (list endpoint). req.customer does
+    // NOT carry feature_* columns (customerAuth only selects identity), so we
+    // read the row here — the previous req.customer.feature_quotes check was a
+    // silent no-op (always undefined).
+    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
+    if (!account || account.feature_quotes === false || account.feature_quotes === 0) {
+      return res.status(403).json({ error: 'Quotes are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
+    }
    const quote = await dbi('quotes')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -584,6 +588,12 @@ router.get('/quotes/:id/pdf', customerAuth, async (req, res) => {
 router.get('/invoices/:id/pdf', customerAuth, async (req, res) => {
  try {
    const { db: dbi } = require('../database/db');
+    // Feature-gate identically to /invoices (list endpoint) — a direct hit must
+    // not download an invoice PDF when Bills is disabled for the account.
+    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
+    if (!account || account.feature_bills === false || account.feature_bills === 0) {
+      return res.status(403).json({ error: 'Invoices are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
+    }
    const invoice = await dbi('invoices')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -623,6 +633,12 @@ router.get('/contracts', customerAuth, async (req, res) => {
      // Feature not migrated on this install yet.
      return res.json({ contracts: [] });
    }
+    // Per-customer contracts gate (migration 131) — mirrors /quotes + /invoices
+    // so a direct hit is refused when Contracts is off for the account.
+    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
+    if (!account || account.feature_contracts === false || account.feature_contracts === 0) {
+      return res.status(403).json({ error: 'Contracts are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
+    }
    const rows = await dbi('contracts')
      .where({ customer_account_id: req.customer.id })
      .whereNotIn('status', ['draft'])
@@ -680,6 +696,10 @@ router.get('/contracts/:id/pdf', customerAuth, async (req, res) => {
    if (!(await dbi.schema.hasTable('contracts'))) {
      return res.status(404).json({ error: 'Contract not found' });
    }
+    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
+    if (!account || account.feature_contracts === false || account.feature_contracts === 0) {
+      return res.status(403).json({ error: 'Contracts are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
+    }
    const contract = await dbi('contracts')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -480,6 +480,7 @@ async function listCustomers({ search } = {}) {
      'customer_accounts.feature_quotes',
      'customer_accounts.feature_bills',
      'customer_accounts.feature_hours_logging',
+      'customer_accounts.feature_contracts',
      'customer_accounts.hourly_rate_minor',
      'customer_accounts.last_login',
      'customer_accounts.created_at',
@@ -548,6 +549,9 @@ async function updateCustomer(id, updates, updatedByAdminId) {
    // Per-customer feature flags (#354 follow-up). Booleans below are
    // coerced via formatBoolean for SQLite compatibility.
    'feature_calendar', 'feature_quotes', 'feature_bills', 'feature_hours_logging',
+    // Per-customer contracts override (migration 131). Defaults TRUE so
+    // existing customers keep their Contracts tab.
+    'feature_contracts',
    // CRM billing cadence (migration 102). 'per_event' (default) keeps
    // each invoice firing on its own schedule; monthly/quarterly snap
    // every scheduled invoice to billing_cycle_day of the next period.
@@ -570,6 +574,7 @@ async function updateCustomer(id, updates, updatedByAdminId) {
      } else if (
        f === 'feature_calendar' || f === 'feature_quotes'
        || f === 'feature_bills' || f === 'feature_hours_logging'
+        || f === 'feature_contracts'
        || f === 'skonto_disabled'
      ) {
        allowed[f] = formatBoolean(updates[f]);
@@ -1267,9 +1272,9 @@ async function getEffectiveFeaturesForCustomer(customerOrId) {
  // for hours, so we skip the third gate the bills/quotes use.
  const hoursMaster = await db('feature_flags').where({ key: 'hoursLogging' }).first();
  const hoursLoggingMaster = hoursMaster ? Boolean(hoursMaster.value) : true;
-  // Contracts (migration 130): no per-customer flag, just the global
-  // feature_flags row. When on, every customer with an active account
-  // sees the Contracts tab on their portal.
+  // Contracts: global feature_flags row AND the per-customer override
+  // (migration 131). feature_contracts defaults TRUE, so existing customers
+  // keep their Contracts tab; an admin can hide it per customer.
  const contractsMaster = await db('feature_flags').where({ key: 'contracts' }).first();
  const contractsEnabled = contractsMaster ? Boolean(contractsMaster.value) : false;
  return {
@@ -1277,7 +1282,7 @@ async function getEffectiveFeaturesForCustomer(customerOrId) {
    quotes:   globals.quotesEnabled   && truthy(customer.feature_quotes),
    bills:    globals.billsEnabled    && truthy(customer.feature_bills),
    hoursLogging: hoursLoggingMaster && truthy(customer.feature_hours_logging),
-    contracts: contractsEnabled,
+    contracts: contractsEnabled && truthy(customer.feature_contracts),
  };
 }