test(pgmpro/resources): coverage for security guards + updateBudgetItem regression

[gloriafolaron]

Companion to plugins#75 — the code under test lives in the plugin submodule at that PR's tip.

Depends on plugins#75 merging first. CI on this PR will be red until the plugin PR merges into feat/logic-model-reverse and the submodule pointer bumps. All 40 assertions pass locally against the plugin PR branch's working tree.

Coverage added

tests/Unit/.../Hxcontrollers/Concerns/ChecksResourceAccessTest.php (new — 21 tests)

Ownership trait every mutating HxController depends on:

• canEditProgram — refuses zero id / nonexistent / regular project type / strategy type / missing session user
• canEditProgram — owner + admin bypass assignment
• canEditProgram — assigned non-admin allowed; unassigned non-admin refused (the core IDOR case)
• canEditCanvas / canEditItem — refuse when repo resolver returns null (blocks cross-canvas-type IDOR); delegate to canEditProgram when it returns a program id
• canAttachProject — refuses program-type / strategy-type; enforces assignment / admin bypass
• forbid() throws AccessDeniedHttpException → 403

tests/Unit/.../ResourceStructureServiceTest.php (12 added)

• updateBudgetItem refuses wrong box / missing item (without touching updateItem)
• updateBudgetItem persists spent (Marcel's exact regression — old isset guard dropped it)
• updateBudgetItem persists projectId (new — budget→project reassignment)
• projectId=0 sentinel persists as 0
• spent: 0 persists (isset→array_key_exists fix)
• Partial update preserves unmentioned fields
• name maps to description column
• getProgramIdForCanvas / getProgramIdForItem pass-through + null path (the null return is the security guarantee)

Not included

• Controller-level integration test (Marcel's suggested negative-auth test: foreign programId → 403). Deferred — requires bootstrapping the HTMX dispatcher stack. The trait tests cover the same guarantee at the enforcement layer.

Test run

• ChecksResourceAccessTest: 21 tests / 21 assertions — all pass
• ResourceStructureServiceTest: 19 tests / 53 assertions (7 pre-existing + 12 new) — all pass

🤖 Generated with Claude Code

[marcelfolaron]

Status: ready-pending-dependency · Priority: P2 (test-only; unblocks the resources security work) · Next action: land plugins#75 first, then bump submodule pointer · Owner: @gloriafolaron, review gate @marcelfolaron / @broskees

Automated maintainer review (advisory only — not an approval; merge authority stays with @marcelfolaron / @broskees). Reviewed at head 4911231704 (created today, first review).

Intent: Adds unit coverage for the PgmPro Resources security guards and the updateBudgetItem regression — a new ChecksResourceAccessTest (21 tests) for the ownership trait, plus 12 tests on ResourceStructureServiceTest covering the isset()→array_key_exists() fix and budget→project reassignment. Test-only; the code under test lives in the plugins#75 submodule tip.

This is exactly the kind of coverage the resources work was missing — well-targeted. The tests encode the real regressions (spent: 0 dropped under the old isset guard; the getProgramIdFor* null path as the IDOR guarantee). Points:

1. Hard dependency — CI will be red until plugins#75 merges (expected, but gating). The PR body states this depends on plugins#75 merging into feat/logic-model-reverse and the submodule pointer bumping. Until then ChecksResourceAccessTest references trait/service classes that don't exist at the pinned submodule commit, so the suite can't compile. Do not merge before plugins#75 is merged and the pointer is bumped — otherwise master goes red. This is the merge-order gate.

2. Confirm the assertion matches the fix direction on projectId=0. ResourceStructureServiceTest::test_updateBudgetItem_persists_projectId asserts 0 persists as the "unassigned" sentinel via array_key_exists. Good — just confirm the service's real write path treats 0 as a valid stored value and not as "clear the field," so the test and implementation agree.

3. Controller-level negative-auth test is deferred (acknowledged). The body notes the foreign-programId→403 integration test is left out (needs the HTMX dispatcher stack). The trait tests cover the enforcement layer, which is acceptable for this PR — but track the controller test as a follow-up so the 403 path has an end-to-end guard, not only the unit-level trait assertion.

Acceptance checklist (must pass before merge):

1. plugins#75 merged + this PR's submodule pointer bumped to that merge commit (the whole reason CI is red now).
2. CI green after the bump: Pint + PHPStan level 5 + the new ChecksResourceAccessTest (21) and ResourceStructureServiceTest (19 total) all passing in-suite, not just locally.
3. No production code in this PR (verified — tests + pointer only), so no migration/back-compat surface; keep it that way.

CI: Red-until-dependency is expected here and is not a flaky-CI flag — but confirm in the UI that it goes green after the pointer bump before merging. Since this is the test half of a security-guard change, treat green CI post-bump as the gate.

[marcelfolaron]

Status: ready-pending-dependency (progressed) · Priority: P2 (test-only; unblocks the resources security work) · Next action: land plugins#75 first, then bump the submodule pointer · Owner: @gloriafolaron, review gate @marcelfolaron / @broskees

Automated maintainer re-review (advisory only — not an approval; merge authority stays with @marcelfolaron / @broskees). Re-reviewed at head 46a4a688 — advanced from my previously-reviewed 4911231704 by a new commit 46a4a688 ("controller-level wiring — every mutating method calls the guard"). Full diff of the new commit read.

Intent: Adds unit coverage for the PgmPro Resources security guards + the updateBudgetItem regression. Now two files: ChecksResourceAccessTest (21 tests — the trait returns the right bool) and a new controller-level ResourceItem test (16 tests — proves each mutating method actually invokes the guard), plus the 12 ResourceStructureServiceTest additions.

Progress since last review — the one open item I flagged is now closed:

• ✅ Controller-level negative-auth test added (was CR Create Email Notification Class (including user settings to opt out) #3, deferred). The new commit lands exactly the "at least one negative-auth integration test" I asked for: seedFromProjects/addPerson/updatePerson/attachProject/createProject each assert a foreign programId/projectId is refused, and — critically — attachProject tests both directions of the reparent IDOR (foreign programId, and foreign projectId even when programId is owned). This closes the "guard exists AND is invoked" loop without booting the HTMX dispatcher.

Change requests on the new commit:

1. ⚠ newInstanceWithoutConstructor + anonymous-subclass stubs are brittle to refactor. The controller tests skip HtmxController::__construct and inject services on an anonymous subclass. That's a reasonable cost trade-off, but it means the test passes even if the real DI/constructor wiring for ResourceItem breaks — the guarantee is "the method body calls the guard," not "the wired controller calls the guard." Acceptable for this PR; note it in the test file's class docblock so a future reader knows the DI path is deliberately out of scope, and keep the follow-up idea of one real HTTP-level 403 test on the backlog.
2. ⚠ Confirm the call-count stubs assert order, not just reach, on attachProject. attachProject gates canEditProgram($programId) then canAttachProject($projectId). Verify the "foreign projectId, owned programId" test would still fail if someone reordered the two checks so the projectId gate runs first against the wrong scope — i.e. the test pins that both gates fire, in a way a reorder can't silently satisfy.
3. ◐ Merge-order gate unchanged (carried over, still the headline). This is the test half; the code under test lives at the plugins#75 submodule tip. CI here is red until plugins#75 merges into feat/logic-model-reverse and this PR's pointer bumps to that commit. Do not merge before that or feat/logic-model-reverse goes red. test(pgmpro/resources): coverage for security guards + updateBudgetItem regression #3655 ↔ Sometimes jQuery fails to load #75 must land pinned together.

Acceptance checklist (must pass before merge):

1. plugins#75 merged + this PR's submodule pointer bumped to that merge commit (the reason CI is red now).
2. CI green after the bump: Pint + PHPStan level 5 + ChecksResourceAccessTest (21) + the new ResourceItem controller tests (16) + ResourceStructureServiceTest (19 total), all passing in-suite (not just locally).
3. No production code in this PR (verified — tests + eventual pointer bump only); keep it test-only so there's no migration/back-compat surface.

CI: Red-until-dependency is expected and is not a flaky-CI flag — but confirm in the UI it goes green after the pointer bump before merging. This is the test gate for a security-guard change: treat green CI post-bump as the merge condition.

Advisory re-review only — not an approval, not marking ready, not merging.

[Copilot]

Pull request overview

Adds unit-level coverage for the PgmPro plugin’s resource ownership/authorization guards and for ResourceStructureService::updateBudgetItem() regressions/field persistence, strengthening the security/IDOR guarantees and preventing silent budget update drops.

Changes:

• Adds trait-level tests for ChecksResourceAccess (program/canvas/item edit checks, attach-project checks, and 403 failure surface).
• Adds a wiring test to ensure each mutating ResourceItem action method actually invokes the guard(s).
• Extends ResourceStructureServiceTest with regression and partial-update coverage for budget item updates plus program-id resolver pass-through/null behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
tests/Unit/app/Plugins/PgmPro/Hxcontrollers/ResourceItemAuthTest.php	New wiring tests ensuring ResourceItem mutating endpoints call the ownership guards and throw 403 on refusal.
tests/Unit/app/Plugins/PgmPro/Hxcontrollers/Concerns/ChecksResourceAccessTest.php	New unit tests covering the guard trait’s allow/deny behavior and its 403 “forbid” surface.
tests/Unit/app/Plugins/PgmPro/Domain/Resources/Services/ResourceStructureServiceTest.php	Adds regression/field persistence tests for updateBudgetItem and verifies resolver methods’ delegation/null guarantees.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.