ahmido/TenantAtlas
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects 1 Releases Wiki Activity
1b90927c9e
TenantAtlas/docs/audits/legacy-orphaned-truth-audit.md
Ahmed Darrazi 1b90927c9e feat: centralize tenant lifecycle presentation
2026-03-16 19:17:35 +01:00

25 KiB
Raw Blame History

Repo-wide Legacy / Orphaned Truth Audit

Date: 2026-03-16
Scope: Full codebase — models, migrations, enums, services, jobs, observers, Filament resources, policies, capabilities, badges, tests, factories
Method: Systematic source-of-truth tracing across all layers

Executive Summary

Top 5 Most Dangerous Stale/Legacy Patterns

  1. Tenant.app_status displayed in TenantResource as an active badge despite having ZERO write paths — admins see a permanently frozen "OK" badge that never reflects real provider health. This is a dangerous operational lie.

  2. ProviderConnection dual-status architecture — four status columns exist (status, health_status, consent_status, verification_status). The first two are projections of the latter two, but the ProviderConnectionStateProjector computes values without always persisting them consistently. The UI shows both stacks, creating confusing overlapping semantics.

  3. backup_sets.item_count denormalized counter — actively used in UI and operations but drifts silently when BackupItems are mutated outside the AddPoliciesToBackupSetJob/RemovePoliciesFromBackupSetJob paths (e.g., through RelationManager deletions).

  4. TenantFactory populates stale fields (app_status='ok', app_client_id=uuid) on every test tenant creation, keeping legacy architecture alive in the test suite and masking that these fields are production-dead.

  5. SyncRestoreRunToOperation.php orphaned listener — never called, superseded by SyncRestoreRunToOperationRun.php, but its presence creates confusion about which sync path is canonical.

Cleanup-only Items (Low Risk)

  • Tenant.app_notes — never written, never displayed, pure dead column
  • tenant_user legacy pivot table — data migrated to tenant_memberships, table still present
  • No-op migration file 2026_01_07_142719_create_inventory_items_table.php

Items That Can Actively Mislead Operators

  • app_status badge in TenantResource (dangerous lie — frozen at factory default)
  • ProviderConnection showing 4 status fields with overlapping semantics (confusing UX)
  • Badge normalization mismatches (TenantAppStatus filter accepts unknown but badge has no matching spec → falls back to gray question mark)

Spec Candidate Themes

  • Spec: Tenant Legacy Identity Cleanup — remove app_status, app_notes, clean up app_client_id/app_client_secret to explicit migration-only fields
  • Spec: ProviderConnection Status Simplification — collapse 4 status columns to 2 canonical enums, derive projections live
  • Spec: Denormalized Counter Integrity — event-driven sync for backup_sets.item_count

Findings by Category

Finding 1: Tenant.app_status — Stale Badge Displayed as Truth

Category: 1. Stale duplicate state / 5. Dead-end UI
Severity: CRITICAL
Confidence: HIGH

Why this exists: Early in the project (Dec 2025), app_status was added to track tenant app registration health (ok, consent_required, error, unknown). The ProviderConnection model was introduced Jan 2026, superseding this with consent_status and verification_status.

Canonical source of truth: ProviderConnection.consent_status + ProviderConnection.verification_status

Stale structure: tenants.app_status column (varchar, default 'unknown', indexed)

Evidence:

  • Migration: database/migrations/2025_12_11_121623_add_app_fields_to_tenants_table.php
  • Model: app/Models/Tenant.php — field is fillable
  • Badge domain: BadgeDomain::TenantAppStatus maps values (ok, configured, pending, consent_required, error)
  • UI display: app/Filament/Resources/TenantResource.php line 277 (table column), line 835 (infolist)
  • Filter: app/Filament/Resources/TenantResource.php line 302 (SelectFilter with hardcoded options)

Read paths: TenantResource table column (badge), TenantResource infolist, TenantResource filter
Write paths: NONE — zero production code writes to this field after initial migration default

Why this is dangerous: An admin viewing the tenant list sees an "App Status: OK" badge on every tenant. This badge is frozen at the factory/migration default. Real provider health has moved to ProviderConnection, but this badge creates false confidence that app registration is healthy.

Recommended action: MUST REMOVE

  1. Remove app_status column display from TenantResource (table + infolist + filter)
  2. Remove BadgeDomain::TenantAppStatus badge domain
  3. Drop app_status column via migration
  4. If admins need this info: derive from ProviderConnection.consent_status live

Classification: must remove

Finding 2: Tenant.app_notes — Orphaned Column

Category: 1. Stale duplicate state
Severity: LOW
Confidence: HIGH

Why this exists: Added alongside app_status in Dec 2025 for freeform notes about app registration.

Stale structure: tenants.app_notes column (text, nullable)

Read paths: None — not displayed in any resource or service
Write paths: None — only set to null by TenantFactory

Why this is harmless: Not displayed anywhere, not read by any code path. Pure dead column.

Recommended action: Remove — drop column via migration
Classification: can remove

Finding 3: Tenant Legacy Identity Fields (app_client_id, app_client_secret)

Category: 6. Partially completed migrations
Severity: MEDIUM
Confidence: HIGH

Why this exists: Before ProviderConnection + ProviderCredential existed, tenants stored app registration credentials directly. The provider migration system (Spec 081) introduced a classification layer that reads these fields to determine if a tenant has legacy identity that needs migration review.

Canonical source of truth: ProviderCredential.client_id / ProviderCredential.client_secret

Legacy structure: tenants.app_client_id, tenants.app_client_secret (encrypted), tenants.app_certificate_thumbprint

Read paths:

  • ProviderConnectionMigrationClassificationService — reads for legacy identity detection
  • Guard tests — NoPlatformCredentialFallbackTest, NoLegacyTenantProviderFallbackTest
  • MembershipAuditLogTest — asserts app_client_secret is NOT in audit metadata

Write paths: Only TenantFactory (sets app_client_id to fake()->uuid() on every creation)

Why this matters: These fields are intentionally read-only during the provider migration cutover. The classification service uses them to detect legacy identity and flag migration_review_required on ProviderConnection. They are NOT duplicate state — they are migration-source data.

Recommended action: Keep temporarily — explicitly mark as @deprecated in model, add PHPDoc explaining migration-only purpose. Schedule removal after all tenants pass migration review.
Classification: should remove (after migration completion)

Finding 4: ProviderConnection Dual Status Architecture

Category: 1. Stale duplicate state / 3. Mapping drift
Severity: HIGH
Confidence: HIGH

Why this exists: ProviderConnection was created (Jan 2026) with varchar status and health_status fields. In Mar 2026, enum-backed consent_status and verification_status were added as the new source of truth. The old varchar fields were kept as "projected" summaries for UI display.

Canonical source of truth: consent_status (ProviderConsentStatus enum) + verification_status (ProviderVerificationStatus enum)

Stale structure: status (varchar: connected/needs_consent/error/disabled) + health_status (varchar: ok/degraded/down/unknown)

Evidence:

  • Original migration: database/migrations/2026_01_24_000001_create_provider_connections_table.php
  • New enum columns: database/migrations/2026_03_13_000001_add_provider_identity_fields_to_provider_connections.php
  • Projector: app/Services/Providers/ProviderConnectionStateProjector.php
  • UI reads BOTH stacks: app/Filament/Resources/ProviderConnectionResource.php

Read paths: ProviderConnectionResource table (4 columns), filters (2 filters on varchar fields), badge system (2 badge domains)
Write paths: ProviderConnectionHealthCheckJob calls projector → writes both stacks atomically

Why this is dangerous:

  1. Three incompatible status vocabularies displayed together (varchar status, varchar health, enum consent, enum verification)
  2. Badge normalization functions hidden in BadgeCatalog translate between vocabularies
  3. If projector fails or isn't called on a code path, varchar fields drift from enum truth
  4. ProviderConnectionStateProjector does NOT always persist projected values
  5. UI filters use varchar vocabulary, which breaks if normalization rules change

Recommended action: Finish cutover — migrate UI to read consent_status and verification_status directly. Deprecate varchar status and health_status. Eventually drop columns.
Classification: should remove

Finding 5: backup_sets.item_count Denormalized Counter Drift

Category: 4. Read paths without credible write paths
Severity: MEDIUM
Confidence: HIGH

Why this exists: item_count is a denormalized integer stored on BackupSet for quick display without counting related BackupItems.

Canonical source of truth: BackupSet::items()->count() (relationship count)

Stale structure: backup_sets.item_count column (unsignedInteger, default 0)

Read paths: BackupSetResource table column, BackupSetResource detail page (2x), RunBackupScheduleJob, RemovePoliciesFromBackupSetJob
Write paths: AddPoliciesToBackupSetJob, RemovePoliciesFromBackupSetJob, BackupService (all call $backupSet->items()->count() to sync)

Why this matters: The write path re-counts from DB, which is correct. But if BackupItems are deleted through the BackupItemsRelationManager or soft-delete cascading, and the parent BackupSet isn't re-synced through the job infrastructure, the counter drifts silently. Currently no event/observer recounts on BackupItem delete.

Recommended action: Either add an observer on BackupItem to recount parent, or replace with withCount('items') in queries and drop the denormalized column.
Classification: should remove (or add sync observer)

Finding 6: SyncRestoreRunToOperation.php — Orphaned Listener

Category: 8. Jobs/events/listeners that no longer matter
Severity: LOW
Confidence: HIGH (confirmed via Spec 087)

Why this exists: Early listener for RestoreRun → OperationRun sync. Superseded by SyncRestoreRunToOperationRun.php which is called directly from the RestoreRunObserver.

Stale structure: app/Listeners/SyncRestoreRunToOperation.php

Read paths: None — never imported, never registered
Write paths: N/A — never instantiated

Evidence: Zero grep matches for class name. Spec 087 analysis report (line 616) flagged it as "Low risk; verify/keep during transition."

Recommended action: Remove — Spec 087 is complete, transition is done
Classification: can remove

Finding 7: TenantFactory Populates Stale Fields

Category: 9. Test suite preserving dead architecture
Severity: MEDIUM
Confidence: HIGH

Why this exists: Factory was created with the original Tenant schema and never cleaned up as fields became stale.

Stale structure: database/factories/TenantFactory.php lines 40-44, 49

Evidence:

'app_client_id' => fake()->uuid(),  // Legacy: only read for migration classification
'app_status' => 'ok',               // Dead: never updated in production
'app_notes' => null,                // Dead: never written or read
'rbac_status' => 'ok',              // Hardcoded: never reflects real health checks

Why this matters: Every Tenant::factory()->create() call populates dead fields with plausible-looking values. Tests pass even though these values never match production behavior. app_status='ok' in tests masks that the field is always 'unknown' in production (migration default). rbac_status='ok' prevents testing RBAC degradation paths.

Recommended action:

  1. Remove app_status and app_notes from factory definition
  2. Make app_client_id nullable in factory (set only when testing migration classification)
  3. Set rbac_status to null in default definition; use named states for health scenarios

Classification: should remove (update factory)

Finding 8: Badge Normalization Mismatches

Category: 3. Mapping drift / badge drift
Severity: MEDIUM
Confidence: HIGH

Why this exists: Badge normalization functions translate between different state vocabularies (enum values → display values). Over time, the translation rules have diverged from the actual value sets.

Evidence:

  • TenantAppStatus filter accepts unknown but badge domain has no explicit mapping → falls back to BadgeSpec::unknown() (gray question mark)
  • ProviderConnectionStatus badge normalizes 7+ input values into 3 display states; drift if new enum cases added
  • ProviderConnectionHealth badge normalizes healthy → ok and blocked/error → down; these translations are buried in badge catalog, not documented

Why this matters: When an admin sees a gray question mark badge, they cannot distinguish "this record has a genuinely unknown status" from "this record has a status value that the badge system doesn't recognize." This masks data quality issues.

Recommended action: Add exhaustive badge mapping tests (dataset-driven) ensuring every enum case has an explicit badge spec. Remove BadgeSpec::unknown() fallback for production badge domains — force all values to be explicitly mapped.
Classification: should remove (fallback masking)

Finding 9: tenant_user Legacy Pivot Table

Category: 6. Partially completed migrations
Severity: LOW
Confidence: HIGH

Why this exists: Original many-to-many pivot for tenant ↔ user. Migrated to tenant_memberships table (richer model with role, source, created_by) in Jan 2026.

Old world: tenant_user pivot table
New world: tenant_memberships table (TenantMembership model)
What still references old: Data was backfilled via 2026_01_25_023708_backfill_tenant_memberships_from_tenant_user.php. No model references tenant_user.

What prevents deletion: No drop migration exists. Table is present but unused.

Recommended action: Create drop migration for tenant_user table. Add architecture guard test.
Classification: can remove

Finding 10: EntraGroupPolicy Bypasses Capability Layer

Category: 7. Orphaned policies / security-relevant
Severity: MEDIUM
Confidence: HIGH

Why this exists: EntraGroupPolicy was created for the EntraGroupResource but only checks canAccessTenant() — it does not check any capability constants.

Evidence: app/Policies/EntraGroupPolicy.php — viewAny() and view() methods call $user->canAccessTenant() only.

Why this matters: All other policies enforce capability checks (e.g., TENANT_FINDINGS_VIEW, WORKSPACE_BASELINES_MANAGE). EntraGroupPolicy is the only read-access policy that skips the capability layer entirely. While EntraGroups are read-only, this inconsistency could lead to copy-paste errors if new methods are added.

Recommended action: Add appropriate capability check (e.g., TENANT_VIEW) or document the intentional bypass.
Classification: should remove (the bypass, not the policy)

Finding 11: ProviderConnectionStateProjector Untested

Category: 9. Test suite preserving dead architecture (gap variant)
Severity: MEDIUM
Confidence: HIGH

Why this exists: The projector is a critical bridge between the new enum status sources and the old varchar display fields. Despite being called from 5+ production paths, it has no dedicated test coverage.

Evidence: app/Services/Providers/ProviderConnectionStateProjector.php — called by TenantOnboardingController, AdminConsentCallbackController, ManagedTenantOnboardingWizard, CreateProviderConnection, ProviderConnectionHealthCheckJob.

Why this matters: If the projection logic breaks (e.g., a new enum case is added to ProviderConsentStatus without updating the projector), the derived status and health_status fields silently produce wrong values. No test would catch this.

Recommended action: Create dataset-driven unit test covering all enum combinations → projected varchar values.
Classification: must address (test gap for critical infrastructure)

Finding 12: RestoreRunStatus Legacy Enum Cases

Category: 3. Mapping drift / enum drift
Severity: LOW
Confidence: HIGH

Why this exists: RestoreRunStatus enum includes Aborted and CompletedWithErrors cases labeled as (legacy) in the enum inventory. These were from an earlier restore workflow.

Evidence: app/Support/RestoreRunStatus.php — cases Aborted and CompletedWithErrors exist but are never written by current restore execution code.

Why this matters: These cases still have badge mappings and can appear in filters, but no production code path produces them. They are backward-compatibility noise that should be formally deprecated.

Recommended action: Mark with @deprecated annotation. Keep for now (DB may contain historical records with these values) but exclude from filters and creation logic.
Classification: can remove (from active UX, keep in enum for deserialization)

Finding 13: TENANT_ROLE_MAPPING Capabilities — Defined but No UI

Category: 7. Orphaned capabilities
Severity: LOW
Confidence: MEDIUM

Why this exists: TENANT_ROLE_MAPPING_VIEW and TENANT_ROLE_MAPPING_MANAGE are defined in Capabilities.php and mapped in role capability resolvers, but no Filament resource or page enforces them. The TenantRoleMapping model exists with a migration but has no admin UI.

Evidence: app/Support/Auth/Capabilities.php — constants defined. Tests confirm capability resolver maps them correctly. No Filament resource references them.

Why this matters: This is intentional WIP infrastructure (Spec 062, T028 marked optional). Not dangerous, just incomplete.

Recommended action: Keep — this is staged infrastructure. Add a comment documenting the planned UI.
Classification: keep (intentional WIP)

Finding 14: Direct Graph Calls Bypassing ProviderGateway

Category: 2. Legacy abstractions still referenced
Severity: MEDIUM
Confidence: HIGH

Why this exists: Two locations in TenantResource.php make direct $graph->request('GET', ...) calls instead of using the ProviderGateway abstraction layer.

Evidence:

  • app/Filament/Resources/TenantResource.php line ~1571: $graph->request('GET', 'deviceManagement/roleDefinitions', ...)
  • app/Filament/Resources/TenantResource.php line ~1692: $graph->request('GET', 'groups', ...)

Why this matters: All other Graph calls go through ProviderGateway, which handles credential resolution, contract validation, and error mapping. These direct calls bypass those protections and use an inconsistent error handling pattern.

Recommended action: Refactor to use ProviderGateway or a dedicated service method.
Classification: should remove (the direct calls)

Finding 15: AlertDeliveryPolicy Incomplete Methods

Category: 7. Orphaned policies
Severity: LOW
Confidence: MEDIUM

Evidence: app/Policies/AlertDeliveryPolicy.php — only viewAny() and view() exist. No create(), update(), delete(). AlertDelivery is a read-only model (deliveries are created by jobs, not users), so this is likely intentional but should be documented.

Recommended action: Add create(), update(), delete() methods returning false to make read-only intent explicit. Or document in PHPDoc.
Classification: can remove (the ambiguity, not the policy)

Cross-cutting Architectural Themes

Theme A: Denormalized Status Shadow Fields

Pattern: A canonical source of truth exists (enum column, service output, relationship count) but a denormalized varchar/integer copy is persisted for query performance or historical reasons, then displayed in UI without lifecycle maintenance.

Instances: Tenant.app_status, ProviderConnection.status/health_status, backup_sets.item_count

Root cause: Columns were added before the canonical source existed, then the canonical source was introduced without deprecating the shadow field.

Fix pattern: For each shadow field, choose one of:

  • Live derivation (compute on read via accessor or query join)
  • Event-driven sync (observer/listener updates shadow on canonical change)
  • Remove entirely (if no query performance justification)

Theme B: Incomplete Provider Identity Cutover

Pattern: Tenant was the original identity holder (app_client_id, app_client_secret, app_status). ProviderConnection + ProviderCredential now own identity. Migration classification exists but the old Tenant columns remain populated (by factory) and displayed (by TenantResource).

What blocks full cutover: The classification service still reads Tenant.app_client_id to detect legacy identity. Until all tenants pass migration review, these fields serve as migration input.

Residual risk: Factory continues populating legacy fields, tests assert against them, TenantResource displays app_status badge.

Theme C: Badge Normalization as Hidden Translation Layer

Pattern: Badge domains contain normalization functions that translate between different value vocabularies. These are undocumented, untested, and drift-prone.

Instances: TenantAppStatus normalizer, ProviderConnectionStatus normalizer, ProviderConnectionHealth normalizer.

Fix pattern: Exhaustive enum-to-badge mapping tests. Remove BadgeSpec::unknown() fallback for production domains. Move normalization out of badge catalog into explicit enum methods.

Theme D: Test Suite Preserving Dead Architecture

Pattern: Factories populate stale fields with plausible values, making tests pass even though those fields are production-dead. Guard tests exist (excellent practice) but factory defaults create a false baseline.

Instances: TenantFactory sets app_status='ok', rbac_status='ok', generates app_client_id.

Fix pattern: Factory defaults should match production reality. Use named factory states for legacy-identity-specific test scenarios.

Theme E: Projection Without Test Coverage

Pattern: Critical state-computation services (ProviderConnectionStateProjector) translate between enum sources and derived display fields but have no dedicated test coverage.

Risk: A new enum case added without projector update silently produces wrong UI state.

Fix pattern: Dataset-driven exhaustive projector tests covering all enum combinations.

Spec Candidate Recommendations

Spec Candidate 1: Tenant Legacy Identity Cleanup

Scope: Remove app_status, app_notes column and UI surfaces. Mark app_client_id/app_client_secret as @deprecated migration-only fields. Clean TenantFactory.

Tasks:

  1. Remove app_status from TenantResource (table, infolist, filter)
  2. Remove BadgeDomain::TenantAppStatus and related badge files
  3. Migration: drop app_status, app_notes columns + index
  4. Update TenantFactory: remove app_status, app_notes from definition
  5. Add @deprecated PHPDoc to app_client_id, app_client_secret on Tenant model
  6. Update tests that reference app_status
  7. Drop tenant_user legacy pivot table

Findings covered: #1, #2, #3, #7, #9

Spec Candidate 2: ProviderConnection Status Simplification

Scope: Collapse 4 status columns to 2 canonical enums. UI reads enums directly. Drop varchar projection columns.

Tasks:

  1. Migrate TenantResource and ProviderConnectionResource to read consent_status/verification_status directly
  2. Update badge domains to map enum values (not projected varchars)
  3. Update filters to use enum values
  4. Create projector test suite (exhaustive enum combinations)
  5. Deprecate, then drop status and health_status varchar columns
  6. Remove normalization functions from BadgeCatalog

Findings covered: #4, #8, #11

Spec Candidate 3: Denormalized Counter Integrity

Scope: Fix backup_sets.item_count drift. Either add observer for sync or replace with live count.

Tasks:

  1. Add BackupItem observer that recounts parent BackupSet .item_count on create/delete/restore
  2. Or: replace all reads of item_count with withCount('items') and drop the column
  3. Add test coverage for count drift scenario

Findings covered: #5

Spec Candidate 4: Orphaned Code Removal (Quick Wins)

Scope: Remove clearly dead code identified in this audit.

Tasks:

  1. Delete app/Listeners/SyncRestoreRunToOperation.php
  2. Add architecture guard test confirming it's gone
  3. Clean up RestoreRunStatus::Aborted and CompletedWithErrors from active filters (keep in enum)
  4. Add capability check to EntraGroupPolicy or document bypass
  5. Add explicit deny methods to AlertDeliveryPolicy
  6. Refactor 2 direct Graph calls in TenantResource to use ProviderGateway

Findings covered: #6, #10, #12, #14, #15