From 1fcf9a836a00419101ed0c9b3db3f855a45024ca Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:10:17 +0200 Subject: [PATCH 1/7] test(sca,workflows): pin wire contracts, JSON tags and response guards Close the SCA / workflows / models rows of the mutation ledger. sca: record route/body/params in mockHTTPClient; assert the exact route for every endpoint and the full POST body contents; replace the tautological TestListSessions_WithCSPFilter with an assertion on the sent csp query param; pin the on-demand target_category and pageSize literals; cover context propagation and the paginated decode errors; extract the X-API-Version guard into its own test; pin the sca/uar ISP service slugs via the constructed BaseURL. workflows: drive a 500 through all six checkResponse call sites; assert the exact finalize route and FinalizationReason; pin the limit query param and defaultPageSize; add logging_client_test.go (including the Authorization redaction guard) and service_config_test.go. models: pin the requestDetails, result, cancelReason and roleId request tags, and decode a populated accessCredentials off the wire through ParseAWSCredentials; assert the parsed values in TestParseAWSCredentials. No production code changed. --- CLAUDE.md | 2 + docs/mutation-ledger.md | 88 +++--- internal/sca/models/credentials_test.go | 32 +- internal/sca/models/wire_tags_test.go | 142 +++++++++ internal/sca/service_client_test.go | 72 +++++ internal/sca/service_test.go | 94 ++++-- internal/sca/wire_contract_test.go | 326 ++++++++++++++++++++ internal/workflows/logging_client_test.go | 317 +++++++++++++++++++ internal/workflows/models/wire_tags_test.go | 90 ++++++ internal/workflows/service_config_test.go | 92 ++++++ internal/workflows/service_test.go | 32 +- internal/workflows/wire_contract_test.go | 271 ++++++++++++++++ 12 files changed, 1475 insertions(+), 83 deletions(-) create mode 100644 internal/sca/models/wire_tags_test.go create mode 100644 internal/sca/service_client_test.go create mode 100644 internal/sca/wire_contract_test.go create mode 100644 internal/workflows/logging_client_test.go create mode 100644 internal/workflows/models/wire_tags_test.go create mode 100644 internal/workflows/service_config_test.go create mode 100644 internal/workflows/wire_contract_test.go diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..c8a0a5b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,6 +32,8 @@ Custom `SCAAccessService` follows SDK conventions: - Create client via `isp.FromISPAuth(ispAuth, "sca", ".", "", refreshCallback)` - Set `X-API-Version: 2.0` header on all requests - `httpClient` interface for DI/testing +- The service slug (`"sca"` / `"uar"`) is what `isp.FromISPAuth` resolves into the live host, and the retry/header tests overwrite `client.BaseURL` before issuing a request — so it is pinned separately, by asserting the constructed `BaseURL` against the fake-JWT tenant (`TestNewSCAAccessService_UsesSCAServiceSlug`, `TestNewAccessRequestService_UsesUARServiceSlug`) **before** any swap. Assert it before mutating `BaseURL`, never after +- Wire contracts are asserted on what is *sent*: the `mockHTTPClient` in both packages records `gotRoute`/`gotBody`/`gotParams` before dispatching. Assert the exact route and the full body contents — "non-nil body" lets a nil payload through, and a canned response tells you nothing about the request ## SCA Access API - **Base URL:** `https://{subdomain}.sca.{platform_domain}/api` diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..38e7c87 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -87,44 +87,46 @@ premise does not hold). | OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | -| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | -| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | todo | -| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | todo | -| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | todo | -| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | todo | -| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | todo | -| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | todo | -| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | todo | -| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | todo | -| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | -| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | -| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | todo | -| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | todo | -| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | todo | -| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | todo | -| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | todo | -| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | todo | -| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | todo | -| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | todo | -| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | todo | -| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | todo | -| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | todo | -| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | todo | -| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | todo | -| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | todo | -| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | todo | -| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | todo | -| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | todo | -| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | todo | -| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | todo | -| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | todo | -| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | -| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | +| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | done | +| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | done | +| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | done | +| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | done | +| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | done | +| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | done | +| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | done | +| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | done | +| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | +| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | +| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | done | +| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | done | +| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | done | +| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | done | +| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | done | +| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | done | +| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | done | +| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | done | +| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | done | +| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | done | +| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | done | +| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | done | +| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | done | +| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | done | +| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | done | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | done | +| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | done | +| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | done | +| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | done | +| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | done | +| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | +| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | +| SCA-19 | internal/sca | `internal/sca/service.go:60` | `isp.FromISPAuth(ispAuth, "sca", ...)` → `"WRONG"`. Not in the original audit; found while closing SCA-15. Both retry-policy tests overwrite `client.BaseURL` before issuing a request, so the slug — which is what resolves the live host — was unpinned. **Decision: pin, not won't-fix.** The assertion is one line against the already-constructed client, needs no network, and the failure mode (every request to the wrong host) is total | CONFIRMED | test | `TestNewSCAAccessService_UsesSCAServiceSlug` — asserts `BaseURL == "https://testtenant.sca.example.test"` from the existing fake JWT, **before** any BaseURL swap | PR8 | done | +| WF-21 | internal/workflows | `internal/workflows/service.go:56` | `isp.FromISPAuth(ispAuth, "uar", ...)` → `"WRONG"`. The UAR twin of SCA-19, same rationale and same decision | CONFIRMED | test | `TestNewAccessRequestService_UsesUARServiceSlug` | PR8 | done | | ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | | ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | | ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | @@ -216,15 +218,15 @@ premise does not hold). | PR5 — Output contracts | 32 | 30 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | -| PR8 — SCA / workflows / models wire contracts | 36 | 34 | +| PR8 — SCA / workflows / models wire contracts | 38 | 36 | | *(no PR — settled, recorded only)* | 5 | 0 | -| **Total** | **165** | **152** | +| **Total** | **167** | **154** | ### By verdict | Verdict | Rows | |---|---| -| CONFIRMED | 152 | +| CONFIRMED | 154 | | OVERSTATED | 9 | | REFUTED | 4 | @@ -232,12 +234,12 @@ premise does not hold). | Disposition | Rows | |---|---| -| `test` | 149 | +| `test` | 151 | | `test + prod-fix` | 5 | | `prod-fix` | 1 | | `wont-fix` | 5 | | `refuted` | 5 | -| **Total** | **165** | +| **Total** | **167** | The seven production changes, matching the plan's table: diff --git a/internal/sca/models/credentials_test.go b/internal/sca/models/credentials_test.go index 9e1798c..794191e 100644 --- a/internal/sca/models/credentials_test.go +++ b/internal/sca/models/credentials_test.go @@ -54,11 +54,21 @@ func TestParseAWSCredentials(t *testing.T) { name string input string wantErr bool + // Asserted only when wantErr is false. Without these the parser could + // swap SecretAccessKey and SessionToken and this package would not + // notice — it discarded the parsed value entirely. + wantAccessKeyID string + wantSecretKey string + wantSessionToken string }{ { - name: "valid JSON string", - input: `{"aws_access_key":"AKIA","aws_secret_access_key":"secret","aws_session_token":"token"}`, - wantErr: false, + name: "valid JSON string", + // Distinguishable values: a swap of any two must change the result. + input: `{"aws_access_key":"AKIAVALUE","aws_secret_access_key":"SECRETVALUE","aws_session_token":"TOKENVALUE"}`, + wantErr: false, + wantAccessKeyID: "AKIAVALUE", + wantSecretKey: "SECRETVALUE", + wantSessionToken: "TOKENVALUE", }, { name: "empty string", @@ -85,9 +95,21 @@ func TestParseAWSCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := ParseAWSCredentials(tt.input) + creds, err := ParseAWSCredentials(tt.input) if (err != nil) != tt.wantErr { - t.Errorf("ParseAWSCredentials() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("ParseAWSCredentials() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + return + } + if creds.AccessKeyID != tt.wantAccessKeyID { + t.Errorf("AccessKeyID = %q, want %q", creds.AccessKeyID, tt.wantAccessKeyID) + } + if creds.SecretAccessKey != tt.wantSecretKey { + t.Errorf("SecretAccessKey = %q, want %q", creds.SecretAccessKey, tt.wantSecretKey) + } + if creds.SessionToken != tt.wantSessionToken { + t.Errorf("SessionToken = %q, want %q", creds.SessionToken, tt.wantSessionToken) } }) } diff --git a/internal/sca/models/wire_tags_test.go b/internal/sca/models/wire_tags_test.go new file mode 100644 index 0000000..3a8710a --- /dev/null +++ b/internal/sca/models/wire_tags_test.go @@ -0,0 +1,142 @@ +package models + +import ( + "encoding/json" + "testing" +) + +// TestElevateRequest_JSONTags pins the request-body field names. The existing +// marshal tests decode into the same Go struct, so a renamed tag round-trips +// trivially; these assert the literal keys the API sees. +func TestElevateRequest_JSONTags(t *testing.T) { + t.Parallel() + + req := ElevateRequest{ + CSP: CSPAzure, + OrganizationID: "org-tag-1", + Targets: []ElevateTarget{ + {WorkspaceID: "ws-tag-2", RoleID: "role-tag-3", RoleName: "Role Tag Four"}, + }, + } + b, err := json.Marshal(req) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"csp", "organizationId", "targets"} { + if _, ok := raw[key]; !ok { + t.Errorf("request body is missing key %q: %s", key, b) + } + } + + var targets []map[string]json.RawMessage + if err := json.Unmarshal(raw["targets"], &targets); err != nil { + t.Fatalf("unmarshal targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets len = %d, want 1", len(targets)) + } + for _, key := range []string{"workspaceId", "roleId", "roleName"} { + if _, ok := targets[0][key]; !ok { + t.Errorf("target is missing key %q: %s", key, b) + } + } +} + +// TestElevateResponse_DecodesPopulatedAccessCredentials is the guard for the +// single field `grant env` exists to deliver. Every prior test only ever saw +// "accessCredentials": null, or marshaled a Go struct whose field was nil, so +// renaming the tag passed the entire repo suite while every AWS elevation +// silently returned no credentials. +func TestElevateResponse_DecodesPopulatedAccessCredentials(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap in the parser cannot be masked. + const wire = `{ + "response": { + "csp": "AWS", + "organizationId": "org-creds-1", + "results": [ + { + "workspaceId": "111122223333", + "roleId": "arn:aws:iam::111122223333:role/Admin", + "sessionId": "sess-creds-2", + "accessCredentials": "{\"aws_access_key\":\"ACCESSKEYVALUE\",\"aws_secret_access_key\":\"SECRETKEYVALUE\",\"aws_session_token\":\"SESSIONTOKENVALUE\"}", + "errorInfo": null + } + ] + } + }` + + var resp ElevateResponse + if err := json.Unmarshal([]byte(wire), &resp); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(resp.Response.Results) != 1 { + t.Fatalf("results len = %d, want 1", len(resp.Response.Results)) + } + raw := resp.Response.Results[0].AccessCredentials + if raw == nil { + t.Fatal("accessCredentials did not decode: got nil, want the populated credentials string") + } + + creds, err := ParseAWSCredentials(*raw) + if err != nil { + t.Fatalf("ParseAWSCredentials: %v", err) + } + if creds.AccessKeyID != "ACCESSKEYVALUE" { + t.Errorf("AccessKeyID = %q, want %q", creds.AccessKeyID, "ACCESSKEYVALUE") + } + if creds.SecretAccessKey != "SECRETKEYVALUE" { + t.Errorf("SecretAccessKey = %q, want %q", creds.SecretAccessKey, "SECRETKEYVALUE") + } + if creds.SessionToken != "SESSIONTOKENVALUE" { + t.Errorf("SessionToken = %q, want %q", creds.SessionToken, "SESSIONTOKENVALUE") + } +} + +// TestElevateResponse_AccessCredentialsAbsentOrEmpty covers the non-populated +// shapes of the same field. +func TestElevateResponse_AccessCredentialsAbsentOrEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + result string + wantNil bool + wantParseErr bool + }{ + {name: "explicit null", result: `{"accessCredentials": null}`, wantNil: true}, + {name: "absent", result: `{"sessionId": "sess-1"}`, wantNil: true}, + {name: "empty string", result: `{"accessCredentials": ""}`, wantParseErr: true}, + {name: "malformed inner JSON", result: `{"accessCredentials": "{not json}"}`, wantParseErr: true}, + {name: "incomplete inner JSON", result: `{"accessCredentials": "{\"aws_access_key\":\"AK\"}"}`, wantParseErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var res ElevateTargetResult + if err := json.Unmarshal([]byte(tt.result), &res); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if tt.wantNil { + if res.AccessCredentials != nil { + t.Fatalf("accessCredentials = %q, want nil", *res.AccessCredentials) + } + return + } + if res.AccessCredentials == nil { + t.Fatal("accessCredentials = nil, want a decoded string") + } + _, err := ParseAWSCredentials(*res.AccessCredentials) + if (err != nil) != tt.wantParseErr { + t.Errorf("ParseAWSCredentials error = %v, wantErr %v", err, tt.wantParseErr) + } + }) + } +} diff --git a/internal/sca/service_client_test.go b/internal/sca/service_client_test.go new file mode 100644 index 0000000..c9a311f --- /dev/null +++ b/internal/sca/service_client_test.go @@ -0,0 +1,72 @@ +package sca + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/cyberark/idsec-sdk-golang/pkg/common/isp" +) + +// ispClientFromService reaches through the logging decorator to the SDK client +// the constructor actually published. +func ispClientFromService(t *testing.T, svc *SCAAccessService) *isp.IdsecISPServiceClient { + t.Helper() + lc, ok := svc.httpClient.(*loggingClient) + if !ok { + t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient) + } + client, ok := lc.inner.(*isp.IdsecISPServiceClient) + if !ok { + t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner) + } + return client +} + +// TestNewSCAAccessService_SetsAPIVersionHeader is the standalone guard for the +// X-API-Version header. It previously existed only as two lines inside +// TestNewSCAAccessServiceDisablesTransientRetry, so any retry-motivated rename +// or deletion of that test would have removed the header guard silently. +func TestNewSCAAccessService_SetsAPIVersionHeader(t *testing.T) { + gotAPIVersion := make(chan string, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAPIVersion <- r.Header.Get("X-API-Version") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + svc, err := NewSCAAccessService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewSCAAccessService: %v", err) + } + ispClientFromService(t, svc).BaseURL = srv.URL + + resp, err := svc.httpClient.Get(t.Context(), "/api/access/sessions", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if got := <-gotAPIVersion; got != "2.0" { + t.Errorf("X-API-Version = %q, want %q", got, "2.0") + } +} + +// TestNewSCAAccessService_UsesSCAServiceSlug pins the "sca" service slug passed +// to isp.FromISPAuth. The retry and header tests both overwrite BaseURL before +// issuing a request, so without this the slug could be changed to anything and +// no test in the repo would notice — while every live request would go to the +// wrong host. +func TestNewSCAAccessService_UsesSCAServiceSlug(t *testing.T) { + svc, err := NewSCAAccessService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewSCAAccessService: %v", err) + } + + // The fake JWT carries subdomain=testtenant, platform_domain=example.test. + const want = "https://testtenant.sca.example.test" + if got := ispClientFromService(t, svc).BaseURL; got != want { + t.Errorf("BaseURL = %q, want %q", got, want) + } +} diff --git a/internal/sca/service_test.go b/internal/sca/service_test.go index 78d3901..cb4c09d 100644 --- a/internal/sca/service_test.go +++ b/internal/sca/service_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "net/http" + "reflect" "strings" "testing" @@ -21,9 +22,19 @@ type mockHTTPClient struct { getFunc func(ctx context.Context, route string, params interface{}) (*http.Response, error) // postFunc, when set, overrides postResponse/postError for dynamic responses. postFunc func(ctx context.Context, route string, body interface{}) (*http.Response, error) + + // Recorded arguments of the most recent call. They are populated before the + // call is dispatched, so they are available whichever response mechanism is + // in use. Wire-contract tests assert on them; without them, passing nil + // bodies or a typo'd route is invisible. + gotRoute string + gotBody interface{} + gotParams interface{} } func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface{}) (*http.Response, error) { + m.gotRoute = route + m.gotParams = params if m.getFunc != nil { return m.getFunc(ctx, route, params) } @@ -34,6 +45,8 @@ func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface } func (m *mockHTTPClient) Post(ctx context.Context, route string, body interface{}) (*http.Response, error) { + m.gotRoute = route + m.gotBody = body if m.postFunc != nil { return m.postFunc(ctx, route, body) } @@ -508,45 +521,60 @@ func TestListSessions_Empty(t *testing.T) { } } -func TestListSessions_WithCSPFilter(t *testing.T) { - csp := models.CSPAzure - resp := models.SessionsResponse{ - Response: []models.SessionInfo{ - { - SessionID: "session1", - UserID: "user1", - CSP: models.CSPAzure, - WorkspaceID: "sub1", - RoleID: "role1", - SessionDuration: 3600, - }, - }, - NextToken: nil, - Total: 1, - } +// TestListSessions_SendsCSPQueryParam replaces the former +// TestListSessions_WithCSPFilter, which asserted only that the canned response +// it fed the mock contained CSPAzure — returning nil query params survived it. +// The filter is server-side (grant does no local filtering), so if the param is +// dropped `grant status --provider azure` silently lists every provider. +func TestListSessions_SendsCSPQueryParam(t *testing.T) { + tests := []struct { + name string + csp *models.CSP + wantParams map[string]string // nil means: expect no params at all + }{ + {name: "filter sends csp param", csp: cspPtr(models.CSPAzure), wantParams: map[string]string{"csp": "AZURE"}}, + {name: "aws filter sends its own csp", csp: cspPtr(models.CSPAWS), wantParams: map[string]string{"csp": "AWS"}}, + {name: "no filter sends no params", csp: nil, wantParams: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body, _ := json.Marshal(models.SessionsResponse{ + Response: []models.SessionInfo{{SessionID: "session1", CSP: models.CSPAzure}}, + Total: 1, + }) + mock := &mockHTTPClient{ + getResponse: &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(body))), + }, + } - body, _ := json.Marshal(resp) - mock := &mockHTTPClient{ - getResponse: &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(string(body))), - }, - } + svc := &SCAAccessService{httpClient: mock} + if _, err := svc.ListSessions(t.Context(), tt.csp); err != nil { + t.Fatalf("expected no error, got %v", err) + } - svc := &SCAAccessService{httpClient: mock} - result, err := svc.ListSessions(t.Context(), &csp) + if tt.wantParams == nil { + if mock.gotParams != nil { + t.Fatalf("params = %#v, want nil", mock.gotParams) + } + return + } - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if len(result.Response) != 1 { - t.Errorf("expected 1 session, got %d", len(result.Response)) - } - if result.Response[0].CSP != models.CSPAzure { - t.Errorf("expected CSP AZURE, got %s", result.Response[0].CSP) + got, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + if !reflect.DeepEqual(got, tt.wantParams) { + t.Errorf("params = %#v, want %#v", got, tt.wantParams) + } + }) } } +func cspPtr(c models.CSP) *models.CSP { return &c } + func TestListGroupsEligibility_Success(t *testing.T) { resp := models.GroupsEligibilityResponse{ Response: []models.GroupsEligibleTarget{ diff --git a/internal/sca/wire_contract_test.go b/internal/sca/wire_contract_test.go new file mode 100644 index 0000000..f14286c --- /dev/null +++ b/internal/sca/wire_contract_test.go @@ -0,0 +1,326 @@ +package sca + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/sca/models" +) + +// Wire contracts: what grant actually *sends*. The mocks used to discard the +// route, the body and the query params, so a typo'd path or a nil body was +// invisible to every test in this package. + +func okBody(v interface{}) *http.Response { + b, _ := json.Marshal(v) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(b))), + } +} + +func TestElevate_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(models.ElevateResponse{}), + } + svc := &SCAAccessService{httpClient: mock} + + // Distinguishable values: every field differs from every other so a swap + // or a dropped field cannot be masked. Do not "tidy" these to "test". + req := &models.ElevateRequest{ + CSP: models.CSPAzure, + OrganizationID: "org-elevate-1", + Targets: []models.ElevateTarget{ + {WorkspaceID: "ws-elevate-2", RoleID: "role-elevate-3", RoleName: "Role Elevate Four"}, + }, + } + if _, err := svc.Elevate(t.Context(), req); err != nil { + t.Fatalf("Elevate: %v", err) + } + + if mock.gotRoute != "/api/access/elevate" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/elevate") + } + got, ok := mock.gotBody.(*models.ElevateRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.ElevateRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +} + +func TestRevokeSessions_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(models.RevokeResponse{}), + } + svc := &SCAAccessService{httpClient: mock} + + req := &models.RevokeRequest{SessionIDs: []string{"sess-revoke-1", "sess-revoke-2"}} + if _, err := svc.RevokeSessions(t.Context(), req); err != nil { + t.Fatalf("RevokeSessions: %v", err) + } + + if mock.gotRoute != "/api/access/sessions/revoke" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/sessions/revoke") + } + got, ok := mock.gotBody.(*models.RevokeRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.RevokeRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got.SessionIDs, req.SessionIDs) { + t.Errorf("sessionIds = %#v, want %#v", got.SessionIDs, req.SessionIDs) + } +} + +func TestElevateGroups_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(map[string]interface{}{"response": models.GroupsElevateResponse{}}), + } + svc := &SCAAccessService{httpClient: mock} + + req := &models.GroupsElevateRequest{ + DirectoryID: "dir-groups-1", + CSP: models.CSPAzure, + Targets: []models.GroupsElevateTarget{ + {GroupID: "grp-groups-2"}, + }, + } + if _, err := svc.ElevateGroups(t.Context(), req); err != nil { + t.Fatalf("ElevateGroups: %v", err) + } + + if mock.gotRoute != "/api/access/elevate/groups" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/elevate/groups") + } + got, ok := mock.gotBody.(*models.GroupsElevateRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.GroupsElevateRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +} + +func TestGetRoutes_Exact(t *testing.T) { + tests := []struct { + name string + call func(svc *SCAAccessService) error + wantRoute string + }{ + { + name: "list sessions", + call: func(svc *SCAAccessService) error { + _, err := svc.ListSessions(t.Context(), nil) + return err + }, + wantRoute: "/api/access/sessions", + }, + { + name: "list eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListEligibility(t.Context(), models.CSPAzure) + return err + }, + wantRoute: "/api/access/AZURE/eligibility", + }, + { + name: "list groups eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListGroupsEligibility(t.Context(), models.CSPAzure) + return err + }, + wantRoute: "/api/access/AZURE/eligibility/groups", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFunc: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"response":[],"total":0}`)), + }, nil + }, + } + svc := &SCAAccessService{httpClient: mock} + if err := tt.call(svc); err != nil { + t.Fatalf("call: %v", err) + } + if mock.gotRoute != tt.wantRoute { + t.Errorf("route = %q, want %q", mock.gotRoute, tt.wantRoute) + } + }) + } +} + +// TestListOnDemandResources_ExactQueryParams pins the literal values grant puts +// on the wire for the on-demand role endpoints. It deliberately asserts only the +// values sent: the semantics of pageSize -1 are not documented in this repo, the +// pinned SDK, or CyberArk's published material, so no consequence is claimed. +func TestListOnDemandResources_ExactQueryParams(t *testing.T) { + t.Run("GET search object", func(t *testing.T) { + mock := &mockHTTPClient{ + getResponse: okBody([]models.OnDemandResource{}), + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListOnDemandResources(t.Context(), models.OnDemandRequest{ + WorkspaceID: "ws-ondemand-1", + PlatformName: "azure_ad", + OrgID: "org-ondemand-2", + }) + if err != nil { + t.Fatalf("ListOnDemandResources: %v", err) + } + + if mock.gotRoute != "/api/cloud/resources/ondemand" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/cloud/resources/ondemand") + } + params, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + var search map[string]interface{} + if err := json.Unmarshal([]byte(params["search"]), &search); err != nil { + t.Fatalf("search param is not JSON: %v (%q)", err, params["search"]) + } + want := map[string]interface{}{ + "workspaceId": "ws-ondemand-1", + "pageNumber": float64(-1), + "pageSize": float64(-1), + "platformName": "azure_ad", + "org_id": "org-ondemand-2", + "target_category": "cloud_console", + } + if !reflect.DeepEqual(search, want) { + t.Errorf("search = %#v, want %#v", search, want) + } + }) + + t.Run("POST body", func(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody([]models.OnDemandResource{}), + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListOnDemandResources(t.Context(), models.OnDemandRequest{ + WorkspaceID: "ws-ondemand-3", + PlatformName: "azure_resource", + OrgID: "org-ondemand-4", + ResourceType: "management_group", + Ancestors: []string{"/org-ondemand-4", "/mg-ondemand-5"}, + }) + if err != nil { + t.Fatalf("ListOnDemandResources: %v", err) + } + + if mock.gotRoute != "/api/cloud/cloud-roles/ondemand" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/cloud/cloud-roles/ondemand") + } + body, ok := mock.gotBody.(map[string]interface{}) + if !ok { + t.Fatalf("body = %#v (%T), want map[string]interface{}", mock.gotBody, mock.gotBody) + } + want := map[string]interface{}{ + "workspaceId": "ws-ondemand-3", + "resourceType": "management_group", + "pageNumber": -1, + "pageSize": -1, + "platformName": "azure_resource", + "org_id": "org-ondemand-4", + "ancestors": []string{"/org-ondemand-4", "/mg-ondemand-5"}, + "target_category": "cloud_console", + } + if !reflect.DeepEqual(body, want) { + t.Errorf("body = %#v, want %#v", body, want) + } + }) +} + +// TestPaginate_PropagatesContextCancellation covers the ctx argument of the +// paginated GET. Every mock in this package ignores ctx, so replacing it with +// context.Background() otherwise survives untouched. +func TestPaginate_PropagatesContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + mock := &mockHTTPClient{ + getFunc: func(ctx context.Context, _ string, _ interface{}) (*http.Response, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return okBody(models.SessionsResponse{}), nil + }, + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListSessions(ctx, nil) + if err == nil { + t.Fatal("expected the canceled context to reach the HTTP client, got nil error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want context.Canceled", err) + } +} + +// TestPaginate_PropagatesDecodeError covers all three paginated decode closures. +// Swallowing the decode error there would turn a malformed payload into an +// empty, successful result. +func TestPaginate_PropagatesDecodeError(t *testing.T) { + tests := []struct { + name string + call func(svc *SCAAccessService) error + }{ + { + name: "eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListEligibility(t.Context(), models.CSPAzure) + return err + }, + }, + { + name: "sessions", + call: func(svc *SCAAccessService) error { + _, err := svc.ListSessions(t.Context(), nil) + return err + }, + }, + { + name: "groups eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListGroupsEligibility(t.Context(), models.CSPAzure) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFunc: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"response": "not-an-array"}`)), + }, nil + }, + } + svc := &SCAAccessService{httpClient: mock} + + err := tt.call(svc) + if err == nil { + t.Fatal("expected a decode error, got nil") + } + if !strings.Contains(err.Error(), "failed to decode") { + t.Errorf("error = %v, want it to name the decode failure", err) + } + }) + } +} diff --git a/internal/workflows/logging_client_test.go b/internal/workflows/logging_client_test.go new file mode 100644 index 0000000..ba7ec5b --- /dev/null +++ b/internal/workflows/logging_client_test.go @@ -0,0 +1,317 @@ +package workflows + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "testing" +) + +// This file mirrors internal/sca/logging_client_test.go. The two logging +// clients are byte-identical, but this one had no test at all while sitting on +// the live path — NewAccessRequestService wraps every UAR request in it. + +type mockLogger struct { + calls []logCall +} + +type logCall struct { + level string + msg string +} + +func (m *mockLogger) Info(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "info", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) Error(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "error", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) Debug(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "debug", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) has(level, substr string) bool { + for _, c := range m.calls { + if c.level == level && strings.Contains(c.msg, substr) { + return true + } + } + return false +} + +func TestLoggingClient_Get(t *testing.T) { + tests := []struct { + name string + route string + resp *http.Response + err error + wantLevel string + wantSubstr string + }{ + { + name: "success logs method route and status", + route: "/api/workflows/requests", + resp: &http.Response{StatusCode: 200, Header: http.Header{}}, + wantLevel: "info", + wantSubstr: "GET /api/workflows/requests -> 200", + }, + { + name: "error logs method route and error", + route: "/api/workflows/requests", + err: errors.New("connection refused"), + wantLevel: "error", + wantSubstr: "GET /api/workflows/requests failed: connection refused", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{getResponse: tt.resp, getError: tt.err} + lc := newLoggingClient(inner, ml) + + resp, err := lc.Get(t.Context(), tt.route, nil) + + if tt.err != nil { + if err == nil { + t.Fatal("expected error, got nil") + } + } else { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if resp.StatusCode != tt.resp.StatusCode { + t.Errorf("expected status %d, got %d", tt.resp.StatusCode, resp.StatusCode) + } + } + + if !ml.has(tt.wantLevel, tt.wantSubstr) { + t.Errorf("expected %s log containing %q, got calls: %v", tt.wantLevel, tt.wantSubstr, ml.calls) + } + }) + } +} + +func TestLoggingClient_Post(t *testing.T) { + tests := []struct { + name string + route string + resp *http.Response + err error + wantLevel string + wantSubstr string + }{ + { + name: "success logs method route and status", + route: "/api/workflows/requests", + resp: &http.Response{StatusCode: 200, Header: http.Header{}}, + wantLevel: "info", + wantSubstr: "POST /api/workflows/requests -> 200", + }, + { + name: "error logs method route and error", + route: "/api/workflows/requests", + err: errors.New("timeout"), + wantLevel: "error", + wantSubstr: "POST /api/workflows/requests failed: timeout", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{postResponse: tt.resp, postError: tt.err} + lc := newLoggingClient(inner, ml) + + resp, err := lc.Post(t.Context(), tt.route, nil) + + if tt.err != nil { + if err == nil { + t.Fatal("expected error, got nil") + } + } else { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if resp.StatusCode != tt.resp.StatusCode { + t.Errorf("expected status %d, got %d", tt.resp.StatusCode, resp.StatusCode) + } + } + + if !ml.has(tt.wantLevel, tt.wantSubstr) { + t.Errorf("expected %s log containing %q, got calls: %v", tt.wantLevel, tt.wantSubstr, ml.calls) + } + }) + } +} + +// TestLoggingClient_GetUsesGet and its Post twin pin the verb dispatch: routing +// Get through inner.Post would otherwise be invisible. +func TestLoggingClient_GetUsesGet(t *testing.T) { + var gotGet, gotPost bool + inner := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotGet = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotPost = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + } + lc := newLoggingClient(inner, &mockLogger{}) + + if _, err := lc.Get(t.Context(), "/api/workflows/requests", nil); err != nil { + t.Fatalf("Get: %v", err) + } + if !gotGet { + t.Error("loggingClient.Get did not call inner.Get") + } + if gotPost { + t.Error("loggingClient.Get called inner.Post") + } +} + +func TestLoggingClient_PostUsesPost(t *testing.T) { + var gotGet, gotPost bool + inner := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotGet = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotPost = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + } + lc := newLoggingClient(inner, &mockLogger{}) + + if _, err := lc.Post(t.Context(), "/api/workflows/requests", nil); err != nil { + t.Fatalf("Post: %v", err) + } + if !gotPost { + t.Error("loggingClient.Post did not call inner.Post") + } + if gotGet { + t.Error("loggingClient.Post called inner.Get") + } +} + +// TestLoggingClient_PropagatesInnerError guards against the decorator +// swallowing the inner error and manufacturing a success. +func TestLoggingClient_PropagatesInnerError(t *testing.T) { + sentinel := errors.New("inner transport failure") + + t.Run("get", func(t *testing.T) { + lc := newLoggingClient(&mockHTTPClient{getError: sentinel}, &mockLogger{}) + resp, err := lc.Get(t.Context(), "/api/workflows/requests", nil) + if !errors.Is(err, sentinel) { + t.Errorf("error = %v, want the inner error", err) + } + if resp != nil { + t.Errorf("response = %#v, want nil on error", resp) + } + }) + + t.Run("post", func(t *testing.T) { + lc := newLoggingClient(&mockHTTPClient{postError: sentinel}, &mockLogger{}) + resp, err := lc.Post(t.Context(), "/api/workflows/requests", nil) + if !errors.Is(err, sentinel) { + t.Errorf("error = %v, want the inner error", err) + } + if resp != nil { + t.Errorf("response = %#v, want nil on error", resp) + } + }) +} + +func TestLoggingClient_LogsDuration(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{getResponse: &http.Response{StatusCode: 200, Header: http.Header{}}} + lc := newLoggingClient(inner, ml) + + _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) + + if !ml.has("info", "ms)") { + t.Errorf("expected info log containing duration in ms, got calls: %v", ml.calls) + } +} + +// TestLoggingClient_DebugLogsHeaders is the token-leak guard: response headers +// go to the debug log verbatim except Authorization, which must be redacted. +func TestLoggingClient_DebugLogsHeaders(t *testing.T) { + ml := &mockLogger{} + h := http.Header{} + h.Set("Content-Type", "application/json") + h.Set("Authorization", "Bearer eyJhbGci.secret.token") + inner := &mockHTTPClient{getResponse: &http.Response{StatusCode: 200, Header: h}} + lc := newLoggingClient(inner, ml) + + _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) + + var debugCall *logCall + for i, c := range ml.calls { + if c.level == "debug" { + debugCall = &ml.calls[i] + break + } + } + if debugCall == nil { + t.Fatal("expected debug log for response headers") + } + if strings.Contains(debugCall.msg, "eyJhbGci") { + t.Error("expected Authorization header to be redacted, but found token value") + } + if !strings.Contains(debugCall.msg, "[REDACTED]") { + t.Error("expected [REDACTED] in debug log for Authorization header") + } + if !strings.Contains(debugCall.msg, "application/json") { + t.Error("expected Content-Type header in debug log") + } +} + +func TestRedactHeaders(t *testing.T) { + tests := []struct { + name string + headers http.Header + wantHas string + wantNot string + }{ + { + name: "redacts Authorization", + headers: func() http.Header { + h := http.Header{} + h.Set("Authorization", "Bearer eyJtoken123") + return h + }(), + wantHas: "Bearer [REDACTED]", + wantNot: "eyJtoken123", + }, + { + name: "preserves other headers", + headers: func() http.Header { + h := http.Header{} + h.Set("Content-Type", "application/json") + return h + }(), + wantHas: "application/json", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + str := fmt.Sprintf("%v", redactHeaders(tt.headers)) + + if tt.wantHas != "" && !strings.Contains(str, tt.wantHas) { + t.Errorf("expected %q in result %q", tt.wantHas, str) + } + if tt.wantNot != "" && strings.Contains(str, tt.wantNot) { + t.Errorf("did not expect %q in result %q", tt.wantNot, str) + } + }) + } +} diff --git a/internal/workflows/models/wire_tags_test.go b/internal/workflows/models/wire_tags_test.go new file mode 100644 index 0000000..11a82ff --- /dev/null +++ b/internal/workflows/models/wire_tags_test.go @@ -0,0 +1,90 @@ +package models + +import ( + "encoding/json" + "testing" +) + +// These pin the literal JSON keys of the three request bodies grant sends to +// the UAR API. Round-tripping through the same Go struct cannot catch a renamed +// tag, so each test inspects the marshaled keys directly. + +func TestSubmitAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + b, err := json.Marshal(SubmitAccessRequest{ + TargetCategory: "CLOUD_CONSOLE", + RequestDetails: map[string]interface{}{"reason": "need access to prod"}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"targetCategory", "requestDetails"} { + if _, ok := raw[key]; !ok { + t.Errorf("submit body is missing key %q: %s", key, b) + } + } + if string(raw["targetCategory"]) != `"CLOUD_CONSOLE"` { + t.Errorf("targetCategory = %s, want \"CLOUD_CONSOLE\"", raw["targetCategory"]) + } + if string(raw["requestDetails"]) != `{"reason":"need access to prod"}` { + t.Errorf("requestDetails = %s", raw["requestDetails"]) + } +} + +func TestFinalizeAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + reason := "approved after review" + b, err := json.Marshal(FinalizeAccessRequest{Result: "APPROVED", FinalizationReason: &reason}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if string(raw["result"]) != `"APPROVED"` { + t.Errorf("result = %s, want \"APPROVED\" under key \"result\": %s", raw["result"], b) + } + if string(raw["finalizationReason"]) != `"approved after review"` { + t.Errorf("finalizationReason = %s: %s", raw["finalizationReason"], b) + } + + // omitempty: a nil reason must not appear at all. + b, err = json.Marshal(FinalizeAccessRequest{Result: "REJECTED"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + raw = nil + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, ok := raw["finalizationReason"]; ok { + t.Errorf("finalizationReason present for a nil reason: %s", b) + } +} + +func TestCancelAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + reason := "no longer needed" + b, err := json.Marshal(CancelAccessRequest{CancelReason: &reason}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if string(raw["cancelReason"]) != `"no longer needed"` { + t.Errorf("cancelReason = %s: %s", raw["cancelReason"], b) + } +} diff --git a/internal/workflows/service_config_test.go b/internal/workflows/service_config_test.go new file mode 100644 index 0000000..d2d0cb4 --- /dev/null +++ b/internal/workflows/service_config_test.go @@ -0,0 +1,92 @@ +package workflows + +import ( + "testing" + + "github.com/cyberark/idsec-sdk-golang/pkg/common/isp" + "github.com/cyberark/idsec-sdk-golang/pkg/services" +) + +func TestServiceConfig_ServiceName(t *testing.T) { + config := ServiceConfig() + expected := "access-requests" + if config.ServiceName != expected { + t.Errorf("expected ServiceName %q, got %q", expected, config.ServiceName) + } +} + +func TestServiceConfig_RequiredAuthenticators(t *testing.T) { + config := ServiceConfig() + if len(config.RequiredAuthenticatorNames) != 1 { + t.Fatalf("expected 1 required authenticator, got %d", len(config.RequiredAuthenticatorNames)) + } + expected := "isp" + if config.RequiredAuthenticatorNames[0] != expected { + t.Errorf("expected required authenticator %q, got %q", expected, config.RequiredAuthenticatorNames[0]) + } +} + +func TestServiceConfig_OptionalAuthenticators(t *testing.T) { + config := ServiceConfig() + if len(config.OptionalAuthenticatorNames) != 0 { + t.Errorf("expected 0 optional authenticators, got %d", len(config.OptionalAuthenticatorNames)) + } +} + +func TestServiceConfig_ActionsConfigurations(t *testing.T) { + config := ServiceConfig() + if config.ActionsConfigurations != nil { + t.Errorf("expected nil ActionsConfigurations, got %v", config.ActionsConfigurations) + } +} + +func TestServiceConfig_ReturnsIdsecServiceConfig(t *testing.T) { + config := ServiceConfig() + // Verify it returns the correct SDK type + var _ services.IdsecServiceConfig = config +} + +// TestNewAccessRequestService_ResolvesISPAuthenticator pins the authenticator +// name the constructor looks up. Asking for anything other than "isp" makes the +// constructor fail outright. +func TestNewAccessRequestService_ResolvesISPAuthenticator(t *testing.T) { + svc, err := NewAccessRequestService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewAccessRequestService: %v", err) + } + if svc.ispAuth == nil { + t.Error("ispAuth = nil, want the resolved ISP authenticator") + } +} + +// TestNewAccessRequestService_UsesUARServiceSlug pins the "uar" service slug +// passed to isp.FromISPAuth. The retry test overwrites BaseURL before issuing a +// request, so without this the slug could be changed to anything and no test in +// the repo would notice — while every live request would go to the wrong host. +func TestNewAccessRequestService_UsesUARServiceSlug(t *testing.T) { + svc, err := NewAccessRequestService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewAccessRequestService: %v", err) + } + + // The fake JWT carries subdomain=testtenant, platform_domain=example.test. + const want = "https://testtenant.uar.example.test" + if got := ispClientFromService(t, svc).BaseURL; got != want { + t.Errorf("BaseURL = %q, want %q", got, want) + } +} + +// ispClientFromService reaches through the logging decorator to the SDK client +// the constructor actually published. +func ispClientFromService(t *testing.T, svc *AccessRequestService) *isp.IdsecISPServiceClient { + t.Helper() + lc, ok := svc.httpClient.(*loggingClient) + if !ok { + t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient) + } + client, ok := lc.inner.(*isp.IdsecISPServiceClient) + if !ok { + t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner) + } + return client +} diff --git a/internal/workflows/service_test.go b/internal/workflows/service_test.go index c696005..5c507dc 100644 --- a/internal/workflows/service_test.go +++ b/internal/workflows/service_test.go @@ -15,14 +15,42 @@ import ( type mockHTTPClient struct { getFn func(ctx context.Context, route string, params interface{}) (*http.Response, error) postFn func(ctx context.Context, route string, body interface{}) (*http.Response, error) + + // Fallbacks used when the corresponding Fn is nil. + getResponse *http.Response + getError error + postResponse *http.Response + postError error + + // Recorded arguments of the most recent call, populated before dispatch so + // wire-contract tests can assert what was actually sent. + gotRoute string + gotBody interface{} + gotParams interface{} } func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface{}) (*http.Response, error) { - return m.getFn(ctx, route, params) + m.gotRoute = route + m.gotParams = params + if m.getFn != nil { + return m.getFn(ctx, route, params) + } + if m.getError != nil { + return nil, m.getError + } + return m.getResponse, nil } func (m *mockHTTPClient) Post(ctx context.Context, route string, body interface{}) (*http.Response, error) { - return m.postFn(ctx, route, body) + m.gotRoute = route + m.gotBody = body + if m.postFn != nil { + return m.postFn(ctx, route, body) + } + if m.postError != nil { + return nil, m.postError + } + return m.postResponse, nil } func jsonResponse(status int, body interface{}) *http.Response { diff --git a/internal/workflows/wire_contract_test.go b/internal/workflows/wire_contract_test.go new file mode 100644 index 0000000..08deb34 --- /dev/null +++ b/internal/workflows/wire_contract_test.go @@ -0,0 +1,271 @@ +package workflows + +import ( + "context" + "errors" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/workflows/models" +) + +// errorResponse builds a non-200 whose body still decodes cleanly into every +// response model. That is deliberate: if the checkResponse guard is removed, +// the operation succeeds and returns an empty value rather than failing, which +// is exactly the escape this table exists to catch. +func errorResponse(status int) *http.Response { + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(`{"message":"internal server error"}`)), + Header: make(http.Header), + } +} + +// TestWorkflows_Non200 drives a 500 through all six checkResponse call sites. +// TestCheckResponse_Error only calls the helper directly, which proves it +// formats an error but not that any operation invokes it — so deleting any of +// the six guards survived. The user-visible consequence at the submit site: a +// 500 decodes as an empty AccessRequest and `grant request submit` prints a +// blank request and exits 0. +func TestWorkflows_Non200(t *testing.T) { + tests := []struct { + name string + call func(svc *AccessRequestService) error + wantOperation string + }{ + { + name: "request forms", + call: func(svc *AccessRequestService) error { + _, err := svc.GetRequestForms(t.Context(), "CLOUD_CONSOLE", "ON_DEMAND") + return err + }, + wantOperation: "request forms", + }, + { + name: "list requests", + call: func(svc *AccessRequestService) error { + _, _, err := svc.ListRequests(t.Context(), ListRequestsParams{}) + return err + }, + wantOperation: "list requests", + }, + { + name: "get request", + call: func(svc *AccessRequestService) error { + _, err := svc.GetRequest(t.Context(), "req-500") + return err + }, + wantOperation: "get request", + }, + { + name: "submit request", + call: func(svc *AccessRequestService) error { + _, err := svc.SubmitRequest(t.Context(), &models.SubmitAccessRequest{TargetCategory: "CLOUD_CONSOLE"}) + return err + }, + wantOperation: "submit request", + }, + { + name: "cancel request", + call: func(svc *AccessRequestService) error { + _, err := svc.CancelRequest(t.Context(), "req-500", nil) + return err + }, + wantOperation: "cancel request", + }, + { + name: "finalize request", + call: func(svc *AccessRequestService) error { + _, err := svc.FinalizeRequest(t.Context(), "req-500", "APPROVED", nil) + return err + }, + wantOperation: "finalize request", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return errorResponse(http.StatusInternalServerError), nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return errorResponse(http.StatusInternalServerError), nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + err := tt.call(svc) + if err == nil { + t.Fatalf("%s returned nil error on HTTP 500", tt.wantOperation) + } + if !strings.Contains(err.Error(), tt.wantOperation) { + t.Errorf("error = %v, want it to name the operation %q", err, tt.wantOperation) + } + if !strings.Contains(err.Error(), "500") { + t.Errorf("error = %v, want it to carry the HTTP status", err) + } + }) + } +} + +// TestFinalizeRequest_ExactRouteAndReason replaces the HasSuffix("/finalize") +// check, which let the request ID drop out of the route and let the approver's +// reason be dropped from the body. The cancel twin already asserts both. +func TestFinalizeRequest_ExactRouteAndReason(t *testing.T) { + tests := []struct { + name string + result string + reason *string + }{ + {name: "approve with reason", result: "APPROVED", reason: strPtr("approved because it is justified")}, + {name: "reject with reason", result: "REJECTED", reason: strPtr("rejected because scope is too broad")}, + {name: "no reason", result: "APPROVED", reason: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: jsonResponse(200, models.AccessRequest{RequestID: "req-finalize-1"}), + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, err := svc.FinalizeRequest(t.Context(), "req-finalize-1", tt.result, tt.reason); err != nil { + t.Fatalf("FinalizeRequest: %v", err) + } + + const wantRoute = "/api/workflows/requests/req-finalize-1/finalize" + if mock.gotRoute != wantRoute { + t.Errorf("route = %q, want %q", mock.gotRoute, wantRoute) + } + body, ok := mock.gotBody.(*models.FinalizeAccessRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.FinalizeAccessRequest", mock.gotBody, mock.gotBody) + } + if body.Result != tt.result { + t.Errorf("result = %q, want %q", body.Result, tt.result) + } + switch { + case tt.reason == nil && body.FinalizationReason != nil: + t.Errorf("finalizationReason = %q, want nil", *body.FinalizationReason) + case tt.reason != nil && body.FinalizationReason == nil: + t.Errorf("finalizationReason = nil, want %q", *tt.reason) + case tt.reason != nil && *body.FinalizationReason != *tt.reason: + t.Errorf("finalizationReason = %q, want %q", *body.FinalizationReason, *tt.reason) + } + }) + } +} + +func strPtr(s string) *string { return &s } + +// TestListRequests_SendsLimit pins the limit query parameter. Offset pagination +// is already well covered; limit was never asserted, so deleting it, or +// changing defaultPageSize, went unnoticed. +func TestListRequests_SendsLimit(t *testing.T) { + tests := []struct { + name string + params ListRequestsParams + wantLimit string + }{ + {name: "no limit uses defaultPageSize", params: ListRequestsParams{}, wantLimit: "50"}, + {name: "zero limit uses defaultPageSize", params: ListRequestsParams{Limit: 0}, wantLimit: "50"}, + {name: "negative limit uses defaultPageSize", params: ListRequestsParams{Limit: -3}, wantLimit: "50"}, + {name: "caller limit is sent verbatim", params: ListRequestsParams{Limit: 7}, wantLimit: "7"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getResponse: jsonResponse(200, models.ListRequestsResponse{ + Items: []models.AccessRequest{{RequestID: "id-1"}}, Count: 1, TotalCount: 1, + }), + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, _, err := svc.ListRequests(t.Context(), tt.params); err != nil { + t.Fatalf("ListRequests: %v", err) + } + + qp, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + if got := qp["limit"]; got != tt.wantLimit { + t.Errorf("limit = %q, want %q", got, tt.wantLimit) + } + }) + } +} + +// TestListRequests_PropagatesContextCancellation covers the ctx argument of the +// pagination loop's GET; every mock ignores ctx, so swapping it for +// context.Background() otherwise survives. +func TestListRequests_PropagatesContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + mock := &mockHTTPClient{ + getFn: func(ctx context.Context, _ string, _ interface{}) (*http.Response, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return jsonResponse(200, models.ListRequestsResponse{}), nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + _, _, err := svc.ListRequests(ctx, ListRequestsParams{}) + if err == nil { + t.Fatal("expected the canceled context to reach the HTTP client, got nil error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want context.Canceled", err) + } +} + +// TestGetRequest_PropagatesDecodeError guards the decode error return in +// GetRequest: swallowing it would return a blank request as a success. +func TestGetRequest_PropagatesDecodeError(t *testing.T) { + mock := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"requestId": 12345}`)), + Header: make(http.Header), + }, nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + result, err := svc.GetRequest(t.Context(), "req-decode-1") + if err == nil { + t.Fatalf("expected a decode error, got nil (result = %#v)", result) + } + if !strings.Contains(err.Error(), "failed to decode") { + t.Errorf("error = %v, want it to name the decode failure", err) + } +} + +// TestGetRequestForms_SendsExactParams pins the query params for the forms +// endpoint alongside its route. +func TestGetRequestForms_SendsExactParams(t *testing.T) { + mock := &mockHTTPClient{ + getResponse: jsonResponse(200, models.RequestFormResponse{}), + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, err := svc.GetRequestForms(t.Context(), "CLOUD_CONSOLE", "ON_DEMAND"); err != nil { + t.Fatalf("GetRequestForms: %v", err) + } + if mock.gotRoute != "/api/workflows/request-forms" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/request-forms") + } + want := map[string]string{"targetCategory": "CLOUD_CONSOLE", "requestType": "ON_DEMAND"} + if !reflect.DeepEqual(mock.gotParams, want) { + t.Errorf("params = %#v, want %#v", mock.gotParams, want) + } +} From f0faa868207c02d085c0b64d180ad3ff0e58720a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:02:57 +0200 Subject: [PATCH 2/7] docs(ledger): correct PR8 test names and record the two reproduced verdicts Twelve PR8 rows named tests that the implementer consolidated away. Point each row at the test that actually exists, and say explicitly where several rows now share one test rather than repeating a name with no note. SCA-18 was recorded as OVERSTATED/refuted on a "not reproducible" verdict that came from a sandbox without loopback TCP. It has now been reproduced on both services (inbound requests = 4, want 1), so it flips to CONFIRMED/test naming the two retry-policy tests as its killers. SCA-15's identical clause is dropped: both X-API-Version mutants were executed and both fail. Record the reusable lesson in the preamble: a bare deletion of sdkclient.DisableTransientRetry orphans the import and is a compile kill, not a test kill; the honest mutation is the assignment form. Summary counts updated. Also: soften two test comments that claimed to replace assertions still present elsewhere, and assert the error message in the credentials "empty string" row so the previously inert empty-string guard is killed. --- docs/mutation-ledger.md | 73 ++++++++++++++---------- internal/sca/models/credentials_test.go | 15 ++++- internal/sca/service_client_test.go | 7 ++- internal/workflows/wire_contract_test.go | 7 ++- 4 files changed, 64 insertions(+), 38 deletions(-) diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 38e7c87..c9f7830 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -24,6 +24,20 @@ shifted by +13 for the non-interactive guard this branch inserts at are noted in the Mutation cell with `(was ...)`. **File:line is always the production site, never the test site.** +**Mutating a call whose only purpose is its side effect.** A bare deletion of +`sdkclient.DisableTransientRetry(...)` orphans the `internal/sdkclient` import, so +the build fails — that is a **compile kill, not a test kill**, and it proves +nothing about coverage. The honest mutation is the assignment form +`_ = sdkclient.DisableTransientRetry`, which keeps the import used so only +behaviour changes. This tripped an earlier verification pass (see SCA-18). The +same shape applies to any mutation that would leave an identifier or import +unused: use the semantic form, not a token deletion. + +**A sandbox that blocks loopback TCP cannot refute a row.** Several rows were once +recorded as "not reproducible" purely because the verifier could not bind an +`httptest` server. That is an environmental limitation, not a property of the code; +re-run such rows in an environment with loopback before recording a verdict. + **Verdicts** are the verifiers' conclusions, not the original reports': `CONFIRMED` (survivor reproduced), `OVERSTATED` (survivor real, stated consequence weaker than claimed), `REFUTED` (the claim is false — the mutant dies, or the @@ -88,23 +102,23 @@ premise does not hold). | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | -| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | done | -| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | done | -| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | done | -| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | done | -| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | done | -| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | done | -| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | done | -| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | done | +| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactRouteAndBody` (added `gotRoute`/`gotBody` to `mockHTTPClient`) — one test, shared with SCA-05 | PR8 | done | +| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactRouteAndBody` — one test, shared with SCA-07 | PR8 | done | +| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactRouteAndBody` — one test, shared with SCA-09 | PR8 | done | +| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestElevate_SendsExactRouteAndBody` (same test as SCA-02; it asserts route **and** body) | PR8 | done | +| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestGetRoutes_Exact/list_sessions` | PR8 | done | +| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestRevokeSessions_SendsExactRouteAndBody` (same test as SCA-03) | PR8 | done | +| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestGetRoutes_Exact/list_groups_eligibility` | PR8 | done | +| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestElevateGroups_SendsExactRouteAndBody` (same test as SCA-04) | PR8 | done | | SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | | SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | | SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | done | | SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | done | -| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | done | -| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | done | +| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestPaginate_PropagatesDecodeError` | PR8 | done | +| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lived inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Both mutants were subsequently executed and both fail `service_client_test.go:52` (deletion → `X-API-Version = ""`; `"1.0"` → `X-API-Version = "1.0"`) | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | done | | SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | done | | SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | done | -| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | done | +| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | `_ = sdkclient.DisableTransientRetry` (the assignment form — a bare deletion orphans the import and is a compile kill, not a test kill). Reproduced on **both** services exactly as claimed: `inbound requests = 4, want 1` from `internal/sca/retry_policy_test.go:91` and `internal/workflows/retry_policy_test.go:90` | CONFIRMED | test | `TestNewSCAAccessServiceDisablesTransientRetry` and `TestNewAccessRequestServiceDisablesTransientRetry` (both pre-existing and correctly aimed; PR8 executed them against the mutant) | PR8 | done | | WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | done | | WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | | WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | @@ -117,10 +131,10 @@ premise does not hold). | WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | done | | WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | done | | WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | done | -| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | done | -| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | done | +| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_ExactRouteAndReason` — one test, shared with WF-14 | PR8 | done | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | Folded into `TestFinalizeRequest_ExactRouteAndReason` (same test as WF-13) | PR8 | done | | WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | done | -| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | done | +| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_SendsLimit` (same test as WF-15; its `defaultPageSize` rows pin the 50) | PR8 | done | | WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | done | | WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | done | | WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | @@ -218,27 +232,27 @@ premise does not hold). | PR5 — Output contracts | 32 | 30 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | -| PR8 — SCA / workflows / models wire contracts | 38 | 36 | -| *(no PR — settled, recorded only)* | 5 | 0 | -| **Total** | **167** | **154** | +| PR8 — SCA / workflows / models wire contracts | 39 | 37 | +| *(no PR — settled, recorded only)* | 4 | 0 | +| **Total** | **167** | **155** | ### By verdict | Verdict | Rows | |---|---| -| CONFIRMED | 154 | -| OVERSTATED | 9 | +| CONFIRMED | 155 | +| OVERSTATED | 8 | | REFUTED | 4 | ### By disposition | Disposition | Rows | |---|---| -| `test` | 151 | +| `test` | 152 | | `test + prod-fix` | 5 | | `prod-fix` | 1 | | `wont-fix` | 5 | -| `refuted` | 5 | +| `refuted` | 4 | | **Total** | **167** | The seven production changes, matching the plan's table: @@ -255,10 +269,11 @@ The seven production changes, matching the plan's table: ### Total CONFIRMED -**152 CONFIRMED**, plus **9 OVERSTATED** (real survivors whose stated consequence -was weaker than originally claimed) and **4 REFUTED**. **165 rows total.** -5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10), so **160 rows require -work**, of which 6 carry a production change. +**155 CONFIRMED**, plus **8 OVERSTATED** (real survivors whose stated consequence +was weaker than originally claimed) and **4 REFUTED**. **167 rows total.** +5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10) and 4 are closed by +review (`refuted`), so **158 rows require work**, of which 6 carry a production +change. ### Reconciliation against "145" @@ -270,14 +285,14 @@ to a named, independently reproduced finding in a verification report. |---|---|---|---| | 1 — cmd request/auth | 22 confirmed | 23 (22 CONFIRMED + 1 OVERSTATED) | **Yes.** The 22 CONFIRMED rows are mutation-level and match exactly. REQ-23 (integration suite absent from CI) is reported *outside* the 22. | | 2 — cmd status/favorites/list | 25 real of 29 claimed | 29 (23 CONFIRMED, 3 OVERSTATED, 3 REFUTED) | **Approximately.** All 29 claimed items are listed so the three refutations stay recorded. 23 + 3 OVERSTATED = 26 actionable, one above the verifier's "25" — its own prose is imprecise about whether OUT-21/OUT-25/OUT-26 count as "real". | -| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (34 CONFIRMED, 3 OVERSTATED, 1 REFUTED) | **Yes.** 34 CONFIRMED + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The other 3 rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on) and SCA-17/SCA-18, recorded so they are not re-filed. | +| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (35 CONFIRMED, 2 OVERSTATED, 1 REFUTED) | **Yes.** 34 of the CONFIRMED rows + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The remaining rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on), SCA-18 (reproduced by PR8, above the headline) and SCA-17, recorded so it is not re-filed. | | 4 — cmd root/elevate/env | 22 confirmed | 27 (26 CONFIRMED, 1 OVERSTATED) | **No — +5.** The report's headline groups its own sub-lettered mutations inconsistently: H2.1–2.3, H3.1–3.4, M4.1–2, M5.1–3, M6.1–6.4 and M9.1–3 are enumerated individually in the body, each with its own `go test ./cmd/ -count=1 → ok` transcript, but collapsed in the total. Listing them individually gives 25 production mutations plus 2 test-quality rows (ELV-26, ELV-27). | | 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (all CONFIRMED, 4 of them `wont-fix`) | **Partly — +4.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. | -**Net: 165 rows against a headline of 145.** Roughly 9 of the excess is finer +**Net: 167 rows against a headline of 145.** Roughly 9 of the excess is finer enumeration in batches 4 and 5 (mutations the reports reproduced individually but -totalled in groups); the rest is the 13 OVERSTATED/REFUTED rows the headline count -deliberately excluded but which belong here as settled questions. Nothing was +totalled in groups); most of the rest is the 12 OVERSTATED/REFUTED rows the headline +count deliberately excluded but which belong here as settled questions. Nothing was invented and nothing was dropped to hit a number. There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about diff --git a/internal/sca/models/credentials_test.go b/internal/sca/models/credentials_test.go index 794191e..4dc98dc 100644 --- a/internal/sca/models/credentials_test.go +++ b/internal/sca/models/credentials_test.go @@ -2,6 +2,7 @@ package models import ( "encoding/json" + "strings" "testing" ) @@ -54,6 +55,10 @@ func TestParseAWSCredentials(t *testing.T) { name string input string wantErr bool + // Asserted only when wantErr is true and non-empty. Without it the + // empty-string guard is inert: json.Unmarshal rejects "" anyway, so + // deleting the guard changes only the message. + wantErrContains string // Asserted only when wantErr is false. Without these the parser could // swap SecretAccessKey and SessionToken and this package would not // notice — it discarded the parsed value entirely. @@ -71,9 +76,10 @@ func TestParseAWSCredentials(t *testing.T) { wantSessionToken: "TOKENVALUE", }, { - name: "empty string", - input: "", - wantErr: true, + name: "empty string", + input: "", + wantErr: true, + wantErrContains: "empty credentials string", }, { name: "malformed JSON", @@ -100,6 +106,9 @@ func TestParseAWSCredentials(t *testing.T) { t.Fatalf("ParseAWSCredentials() error = %v, wantErr %v", err, tt.wantErr) } if tt.wantErr { + if tt.wantErrContains != "" && !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %v, want it to contain %q", err, tt.wantErrContains) + } return } if creds.AccessKeyID != tt.wantAccessKeyID { diff --git a/internal/sca/service_client_test.go b/internal/sca/service_client_test.go index c9a311f..622c622 100644 --- a/internal/sca/service_client_test.go +++ b/internal/sca/service_client_test.go @@ -24,9 +24,10 @@ func ispClientFromService(t *testing.T, svc *SCAAccessService) *isp.IdsecISPServ } // TestNewSCAAccessService_SetsAPIVersionHeader is the standalone guard for the -// X-API-Version header. It previously existed only as two lines inside -// TestNewSCAAccessServiceDisablesTransientRetry, so any retry-motivated rename -// or deletion of that test would have removed the header guard silently. +// X-API-Version header. It supersedes — but does not remove — the two-line +// assertion inside TestNewSCAAccessServiceDisablesTransientRetry, which remains +// as deliberate redundancy. Without this standalone test, a retry-motivated +// rename or deletion of that test would have dropped the header guard silently. func TestNewSCAAccessService_SetsAPIVersionHeader(t *testing.T) { gotAPIVersion := make(chan string, 1) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/workflows/wire_contract_test.go b/internal/workflows/wire_contract_test.go index 08deb34..fc96540 100644 --- a/internal/workflows/wire_contract_test.go +++ b/internal/workflows/wire_contract_test.go @@ -112,9 +112,10 @@ func TestWorkflows_Non200(t *testing.T) { } } -// TestFinalizeRequest_ExactRouteAndReason replaces the HasSuffix("/finalize") -// check, which let the request ID drop out of the route and let the approver's -// reason be dropped from the body. The cancel twin already asserts both. +// TestFinalizeRequest_ExactRouteAndReason supersedes the HasSuffix("/finalize") +// check in service_test.go, which let the request ID drop out of the route and +// let the approver's reason be dropped from the body. That looser check is still +// present there; the redundancy is harmless. The cancel twin already asserts both. func TestFinalizeRequest_ExactRouteAndReason(t *testing.T) { tests := []struct { name string From ecd3a6072bd0c507e410c3d5895520f3c3899e53 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:35:26 +0200 Subject: [PATCH 3/7] test(workflows): pin the list route and the full submit request body TestListRequests_SendsLimit already recorded mock.gotRoute and never asserted it, leaving /api/workflows/requests the only unpinned route in either service. TestSubmitRequest asserted only TargetCategory, so dropping RequestDetails at the service boundary survived the whole suite. --- internal/workflows/wire_contract_test.go | 55 ++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/internal/workflows/wire_contract_test.go b/internal/workflows/wire_contract_test.go index fc96540..6275410 100644 --- a/internal/workflows/wire_contract_test.go +++ b/internal/workflows/wire_contract_test.go @@ -163,9 +163,11 @@ func TestFinalizeRequest_ExactRouteAndReason(t *testing.T) { func strPtr(s string) *string { return &s } -// TestListRequests_SendsLimit pins the limit query parameter. Offset pagination -// is already well covered; limit was never asserted, so deleting it, or -// changing defaultPageSize, went unnoticed. +// TestListRequests_SendsLimit pins the route and the limit query parameter. +// Offset pagination is already well covered; limit was never asserted, so +// deleting it, or changing defaultPageSize, went unnoticed. The route was the +// only unpinned route in either service — the mock already recorded it and no +// test looked, so `grant request list` could have been pointed anywhere. func TestListRequests_SendsLimit(t *testing.T) { tests := []struct { name string @@ -191,6 +193,9 @@ func TestListRequests_SendsLimit(t *testing.T) { t.Fatalf("ListRequests: %v", err) } + if mock.gotRoute != "/api/workflows/requests" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/requests") + } qp, ok := mock.gotParams.(map[string]string) if !ok { t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) @@ -270,3 +275,47 @@ func TestGetRequestForms_SendsExactParams(t *testing.T) { t.Errorf("params = %#v, want %#v", mock.gotParams, want) } } + +// TestSubmitRequest_SendsExactRouteAndBody pins the submit wire contract. +// TestSubmitRequest asserts only TargetCategory, so RequestDetails — reason, +// role, target, dates, priority, i.e. the entire substance of +// `grant request submit` — could be dropped at the service boundary and every +// test still passed. Mirrors TestElevate_SendsExactRouteAndBody in internal/sca. +func TestSubmitRequest_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: jsonResponse(200, models.AccessRequest{RequestID: "req-submit-1"}), + } + svc := NewAccessRequestServiceWithClient(mock) + + // Distinguishable values: no two keys share a value, so a swap cannot be + // masked. Do not "tidy" these to "test". + req := &models.SubmitAccessRequest{ + TargetCategory: "CLOUD_CONSOLE", + RequestDetails: map[string]interface{}{ + "reason": "reason-submit-2", + "roleId": "role-id-submit-3", + "roleName": "Role Name Submit Four", + "targetId": "target-submit-5", + "priority": "priority-submit-6", + "startDate": "2025-08-12T09:41:00", + "endDate": "2025-08-12T17:41:00", + "timezone": "timezone-submit-7", + "provider": "provider-submit-8", + "workspaceId": "ws-submit-9", + }, + } + if _, err := svc.SubmitRequest(t.Context(), req); err != nil { + t.Fatalf("SubmitRequest: %v", err) + } + + if mock.gotRoute != "/api/workflows/requests" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/requests") + } + got, ok := mock.gotBody.(*models.SubmitAccessRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.SubmitAccessRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +} From bb2f7045b9a8d13f32e4735021f08dfcdb5261e9 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:36:04 +0200 Subject: [PATCH 4/7] test(sca): pin the group-elevation wire model in both directions GroupsElevateTarget.groupId is the whole per-target payload of POST /api/access/elevate/groups, and GroupsElevateTargetResult.sessionId is the only handle for revoking a group session by ID. Five tags across the request and response models survived renaming. --- internal/sca/models/wire_tags_test.go | 96 +++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/internal/sca/models/wire_tags_test.go b/internal/sca/models/wire_tags_test.go index 3a8710a..8c61a7b 100644 --- a/internal/sca/models/wire_tags_test.go +++ b/internal/sca/models/wire_tags_test.go @@ -140,3 +140,99 @@ func TestElevateResponse_AccessCredentialsAbsentOrEmpty(t *testing.T) { }) } } + +// TestGroupsElevateRequest_JSONTags is the twin of TestElevateRequest_JSONTags +// for the group-elevation request body. The nested `groupId` inside `targets` +// is the entire per-target payload of POST /api/access/elevate/groups: if it +// were renamed, every `grant --group` elevation would send +// {"targets":[{"ZgroupId":"..."}]} and group elevation would be broken outright. +func TestGroupsElevateRequest_JSONTags(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap between fields cannot be masked. + req := GroupsElevateRequest{ + DirectoryID: "dir-groups-1", + CSP: CSPAzure, + Targets: []GroupsElevateTarget{ + {GroupID: "group-groups-2"}, + }, + } + b, err := json.Marshal(req) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"directoryId", "csp", "targets"} { + if _, ok := raw[key]; !ok { + t.Errorf("request body is missing key %q: %s", key, b) + } + } + if string(raw["directoryId"]) != `"dir-groups-1"` { + t.Errorf("directoryId = %s, want %q", raw["directoryId"], "dir-groups-1") + } + if string(raw["csp"]) != `"AZURE"` { + t.Errorf("csp = %s, want %q", raw["csp"], "AZURE") + } + + var targets []map[string]json.RawMessage + if err := json.Unmarshal(raw["targets"], &targets); err != nil { + t.Fatalf("unmarshal targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets len = %d, want 1", len(targets)) + } + got, ok := targets[0]["groupId"] + if !ok { + t.Fatalf("target is missing key %q: %s", "groupId", b) + } + if string(got) != `"group-groups-2"` { + t.Errorf("targets[0].groupId = %s, want %q", got, "group-groups-2") + } +} + +// TestGroupsElevateResponse_DecodesPopulatedResult pins the response side of +// group elevation. Every prior test marshaled a Go struct or decoded a body +// produced from one, so renaming a tag round-tripped trivially. sessionId is +// the ID `grant --group` reports and the only handle for revoking the session +// by ID: if it broke, elevation appears to succeed while grant prints an empty +// session ID and the session can never be revoked. +func TestGroupsElevateResponse_DecodesPopulatedResult(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap between fields cannot be masked. + const wire = `{ + "directoryId": "dir-groupsresp-1", + "csp": "AZURE", + "results": [ + { + "groupId": "group-groupsresp-2", + "sessionId": "sess-groupsresp-3", + "errorInfo": null + } + ] + }` + + var resp GroupsElevateResponse + if err := json.Unmarshal([]byte(wire), &resp); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if resp.DirectoryID != "dir-groupsresp-1" { + t.Errorf("directoryId = %q, want %q", resp.DirectoryID, "dir-groupsresp-1") + } + if resp.CSP != CSPAzure { + t.Errorf("csp = %q, want %q", resp.CSP, CSPAzure) + } + if len(resp.Results) != 1 { + t.Fatalf("results len = %d, want 1", len(resp.Results)) + } + if resp.Results[0].GroupID != "group-groupsresp-2" { + t.Errorf("results[0].groupId = %q, want %q", resp.Results[0].GroupID, "group-groupsresp-2") + } + if resp.Results[0].SessionID != "sess-groupsresp-3" { + t.Errorf("results[0].sessionId = %q, want %q", resp.Results[0].SessionID, "sess-groupsresp-3") + } +} From fa2e5d5345b0bda1734e4f2fd39c52678697b2d7 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:36:57 +0200 Subject: [PATCH 5/7] test(workflows): pin the user-visible response wire tags The workflows models test pinned only the three request bodies while the SCA twin pinned both directions, leaving the keys `grant request get` and `grant request list` render free to be renamed. Form validation metadata remains unpinned and is recorded as a follow-up in the ledger. --- internal/workflows/models/wire_tags_test.go | 113 ++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/internal/workflows/models/wire_tags_test.go b/internal/workflows/models/wire_tags_test.go index 11a82ff..4986f0a 100644 --- a/internal/workflows/models/wire_tags_test.go +++ b/internal/workflows/models/wire_tags_test.go @@ -88,3 +88,116 @@ func TestCancelAccessRequest_JSONTags(t *testing.T) { t.Errorf("cancelReason = %s: %s", raw["cancelReason"], b) } } + +// The response models below were previously unpinned in both directions: every +// test decoded a body produced by marshaling the same Go struct, so a renamed +// tag round-tripped trivially while `grant request get` / `grant request list` +// rendered blanks. These pin the keys that reach the user's screen. The +// validation metadata on form.go is still unpinned — see ledger row WF-16. + +// TestAccessRequest_DecodesPopulatedResponse pins the response keys that +// `grant request get` renders, decoding a literal wire body rather than a +// marshaled struct. +func TestAccessRequest_DecodesPopulatedResponse(t *testing.T) { + t.Parallel() + + // Distinguishable values: no two keys share a value, so a swap cannot be + // masked. Do not "tidy" these. + const wire = `{ + "requestId": "req-decode-1", + "targetCategory": "CLOUD_CONSOLE", + "requestState": "PENDING", + "requestResult": "UNKNOWN", + "requestDetails": {"reason": "reason-decode-2"}, + "requestApprovers": [ + {"approver": {"entityId": "ent-decode-3", "entityName": "approver-decode-4"}, "result": "APPROVED"} + ], + "requester": {"entityId": "ent-decode-5", "entityName": "requester-decode-6"}, + "createdBy": "createdby-decode-7", + "createdAt": "2025-08-12T09:41:00", + "updatedBy": "updatedby-decode-8", + "updatedAt": "2025-08-12T17:41:00" + }` + + var req AccessRequest + if err := json.Unmarshal([]byte(wire), &req); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if req.RequestID != "req-decode-1" { + t.Errorf("requestId = %q, want %q", req.RequestID, "req-decode-1") + } + if req.TargetCategory != "CLOUD_CONSOLE" { + t.Errorf("targetCategory = %q, want %q", req.TargetCategory, "CLOUD_CONSOLE") + } + if req.RequestState != RequestStatePending { + t.Errorf("requestState = %q, want %q", req.RequestState, RequestStatePending) + } + if req.RequestResult != RequestResultUnknown { + t.Errorf("requestResult = %q, want %q", req.RequestResult, RequestResultUnknown) + } + if got := req.DetailString("reason"); got != "reason-decode-2" { + t.Errorf("requestDetails[reason] = %q, want %q", got, "reason-decode-2") + } + if req.Requester == nil { + t.Fatal("requester = nil, want the decoded entity") + } + if req.Requester.EntityName != "requester-decode-6" { + t.Errorf("requester.entityName = %q, want %q", req.Requester.EntityName, "requester-decode-6") + } + if req.Requester.EntityID != "ent-decode-5" { + t.Errorf("requester.entityId = %q, want %q", req.Requester.EntityID, "ent-decode-5") + } + if len(req.RequestApprovers) != 1 { + t.Fatalf("requestApprovers len = %d, want 1", len(req.RequestApprovers)) + } + if req.RequestApprovers[0].Result != RequestResultApproved { + t.Errorf("requestApprovers[0].result = %q, want %q", req.RequestApprovers[0].Result, RequestResultApproved) + } + if req.RequestApprovers[0].Approver.EntityName != "approver-decode-4" { + t.Errorf("requestApprovers[0].approver.entityName = %q, want %q", + req.RequestApprovers[0].Approver.EntityName, "approver-decode-4") + } + if req.CreatedBy != "createdby-decode-7" { + t.Errorf("createdBy = %q, want %q", req.CreatedBy, "createdby-decode-7") + } + if req.CreatedAt != "2025-08-12T09:41:00" { + t.Errorf("createdAt = %q, want %q", req.CreatedAt, "2025-08-12T09:41:00") + } + if req.UpdatedBy != "updatedby-decode-8" { + t.Errorf("updatedBy = %q, want %q", req.UpdatedBy, "updatedby-decode-8") + } + if req.UpdatedAt != "2025-08-12T17:41:00" { + t.Errorf("updatedAt = %q, want %q", req.UpdatedAt, "2025-08-12T17:41:00") + } +} + +// TestListRequestsResponse_DecodesPopulatedPage pins the pagination envelope. +// count and totalCount drive both the pagination loop and what the user is told +// about how many requests exist. +func TestListRequestsResponse_DecodesPopulatedPage(t *testing.T) { + t.Parallel() + + const wire = `{ + "items": [{"requestId": "req-page-1"}, {"requestId": "req-page-2"}], + "count": 2, + "totalCount": 7 + }` + + var page ListRequestsResponse + if err := json.Unmarshal([]byte(wire), &page); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(page.Items) != 2 { + t.Fatalf("items len = %d, want 2", len(page.Items)) + } + if page.Items[0].RequestID != "req-page-1" { + t.Errorf("items[0].requestId = %q, want %q", page.Items[0].RequestID, "req-page-1") + } + if page.Count != 2 { + t.Errorf("count = %d, want 2", page.Count) + } + if page.TotalCount != 7 { + t.Errorf("totalCount = %d, want 7", page.TotalCount) + } +} From e3a727b8f1a0496a85ef6c5966929f00f35abef9 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:37:46 +0200 Subject: [PATCH 6/7] test: queue mock GET responses and drop a tautological config assertion The workflows mock returned the same *http.Response on every Get, so the second page of a paginating test would have read an already-drained Body. Replaced with a per-call queue that errors clearly when exhausted. TestServiceConfig_ReturnsIdsecServiceConfig asserted that ServiceConfig() returns the type its signature already declares; removed from both the workflows and SCA copies. --- internal/sca/service_config_test.go | 8 -------- internal/workflows/logging_client_test.go | 6 +++--- internal/workflows/service_config_test.go | 7 ------- internal/workflows/service_test.go | 15 ++++++++++++--- internal/workflows/wire_contract_test.go | 6 +++--- 5 files changed, 18 insertions(+), 24 deletions(-) diff --git a/internal/sca/service_config_test.go b/internal/sca/service_config_test.go index 63c6518..215b81a 100644 --- a/internal/sca/service_config_test.go +++ b/internal/sca/service_config_test.go @@ -2,8 +2,6 @@ package sca import ( "testing" - - "github.com/cyberark/idsec-sdk-golang/pkg/services" ) func TestServiceConfig_ServiceName(t *testing.T) { @@ -38,9 +36,3 @@ func TestServiceConfig_ActionsConfigurations(t *testing.T) { t.Errorf("expected nil ActionsConfigurations, got %v", config.ActionsConfigurations) } } - -func TestServiceConfig_ReturnsIdsecServiceConfig(t *testing.T) { - config := ServiceConfig() - // Verify it returns the correct SDK type - var _ services.IdsecServiceConfig = config -} diff --git a/internal/workflows/logging_client_test.go b/internal/workflows/logging_client_test.go index ba7ec5b..4cb4cdd 100644 --- a/internal/workflows/logging_client_test.go +++ b/internal/workflows/logging_client_test.go @@ -71,7 +71,7 @@ func TestLoggingClient_Get(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ml := &mockLogger{} - inner := &mockHTTPClient{getResponse: tt.resp, getError: tt.err} + inner := &mockHTTPClient{getResponses: []*http.Response{tt.resp}, getError: tt.err} lc := newLoggingClient(inner, ml) resp, err := lc.Get(t.Context(), tt.route, nil) @@ -231,7 +231,7 @@ func TestLoggingClient_PropagatesInnerError(t *testing.T) { func TestLoggingClient_LogsDuration(t *testing.T) { ml := &mockLogger{} - inner := &mockHTTPClient{getResponse: &http.Response{StatusCode: 200, Header: http.Header{}}} + inner := &mockHTTPClient{getResponses: []*http.Response{{StatusCode: 200, Header: http.Header{}}}} lc := newLoggingClient(inner, ml) _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) @@ -248,7 +248,7 @@ func TestLoggingClient_DebugLogsHeaders(t *testing.T) { h := http.Header{} h.Set("Content-Type", "application/json") h.Set("Authorization", "Bearer eyJhbGci.secret.token") - inner := &mockHTTPClient{getResponse: &http.Response{StatusCode: 200, Header: h}} + inner := &mockHTTPClient{getResponses: []*http.Response{{StatusCode: 200, Header: h}}} lc := newLoggingClient(inner, ml) _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) diff --git a/internal/workflows/service_config_test.go b/internal/workflows/service_config_test.go index d2d0cb4..69610bc 100644 --- a/internal/workflows/service_config_test.go +++ b/internal/workflows/service_config_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/cyberark/idsec-sdk-golang/pkg/common/isp" - "github.com/cyberark/idsec-sdk-golang/pkg/services" ) func TestServiceConfig_ServiceName(t *testing.T) { @@ -40,12 +39,6 @@ func TestServiceConfig_ActionsConfigurations(t *testing.T) { } } -func TestServiceConfig_ReturnsIdsecServiceConfig(t *testing.T) { - config := ServiceConfig() - // Verify it returns the correct SDK type - var _ services.IdsecServiceConfig = config -} - // TestNewAccessRequestService_ResolvesISPAuthenticator pins the authenticator // name the constructor looks up. Asking for anything other than "isp" makes the // constructor fail outright. diff --git a/internal/workflows/service_test.go b/internal/workflows/service_test.go index 5c507dc..150976f 100644 --- a/internal/workflows/service_test.go +++ b/internal/workflows/service_test.go @@ -16,8 +16,12 @@ type mockHTTPClient struct { getFn func(ctx context.Context, route string, params interface{}) (*http.Response, error) postFn func(ctx context.Context, route string, body interface{}) (*http.Response, error) - // Fallbacks used when the corresponding Fn is nil. - getResponse *http.Response + // Fallbacks used when the corresponding Fn is nil. getResponse is consumed + // once per call: ListRequests paginates, and an *http.Response returned a + // second time has an already-drained Body, which surfaces as a confusing + // "failed to decode" instead of a clear assertion failure. Set getResponses + // to queue one response per page. + getResponses []*http.Response getError error postResponse *http.Response postError error @@ -38,7 +42,12 @@ func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface if m.getError != nil { return nil, m.getError } - return m.getResponse, nil + if len(m.getResponses) == 0 { + return nil, errors.New("mockHTTPClient: Get called with no queued response left") + } + resp := m.getResponses[0] + m.getResponses = m.getResponses[1:] + return resp, nil } func (m *mockHTTPClient) Post(ctx context.Context, route string, body interface{}) (*http.Response, error) { diff --git a/internal/workflows/wire_contract_test.go b/internal/workflows/wire_contract_test.go index 6275410..4195396 100644 --- a/internal/workflows/wire_contract_test.go +++ b/internal/workflows/wire_contract_test.go @@ -183,9 +183,9 @@ func TestListRequests_SendsLimit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mock := &mockHTTPClient{ - getResponse: jsonResponse(200, models.ListRequestsResponse{ + getResponses: []*http.Response{jsonResponse(200, models.ListRequestsResponse{ Items: []models.AccessRequest{{RequestID: "id-1"}}, Count: 1, TotalCount: 1, - }), + })}, } svc := NewAccessRequestServiceWithClient(mock) @@ -260,7 +260,7 @@ func TestGetRequest_PropagatesDecodeError(t *testing.T) { // endpoint alongside its route. func TestGetRequestForms_SendsExactParams(t *testing.T) { mock := &mockHTTPClient{ - getResponse: jsonResponse(200, models.RequestFormResponse{}), + getResponses: []*http.Response{jsonResponse(200, models.RequestFormResponse{})}, } svc := NewAccessRequestServiceWithClient(mock) From 04fe58c3fe710426c9e28c53051753117fe22f90 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:38:45 +0200 Subject: [PATCH 7/7] docs(ledger): correct two stale references and record the PR8 wire-contract rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SCA-16 cited elevate.go:28 (the response model); the request model's roleId is at :6. WF-14 cited a test line for the cancel twin, against the preamble's rule that File:line is always the production site — named the test instead. Adds WF-22/WF-23 (list route, submit body), SCA-20..24 (group-elevation tags), WF-24 (workflows response tags, closed) and WF-25 (form validation metadata, recorded as a follow-up). --- docs/mutation-ledger.md | 13 +++++++++++-- internal/workflows/models/wire_tags_test.go | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index c9f7830..4332640 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -116,7 +116,7 @@ premise does not hold). | SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | done | | SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestPaginate_PropagatesDecodeError` | PR8 | done | | SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lived inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Both mutants were subsequently executed and both fail `service_client_test.go:52` (deletion → `X-API-Version = ""`; `"1.0"` → `X-API-Version = "1.0"`) | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | done | -| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | done | +| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:6` (was `:28`, which is `ElevateTargetResult` — the **response** model, killed by the pre-existing `TestElevateResponse_Success`) | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | done | | SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | done | | SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | `_ = sdkclient.DisableTransientRetry` (the assignment form — a bare deletion orphans the import and is a compile kill, not a test kill). Reproduced on **both** services exactly as claimed: `inbound requests = 4, want 1` from `internal/sca/retry_policy_test.go:91` and `internal/workflows/retry_policy_test.go:90` | CONFIRMED | test | `TestNewSCAAccessServiceDisablesTransientRetry` and `TestNewAccessRequestServiceDisablesTransientRetry` (both pre-existing and correctly aimed; PR8 executed them against the mutant) | PR8 | done | | WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | done | @@ -132,7 +132,7 @@ premise does not hold). | WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | done | | WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | done | | WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_ExactRouteAndReason` — one test, shared with WF-14 | PR8 | done | -| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | Folded into `TestFinalizeRequest_ExactRouteAndReason` (same test as WF-13) | PR8 | done | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`TestCancelRequest`), which is the contrast that proves the gap | CONFIRMED | test | Folded into `TestFinalizeRequest_ExactRouteAndReason` (same test as WF-13) | PR8 | done | | WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | done | | WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_SendsLimit` (same test as WF-15; its `defaultPageSize` rows pin the 50) | PR8 | done | | WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | done | @@ -141,6 +141,15 @@ premise does not hold). | WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | | SCA-19 | internal/sca | `internal/sca/service.go:60` | `isp.FromISPAuth(ispAuth, "sca", ...)` → `"WRONG"`. Not in the original audit; found while closing SCA-15. Both retry-policy tests overwrite `client.BaseURL` before issuing a request, so the slug — which is what resolves the live host — was unpinned. **Decision: pin, not won't-fix.** The assertion is one line against the already-constructed client, needs no network, and the failure mode (every request to the wrong host) is total | CONFIRMED | test | `TestNewSCAAccessService_UsesSCAServiceSlug` — asserts `BaseURL == "https://testtenant.sca.example.test"` from the existing fake JWT, **before** any BaseURL swap | PR8 | done | | WF-21 | internal/workflows | `internal/workflows/service.go:56` | `isp.FromISPAuth(ispAuth, "uar", ...)` → `"WRONG"`. The UAR twin of SCA-19, same rationale and same decision | CONFIRMED | test | `TestNewAccessRequestService_UsesUARServiceSlug` | PR8 | done | +| WF-22 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `"/apiZ/workflows/requests"`. The only unpinned route in either service: `TestListRequests_SendsLimit` already recorded `mock.gotRoute` and never asserted it, so `grant request list` could be pointed anywhere and stay green | CONFIRMED | test | `TestListRequests_SendsLimit` — route assertion added alongside the existing limit assertion | PR8 | done | +| WF-23 | internal/workflows | `internal/workflows/service.go:215` | In `SubmitRequest`, forward a stripped copy: `mangled := *req; mangled.RequestDetails = nil; s.httpClient.Post(ctx, "/api/workflows/requests", &mangled)`. `TestSubmitRequest` asserted only `TargetCategory`, so reason, role, target, dates and priority — the entire substance of `grant request submit` — were unasserted at the service boundary. The identical mutation on the two SCA pass-through POSTs dies, because those tests `DeepEqual` the whole body | CONFIRMED | test | `TestSubmitRequest_SendsExactRouteAndBody` — mirrors `TestElevate_SendsExactRouteAndBody`, `DeepEqual` against a populated, all-distinct `RequestDetails` | PR8 | done | +| SCA-20 | internal/sca models | `internal/sca/models/groups.go:20` | `GroupID string \`json:"groupId"\`` → `json:"ZgroupId"` on `GroupsElevateTarget` — the **request** model. Its single field is the whole per-target payload of `POST /api/access/elevate/groups`: mutated, every `grant --group` elevation sends `{"targets":[{"ZgroupId":"..."}]}` and group elevation is broken outright. `TestElevateRequest_JSONTags` pins the cloud twin; there was no groups twin | CONFIRMED | test | `TestGroupsElevateRequest_JSONTags` — asserts the **nested** `groupId` inside `targets`, plus `directoryId` and `csp` | PR8 | done | +| SCA-21 | internal/sca models | `internal/sca/models/groups.go:33` | `SessionID string \`json:"sessionId"\`` → `json:"ZsessionId"` on `GroupsElevateTargetResult`. Elevation still succeeds, but grant reports an empty session ID and the session can never be revoked by ID | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-22 | internal/sca models | `internal/sca/models/groups.go:32` | `GroupID string \`json:"groupId"\`` → `json:"ZgroupId"` on `GroupsElevateTargetResult` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-23 | internal/sca models | `internal/sca/models/groups.go:40` | `DirectoryID string \`json:"directoryId"\`` → `json:"ZdirectoryId"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-24 | internal/sca models | `internal/sca/models/groups.go:41` | `CSP CSP \`json:"csp"\`` → `json:"Zcsp"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| WF-24 | internal/workflows models | `internal/workflows/models/request.go:32`, `:63` (and the rest of `AccessRequest`, `Entity`, `ApproverAction`, `ListRequestsResponse`) | Rename any response tag, e.g. `RequestDetails ... \`json:"requestDetails,omitempty"\`` → `json:"ZrequestDetails,omitempty"`, or `ApproverAction.Result \`json:"result"\`` → `json:"Zresult"`. `internal/workflows/models/wire_tags_test.go` pinned only the three request bodies while the SCA twin pinned both directions; every response test decoded a body marshaled from the same struct, so a rename round-tripped and `grant request get` / `list` would render blanks. Not claimed by the PR — recorded because it was found while closing WF-23 | CONFIRMED | test | `TestAccessRequest_DecodesPopulatedResponse` and `TestListRequestsResponse_DecodesPopulatedPage` — 18 tags verified killed (`requestId`, `targetCategory`, `requestState`, `requestResult`, `requestDetails`, `requestApprovers`, `requester`, `createdBy/At`, `updatedBy/At`, `entityId`, `entityName`, `approver`, `result`, `items`, `count`, `totalCount`) | PR8 | done | +| WF-25 | internal/workflows models | `internal/workflows/models/form.go` (`FormQuestion`, `Validator`) | Rename any form-metadata tag, e.g. `Validator.Regex \`json:"regex,omitempty"\`` → `json:"Zregex,omitempty"`. Deliberately **not** closed in PR8: this is validation metadata for the interactive `grant request submit` form, one step removed from the user-visible output that WF-24 covers, and pinning it well needs a populated-form fixture rather than another tag list. Recorded so the gap is stated rather than silent | CONFIRMED | test | Follow-up: a populated `RequestFormResponse` decode fixture pinning `requestForms`, `requestForm`, `questions`, `key`, `required`, `valueType`, `valueChoices`, `validators` and the `Validator` fields | — | todo | | ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | | ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | | ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | diff --git a/internal/workflows/models/wire_tags_test.go b/internal/workflows/models/wire_tags_test.go index 4986f0a..1e61960 100644 --- a/internal/workflows/models/wire_tags_test.go +++ b/internal/workflows/models/wire_tags_test.go @@ -93,7 +93,7 @@ func TestCancelAccessRequest_JSONTags(t *testing.T) { // test decoded a body produced by marshaling the same Go struct, so a renamed // tag round-tripped trivially while `grant request get` / `grant request list` // rendered blanks. These pin the keys that reach the user's screen. The -// validation metadata on form.go is still unpinned — see ledger row WF-16. +// validation metadata on form.go is still unpinned — see ledger row WF-25. // TestAccessRequest_DecodesPopulatedResponse pins the response keys that // `grant request get` renders, decoding a literal wire body rather than a