From d291dfc513dff6608a9667168867b1a5df687347 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Thu, 27 Aug 2026 12:14:09 -0400 Subject: [PATCH] Aug 27 - Update free-plan gate, MFA is no longer bypassable via the app --- MULTI_TENANT_PLAN.md | 17 +++++++- app/api/auth.py | 60 +++++++++++++++++++++++++++ app/api/errors.py | 19 +++++++-- app/routes/issues.py | 14 +++++++ app/templates/modern/issues/list.html | 8 ++-- app/tenancy/middleware.py | 48 ++++++++++++++++++++- control/tenant_migrate.py | 60 +++++++++++++++++++++++---- scripts/scratch_bootstrap_test.py | 45 +++++++++++++++++++- 8 files changed, 251 insertions(+), 20 deletions(-) diff --git a/MULTI_TENANT_PLAN.md b/MULTI_TENANT_PLAN.md index cdd1588..95a1491 100644 --- a/MULTI_TENANT_PLAN.md +++ b/MULTI_TENANT_PLAN.md @@ -416,11 +416,26 @@ ST's bare `Length(min=6)`. Porting ST's version would be a downgrade. ### 12.2 Phase log -Migrations `phase41` → `phase52` in `migrations/versions/` are the parity track: +Migrations `phase41` → `phase56` in `migrations/versions/` are the parity track: auditor role, area QR tokens, schedule plan fields, internal handler, frequency ENUM widening, recurrence, end date, parent inspection, follow-up attribution, schedule acknowledgement, and then: +**Aug 2026 tail — ST parity, ported after MT-17.** ST's chain numbers these +differently (its phase51/52/53); MT's was already past those numbers, so match +by NAME: + +| MT | ST | What | +|---|---|---| +| `phase54_user_notif_matrix` | phase51 | Per-account notification overrides for the two customer-side roles. Empty table = everyone inherits the global matrix, so it shipped changing nothing. See CLAUDE.md §27b. | +| `phase55_template_contracts` | phase52 | Forms restricted to specific contracts. **No rows = shared**, which is why it needed no backfill. | +| `phase56_followup_assignee` | phase53 | `inspections.follow_up_assigned_to` — hand a re-inspection to another inspector. NULL = the inspection's own inspector, as before. See CLAUDE.md §27c. | + +Shipped alongside them, without migrations: customer-role management under +`/customers`, bulk actions on the issues/inspections lists, list-filter +preservation, contract-scoped flag-issue assignees, Customer Directors planning +their own inspections, and the support surface serving both customer roles. + **MT-15 — External Inspector role. ✅ DONE** (`phase51_external_inspector`) Adds `external_inspector` to the `users.role` ENUM: an inspector employed by the customer or a third party, with identical capabilities to `inspector` and scoped diff --git a/app/api/auth.py b/app/api/auth.py index 67254ed..9222bd8 100644 --- a/app/api/auth.py +++ b/app/api/auth.py @@ -32,6 +32,7 @@ import logging from flask import Blueprint, request, g from app import db, limiter +from app.tenancy.gates import feature_required from app.models.user import User from app.models.api_token import RefreshToken, DeviceToken from app.api.errors import api_ok, api_error @@ -61,6 +62,13 @@ def _user_payload(user: User) -> dict: @bp.route('/auth/login', methods=['POST']) @limiter.limit('10 per minute; 3 per second') +# The plan gate belongs HERE, not only on the write endpoints. It used to sit +# on POST /inspections and POST /issues alone, so a tenant without mobile API +# access could sign in, sync reference data and let an inspector complete a +# whole inspection on site — and only then get a 403, with the work already +# done and no way to submit it. Refusing at the door is the honest answer. +# Inert in single-tenant mode and for any plan that allows the mobile API. +@feature_required('mobile_api') def login(): """ Authenticate with username + password. @@ -70,10 +78,22 @@ def login(): { "username": "john", "password": "secret", + "mfa_code": "123456", // required IF the account has 2FA on "device_id": "A1B2C3D4...", // UIDevice.identifierForVendor (optional) "device_name": "John's iPhone" // (optional) } + Response 401 — second factor needed + ----------------------------------- + { + "ok": false, + "error": "A verification code is required for this account.", + "mfa_required": true + } + + The password was correct; the client should prompt for the 6-digit code + (or a recovery code) and POST again with `mfa_code`. + Response 200 ------------ { @@ -106,6 +126,41 @@ def login(): if not user.active: return api_error('Account is disabled. Please contact an administrator.', 401) + # ── Two-factor (phase35 parity) ─────────────────────────────────────── + # The web login defers identity to /auth/mfa when an account has TOTP + # enabled. This endpoint did not, so anyone who turned MFA on could skip it + # entirely by signing in through the app — the factor was decorative for + # exactly the accounts that chose to enable it. + # + # Accepts either a TOTP code or a single-use recovery code, the same two + # the web challenge accepts. A missing code is answered with + # `mfa_required: true` so a client can prompt for it rather than treating + # this as a wrong password. + if user.mfa_enabled and user.mfa_secret: + from app.utils.mfa import verify_totp, check_and_consume_recovery + + code = (data.get('mfa_code') or '').strip() + if not code: + logger.info('API login | mfa_required | username=%s', user.username) + return api_error('A verification code is required for this account.', + 401, extra={'mfa_required': True}) + + if not verify_totp(user.mfa_secret, code): + matched, remaining = check_and_consume_recovery( + user.mfa_recovery_codes, code) + if not matched: + logger.warning('API login | mfa_failed | username=%s | ip=%s', + user.username, request.remote_addr) + return api_error('That verification code is not valid.', + 401, extra={'mfa_required': True}) + # Recovery codes are single-use — persist the shortened list before + # any token is issued, so a crash cannot hand out a login while + # leaving the code usable again. + user.mfa_recovery_codes = remaining + db.session.commit() + logger.warning('API login | recovery_code_used | username=%s | ' + 'remaining=%d', user.username, len(remaining)) + device_id = (data.get('device_id') or '')[:64] or None device_name = (data.get('device_name') or '')[:100] or None @@ -154,6 +209,11 @@ def login(): @bp.route('/auth/refresh', methods=['POST']) @limiter.limit('30 per minute; 5 per second') +# Gated too: without it a device that signed in before the plan changed would +# keep rotating tokens forever and never notice it had lost access. +# logout stays open on purpose — a blocked device must still be able to +# surrender its refresh token and clean up. +@feature_required('mobile_api') def refresh(): """ Exchange a valid refresh token for a new access token. diff --git a/app/api/errors.py b/app/api/errors.py index 8934648..715d901 100644 --- a/app/api/errors.py +++ b/app/api/errors.py @@ -31,13 +31,24 @@ def api_ok(data=None, status=200): }), status -def api_error(message: str, status: int = 400): - """Return an error JSON response.""" - return jsonify({ +def api_error(message: str, status: int = 400, extra: dict | None = None): + """Return an error JSON response. + + `extra` merges additional top-level keys into the envelope — for flags a + client must branch on rather than parse out of the message, e.g. + `mfa_required` on a login that needs a second factor. Reserved keys + (ok/data/error) always win, so a caller cannot accidentally rewrite the + envelope's shape. + """ + payload = { 'ok': False, 'data': None, 'error': message, - }), status + } + if extra: + for k, v in extra.items(): + payload.setdefault(k, v) + return jsonify(payload), status # ── Registered error handlers (attached to the api blueprint) ───────────────── diff --git a/app/routes/issues.py b/app/routes/issues.py index 204357f..b0f97ad 100644 --- a/app/routes/issues.py +++ b/app/routes/issues.py @@ -1388,6 +1388,20 @@ def export_pdf(issue_id): # ── Vendor work order dispatch (phase36) ───────────────────────────────────── +# +# NOT LINKED FROM THE UI (Aug 2026). The "Contractor Work Orders" card was +# removed from the issue detail page, so nothing posts here any more. The +# endpoint is kept deliberately rather than deleted: +# +# * it is the ONLY way to create a work order, so removing it would strand +# the public contractor pages (/work-orders/), the model, the email +# template and the phase36 migration — a whole feature, not dead code; +# * tests/test_work_orders.py drives the end-to-end flow through it. +# +# To bring the feature back, restore the card in templates/issues/view.html — +# nothing here needs to change. To retire it for good, remove this route, the +# work_orders blueprint, its templates, the model and those tests together, +# and only once no tokenized links are still outstanding with contractors. @bp.route('//work-order', methods=['POST']) @login_required diff --git a/app/templates/modern/issues/list.html b/app/templates/modern/issues/list.html index febf839..b7f56a5 100644 --- a/app/templates/modern/issues/list.html +++ b/app/templates/modern/issues/list.html @@ -119,9 +119,9 @@ @@ -314,7 +314,7 @@ {% if p %}
  • {{ p }} + href="{{ url_for('issues.index', page=p, issue_id=issue_id_filter, severity=severity_filter, status=status_filter, sla=sla_filter, contract_id=contract_filter, facility_id=facility_filter, date_from=date_from_filter, date_to=date_to_filter, reporter_id=reporter_filter, handler_type=handler_type_filter, unassigned=unassigned_filter) }}">{{ p }}
    • {% else %}
  • …
    • {% endif %} {% endfor %} diff --git a/app/tenancy/middleware.py b/app/tenancy/middleware.py index 115f215..96a60a3 100644 --- a/app/tenancy/middleware.py +++ b/app/tenancy/middleware.py @@ -27,7 +27,8 @@ g.tenant_engine — no teardown handler is needed here. import logging -from flask import g, request, current_app, Response, session, redirect, url_for +from flask import (g, request, current_app, Response, session, redirect, + url_for, jsonify) from app.tenancy.resolver import resolve_tenant from app.tenancy.engine_cache import get_tenant_engine @@ -52,6 +53,26 @@ _UNKNOWN_TENANT_PAGE = ( ) +def _wants_json(): + """True when this request is the mobile API (or explicitly asks for JSON). + + Mirrors gates._is_api_request(). The tenancy and billing gates run BEFORE + any route, so without this they answer an iPad with a 302 to an HTML page: + URLSession follows it, the client decodes the login/billing markup as JSON + and reports "the data couldn't be read". The inspector sees a parse error + instead of "your subscription has expired", and nothing in the app can tell + the two apart. + """ + return (request.path.startswith('/api/') + or request.accept_mimetypes.best == 'application/json') + + +def _json(payload, status): + """Small local responder — the API error helpers live in a blueprint that + is not necessarily importable this early in the request.""" + return jsonify(payload), status + + def _is_exempt(path): if path.startswith('/static/'): return True @@ -158,6 +179,12 @@ def init_tenancy(app): host = (request.host or '').split(':')[0].strip().lower() tenant = resolve_tenant(host) if tenant is None: + if _wants_json(): + # An HTML "Workspace not found" page is unreadable to the iPad + # — it decodes as a parse failure, which looks like a bug in + # the app rather than a wrong/retired server address. + return _json({'ok': False, + 'error': 'Workspace not found for this address.'}, 404) return Response(_UNKNOWN_TENANT_PAGE, status=404, mimetype='text/html') g.tenant = tenant @@ -205,6 +232,18 @@ def init_tenancy(app): # they can still see their plan page and the subscribe button. if not (request.path.startswith('/billing/') or request.path.startswith('/settings/')): + if _wants_json(): + # 402, not a redirect: the caller is a program. + # Distinct from 401 on purpose — the iPad retries a + # 401 by refreshing its token, which would loop + # forever against a billing block. + return _json({ + 'ok': False, + 'error': 'This workspace\'s trial has ended. ' + 'An administrator needs to choose a plan ' + 'before the app can sync again.', + 'billing_required': True, + }, 402) return redirect(url_for('billing.subscribe')) else: days_left = (trial_ends_at - now).days @@ -217,4 +256,11 @@ def init_tenancy(app): return # status == 'cancelled' — block and redirect to subscription page. + if _wants_json(): + return _json({ + 'ok': False, + 'error': 'This workspace is suspended. An administrator needs to ' + 'reactivate the subscription before the app can sync again.', + 'billing_required': True, + }, 402) return redirect(url_for('billing.suspended')) diff --git a/control/tenant_migrate.py b/control/tenant_migrate.py index c3b18f3..fc11c44 100644 --- a/control/tenant_migrate.py +++ b/control/tenant_migrate.py @@ -49,6 +49,17 @@ MIGRATABLE_STATUSES = ('provisioning', 'active', 'suspended') # Squashed baseline that builds the full schema (migrations/versions root). BASELINE_REVISION = '0003_add_user_active' +#: The last revision the squashed baseline actually covers. +#: +#: Everything AFTER this point is existence-guarded (INFORMATION_SCHEMA checks, +#: or an idempotent ENUM MODIFY), so it can be replayed over the baseline +#: safely. Everything BEFORE it is not — several pre-phase33 migrations would +#: collide with the full-schema baseline, which is why bootstrap stamps past +#: them instead of running them. +#: +#: If the baseline is ever re-squashed to a later point, move this with it. +BASELINE_COVERS_THROUGH = 'phase32_device_token_columns' + TenantRef = namedtuple('TenantRef', ['id', 'slug', 'db_uri']) @@ -130,14 +141,34 @@ def upgrade_tenant(tenant, script_location=None, version_locations=None, record_ def bootstrap_tenant(tenant, script_location=None, version_locations=None, baseline_rev=BASELINE_REVISION, record_job=True): - """Build a FRESH tenant database, then mark it current. + """Build a FRESH tenant database, then bring it to head. - Runs the squashed baseline (full schema) only, then `stamp head` — the - historical phase migrations are NOT replayed (several are not idempotent and - would conflict with the full-schema baseline). Use this for brand-new tenant - databases; use upgrade_tenant() for ongoing incremental migrations. + Three steps, and the middle one is the load-bearing part: - Returns the stamped head revision. + 1. `upgrade(baseline)` — the squashed baseline builds the bulk of the + schema in one go. + 2. `stamp(BASELINE_COVERS_THROUGH)` — declare the DB to be at the last + revision the baseline actually covers, WITHOUT running the pre-phase33 + migrations. Those are not idempotent and would collide with the + baseline; skipping them is the whole reason bootstrap exists. + 3. `upgrade(head)` — replay the guarded tail (phase33 onward). Every one + of those checks INFORMATION_SCHEMA before touching anything (the two + ENUM widenings use an idempotent MODIFY), so this adds exactly what + the baseline lacks and skips the rest. + + This used to stop after step 1 and `stamp('head')` instead — claiming the + database was current when it was missing every column and table added since + phase32. Because the baseline was last refreshed around phase33, a tenant + provisioned that way lacked `users.mfa_enabled` and `users.ui_theme`, and + SQLAlchemy emits every mapped column in its SELECT — so the new workspace + could not even log in. It failed at the first query, not at some optional + feature, and only for freshly provisioned tenants (tenant-zero was adopted + in place with a real schema), which is what kept it hidden. + + Use upgrade_tenant() for ongoing incremental migrations of a tenant that + already exists. + + Returns the head revision the database ends up at. """ db_uri = tenant.db_uri cfg = _make_config(db_uri, script_location, version_locations) @@ -153,10 +184,20 @@ def bootstrap_tenant(tenant, script_location=None, version_locations=None, try: with contextlib.redirect_stdout(io.StringIO()): - command.upgrade(cfg, baseline_rev) # build full schema (baseline only) - command.stamp(cfg, 'head') # mark at head without replaying phases + command.upgrade(cfg, baseline_rev) # 1. baseline schema + command.stamp(cfg, BASELINE_COVERS_THROUGH) # 2. skip the unguarded past + command.upgrade(cfg, 'head') # 3. replay the guarded tail applied = current_revision(db_uri) + # A bootstrap that does not end at head has silently produced a broken + # tenant — exactly the failure this sequence exists to prevent. Say so + # here rather than letting it surface as a missing column later. + expected = chain_head(script_location, version_locations) + if applied != expected: + raise RuntimeError( + f'bootstrap ended at {applied!r}, expected head {expected!r} — ' + f'the tenant database is incomplete') + with control_session() as s: t = s.get(Tenant, tenant.id) if t is not None: @@ -166,7 +207,8 @@ def bootstrap_tenant(tenant, script_location=None, version_locations=None, if j is not None: j.status = 'ok' j.finished_at = now_eastern() - j.log = f'bootstrapped (baseline {baseline_rev}) + stamped {applied}' + j.log = (f'bootstrapped (baseline {baseline_rev}, stamped ' + f'{BASELINE_COVERS_THROUGH}, upgraded to {applied})') return applied except Exception as e: diff --git a/scripts/scratch_bootstrap_test.py b/scripts/scratch_bootstrap_test.py index 58334f8..f08f1c3 100644 --- a/scripts/scratch_bootstrap_test.py +++ b/scripts/scratch_bootstrap_test.py @@ -37,14 +37,41 @@ from control.models import Plan, Tenant, ProvisioningJob from control.seed import seed_plans from control.tenant_migrate import bootstrap_tenant, chain_head +# What a COMPLETE tenant database holds: the baseline tables plus everything +# the guarded tail (phase33 onward) adds. This list used to stop at the +# baseline, which is why it passed while bootstrap was producing databases that +# could not serve a login — the expectation encoded the bug. EXPECTED_TABLES = { + # baseline (0003_add_user_active) 'api_device_tokens', 'api_refresh_tokens', 'areas', 'audit_logs', 'broadcasts', 'checklist_items', 'customer_assignments', 'device_registrations', 'facilities', 'facility_score_alerts', 'inspection_results', 'inspection_templates', 'inspections', 'inspector_assignments', 'issue_comments', 'issue_followers', 'issues', 'notification_matrix', 'notification_preferences', 'notifications', 'projects', 'scheduled_reports', 'support_ticket_replies', 'support_tickets', - 'users', + 'tenant_settings', 'users', + # guarded tail + 'inspection_schedules', # phase34 + 'issue_work_orders', # phase36 + 'project_notification_recipients', # phase37 + 'support_chat_sessions', # phase40 + 'support_chat_messages', # phase40 + 'support_knowledge', # phase40 + 'user_notification_matrix', # phase54 + 'template_contracts', # phase55 +} + +# Columns added after the baseline, on tables the baseline already creates. +# A missing one here is what broke login: SQLAlchemy SELECTs every mapped +# column, so the first User.query fails with "Unknown column". +EXPECTED_COLUMNS = { + 'users': ['mfa_enabled', 'mfa_secret', 'mfa_recovery_codes', 'ui_theme'], + 'facilities': ['qr_token'], + 'areas': ['qr_token'], + 'issues': ['handler_type', 'facility_handler_name'], + 'inspections': ['inspection_schedule_id', 'follow_up_requested_by', + 'follow_up_requested_at', 'follow_up_assigned_to'], + 'support_knowledge': ['sort_order'], } @@ -148,6 +175,22 @@ def verify(args): if need not in ucols: print(f" !! users.{need} missing"); ok = False print(f" users columns: {len(ucols)} (incl id/username/email/role)") + + # Columns added AFTER the baseline. These are the ones that decide + # whether the tenant can serve a request at all: SQLAlchemy SELECTs + # every mapped column, so one missing here means the first query + # against that table raises "Unknown column" — for users, that is the + # login. Checked explicitly because a table-only check passed happily + # while every post-baseline column was absent. + for table, needed in sorted(EXPECTED_COLUMNS.items()): + if table not in built: + continue # already reported as a missing table above + have = {c['name'] for c in insp.get_columns(table)} + gone = [c for c in needed if c not in have] + if gone: + print(f" !! {table} missing post-baseline columns: {gone}"); ok = False + if ok: + print(" post-baseline columns: all present") finally: engine.dispose() return ok