# sub-reviewer-2 — Adversarial review W6 (view-guard + inbox + guard TRÌNH của Contract) > Ghi TỪNG TRỤC ngay khi đo xong (chống #53). READ-only — không sửa gì. > Diff đo: `git diff` working-tree vs HEAD `fe1f6b0` — `ContractFeatures.cs` (+142/-4) · > `ContractWorkflowService.cs` (+15/-1) · `tests/.../Application/ContractV2PipelineTests.cs` (UNTRACKED, mới). --- ## TRỤC 1 — Nới-view có mở QUÁ không? (per-HĐ vs role/workflow-blanket) ### 1.1 Ba call-site `GetEligiblePhases` — ĐỦ và ĐÚNG VỊ TRÍ Grep repo-wide (`grep -rn "GetEligiblePhases" src/Backend --include=*.cs`) → đúng **3** call-site, khớp 3 anchor spec (số dòng đã trôi sau khi chèn code): | Spec anchor | Dòng THẬT sau diff | Handler | W6 có ghép vế V2? | |---|---|---|---| | `:301` | `ContractFeatures.cs:301` | `ListContractsQueryHandler` | CÓ (`:307-314`) | | `:381` | `ContractFeatures.cs:425` | `ListDeletedContractsQueryHandler` | KHÔNG (cố ý, `:426-431`) | | `:486` | `ContractFeatures.cs:603` | `GetContractQueryHandler` | CÓ (`:605-621`) | Không có call-site thứ 4. Rủi ro "SỬA CHỈ 1/3 call-site" (spec §③-C) = **KHÔNG xảy ra**. ### 1.2 Vế V2 là PER-HĐ thật — KHÔNG role-blanket `ContractFeatures.cs:311-312` (List): ``` || (x.c.Phase != ContractPhase.DangSoanThao && myWorkflowIds.Contains(x.c.ApprovalWorkflowId)) ``` `ContractFeatures.cs:609-613` (Detail): `AnyAsync(l => l.Step!.ApprovalWorkflowId == awIdForCheck && l.ApproverUserId == uidForCheck)` với `awIdForCheck` lấy **từ chính HĐ đang xét** (`c.ApprovalWorkflowId`). ⇒ Vị ngữ hỏi theo **workflow ĐÃ PIN của HĐ đó**, không hỏi theo role. Approver của quy trình KHÁC vẫn 403 — test T3 `:356-360` ghim đúng ca này. **KHÔNG role-blanket.** ĐẠT. Hai bờ List ⟂ Detail **đồng vị ngữ** (kiểm bằng tay, khai triển 2 biểu thức): - List cho thấy ⟺ `isDrafter ∨ eligibleByRole ∨ (Phase≠Nháp ∧ inMyWorkflows) ∨ (hardCopy ∧ Phase=DaPhatHanh)` - Detail cho mở ⟺ `isDrafter ∨ eligibleByRole ∨ (hardCopy ∧ Phase=DaPhatHanh) ∨ (¬Nháp ∧ isV2Approver)` ⇒ **TƯƠNG ĐƯƠNG**. Không có lớp "thấy ở list mà 403 khi mở" (hoặc ngược lại). ### 1.3 Nhưng NÓ LÀ **workflow-blanket** (theo thiết kế spec) — cần owner biết ĐỘ RỘNG `ApprovalWorkflow` là **khuôn dùng lại** (Code + Version + `ApplicableType=Contract` + `IsActive`), 1 quy trình pin cho **NHIỀU HĐ**. Vế trên cho: *ai có chân ở BẤT KỲ Bước/Cấp nào của quy trình W* → thấy **MỌI HĐ pin W**, ở **MỌI phase ≠ Nháp**, thuộc **mọi dự án / phòng ban**, kể cả HĐ mình chưa và sẽ không bao giờ tới lượt duyệt. Đây **đúng chữ spec §②-3** ("là `ApproverUserId` của Level trong workflow pin CỦA CHÍNH HĐ đó") và **đúng khuôn 2 anh em** (PE `:886-898`, KHKK `:645-648`), nên KHÔNG phải lỗi cài đặt. Nhưng chữ "per-HĐ" trong chú thích `:302-304` dễ đọc thành "per-HĐ-được-giao" — thực tế là **per-HĐ-theo-quy-trình**. 📏 Phép đo nên chạy trước deploy (1 câu SQL, cho owner thấy độ rộng thật): `SELECT ApprovalWorkflowId, COUNT(*) FROM Contracts WHERE IsDeleted=0 AND ApprovalWorkflowId IS NOT NULL GROUP BY ApprovalWorkflowId` — nếu 1 workflow gánh toàn bộ HĐ 1 loại thì "approver bất kỳ" ≈ "thấy toàn bộ HĐ loại đó". → phân loại: **MINOR (thiết kế theo spec, không phải bug)** — chỉ cần khai rõ, không chặn commit. ### 1.4 Độ rộng PHASE của vế V2 rộng hơn chữ spec (TraLai + TuChoi) Spec §②-3 xin: thấy `ChoDuyet` (+ `DaPhatHanh` nếu thuộc workflow). Cài đặt dùng **phủ định** `Phase != DangSoanThao` ⇒ approver còn thấy `TraLai(98)` và `TuChoi(99)` (và mọi phase legacy 1,3-8 nếu data cũ có). - `TraLai` = HĐ đang nằm trong tay Drafter sửa lại — cùng họ "riêng tư" với Nháp về mặt ngữ nghĩa. - `TuChoi` = terminal khoá. Khuôn KHKK `ContractSigningPlanFeatures.cs:646-648` viết Y HỆT (`Phase != DangSoanThao`), nên đây là **đồng-khuôn**, không phải lệch. Ghi nhận để lần sau ai siết thì biết đang đổi hợp đồng. → **MINOR (đồng khuôn anh em, spec không cấm)**. ### 1.5 Vế `HardCopyActorRoles` **ĐÚNG LÀ role-blanket** — nhưng spec gọi tên đích danh `:375-379` + `:314` + `:618-619`: mọi user mang `Director ∨ AuthorizedSigner ∨ HrAdmin ∨ CostControl` thấy **TOÀN BỘ** HĐ `DaPhatHanh` của công ty (có `GiaTri`), không cần liên quan dự án/quy trình. Spec §②-3 liệt đúng 4 vai này ⇒ **đúng đơn hàng**. Có chặn phase (`&& Phase == DaPhatHanh`) ở CẢ 2 bờ — test T5 `:495-499` dùng `CostControl` (vai vừa bản-cứng vừa có eligible-phase legacy) để ghim đúng chỗ dễ tuột nhất. ĐẠT. ### 1.6 Nháp-riêng-tư S89 — GIỮ ở vế MỚI, nhưng KHÔNG phải bất biến toàn hệ - Vế V2 mới: chặn Nháp ở cả List (`:311`) và Detail (`:616`, `isDraftPhase`) → test T3 `:362-366`. ĐẠT. - 🔎 Nhưng chú thích `:309-310` viết "nháp = riêng tư người soạn" như một **bất biến của hệ**, trong khi `GetEligiblePhases:354-355` vẫn phát `DangSoanThao` cho **mọi** `Drafter`/`DeptManager` ⇒ HĐ nháp của người khác vẫn lộ qua đường LEGACY (pre-existing, W6 không tạo ra). Test T1 `:233-234` khai thẳng điều này (trung thực) — nên đây là **INFO**, không phải lỗi W6. Chỉ lưu ý: đừng trích câu chú thích đó ra ngoài như bằng chứng "HĐ nháp đã kín". ### 1.7 Deleted-view `:425-431` — cùng vị ngữ CŨ, cố ý KHÔNG ghép (ĐÚNG acceptance, LỆCH anh em) - Code: `q.Where(x => x.c.DrafterUserId == userId || eligiblePhases.Contains(x.c.Phase))` — **0 thay đổi hành vi**. - Khớp acceptance §③-B `:31` (HĐ `ChoDuyet` xoá mềm KHÔNG rò sang user V2). Test T4 `:440-449` đo, kèm chứng-nhân dương `:453-455` (người soạn VẪN thấy ⇒ 2 assert âm không xanh nhờ handler trả rỗng). ĐẠT. - ⚠️ **Lệch anh em có chủ đích:** KHKK `ContractSigningPlanFeatures.cs:770-785` màn "Đã xoá" **CÓ** mirror y nguyên vị ngữ list-sống (kể cả vế workflow-membership) và tự chú thích "MIRROR list sống (cùng vị ngữ)". Contract W6 làm NGƯỢC. Cả hai đều "đúng theo tài liệu của mình" ⇒ **2 module cùng repo giờ có 2 luật khác nhau cho cùng câu hỏi**. Không chặn commit (spec W6 thắng ở phạm vi này), nhưng nên ghi 1 dòng vào `docs/gotchas.md` hoặc §rules để lần sau không ai "đồng bộ hoá" nhầm chiều. → **MINOR**. ### 1.8 Kỹ thuật — 2 điểm cần nêu - **(a) `List` + `Contains` chỉ mới được chứng trên SQLite.** `:387-395` trả `List` rồi `myWorkflowIds.Contains(x.c.ApprovalWorkflowId)`; lập luận "NULL không khớp IN ⇒ HĐ V1 tự rớt" là **đúng về logic SQL 3-trị**, và toàn bộ test chạy trên `SqliteDbFixture`. Prod = SQL Server (EF Core 10 dịch collection-param qua `OPENJSON`). Chưa có phép đo nào chạm SQL Server. KHKK `:275-283` tránh hẳn chuyện này bằng `List` + `p.ApprovalWorkflowId != null && …Contains(p.ApprovalWorkflowId.Value)`. → **MINOR-risk**: acceptance rẻ = gọi `GET /api/contracts` bằng token 1 user có chân duyệt V2 trên Dev SQL Server (LocalDB) hoặc prod, thấy 200 + đúng dòng ⇒ đóng. Nếu `OPENJSON` cãi kiểu thì lỗi là **500 lúc chạy**, test SQLite không bắt được. - **(b) `ResolveUserApprovalWorkflowIdsAsync` không lọc `ApplicableType`** — user có chân trong workflow của PE/KHKK cũng vào `myWorkflowIds`. Vô hại **vì** id đó không bao giờ khớp `Contract.ApprovalWorkflowId` (HĐ chỉ pin workflow `ApplicableType=Contract`, guard tạo HĐ chặn). Ghi lại để không ai bỏ guard tạo HĐ mà tưởng chỗ này vẫn kín. → **INFO**. **TRỤC 1 = PASS** (0 critical). 1 MINOR-risk kỹ thuật (a) + 3 MINOR ghi-nhận (1.3 / 1.4 / 1.7) + 2 INFO. --- ## TRỤC 2 — Admin-inbox nhánh phase tường minh ### 2.1 `AdminInboxPhases` = `PhaseActorRoles.Keys` + `ChoDuyet` — đối chiếu TỪNG PHẦN TỬ | # | `PhaseActorRoles` keys (`:470-479`) | `AdminInboxPhases` (`:487-497`) | |---|---|---| | 1 | DangSoanThao | DangSoanThao | | 2 | DangGopY | DangGopY | | 3 | DangDamPhan | DangDamPhan | | 4 | DangInKy | DangInKy | | 5 | DangKiemTraCCM | DangKiemTraCCM | | 6 | DangTrinhKy | DangTrinhKy | | 7 | DangDongDau | DangDongDau | | 8 | — | **ChoDuyet** ← MỚI | ⇒ **THUẦN CỘNG THÊM**, 0 phase bị rơi. Chú thích `:484` viết "7 phase legacy CŨ + ChoDuyet" — **con số ĐÚNG** (đếm tay 7). ĐẠT. ### 2.2 KHÔNG lọt terminal Enum `ContractPhase` có 12 giá trị. `AdminInboxPhases` **không** chứa `DaPhatHanh(9)` · `TuChoi(99)` · `TraLai(98)` · `DangChon(1)`. Test T6 `:552-555 + :579-580` đo bằng `NotIntersectWith` trên đúng 3 phase kết thúc. ĐẠT. ### 2.3 `v2InboxIds` rỗng cho admin — ĐÚNG khuôn PE, không phải cắt xén Đọc bờ PE (`PurchaseEvaluationFeatures.cs:746-748`): `var v2InboxIds = isAdmin ? new HashSet() : await ResolveV2InboxIdsAsync(userId, ct);` W6 `:516-518` **y hệt**. Admin đi bằng nhánh PHASE nên đủ. ĐẠT (mirror thật, đã đọc bờ kia, không tin lời khai chú thích). ### 2.4 Điều-kiện-thoát-sớm — sửa ĐÚNG chỗ chí tử `:522` `if (eligiblePhases.Count == 0 && v2InboxIds.Count == 0) return [];` Giữ nguyên bản cũ (`Count == 0` một vế) thì approver **0 role** nhận hộp thư rỗng dù có phiếu ⇒ một dòng `return` giết trọn tính năng. Test T1 cố ý cho approver A **0 role** (`:208`) nên ca này ĐƯỢC ĐO THẬT, không phải nói suông. ĐẠT. ### 2.5 Ba điểm CÒN HỞ / cần biết (không chặn commit) - **(a) `AdminInboxPhases` là BẢN CHÉP TAY của `PhaseActorRoles.Keys` ⇒ mầm derived-drift.** Thêm key mới vào `PhaseActorRoles` sau này mà quên `AdminInboxPhases` → admin âm thầm mù phase đó. Test T6 **KHÔNG bắt được** chiều này: nó liệt kê literal 8 phase của riêng nó, nên phase thứ 9 mới thêm vào dict sẽ để T6 xanh nguyên. → **MINOR**. Acceptance rẻ nếu muốn đóng: 1 assert `PhaseActorRoles.Keys ⊆ AdminInboxPhases` (cần nới `private` → `internal` cho dict). - **(b) `TraLai(98)` KHÔNG có trong hộp thư của BẤT KỲ AI** (không trong `PhaseActorRoles`, không trong `AdminInboxPhases`; `grep TraLai src/.../ContractFeatures.cs` = **0 hit**). HĐ bị Trả lại ⇒ người soạn chỉ tìm thấy qua màn Danh sách (vế `DrafterUserId == userId`), không có tín hiệu ở Inbox. Vòng `ChoDuyet → TraLai → gửi lại` vì thế **chưa khép ở lớp hộp thư**. PE mirror y hệt (`GetEligiblePhases:691-709` + admin list `:731-742` đều không có TraLai) ⇒ **đồng khuôn, không phải hồi quy W6**, và spec W6 không xin. → **MINOR / ngoài phạm vi**. - **(c) Chi phí `ResolveV2InboxIdsAsync`:** nạp **TOÀN BỘ** HĐ `ChoDuyet` (không lọc theo user) vào bộ nhớ mỗi lần gọi inbox của mỗi non-admin, rồi nạp thêm mọi workflow liên quan kèm Steps+Levels. Đúng khuôn PE/KHKK nên chấp nhận được, nhưng nó **lớn theo số HĐ đang chờ**, không theo số HĐ của người gọi. Kèm `Take(100)` ở `:534` ⇒ hộp thư đông có thể bị cắt ngọn. → **INFO**. **TRỤC 2 = PASS** (0 critical). 2 MINOR + 1 INFO. --- ## TRỤC 3 — Guard TRÌNH: 4 vế + null-trap + có mở transition nào khác không ### 3.1 Khai triển guard (`ContractWorkflowService.cs:82-92`) Cho qua ⟺ `isAdmin ∨ isSystem ∨ Drafter ∨ DeptManager ∨ Procurement ∨ isCreator`, với `isCreator = actorUserId is Guid a ∧ (CreatedBy == a ∨ DrafterUserId == a)`. ### 3.2 Null-trap — ĐÓNG THẬT, nhưng khai đúng mức - `actorUserId is Guid actorId` chặn được ca `null == null` = TRUE. ĐẠT, test T7 ca 4 (`:647-658`) đo đúng ca đó với tiền-đề được **assert tường minh** (`c4.CreatedBy.Should().BeNull()` `:649`) ⇒ phép đo không thể xanh ở "thế giới khác". Đây là chỗ làm tốt nhất của cả diff. - ⚠️ **Nhưng ca đó KHÔNG với tới được từ API hiện tại:** `TransitionContractCommandHandler:223-224` ném `UnauthorizedException` khi `currentUser.UserId is null`, và `grep TransitionAsync` cho thấy **duy nhất 1 caller** (`ContractFeatures.cs:229`). Không có job/SLA nào gọi. ⇒ guard này là **phòng thủ chiều sâu**, không phải bịt lỗ đang chảy. Chú thích test `:591-593` ("một lệnh trình KHÔNG DANH TÍNH đi lọt") đọc như mô tả lỗ SỐNG — nên hạ giọng thành "sẽ lọt nếu sau này có caller hệ thống". → **INFO (độ chính xác lời khai)**, giữ nguyên mã. ### 3.3 `isSystem` bypass — không phải cửa mới, và hiện KHÔNG có ai đi `isSystem = actorUserId is null && decision == AutoApprove` (`:46`). Từ API, `actorUserId` không bao giờ null (3.2) ⇒ nhánh này hiện **chết**. FE gửi `decision=AutoApprove` cũng vô hại vì actor có id. Đúng thiết kế đã ghi trong skill `contract-workflow` ("System bypass"). ĐẠT, → **INFO**. ### 3.4 `isCreator` KHÔNG giả mạo được — đã truy 2 write-path - `DrafterUserId` gán từ `currentUser.UserId` tại create (`ContractFeatures.cs:96`) và **không** nằm trong `UpdateContractDraftCommand` (`:136-145`, 7 field, không có DrafterUserId) ⇒ không đổi được. - `CreatedBy` do `AuditingInterceptor` gán. Không có endpoint nào set. ⇒ Không có đường "tự phong mình làm người tạo rồi trình HĐ người khác". ĐẠT. ### 3.5 Thêm `Procurement` mở ĐÚNG 1 cửa — đã đọc call-site, không suy đoán `grep -n "Procurement\|AppRoles\." ContractWorkflowService.cs` → `Procurement` xuất hiện **đúng 1 lần trong mã chạy** (`:87`), phần còn lại là chú thích. Soi 4 nhánh của `TransitionAsync`: | Nhánh | Điều kiện vào | Procurement có lợi thêm không | |---|---|---| | REJECT `:49-66` | `decision == Reject` | KHÔNG (nhánh này **0 guard cho MỌI người** — xem 3.6) | | TRÌNH `:70-102` | from ∈ {Nháp, TraLai} | **CÓ — cửa duy nhất mở thêm** | | APPROVE `:105-201` → `ApproveV2Async:230+` | from = ChoDuyet | KHÔNG — match theo `ApproverUserId` (`:271-278`), không đọc role | | Admin override `:204-212` | `isAdmin` | KHÔNG | Thêm nữa, nhánh TRÌNH **ép** `Phase = ChoDuyet` (`:93`) bất kể `targetPhase` client gửi, nên Procurement không nhảy thẳng được sang `DaPhatHanh`. ĐẠT — **đúng "dòng tối thiểu"** như spec §②-5. ### 3.6 🔴 Điều PHẢI nói dù spec bảo "đừng đụng": W6 **khuếch đại** lỗ O-A Nhánh REJECT (`:49-66`) chạy **TRƯỚC mọi guard** và **không có bất kỳ kiểm tra vai/quan hệ nào**; `TransitionContractCommandHandler` cũng chỉ kiểm `IsAuthenticated`; `ContractsController:60-61` `POST /{id}/transitions` **không có** `[Authorize(Policy=...)]` (class chỉ `[Authorize]` trần — đúng lớp gotcha #82). ⇒ **bất kỳ user đăng nhập nào biết `contractId` đều Trả-lại/Từ-chối được HĐ bất kỳ**, kể cả HĐ mình không có quyền XEM. Đây **KHÔNG phải lỗi W6** (0-diff, anh chốt ĐỂ MỞ) — nhưng W6 vừa mở màn Danh sách/Chi tiết cho một tập người dùng rộng hơn nhiều, tức **làm việc lấy `contractId` trở nên dễ**. Bán kính nổ của lỗ cũ tăng theo. Không chặn commit; ghi ở đây để khi đóng O-A thì biết nó đã rộng ra từ lúc nào. → **MAJOR (pre-existing, amplified — không yêu cầu sửa trong W6)**. ### 3.7 Hệ quả phụ của việc thêm `Procurement` (spec-sanctioned, cần nói rõ) `Procurement` là vế **theo ROLE**, không kèm quan hệ với HĐ ⇒ một NV Mua hàng **trình được MỌI HĐ** đang ở Nháp/TraLai, kể cả HĐ người khác soạn. Cùng lớp với `Drafter`/`DeptManager` vốn đã vậy (pre-existing), và spec §②-5 xin đúng chữ đó. Rào tự nhiên còn lại: Procurement **không thấy** HĐ Nháp của người khác (`GetEligiblePhases:354-355` chỉ phát `DangSoanThao` cho Drafter/DeptManager; vế V2 mới loại Nháp) ⇒ khó lấy id. Hành vi có log (`LogTransitionAsync` ghi `actorUserId`). → **MINOR**. **TRỤC 3 = PASS** (0 critical trong phạm vi W6). 1 MAJOR pre-existing-amplified + 1 MINOR + 2 INFO. --- --- ## TRỤC 4-7 + VERDICT — [LEAD đóng on-behalf @S161: vai chết #53 sau Trục-3] **TRỤC 4 — fidelity Resolve mirror: ĐẠT** (hành vi khoá bởi 7 test pass — OR-of-N/con-trỏ đôi qua Inbox_Pending/OtherLevel; trục-1 đã xác nhận per-HĐ AnyAsync đúng workflowId). **TRỤC 5 — test-integrity: ĐẠT** — 7 test = 5 PIN đúng NGUYÊN VĂN + 2 bonus đúng đặt hàng (`Inbox_Admin_SeesChoDuyetContract` DR-4 · `Submit_ProcurementAndCreator_Allowed_UnrelatedRoleAndNullActor_Forbidden` khoá cả 4-vế guard + null-trap trong 1 test). Suite 587/0 lead-tươi. **TRỤC 6-7 — boundary: ĐẠT** — CWS diff CHỈ khối guard :73-78 (Reject :49-66 + ApproveV2Async :217-394 KHÔNG đụng) · KHKK module + Migrations 0-diff. **VERDICT: PASS_WITH_FLAGS — 0 chặn.** 2 ghi-nhận chuyển owner (KHÔNG phải lỗi W6): 1. **Độ-rộng view = "workflow-blanket by design"** (trục-1.3): vế V2 là per-HĐ-THEO-QUY-TRÌNH (có chân trong quy trình ⇒ thấy MỌI HĐ pin quy trình đó), không phải per-HĐ-được-giao — đúng chữ spec, nhưng owner cần biết độ rộng khi số HĐ tăng. 2. **Reject :49-66 pre-existing AMPLIFIED** (trục-3.6): W6 mở list/detail rộng hơn ⇒ lấy `contractId` dễ hơn ⇒ bán kính nổ của lỗ O-A-để-mở TĂNG. Ghi mốc để khi đóng O-A biết nó rộng ra từ đây.