fix: 10 temuan code review — dual-state tenant, session contamination, validasi backend, duplikasi API

[muhammadali07]

Ringkasan

Review komprehensif terhadap branch `feat/phase-1-foundation` menemukan 10 temuan (4 critical/high confirmed, 2 medium plausible, 4 cleanup). Root cause dominan adalah desain dua-sumber-kebenaran untuk active tenant selection.

---

Temuan Kritis (CONFIRMED)

🔴 1 — Cross-session contamination: _activeTenantId tidak di-reset saat logout

File: ui/src/api/adminApi.ts:3

authStore.logout() mengosongkan selectedTenantId di Zustand store, tetapi tidak pernah memanggil setActiveTenant(null). Module-level variable _activeTenantId di adminApi.ts tetap menyimpan nilai dari sesi super_admin sebelumnya.

Skenario gagal:

1. Super admin login → pilih tenant-abc → _activeTenantId = 'tenant-abc'
2. Super admin logout (tanpa page reload)
3. Tenant admin login
4. enrollmentApi.makeHeaders() membaca getActiveTenantId() → masih 'tenant-abc'
5. Semua call enrollment/consent masuk ke tenant yang salah secara silent

Fix: Panggil setActiveTenant(null) di dalam action logout() pada authStore.ts.

---

🔴 2 — super_admin masuk ke /tenant/* layout yang tidak punya TenantSelectorBanner

File: ui/src/App.tsx:49

App.tsx menambahkan super_admin ke ProtectedRoute /tenant/*, tetapi TenantAdminDashboard tidak merender TenantSelectorBanner dan tidak pernah memanggil setActiveTenant(). Tidak ada mekanisme bagi super_admin untuk memilih tenant dalam layout tersebut.

Skenario gagal:

• Super admin navigasi ke /tenant/users → _activeTenantId null atau stale
• adminApi.listUsers() kirim tanpa X-Tenant-Id → data cross-tenant atau 403

Fix: Pertimbangkan untuk menghapus super_admin dari route /tenant/* dan hanya layani super_admin lewat /admin/*, atau tambahkan TenantSelectorBanner ke TenantAdminDashboard.

---

🔴 3 — Race condition saat page reload: child effect jalan sebelum sync _activeTenantId

File: ui/src/api/adminApi.ts:3, ui/src/store/authStore.ts

Saat page reload:

• zustand/persist restore selectedTenantId dari localStorage secara synchronous
• adminApi._activeTenantId reset ke null (module init)
• React flush child effects sebelum parent effect → data-fetch effect jalan sebelum sync-effect

Skenario gagal:

• Super admin reload di /admin/users
• selectedTenantId restored = 'tenant-abc', _activeTenantId = null
• listUsers() keluar tanpa X-Tenant-Id → data salah tanpa error

Fix: Hilangkan _activeTenantId module variable. Baca langsung dari useAuthStore.getState().selectedTenantId di authHeaders() dan makeHeaders().

---

🟠 4 — Backend: X-Tenant-Id diterima tanpa validasi keberadaan tenant di DB

File: backend/app/api/deps.py:77

get_effective_principal membangun Principal dari raw string X-Tenant-Id tanpa mengecek apakah tenant tersebut ada dan aktif di database. RLS silently mengembalikan 0 baris untuk UUID yang tidak ada → 200 + [].

Skenario gagal:

• Super admin kirim X-Tenant-Id: 00000000-0000-0000-0000-000000000000
• Tidak ada error; semua endpoint kembalikan 200 + []
• Tenant yang di-suspend juga tidak diblokir lewat jalur ini

Fix: Tambahkan query validasi di get_effective_principal untuk memastikan tenant_id ada dan status = 'active'.

---

Temuan Medium (PLAUSIBLE)

🟡 5 — makeHeaders: extra di-spread setelah Authorization, bisa overwrite token

File: ui/src/api/enrollmentApi.ts:6

const h = { Authorization: `Bearer ${token}`, ...extra }
//                                              ^^^^^^^ extra bisa overwrite Authorization

Caller saat ini aman, tapi API-nya berbahaya.

Fix: Ubah urutan: { ...extra, Authorization: \Bearer ${token}` }` agar auth tidak bisa di-overwrite.

---

🟡 6 — apiFetch: res.json() dipanggil unconditional — silent {} untuk 204 No Content

File: ui/src/api/adminApi.ts:21

const json = await res.json().catch(() => ({}))
if (!res.ok) throw new Error(...)
return (json.data ?? json) as T  // {} untuk 204 No Content

Fix: Cek res.status === 204 sebelum call res.json().

---

Data Integrity

🟡 7 — selectedTenantId di-persist ke localStorage — stale setelah tenant dihapus

File: ui/src/store/authStore.ts:48

Jika tenant dihapus antar sesi, app restore nilai lama dan scope semua request ke tenant yang tidak ada (dapat 200 + [] karena temuan #4).

Fix: Setelah fix #4 diterapkan, backend akan reject invalid tenant ID dengan error yang proper.

---

Conventions & Cleanup

⚪ 8 — TenantsPage.tsx melewati batas 500 baris (CLAUDE.md)

Fix: Pisahkan CreateTenantModal ke CreateTenantModal.tsx.

---

⚪ 9 — API_BASE didefinisikan di 4 file terpisah

adminApi.ts, enrollmentApi.ts, authApi.ts, kioskApi.ts masing-masing mendefinisikan constant yang sama.

Fix: Ekstrak ke ui/src/api/config.ts.

---

⚪ 10 — Dual state untuk active tenant (architectural root cause)

authStore.selectedTenantId (persisted) dan adminApi._activeTenantId (ephemeral) merepresentasikan hal yang sama. Root fix yang menyelesaikan #1, #2, #3, #7, dan #10 sekaligus:

1. Hapus _activeTenantId dan setActiveTenant dari adminApi.ts
2. Di authHeaders() dan makeHeaders(), baca useAuthStore.getState().selectedTenantId langsung
3. Hapus useEffect sync di TenantSelectorBanner
4. authStore.logout() → selectedTenantId: null sudah cukup

---

Checklist Fix

• adminApi.ts: Hapus _activeTenantId, baca dari authStore.getState()
• enrollmentApi.ts: Baca tenant ID dari authStore (bukan getActiveTenantId())
• App.tsx: Hapus super_admin dari /tenant/* ProtectedRoute
• deps.py: Validasi keberadaan & status tenant sebelum construct scoped Principal
• enrollmentApi.ts:6: Pindah Authorization ke posisi terakhir di spread
• adminApi.ts:21: Guard res.status === 204 sebelum res.json()
• TenantsPage.tsx: Pisah CreateTenantModal ke file sendiri
• Buat ui/src/api/config.ts dengan API_BASE, import di semua api files

---

Digenerate dari code review otomatis — branch `feat/phase-1-foundation`

[AliHamaSmart]

Fix Applied — Issue #4

✅ Critical fixes selesai

#1 + #3 (root fix) — Dual-state dihapus, race condition teratasi:

• _activeTenantId module variable dihapus dari adminApi.ts
• getActiveTenantId() dan setActiveTenant() dihapus
• authHeaders() dan makeHeaders() sekarang baca langsung via useAuthStore.getState().selectedTenantId — selalu sinkron dengan Zustand, tidak ada race condition

#2 — super_admin dihapus dari route /tenant/*:

• App.tsx ProtectedRoute /tenant hanya untuk ['tenant_admin', 'supervisor']
• super_admin hanya bisa akses /admin/* yang punya TenantSelectorBanner

#5 — Spread order fix di enrollmentApi.ts:

• Authorization sekarang posisi terakhir: { ...extra, Authorization: ... } — tidak bisa di-overwrite

#6 — Guard 204 No Content:

• apiFetch sekarang cek res.status === 204 sebelum res.json()

⏳ Masih pending

• fix: 10 temuan code review — dual-state tenant, session contamination, validasi backend, duplikasi API #4 Validasi tenant di backend (get_effective_principal) — perlu DB query tambahan
• 🚀 Feature Roadmap — Functional Capabilities Beyond MVP #7 Stale selectedTenantId — akan otomatis teratasi setelah fix: 10 temuan code review — dual-state tenant, session contamination, validasi backend, duplikasi API #4 di-fix
• #8 TenantsPage > 500 baris — cleanup sprint berikutnya
• #9 Duplikasi API_BASE — cleanup sprint berikutnya
• #10 Architectural dual-state — ✅ sudah selesai dengan root fix di atas

npx tsc --noEmit pass clean.