Bug #14
openFax Tone/Send Module - Modernization: Enum Consolidation, IDOR Guard, Controller Decomposition & API Deprecation
0%
Description
User Story¶
As a platform engineer maintaining the kc-web application, I want to modernize the Fax Tone/Send module by eliminating dual-family code duplication, decomposing the god-controller, closing the IDOR/broken-access-control gap, and establishing a baseline test suite, so that each defect fix or schema change applies once (not twice), the module is independently testable, and the OWASP A01 exposure is closed before the next compliance review.
Background¶
The Fax Tone/Send module implements automated fax-line verification (tone-detection and document-send sub-modes) for the KlearCom platform. A static code analysis (run 20260819T153730_ary8ch) identified four high-severity structural issues:
-
Dual plain/Verify+ family duplication — every entity (model, service, migration, repository) exists in two near-identical copies selected at runtime by the client-supplied
fax_test_typediscriminator (1 = plain, 2 = Verify+). This doubles the maintenance surface for all schema changes and bug fixes. -
God-controller
KcFaxSchedulerController— handles 10+ distinct responsibilities (CRUD, CDR, stuck-test reset, billable/non-billable, alert history, filter dropdowns, bulk actions). Untestable in isolation. -
fax_test_typeIDOR risk — client-supplied discriminator routes requests to the DB family without a confirmed cross-validation of record ownership + family membership (OWASP A01:2021). - API version drift (v1/v2/v3) — three parallel REST API copies with no deprecation timeline; older versions likely lack security/validation fixes applied to v3.
Scope¶
In scope: app/Http/Controllers/Fax/, RestAPIControllers/v1|v2|v3/ (fax only), app/Http/Requests/Fax/, app/Services/Fax/, app/Services/OutboundFax/, app/Models/Fax/, app/Repositories/Fax/, ProcessFaxMonitor, FaxScheduleAlertEmail, FaxRoutes.php, OutboundFaxRoutes.php, 14 fax migrations, fax-related enums and constants.
Out of scope (must not be changed): KcDashboardController, KcBillingControllers, KcCronPdfReports, KcNumberPerformanceTracker, ScheduledClientReport, any non-Fax module, the telephony/fax carrier test engine.
Acceptance Criteria¶
AC-1: FaxTestType PHP Enum introduced
-
FaxTestTypeenum exists withPLAIN = 1andVERIFY_PLUS = 2cases. - All magic literals
1/2forfax_test_typereplaced with enum across all fax controllers, services, models, and Form Requests. -
TEST_TYPE_FAX = 6replaced with a named constant; integer value remains6for cross-module compatibility.
AC-2: IDOR / broken-access-control guard in place
- Every action accepting
fax_test_type+ resource ID validates: (a) record belongs to the submitted family, (b) record belongs to the authenticated account. - PHPUnit feature test asserts mismatched
fax_test_typereturns HTTP 403 or 404.
AC-3: KcFaxSchedulerController decomposed
- Split into ≥ 4 focused controllers:
FaxSchedulerController(CRUD),FaxBillingController,FaxAlertHistoryController,FaxCDRController,FaxStuckTestController. - All existing routes remain at the same URLs; only the backing controller class changes.
AC-4: REST API v1/v2 deprecated; v3 security parity confirmed
- v1/v2 responses include
Deprecation: trueandLinkheaders pointing to v3. - Migration guide document (markdown) listing v1→v3 and v2→v3 field/endpoint mapping written.
- Missing auth middleware or validation rules backported to v1/v2 or documented as accepted risk with a removal target date.
AC-5: Dual-family architecture consolidated (Phase 1)
-
FaxServiceContractinterface andFaxServiceFactoryintroduced; controllers resolve service via factory rather than inlineif/elseonfax_test_type. -
FaxServicesandFaxVerifyServicesimplement the contract. Existing MySQL tables unchanged. - Phase 2 (full model/table merge) deferred to a follow-on ticket, blocked on AC-7.
AC-6: Dashboard query guards added
- Dashboard services enforce a 90-day maximum date-range window (HTTP 422 if exceeded).
- Reporting queries enforce query-layer pagination (LIMIT applied before hydration).
AC-7: Baseline PHPUnit test suite established
- Tests under
tests/Unit/Services/Fax/andtests/Feature/Controllers/Fax/covering: service happy paths, Form Request validation rules (including conditionalfax_verify_doc_id), CRUD happy path, and IDOR guard. - All tests pass on
global-logicbranch CI.
AC-8: ProcessFaxMonitor cron chunked
- Cron uses
chunk(100)orchunkById; no full-table in-memory load.
AC-9: MongoDB PII review completed
-
FaxMonitorServicedocuments data classification of each logged field. - CLI values masked/hashed OR a decision record confirms legal basis for plain-text storage.
Field Definitions¶
| Field | Type | Mandatory | Default | Rules |
|---|---|---|---|---|
fax_test_type |
FaxTestType enum (int: 1 or 2) |
Yes | — |
1 = PLAIN, 2 = VERIFY_PLUS |
did |
string | Yes | — | Valid phone number; must belong to authenticated account |
schedule |
string/int | Yes | — | Non-empty scheduler config |
fax_verify_doc_id |
int | Conditional | — | Required when fax_test_type = 2; FK to fax_verify_doc
|
timezone |
string | No | Account default | IANA timezone string |
cli (monitor) |
string | No | — | Phone number format; PII — see AC-9 |
start_date / end_date
|
date | Yes (reporting) | — |
end >= start; ≤ 90 days for dashboard |
ids (bulk) |
int[] | Yes (bulk) | — | Non-empty; all IDs must belong to authenticated account |
action (bulk) |
string | Yes (bulk) | — | Enum: archive, activate, billable, non_billable
|
history_id |
int | Yes (rerun) | — | FK to correct family history table, scoped to account |
TEST_TYPE_FAX |
int constant | N/A | 6 | Value must remain 6 for cross-module compatibility |
Technical Notes¶
Implementation order (recommended):
- AC-1 (
FaxTestTypeenum) — prerequisite for all - AC-7 (baseline tests) — required before structural refactor
- AC-2 (IDOR guard) — high priority, low effort; unblocked after AC-1
- AC-8 (cron chunking) + AC-9 (MongoDB PII) — low effort; unblocked immediately
- AC-4 (REST deprecation) + AC-6 (query guards) — medium effort; unblocked after AC-1
- AC-3 (controller decomposition) — blocked on AC-7
- AC-5 Phase 1 (service factory) — blocked on AC-7 + AC-3
Key pattern — FaxServiceFactory:
class FaxServiceFactory {
public function make(FaxTestType $type): FaxServiceContract {
return match($type) {
FaxTestType::PLAIN => app(FaxServices::class),
FaxTestType::VERIFY_PLUS => app(FaxVerifyServices::class),
};
}
}
Key pattern — IDOR guard:
$record = $factory->make($request->fax_test_type)->findForAccount($id, auth()->user()->account_id);
abort_if(!$record, 404);
Key constraint: MySQL table names and column names must not change in Phase 1. Cross-module consumers (KcDashboardController, KcBillingControllers, KcCronPdfReports, etc.) read these tables directly and are out of scope.
Risk: No baseline tests currently exist; AC-7 is a hard prerequisite for AC-3 and AC-5 to avoid undetected regressions.
Dependencies¶
- PHP 8.1+ (backed enums)
- Baseline test suite (AC-7) must precede controller decomposition (AC-3) and family consolidation (AC-5)
-
TEST_TYPE_FAX = 6constant contract withScheduledClientReport— value must not change - Cross-module table contracts with Dashboard/Billing teams — table structure frozen in Phase 1
- MongoDB access-control review (AC-9) — coordinate with DevOps/DBA
- v1/v2 REST API consumer inventory — needed before deprecation headers go live
Testing Requirements¶
Unit: FaxTestType enum edge cases; FaxServiceFactory dispatch; FaxServices/FaxVerifyServices schedule CRUD; FaxAlertServices threshold logic; ProcessFaxMonitor chunk behaviour.
Feature: StoreFaxRequest/UpdateFaxRequest all validation rules; IDOR guard (mismatched family → 403/404; cross-account ID → 403); scheduler CRUD happy path; bulk action with cross-account IDs → 403; dashboard date-range > 90 days → 422; reporting pagination meta.
Regression: All existing global-logic CI tests remain green; cross-module smoke tests (dashboard fax stats, billing fax count, PDF report fax bucket) return unchanged results after Phase 1.
Labels: ai-created, modernization, fax-tone-send, security, refactor, php, laravel
Labels: ai-created, modernization, fax-tone-send, security, refactor, php
No data to display