refactor(iac/admin): remove scenario stubs from engine core + 4 RBAC-status fixes#818
Merged
intel352 merged 4 commits intoJun 1, 2026
Conversation
Delete iac/stubprovider, plugins/stubprovider, and the scenario_stub build-tag mechanism from plugins/all. Scenario test fixtures must not live in workflow core; callers that needed stub IaCProvider behaviour now use inline types that embed iac/iactest.NoopProvider or implement the interface directly. Touched files: - iac/stubprovider/ — deleted (provider.go + provider_test.go) - plugins/stubprovider/ — deleted (plugin.go + plugin_test.go) - plugins/all/extras_stub.go, extras_stub_test.go, extras_base_test.go — deleted - plugins/all/all.go — remove scenarioExtras var + append; DefaultPlugins returns base only - iac/admin/handler/*_test.go — replace stubprovider.New() with planningProvider (defined in list_resources_test.go; shared across package handler_test) - module/infra_admin_mutation_integration_test.go — replace stubprovider.New() with testMutationProvider (inline type with real Plan+Destroy+ResourceDriver) Grep clean: zero remaining refs to stubprovider/localauthz/scenario_stub. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bug 1 — contribution payload shape
infra_admin.go: permissions field was []map[string]any (Go rejects type-assert
to []any); changed to []any{map[string]any{…}} across all 4 contribution pipelines.
Bug 2 — nil-store guard
handler/list_resources.go: panic when state_module not configured → explicit
nil check returns typed error before store.ListResources call.
handler/get_resource.go: same guard before store.GetResource.
Bug 3 — typed 403-on-authz-denial
handler/authz.go: introduce handler.ErrAuthzDenied sentinel.
handler/plan_resource.go, apply_resource.go, destroy_resource.go: evidence
default-deny + server-side Enforcer denial now return (output, ErrAuthzDenied)
instead of (output, nil). Provider/backend errors continue to return (output, nil)
with the error in output.Error — no string matching.
module/infra_admin.go: writeMutationResponse() maps errors.Is(ErrAuthzDenied)→403
and non-empty output.Error→500. NOT strings.Contains("denied") — false-positive
on provider errors whose message happens to contain "denied".
Bug 4 — handlePlanResource denial→403
Plan is infra:apply-gated (viewers must not probe desired-state hash or action
list). handlePlanResource now calls m.authz.Enforce before handler.PlanResource
and returns 403 directly on denial. writeMutationResponse replaces writeProtoMsg
for apply/destroy routes too.
Unit tests added:
TestInfraAdmin_TypedAuthzDenied_Returns403 — subtests:
viewer/plan=403, viewer/apply=403, operator/apply=200,
provider-error-denied-text/apply=500-not-403
Existing test updates:
TestInfraAdmin_ViewerCannotApply: 200+body → 403 (typed discriminator)
TestInfraAdmin_ApplyRejectsStalePlanHash: 200 → 500 (non-authz output.Error)
handler denial tests: err!=nil fatalf → errors.Is(ErrAuthzDenied) assertion
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR cleans up the workflow engine core by removing scenario-only IaC stub fixtures (previously gated by the scenario_stub build tag) and hardens the infra.admin surface based on RBAC scenario findings (typed 403 mapping, nil-store guards, and plan enforcement).
Changes:
- Removed stub provider plugin/packages and
scenario_stubplugin-wiring hooks/tests from the engine repo. - Introduced typed authz-denial signaling (
handler.ErrAuthzDenied) and a sharedwriteMutationResponsehelper to map authz denials to HTTP 403 and backend/provider errors to HTTP 500. - Fixed
infra.adminrobustness issues (permissions payload shape; handler nil-store guards) and updated/added tests to pin the new status-code behavior.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/stubprovider/plugin.go |
Removed scenario-only stub provider EnginePlugin implementation. |
plugins/stubprovider/plugin_test.go |
Removed tests for the deleted stub provider plugin. |
plugins/all/extras_stub.go |
Removed scenario_stub build-tag init hook that appended stub plugins. |
plugins/all/extras_stub_test.go |
Removed build-tagged test asserting stub plugin inclusion. |
plugins/all/extras_base_test.go |
Removed non-tag test asserting stub plugin exclusion. |
plugins/all/all.go |
Simplified DefaultPlugins() to return only the base plugin set (no scenario extras). |
iac/stubprovider/provider.go |
Removed in-process stub IaC provider fixture from engine core. |
iac/stubprovider/provider_test.go |
Removed tests for the deleted stub provider. |
module/infra_admin.go |
Fixed contributions payload shape; added typed mutation response writing; enforced plan RBAC; switched apply/destroy to typed status responses. |
module/infra_admin_test.go |
Updated existing tests and added coverage for typed 403 vs 500 classification. |
module/infra_admin_mutation_integration_test.go |
Replaced deleted stubprovider dependency with an in-test IaCProvider/driver double. |
iac/admin/handler/authz.go |
Added ErrAuthzDenied sentinel for typed authz denials. |
iac/admin/handler/plan_resource.go |
Returned ErrAuthzDenied on authz default-deny. |
iac/admin/handler/plan_resource_test.go |
Replaced stubprovider dependency; asserted ErrAuthzDenied on default-deny. |
iac/admin/handler/apply_resource.go |
Returned ErrAuthzDenied on authz denials. |
iac/admin/handler/apply_resource_test.go |
Replaced stubprovider dependency; asserted ErrAuthzDenied on denials. |
iac/admin/handler/destroy_resource.go |
Returned ErrAuthzDenied on authz denials. |
iac/admin/handler/destroy_resource_test.go |
Replaced stubprovider dependency; asserted ErrAuthzDenied on denials. |
iac/admin/handler/drift_check_test.go |
Replaced stubprovider dependency with local test provider. |
iac/admin/handler/list_resources.go |
Added nil-store guard to prevent panic when state store is unset. |
iac/admin/handler/get_resource.go |
Added nil-store guard to prevent panic when state store is unset. |
iac/admin/handler/list_resources_test.go |
Introduced shared local test provider/driver used across handler tests. |
Comment on lines
+1145
to
+1149
| if enforceErr != nil { | ||
| http.Error(w, "plan: authz enforce error", http.StatusInternalServerError) | ||
| m.auditAccess(r, "plan", in.GetEvidence(), "error") | ||
| return | ||
| } |
Comment on lines
+1150
to
+1154
| if !ok { | ||
| http.Error(w, "plan: infra:apply denied for subject "+subject, http.StatusForbidden) | ||
| m.auditAccess(r, "plan", in.GetEvidence(), "denied") | ||
| return | ||
| } |
Comment on lines
38
to
40
| if msg := authzError(in.GetEvidence()); msg != "" { | ||
| return &adminpb.AdminPlanOutput{Error: msg}, nil | ||
| return &adminpb.AdminPlanOutput{Error: msg}, ErrAuthzDenied | ||
| } |
Fix 1 — auditResultFromErr: typed mutation audit classification
Add auditResultFromErr(err error, outError string) string that uses
errors.Is(err, handler.ErrAuthzDenied) for "denied" vs plain outError
non-empty for "error". Replace auditResultFor (strings.Contains path) in
handlePlanResource/handleApplyResource/handleDestroyResource audit calls.
auditResultFor is kept for read handlers (ListResources, GetResource, etc.)
where no typed error is available.
Key regression closed: provider error whose message contains "denied"
(e.g. "provider: access denied to cloud API") now logs result:"error",
NOT "denied". TestInfraAdmin_AuditResultFromErr covers this with 6 table
cases including the false-positive regression case.
Fix 2 — plan module-layer 403 body shape
handlePlanResource Enforcer-deny path now routes through
writeMutationResponse(w, &AdminPlanOutput{Error: msg}, ErrAuthzDenied)
instead of http.Error — proto-JSON body consistent with apply/destroy 403s.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
| ) (*adminpb.AdminPlanOutput, error) { | ||
| if msg := authzError(in.GetEvidence()); msg != "" { | ||
| return &adminpb.AdminPlanOutput{Error: msg}, nil | ||
| return &adminpb.AdminPlanOutput{Error: msg}, ErrAuthzDenied |
Finding 1 — second http.Error site in handlePlanResource
The authz-enforce-error path (Enforce returns err) also used plaintext
http.Error → now routes through writeStatusProto(500, AdminPlanOutput{Error:…})
matching the proto-JSON contract of all other mutation error responses.
Finding 2 — subject reflected in denial body
module/infra_admin.go: plan module-layer denial was "plan: infra:apply denied
for subject <subject>" — genericized to "plan: infra:apply denied" (no principal).
iac/admin/handler/apply_resource.go: "apply: infra:apply denied for subject X"
→ "apply: infra:apply denied".
iac/admin/handler/destroy_resource.go: "destroy: infra:destroy denied for subject X"
→ "destroy: infra:destroy denied".
Subject is captured in the audit log separately; it must not leak in HTTP body.
Finding 3 — stale comment in plan_resource.go
"HTTP stays 200; consumer sniffs tag-100" updated to reflect the current
contract: evidence denial returns (output, ErrAuthzDenied) → HTTP 403.
Test updates:
- viewer/plan=403 subtest extended: asserts Content-Type=application/json +
valid AdminPlanOutput proto body + body does NOT contain "viewer" (no leak).
- TestInfraAdmin_AuditResultFor3Way, TestInfraAdmin_AuditDistinguishesDeniedFromError,
TestInfraAdmin_AuditResultFromErr: example strings updated to new generic messages.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
Copilot's re-review comments (15:23) are stale re-posts — all three findings are addressed in 252f844, verified against source:
|
intel352
deleted the
refactor/infra-admin-remove-scenario-stubs-from-core-2026-06-01T0155
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two things, both arising from infra-admin v1.1 review/QA:
Part A — remove scenario test fixtures from the engine repo. PR #807 (merged) added a
scenario_stub-tagged stubiac.provider+ (#815, closed) an in-process enforcer intoworkflowcore. Maintainer feedback: test fixtures must not live in the production engine repo — even build-tagged. The established pattern (scenarios 85/86/87) is a scenario-ownedcmd/server/main.go. This removes:iac/stubprovider,plugins/stubprovider,plugins/all/extras_stub*.go, thescenarioExtrashook inplugins/all/all.go, and the stub-exclusion tests.DefaultPlugins()returns to its plain base set. The mutation integration test + handler tests migrate to localinterfaces.IaCProvidertest doubles (no production dependency). Scenario 92 will register its stub provider + enforcer from the scenario repo (workflow-scenarios PR).Part B — 4 correctness bugs the live RBAC scenario surfaced in
module/infra_admin.go+ handlers:[]any{map[string]any{...}}(was[]map[string]any).ListResources+GetResource(panic whenstate_moduleunset).handler.ErrAuthzDenied→ HTTP 403 viawriteMutationResponse; provider/operational errors → 500. Replaces fragilestrings.Contains(errMsg,"denied")(false-positived on provider "access denied").handlePlanResourcenow enforces (PlanResourceisinfra:apply-gated) + routes through the denial→403 path (was returning 200 for denied viewers).New test
TestInfraAdmin_TypedAuthzDenied_Returns403: viewer→/plan=403, viewer→/apply=403, operator→/apply=200, provider-error-with-"denied"-text→500 (not 403).Verification
go test -race ./iac/admin/... ./plugins/all/ ./module/green;go build ./cmd/server(no-tags) OK;golangci-lint --new-from-rev=origin/main0 issues; grep-clean of stubprovider/localauthz/scenario_stub. Net: engine core is cleaner than before v1.1 (zero fixtures) + 4 robustness/correctness fixes. ADR for the architecture correction lands in the workspace docs.🤖 Generated with Claude Code